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

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

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

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

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 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

[PATCH 2/2] arm64: dts: mediatek: mt7988: add PWM controller

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Add binding for on-SoC controller that can control up to 8 PWMs.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
index bba97de4fb44..67007626b5cd 100644
--- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only OR MIT
 
+#include <dt-bindings/clock/mediatek,mt7988-clk.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
@@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
 			#interrupt-cells = <3>;
 		};
 
-		clock-controller@10001000 {
+		infracfg: clock-controller@10001000 {
 			compatible = "mediatek,mt7988-infracfg", "syscon";
 			reg = <0 0x10001000 0 0x1000>;
 			#clock-cells = <1>;
@@ -103,6 +104,24 @@ clock-controller@1001e000 {
 			#clock-cells = <1>;
 		};
 
+		pwm@10048000 {
+			compatible = "mediatek,mt7988-pwm";
+			reg = <0 0x10048000 0 0x1000>;
+			clocks = <&infracfg CLK_INFRA_66M_PWM_BCK>,
+				 <&infracfg CLK_INFRA_66M_PWM_HCK>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK1>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK2>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK3>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK4>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK5>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK6>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK7>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK8>;
+			clock-names = "top", "main", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6",
+				      "pwm7","pwm8";
+			#pwm-cells = <2>;
+		};
+
 		clock-controller@11f40000 {
 			compatible = "mediatek,mt7988-xfi-pll";
 			reg = <0 0x11f40000 0 0x1000>;
-- 
2.35.3

Re: [PATCH 2/2] arm64: dts: mediatek: mt7988: add PWM controller

[AngeloGioacchino Del Regno]

Il 13/02/24 17:46, Rafał Miłecki ha scritto:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Add binding for on-SoC controller that can control up to 8 PWMs.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> index bba97de4fb44..67007626b5cd 100644
> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-only OR MIT
>   
> +#include <dt-bindings/clock/mediatek,mt7988-clk.h>
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>   
>   / {
> @@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
>   			#interrupt-cells = <3>;
>   		};
>   
> -		clock-controller@10001000 {
> +		infracfg: clock-controller@10001000 {
>   			compatible = "mediatek,mt7988-infracfg", "syscon";
>   			reg = <0 0x10001000 0 0x1000>;
>   			#clock-cells = <1>;
> @@ -103,6 +104,24 @@ clock-controller@1001e000 {
>   			#clock-cells = <1>;
>   		};
>   
> +		pwm@10048000 {
> +			compatible = "mediatek,mt7988-pwm";

I can't take this unless there's a driver that supports your device.

Regards,
Angelo

> +			reg = <0 0x10048000 0 0x1000>;
> +			clocks = <&infracfg CLK_INFRA_66M_PWM_BCK>,
> +				 <&infracfg CLK_INFRA_66M_PWM_HCK>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK1>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK2>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK3>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK4>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK5>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK6>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK7>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK8>;
> +			clock-names = "top", "main", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6",
> +				      "pwm7","pwm8";
> +			#pwm-cells = <2>;
> +		};
> +
>   		clock-controller@11f40000 {
>   			compatible = "mediatek,mt7988-xfi-pll";
>   			reg = <0 0x11f40000 0 0x1000>;

Re: [PATCH 2/2] arm64: dts: mediatek: mt7988: add PWM controller

[Rafał Miłecki]

On 14.02.2024 10:09, AngeloGioacchino Del Regno wrote:
> Il 13/02/24 17:46, Rafał Miłecki ha scritto:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Add binding for on-SoC controller that can control up to 8 PWMs.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>> index bba97de4fb44..67007626b5cd 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>> @@ -1,5 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +#include <dt-bindings/clock/mediatek,mt7988-clk.h>
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   / {
>> @@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
>>               #interrupt-cells = <3>;
>>           };
>> -        clock-controller@10001000 {
>> +        infracfg: clock-controller@10001000 {
>>               compatible = "mediatek,mt7988-infracfg", "syscon";
>>               reg = <0 0x10001000 0 0x1000>;
>>               #clock-cells = <1>;
>> @@ -103,6 +104,24 @@ clock-controller@1001e000 {
>>               #clock-cells = <1>;
>>           };
>> +        pwm@10048000 {
>> +            compatible = "mediatek,mt7988-pwm";
> 
> I can't take this unless there's a driver that supports your device.

I'd argue you should rather look for a documented binding rather than a
(Linux?) driver. Otherwise you would refuse changes that are not
strictly Linux related. DTS files are meant to describe hardware in a
generic way and not be driven by Linux drivers / design.

Example:
We have bindings for "brcm,bcm6345-timer" and "bcm63138-timer" (see
commit e112f2de151b) and DTS files with those bindings.
There is no Linux driver for that hardware block as there is no need
for it.


In this context I'm explaining binding thing with Conor in discussion
on PATCH 1/1. So stay tuned :)

Re: [PATCH 2/2] arm64: dts: mediatek: mt7988: add PWM controller

[AngeloGioacchino Del Regno]

Il 14/02/24 10:24, Rafał Miłecki ha scritto:
> On 14.02.2024 10:09, AngeloGioacchino Del Regno wrote:
>> Il 13/02/24 17:46, Rafał Miłecki ha scritto:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Add binding for on-SoC controller that can control up to 8 PWMs.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
>>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi 
>>> b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>>> index bba97de4fb44..67007626b5cd 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>>> @@ -1,5 +1,6 @@
>>>   // SPDX-License-Identifier: GPL-2.0-only OR MIT
>>> +#include <dt-bindings/clock/mediatek,mt7988-clk.h>
>>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>   / {
>>> @@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
>>>               #interrupt-cells = <3>;
>>>           };
>>> -        clock-controller@10001000 {
>>> +        infracfg: clock-controller@10001000 {
>>>               compatible = "mediatek,mt7988-infracfg", "syscon";
>>>               reg = <0 0x10001000 0 0x1000>;
>>>               #clock-cells = <1>;
>>> @@ -103,6 +104,24 @@ clock-controller@1001e000 {
>>>               #clock-cells = <1>;
>>>           };
>>> +        pwm@10048000 {
>>> +            compatible = "mediatek,mt7988-pwm";
>>
>> I can't take this unless there's a driver that supports your device.
> 
> I'd argue you should rather look for a documented binding rather than a
> (Linux?) driver. Otherwise you would refuse changes that are not
> strictly Linux related. DTS files are meant to describe hardware in a
> generic way and not be driven by Linux drivers / design.
> 

Of course, devicetree describes hardware - that is pretty much globally known.

As I wrote in the bindings patch, I still anyway want to see the driver part
for this block before deciding if your description of this hardware is correct.

Regards,
Angelo

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

Hello Rafał,

On Tue, Feb 13, 2024 at 05:46:32PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> MT7988 has on-SoC controller that can control up to 8 PWMs.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

please make sure that the email address used for sending the patch
matches the Signed-off-by line.

(It depends on the pickyness of the relevant maintainer if that is a
stopper or not.)

Assuming this patch will go in via a mediatek tree:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Krzysztof Kozlowski]

On 13/02/2024 18:48, Uwe Kleine-König wrote:
> Hello Rafał,
> 
> On Tue, Feb 13, 2024 at 05:46:32PM +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> MT7988 has on-SoC controller that can control up to 8 PWMs.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> 
> please make sure that the email address used for sending the patch
> matches the Signed-off-by line.
> 
> (It depends on the pickyness of the relevant maintainer if that is a
> stopper or not.)

Does not have to... It must match From field which is correct here.
Syntax like:

From: Foo <foo@com>
Signed-off-by: Foo <foo@com>
Signed-off-by: Foo <my-other-email-foo@com>

makes sense only if copyrights change (e.g. change of employers) and/or
one email stop working. How would be the point of having to SoBs for the
same person in other cases?

Best regards,
Krzysztof

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On Wed, Feb 14, 2024 at 10:05:47AM +0100, Krzysztof Kozlowski wrote:
> On 13/02/2024 18:48, Uwe Kleine-König wrote:
> > Hello Rafał,
> > 
> > On Tue, Feb 13, 2024 at 05:46:32PM +0100, Rafał Miłecki wrote:
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> MT7988 has on-SoC controller that can control up to 8 PWMs.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > 
> > please make sure that the email address used for sending the patch
> > matches the Signed-off-by line.
> > 
> > (It depends on the pickyness of the relevant maintainer if that is a
> > stopper or not.)
> 
> Does not have to... It must match From field which is correct here.

Rafał's Signed-off matches the author, but not the sender. Together with
"the Signed-off-by: must always be that of the developer submitting the
patch." (from Documentation/process/submitting-patches.rst) I'd say it's
reasonable to request that there is a sign-off with the email matching
the sender. In my understanding the Sign-off line by the author isn't
really required because the sender can vouch for the author. Of course
this is a somewhat artificial discussion if the sender is the same
person as the author and only the email addresses differ. So this about
the strictness of the applying maintainer.

FTR:

	$  curl -s https://lore.kernel.org/linux-pwm/20240213164633.25447-1-zajec5@gmail.com/raw | grep -E '^(From|Signed-off-by):'
	From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <zajec5@gmail.com>
	From: Rafał Miłecki <rafal@milecki.pl>
	Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

the first From: is the sender, the second the author.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Tue, Feb 13, 2024 at 05:46:32PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.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?

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

Cheers,
Conor.

> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Rafał Miłecki]

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@milecki.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?


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

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@milecki.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.

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

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@milecki.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.

> > 
> > > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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@milecki.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.

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

Hello,

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@milecki.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.
> 
> 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.

Just to put in my 2 ¢:
My preference is to not avoid cross-trees patchsets and put all three
patches in a single series. This combines the advantages of 1. and 2.
Given this happens often enough this is something that the maintainers
are used to handle just fine, so the cross-tree issue isn't problematic
most of the time. The conflicts that sometimes arise with cross-tree
patches aren't bad enough to out-weight having binding, driver and dts
changes all together.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Krzysztof Kozlowski]

On 14/02/2024 07:34, Rafał Miłecki wrote:
> 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@milecki.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?

That was explained many, many times. For cases like driver+bindings+DTS,
bindings go via driver subsystem, so they should be posted with driver
patches.

Feel free to include DTS at the end of series, unless you send to Greg
or netdev.
Better, also required for Greg and netdev, is to send DTS separately
with lore links to the bindings.

Best regards,
Krzysztof