| Message ID | crk42dsypmbyqk7avldghjq32vslmalfmmouwxzgtdci4agfhz@rkbmxj5z22fx |
|---|---|
| State | New |
| Headers | show |
| Series | New default binding for PWM devices? [Was: Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable] | expand |
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
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
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";