lkml.org 
[lkml]   [2025]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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]
\
 
 \ /
  Last update: 2025-06-06 16:58    [from the cache]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog