Re: [PATCH v2] hwmon: (it87) Add support for IT8625E

[Guenter Roeck <linux@roeck-us.net>]

On 10/22/24 03:13, Frank Crawford wrote:
> On Tue, 2024-10-22 at 17:13 +0800, Ai Chao wrote:
>> Add support for IT8625E on Centerm P410.
>>
>> Signed-off-by: Ai Chao <aichao@kylinos.cn>
>> ---
>> change for v2
>>   - Move IT8625E_DEVID after IT8623E_DEVID
>> ---
>>   drivers/hwmon/it87.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
>> index e233aafa8856..4aeb09f3bfdf 100644
>> --- a/drivers/hwmon/it87.c
>> +++ b/drivers/hwmon/it87.c
>> @@ -15,6 +15,7 @@
>>    *            IT8620E  Super I/O chip w/LPC interface
>>    *            IT8622E  Super I/O chip w/LPC interface
>>    *            IT8623E  Super I/O chip w/LPC interface
>> + *            IT8625E  Super I/O chip w/LPC interface
>>    *            IT8628E  Super I/O chip w/LPC interface
>>    *            IT8705F  Super I/O chip w/LPC interface
>>    *            IT8712F  Super I/O chip w/LPC interface
>> @@ -161,6 +162,7 @@ static inline void superio_exit(int ioreg, bool noexit)
>>   #define IT8620E_DEVID 0x8620
>>   #define IT8622E_DEVID 0x8622
>>   #define IT8623E_DEVID 0x8623
>> +#define IT8625E_DEVID 0x8625
>>   #define IT8628E_DEVID 0x8628
>>   #define IT87952E_DEVID 0x8695
>>   
>> @@ -2782,6 +2784,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>>    case IT8622E_DEVID:
>>    sio_data->type = it8622;
>>    break;
>> + case IT8625E_DEVID:
>>    case IT8628E_DEVID:
>>    sio_data->type = it8628;
>>    break;
> 
> Can I just add that it isn't a good idea to use the same type for
> different chips.  There are some specific differences between the
> chips, which mean that it should have its own entry in
> 
> static const struct it87_devices it87_devices[]
> 
> even if currently they are very similar.
> 

According to the information I have, the ADC voltage is different,
and 8628 supports PECI but 8625 doesn't. Most importantly, 8625
has multiple register banks. There are also some differences in
fan control; 8628 can explicitly turn fans off using register bits.

Just mapping the chip to it8628 may be convenient, but it is not
acceptable.

Guenter