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

On 5/2/26 09:28, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?
> 

We know about that. Keep as is. If it turns out to be wrong after I get
the eval boards / samples, I'll submit a fix.

> [ ... ]
> 
>> @@ -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?
> 

Sigh. This chip is so confusing. Yes, it needs to be "==". Sorry again.

Otherwise looks good, but please remember to provide change logs.

Thanks,
Guenter