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

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

Pre-existing issues:
- [Medium] The `hwmon_temp_crit_alarm` read path ignores the `channel` parameter and unconditionally clears global alerts, causing alarms to be misattributed and lost.
--

--- Patch [2]: [PATCH v2 2/2] hwmon: (max6621) fix negative temperature offset and crit readings ---
commit 0116e27b3257c1efaee364113dfc90d35d8fb869
Author: Cong Nguyen <[email protected]>

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

This commit fixes a bug where negative temperature offsets and critical alerts
were read back as large positive numbers due to missing sign extension. By
casting the register value to s16 before scaling, the read path now preserves
the sign encoded during writes.

> diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
> index 16a3a61d25326..1a644d7871431 100644
> --- a/drivers/hwmon/max6621.c
> +++ b/drivers/hwmon/max6621.c

[ ... ]

> @@ -254,7 +254,7 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  			if (ret)
>  				return ret;
>  
> -			*val = regval * 1000L;
> +			*val = (s16)regval * 1000L;
>  
>  			break;
>  		case hwmon_temp_crit_alarm:
>  			/*
>  			 * Set val to zero to recover the case, when reading
>  			 * MAX6621_TEMP_ALERT_CAUSE_REG results in for example
>  			 * MAX6621_ALERT_DIS. Reading will return with error,
>  			 * but in such case alarm should be returned as 0.
>  			 */
>  			*val = 0;
>  			ret = regmap_read(data->regmap,
>  					  MAX6621_TEMP_ALERT_CAUSE_REG,
>  					  &regval);

[ ... ]

>  			if (regval) {
>  				ret = i2c_smbus_write_byte(data->client,
>  						MAX6621_CLEAR_ALERT_REG);
>  				if (ret)
>  					return ret;
>  			}
> 
>  			*val = !!regval;
> 
>  			break;

[Severity: Medium]
This is a pre-existing issue, but does this hwmon_temp_crit_alarm read path
in max6621_read() ignore the channel parameter?

When userspace reads the tempX_crit_alarm sysfs attribute for any channel,
this code reads the global MAX6621_TEMP_ALERT_CAUSE_REG and unconditionally
clears it via MAX6621_CLEAR_ALERT_REG if an alert is active.

Could this result in misattributed alarms where a read on one channel
incorrectly reports an alarm belonging to a different channel, and causes
subsequent reads on other channels to miss their alarms because the global
alert state was already cleared?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/9d3a4f1895a47794bb359a2a32fb1ccd6a15812c.1786334038.git.congnt264@gmail.com?part=2