[PATCH 0/3] *** hwmon: (nct6775) Fix pwm bugs for NCT chips ***

[PATCH 0/3] *** hwmon: (nct6775) Fix pwm bugs for NCT chips ***

[Xing Tong Wu]

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

These patches address bugs in the pwm features that do not meet the
specification definition.

Xing Tong Wu (3):
  hwmon: (nct6775) Fix incomplete register array
  hwmon: (nct6775) Fix logic error for PWM enable
  hwmon: (nct6775) Fix fan speed set failure in automatic mode

 drivers/hwmon/nct6775-core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/3] hwmon: (nct6775) Fix incomplete register array

[Xing Tong Wu]

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

The nct6116 specification actually includes 5 PWMs, but only 3
PWMs are present in the array. To address this, the missing 2
PWMs have been added to the array.

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

diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
index d928eb8ae5a3..2111f0cd9787 100644
--- a/drivers/hwmon/nct6775-core.c
+++ b/drivers/hwmon/nct6775-core.c
@@ -769,7 +769,7 @@ static const u16 NCT6106_FAN_PULSE_SHIFT[] = { 0, 2, 4 };
 
 static const u8 NCT6106_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3 };
 static const u8 NCT6106_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04 };
-static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c };
+static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c, 0xd8, 0xd9 };
 static const u16 NCT6106_REG_FAN_MODE[] = { 0x113, 0x123, 0x133 };
 static const u16 NCT6106_REG_TEMP_SOURCE[] = {
 	0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5 };
@@ -3595,7 +3595,7 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data,
 		break;
 	case nct6116:
 		data->in_num = 9;
-		data->pwm_num = 3;
+		data->pwm_num = 5;
 		data->auto_pwm_num = 4;
 		data->temp_fixed_num = 3;
 		data->num_temp_alarms = 3;
-- 
2.25.1

Re: [PATCH 1/3] hwmon: (nct6775) Fix incomplete register array

[Guenter Roeck]

On Thu, Nov 16, 2023 at 10:23:28AM +0800, Xing Tong Wu wrote:
> From: Xing Tong Wu <xingtong.wu@siemens.com>
> 
> The nct6116 specification actually includes 5 PWMs, but only 3
> PWMs are present in the array. To address this, the missing 2
> PWMs have been added to the array.
> 
> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
> ---
>  drivers/hwmon/nct6775-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
> index d928eb8ae5a3..2111f0cd9787 100644
> --- a/drivers/hwmon/nct6775-core.c
> +++ b/drivers/hwmon/nct6775-core.c
> @@ -769,7 +769,7 @@ static const u16 NCT6106_FAN_PULSE_SHIFT[] = { 0, 2, 4 };
>  
>  static const u8 NCT6106_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3 };
>  static const u8 NCT6106_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04 };
> -static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c };
> +static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c, 0xd8, 0xd9 };

I'll have to check the datasheet if this is generic, but at the very least
it is incomplete and REG_PWM_MODE as well as REG_PWM_MODE_MASK would
have to be updated as well. Also, I don't see an update to has_pwm,
meaning the two additional pwm controls won't ever be used/enabled.

Guenter

Re: [PATCH 1/3] hwmon: (nct6775) Fix incomplete register array

[Guenter Roeck]

On 11/15/23 18:23, Xing Tong Wu wrote:
> From: Xing Tong Wu <xingtong.wu@siemens.com>
> 
> The nct6116 specification actually includes 5 PWMs, but only 3
> PWMs are present in the array. To address this, the missing 2
> PWMs have been added to the array.
> 
> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
> ---
>   drivers/hwmon/nct6775-core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
> index d928eb8ae5a3..2111f0cd9787 100644
> --- a/drivers/hwmon/nct6775-core.c
> +++ b/drivers/hwmon/nct6775-core.c
> @@ -769,7 +769,7 @@ static const u16 NCT6106_FAN_PULSE_SHIFT[] = { 0, 2, 4 };
>   
>   static const u8 NCT6106_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3 };
>   static const u8 NCT6106_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04 };
> -static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c };
> +static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c, 0xd8, 0xd9 };

I have no idea where you got the above register addresses from. Looking at
the datasheet, NCT6116 doesn't use those registers at all, and neither does
NCT6106. The PWM registers for NCT6116 are

static const u16 NCT6116_REG_PWM[] = { 0x119, 0x129, 0x139, 0x199, 0x1a9 };

>   static const u16 NCT6106_REG_FAN_MODE[] = { 0x113, 0x123, 0x133 };
>   static const u16 NCT6106_REG_TEMP_SOURCE[] = {
>   	0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5 };
> @@ -3595,7 +3595,7 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data,
>   		break;
>   	case nct6116:
>   		data->in_num = 9;
> -		data->pwm_num = 3;
> +		data->pwm_num = 5;

This does look correct, though.

Guenter

>   		data->auto_pwm_num = 4;
>   		data->temp_fixed_num = 3;
>   		data->num_temp_alarms = 3;

Re: [PATCH 1/3] hwmon: (nct6775) Fix incomplete register array

[xingtong.wu]

On 2023/11/17 09:35, Guenter Roeck wrote:
> On 11/15/23 18:23, Xing Tong Wu wrote:
>> From: Xing Tong Wu <xingtong.wu@siemens.com>
>>
>> The nct6116 specification actually includes 5 PWMs, but only 3
>> PWMs are present in the array. To address this, the missing 2
>> PWMs have been added to the array.
>>
>> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
>> ---
>>   drivers/hwmon/nct6775-core.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
>> index d928eb8ae5a3..2111f0cd9787 100644
>> --- a/drivers/hwmon/nct6775-core.c
>> +++ b/drivers/hwmon/nct6775-core.c
>> @@ -769,7 +769,7 @@ static const u16 NCT6106_FAN_PULSE_SHIFT[] = { 0, 2, 4 };
>>     static const u8 NCT6106_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3 };
>>   static const u8 NCT6106_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04 };
>> -static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c };
>> +static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c, 0xd8, 0xd9 };
> 
> I have no idea where you got the above register addresses from. Looking at
> the datasheet, NCT6116 doesn't use those registers at all, and neither does
> NCT6106. The PWM registers for NCT6116 are
> 

I obtain these registers from the Nuvoton official specification
"NCT6116D_Datasheet_V1_0.pdf". There is a table that describes the access for the
PWM registers and corresponding fans:

Fans: SYSFANOUT, CPUFANOUT, AUXFANOUT0, AUXFANOUT1, AUXFANOUT2
PWM output duty (write): Bank1 Index19 bit[7:0], Bank1 Index29 bit[7:0], Bank1 Index39 bit[7:0], Bank1 Index99 bit[7:0], Bank1 IndexA9 bit[7:0]
Current output value (read): Bank0 Index4A, Bank0 Index4B, Bank0 Index4C, Bank0 IndexD8, Bank0 IndexD9

I have checked the NCT6106-NCT6126 series specification(These documents are not
publicly available, so I cannot share them with you), there are only 3 fans in
NCT6106: SYSFANOUT, CPUFANOUT, AUXFANOU0. However, for NCT6116D-NCT6126D, there
are 2 additional fans: AUXFANOUT1, AUXFANOUT2. The registers for these fans are
the same. I'll add a new array for NCT6116D's PWM read in my new patch.

> static const u16 NCT6116_REG_PWM[] = { 0x119, 0x129, 0x139, 0x199, 0x1a9 };

Therefore, this array is only for writing, we need to add an array of registers for reading.

> 
>>   static const u16 NCT6106_REG_FAN_MODE[] = { 0x113, 0x123, 0x133 };
>>   static const u16 NCT6106_REG_TEMP_SOURCE[] = {
>>       0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5 };
>> @@ -3595,7 +3595,7 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data,
>>           break;
>>       case nct6116:
>>           data->in_num = 9;
>> -        data->pwm_num = 3;
>> +        data->pwm_num = 5;
> 
> This does look correct, though.
> 
> Guenter
> 
>>           data->auto_pwm_num = 4;
>>           data->temp_fixed_num = 3;
>>           data->num_temp_alarms = 3;

Re: [PATCH 1/3] hwmon: (nct6775) Fix incomplete register array

[Guenter Roeck]

On 11/19/23 19:30, xingtong.wu wrote:
> On 2023/11/17 09:35, Guenter Roeck wrote:
>> On 11/15/23 18:23, Xing Tong Wu wrote:
>>> From: Xing Tong Wu <xingtong.wu@siemens.com>
>>>
>>> The nct6116 specification actually includes 5 PWMs, but only 3
>>> PWMs are present in the array. To address this, the missing 2
>>> PWMs have been added to the array.
>>>
>>> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
>>> ---
>>>    drivers/hwmon/nct6775-core.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
>>> index d928eb8ae5a3..2111f0cd9787 100644
>>> --- a/drivers/hwmon/nct6775-core.c
>>> +++ b/drivers/hwmon/nct6775-core.c
>>> @@ -769,7 +769,7 @@ static const u16 NCT6106_FAN_PULSE_SHIFT[] = { 0, 2, 4 };
>>>      static const u8 NCT6106_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3 };
>>>    static const u8 NCT6106_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04 };
>>> -static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c };
>>> +static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c, 0xd8, 0xd9 };
>>
>> I have no idea where you got the above register addresses from. Looking at
>> the datasheet, NCT6116 doesn't use those registers at all, and neither does
>> NCT6106. The PWM registers for NCT6116 are
>>
> 
> I obtain these registers from the Nuvoton official specification
> "NCT6116D_Datasheet_V1_0.pdf". There is a table that describes the access for the
> PWM registers and corresponding fans:
> 
> Fans: SYSFANOUT, CPUFANOUT, AUXFANOUT0, AUXFANOUT1, AUXFANOUT2
> PWM output duty (write): Bank1 Index19 bit[7:0], Bank1 Index29 bit[7:0], Bank1 Index39 bit[7:0], Bank1 Index99 bit[7:0], Bank1 IndexA9 bit[7:0]
> Current output value (read): Bank0 Index4A, Bank0 Index4B, Bank0 Index4C, Bank0 IndexD8, Bank0 IndexD9
> 
> I have checked the NCT6106-NCT6126 series specification(These documents are not
> publicly available, so I cannot share them with you), there are only 3 fans in
> NCT6106: SYSFANOUT, CPUFANOUT, AUXFANOU0. However, for NCT6116D-NCT6126D, there
> are 2 additional fans: AUXFANOUT1, AUXFANOUT2. The registers for these fans are
> the same. I'll add a new array for NCT6116D's PWM read in my new patch.
> 

I wasn't aware of NCT6126D, but I do have datasheets for NCT6102/4/6 and for NCT6112/4/6.

There is no need to add a separate array. This is good as-is since the added registers
are not used for NCT6102/4/6.

>> static const u16 NCT6116_REG_PWM[] = { 0x119, 0x129, 0x139, 0x199, 0x1a9 };
> 
> Therefore, this array is only for writing, we need to add an array of registers for reading.
> 

Ah, good point. I forgot that the read and write registers are different.

Still, you'll need to extend NCT6106_REG_PWM_MODE[] and NCT6106_PWM_MODE_MASK[] as well.
As far as I can see, aux1 and aux2 are always in pwm mode, so the register arrays
need to be extended with ", 0, 0".

Thanks,
Guenter

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

[Xing Tong Wu]

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(-)

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

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

[Guenter Roeck]

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
> 

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

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

[Guenter Roeck]

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

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

[PATCH 3/3] hwmon: (nct6775) Fix fan speed set failure in automatic mode

[Xing Tong Wu]

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

Setting the fan speed is only valid in manual mode; it is not possible
to set the fan's speed in automatic mode.
Return error and show error message when attempting to set the fan
speed in automatic mode.

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

diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
index 575db6cb96e9..3fb9896c61ce 100644
--- a/drivers/hwmon/nct6775-core.c
+++ b/drivers/hwmon/nct6775-core.c
@@ -2551,6 +2551,14 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 	int err;
 	u16 reg;
 
+	if (index == 0 && data->pwm_enable[nr] != manual) {
+		dev_err(dev,
+			"The pwm%d doesn't support manual fan speed control in automatic mode.\n",
+			nr + 1);
+		dev_err(dev, "Please set pwm%d_enable to manual mode.\n", nr + 1);
+		return -EOPNOTSUPP;
+	}
+
 	err = kstrtoul(buf, 10, &val);
 	if (err < 0)
 		return err;
-- 
2.25.1

Re: [PATCH 3/3] hwmon: (nct6775) Fix fan speed set failure in automatic mode

[Guenter Roeck]

On Thu, Nov 16, 2023 at 10:23:30AM +0800, Xing Tong Wu wrote:
> From: Xing Tong Wu <xingtong.wu@siemens.com>
> 
> Setting the fan speed is only valid in manual mode; it is not possible
> to set the fan's speed in automatic mode.
> Return error and show error message when attempting to set the fan
> speed in automatic mode.
> 
> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
> ---
>  drivers/hwmon/nct6775-core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
> index 575db6cb96e9..3fb9896c61ce 100644
> --- a/drivers/hwmon/nct6775-core.c
> +++ b/drivers/hwmon/nct6775-core.c
> @@ -2551,6 +2551,14 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
>  	int err;
>  	u16 reg;
>  
> +	if (index == 0 && data->pwm_enable[nr] != manual) {
> +		dev_err(dev,
> +			"The pwm%d doesn't support manual fan speed control in automatic mode.\n",
> +			nr + 1);
> +		dev_err(dev, "Please set pwm%d_enable to manual mode.\n", nr + 1);

No kernel log messages as reponse to user input, please. This
could and likely would clog the kernel log if, for example, some automated
script tries to write into the register.

Please add a comment describing why this is blocked.

> +		return -EOPNOTSUPP;

Wrong error code. I would suggest -EBUSY; that is what is used in the it87
driver.

Guenter

> +	}
> +
>  	err = kstrtoul(buf, 10, &val);
>  	if (err < 0)
>  		return err;
> -- 
> 2.25.1
>