Message-ID: <d1ae914a-26be-437d-ba0b-60aa327c2218@roeck-us.net>
Date: Sun, 5 Jan 2025 08:42:05 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Rob Herring <robh@...nel.org>, Peter Korsgaard <peter@...sgaard.com>
Cc: devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org,
 Jean Delvare <jdelvare@...e.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm
 property

On 1/3/25 11:58, Rob Herring wrote:
> On Fri, Jan 03, 2025 at 11:14:47AM +0100, Peter Korsgaard wrote:
>> The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
>> always be desirable because of noise or power consumption peaks, so add an
>> optional "default-pwm" property that can be used to specify a custom default
>> PWM duty cycle (0..255).
>>
>> This is somewhat similar to target-rpm from fan-common.yaml, but that cannot
>> be used here as target-rpm specifies the target fan speed, whereas this is
>> the default pwm to set when the device is instantiated - And the resulting
>> fan RPM resulting from a given PWM duty cycle is fan dependent.
> 
> I still don't agree. Quoting Guenter:
> 
>> The two values are also orthogonal. The fan rpm is fan dependent.
>> Each fan will require a different pwm value to reach the target speed.
>> Trying to use target-rpm to set a default pwm value would really
>> not make much if any sense.
> 

That is a mis-quote, really. I was talking about target-rpm. The property
to be added here is default-pwm, which is completely different.

> But RPM is ultimately what you care about and is the fan parameter
> that's universal yet independent of the underlying control. RPM is what
> determines noise and power consumption.
> 
> There's 2 cases to consider: you have a tach signal and know the fan RPM
> or you don't know the RPM. If you have a tach signal, we probably
> wouldn't be discussing this because target-rpm would be enough. So I'm
> assuming this is the case and you have no idea what RPM the fan runs at.
> The fan-common.yaml binding is a bit incomplete for this. What you need
> is some map of fan speed to PWM duty cycle as most likely it is not
> linear response. I think there are 2 options here:
> 
> Use the 'cooling-levels' property. Fan "speed" is the index of the
> array. So you just need a 'default-cooling-level' property that's the
> default index.
> 
> The other option is define an array of (fan RPM, PWM duty cycle) tuples.
> Then target-rpm can be used to select the entry. We already have
> something like this with 'gpio-fan,speed-map'.
> 

That won't work. Literally each individual fan (even the same model)
runs at a different rpm for a given pwm value, and I am sure the rpm
for a given pwm input changes depending on fan age and temperature.

Quoting from the NCT6796D-S datasheet, for an example:

"The default duty cycle is 7Fh, or 50% for SYSFANOUT, CPUFANOUT, AUXFANOUT0, AUXFANOUT1,
  AUXFANOUT2, AUXFANOUT3 and AUXFANOUT4.
  Note.The default speed of fan output is specified in registers CR[E0h] ~ CR[E4h] and CR[E7h] of Logical Device B,
  and the AUXFANOUT4 default speed of fan output is specified in register CR[E3h] of Logical Device D.
"

Note that the term "default speed" as used in the datasheet refers to pwm.
Again, from the datasheet:

CR E3h. AUXFAN4 Duty Cycle Register
                 ^^^^^^^^^^^^^^^^^^^

Setting the default value for such a register is what we are talking about here.
No rpm involved, just default pwm values. This is completely independent of rpm.

Guenter

Powered by blists - more mailing lists