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

[Rafał Miłecki]

On 14.02.2024 11:06, Conor Dooley wrote:
> On Wed, Feb 14, 2024 at 10:27:54AM +0100, AngeloGioacchino Del Regno wrote:
> > 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.
>
> > 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.
>
> Re-ordering to make my reply make more sense...
>
> > 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.
>
> Ye, either of those two makes my life a lot easier. I can then at least
> go and check the driver patch to see if things match up. In this case, I
> would want to check that the driver requires changes to support this
> device, given the commit message mentions nothing about the difference
> between this device and others. I'd still probably request that the
> commit message be improved to explain the lack of a fallback, but at
> least I would be clear about what I want and could provide a conditional
> Ack.
>
> If you're not sending the bindings patch with the driver, there's an
> extra onus on you to explain exactly what makes this device incompatible
> with the other devices in the enum, although in an ideal world it'd make
> no difference and every bindings patch would contain that information.

I understand, thanks guys for discussing this with me.

I'll send V2 with Linux driver part.


> > > > 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.
>
> Put this information in your commit message next time :)
>
> Cheers,
> Conor.