* Re: [PATCH 1/2] hwmon: (lm75) Fix AS6200 setup and alarm handling
[not found] ` <20260502101009.DFDF8C19425@smtp.kernel.org>
@ 2026-05-02 14:45 ` Guenter Roeck
2026-05-04 15:03 ` AW: " markus.stockhausen
0 siblings, 1 reply; 2+ messages in thread
From: Guenter Roeck @ 2026-05-02 14:45 UTC (permalink / raw)
To: sashiko, Markus Stockhausen; +Cc: linux-hwmon
On 5/2/26 03:10, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The updated set_mask of 0x10C0 is byte-swapped over I2C and will force the sensor into single-shot mode instead of the intended continuous mode.
> - [High] The new alarm reporting logic is mathematically inverted, violating the hwmon ABI by returning 0 during an active alarm.
> --
>
> commit bef39cd430ab022cea5921c7c9e935e0fe939e92
> 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 set
> mask to enable continuous conversion mode. It also modifies the alarm
> handling logic with the intention of correctly reporting alarms regardless
> of the configured polarity.
>
>> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
>> index f1a1e5b888f6..4fd4288ae502 100644
>> --- 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 */
>
> The intent described in the commit message is to clear bit 15 to disable
> single-shot mode. However, won't this new mask still force the sensor into
> single-shot mode over the wire?
>
> The LM75 driver writes LM75_REG_CONF using i2c_smbus_write_word_data(),
> which transmits data in little-endian order. The AS6200 is a big-endian I2C
> sensor. When 0x10C0 is transmitted, the lower byte (0xC0) is sent first and
> interpreted by the device as the MSB. Since 0xC0 is 11000000 in binary,
> bit 15 remains set to 1.
>
... except for the config register, which is little endian, and the first
byte sent to the chip is the LSB. No idea how to tell that to the agent.
Either case, I ended up ordering an evaluation board for as6200 and samples
for TMP112, so we'll see how this works for real once I get that.
> [ ... ]
>
>> @@ -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 logic correctly map to the hwmon ABI, which expects a value of 1
> during an active alarm?
>
> According to the sensor datasheets, the AL bit reflects the ALERT pin
> directly.
>
> If POL=0 (active low), the pin is low on alarm (AL=0), so 0 ^ 0 = 0.
> If POL=1 (active high), the pin is high on alarm (AL=1), so 1 ^ 1 = 0.
>
> In both cases, it appears this expression evaluates to 0 during an active
> alarm and 1 during normal operation, inverting the expected hwmon behavior.
>
I had another look at the datasheet. Turns out the agent is correct. The
bits must be _different_ for the alarm to be active. So this has to be
something like
!!(regval & BIT(13)) != !!(regval & BIT(2));
Sorry for that.
> Additionally, does hardcoding BIT(13) and BIT(2) solidify an underlying
> I2C byte-swapping behavior directly into the logic rather than fixing the
> root cause?
>
We can live with that.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 2+ messages in thread