Re: [PATCH] hwmon: (sbtsi) Don't read sensor more than once if it doesn't respond

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

On 4/1/21 2:45 PM, Konstantin Aladyshev wrote:
> From: Konstantin Aladyshev <aladyshev@nicevt.ru>
> 
> SBTSI sensors don't work when the CPU is off.
> In this case every 'i2c_smbus_read_byte_data' function would fail
> by a timeout.
> Currently temp1_max/temp1_min file reads can cause two such timeouts
> for every read.
> Restructure code so there will be no more than one timeout for every
> read opeartion.
> 

operation

> Signed-off-by: Konstantin Aladyshev <aladyshev@nicevt.ru>
> ---
>  drivers/hwmon/sbtsi_temp.c | 59 +++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index e35357c48b8e..e09a8cf6de45 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -74,53 +74,52 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
>  		      u32 attr, int channel, long *val)
>  {
>  	struct sbtsi_data *data = dev_get_drvdata(dev);
> +	u8 temp_int_reg, temp_dec_reg;
>  	s32 temp_int, temp_dec;
>  	int err;
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> -		/*
> -		 * ReadOrder bit specifies the reading order of integer and
> -		 * decimal part of CPU temp for atomic reads. If bit == 0,
> -		 * reading integer part triggers latching of the decimal part,
> -		 * so integer part should be read first. If bit == 1, read
> -		 * order should be reversed.
> -		 */
> -		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> -		if (err < 0)
> -			return err;
> -
> -		mutex_lock(&data->lock);
> -		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> -			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
> -			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
> -		} else {
> -			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
> -			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
> -		}
> -		mutex_unlock(&data->lock);
> +		temp_int_reg = SBTSI_REG_TEMP_INT;
> +		temp_dec_reg = SBTSI_REG_TEMP_DEC;
>  		break;
>  	case hwmon_temp_max:
> -		mutex_lock(&data->lock);
> -		temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_HIGH_INT);
> -		temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_HIGH_DEC);
> -		mutex_unlock(&data->lock);
> +		temp_int_reg = SBTSI_REG_TEMP_HIGH_INT;
> +		temp_dec_reg = SBTSI_REG_TEMP_HIGH_DEC;
>  		break;
>  	case hwmon_temp_min:
> -		mutex_lock(&data->lock);
> -		temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_LOW_INT);
> -		temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_LOW_DEC);
> -		mutex_unlock(&data->lock);
> +		temp_int_reg = SBTSI_REG_TEMP_LOW_INT;
> +		temp_dec_reg = SBTSI_REG_TEMP_LOW_DEC;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * ReadOrder bit specifies the reading order of integer and
> +	 * decimal part of CPU temp for atomic reads. If bit == 0,
> +	 * reading integer part triggers latching of the decimal part,
> +	 * so integer part should be read first. If bit == 1, read
> +	 * order should be reversed.
> +	 */
> +	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> +	if (err < 0)
> +		return err;
> +

It seems the "fix" is to always execute above code, which presumably
fails if the CPU is off. The downside of this approach is that it forces
an unnecessary extra and unnecessary i2c operation when reading
the limits.

The real fix would be to check for error after each i2c operation,
not only after both operations are complete.

> +	mutex_lock(&data->lock);
> +	if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> +		temp_dec = i2c_smbus_read_byte_data(data->client, temp_dec_reg);
> +		temp_int = i2c_smbus_read_byte_data(data->client, temp_int_reg);
> +	} else {
> +		temp_int = i2c_smbus_read_byte_data(data->client, temp_int_reg);
> +		temp_dec = i2c_smbus_read_byte_data(data->client, temp_dec_reg);
> +	}
> +	mutex_unlock(&data->lock);
>  
> -	if (temp_int < 0)
> -		return temp_int;
>  	if (temp_dec < 0)
>  		return temp_dec;
> +	if (temp_int < 0)
> +		return temp_int;

I don't see a value in swapping the checks.

Guenter

>  
>  	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
>  
>