Re: [PATCH v3 3/3] hwmon: (lm75) add MAX31875 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hwmon: lm75: unconditional 16-bit polarity mask corrupts max31875 conversion rate
- [Medium] hwmon: lm75: Fix incorrect bit mask for MAX31875 conversion rate
- [Medium] hwmon: lm75: max31875 state desynchronization on probe
--

commit 5136e5422ce7bd39a8f47ef8a45e61f3dcdb4cf8
Author: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

hwmon: (lm75) add MAX31875 support

This commit adds support for the Maxim MAX31875 I2C temperature sensor
by extending the lm75 driver. The new device supports a 16-bit
configuration register with configurable resolutions and selectable
conversion rates.

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 2d2d752aeac9d..cda28b9b093c1 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c

[ ... ]

> @@ -222,6 +223,14 @@ static const struct lm75_params device_params[] = {
>  		.default_resolution = 16,
>  		.default_sample_time = MSEC_PER_SEC / 20,
>  	},
> +	[max31875] = {
> +		.config_reg_16bits = true,
> +		.default_resolution = 10,
> +		.default_sample_time = 4000,
> +		.num_sample_times = 4,
> +		.sample_times = (unsigned int []){ 125, 250, 1000, 4000 },
> +		.alarm = true,
> +	},

[Severity: High]
Does setting .config_reg_16bits to true for max31875 cause a regression
where the conversion rate is corrupted during initialization?

In lm75_generic_probe(), the polarity mask is unconditionally set to BIT(10)
for 16-bit chips:

    if (!data->params->config_reg_16bits)
        pol_mask = LM75_ALERT_POLARITY_HIGH_8_BIT;
    else
        pol_mask = LM75_ALERT_POLARITY_HIGH_16_BIT;

This mask is then applied to the configuration register:

    clr_mask = data->params->clr_mask | pol_mask;
    ...
    err = lm75_write_config(data, set_mask, clr_mask);

Because the max31875 conversion rate mask includes BIT(10) and BIT(9),
could probing the max31875 forcefully clear or set the MSB of its
conversion rate, causing mutual corruption between the alarm polarity and
conversion rate settings?

[Severity: Medium]
Since .set_mask and .clr_mask are not defined here, does this result in a
state desynchronization on probe?

In lm75_generic_probe(), the software state is unconditionally initialized to
the defaults:

    data->sample_time = data->params->default_sample_time;
    data->resolution = data->params->default_resolution;

But since the configuration masks are 0, lm75_write_config() fails to
enforce these defaults on the hardware.

If a bootloader or prior system state configured the conversion rate or
resolution differently, will the driver state become desynchronized from the
actual hardware state?

[ ... ]

> @@ -494,6 +506,13 @@ static int lm75_update_interval(struct device *dev, long val)
>  			return err;
>  		data->sample_time = data->params->sample_times[index];
>  		break;
> +	case max31875:
> +		err = regmap_update_bits(data->regmap, LM75_REG_CONF,
> +					 0x0600, (3 - index) << 9);

[Severity: Medium]
Is the bit mask for the MAX31875 conversion rate correct here?

Because lm75_i2c_reg_read() uses i2c_smbus_read_word_data() for 16-bit
configuration registers, the chip's MSB is placed in the lower 8 bits of the
returned value to maintain compatibility with 8-bit standard LM75 features.

This means D10 and D9 map to bits 2 and 1 of regval.

Using the mask 0x0600 and shifting by 9 modifies bits 10 and 9 of regval,
which alters bits D2 and D1 of the chip's LSB instead of setting the
conversion rate.

Should the mask be 0x0006 and the shift be 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009115617.1976-1-johnerasmusmari.geronimo@analog.com?part=3