diff mbox series

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

Message ID 20231116022330.2696-2-xingtong_wu@163.com
State New
Headers
Series *** hwmon: (nct6775) Fix pwm bugs for NCT chips *** |

Commit Message

Xing Tong Wu Nov. 16, 2023, 2:23 a.m. UTC 
  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(-)

Comments

Guenter Roeck Nov. 16, 2023, 9:03 a.m. UTC | #1 
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
Guenter Roeck Nov. 17, 2023, 1:35 a.m. UTC | #2 
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;
Xing Tong Wu Nov. 20, 2023, 3:30 a.m. UTC | #3 
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;
Guenter Roeck Nov. 20, 2023, 2:41 p.m. UTC | #4 
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
diff mbox series

Patch

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;