| Message ID | 20240424185913.1177127-1-dima.fedrau@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: temperature: mcp9600: Fix temperature reading for negative values | expand |
Hi Dimitri, Interesting patch this one. I think this does apply, although, the cold junction register has for sign bits so I think we could also have a mask to clear those out. Some code suggestions inline. Regards, Marcelo On 04/24, Dimitri Fedrau wrote: > Temperature is stored as 16bit value in two's complement format. Current > implementation ignores the sign bit. Make it aware of the sign bit by > using sign_extend32. > > Fixes: 3f6b9598b6df ("iio: temperature: Add MCP9600 thermocouple EMF converter") > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > --- > drivers/iio/temperature/mcp9600.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c > index 46845804292b..7a3eef5d5e75 100644 > --- a/drivers/iio/temperature/mcp9600.c > +++ b/drivers/iio/temperature/mcp9600.c #define MCP9600_COLD_JUNCTION_SIGN_MSK GENMASK(15,12) ... > @@ -52,7 +52,8 @@ static int mcp9600_read(struct mcp9600_data *data, > > if (ret < 0) > return ret; > - *val = ret; > + > + *val = sign_extend32(ret, 15); if (chan->address == MCP9600_COLD_JUNCTION) *val &= ~MCP9600_COLD_JUNCTION_SIGN_MSK; > > return 0; > } > -- > 2.39.2 > >
Am Sat, Apr 27, 2024 at 05:49:18AM -0300 schrieb Marcelo Schmitt: > Hi Dimitri, > > Interesting patch this one. > I think this does apply, although, the cold junction register has for sign bits > so I think we could also have a mask to clear those out. > Some code suggestions inline. > Hi Marcelo, the temperature bits are in two’s complement format for hot and cold junction. Equations to calculate the temperature are also the same in the datasheet. There should be no difference when handling them. I don't think we need to do anything more with the value except sign_extend it to the appropriate data type. If the sign bits aren't right, there is a bug in the chip, until then futher processing of it is unneeded. I could add a comment here if it helps. > On 04/24, Dimitri Fedrau wrote: > > Temperature is stored as 16bit value in two's complement format. Current > > implementation ignores the sign bit. Make it aware of the sign bit by > > using sign_extend32. > > > > Fixes: 3f6b9598b6df ("iio: temperature: Add MCP9600 thermocouple EMF converter") > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > --- > > drivers/iio/temperature/mcp9600.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c > > index 46845804292b..7a3eef5d5e75 100644 > > --- a/drivers/iio/temperature/mcp9600.c > > +++ b/drivers/iio/temperature/mcp9600.c > > #define MCP9600_COLD_JUNCTION_SIGN_MSK GENMASK(15,12) > ... > > > @@ -52,7 +52,8 @@ static int mcp9600_read(struct mcp9600_data *data, > > > > if (ret < 0) > > return ret; > > - *val = ret; > > + > > + *val = sign_extend32(ret, 15); > if (chan->address == MCP9600_COLD_JUNCTION) > *val &= ~MCP9600_COLD_JUNCTION_SIGN_MSK; > This won't work. Assuming int is 32-bit ret = 0xfffe and *val = -2 after sign_extends32, this would result in *val = -61442 which is wrong. > > > > return 0; > > } > > -- > > 2.39.2 > > > > Best regards, Dimitri Fedrau
On Sat, 27 Apr 2024 21:57:58 +0200 Dimitri Fedrau <dima.fedrau@gmail.com> wrote: > Am Sat, Apr 27, 2024 at 05:49:18AM -0300 schrieb Marcelo Schmitt: > > Hi Dimitri, > > > > Interesting patch this one. > > I think this does apply, although, the cold junction register has for sign bits > > so I think we could also have a mask to clear those out. > > Some code suggestions inline. > > > Hi Marcelo, > > the temperature bits are in two’s complement format for hot and cold > junction. Equations to calculate the temperature are also the same in > the datasheet. There should be no difference when handling them. I don't > think we need to do anything more with the value except sign_extend it to > the appropriate data type. If the sign bits aren't right, there is a bug > in the chip, until then futher processing of it is unneeded. I could add > a comment here if it helps. Agreed - by my reading the original patch is correct. Maybe it would act as cleaner 'documentation' to have the sign_extend32() for the cold junction be from bit 12 rather than 15, but I'm not sure it's worth the effort. Andrew, would be great if you can review this fix in case we are all missing something! Jonathan > > > On 04/24, Dimitri Fedrau wrote: > > > Temperature is stored as 16bit value in two's complement format. Current > > > implementation ignores the sign bit. Make it aware of the sign bit by > > > using sign_extend32. > > > > > > Fixes: 3f6b9598b6df ("iio: temperature: Add MCP9600 thermocouple EMF converter") > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > --- > > > drivers/iio/temperature/mcp9600.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c > > > index 46845804292b..7a3eef5d5e75 100644 > > > --- a/drivers/iio/temperature/mcp9600.c > > > +++ b/drivers/iio/temperature/mcp9600.c > > > > #define MCP9600_COLD_JUNCTION_SIGN_MSK GENMASK(15,12) > > ... > > > > > @@ -52,7 +52,8 @@ static int mcp9600_read(struct mcp9600_data *data, > > > > > > if (ret < 0) > > > return ret; > > > - *val = ret; > > > + > > > + *val = sign_extend32(ret, 15); > > if (chan->address == MCP9600_COLD_JUNCTION) > > *val &= ~MCP9600_COLD_JUNCTION_SIGN_MSK; > > > This won't work. Assuming int is 32-bit ret = 0xfffe and *val = -2 after > sign_extends32, this would result in *val = -61442 which is wrong. > > > > > > return 0; > > > } > > > -- > > > 2.39.2 > > > > > > > Best regards, > Dimitri Fedrau
Hi all, On Apr 28, 2024, at 9:46 AM, Jonathan Cameron <jic23@kernel.org> wrote: > Agreed - by my reading the original patch is correct. Maybe it would act > as cleaner 'documentation' to have the sign_extend32() for the cold junction be > from bit 12 rather than 15, but I'm not sure it's worth the effort. > > Andrew, would be great if you can review this fix in case we are all missing > something! I also agree that Dimitri’s original patch appears correct per the data sheet. I think of the cold junction register as a 16 bit 2s complement signed integer, despite the limited range. I like sign extending it from bit 15 rather than from 12. I applied Dimitri’s patch and stuck a dev board in my icebox. Before patch: # cat /sys/bus/iio/devices/iio:device0/in_temp_raw 65222 # cat /sys/bus/iio/devices/iio:device0/in_temp_ambient_raw 65256 After patch: # cat /sys/bus/iio/devices/iio:device0/in_temp_raw -260 # cat /sys/bus/iio/devices/iio:device0/in_temp_ambient_raw -212 Looks good to me! Thanks for the patch Dimitri, and thanks to all for the review! Andrew
On 04/28, Jonathan Cameron wrote: > On Sat, 27 Apr 2024 21:57:58 +0200 > Dimitri Fedrau <dima.fedrau@gmail.com> wrote: > > > Am Sat, Apr 27, 2024 at 05:49:18AM -0300 schrieb Marcelo Schmitt: > > > Hi Dimitri, > > > > > > Interesting patch this one. > > > I think this does apply, although, the cold junction register has for sign bits > > > so I think we could also have a mask to clear those out. > > > Some code suggestions inline. > > > > > Hi Marcelo, > > > > the temperature bits are in two’s complement format for hot and cold > > junction. Equations to calculate the temperature are also the same in > > the datasheet. There should be no difference when handling them. I don't > > think we need to do anything more with the value except sign_extend it to > > the appropriate data type. If the sign bits aren't right, there is a bug > > in the chip, until then futher processing of it is unneeded. I could add > > a comment here if it helps. Ah yes, I missed the fact that other bits need to be set to make the value smaller (closer to zero) when the MSB of two's complement number is set. > > Agreed - by my reading the original patch is correct. Maybe it would act > as cleaner 'documentation' to have the sign_extend32() for the cold junction be > from bit 12 rather than 15, but I'm not sure it's worth the effort. Yes, the original patch should be correct. Yeah, no practical difference using bit 12 or 15 to sign extend the number so probably not worth changing that. > > Andrew, would be great if you can review this fix in case we are all missing > something! > > Jonathan > > > > > > On 04/24, Dimitri Fedrau wrote: > > > > Temperature is stored as 16bit value in two's complement format. Current > > > > implementation ignores the sign bit. Make it aware of the sign bit by > > > > using sign_extend32. > > > > Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com> > > > > Fixes: 3f6b9598b6df ("iio: temperature: Add MCP9600 thermocouple EMF converter") > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > > --- > > > > drivers/iio/temperature/mcp9600.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c > > > > index 46845804292b..7a3eef5d5e75 100644 > > > > --- a/drivers/iio/temperature/mcp9600.c > > > > +++ b/drivers/iio/temperature/mcp9600.c > > > > > > #define MCP9600_COLD_JUNCTION_SIGN_MSK GENMASK(15,12) > > > ... > > > > > > > @@ -52,7 +52,8 @@ static int mcp9600_read(struct mcp9600_data *data, > > > > > > > > if (ret < 0) > > > > return ret; > > > > - *val = ret; > > > > + > > > > + *val = sign_extend32(ret, 15); > > > if (chan->address == MCP9600_COLD_JUNCTION) > > > *val &= ~MCP9600_COLD_JUNCTION_SIGN_MSK; > > > > > This won't work. Assuming int is 32-bit ret = 0xfffe and *val = -2 after > > sign_extends32, this would result in *val = -61442 which is wrong. > > > > > > > > return 0; > > > > } > > > > -- > > > > 2.39.2 > > > > > > > > > > Best regards, > > Dimitri Fedrau >
On Sun, 28 Apr 2024 14:46:39 -0400 Andrew Hepp <andrew.hepp@ahepp.dev> wrote: > Hi all, > > On Apr 28, 2024, at 9:46 AM, Jonathan Cameron <jic23@kernel.org> wrote: > > > Agreed - by my reading the original patch is correct. Maybe it would act > > as cleaner 'documentation' to have the sign_extend32() for the cold junction be > > from bit 12 rather than 15, but I'm not sure it's worth the effort. > > > > Andrew, would be great if you can review this fix in case we are all missing > > something! > > I also agree that Dimitri’s original patch appears correct per the data sheet. > > I think of the cold junction register as a 16 bit 2s complement signed integer, > despite the limited range. I like sign extending it from bit 15 rather than from 12. > > I applied Dimitri’s patch and stuck a dev board in my icebox. > > Before patch: > # cat /sys/bus/iio/devices/iio:device0/in_temp_raw > 65222 > # cat /sys/bus/iio/devices/iio:device0/in_temp_ambient_raw > 65256 > > After patch: > # cat /sys/bus/iio/devices/iio:device0/in_temp_raw > -260 > # cat /sys/bus/iio/devices/iio:device0/in_temp_ambient_raw > -212 > > Looks good to me! Thanks for the patch Dimitri, and thanks to all for the review! > > Andrew That definitely deserves a Tested-by tag so I added one. Hope you don't mind! Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan
diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c index 46845804292b..7a3eef5d5e75 100644 --- a/drivers/iio/temperature/mcp9600.c +++ b/drivers/iio/temperature/mcp9600.c @@ -52,7 +52,8 @@ static int mcp9600_read(struct mcp9600_data *data, if (ret < 0) return ret; - *val = ret; + + *val = sign_extend32(ret, 15); return 0; }
Temperature is stored as 16bit value in two's complement format. Current implementation ignores the sign bit. Make it aware of the sign bit by using sign_extend32. Fixes: 3f6b9598b6df ("iio: temperature: Add MCP9600 thermocouple EMF converter") Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> --- drivers/iio/temperature/mcp9600.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)