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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hwmon: lm75: incorrect bitmasks due to 16-bit byte-swap for MAX31875
- [High] hwmon: lm75: missing resolution state and hardware update for MAX31875
--

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

hwmon: (lm75) add MAX31875 support

Add support for the Maxim MAX31875 I2C temperature sensor by extending
the lm75 driver. The MAX31875 features a 16-bit configuration register,
configurable resolution (8, 9, 10, or 12 bits), and selectable conversion
rates (125ms, 250ms, 1s, 4s).

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 104149a03bad0..886f83c8b0420 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c

[ ... ]

> @@ -223,6 +224,15 @@ 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,
> +		.resolutions = (u8 []) {8, 9, 10, 12 },

[Severity: High]
Does the custom switch block for max31875 in lm75_update_interval() bypass
updating data->resolution, rendering this .resolutions mapping dead code?

Without updating data->resolution or the hardware resolution bits, the software
would truncate or misinterpret temperature data (since it remains stuck at 10
bits), and the hardware would be prevented from achieving higher sample rates.

> +		.default_sample_time = 4000,
> +		.num_sample_times = 4,
> +		.sample_times = (unsigned int []){ 125, 250, 1000, 4000 },
> +		.alarm = true,
> +	},

[ ... ]

> @@ -421,6 +431,9 @@ static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
>  			case tmp112:
>  				*val = !!(regval & BIT(13)) == !!(regval & BIT(2));
>  				break;
> +			case max31875:
> +				*val = (regval >> 7) & 0x1;

[Severity: High]
Does this correctly extract the alarm bit? The SMBus word read returns
byte-swapped data (MSB in the lower byte, LSB in the upper byte).

Because of the swap, hardware bit D7 (alarm) maps to val bit 15. Shifting
regval by 7 incorrectly extracts D15 of the hardware register instead of D7,
returning garbage state when reading temp1_alarm via sysfs.

> +				break;
>  			default:
>  				return -EINVAL;
>  			}

[ ... ]

> @@ -495,6 +508,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: High]
Does the conversion rate mask 0x0600 apply directly to the byte-swapped regval?

With the swapped value, hardware bits D10/D9 map to bits 2/1 (mask 0x0006).
Using 0x0600 mistakenly clobbers the alarm polarity bit (D2) and fails to set
the conversion rate.

Also, as mentioned above, this block bypasses the default resolution update
logic (data->resolution = data->params->resolutions[index]) and never updates
the hardware resolution bits, silently breaking the configured resolution
mapping.

> +		if (err < 0)
> +			return err;
> +		data->sample_time = data->params->sample_times[index];
> +		break;

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