Message-ID: <20240517-pointer-cloning-3889f3d6f744@spud>
Date: Fri, 17 May 2024 18:00:06 +0100
From: Conor Dooley <conor@...nel.org>
To: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
Cc: Guenter Roeck <linux@...ck-us.net>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	"jdelvare@...e.com" <jdelvare@...e.com>,
	"robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial
 duty cycle

On Fri, May 17, 2024 at 01:09:03AM +0000, Chris Packham wrote:
> 
> On 13/05/24 04:58, Guenter Roeck wrote:
> > On 5/10/24 08:51, Chris Packham wrote:
> >>
> >> On 10/05/24 15:36, Guenter Roeck wrote:
> >>> Chris,
> >>>
> >>> On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
> >>>>> On 08/05/2024 23:55, Chris Packham wrote:
> >>>>>> Add documentation for the pwm-initial-duty-cycle and
> >>>>>> pwm-initial-frequency properties. These allow the starting state 
> >>>>>> of the
> >>>>>> PWM outputs to be set to cater for hardware designs where 
> >>>>>> undesirable
> >>>>>> amounts of noise is created by the default hardware state.
> >>>>>>
> >>>>>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> >>>>>> ---
> >>>>>>
> >>>>>> Notes:
> >>>>>>        Changes in v2:
> >>>>>>        - Document 0 as a valid value (leaves hardware as-is)
> >>>>>>
> >>>>>>     .../devicetree/bindings/hwmon/adt7475.yaml    | 27 
> >>>>>> ++++++++++++++++++-
> >>>>>>     1 file changed, 26 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml 
> >>>>>> b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >>>>>> index 051c976ab711..97deda082b4a 100644
> >>>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >>>>>> @@ -51,6 +51,30 @@ properties:
> >>>>>>           enum: [0, 1]
> >>>>>>           default: 1
> >>>>>>     +  adi,pwm-initial-duty-cycle:
> >>>>>> +    description: |
> >>>>>> +      Configures the initial duty cycle for the PWM outputs. The 
> >>>>>> hardware
> >>>>>> +      default is 100% but this may cause unwanted fan noise at 
> >>>>>> startup. Set
> >>>>>> +      this to a value from 0 (0% duty cycle) to 255 (100% duty 
> >>>>>> cycle).
> >>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >>>>>> +    minItems: 3
> >>>>>> +    maxItems: 3
> >>>>>> +    items:
> >>>>>> +      minimum: 0
> >>>>>> +      maximum: 255
> >>>>>> +      default: 255
> >>>>>> +
> >>>>>> +  adi,pwm-initial-frequency:
> >>>>> Frequency usually has some units, so use appropriate unit suffix and
> >>>>> drop $ref.  Maybe that's just target-rpm property?
> >>>>>
> >>>>> But isn't this duplicating previous property? This is fan controller,
> >>>>> not PWM provider (in any case you miss proper $refs to pwm.yaml or
> >>>>> fan-common.yaml), so the only thing you initially want to 
> >>>>> configure is
> >>>>> the fan rotation, not specific PWM waveform. If you you want to
> >>>>> configure specific PWM waveform, then it's a PWM provider... but 
> >>>>> it is
> >>>>> not... Confused.
> >>>> There's two things going on here. There's a PWM duty cycle which is
> >>>> configurable from 0% to 100%. It might be nice if this was 
> >>>> expressed as
> >>>> a percentage instead of 0-255 but I went with the latter because 
> >>>> that's
> >>>> how the sysfs ABI for the duty cycle works.
> >>>>
> >>>> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3)
> >>>> affects how that duty cycle is presented to the fans. So you could 
> >>>> still
> >>>> have a duty cycle of 50% at any frequency. What frequency is best
> >>>> depends on the kind of fans being used. In my particular case the 
> >>>> lower
> >>>> frequencies end up with the fans oscillating annoyingly so I use the
> >>>> highest setting.
> >>>>
> >>> My udnerstanding is that we are supposed to use standard pwm provider
> >>> properties. The property description is provider specicic, so I think
> >>> we can pretty much just make it up.
> >>>
> >>> Essentially you'd first define a pwm provider which defines all the
> >>> pwm parameters needed, such as pwm freqency, default duty cycle,
> >>> and flags such as PWM_POLARITY_INVERTED. You'd then add something like
> >>>
> >>>     pwms = <&pwm index frequency duty_cycle ... flags>;
> >>>
> >>> to the node for each fan, and be done.
> >>>
> >>> That doesn't mean that we would actually have to register the chip
> >>> as pwm provider with the pwm subsystem; all we would have to do is to
> >>> interpret the property values.
> >>
> >> We've already got the pwm-active-state as a separate property so that
> >> might be tricky to deal with, I guess it could be deprecated in favour
> >> of something else. Looking at pwm.yaml and fan-common.yaml I can't quite
> >> see how that'd help here. Were you thinking maybe something like
> >>
> >> pwm: hwmon@2e {
> >>       compatible = "adi,adt7476";
> >>       reg = <0x2e>;
> >>       #pwm-cells = <4>;
> >>       fan-0 {
> >>           pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> >>           pwm-names = "PWM1";
> >>           tach-ch = <0>;
> >>       };
> >>       fan-1 {
> >>           // controlled by pwm 0
> >>           tach-ch = <1>
> >>       };
> >>       fan-0 {
> >>           pwms = <&pwm 2 255 22500 PWM_POLARITY_INVERTED>;
> >>           pwm-names = "PWM3";
> >>           tach-ch <2>;
> >>       };
> >>       fan-1 {
> >>           // controlled by pwm 2
> >>           tach-ch = <3>
> >
> > I think that would have to be
> >
> >     ...
> >     fan-0 {
> >         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> >         tach-ch = <1 2>;
> >     };
> >     fan-1 {
> >         tach-ch = <3>
> >     };
> >     ...
> >
> > Context: pwm-names is optional and does not add value here unless I am 
> > missing
> > something. Also, if I understand the bindings correctly, all 
> > tachometer channels
> > controlled by a single pwm are supposed to be listed in a single node. 
> > With the
> > above, you'd then have fan1, fan2, and fan3 plus pwm1 and pwm3 (pwm2 
> > would be
> > disabled/unused).
> >
> > Code-wise, I think you'd then call
> >
> >     struct of_phandle_args args;
> >     ...
> >     err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", 0, &args)
> >
> > with np pointing to the fan node. This should return the parameters in 
> > 'args'.
> 
> On that point. How would I explain in the bindings that cell 2 is the 
> duty cycle, cell 3 is the frequency and cell 4 is the flags?

In the pwm-cells property in the pwm provider binding . You might want to
order it as <index freq flags duty> as usually that's the ordering done
in most (all?) pwm provider bindings that I have seen.
The pwm bindings I think are really unhelpful though - they all say "see
pwm.yaml for info on the cells in #pwm-cells, but then pwm.yaml has no
information. The information is actually in pwm.text, but the binding
conversion did s/pwm.text/pwm.yaml/ in pwm controller bindings.
I'll send a patch that fixes up pwm.yaml.

> 
> The other complication is that one of the systems I have is x86 so I 
> need to express this with the ACPI Device Properties compatibility code. 
> I think I can figure out the ACPI table stuff but I can't call 
> of_parse_phandle_with_args() directly.
> 
> >
> > However, unless you have a use case, I'd suggest not to implement 
> > support for
> > "multiple fans controlled by single pwm" since that would require extra
> > code and you would not actually be able to test it. A mandatory 1:1 
> > mapping
> > is fine with me. Support for 1:n mapping can be implemented if / when 
> > there
> > is a use case. 
> 
> The system I'm dealing with has exactly that. But we don't adjust the 
> fan RPM directly so I think we're OK (just maybe some comments so people 
> aren't confused by missing fans). The ADT7476 will adjust the PWM duty 
> cycle based on the temperature, the fan RPM is just something we report 
> (and generate an alarm if it goes too low).
> 
> > The same is true for registering the driver with the pwm
> > subsystem - that would only be necessary if anyone ever uses one of the
> > pwm channels for non-fan use.
> 
> Agreed. I won't plumb anything into the pwm subsystem. Although it would 
> be kind of neat to see a LED that changes as the system gets hotter, 
> kind of like an electronic thermochromic crystal.
> 
> >
> > That makes me wonder if we actually need tach-ch in the first place or if
> > something like
> >
> >     fan-0 {
> >         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> >     };
> >     fan-1 {
> >         pwms = <&pwm 1 255 22500 0>;
> >     };
> >     ...
> > would do for this chip. 
> 
> Yeah that'd be fine for me.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists