Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle
From: Conor Dooley
Date: Fri May 17 2024 - 13:00:19 EST
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@xxxxxxxxxxxxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> 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.
Attachment:
signature.asc
Description: PGP signature