Re: [PATCH v1 1/2] hwmon: (lm90): Add support for NCT7716, NCT7717 and NCT7718

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 1/10/25 00:26, Ming Yu wrote:
...
> @@ -2288,7 +2329,19 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
>   	if (config2 < 0)
>   		return NULL;
>   
> -	if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
> +	if (address == 0x48 && !(config1 & 0x30) && !(config2 & 0xfe) &&

Why config1 & 0x30 (instead of 0x3e) ?

> +	    convrate <= 0x08) {
> +		if (chip_id == 0x90)
> +			name = "nct7717";
> +		else if (chip_id == 0x91)
> +			name = "nct7716";
> +	} else if (address == 0x49 && !(config1 & 0x30) && !(config2 & 0xfe) &&
> +		   convrate <= 0x08) {
> +		name = "nct7716";

Please also check the chip ID, and the other unused configuration register bits.

> +	} else if (address == 0x4c && !(config1 & 0x18) && !(config2 & 0xf8) &&
> +		   convrate <= 0x08) {
> +		name = "nct7718";

Please also check the chip ID (0x90 according to the datasheet). Why not check bit 5
of config1 ?

If there is a reason for not checking the reserved configuration register bits,
please add a comment to the code explaining the reason.

> +	} else if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
>   		if (chip_id == 0x01 && convrate <= 0x09) {
>   			/* W83L771W/G */
>   			name = "w83l771";
> @@ -2297,6 +2350,7 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
>   			name = "w83l771";
>   		}
>   	}
> +
>   	return name;
>   }
>   
> @@ -2484,6 +2538,10 @@ static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info)
>   		name = lm90_detect_maxim(client, common_address, chip_id,
>   					 config1, convrate);
>   		break;
> +	case 0x50:	/* Nuvoton */
> +	case 0x5c:	/* Winbond/Nuvoton */

The new detection code should be implemented as separate function to avoid
weakening the detection mechanism. I would suggest to rename the current
lm90_detect_nuvoton() to lm90_detect_winbond() and introduce a new
lm90_detect_nuvoton(). Alternatively, add something like lm90_detect_nuvoton_50().

Given that all new chips have a chip ID register (called device ID), I would suggest
to arrange the new code around the chip IDs. Since all chips have another chip ID
register at address 0xfd, it would make sense to check that register as well.
That would only require a single check since it looks like the value is the same
for all chips. Something like

	int chid = i2c_smbus_read_byte_data(client, 0xfd);
	...

	if (chid < 0 || config2 < 0)
		return NULL;

	if (chid != 0x50 || convrate > 0x08)
		return NULL;

	switch (chip_id) {
	case 0x90:
		...
	case 0x91:
		...
	default:
		...
	}

Thanks,
Guenter