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

[Guenter Roeck]

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@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)
> 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                 - 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@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
> > 
>