[PATCH v2 0/2] hwmon: (lm75) Fix AS6200 and config register handling

[PATCH v2 0/2] hwmon: (lm75) Fix AS6200 and config register handling

[Markus Stockhausen]

This series fixes two issues in the LM75 driver. 

1. The AMS AS6200 is not initialized as documented in the device-add-commit.
2. The lm75_write_config() skips bits during initialization.

Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>

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

[Markus Stockhausen]

The initialization of the AS6200 has two shortcomings

- The device-add-commit states "Conversion mode: continuous" but the
  the lm75_params structure uses set_mask = 0x94c0. This activates
  single shot mode (bit 15). According to the datasheet "The device
  features a single shot measurement mode if the device is in sleep
  mode (SM=1)". All this is quite contradictionary.
- It is the only device that activates polarity active-high (bit 10)

All this is paired with a undefined clear mask bug in function
lm75_write_config() that was introduced with a later refactoring
commit.

[as6200] = {
	.config_reg_16bits = true,
	.set_mask = 0x94C0,
        -> .clr_mask not defined here
	.default_resolution = 12,
...
static inline int lm75_write_config(struct lm75_data *data, u16 set_mask,
				    u16 clr_mask)
{
	return regmap_update_bits(data->regmap, LM75_REG_CONF,
				  clr_mask | LM75_SHUTDOWN, set_mask);
}

regmap_update_bits() requires clr_mask to be a superset of set_mask.
So basically all sensors with "wrong" masks like the AS6200 are not
initialized as intended.

Fix that by

- 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.
- Adapt the alarm handling so it can report the alarm correctly
  even if it is high active. This is done by comparing config register
  bit 5 and 10 (translated to 2 and 13).

This commit does not introduce any ABI breakage as the mutliple bugs
effectly drive the AS6200 in standard active-low mode.

Fixes 4b6358e ("hwmon: (lm75) Add AMS AS6200 temperature sensor")
Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
 drivers/hwmon/lm75.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index f1a1e5b888f6..724efc0e820a 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 */
 		.default_resolution = 12,
 		.default_sample_time = 125,
 		.num_sample_times = 4,
@@ -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));
 				break;
 			default:
 				return -EINVAL;
-- 
2.54.0

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

[sashiko-bot]

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

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

[Guenter Roeck]

On 5/2/26 09:28, sashiko-bot@kernel.org 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 <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?
> 

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

[PATCH v2 2/2] hwmon: (lm75) Fix configuration register writes.

[Markus Stockhausen]

Sensors configurations are defined by set and clear masks. These
do not follow a consistent "clear mask is a superset of set mask"
rule. This relaxed definition breaks lm75_write_config()

static inline int lm75_write_config(struct lm75_data *data, u16 set_mask,
				    u16 clr_mask)
{
	return regmap_update_bits(data->regmap, LM75_REG_CONF,
				  clr_mask | LM75_SHUTDOWN, set_mask);
}

Basically all bits from set_mask that are not defined in clr_mask are
dropped. Fix that by enhancing the helper to always combine clr_mask
and set_mask into the mask bits of regmap_update_bits().

Fixes: 6da24a2 ("hwmon: (lm75) Hide register size differences in regmap access functions")
Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
 drivers/hwmon/lm75.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 724efc0e820a..b8d67a9b58ec 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -353,7 +353,7 @@ static inline int lm75_write_config(struct lm75_data *data, u16 set_mask,
 				    u16 clr_mask)
 {
 	return regmap_update_bits(data->regmap, LM75_REG_CONF,
-				  clr_mask | LM75_SHUTDOWN, set_mask);
+				  clr_mask | set_mask | LM75_SHUTDOWN, set_mask);
 }
 
 static irqreturn_t lm75_alarm_handler(int irq, void *private)
-- 
2.54.0