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

[Nguyễn Công]

On Sat, Aug 8, 2026 at 9:48 PM Guenter Roeck <linux@xxxxxxxxxxxx> 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: stable@xxxxxxxxxxxxxxx
> > Assisted-by: Claude:claude-opus-4
> > Signed-off-by: Cong Nguyen <congnt264@xxxxxxxxx>
> > ---
> >   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.

Thanks,
Cong