[PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable

[PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable

[Uwe Kleine-König]

With #pwm-cells = <0> no usable reference to that PWM can be created.
Even though a xlnx,xps-timer device only provides a single PWM line, Linux
would fail to determine the right (pwmchip, pwmnumber) combination.

Fix the example to use the recommended value 3 for #pwm-cells.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
index b1597db04263..d36cbf0efbd6 100644
--- a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
+++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
@@ -82,7 +82,7 @@ examples:
     };
 
     timer@800f0000 {
-        #pwm-cells = <0>;
+        #pwm-cells = <3>;
         clock-names = "s_axi_aclk";
         clocks = <&zynqmp_clk 71>;
         compatible = "xlnx,xps-timer-1.00.a";

base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
-- 
2.47.2

Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable

[Sean Anderson]

On 5/27/25 13:15, Uwe Kleine-König wrote:
> With #pwm-cells = <0> no usable reference to that PWM can be created.
> Even though a xlnx,xps-timer device only provides a single PWM line, Linux
> would fail to determine the right (pwmchip, pwmnumber) combination.

Well, it's OK if you are programming the PWM from userspace.

> Fix the example to use the recommended value 3 for #pwm-cells.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> index b1597db04263..d36cbf0efbd6 100644
> --- a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> @@ -82,7 +82,7 @@ examples:
>      };
>  
>      timer@800f0000 {
> -        #pwm-cells = <0>;
> +        #pwm-cells = <3>;
>          clock-names = "s_axi_aclk";
>          clocks = <&zynqmp_clk 71>;
>          compatible = "xlnx,xps-timer-1.00.a";
> 
> base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca

Reviewed-by: Sean Anderson <sean.anderson@linux.dev>

Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable

[Krzysztof Kozlowski]

On 27/05/2025 19:15, Uwe Kleine-König wrote:
> With #pwm-cells = <0> no usable reference to that PWM can be created.
> Even though a xlnx,xps-timer device only provides a single PWM line, Linux
> would fail to determine the right (pwmchip, pwmnumber) combination.
> 
> Fix the example to use the recommended value 3 for #pwm-cells.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
And what about the binding itself? It allows any arbitrary value.
Setting it to const=3 would not break the ABI, as long as driver does
not care.

Best regards,
Krzysztof

New default binding for PWM devices? [Was: Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable]

[Uwe Kleine-König]

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

Hello,

On Wed, May 28, 2025 at 09:43:48AM +0200, Krzysztof Kozlowski wrote:
> On 27/05/2025 19:15, Uwe Kleine-König wrote:
> > With #pwm-cells = <0> no usable reference to that PWM can be created.
> > Even though a xlnx,xps-timer device only provides a single PWM line, Linux
> > would fail to determine the right (pwmchip, pwmnumber) combination.
> > 
> > Fix the example to use the recommended value 3 for #pwm-cells.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---
> >  Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> And what about the binding itself? It allows any arbitrary value.
> Setting it to const=3 would not break the ABI, as long as driver does
> not care.

Oh indeed. Now I wonder about myself that I didn't notice that without a
hint.

So with the intention to move all drivers to #pwm-cells = <3>, the patch
to create here is:

diff --git a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
index b1597db04263..8d7a87fb2d35 100644
--- a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
+++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
@@ -26,7 +26,8 @@ properties:
   reg:
     maxItems: 1
 
-  '#pwm-cells': true
+  '#pwm-cells':
+    const: 3
 
   xlnx,count-width:
     $ref: /schemas/types.yaml#/definitions/uint32
@@ -82,7 +83,7 @@ examples:
     };
 
     timer@800f0000 {
-        #pwm-cells = <0>;
+        #pwm-cells = <3>;
         clock-names = "s_axi_aclk";
         clocks = <&zynqmp_clk 71>;
         compatible = "xlnx,xps-timer-1.00.a";

There is however one concern that I want to get resolved first to
prevent churn:

In principle I think it's bad that a phandle to a PWM must contain a
period and flags specifying the polarity. For some use cases the period
might not matter or is implicitly given or more than one period length
is relevant.

So I wonder if instead of unifying all PWM bindings to #pwm-cells = <3>
I should instead go to something like

	mypwm: pwm {
		compatible = "...."
		#pwm-cells = <1>;
	};

	fan {
		compatible = "pwm-fan";
		pwms = <&mypwm 1>;
		assigned-pwms = <&mypwm>;
		assigned-pwm-default-period-lengths-ns = <40000>;
		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
	};

(where the single cell specifies the index of the PWM's output).

I already suggested that in
https://lore.kernel.org/linux-pwm/jmxmxzzfyobuheqe75lj7qcq5rlt625wddb3rlhiernunjdodu@tgxghvfef4tl/.
When I asked about that in #armlinux Rob said "no. We don't need a 2nd
way to set period and flags." Is this still a bad idea if the
traditional binding with 3 cells will be deprecated for all PWM
devices? If this would be ok then, I'm also open for improvements to
the new concept. Maybe something like:

	fan {
		compatible = "pwm-fan";
		pwms = <&mypwm 1>;
		pwm-default-period-lengths-ns = <40000>;
		pwm-default-flags = <PWM_POLARITY_INVERTED>;
	};

?

Looking forward to your feedback.

Best regards
Uwe

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

Re: New default binding for PWM devices? [Was: Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable]

[Rob Herring]

On Tue, Jun 03, 2025 at 07:41:28PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, May 28, 2025 at 09:43:48AM +0200, Krzysztof Kozlowski wrote:
> > On 27/05/2025 19:15, Uwe Kleine-König wrote:
> > > With #pwm-cells = <0> no usable reference to that PWM can be created.
> > > Even though a xlnx,xps-timer device only provides a single PWM line, Linux
> > > would fail to determine the right (pwmchip, pwmnumber) combination.
> > > 
> > > Fix the example to use the recommended value 3 for #pwm-cells.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > ---
> > >  Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > And what about the binding itself? It allows any arbitrary value.
> > Setting it to const=3 would not break the ABI, as long as driver does
> > not care.
> 
> Oh indeed. Now I wonder about myself that I didn't notice that without a
> hint.
> 
> So with the intention to move all drivers to #pwm-cells = <3>, the patch
> to create here is:
> 
> diff --git a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> index b1597db04263..8d7a87fb2d35 100644
> --- a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml
> @@ -26,7 +26,8 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  '#pwm-cells': true
> +  '#pwm-cells':
> +    const: 3
>  
>    xlnx,count-width:
>      $ref: /schemas/types.yaml#/definitions/uint32
> @@ -82,7 +83,7 @@ examples:
>      };
>  
>      timer@800f0000 {
> -        #pwm-cells = <0>;
> +        #pwm-cells = <3>;
>          clock-names = "s_axi_aclk";
>          clocks = <&zynqmp_clk 71>;
>          compatible = "xlnx,xps-timer-1.00.a";
> 
> There is however one concern that I want to get resolved first to
> prevent churn:
> 
> In principle I think it's bad that a phandle to a PWM must contain a
> period and flags specifying the polarity. For some use cases the period
> might not matter or is implicitly given or more than one period length
> is relevant.

Why can't the period be 0 and no flags set if they aren't needed?


> So I wonder if instead of unifying all PWM bindings to #pwm-cells = <3>
> I should instead go to something like
> 
> 	mypwm: pwm {
> 		compatible = "...."
> 		#pwm-cells = <1>;
> 	};
> 
> 	fan {
> 		compatible = "pwm-fan";
> 		pwms = <&mypwm 1>;
> 		assigned-pwms = <&mypwm>;
> 		assigned-pwm-default-period-lengths-ns = <40000>;
> 		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
> 	};
> 
> (where the single cell specifies the index of the PWM's output).

Sigh. You just changed everyone to 3 cells and now you want to change 
again? Changing existing users to 3 was borderline churn. Changing again 
I won't be receptive to. 


> I already suggested that in
> https://lore.kernel.org/linux-pwm/jmxmxzzfyobuheqe75lj7qcq5rlt625wddb3rlhiernunjdodu@tgxghvfef4tl/.
> When I asked about that in #armlinux Rob said "no. We don't need a 2nd
> way to set period and flags." Is this still a bad idea if the
> traditional binding with 3 cells will be deprecated for all PWM
> devices? If this would be ok then, I'm also open for improvements to
> the new concept. Maybe something like:
> 
> 	fan {
> 		compatible = "pwm-fan";
> 		pwms = <&mypwm 1>;
> 		pwm-default-period-lengths-ns = <40000>;
> 		pwm-default-flags = <PWM_POLARITY_INVERTED>;
> 	};
> 
> ?

How is this any different than a slight name change?

What I also said there is that case looked like a property of the fan. 
If you want a default fan speed, then you should express that in fan 
terms (i.e. RPM or %) and then have a table to go from fan speeds to fan 
control settings (i.e. PWM duty cycle in this case). Even if you need 
something like minimum startup duty cycle, that's still a property of 
the fan.

It's the same thing for LEDs. What we ultimately care about is 
brightness, not current or PWM duty cycle. So express things where we 
can derive PWM duty cycle starting from brightness. And that's what the 
bindings do.

Rob

Re: New default binding for PWM devices? [Was: Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable]

[Uwe Kleine-König]

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

Hello Rob,

On Fri, Jun 06, 2025 at 09:13:24AM -0500, Rob Herring wrote:
>    reg:
> >      maxItems: 1
> >  
> > -  '#pwm-cells': true
> > +  '#pwm-cells':
> > +    const: 3
> >  
> >    xlnx,count-width:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> > @@ -82,7 +83,7 @@ examples:
> >      };
> >  
> >      timer@800f0000 {
> > -        #pwm-cells = <0>;
> > +        #pwm-cells = <3>;
> >          clock-names = "s_axi_aclk";
> >          clocks = <&zynqmp_clk 71>;
> >          compatible = "xlnx,xps-timer-1.00.a";
> > 
> > There is however one concern that I want to get resolved first to
> > prevent churn:
> > 
> > In principle I think it's bad that a phandle to a PWM must contain a
> > period and flags specifying the polarity. For some use cases the period
> > might not matter or is implicitly given or more than one period length
> > is relevant.
> 
> Why can't the period be 0 and no flags set if they aren't needed?

I don't say they cannot, and probably that's the most sane option if
there is no fixed default period and flags and we're sticking to 3
cells.

> > So I wonder if instead of unifying all PWM bindings to #pwm-cells = <3>
> > I should instead go to something like
> > 
> > 	mypwm: pwm {
> > 		compatible = "...."
> > 		#pwm-cells = <1>;
> > 	};
> > 
> > 	fan {
> > 		compatible = "pwm-fan";
> > 		pwms = <&mypwm 1>;
> > 		assigned-pwms = <&mypwm>;
> > 		assigned-pwm-default-period-lengths-ns = <40000>;
> > 		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
> > 	};
> > 
> > (where the single cell specifies the index of the PWM's output).
> 
> Sigh. You just changed everyone to 3 cells and now you want to change 
> again?

I did? I admit that I intended to, but before starting to modify the
bindings I thought about if #pwm-cells = <3> is really the best way
forward.

> Changing existing users to 3 was borderline churn. Changing again 
> I won't be receptive to. 

I'm puzzled about what you mean.

There is 2bb369ab50e107a7de6df060a1ece2f33a6a0b9e but this is hardly
churn? And I prepared switching to 3 cells in
895fe4537cc8586f51abb5c66524efaa42c29883 but didn't touch the bindings
yet.
 
> > I already suggested that in
> > https://lore.kernel.org/linux-pwm/jmxmxzzfyobuheqe75lj7qcq5rlt625wddb3rlhiernunjdodu@tgxghvfef4tl/.
> > When I asked about that in #armlinux Rob said "no. We don't need a 2nd
> > way to set period and flags." Is this still a bad idea if the
> > traditional binding with 3 cells will be deprecated for all PWM
> > devices? If this would be ok then, I'm also open for improvements to
> > the new concept. Maybe something like:
> > 
> > 	fan {
> > 		compatible = "pwm-fan";
> > 		pwms = <&mypwm 1>;
> > 		pwm-default-period-lengths-ns = <40000>;
> > 		pwm-default-flags = <PWM_POLARITY_INVERTED>;
> > 	};
> > 
> > ?
> 
> How is this any different than a slight name change?

Compared to the suggestion with assigned-pwms it's mostly just a name
change, but dropping assigned-pwms is a relevant change. Compared to
what we have now (i.e. #pwm-cells = <3> for most bindings) the
specification of flags and period is optional which is IMHO a nicer
design pattern.

> What I also said there is that case looked like a property of the fan. 
> If you want a default fan speed, then you should express that in fan 
> terms (i.e. RPM or %) and then have a table to go from fan speeds to fan 
> control settings (i.e. PWM duty cycle in this case). Even if you need 
> something like minimum startup duty cycle, that's still a property of 
> the fan.

I fully agree and want to fix the #pwm-cells = <4> case. In that context
it's also relevant if the change should go to <3> or <1>.

Best regards
Uwe

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