[PATCH] hwmon: (lm90) Fix max6658 sporadic wrong temperature reading

[PATCH] hwmon: (lm90) Fix max6658 sporadic wrong temperature reading

[Boyang Yu]

max6658 may report unrealistically high temperature during
the driver initialization, for which, its overtemp alarm pin
also gets asserted. For certain devices implementing overtemp
protection based on that pin, it may further trigger a reset to
the device. By reproducing the problem, the wrong reading is
found to be coincident with changing the conversion rate.

To mitigate this issue, set the stop bit before changing the
conversion rate and unset it thereafter. After such change, the
wrong reading is not reproduced. As the issue is only found on
max6658, apply this change only to the max6657 kind.

Signed-off-by: Boyang Yu <byu@arista.com>
---
 drivers/hwmon/lm90.c | 54 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index e562a578f20e..de551c51a908 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -567,6 +567,35 @@ static inline int lm90_select_remote_channel(struct i2c_client *client,
 	return 0;
 }
 
+static int max6657_write_convrate(struct i2c_client *client, int val)
+{
+	int err_c = 0;
+	int err;
+	int config_orig, config_stop;
+
+	/* Save config and stop conversion*/
+	config_orig = lm90_read_reg(client, LM90_REG_R_CONFIG1);
+	if (config_orig < 0)
+		return config_orig;
+	config_stop = config_orig | 0x40;
+	if (config_orig != config_stop) {
+		err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+						config_stop);
+		if (err < 0)
+			return err;
+	}
+
+	/* Set conv rate */
+	err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, val);
+
+	/* Revert change to config */
+	if (config_orig != config_stop)
+		err_c = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+						  config_orig);
+
+	return err < 0 ? err : err_c;
+}
+
 /*
  * Set conversion rate.
  * client->update_lock must be held when calling this function (unless we are
@@ -587,7 +616,11 @@ static int lm90_set_convrate(struct i2c_client *client, struct lm90_data *data,
 		if (interval >= update_interval * 3 / 4)
 			break;
 
-	err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, i);
+	if (data->kind == max6657)
+		err = max6657_write_convrate(client, i);
+	else
+		err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, i);
+
 	data->update_interval = DIV_ROUND_CLOSEST(update_interval, 64);
 	return err;
 }
@@ -1593,8 +1626,11 @@ static void lm90_restore_conf(void *_data)
 	struct i2c_client *client = data->client;
 
 	/* Restore initial configuration */
-	i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE,
-				  data->convrate_orig);
+	if (data->kind == max6657)
+		max6657_write_convrate(client, data->convrate_orig);
+	else
+		i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE,
+					  data->convrate_orig);
 	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
 				  data->config_orig);
 }
@@ -1602,6 +1638,7 @@ static void lm90_restore_conf(void *_data)
 static int lm90_init_client(struct i2c_client *client, struct lm90_data *data)
 {
 	int config, convrate;
+	int err;
 
 	convrate = lm90_read_reg(client, LM90_REG_R_CONVRATE);
 	if (convrate < 0)
@@ -1611,12 +1648,21 @@ static int lm90_init_client(struct i2c_client *client, struct lm90_data *data)
 	/*
 	 * Start the conversions.
 	 */
-	lm90_set_convrate(client, data, 500);	/* 500ms; 2Hz conversion rate */
+	/* 500ms; 2Hz conversion rate */
+	if (data->kind != max6657)
+		lm90_set_convrate(client, data, 500);
+
 	config = lm90_read_reg(client, LM90_REG_R_CONFIG1);
 	if (config < 0)
 		return config;
 	data->config_orig = config;
 
+	if (data->kind == max6657) {
+		err = lm90_set_convrate(client, data, 500);
+		if (err < 0)
+			return err;
+	}
+
 	/* Check Temperature Range Select */
 	if (data->kind == adt7461 || data->kind == tmp451) {
 		if (config & 0x04)
-- 
2.17.1

Re: [PATCH] hwmon: (lm90) Fix max6658 sporadic wrong temperature reading

[Guenter Roeck]

Hi,

On Thu, Jun 20, 2019 at 07:25:25PM +0000, Boyang Yu wrote:
> max6658 may report unrealistically high temperature during
> the driver initialization, for which, its overtemp alarm pin
> also gets asserted. For certain devices implementing overtemp
> protection based on that pin, it may further trigger a reset to
> the device. By reproducing the problem, the wrong reading is
> found to be coincident with changing the conversion rate.
> 
> To mitigate this issue, set the stop bit before changing the
> conversion rate and unset it thereafter. After such change, the
> wrong reading is not reproduced. As the issue is only found on
> max6658, apply this change only to the max6657 kind.
> 
> Signed-off-by: Boyang Yu <byu@arista.com>
> ---
>  drivers/hwmon/lm90.c | 54 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 4 deletions(-)
[ ... ]
> 
>  /*
>   * Set conversion rate.
>   * client->update_lock must be held when calling this function (unless we are
> @@ -587,7 +616,11 @@ static int lm90_set_convrate(struct i2c_client *client, struct lm90_data *data,
>  		if (interval >= update_interval * 3 / 4)
>  			break;
>  
> -	err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, i);
> +	if (data->kind == max6657)
> +		err = max6657_write_convrate(client, i);
> +	else
> +		err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, i);
> +
Please name the new function lm90_write_convrate, call it unconditionally,
and check if the chip is affected in the function. Also, please implement
the check with a feature flag - it is quite unlikely that only max6658
is affected, and we'll want an easy method to add support for other chips.

Thanks,
Guenter