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

On Sun, Aug 9, 2026 at 9:45 AM Guenter Roeck <[email protected]> wrote:
>
> 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
>

Yeah,
I just sent a v2 as two patches: (1) fix the -128000/127000 range, and
(2) sign-extend the _offset and _crit reads.
Thanks,
Cong