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

[Christian Lamparter <chunkeey@gmail.com>]

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@gmail.com>
>>
>>
>>> 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@gmail.com>
>>> ----
>> 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

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