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

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

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?

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á