| Message ID | 20180321092709.22377-1-alvaro.gamez@hazent.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hi Alvaro, On Wed, 21 Mar 2018 10:27:09 +0100, Alvaro Gamez Machado wrote: > An assumption was made that reads of a max6635 LM92_REG_MAN_ID register > would return the last value read from any other of its registers. > > This assumption is not only something that is not documented anywhere, Correct. > but it is also untrue, so this prevented max6635 recognition. You can easily imagine that it must have been true at some point. Do you really believe some developer would put that code in the driver just for fun? > > Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> > --- > > Changes in v2: > Sorry, I seem to be slightly dyslexic and keep confusing max6635 <> max6335 > > > drivers/hwmon/lm92.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c > index 2a91974a10bb..a2c3f3080192 100644 > --- a/drivers/hwmon/lm92.c > +++ b/drivers/hwmon/lm92.c > @@ -272,16 +272,8 @@ static int max6635_check(struct i2c_client *client) > u8 conf; > int i; > > - /* > - * No manufacturer ID register, so a read from this address will > - * always return the last read value. > - */ > temp_low = i2c_smbus_read_word_data(client, LM92_REG_TEMP_LOW); > - if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_low) > - return 0; > temp_high = i2c_smbus_read_word_data(client, LM92_REG_TEMP_HIGH); > - if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_high) > - return 0; > > /* Limits are stored as integer values (signed, 9-bit). */ > if ((temp_low & 0x7f00) || (temp_high & 0x7f00)) The detection was already rather weak for these devices. With this step removed, it is below what I think is acceptable in terms of reliability, as it could easily misdetect another chip and accidentally bind to it. For a driver which uses SMBus word transactions, this could be pretty problematic. So if register LM92_REG_MAN_ID does not contain any useful value on the MAX6635, I would rather drop auto-detection of these devices altogether, and only support them when explicitly instantiated. Thanks,
Hi! On Wed, Mar 21, 2018 at 11:09:44AM +0100, Jean Delvare wrote: > Hi Alvaro, > > On Wed, 21 Mar 2018 10:27:09 +0100, Alvaro Gamez Machado wrote: > > An assumption was made that reads of a max6635 LM92_REG_MAN_ID register > > would return the last value read from any other of its registers. > > > > This assumption is not only something that is not documented anywhere, > > Correct. > > > but it is also untrue, so this prevented max6635 recognition. > > You can easily imagine that it must have been true at some point. Do > you really believe some developer would put that code in the driver > just for fun? > Who knows :) But in the abscense of documentation and being able to test with hardware I own, I can only say what I did say: it prevents max6635 recognition. I don't make any judgment on the intent of the developer that put that there. > > The detection was already rather weak for these devices. With this step > removed, it is below what I think is acceptable in terms of > reliability, as it could easily misdetect another chip and accidentally > bind to it. For a driver which uses SMBus word transactions, this could > be pretty problematic. So if register LM92_REG_MAN_ID does not contain > any useful value on the MAX6635, I would rather drop auto-detection of > these devices altogether, and only support them when explicitly > instantiated. I understand this is not an easy device to test for, but current code with modern chips makes max6635_check useless, so it's in fact as good as if autodetection didn't exist. If you decide to drop auto detection that's fine by me, as at least it will be made clear that this device can't be found automatically. I've spent a few hours trying to guess why the driver won't bind to my sensor, so it not trying to do what it can't do would have been better for my sanity. Best regards!
On 03/21/2018 03:24 AM, Alvaro Gamez Machado wrote: > Hi! > > On Wed, Mar 21, 2018 at 11:09:44AM +0100, Jean Delvare wrote: >> Hi Alvaro, >> >> On Wed, 21 Mar 2018 10:27:09 +0100, Alvaro Gamez Machado wrote: >>> An assumption was made that reads of a max6635 LM92_REG_MAN_ID register >>> would return the last value read from any other of its registers. >>> >>> This assumption is not only something that is not documented anywhere, >> >> Correct. >> >>> but it is also untrue, so this prevented max6635 recognition. >> >> You can easily imagine that it must have been true at some point. Do >> you really believe some developer would put that code in the driver >> just for fun? >> > > Who knows :) But in the abscense of documentation and being able to test > with hardware I own, I can only say what I did say: it prevents max6635 > recognition. I don't make any judgment on the intent of the developer that > put that there. > >> >> The detection was already rather weak for these devices. With this step >> removed, it is below what I think is acceptable in terms of >> reliability, as it could easily misdetect another chip and accidentally >> bind to it. For a driver which uses SMBus word transactions, this could >> be pretty problematic. So if register LM92_REG_MAN_ID does not contain >> any useful value on the MAX6635, I would rather drop auto-detection of >> these devices altogether, and only support them when explicitly >> instantiated. > > I understand this is not an easy device to test for, but current code with > modern chips makes max6635_check useless, so it's in fact as good as if > autodetection didn't exist. > > If you decide to drop auto detection that's fine by me, as at least it will > be made clear that this device can't be found automatically. I've spent a > few hours trying to guess why the driver won't bind to my sensor, so it not > trying to do what it can't do would have been better for my sanity. > Please feel free to submit that patch. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c index 2a91974a10bb..a2c3f3080192 100644 --- a/drivers/hwmon/lm92.c +++ b/drivers/hwmon/lm92.c @@ -272,16 +272,8 @@ static int max6635_check(struct i2c_client *client) u8 conf; int i; - /* - * No manufacturer ID register, so a read from this address will - * always return the last read value. - */ temp_low = i2c_smbus_read_word_data(client, LM92_REG_TEMP_LOW); - if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_low) - return 0; temp_high = i2c_smbus_read_word_data(client, LM92_REG_TEMP_HIGH); - if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_high) - return 0; /* Limits are stored as integer values (signed, 9-bit). */ if ((temp_low & 0x7f00) || (temp_high & 0x7f00))
An assumption was made that reads of a max6635 LM92_REG_MAN_ID register would return the last value read from any other of its registers. This assumption is not only something that is not documented anywhere, but it is also untrue, so this prevented max6635 recognition. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> --- Changes in v2: Sorry, I seem to be slightly dyslexic and keep confusing max6635 <> max6335 drivers/hwmon/lm92.c | 8 -------- 1 file changed, 8 deletions(-)