Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Marek Vasut <marex@denx.de>]

On 11/6/24 2:55 AM, Guenter Roeck wrote:
> On 11/5/24 17:28, Marek Vasut wrote:
>> On 11/6/24 1:34 AM, Guenter Roeck wrote:
>>> On 11/5/24 10:53, Marek Vasut wrote:
>>>> On 11/5/24 3:11 PM, Guenter Roeck wrote:
>>>>> On 11/5/24 05:52, Marek Vasut wrote:
>>>>>> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
>>>>>> to spin up from a dead stop, and can be afterward throttled down to
>>>>>> lower PWM duty cycle. Introduce support for operating such fans which
>>>>>
>>>>> Doesn't this imply that a minimum pwm value is needed as well ?
>>>>
>>>> It depends. For this fan, yes, it does stop at around 8% PWM duty 
>>>> cycle.
>>>>
>>>>> Super-IO chips such as the NCT67xx series typically have two separate
>>>>> registers, one for the pwm start value and one for the minimum pwm 
>>>>> value.
>>>>
>>>> I use plain SoC PWM output to operate the fan. This one needs to be 
>>>> set to higher PWM duty cycle first, to spin up, and can be reduced 
>>>> to lower PWM duty cycle afterward without stopping.
>>>>
>>>
>>> Yes, exactly. That is what many fans require.
>>>
>>>>>> need to start at higher PWM duty cycle first and can slow down next.
>>>>>>
>>>>>> Document two new DT properties, "fan-dead-stop-start-percent" and
>>>>>> "fan-dead-stop-start-usec". The former describes the minimum percent
>>>>>> of fan RPM at which it will surely spin up from dead stop. This value
>>>>>> can be found in the fan datasheet and can be converted to PWM duty
>>>>>> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
>>>>>> time in microseconds for which the fan has to be set to dead stop
>>>>>> start RPM for the fan to surely spin up.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>>> Cc: devicetree@vger.kernel.org
>>>>>> Cc: linux-hwmon@vger.kernel.org
>>>>>> ---
>>>>>>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 ++++++ 
>>>>>> +++++
>>>>>>   1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml 
>>>>>> b/ Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>>> index 4e5abf7580cc6..f1042471b5176 100644
>>>>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>>> @@ -31,6 +31,17 @@ properties:
>>>>>>         it must be self resetting edge interrupts.
>>>>>>       maxItems: 1
>>>>>> +  fan-dead-stop-start-percent:
>>>>>
>>>>> Personally I don't think that "dead-stop" in the property name adds 
>>>>> any value.
>>>>> On the contrary, I think it leads to confusion. I head to read the 
>>>>> description
>>>>> to understand.
>>>>
>>>> The documentation refers to this behavior as a "dead stop" , hence 
>>>> the property name. I can change it to fan-stop-to-start-percent ?
>>>
>>> I do not understand the need for that much complexity in the property 
>>> name,
>>> and I don't think it makes sense to name a property based on a specific
>>> chip documentation. I have seen that before, where different vendors use
>>> different names for the same functionality. That doesn't mean that the
>>> vendor-determined name has to make it into the property name.
>>>
>>> As an example, Nuvoton calls the values "Start-Up Value" and "Stop 
>>> Value".
>>> ITE calls the start value "start PWM value" (and as far as I can see 
>>> doesn't
>>> have a separate stop value). I am sure pretty much every vendor uses a
>>> different description.
>>>
>>> I am personally not a friend of long property names. Having said that,
>>> I'll let you use whatever DT maintainers accept. They may have a 
>>> different
>>> opinion.
>> Do you have a different suggestion for the property name ? Else I'll 
>> just send a V2 .
> 
> 
> fan-start-percent and fan-stop-percent would be good enough for me.
> However, the existing cooling-levels property uses duty cycle values
> from 0 ..255. Using % for the new properties will create an inconsistency.
> It will be up to DT maintainers to decide how the properties should be
> defined.
All right, well ... I sent V2 with what I have in tree now, and let's 
see what happens.