Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

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

On Thu, Jul 15, 2021 at 10:26:05AM +0000, Sa, Nuno wrote:
> > From: Rob Herring <robh@kernel.org>
> > Sent: Monday, July 12, 2021 7:27 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> > Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> > <jdelvare@suse.com>
> > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> > properties
> > 
> > [External]
> > 
> > On Thu, Jul 08, 2021 at 02:01:08PM +0200, Nuno Sá wrote:
> > > Add the bindings for the tacho signal evaluation parameters which
> > depend
> > > on the FAN being used.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  .../bindings/hwmon/adi,axi-fan-control.yaml          | 12
> > ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > control.yaml b/Documentation/devicetree/bindings/hwmon/adi,axi-
> > fan-control.yaml
> > > index 6747b870f297..0481eb34d9f1 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > control.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > control.yaml
> > > @@ -37,6 +37,18 @@ properties:
> > >      $ref: /schemas/types.yaml#/definitions/uint32
> > >      enum: [1, 2, 4]
> > >
> > > +  adi,tacho-25-us:
> > > +    description: Expected tacho signal when the PWM is at 25%.
> > > +
> > > +  adi,tacho-50-us:
> > > +    description: Expected tacho signal when the PWM is at 50%.
> > > +
> > > +  adi,tacho-75-us:
> > > +    description: Expected tacho signal when the PWM is at 75%.
> > > +
> > > +  adi,tacho-100-us:
> > > +    description: Expected tacho signal when the PWM is at 100%.
> > 
> > This looks like it should be common. But having PWM percents in the
> > property names doesn't scale. This is also a property of the fan, not
> > the fan controller.
> 
> Yes, I see that these parameters are definitely a property of the attached
> fan but the evaluation of these timings are very specific to this controller
> (I think). The way it works is that the HW can fully operate without any
> runtime SW configuration. In this case, it will use the values in these
> registers to evaluate the tacho signal coming from the FAN. And the HW
> really uses the evaluation points like this (0, 25%, 50% and 100%). It has
> some predefined values that work for the FAN that was used to develop
> the IP but naturally the evaluation will fail as soon as other FAN is attached
> (resulting in fan fault interrupts). And yes, writing these parameters is 
> already SW configuration but what I mean with "runtime" is after probe :). 
> 

Are you sure you can ever get this stable ? Each fan has its own properties
and tolerances. If you replace a fan in a given system, you might get
different RPM numbers. The RPM will differ widely from system to system
and from fan to fan. Anything that assumes a specific RPM in devicetree
data seems to be quite vulnerable to failures. I have experienced that
recently with a different chip which also tries to correlate RPM and PWM
and fails quite miserably.

In my experience, anything other than minimum fan speed is really a recipe
for instability and sporadic false failures. Even setting a minimum fan speed
is tricky because it depends a lot on the fan.

> So, I honestly do not know how we could name this better... Maybe a
> 'tacho-eval-points-us' array? The question would be the min/max
> elements? Do you have any suggestion for a more generic property?
> 
I am guessing that the "us" refers to the time between pulses from the
fan. I think this is a bad value to start with - anything fan speed
related should really be expressed in RPM, not in time between pulses.

Overall I don't think this should be handled as generic set of properties.
Whatever we come up with as standard set of pwm or fan related properties
should not be an expected correlation between pwm and rpm. Assuming such
a property is needed here (after all, the controller is what it is),
maybe a set of tuples makes sense, such as

	adi,pwm-rpm-map = <
		25, 250,
		50, 500,
		75, 750,
		100, 1000
	>;

though I think that each of those should also include the tolerance
instead of just assuming that a 25% tolerance (as implemented in patch
2/6) would work for all fans.

Guenter

> > There's only so many ways a fan can be controlled and I'm going to
> > keep
> > telling folks to make a common fan binding. There's some start to it,
> > but it needs some work.
> 
> You mean the pwm-fan.txt? I gave a look to the driver and I don't think
> it fully fits this controller. I'm ok in sending an fan.yaml binding if you
> prefer it but I'm just not sure what we would need there... Maybe
> pulses-per-revolution and the above property
> (whatever the decided name) would be a starting point? 
> 
> - Nuno Sá