* [lm-sensors] [PATCH v3 2/3] hwmon: (lm95241) Fix negative
@ 2011-07-07 20:42 Guenter Roeck
2011-07-09 20:36 ` Jean Delvare
2011-07-09 21:17 ` Guenter Roeck
0 siblings, 2 replies; 3+ messages in thread
From: Guenter Roeck @ 2011-07-07 20:42 UTC (permalink / raw)
To: lm-sensors
Negative temperatures were returned in degrees C instead of milli-Degrees C.
Also, negative temperatures were reported for remote temperature sensors even
if the chip was configured for positive-only results.
Fix by detecting temperature modes, and by treating negative temperatures
similar to positive temperatures, with appropriate sign extension.
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
Candidate for -stable.
drivers/hwmon/lm95241.c | 20 ++++++++++++++------
1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/lm95241.c b/drivers/hwmon/lm95241.c
index 01c638e..ec02e30 100644
--- a/drivers/hwmon/lm95241.c
+++ b/drivers/hwmon/lm95241.c
@@ -98,11 +98,16 @@ struct lm95241_data {
};
/* Conversions */
-static int TempFromReg(u8 val_h, u8 val_l)
+static int temp_from_reg_signed(u8 val_h, u8 val_l)
{
- if (val_h & 0x80)
- return val_h - 0x100;
- return val_h * 1000 + val_l * 1000 / 256;
+ s16 val_hl = (val_h << 8) | val_l;
+ return val_hl * 1000 / 256;
+}
+
+static int temp_from_reg_unsigned(u8 val_h, u8 val_l)
+{
+ u16 val_hl = (val_h << 8) | val_l;
+ return val_hl * 1000 / 256;
}
static struct lm95241_data *lm95241_update_device(struct device *dev)
@@ -135,10 +140,13 @@ static ssize_t show_input(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct lm95241_data *data = lm95241_update_device(dev);
+ int index = to_sensor_dev_attr(attr)->index;
return snprintf(buf, PAGE_SIZE - 1, "%d\n",
- TempFromReg(data->temp[to_sensor_dev_attr(attr)->index],
- data->temp[to_sensor_dev_attr(attr)->index + 1]));
+ !index || (data->config & index) ?
+ temp_from_reg_signed(data->temp[index], data->temp[index + 1]) :
+ temp_from_reg_unsigned(data->temp[index],
+ data->temp[index + 1]));
}
static ssize_t show_type(struct device *dev, struct device_attribute *attr,
--
1.7.3.1
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [lm-sensors] [PATCH v3 2/3] hwmon: (lm95241) Fix negative
2011-07-07 20:42 [lm-sensors] [PATCH v3 2/3] hwmon: (lm95241) Fix negative Guenter Roeck
@ 2011-07-09 20:36 ` Jean Delvare
2011-07-09 21:17 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: Jean Delvare @ 2011-07-09 20:36 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
On Thu, 7 Jul 2011 13:42:44 -0700, Guenter Roeck wrote:
> Negative temperatures were returned in degrees C instead of milli-Degrees C.
> Also, negative temperatures were reported for remote temperature sensors even
> if the chip was configured for positive-only results.
>
> Fix by detecting temperature modes, and by treating negative temperatures
> similar to positive temperatures, with appropriate sign extension.
>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> Candidate for -stable.
>
> drivers/hwmon/lm95241.c | 20 ++++++++++++++------
> 1 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/lm95241.c b/drivers/hwmon/lm95241.c
> index 01c638e..ec02e30 100644
> --- a/drivers/hwmon/lm95241.c
> +++ b/drivers/hwmon/lm95241.c
> @@ -98,11 +98,16 @@ struct lm95241_data {
> };
>
> /* Conversions */
> -static int TempFromReg(u8 val_h, u8 val_l)
> +static int temp_from_reg_signed(u8 val_h, u8 val_l)
> {
> - if (val_h & 0x80)
> - return val_h - 0x100;
> - return val_h * 1000 + val_l * 1000 / 256;
> + s16 val_hl = (val_h << 8) | val_l;
> + return val_hl * 1000 / 256;
> +}
> +
> +static int temp_from_reg_unsigned(u8 val_h, u8 val_l)
> +{
> + u16 val_hl = (val_h << 8) | val_l;
> + return val_hl * 1000 / 256;
> }
>
> static struct lm95241_data *lm95241_update_device(struct device *dev)
> @@ -135,10 +140,13 @@ static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct lm95241_data *data = lm95241_update_device(dev);
> + int index = to_sensor_dev_attr(attr)->index;
>
> return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> - TempFromReg(data->temp[to_sensor_dev_attr(attr)->index],
> - data->temp[to_sensor_dev_attr(attr)->index + 1]));
> + !index || (data->config & index) ?
Hmm. This really should be:
+ !index || (data->config & (1 << (index / 2))) ?
It just _happens_ that (1 << (index / 2)) = index for index values 2
and 4, so your code works. But if we ever add support for a compatible
device with an extra temperature channel, it'll break. Not sure if the
rest of the driver code would properly cope with such a change anyway,
though.
Furthermore, I think that readability could be slightly improved by
changing "!index" to "index = 0", as this is neither an error
condition nor a boolean).
Readability could be improved even more with driver-wide changes but
this is obviously out-of-scope for this patch.
> + temp_from_reg_signed(data->temp[index], data->temp[index + 1]) :
> + temp_from_reg_unsigned(data->temp[index],
> + data->temp[index + 1]));
> }
>
> static ssize_t show_type(struct device *dev, struct device_attribute *attr,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [lm-sensors] [PATCH v3 2/3] hwmon: (lm95241) Fix negative
2011-07-07 20:42 [lm-sensors] [PATCH v3 2/3] hwmon: (lm95241) Fix negative Guenter Roeck
2011-07-09 20:36 ` Jean Delvare
@ 2011-07-09 21:17 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2011-07-09 21:17 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Sat, Jul 09, 2011 at 04:36:18PM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Thu, 7 Jul 2011 13:42:44 -0700, Guenter Roeck wrote:
> > Negative temperatures were returned in degrees C instead of milli-Degrees C.
> > Also, negative temperatures were reported for remote temperature sensors even
> > if the chip was configured for positive-only results.
> >
> > Fix by detecting temperature modes, and by treating negative temperatures
> > similar to positive temperatures, with appropriate sign extension.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > Candidate for -stable.
> >
> > drivers/hwmon/lm95241.c | 20 ++++++++++++++------
> > 1 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm95241.c b/drivers/hwmon/lm95241.c
> > index 01c638e..ec02e30 100644
> > --- a/drivers/hwmon/lm95241.c
> > +++ b/drivers/hwmon/lm95241.c
> > @@ -98,11 +98,16 @@ struct lm95241_data {
> > };
> >
> > /* Conversions */
> > -static int TempFromReg(u8 val_h, u8 val_l)
> > +static int temp_from_reg_signed(u8 val_h, u8 val_l)
> > {
> > - if (val_h & 0x80)
> > - return val_h - 0x100;
> > - return val_h * 1000 + val_l * 1000 / 256;
> > + s16 val_hl = (val_h << 8) | val_l;
> > + return val_hl * 1000 / 256;
> > +}
> > +
> > +static int temp_from_reg_unsigned(u8 val_h, u8 val_l)
> > +{
> > + u16 val_hl = (val_h << 8) | val_l;
> > + return val_hl * 1000 / 256;
> > }
> >
> > static struct lm95241_data *lm95241_update_device(struct device *dev)
> > @@ -135,10 +140,13 @@ static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > struct lm95241_data *data = lm95241_update_device(dev);
> > + int index = to_sensor_dev_attr(attr)->index;
> >
> > return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> > - TempFromReg(data->temp[to_sensor_dev_attr(attr)->index],
> > - data->temp[to_sensor_dev_attr(attr)->index + 1]));
> > + !index || (data->config & index) ?
>
> Hmm. This really should be:
>
> + !index || (data->config & (1 << (index / 2))) ?
>
> It just _happens_ that (1 << (index / 2)) = index for index values 2
> and 4, so your code works. But if we ever add support for a compatible
> device with an extra temperature channel, it'll break. Not sure if the
> rest of the driver code would properly cope with such a change anyway,
> though.
>
> Furthermore, I think that readability could be slightly improved by
> changing "!index" to "index = 0", as this is neither an error
> condition nor a boolean).
>
Makes sense. I changed it to
index = 0 || (data->config & (1 << (index / 2)))
> Readability could be improved even more with driver-wide changes but
> this is obviously out-of-scope for this patch.
>
Yes, I know. Another time, maybe.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-07-09 21:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-07 20:42 [lm-sensors] [PATCH v3 2/3] hwmon: (lm95241) Fix negative Guenter Roeck
2011-07-09 20:36 ` Jean Delvare
2011-07-09 21:17 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.