Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <b6d0dd45-f159-4518-84fa-8a02cc4e9442@163.com>
Date:   Mon, 20 Nov 2023 11:30:20 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] hwmon: (nct6775) Fix incomplete register array
To:     Guenter Roeck <linux@roeck-us.net>,
        Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     xingtong.wu@siemens.com, tobias.schaffner@siemens.com,
        gerd.haeussler.ext@siemens.com
References: <20231116022330.2696-1-xingtong_wu@163.com>
 <20231116022330.2696-2-xingtong_wu@163.com>
 <a18df29d-2e2e-431b-a486-4fd7898e0771@roeck-us.net>
Content-Language: en-US
From:   "xingtong.wu" <xingtong_wu@163.com>
In-Reply-To: <a18df29d-2e2e-431b-a486-4fd7898e0771@roeck-us.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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;