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

[Christian Lamparter <chunkeey@gmail.com>]

On 9/27/26 8:31 AM, David Laight 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...
> 
> A compiler will convert an unsigned divide to a shift, but can't
> do so for signed divides.
 > > But you'll also get flagged because C doesn't define right shifts
> for negative values.

Hmm... For at least x86, any compiler should see that the is an integer type
and use the Shift Arithmetic Right (SAR) instead of the SHR instruction. From
what I know that SAR instructions is very old, like 8086 with improvements
some major improvements on the 286... And linux started with the 386 so, this
should work even into the past. (But please correct if this is wrong).

I can't tell what clang would do, but GCC says this (second last bullet point):
<https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html>

|Bitwise operators act on the representation of the value including both
|the sign and value bits, where the sign bit is considered immediately above
|the highest-value value bit. Signed ‘>>’ acts on negative numbers by sign extension.
|
|As an extension to the C language, GCC does not use the latitude given in C99 and
|later to treat certain aspects of signed ‘<<’ as undefined.
|However, -fsanitize=shift (and -fsanitize=undefined) will diagnose such cases.
|They are also diagnosed where constant expressions are required.

So, the << (shift left) could indeed be a problem/undefined.

Cheers,
Christian