Re: [PATCH] hwmon: (lm70) Fix rounding of negative temperatures

[Guenter Roeck]

On Fri, Sep 25, 2026 at 08:03:06PM +0200, Christian Lamparter wrote:
> Hi,
> 
> On 9/24/26 10:53 PM, Ridham Khurana wrote:
> > temp1_input_show() drops the low bits of the temperature register by
> > dividing the raw value (raw / 32 on the LM70). These bits are not part
> > of the temperature: the LM70, LM71, LM74 and TMP122/TMP124 always
> > return some of them as 1, and the TMP125 fills them with copies of the
> > temperature LSB. Division rounds toward zero, so a negative temperature
> > with any of these bits set is reported one LSB too high.
> > 
> > For example, the TMP122 returns 0xffff for -0.0625 degrees C, which the
> > driver reports as 0. The LM70 returns 0xf39f for -25 degrees C, which
> > the driver reports as -24750.
> > 
> > Use an arithmetic right shift instead, which drops the low bits and
> > rounds down. tmp421 had a similar problem with negative values, fixed
> > by commit 724e8af85854 ("hwmon: (tmp421) fix rounding for negative
> > values").
> 
> For the TMP125 yes:
> 
> Reviewed-by: Christian Lamparter <chunkeey@xxxxxxxxx>
> 
> 
> > Fixes: e1a8e913f97e ("[PATCH] lm70: New hardware monitoring driver")
> > Fixes: a86e94dc946d ("hwmon: (lm70) Add support for LM71 and LM74")
> > Fixes: cd929672a9ef ("hwmon: (lm70) Add ti,tmp125 support")
> > Signed-off-by: Ridham Khurana <khurana.ridham222@xxxxxxxxx>
> > ----
> I wrote a little test program by hand (see below) and ran it on x64.
> No idea if different archs behave differently or if
> a clever unsafe math optimizing compiler flag will
> convert the  "/ 32" to a " >> 5" but I doubt that...
> 
> ---
> tmp125: raw:7ec0 tmp125_proposed: -2500 tmp125_now: -2500
> tmp125: raw:7eff tmp125_proposed: -2250 tmp125_now: -2000  <--- loss of percision
> tmp125: raw:7f00 tmp125_proposed: -2000 tmp125_now: -2000
> tmp125: raw:7f3f tmp125_proposed: -1750 tmp125_now: -1500  <--- more
> tmp125: raw:7f40 tmp125_proposed: -1500 tmp125_now: -1500
> tmp125: raw:7f7f tmp125_proposed: -1250 tmp125_now: -1000  <--- and this
> tmp125: raw:7f80 tmp125_proposed: -1000 tmp125_now: -1000
> tmp125: raw:7fbf tmp125_proposed:  -750 tmp125_now:  -500  <--- also bad
> tmp125: raw:7fc0 tmp125_proposed:  -500 tmp125_now:  -500
> tmp125: raw:7fff tmp125_proposed:  -250 tmp125_now:     0  <--- ouch
> tmp125: raw:   0 tmp125_proposed:     0 tmp125_now:     0
> tmp125: raw:  3f tmp125_proposed:   250 tmp125_now:   250
> tmp125: raw:  40 tmp125_proposed:   500 tmp125_now:   500
> tmp125: raw:  7f tmp125_proposed:   750 tmp125_now:   750
> tmp125: raw:  80 tmp125_proposed:  1000 tmp125_now:  1000
> tmp125: raw:  bf tmp125_proposed:  1250 tmp125_now:  1250
> tmp125: raw:  c0 tmp125_proposed:  1500 tmp125_now:  1500
> tmp125: raw:  ff tmp125_proposed:  1750 tmp125_now:  1750
> tmp125: raw: 100 tmp125_proposed:  2000 tmp125_now:  2000
> tmp125: raw: 13f tmp125_proposed:  2250 tmp125_now:  2250
> tmp125: raw: 140 tmp125_proposed:  2500 tmp125_now:  2500
> 

Sorry, you lost me with the above. Are you suggesting that the patch is
correct, that it is only correct for TMP125, or something else ?

Guenter

> (the positive values all look good.)
> 
> --- SNIP test.c program
> 
> #just name the file test.c and run make test (in a directory without any other makefile)
> #include <stdio.h>
> #include <asm-generic/int-l64.h>
> 
> static __s32 sign_extend32(__u32 value, int index)
> {
>         __u8 shift = 31 - index;
>         return (__s32)(value << shift) >> shift;
> }
> 
> static int tmp125_now(__s16 raw)
> {
>         return (sign_extend32(raw, 14) / 32) * 250;
> 
> }
> 
> static int tmp125_proposed(__s16 raw)
> {
>         return (sign_extend32(raw, 14) >> 5) * 250;
> 
> }
> 
> int main(int argc, char **args)
> {
>         for (__s16 raw = -320; raw <= 320; raw+=32) {
>                 __s16 tmp=raw & 0x7fe0 | ((raw & 32) >> 1) | ((raw & 32) >> 2) | ((raw & 32) >> 3) | ((raw & 32) >> 4) | ((raw & 32) >> 5);
>                 printf("tmp125: raw:%4x tmp125_proposed:%6d tmp125_now:%6d\n", tmp, tmp125_proposed(tmp), tmp125_now(tmp));
>         }
> 
>         return 0;
> }
> 
> --- SNAP
> 
> FYI: TMP125 datasheet says:
> "The  Temperature  Register  of  the  TMP125  is  a  16-bit, read-only register that stores
> the output of the most recentconversion. However, temperature is represented by only10-bits,
> which are in signed two’s complement format. Thefirst bit of the Temperature Register, D15,
> is a leading zero. Bits D14 and [sic] D5 are used to indicate temperature. Bits D4 to D0
> are the same as D5 (see Table 1)."
> 
> (They probably meant Bits D14 >>to<< D5 are used to indicate temperature).
>