Re: [PATCH] hwmon: (lm70) Fix rounding of negative temperatures
From: Christian Lamparter
Date: Sat Sep 26 2026 - 05:22:50 EST
Hi,
On 9/25/26 11:19 PM, Guenter Roeck wrote:
On Fri, Sep 25, 2026 at 08:03:06PM +0200, Christian Lamparter wrote:
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")I wrote a little test program by hand (see below) and ran it on x64.
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>
----
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
Ok, I guess the same thing happend to me now too?
Is there something specific you want to hear? My reasoning is that I wrote that
cd929672a9ef ("hwmon: (lm70) Add ti,tmp125 support")
I definitely wasn't aware that / 32 screws the with the rouding result for negative
integers and >> 5 wasn't. For unsigned compilers actually replace the divison when
the divisor is a 2^x constant on their own. You can find so many threads/topics about
that explaining why (basically pick your favourite).
https://softwareengineering.stackexchange.com/questions/209678/why-does-division-and-multiplication-by-2-use-the-shift-operator-rather-than-div
https://stackoverflow.com/a/27052650
...
It's been too long to remember what went through my head back then, but I knew that
replacing / 32 with >> 5 is a common optimization because the bitshift operation is
faster than spooling up the hardware dividers. But it would have looked funny when
all the other conversion codes (especially the LM70s. Because it's almost identical...
except the LM70 has the sign-bit at D15 and hence the raw value can be directly cast
to a s16 type). So definitly that >> 5 was on my mind.
Now, I was surprised to hear that / 32 and >> 5 differ for negative numbers and I wanted
to find out and the experiment tells me: yes, it's true.
Do you get the same result? What if you replace the function with the other
LM/TMP implementation?
But I have the suspicion, this doesn't answer your question, or does it?
Cheers,
Christian