[PATCH] hwmon: (ina2xx) Change register cache to signed

[PATCH] hwmon: (ina2xx) Change register cache to signed

[Fabio Baltieri]

All devices supported by the ina2xx driver are bidirectional and reports
the measured value as a signed 16 bit, but the current driver
implementation caches the number as an u16, leading to an incorrect sign
extension when reporting to the userspace in ina2xx_get_value().

This patch fixes the problem by using a s16 instead, and has been tested
on an INA219.

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---
 drivers/hwmon/ina2xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 93d26e8..d994280 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -86,7 +86,7 @@ struct ina2xx_data {
 	unsigned long last_updated;
 
 	int kind;
-	u16 regs[INA2XX_MAX_REGISTERS];
+	s16 regs[INA2XX_MAX_REGISTERS];
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
-- 
1.8.4

Re: [PATCH] hwmon: (ina2xx) Change register cache to signed

[Guenter Roeck]

On Sat, Jun 07, 2014 at 09:47:01PM +0100, Fabio Baltieri wrote:
> All devices supported by the ina2xx driver are bidirectional and reports
> the measured value as a signed 16 bit, but the current driver
> implementation caches the number as an u16, leading to an incorrect sign
> extension when reporting to the userspace in ina2xx_get_value().
> 
> This patch fixes the problem by using a s16 instead, and has been tested
> on an INA219.
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>

Applied.

Thanks,
Guenter

Re: [lm-sensors] [PATCH] hwmon: (ina2xx) Change register cache to signed

[Guenter Roeck]

On Sun, Jun 08, 2014 at 01:16:00PM -0700, Guenter Roeck wrote:
> On Sat, Jun 07, 2014 at 09:47:01PM +0100, Fabio Baltieri wrote:
> > All devices supported by the ina2xx driver are bidirectional and reports
> > the measured value as a signed 16 bit, but the current driver
> > implementation caches the number as an u16, leading to an incorrect sign
> > extension when reporting to the userspace in ina2xx_get_value().
> > 
> > This patch fixes the problem by using a s16 instead, and has been tested
> > on an INA219.
> > 
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> 
> Applied.
> 
Actually, no, this won't work. The statement above is only correct for current
and shunt voltage measurements, but not for power measurements and not for bus
voltage measurements. Changing the register to s16 won't help; conversion needs
to be done in ina2xx_get_value() for shunt voltage and current measurement only.
Otherwise we just move the bug from current/shunt voltage measurements to
power / bus voltage measurements.

Even more interesting, the power is supposed to be the product of Bus voltage
and current, and the latter can be negative. However, the power register
description does not suggest that the upper bit would be a sign bit. So there
is some discrepancy in the datasheet, and we'll need some real-world data to
understand if the upper power bit is signed or not.

Guenter

Re: [lm-sensors] [PATCH] hwmon: (ina2xx) Change register cache to signed

[Fabio Baltieri]

On Sun, Jun 8, 2014 at 9:30 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Sun, Jun 08, 2014 at 01:16:00PM -0700, Guenter Roeck wrote:
>> On Sat, Jun 07, 2014 at 09:47:01PM +0100, Fabio Baltieri wrote:
>> > All devices supported by the ina2xx driver are bidirectional and reports
>> > the measured value as a signed 16 bit, but the current driver
>> > implementation caches the number as an u16, leading to an incorrect sign
>> > extension when reporting to the userspace in ina2xx_get_value().
>> >
>> > This patch fixes the problem by using a s16 instead, and has been tested
>> > on an INA219.
>> >
>> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
>>
>> Applied.
>>
> Actually, no, this won't work. The statement above is only correct for current
> and shunt voltage measurements, but not for power measurements and not for bus
> voltage measurements. Changing the register to s16 won't help; conversion needs
> to be done in ina2xx_get_value() for shunt voltage and current measurement only.
> Otherwise we just move the bug from current/shunt voltage measurements to
> power / bus voltage measurements.
>
> Even more interesting, the power is supposed to be the product of Bus voltage
> and current, and the latter can be negative. However, the power register
> description does not suggest that the upper bit would be a sign bit. So there
> is some discrepancy in the datasheet, and we'll need some real-world data to
> understand if the upper power bit is signed or not.

Hi Guenter,

looks like you're right here, I wasn't paying too attention to the
power register and it actually always reads positive, even when the
current is flowing in the reverse direction, real data from the
ina219:

[55694.263502] shunt_voltage=fb46
[55694.263691] bus_voltage=20c2
[55694.263847] power=00fe
[55694.263954] current=fb46

I'll send a v2 to only cast the two signed register then.

Many thanks for spotting this!

Fabio

-- 
Fabio Baltieri