Messages in this thread |  | | Date | Fri, 6 Jun 2025 16:57:10 +0200 | | From | Uwe Kleine-König <> | | Subject | Re: New default binding for PWM devices? [Was: Re: [PATCH] dt-bindings: timer: xlnx,xps-timer: Make PWM in example usable] |
| |
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 [unhandled content-type:application/pgp-signature] |  |