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 alert polarity configuration is silently ignored by the hardware because the new polarity bit is omitted from the register update mask.
- [High] For devices with 16-bit configuration registers, the polarity bit is incorrectly shifted into the wrong byte, modifying a reserved hardware bit instead of the polarity bit.
- [Low] The patch directly uses the Device Tree specific `of_property_read_bool()` API instead of the firmware-agnostic `device_property_read_bool()`.
--

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

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.

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

Does this shift target the correct byte for 16-bit registers?

Because the hardware typically expects the first transmitted byte to be the
most significant byte of its configuration register (which contains the POL bit
at bit 2), and i2c_smbus_write_word_data() transmits the least significant
byte first, the POL bit might need to remain in the LSB of the logical value
(BIT(2)).

By shifting it by 8 bits, does this target the MSB of the value, which maps
to the LSB of the hardware register, potentially modifying a reserved bit
instead of the polarity?

> +#define LM75_SAMPLE_CLEAR_MASK		(3 << 5)
>  

[ ... ]

> @@ -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, "ti,alert-polarity-active-high")) {

Could we use device_property_read_bool() here instead?

The probe function already uses device_property_read_string() to read the label
property earlier. Hardcoding the Device Tree specific API here breaks
consistency and prevents the active-high polarity feature from working on
systems using ACPI or software nodes.

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

Are we missing an update to the clear mask?

When we pass data->params->clr_mask into lm75_write_config():

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

The regmap_update_bits() function uses clr_mask | LM75_SHUTDOWN as the mask
of bits to modify. Since the newly added polarity bit isn't included in the
mask, is the alert polarity configuration silently ignored by the hardware?

If the device tree configures the interrupt controller for an active-high
signal but the sensor remains in its default active-low state, could this
lead to an unhandled interrupt storm?

>  	if (err)
>  		return err;
>

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