Re: [PATCH 2/2] hwmon: (lm75) Support active-high alert polarity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The active-high alert polarity configuration is completely ignored by the hardware because it is not added to the register update mask.
- [High] LM75_ALERT_POLARITY_HIGH_16_BIT targets the wrong byte of the configuration register.
- [High] Changing the AS6200 default from active-high to active-low breaks devicetree backward compatibility.
--

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

hwmon: (lm75) Support active-high alert polarity

This commit adds support for configuring active-high alert polarity based
on the devicetree property for LM75-compatible devices. It also adjusts
the default configuration value for the AS6200 to be active-low for
consistency.

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f1a1e5b888f64..4b075a7b1cd76 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -123,7 +123,9 @@ struct lm75_data {
>  
>  static const u8 lm75_sample_set_masks[] = { 0 << 5, 1 << 5, 2 << 5, 3 << 5 };
>  
> -#define LM75_SAMPLE_CLEAR_MASK	(3 << 5)
> +#define LM75_ALERT_POLARITY_HIGH_8_BIT	(BIT(2))
> +#define LM75_ALERT_POLARITY_HIGH_16_BIT	(BIT(2) << 8)

Should the 16-bit mask also use BIT(2)?

Since 16-bit configuration register accesses use i2c_smbus_read_word_data()
and i2c_smbus_write_word_data() which process the LSB first, the standard
LM75 configuration byte maps to the LSB (bits 0-7) of the 16-bit word. Using
BIT(2) << 8 seems to target the MSB instead.

> +#define LM75_SAMPLE_CLEAR_MASK		(3 << 5)
>  
>  /* The structure below stores the configuration values of the supported devices.
>   * In case of being supported multiple configurations, the default one must
> @@ -137,7 +139,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 = 0x90C0,	/* 8 sample/s, 4 CF */

Does changing this default mask break backward compatibility for existing
devicetrees using the AS6200?

Systems currently using this chip might lack the newly introduced
lm75,alert-polarity-active-high property and rely on the previous
active-high default to route alarms correctly.

[ ... ]
> @@ -762,8 +765,15 @@ static int lm75_generic_probe(struct device *dev, const char *name,
>  		return err;
>  	data->orig_conf = status;
>  
> -	err = lm75_write_config(data, data->params->set_mask,
> -				data->params->clr_mask);
> +	set_mask = data->params->set_mask;
> +	if (of_property_read_bool(dev->of_node, "lm75,alert-polarity-active-high")) {
> +		if (!data->params->config_reg_16bits)
> +			set_mask |= LM75_ALERT_POLARITY_HIGH_8_BIT;
> +		else
> +			set_mask |= LM75_ALERT_POLARITY_HIGH_16_BIT;
> +	}
> +
> +	err = lm75_write_config(data, set_mask, data->params->clr_mask);

Will the new polarity bits added to set_mask be ignored by the hardware?

Looking at lm75_write_config(), the clr_mask is passed as the mask argument
to regmap_update_bits():

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);
}

Because clr_mask is not updated to include the polarity bits,
regmap_update_bits() won't modify them in the hardware register.

Would it also be safer to explicitly include the polarity bit in the
clr_mask so the hardware is deterministically reset from whatever state
the bootloader left it in?

>  	if (err)
>  		return err;

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