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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: (lm75) wrong shutdown bit used for max31875 in probe and PM callbacks
--

commit 35ce43dc6fc53387d6a1aa3a7019a45b202c43af
Author: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

hwmon: (lm75) add MAX31875 support

This commit adds support for the Analog Devices / Maxim MAX31875 I2C
temperature sensor to the lm75 hwmon driver, configuring its 16-bit
configuration register, resolution, conversion rate, and alarm bit.

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 2d2d752aeac9d..349648fd08e15 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
[ ... ]
> @@ -222,6 +223,16 @@ static const struct lm75_params device_params[] = {
>  		.default_resolution = 16,
>  		.default_sample_time = MSEC_PER_SEC / 20,
>  	},
> +	[max31875] = {
> +		.config_reg_16bits = true,
> +		.set_mask = 2 << 13,	/* 10-bit resolution */
> +		.clr_mask = 3 << 13 | 3 << 9,	/* resolution, 4 s conversion rate */
> +		.default_resolution = 10,
> +		.default_sample_time = 4000,
> +		.num_sample_times = 4,
> +		.sample_times = (unsigned int []){ 125, 250, 1000, 4000 },
> +		.alarm = true,
> +	},

[Severity: Medium]
Does the driver manipulate the wrong shutdown bit for max31875 during
probe and system suspend/resume?

For devices with config_reg_16bits set to true, lm75_i2c_reg_read() and
lm75_i2c_reg_write() use i2c_smbus_read_word_data() and
i2c_smbus_write_word_data(), placing the first I2C byte (datasheet D15:D8)
into bits 7:0 of the register value and the second I2C byte (datasheet
D7:D0) into bits 15:8.

Under this layout, MAX31875 places One-Shot (D8) at BIT(0) and Shutdown
(D0) at BIT(8), whereas LM75_SHUTDOWN is defined as 0x01 (BIT(0)).

In drivers/hwmon/lm75.c, lm75_generic_probe() calls lm75_write_config():

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 | set_mask | LM75_SHUTDOWN, set_mask);
}

And the power management callbacks in drivers/hwmon/lm75.c also use
LM75_SHUTDOWN:

static int lm75_suspend(struct device *dev)
{
	struct lm75_data *data = dev_get_drvdata(dev);

	return regmap_update_bits(data->regmap, LM75_REG_CONF, LM75_SHUTDOWN, LM75_SHUTDOWN);
}

static int lm75_resume(struct device *dev)
{
	struct lm75_data *data = dev_get_drvdata(dev);

	return regmap_update_bits(data->regmap, LM75_REG_CONF, LM75_SHUTDOWN, 0);
}

Because device_params[max31875].clr_mask does not include BIT(8) and
lm75_write_config(), lm75_suspend(), and lm75_resume() operate on
LM75_SHUTDOWN (BIT(0)), won't system suspend and resume toggle the
One-Shot bit instead of placing the chip into shutdown mode, and won't
lm75_generic_probe() fail to clear the shutdown bit if the sensor was
already in shutdown mode prior to probe?

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