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

On Thu, Jul 15, 2021 at 10:26:05AM +0000, Sa, Nuno wrote:

quoted

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 [off-list ref]; Jean Delvare
[off-list ref]
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:

quoted

Add the bindings for the tacho signal evaluation parameters which

depend

quoted

on the FAN being used.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../bindings/hwmon/adi,axi-fan-control.yaml          | 12

++++++++++++

quoted

 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

quoted

index 6747b870f297..0481eb34d9f1 100644

--- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-

control.yaml

quoted

+++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-

control.yaml

quoted

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

quoted

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á