On 4/6/21 11:16 AM, Konstantin Aladyshev wrote:
> 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 operation.
>
> Signed-off-by: Konstantin Aladyshev <[email protected]>
> ---
> Changes in v2:
> - Fix typo in a commit message
> - Don't swap temp_int/temp_dec checks at the end of the 'sbtsi_read'
> function
>
This doesn't explain the reason for the extra read operation for
limits. Preventing a second read in error cases is not an argument
for adding an extra read for non-error cases.
Guenter
> drivers/hwmon/sbtsi_temp.c | 55 +++++++++++++++++++-------------------
> 1 file changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index e35357c48b8e..4ae48635bb31 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -74,48 +74,47 @@ 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;
> +
> + 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;
>
