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

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Jun 28, 2019 at 07:06:36PM +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. Apply this change only to the
> max6657 kind for now, controlled by flag LM90_PAUSE_ON_CONFIG.
> 
> Signed-off-by: Boyang Yu <byu@arista.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/lm90.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index e562a578f20e..bd00d8eac066 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -174,6 +174,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>  #define LM90_HAVE_EMERGENCY_ALARM (1 << 5)/* emergency alarm		*/
>  #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
> +#define LM90_PAUSE_FOR_CONFIG	(1 << 8) /* Pause conversion for config	*/
>  
>  /* LM90 status */
>  #define LM90_STATUS_LTHRM	(1 << 0) /* local THERM limit tripped */
> @@ -367,6 +368,7 @@ static const struct lm90_params lm90_params[] = {
>  		.reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
>  	},
>  	[max6657] = {
> +		.flags = LM90_PAUSE_FOR_CONFIG,
>  		.alert_alarms = 0x7c,
>  		.max_convrate = 8,
>  		.reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
> @@ -567,6 +569,38 @@ static inline int lm90_select_remote_channel(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int lm90_write_convrate(struct i2c_client *client,
> +			       struct lm90_data *data, int val)
> +{
> +	int err;
> +	int config_orig, config_stop;
> +
> +	/* Save config and pause conversion */
> +	if (data->flags & LM90_PAUSE_FOR_CONFIG) {
> +		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 (data->flags & LM90_PAUSE_FOR_CONFIG && config_orig != config_stop)
> +		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> +					  config_orig);
> +
> +	return err;
> +}
> +
>  /*
>   * Set conversion rate.
>   * client->update_lock must be held when calling this function (unless we are
> @@ -587,7 +621,7 @@ 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);
> +	err = lm90_write_convrate(client, data, i);
>  	data->update_interval = DIV_ROUND_CLOSEST(update_interval, 64);
>  	return err;
>  }
> @@ -1593,8 +1627,7 @@ 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);
> +	lm90_write_convrate(client, data, data->convrate_orig);
>  	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
>  				  data->config_orig);
>  }
> @@ -1611,12 +1644,13 @@ 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 */
>  	config = lm90_read_reg(client, LM90_REG_R_CONFIG1);
>  	if (config < 0)
>  		return config;
>  	data->config_orig = config;
>  
> +	lm90_set_convrate(client, data, 500); /* 500ms; 2Hz conversion rate */
> +
>  	/* Check Temperature Range Select */
>  	if (data->kind == adt7461 || data->kind == tmp451) {
>  		if (config & 0x04)