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

  From: Xing Tong Wu <xingtong.wu@siemens.com>

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.

Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
---
 drivers/hwmon/nct6775-core.c | 2 --
 1 file changed, 2 deletions(-)
  

On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote:
> From: Xing Tong Wu <xingtong.wu@siemens.com>
> 
> 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)
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                - 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@siemens.com>
> ---
>  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
>
  

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@siemens.com>
>>
>> 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@siemens.com>
>> ---
>>  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
>>
  

On Thu, Nov 16, 2023 at 12:07:06AM -0800, Guenter Roeck wrote:
> On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote:
> > From: Xing Tong Wu <xingtong.wu@siemens.com>
> > 
> > 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)
> 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                 - 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.
> 

As a follow-up, the existing code also handles setting _enable to 0
explicitly by selecting manual mode and setting the pwm value to the
maximum. This does not match the chip specification, but is the best
we can do to match ABI expectations.

That also means that we can not reject setting pwm values if
pwm control is disabled (off) since pwm==255 in manual mode
is equivalent to disabling pwm. Yes, that means that setting pwm
to 254 while pwm_enable==0 automatically enables manual mode,
but that can not be helped. We _could_ possibly combine setting
pwm_enable to manual mode with setting the pwm value to something
other than 255 if it is currently set to 25, but that would be
an optimization, not a bug fix.

Guenter

> Guenter
> 
> > 
> > Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
> > ---
> >  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
> > 
>
  

On Thu, Nov 16, 2023 at 04:36:39PM +0800, xingtong.wu wrote:
> 
> 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@siemens.com>
> >>
> >> 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?

We need to be consistent. Yes, it might be arguable that we should
not return 0 if fan control can not be disabled by the chip, but that would
effectively be a behavioral change. We don't know if there is some userspace
program which expects to be able to disable fan control completely (and,
when doing so, setting fan speed to its maximum). Given that, I don't think
it is feasible to change the behavior.

Guenter