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

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

[Cong Nguyen]

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.

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.

Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
 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;
 
 			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:
-- 
2.25.1

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

[Guenter Roeck]

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@vger.kernel.org
> Assisted-by: Claude:claude-opus-4
> Signed-off-by: Cong Nguyen <congnt264@gmail.com>
> ---
>   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:

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@roeck-us.net> 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@vger.kernel.org
> > Assisted-by: Claude:claude-opus-4
> > Signed-off-by: Cong Nguyen <congnt264@gmail.com>
> > ---
> >   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

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

[Guenter Roeck]

On 8/8/26 08:54, Nguyễn Công wrote:
> On Sat, Aug 8, 2026 at 9:48 PM Guenter Roeck <linux@roeck-us.net> 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@vger.kernel.org
>>> Assisted-by: Claude:claude-opus-4
>>> Signed-off-by: Cong Nguyen <congnt264@gmail.com>
>>> ---
>>>    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

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

[Nguyễn Công]

On Sun, Aug 9, 2026 at 9:45 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/8/26 08:54, Nguyễn Công wrote:
> > On Sat, Aug 8, 2026 at 9:48 PM Guenter Roeck <linux@roeck-us.net> 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@vger.kernel.org
> >>> Assisted-by: Claude:claude-opus-4
> >>> Signed-off-by: Cong Nguyen <congnt264@gmail.com>
> >>> ---
> >>>    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