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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The 16-bit polarity mask incorrectly targets a reserved bit instead of the alert polarity bit due to endianness.
- [High] Destructive overwrite of firmware-configured polarity introduces a backwards compatibility regression.
- [High] Pre-existing bug: `set_mask` default configurations are silently ignored by regmap update logic.
- [Medium] The patch contradicts and breaks the intended active-high default behavior of as6200 sensors.
--

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

hwmon: (lm75) Support active-high alert polarity

This commit adds support for configuring the active-high alert polarity for
LM75 devices using the ti,alert-polarity-active-high devicetree property. It
also enforces the default polarity if the property is missing.

> hwmon: (lm75) Support active-high alert polarity
> 
> LM75 devices supported by this driver support configurable active-high
> alert polarity. This is already documented in the devicetree description.
> Add support for it to the driver.
> 
> Follow documentation and defensively enforce active-low if property is
> not set. This avoids possible inconsistencies for future devices with
> wrong parametrization. No API breakage as all current devices have
> their parameters set to active-low.

Is it accurate that all current devices have their parameters set to active-low?

Looking at the device_params array, the as6200 configuration sets
.set_mask = 0x94C0, which has the positive polarity bit set. By clearing this
bit when the devicetree property is not present, could this break the
established default behavior for as6200 sensors?

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f1a1e5b888f64..af2d6cf370496 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)

Will this 16-bit mask target the correct bit?

For 16-bit devices, the driver reads the configuration register using
i2c_smbus_read_word_data(). Since LM75-compatible sensors send the Most
Significant Byte (MSB) first, the original 8-bit configuration (containing
the polarity bit) ends up in the lower 8 bits of the returned integer, while
the extended configuration is in the upper 8 bits.

By defining LM75_ALERT_POLARITY_HIGH_16_BIT as (BIT(2) << 8), does this
erroneously target a reserved bit in the extended configuration byte instead
of the actual alert polarity bit?

> +#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

[ ... ]

> @@ -762,8 +765,18 @@ 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);
> +	/* Enforce polarity active-low (default) or active-high (devicetree) */
> +	if (!data->params->config_reg_16bits)
> +		pol_mask = LM75_ALERT_POLARITY_HIGH_8_BIT;
> +	else
> +		pol_mask = LM75_ALERT_POLARITY_HIGH_16_BIT;
> +
> +	clr_mask = data->params->clr_mask | pol_mask;
> +	set_mask = data->params->set_mask & ~pol_mask;
> +	if (device_property_read_bool(dev, "ti,alert-polarity-active-high"))
> +		set_mask |= pol_mask;

By always including pol_mask in clr_mask, will this actively clear the
polarity bit for all devices without the devicetree property?

Prior to this patch, the polarity bit was not included in clr_mask, allowing
the driver to inherit an active-high polarity if it was already correctly
initialized by a bootloader. Could this new behavior break existing working
embedded setups that rely on bootloader defaults?

> +
> +	err = lm75_write_config(data, set_mask, clr_mask);

Are the set_mask bits actually being applied here?

Looking at lm75_write_config(), it uses regmap_update_bits():

drivers/hwmon/lm75.c:lm75_write_config() {
	return regmap_update_bits(data->regmap, LM75_REG_CONF,
				  clr_mask | LM75_SHUTDOWN, set_mask);
}

Since regmap_update_bits() only modifies the bits explicitly defined in its
mask argument, any bits in set_mask that are not also in clr_mask will be
silently ignored. Should the mask argument be updated to include set_mask
(e.g., clr_mask | set_mask | LM75_SHUTDOWN) to ensure all intended
configuration bits are written?

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