Re: [PATCH] hwmon: (max6621) fix negative temperature offset and crit readings

On 8/8/26 08:54, Nguyễn Công wrote:
> On Sat, Aug 8, 2026 at 9:48 PM Guenter Roeck <[email protected]> wrote:
>>
>> On 8/8/26 01:37, Cong Nguyen wrote:
>>> max6621_read() reads the temperature input, offset and critical alert
>>> registers into a u32 and assigns them to the output without sign
>>> extension. The device reports these values in two's complement (the
>>> driver comment and the write path, which clamps to
>>> MAX6621_TEMP_INPUT_MIN == -127000 and encodes negatives, both rely on
>>> this), so negative values are misreported as large positive numbers:
>>>
>>>     - temp_offset: *val = (regval >> MAX6621_REG_TEMP_SHIFT) * 1000, an
>>>       unsigned shift, so e.g. a -10 degrees C offset (register 0xfd80)
>>>       reads back as ~+1014000 millidegrees.
>>>     - temp_crit: *val = regval * 1000, so a negative critical threshold
>>>       reads back as a large positive value.
>>>     - temp_input used an s8 intermediate, which is correct for the
>>>       -127..127 range but reports the documented +128 degrees C maximum as
>>>       -128 degrees C.
>>>
>>
>> This problem does not exist. The AI is hallucinating a bit. There is no mention
>> of +128 degrees C in the datasheet.
>>
>> The problem here is that MAX6621_TEMP_INPUT_MIN (-127) and MAX6621_TEMP_INPUT_MAX (128)
>> are wrong. That should be -128 and (+)127. I guess the AI doesn't see that.
>>
>>> The registers are 16-bit (the driver's own PECI error codes occupy
>>> 0x8000-0x80ff), so sign-extend from bit 15 before scaling. This fixes
>>> all three reads and drops the now-unused s8 intermediate.
>>>
>>
>> The PECI error code reference is just confusing AI slop and completely irrelevant
>> for the patch.
>>
>>> Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
>>> Cc: [email protected]
>>> Assisted-by: Claude:claude-opus-4
>>> Signed-off-by: Cong Nguyen <[email protected]>
>>> ---
>>>    drivers/hwmon/max6621.c | 11 +++++------
>>>    1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
>>> index a7066f3a0bb4..03ac3eb5a594 100644
>>> --- a/drivers/hwmon/max6621.c
>>> +++ b/drivers/hwmon/max6621.c
>>> @@ -204,7 +204,6 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>        struct max6621_data *data = dev_get_drvdata(dev);
>>>        u32 regval;
>>>        int reg;
>>> -     s8 temp;
>>>        int ret;
>>>
>>>        switch (type) {
>>> @@ -225,8 +224,8 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>                         * The temperature is given in two's complement and 8
>>>                         * bits is used for the register conversion.
>>>                         */
>>> -                     temp = (regval >> MAX6621_REG_TEMP_SHIFT);
>>> -                     *val = temp * 1000L;
>>> +                     *val = (sign_extend32(regval, 15) >>
>>> +                             MAX6621_REG_TEMP_SHIFT) * 1000L;
>>
>>                          *val = ((s16)regval >> MAX6621_REG_TEMP_SHIFT) * 1000L;
>>
>> However, as mentioned above, this is not a problem in the first place
>> but AI not understanding the code and the real problem..
>>
>> Guenter
>>
>>>
>>>                        break;
>>>                case hwmon_temp_offset:
>>> @@ -239,8 +238,8 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>                        if (ret)
>>>                                return ret;
>>>
>>> -                     *val = (regval >> MAX6621_REG_TEMP_SHIFT) *
>>> -                            1000L;
>>> +                     *val = (sign_extend32(regval, 15) >>
>>> +                             MAX6621_REG_TEMP_SHIFT) * 1000L;
>>>
>>>                        break;
>>>                case hwmon_temp_crit:
>>> @@ -254,7 +253,7 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>                        if (ret)
>>>                                return ret;
>>>
>>> -                     *val = regval * 1000L;
>>> +                     *val = sign_extend32(regval, 15) * 1000L;
>>>
>>>                        break;
>>>                case hwmon_temp_crit_alarm:
>>
> 
> You're right - temp_input is fine (the s8 already sign-extends -128..127),
> and the +128/PECI reasoning was wrong. Sorry for the noise.
> 
> Agreed the real bug is the clamp range: MAX6621_TEMP_INPUT_MIN/MAX should be
> -128000/127000. Would you like me to send a v2 for that, or would you rather
> just fix it directly?
> 
> If a v2 is useful: temp_offset/temp_crit also don't use the s8 - they scale a
> raw u32 - so a negative offset round-trips wrong (write -10 degC -> mc2reg
> 0xfd80 -> reads back 1014000). I can fold a (s16) fix for those in too, if you
> agree it's a real issue.
> 
It should be two patches: One fixing the range, the other fixing the negative
temperature values in _offset and _crit.

Thanks,
Guenter