Messages in this thread |  | | From | Chris Packham <> | Subject | Re: [PATCHv1] hwmon: adm9240: handle temperature readings below 0 | Date | Tue, 4 Oct 2016 21:09:10 +0000 |
| |
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 >
|  |