[PATCHv1] hwmon: adm9240: handle temperature readings below 0

[PATCHv1] hwmon: adm9240: handle temperature readings below 0

[Chris Packham]

Unlike the temperature thresholds the temperature data is a 9-bit signed
value. This allows and additional 0.5 degrees of precision on the
reading but means we can't rely on sign-extension to handle negative
values.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/hwmon/adm9240.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 98114ce..6929b25 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -107,6 +107,21 @@ static inline s8 TEMP_TO_REG(long val)
 	return clamp_val(SCALE(val, 1, 1000), -40, 127);
 }
 
+/* 8-bit signed temperature (from Thigh, Tlow etc) */
+static inline int TEMP_FROM_REG(s8 val)
+{
+	return val * 1000;
+}
+
+/* 9-bit signed temperature (from temperature data) */
+static inline int TEMP_FROM_DATA_REG(s16 val)
+{
+	if (val > 255)
+		val -= 512;
+
+	return val * 500;
+}
+
 /* two fans, each with low fan speed limit */
 static inline unsigned int FAN_FROM_REG(u8 reg, u8 div)
 {
@@ -263,7 +278,7 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *dummy,
 		char *buf)
 {
 	struct adm9240_data *data = adm9240_update_device(dev);
-	return sprintf(buf, "%d\n", data->temp * 500); /* 9-bit value */
+	return sprintf(buf, "%d\n", TEMP_FROM_DATA_REG(data->temp));
 }
 
 static ssize_t show_max(struct device *dev, struct device_attribute *devattr,
@@ -271,7 +286,7 @@ static ssize_t show_max(struct device *dev, struct device_attribute *devattr,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct adm9240_data *data = adm9240_update_device(dev);
-	return sprintf(buf, "%d\n", data->temp_max[attr->index] * 1000);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_max[attr->index]));
 }
 
 static ssize_t set_max(struct device *dev, struct device_attribute *devattr,
-- 
2.10.0

Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0

[Guenter Roeck]

On Tue, Oct 04, 2016 at 05:08:00PM +1300, Chris Packham wrote:
> Unlike the temperature thresholds the temperature data is a 9-bit signed
> value. This allows and additional 0.5 degrees of precision on the
> reading but means we can't rely on sign-extension to handle negative
> values.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  drivers/hwmon/adm9240.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index 98114ce..6929b25 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -107,6 +107,21 @@ static inline s8 TEMP_TO_REG(long val)
>  	return clamp_val(SCALE(val, 1, 1000), -40, 127);
>  }
>  
> +/* 8-bit signed temperature (from Thigh, Tlow etc) */
> +static inline int TEMP_FROM_REG(s8 val)
> +{
> +	return val * 1000;
> +}

This does not any value; please drop.

> +
> +/* 9-bit signed temperature (from temperature data) */
> +static inline int TEMP_FROM_DATA_REG(s16 val)

Please don't use uppercase for functions.

> +{
> +	if (val > 255)
> +		val -= 512;
> +
> +	return val * 500;

At the surface, this is identical to
	return sign_extend(val, 9) * 500;

However, the real problem is around line 200, where the sign extension is
attempted but fails. It would be better to drop the '/ 128'
there and simplify the above code to

	return val / 128 * 500;

With that, it might be better to just drop the new function and move the
expression into the calling code.

Of course, all that doesn't solve the real problem in this driver, which is
that it ignores error codes from the smbus functions, but that is a different
problem.

Thanks,
Guenter

> +}
> +
>  /* two fans, each with low fan speed limit */
>  static inline unsigned int FAN_FROM_REG(u8 reg, u8 div)
>  {
> @@ -263,7 +278,7 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *dummy,
>  		char *buf)
>  {
>  	struct adm9240_data *data = adm9240_update_device(dev);
> -	return sprintf(buf, "%d\n", data->temp * 500); /* 9-bit value */
> +	return sprintf(buf, "%d\n", TEMP_FROM_DATA_REG(data->temp));
>  }
>  
>  static ssize_t show_max(struct device *dev, struct device_attribute *devattr,
> @@ -271,7 +286,7 @@ static ssize_t show_max(struct device *dev, struct device_attribute *devattr,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct adm9240_data *data = adm9240_update_device(dev);
> -	return sprintf(buf, "%d\n", data->temp_max[attr->index] * 1000);
> +	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_max[attr->index]));
>  }
>  
>  static ssize_t set_max(struct device *dev, struct device_attribute *devattr,
> -- 
> 2.10.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0

[Chris Packham]

On 10/05/2016 09:54 AM, Guenter Roeck wrote:
> On Tue, Oct 04, 2016 at 05:08:00PM +1300, Chris Packham wrote:
>> Unlike the temperature thresholds the temperature data is a 9-bit signed
>> value. This allows and additional 0.5 degrees of precision on the
>> reading but means we can't rely on sign-extension to handle negative
>> values.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>  drivers/hwmon/adm9240.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
>> index 98114ce..6929b25 100644
>> --- a/drivers/hwmon/adm9240.c
>> +++ b/drivers/hwmon/adm9240.c
>> @@ -107,6 +107,21 @@ static inline s8 TEMP_TO_REG(long val)
>>  	return clamp_val(SCALE(val, 1, 1000), -40, 127);
>>  }
>>
>> +/* 8-bit signed temperature (from Thigh, Tlow etc) */
>> +static inline int TEMP_FROM_REG(s8 val)
>> +{
>> +	return val * 1000;
>> +}
>
> This does not any value; please drop.
>

My intention was to make the code handling the 8-bit temperature 
thresholds look the same as that handling the 9-bit temperature reading. 
A moot point if we're moving in a new direction.

>> +
>> +/* 9-bit signed temperature (from temperature data) */
>> +static inline int TEMP_FROM_DATA_REG(s16 val)
>
> Please don't use uppercase for functions.
>

Noted. I was following suit with the existing TEMP_TO_REG above.

>> +{
>> +	if (val > 255)
>> +		val -= 512;
>> +
>> +	return val * 500;
>
> At the surface, this is identical to
> 	return sign_extend(val, 9) * 500;

I didn't know sign_extend() existed, thanks for pointing it out.

> However, the real problem is around line 200, where the sign extension is
> attempted but fails. It would be better to drop the '/ 128'
> there and simplify the above code to
>
> 	return val / 128 * 500;
>
> With that, it might be better to just drop the new function and move the
> expression into the calling code.

I'll give that a go and send a v2.

>
> Of course, all that doesn't solve the real problem in this driver, which is
> that it ignores error codes from the smbus functions, but that is a different
> problem.

Yeah I'd noted that too. This patch is actually a port of changes we've 
made to a custom LM81 driver based on the original adm9240 driver which 
included some error handling. I'll look at bringing more of that code in 
for a future patch.

>
> Thanks,
> Guenter
>
>> +}
>> +
>>  /* two fans, each with low fan speed limit */
>>  static inline unsigned int FAN_FROM_REG(u8 reg, u8 div)
>>  {
>> @@ -263,7 +278,7 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *dummy,
>>  		char *buf)
>>  {
>>  	struct adm9240_data *data = adm9240_update_device(dev);
>> -	return sprintf(buf, "%d\n", data->temp * 500); /* 9-bit value */
>> +	return sprintf(buf, "%d\n", TEMP_FROM_DATA_REG(data->temp));
>>  }
>>
>>  static ssize_t show_max(struct device *dev, struct device_attribute *devattr,
>> @@ -271,7 +286,7 @@ static ssize_t show_max(struct device *dev, struct device_attribute *devattr,
>>  {
>>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>  	struct adm9240_data *data = adm9240_update_device(dev);
>> -	return sprintf(buf, "%d\n", data->temp_max[attr->index] * 1000);
>> +	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_max[attr->index]));
>>  }
>>
>>  static ssize_t set_max(struct device *dev, struct device_attribute *devattr,
>> --
>> 2.10.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0

[Guenter Roeck]

On Tue, Oct 04, 2016 at 09:09:10PM +0000, Chris Packham wrote:
> 
> >
> > Of course, all that doesn't solve the real problem in this driver, which is
> > that it ignores error codes from the smbus functions, but that is a different
> > problem.
> 
> Yeah I'd noted that too. This patch is actually a port of changes we've 
> made to a custom LM81 driver based on the original adm9240 driver which 
> included some error handling. I'll look at bringing more of that code in 
> for a future patch.
> 
Sounds good... 

Thanks,
Guenter

adm9240 error handling (was Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0)

[Chris Packham]

Hi Guenter,

On 10/05/2016 11:10 AM, Guenter Roeck wrote:
> On Tue, Oct 04, 2016 at 09:09:10PM +0000, Chris Packham wrote:
>>
>>>
>>> Of course, all that doesn't solve the real problem in this driver, which is
>>> that it ignores error codes from the smbus functions, but that is a different
>>> problem.
>>
>> Yeah I'd noted that too. This patch is actually a port of changes we've
>> made to a custom LM81 driver based on the original adm9240 driver which
>> included some error handling. I'll look at bringing more of that code in
>> for a future patch.
>>
> Sounds good...
>

So on this. The as-yet unpublished changes we have basically consist of 
changing adm9240_update_device() to do something like this


                         || !data->valid) {

                 for (i = 0; i < 6; i++) { /* read voltages */
-                       data->in[i] = i2c_smbus_read_byte_data(client,
+                       ret = i2c_smbus_read_byte_data(client,
                                         ADM9240_REG_IN(i));
+                       if (ret >= 0)
+                               data->in[i] = ret;

The user is still none the wiser that there was an error but at least we 
haven't just attempted to write a error code to the data. Is this the 
direction you want to head? Are there other hwmon drivers that do 
something saner?

Another attempt we've made is to try to read something innocuous like 
the CONFIG register to check that the i2c bus is in a good state before 
reading the rest of the values.

As you can guess these "fixes" have piled up over time and because 
they're hard to test we haven't been brave enough to remove them.

Any thoughts?

Re: adm9240 error handling (was Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0)

[Guenter Roeck]

On 10/04/2016 10:00 PM, Chris Packham wrote:
> Hi Guenter,
>
> On 10/05/2016 11:10 AM, Guenter Roeck wrote:
>> On Tue, Oct 04, 2016 at 09:09:10PM +0000, Chris Packham wrote:
>>>
>>>>
>>>> Of course, all that doesn't solve the real problem in this driver, which is
>>>> that it ignores error codes from the smbus functions, but that is a different
>>>> problem.
>>>
>>> Yeah I'd noted that too. This patch is actually a port of changes we've
>>> made to a custom LM81 driver based on the original adm9240 driver which
>>> included some error handling. I'll look at bringing more of that code in
>>> for a future patch.
>>>
>> Sounds good...
>>
>
> So on this. The as-yet unpublished changes we have basically consist of
> changing adm9240_update_device() to do something like this
>
>
>                          || !data->valid) {
>
>                  for (i = 0; i < 6; i++) { /* read voltages */
> -                       data->in[i] = i2c_smbus_read_byte_data(client,
> +                       ret = i2c_smbus_read_byte_data(client,
>                                          ADM9240_REG_IN(i));
> +                       if (ret >= 0)
> +                               data->in[i] = ret;
>
> The user is still none the wiser that there was an error but at least we
> haven't just attempted to write a error code to the data. Is this the
> direction you want to head? Are there other hwmon drivers that do
> something saner?
>
> Another attempt we've made is to try to read something innocuous like
> the CONFIG register to check that the i2c bus is in a good state before
> reading the rest of the values.
>
> As you can guess these "fixes" have piled up over time and because
> they're hard to test we haven't been brave enough to remove them.
>
> Any thoughts?
>

The usual approach would be to return an error from adm9240_update_device(),
and report it to the user. Hiding the error and returning stale data doesn't
sound like a good idea.

Another option would be to drop value caching from the driver entirely,
and convert it to use regmap (so only the actually changing registers
would be read again).

Thanks,
Guenter