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

[Guenter Roeck]

On Sat, Sep 26, 2026 at 11:22:39AM +0200, Christian Lamparter wrote:
> 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")
> > > > 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
> 
> 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?
> 

I asked, or tried to ask, what I thought is a simple question: Is the
patch correct or not ? You could have answered with "it is correct" or
with "it is not correct (possibly augmented with 'and this is the
reason')". I don't see either in your reply.

So, no, I don't think you answered my question. I'll just assume that
the patch is correct, to be fixed up again later if needed.

Guenter