Hi Guenter,
On Fri, 7 Jun 2013 08:51:20 -0700, Guenter Roeck wrote:
> On a system with both MAX1617 and JC42 sensors, JC42 sensors can be misdetected
> as LM84. Strengthen detection sufficiently enough to avoid this misdetection.
> Also improve detection for ADM1021.
>
> Modeled after chip detection code in sensors-detect command.
Tested on my system and the JC42 chips aren't detected, so it's OK.
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> v2:
> use defines for register addresses
> strengthen detection of ADM1021 as well
> explicitly check for register read errors
> read and check rlo (remote low limit)
> ensure that low limits are not higher than high limits
> explain the various checks
> use typecast to s8 instead of mask with 0x80 to identify and compare
> negative temperatures
>
> drivers/hwmon/adm1021.c | 61 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwmon/adm1021.c b/drivers/hwmon/adm1021.c
> index 08f3762..899178d 100644
> --- a/drivers/hwmon/adm1021.c
> +++ b/drivers/hwmon/adm1021.c
> @@ -344,21 +344,66 @@ static int adm1021_detect(struct i2c_client *client,
> else if (man_id == 0x41) {
> if ((dev_id & 0xF0) == 0x30)
> type_name = "adm1023";
> - else
> + else if ((dev_id & 0xF0) == 0x00)
> type_name = "adm1021";
> + else
> + return -ENODEV;
> } else if (man_id == 0x49)
> type_name = "thmc10";
> else if (man_id == 0x23)
> type_name = "gl523sm";
> else if (man_id == 0x54)
> type_name = "mc1066";
> - /* LM84 Mfr ID in a different place, and it has more unused bits */
> - else if (conv_rate == 0x00
> - && (config & 0x7F) == 0x00
> - && (status & 0xAB) == 0x00)
> - type_name = "lm84";
> - else
> - type_name = "max1617";
> + else {
> + int lte, rte, lhi, rhi, llo, rlo;
> +
> + /* extra checks for LM84 and MAX1617 to avoid misdetections */
> +
> + /*
> + * Both chips don't support man_id and dev_id registers,
> + * but reads must not fail.
> + */
> + if (man_id < 0 || dev_id < 0)
> + return -ENODEV;
dev_id < 0 would ideally be checked before, as you're applying a
bitmask to it earlier in this function. Checking for man_id < 0 earlier
shouldn't hurt either.
> +
> + lte = i2c_smbus_read_byte_data(client, ADM1021_REG_TEMP(0));
> + rte = i2c_smbus_read_byte_data(client, ADM1021_REG_TEMP(1));
> + lhi = i2c_smbus_read_byte_data(client, ADM1021_REG_TOS_R(0));
> + rhi = i2c_smbus_read_byte_data(client, ADM1021_REG_TOS_R(1));
> + llo = i2c_smbus_read_byte_data(client, ADM1021_REG_THYST_R(0));
> + rlo = i2c_smbus_read_byte_data(client, ADM1021_REG_THYST_R(1));
> +
> + /* fail if any of the additional register reads failed */
> + if (llo < 0 || rlo < 0)
> + return -ENODEV;
For performance, you may want to read and check these two registers
first. This saves 4 SMBus transactions if either of these two fail.
> +
> + /* fail if all registers hold the same value */
> + if (lte == rte && lte == lhi && lte == rhi && lte == llo
> + && lte == rlo)
> + return -ENODEV;
> +
> + /*
> + * Fail for negative temperatures and negative high limits.
> + * This check also catches read errors on the tested registers.
> + */
> + if ((s8)lte < 0 || (s8)rte < 0 || (s8)lhi < 0 || (s8)rhi < 0)
Very nice :)
> + return -ENODEV;
> +
> + /*
> + * LM84 Mfr ID is in a different place,
> + * and it has more unused bits.
> + */
> + if (conv_rate == 0x00
> + && (config & 0x7F) == 0x00
> + && (status & 0xAB) == 0x00) {
> + type_name = "lm84";
> + } else {
> + /* fail if low limits are larger than high limits */
> + if ((s8)llo > lhi || (s8)rlo > rhi)
> + return -ENODEV;
> + type_name = "max1617";
> + }
> + }
>
> pr_debug("Detected chip %s at adapter %d, address 0x%02x.\n",
> type_name, i2c_adapter_id(adapter), client->addr);
Looks good overall.
Acked-by: Jean Delvare <khali@xxxxxxxxxxxx>
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
