Re: [PATCH v2 1/2] hwmon: (lm75) Fix AS6200 setup and alarm handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch attempts to disable Single Shot mode but fails due to a misunderstanding of the sensor's endianness, leaving the device in the wrong mode.
- [High] The patch's new alarm logic uses `!=` to compare the alarm and polarity bits, which inverts the output and violates the hwmon sysfs ABI.
--

commit ce15170fb232a37df75ee23f58c0868809e4b7cb
Author: Markus Stockhausen <markus.stockhausen@gmx.de>

hwmon: (lm75) Fix AS6200 setup and alarm handling

This commit attempts to fix the AS6200 initialization by changing the
configuration mask to drive the sensor in continuous mode and active-low
setup. It also adjusts the alarm handling logic to compare the alarm bit
against the polarity bit to correctly report active alarms.

> - Change the set_mask to 0x10c0 to reflect the current active-low
>   setup properly and to drive the sensor in continous mode. This
>   takes into account that the config register is little endian and
>   the first byte sent to the chip is the LSB.

The lm75-compatible 16-bit sensors generally use a big-endian format where
the msb is sent first.

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -137,7 +137,7 @@ static const struct lm75_params device_params[] = {
>  	},
>  	[as6200] = {
>  		.config_reg_16bits = true,
> -		.set_mask = 0x94C0,	/* 8 sample/s, 4 CF, positive polarity */
> +		.set_mask = 0x10C0,	/* 8 sample/s, 4 CF */

If the i2c transfer sends the lsb of 0x10C0 first, the first byte sent
is 0xC0. 

When 0xC0 is interpreted by the chip as the msb, it sets bits 15 and 14.
Since bit 15 controls Single Shot mode, does this unintentionally keep
the device in Single Shot mode instead of enabling continuous mode?

[ ... ]

> @@ -416,7 +416,7 @@ static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
>  			switch (data->kind) {
>  			case as6200:
>  			case tmp112:
> -				*val = (regval >> 13) & 0x1;
> +				*val = !!(regval & BIT(13)) != !!(regval & BIT(2));

Does this comparison invert the expected hwmon alarm abi?

The hwmon abi expects the alarm attribute to return 1 when an alarm
is active and 0 otherwise. The alarm bit (13) reflects the physical state
of the alert pin.

If the setup is active-low (polarity bit 2 is 0) and an alarm triggers,
the pin is pulled low so bit 13 becomes 0. The expression 0 != 0 evaluates
to 0.

During normal operation, the pin is high (1), and 1 != 0 evaluates to 1.

Should this use == instead to ensure the attribute returns 1 when the
alarm condition is active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260502160533.3527619-1-markus.stockhausen@gmx.de?part=1