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

[Rob Herring <robh@kernel.org>]

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