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@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: