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

[AngeloGioacchino Del Regno]

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