Dear Guenter,
Thank you for your comments,
Guenter Roeck <linux@xxxxxxxxxxxx> 於 2025年1月10日 週五 下午11:56寫道：
>
> 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) ?
>
Fix it in the next patch.
> > + 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.
>
Fix it in the next patch.
> > + } 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.
>
Fix it in the next patch.
> > + } 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:
> ...
> }
>
Okay, I will make these modifications in the next patch.
Best regards,
Ming
