Date: Wed, 14 Feb 2024 11:19:10 +0100
From: Rafał Miłecki <zajec5@...il.com>
To: Conor Dooley <conor@...nel.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Matthias Brugger <matthias.bgg@...il.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 John Crispin <john@...ozen.org>, linux-pwm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH 1/2] dt-bindings: pwm: mediatek,mt2712: add compatible for
 MT7988

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@...ecki.pl>
>>>>>
>>>>> 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.

Powered by blists - more mailing lists