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

On 5/2/26 12:36, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?
> 
Ignore.

>> +#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.
> 
Please use device_property_read_bool().

>> +		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?
> 
You'll have to pass the bit in clr_mask if the property is not set
for consistency.

Thanks,
Guenter