[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Writing the maximum allowed temperature of 128°C to hwmon_temp_crit silently wraps to -128°C in hardware.
- [Medium] Fractional temperature precision is discarded due to shifting and integer division before scaling.
--

--- Patch [1]: [PATCH] hwmon: (max6621) fix negative temperature offset and crit readings ---
commit a073f1d4838dda93adf9b46f8e3cc16f1b662f82
Author: Cong Nguyen <congnt264@gmail.com>

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

This patch fixes temperature reporting for the max6621 driver. It
addresses an issue where negative temperatures were reported as large
positive numbers due to missing sign extension before scaling the 16-bit
register values.

Cc: stable@vger.kernel.org

>       - 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.

[Severity: High]
This is a pre-existing issue, but while fixing the read path for the +128
degrees C maximum, is it possible that the write path still wraps to -128
degrees C?

When writing to hwmon_temp_crit, max6621_write() in drivers/hwmon/max6621.c
clamps the input to MAX6621_TEMP_INPUT_MAX (128000) and divides by 1000L:

drivers/hwmon/max6621.c:max6621_write() {
    ...
    case hwmon_temp_crit:
        ...
        /* Clamp to allowed range to prevent overflow. */
        val = clamp_val(val, MAX6621_TEMP_INPUT_MIN,
                MAX6621_TEMP_INPUT_MAX);
        val = val / 1000L;

        return regmap_write(data->regmap, reg, val);
}

If a user writes 128000 to temp_crit, it gets clamped and divided to 128
(0x80). For an 8-bit two's complement alert register in the MAX6621
hardware, 0x80 is interpreted as -128 degrees C.

Could this cause continuous false thermal alerts and potential unintended
system throttling?

> diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
> index e86ec6d237ca6..9becd098d840b 100644
> --- a/drivers/hwmon/max6621.c
> +++ b/drivers/hwmon/max6621.c
[ ... ]
> @@ -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;

[Severity: Medium]
This is a pre-existing issue, but does this arithmetic right shift discard
the hardware's fractional temperature precision?

By shifting right by MAX6621_REG_TEMP_SHIFT (6 bits) before multiplying by
1000L, the lower 6 fractional bits are lost, dropping up to 0.984 degrees C
of precision.

Would it be better to multiply by 1000L first and then shift or divide to
expose the full fractional precision provided by the MAX6621 device?

The same precision loss also happens when writing hwmon_temp_offset in
drivers/hwmon/max6621.c:

drivers/hwmon/max6621.c:max6621_temp_mc2reg() {
    return (val / 1000L) << MAX6621_REG_TEMP_SHIFT;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808083718.2612205-1-congnt264@gmail.com?part=1

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

[Nguyễn Công]

On Sat, Aug 8, 2026 at 3:58 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Writing the maximum allowed temperature of 128°C to hwmon_temp_crit silently wraps to -128°C in hardware.
> - [Medium] Fractional temperature precision is discarded due to shifting and integer division before scaling.
> --
>
> --- Patch [1]: [PATCH] hwmon: (max6621) fix negative temperature offset and crit readings ---
> commit a073f1d4838dda93adf9b46f8e3cc16f1b662f82
> Author: Cong Nguyen <congnt264@gmail.com>
>
> hwmon: (max6621) fix negative temperature offset and crit readings
>
> This patch fixes temperature reporting for the max6621 driver. It
> addresses an issue where negative temperatures were reported as large
> positive numbers due to missing sign extension before scaling the 16-bit
> register values.
>
> Cc: stable@vger.kernel.org
>
> >       - 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.
>
> [Severity: High]
> This is a pre-existing issue, but while fixing the read path for the +128
> degrees C maximum, is it possible that the write path still wraps to -128
> degrees C?
>
> When writing to hwmon_temp_crit, max6621_write() in drivers/hwmon/max6621.c
> clamps the input to MAX6621_TEMP_INPUT_MAX (128000) and divides by 1000L:
>
> drivers/hwmon/max6621.c:max6621_write() {
>     ...
>     case hwmon_temp_crit:
>         ...
>         /* Clamp to allowed range to prevent overflow. */
>         val = clamp_val(val, MAX6621_TEMP_INPUT_MIN,
>                 MAX6621_TEMP_INPUT_MAX);
>         val = val / 1000L;
>
>         return regmap_write(data->regmap, reg, val);
> }
>
> If a user writes 128000 to temp_crit, it gets clamped and divided to 128
> (0x80). For an 8-bit two's complement alert register in the MAX6621
> hardware, 0x80 is interpreted as -128 degrees C.
>
> Could this cause continuous false thermal alerts and potential unintended
> system throttling?
>
> > diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
> > index e86ec6d237ca6..9becd098d840b 100644
> > --- a/drivers/hwmon/max6621.c
> > +++ b/drivers/hwmon/max6621.c
> [ ... ]
> > @@ -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;
>
> [Severity: Medium]
> This is a pre-existing issue, but does this arithmetic right shift discard
> the hardware's fractional temperature precision?
>
> By shifting right by MAX6621_REG_TEMP_SHIFT (6 bits) before multiplying by
> 1000L, the lower 6 fractional bits are lost, dropping up to 0.984 degrees C
> of precision.
>
> Would it be better to multiply by 1000L first and then shift or divide to
> expose the full fractional precision provided by the MAX6621 device?
>
> The same precision loss also happens when writing hwmon_temp_offset in
> drivers/hwmon/max6621.c:
>
> drivers/hwmon/max6621.c:max6621_temp_mc2reg() {
>     return (val / 1000L) << MAX6621_REG_TEMP_SHIFT;
> }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260808083718.2612205-1-congnt264@gmail.com?part=1

Thanks for the review. Both points are pre-existing and on the
write/precision side, so I'd like to keep this patch focused on the
sign-extension read fix and handle them separately:

- temp_crit write / +128 °C: agreed this is a pre-existing write-path
issue. max6621_write() clamps to MAX6621_TEMP_INPUT_MAX (128000), so
128 °C lands in the (non-shifted) alert register as 0x80; if that
register is interpreted as signed, it wraps. I'll send a separate
patch to tighten the clamp once I've confirmed the alert register
width/sign against the datasheet — it's orthogonal to this read-side
fix.
- Fractional precision: also pre-existing — both the read (>>
MAX6621_REG_TEMP_SHIFT before scaling) and the write
(max6621_temp_mc2reg() divides by 1000 before shifting) discard the 6
fractional bits, so the driver has always reported at 1 °C resolution.
Exposing the 1/64 °C resolution is a behavioural change rather than a
fix, so I'd prefer to do that as a follow-up.

This patch only restores the sign of the existing values, which were
being reported as large positives for negative offsets and crit
thresholds.

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

[Nguyễn Công]

On Sat, Aug 8, 2026 at 4:19 PM Nguyễn Công <congnt264@gmail.com> wrote:
>
> On Sat, Aug 8, 2026 at 3:58 PM <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] Writing the maximum allowed temperature of 128°C to hwmon_temp_crit silently wraps to -128°C in hardware.
> > - [Medium] Fractional temperature precision is discarded due to shifting and integer division before scaling.
> > --
> >
> > --- Patch [1]: [PATCH] hwmon: (max6621) fix negative temperature offset and crit readings ---
> > commit a073f1d4838dda93adf9b46f8e3cc16f1b662f82
> > Author: Cong Nguyen <congnt264@gmail.com>
> >
> > hwmon: (max6621) fix negative temperature offset and crit readings
> >
> > This patch fixes temperature reporting for the max6621 driver. It
> > addresses an issue where negative temperatures were reported as large
> > positive numbers due to missing sign extension before scaling the 16-bit
> > register values.
> >
> > Cc: stable@vger.kernel.org
> >
> > >       - 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.
> >
> > [Severity: High]
> > This is a pre-existing issue, but while fixing the read path for the +128
> > degrees C maximum, is it possible that the write path still wraps to -128
> > degrees C?
> >
> > When writing to hwmon_temp_crit, max6621_write() in drivers/hwmon/max6621.c
> > clamps the input to MAX6621_TEMP_INPUT_MAX (128000) and divides by 1000L:
> >
> > drivers/hwmon/max6621.c:max6621_write() {
> >     ...
> >     case hwmon_temp_crit:
> >         ...
> >         /* Clamp to allowed range to prevent overflow. */
> >         val = clamp_val(val, MAX6621_TEMP_INPUT_MIN,
> >                 MAX6621_TEMP_INPUT_MAX);
> >         val = val / 1000L;
> >
> >         return regmap_write(data->regmap, reg, val);
> > }
> >
> > If a user writes 128000 to temp_crit, it gets clamped and divided to 128
> > (0x80). For an 8-bit two's complement alert register in the MAX6621
> > hardware, 0x80 is interpreted as -128 degrees C.
> >
> > Could this cause continuous false thermal alerts and potential unintended
> > system throttling?
> >
> > > diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
> > > index e86ec6d237ca6..9becd098d840b 100644
> > > --- a/drivers/hwmon/max6621.c
> > > +++ b/drivers/hwmon/max6621.c
> > [ ... ]
> > > @@ -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;
> >
> > [Severity: Medium]
> > This is a pre-existing issue, but does this arithmetic right shift discard
> > the hardware's fractional temperature precision?
> >
> > By shifting right by MAX6621_REG_TEMP_SHIFT (6 bits) before multiplying by
> > 1000L, the lower 6 fractional bits are lost, dropping up to 0.984 degrees C
> > of precision.
> >
> > Would it be better to multiply by 1000L first and then shift or divide to
> > expose the full fractional precision provided by the MAX6621 device?
> >
> > The same precision loss also happens when writing hwmon_temp_offset in
> > drivers/hwmon/max6621.c:
> >
> > drivers/hwmon/max6621.c:max6621_temp_mc2reg() {
> >     return (val / 1000L) << MAX6621_REG_TEMP_SHIFT;
> > }
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260808083718.2612205-1-congnt264@gmail.com?part=1
>
> Thanks for the review. Both points are pre-existing and on the
> write/precision side, so I'd like to keep this patch focused on the
> sign-extension read fix and handle them separately:
>
> - temp_crit write / +128 °C: agreed this is a pre-existing write-path
> issue. max6621_write() clamps to MAX6621_TEMP_INPUT_MAX (128000), so
> 128 °C lands in the (non-shifted) alert register as 0x80; if that
> register is interpreted as signed, it wraps. I'll send a separate
> patch to tighten the clamp once I've confirmed the alert register
> width/sign against the datasheet — it's orthogonal to this read-side
> fix.
> - Fractional precision: also pre-existing — both the read (>>
> MAX6621_REG_TEMP_SHIFT before scaling) and the write
> (max6621_temp_mc2reg() divides by 1000 before shifting) discard the 6
> fractional bits, so the driver has always reported at 1 °C resolution.
> Exposing the 1/64 °C resolution is a behavioural change rather than a
> fix, so I'd prefer to do that as a follow-up.
>
> This patch only restores the sign of the existing values, which were
> being reported as large positives for negative offsets and crit
> thresholds.

Following up after checking the datasheet (MAX6621AUB):

All temperature values use 16-bit two's complement, left-shifted so that
the +1 C bit is bit 6 (1/64 C per LSB). The alternate integer
representation is optional and enabled via CONFIG0 bit 6, which this
driver never sets, so the device stays in the default shifted format.

The offset path already accounts for this: it shifts by
MAX6621_REG_TEMP_SHIFT on read and uses max6621_temp_mc2reg() (which
left-shifts by 6) on write. The temp_crit path does not -- it uses a
plain "regval * 1000" on read and "val / 1000" on write, i.e. the math
for the alternate/integer format that isn't enabled.

As far as I can tell the alert-threshold registers are in that same
shifted format (the datasheet doesn't give a separate worked example for
them, but they're compared against PECI temperatures in the current data
format, and CONFIG2/offset is explicitly "represented in the current data
format"). If so, the threshold is programmed 64x too low: a 95 C crit
stores 0x005f, which the device interprets as 0x5f/64 ~= 1.5 C, so ALERT
can assert almost immediately. It round-trips through sysfs (the read
undoes the write), which hides it from a simple read-after-write check.

So this is a scaling bug rather than the clamp/+128 wrap I mentioned
earlier -- with the shift in place, 128 C encodes as 0x2000 and there is
no wrap.

I'll send a patch scaling the crit read/write like the offset path. It
builds on the sign-extension fix in this patch, so I'd send it as a
follow-up on top -- happy to fold both crit changes into a v2 instead if
you'd prefer. Confirmation from someone with the hardware would be
welcome, since I've only verified this against the datasheet.

The 1/64 C precision point stands as a separate enhancement.

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

[Guenter Roeck]

On 8/8/26 02:38, Nguyễn Công wrote:

> The 1/64 C precision point stands as a separate enhancement.
> 

Maybe I am missing something, but I don't see any support for 1/64C precision
in the datasheet.

Guenter

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