| Message ID | 20240630202028.1874325-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (jc42) Strengthen detect function | expand |
Hi Guenter, On Sun, 30 Jun 2024 13:20:28 -0700, Guenter Roeck wrote: > Configuration register bit 5 must read 0 for all JC42.4 compliant chips. > Several capability register bits must be set for all TSE2004 compliant > chips. Use that information to strengthen the detect function. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/jc42.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c > index 1180af1b1638..a260cff750a5 100644 > --- a/drivers/hwmon/jc42.c > +++ b/drivers/hwmon/jc42.c > @@ -413,7 +413,11 @@ static int jc42_detect(struct i2c_client *client, struct i2c_board_info *info) > if (cap < 0 || config < 0 || manid < 0 || devid < 0) > return -ENODEV; > > - if ((cap & 0xff00) || (config & 0xf800)) > + if ((cap & 0xff00) || (config & 0xf820)) > + return -ENODEV; > + > + if ((devid & TSE2004_DEVID_MASK) == TSE2004_DEVID && > + (cap & 0x00e7) != 0x00e7) > return -ENODEV; > > for (i = 0; i < ARRAY_SIZE(jc42_chips); i++) { I have a user reporting that this change is causing the jc42 driver to no longer bind to his memory module temperature sensor devices after updating to kernel v6.11. I asked for a register dump: 0,8 1,9 2,a 3,b 4,c 5,d 6,e 7,f 00: 7f00 0000 0000 0000 0000 6ac2 091b 3022 After swapping the bytes, I see that this is a TSE2004-compliant device (devid = 0x2230) and the capabilities register reads 0x007f. This doesn't pass the 0x00e7 mask test you added, as bit 7 isn't set in his case. The JEDEC standard indeed says that bit 7 should be set, but apparently this isn't always the case in the real world. Also note that I looked at the Renesas TSE2004GB2B0 datasheet and it shows bit 2 (RANGE) as not always set. The ST STTS2004 datasheet shows bits 0 (EVENT) and 2 (RANGE) as possibly reading 0. So I wonder how much we can rely on these capability bits being set in the detect function. Unfortunately I don't have any TS2004-compliant device at hand to verify, nor do I own register dumps of such devices. Would it be OK with you if we relax the check to at least ignore bit 7, and possibly also bits 0 and 2? Thanks,
On 10/14/24 05:12, Jean Delvare wrote: > Hi Guenter, > > On Sun, 30 Jun 2024 13:20:28 -0700, Guenter Roeck wrote: >> Configuration register bit 5 must read 0 for all JC42.4 compliant chips. >> Several capability register bits must be set for all TSE2004 compliant >> chips. Use that information to strengthen the detect function. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> drivers/hwmon/jc42.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c >> index 1180af1b1638..a260cff750a5 100644 >> --- a/drivers/hwmon/jc42.c >> +++ b/drivers/hwmon/jc42.c >> @@ -413,7 +413,11 @@ static int jc42_detect(struct i2c_client *client, struct i2c_board_info *info) >> if (cap < 0 || config < 0 || manid < 0 || devid < 0) >> return -ENODEV; >> >> - if ((cap & 0xff00) || (config & 0xf800)) >> + if ((cap & 0xff00) || (config & 0xf820)) >> + return -ENODEV; >> + >> + if ((devid & TSE2004_DEVID_MASK) == TSE2004_DEVID && >> + (cap & 0x00e7) != 0x00e7) >> return -ENODEV; >> >> for (i = 0; i < ARRAY_SIZE(jc42_chips); i++) { > > I have a user reporting that this change is causing the jc42 driver to > no longer bind to his memory module temperature sensor devices after > updating to kernel v6.11. I asked for a register dump: > > 0,8 1,9 2,a 3,b 4,c 5,d 6,e 7,f > 00: 7f00 0000 0000 0000 0000 6ac2 091b 3022 > > After swapping the bytes, I see that this is a TSE2004-compliant device > (devid = 0x2230) and the capabilities register reads 0x007f. This > doesn't pass the 0x00e7 mask test you added, as bit 7 isn't set in his > case. > > The JEDEC standard indeed says that bit 7 should be set, but apparently > this isn't always the case in the real world. > > Also note that I looked at the Renesas TSE2004GB2B0 datasheet and it > shows bit 2 (RANGE) as not always set. The ST STTS2004 datasheet shows > bits 0 (EVENT) and 2 (RANGE) as possibly reading 0. So I wonder how > much we can rely on these capability bits being set in the detect > function. Unfortunately I don't have any TS2004-compliant device at > hand to verify, nor do I own register dumps of such devices. Would it > be OK with you if we relax the check to at least ignore bit 7, and > possibly also bits 0 and 2? > Sigh. I guess it would have been a miracle if vendors followed the standard. Let's ignore all three bits, with explanation. Care to send a patch, or do you want me to do it ? Thanks, Guenter
On Mon, 14 Oct 2024 06:52:45 -0700, Guenter Roeck wrote: > On 10/14/24 05:12, Jean Delvare wrote: > > I have a user reporting that this change is causing the jc42 driver to > > no longer bind to his memory module temperature sensor devices after > > updating to kernel v6.11. I asked for a register dump: > > > > 0,8 1,9 2,a 3,b 4,c 5,d 6,e 7,f > > 00: 7f00 0000 0000 0000 0000 6ac2 091b 3022 > > > > After swapping the bytes, I see that this is a TSE2004-compliant device > > (devid = 0x2230) and the capabilities register reads 0x007f. This > > doesn't pass the 0x00e7 mask test you added, as bit 7 isn't set in his > > case. > > > > The JEDEC standard indeed says that bit 7 should be set, but apparently > > this isn't always the case in the real world. > > > > Also note that I looked at the Renesas TSE2004GB2B0 datasheet and it > > shows bit 2 (RANGE) as not always set. The ST STTS2004 datasheet shows > > bits 0 (EVENT) and 2 (RANGE) as possibly reading 0. So I wonder how > > much we can rely on these capability bits being set in the detect > > function. Unfortunately I don't have any TS2004-compliant device at > > hand to verify, nor do I own register dumps of such devices. Would it > > be OK with you if we relax the check to at least ignore bit 7, and > > possibly also bits 0 and 2? > > Sigh. I guess it would have been a miracle if vendors followed the standard. > Let's ignore all three bits, with explanation. Care to send a patch, or do > you want me to do it ? Working on it, I'll submit it later today. Thanks,
diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c index 1180af1b1638..a260cff750a5 100644 --- a/drivers/hwmon/jc42.c +++ b/drivers/hwmon/jc42.c @@ -413,7 +413,11 @@ static int jc42_detect(struct i2c_client *client, struct i2c_board_info *info) if (cap < 0 || config < 0 || manid < 0 || devid < 0) return -ENODEV; - if ((cap & 0xff00) || (config & 0xf800)) + if ((cap & 0xff00) || (config & 0xf820)) + return -ENODEV; + + if ((devid & TSE2004_DEVID_MASK) == TSE2004_DEVID && + (cap & 0x00e7) != 0x00e7) return -ENODEV; for (i = 0; i < ARRAY_SIZE(jc42_chips); i++) {
Configuration register bit 5 must read 0 for all JC42.4 compliant chips. Several capability register bits must be set for all TSE2004 compliant chips. Use that information to strengthen the detect function. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/hwmon/jc42.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)