Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Guenter Roeck <linux@roeck-us.net>]

On Thu, May 09, 2024 at 09:06:49AM +0200, 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@alliedtelesis.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?
> 
We are talking pwm here, not rpm.

> 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.
> 

As I have said before ... almost all fan controllers have pwm outputs to
control the fans, because that is how fans are controlled. So, in your
terminology, pretty much all fan controllers are also pwm providers.

At the same time, I resist the push to implement pwm controller code in
fan controller drivers because that would just add a lot of code for no good
reason other than "because". I guess we'll have to find a means to extract
pwm related configuration data such as this one from devicetree without
actually implementing a full blown pwm controller driver.

Guenter