Re: [PATCH 2/3] hwmon: (nct6775) Fix logic error for PWM enable

[xingtong.wu]

On 2023/11/16 16:07, Guenter Roeck wrote:
> On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote:
>> From: Xing Tong Wu <xingtong.wu@xxxxxxxxxxx>
>>
>> The determination of the "pwm_enable" should be based solely on the mode,
>> regardless of the pwm value.
>> According to the specification, the default values for pwm and pwm_enable
>> are 255 and 0 respectively. However, there is a bug in the code where the
>> fan control is actually enabled, but the file "pwm_enable" incorrectly
>> displays "off", indicating that fan control is disabled. This contradiction
>> needs to be addressed and resolved.
>> Solution: Update the logic so that "pwm_enable" is determined by mode + 1,
>> remove the "off" value for "pwm_enable" since it is not specified in the
>> documentation.
> 
> The chip specification is irrelevant. What is relevant is the hwmon ABI,
> which says
> 
> What:           /sys/class/hwmon/hwmonX/pwmY_enable
> Description:
>                 Fan speed control method:
> 
>                 - 0: no fan speed control (i.e. fan at full speed)
> 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think this description may lead to misunderstanding. There are certain
fans that cannot be controlled and operate at full speed while system is
running. However, there are also fans whose speed can be controlled, even
if they are currently set to full speed. In this particular situation, it
would be better to inform the user that the fan can still be controlled
despite being at full speed.
How do you think that?

>                 - 1: manual fan speed control enabled (using `pwmY`)
>                 - 2+: automatic fan speed control enabled
> 
> which is what the code currently implements or at least tries to
> implement.
> 
> Guenter
> 
>>
>> Signed-off-by: Xing Tong Wu <xingtong.wu@xxxxxxxxxxx>
>> ---
>>  drivers/hwmon/nct6775-core.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
>> index 2111f0cd9787..575db6cb96e9 100644
>> --- a/drivers/hwmon/nct6775-core.c
>> +++ b/drivers/hwmon/nct6775-core.c
>> @@ -900,8 +900,6 @@ static const u16 NCT6116_REG_TSI_TEMP[] = { 0x59, 0x5b };
>>  
>>  static enum pwm_enable reg_to_pwm_enable(int pwm, int mode)
>>  {
>> -	if (mode == 0 && pwm == 255)
>> -		return off;
>>  	return mode + 1;
>>  }
>>  
>> -- 
>> 2.25.1
>>