Re: [PATCH 1/2] dt-bindings: pwm: mediatek,mt2712: add compatible for MT7988

From: AngeloGioacchino Del Regno
Date: Wed Feb 14 2024 - 04:28:35 EST


Il 14/02/24 07:34, Rafał Miłecki ha scritto:
On 13.02.2024 19:18, Conor Dooley wrote:
On Tue, Feb 13, 2024 at 05:46:32PM +0100, Rafał Miłecki wrote:
From: Rafał Miłecki <rafal@xxxxxxxxxx>

MT7988 has on-SoC controller that can control up to 8 PWMs.

I see a binding and a dts patch, but no driver patch, how come?

I believe that to avoid cross-trees patchsets (which are sometimes
tricky for maintainers) there are two ways of submiting such changes:
1. dt-binding + driver; then (separately) DTS
2. dt-binding + DTS; then (separately) driver

I chose later in this case as my personal priority right now is to deal
with all MediaTek DTS files.

Is that wrong or unacceptable?


It's not wrong but it's partially unacceptable, at least on my side.

In my opinion (and I believe many do agree with me), sending the binding along
with the driver is the right choice, and if you also want to include the dts
that is also appreciated: series can go through multiple maintainers applying
subsets - it's ok to do.

I want to put emphasis on sending the binding with the driver, as this allows
for a better review on everyone's side because we do see the full picture and
we can give better advices: in this case, I'm not sure whether adding a new
compatible for MT7988 in an enum is a good idea, as the compatible string may
be shared with one of the *eleven* SoCs that are supported in the PWM driver,
meaning that (hardware speaking!) the PWM controller in 7988 might be the same
as the one in mt1234.

Thanks for the great work that you're doing on the bindings btw.
Keep it up!
Angelo


Also, what makes this incompatibly different with the other devices in
the binding, like the 8183?

It can control 8 PWMs unlike any other SoC block except for MT2712.
It uses different registers than MT2712 thought.


Cheers,
Conor.


Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
---
  Documentation/devicetree/bindings/pwm/mediatek,mt2712-pwm.yaml | 1 +
  1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pwm/mediatek,mt2712-pwm.yaml b/Documentation/devicetree/bindings/pwm/mediatek,mt2712-pwm.yaml
index 0fbe8a6469eb..a5c308801619 100644
--- a/Documentation/devicetree/bindings/pwm/mediatek,mt2712-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/mediatek,mt2712-pwm.yaml
@@ -24,6 +24,7 @@ properties:
            - mediatek,mt7629-pwm
            - mediatek,mt7981-pwm
            - mediatek,mt7986-pwm
+          - mediatek,mt7988-pwm
            - mediatek,mt8183-pwm
            - mediatek,mt8365-pwm
            - mediatek,mt8516-pwm
--
2.35.3