Re: [PATCH v5 2/2] hwmon: temperature: add support for EMC1812

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Feb 05, 2026 at 09:09:04AM +0200, Marius Cristea wrote:
> This is the hwmon driver for Microchip EMC1812/13/14/15/33
> Multichannel Low-Voltage Remote Diode Sensor Family.
> 
> EMC1812 has one external remote temperature monitoring channel.
> EMC1813 has two external remote temperature monitoring channels.
> EMC1814 has three external remote temperature monitoring channels and
> channels 2 and 3 supports anti parallel diode.
> EMC1815 has four external remote temperature monitoring channels and
> channels 1/2  and 3/4 supports anti parallel diode.
> EMC1833 has two external remote temperature monitoring channels and
> channels 1 and 2 supports anti parallel diode.
> 
> Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> ---
...
> diff --git a/drivers/hwmon/emc1812.c b/drivers/hwmon/emc1812.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..30bbc377d592622b07a156732b71cad555987934
...
> +static int emc1812_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			 int channel, long val)
> +{
> +	struct emc1812_data *data = dev_get_drvdata(dev);
> +	unsigned int interval, regval;
> +	int convrate;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		/* Range is always -64 to 191.875°C */
> +		val = clamp_val(val, -64000, 191875);
> +		val = val + (EMC1812_TEMP_OFFSET * 1000);
> +
> +		switch (attr) {
> +		case hwmon_temp_min:
> +		case hwmon_temp_max:
> +			return emc1812_set_temp(dev, data, channel, emc1812_temp_map[attr], val);
> +		case hwmon_temp_crit:
> +			/* critical temperature limit is stored on 8bits */
> +			val = DIV_ROUND_CLOSEST(val, 1000);
> +			return regmap_write(data->regmap, emc1812_temp_crit_regs[channel], val);

AI feedback:

If the input value is 191875 (the maximum allowed by the clamp above), adding the offset gives 255875.
DIV_ROUND_CLOSEST(255875, 1000) results in 256. Since the registers are 8-bit, this will be truncated
to 0 (-64 degrees C), potentially causing an immediate critical alarm. The value should be clamped
to 255 for 8-bit registers. This also applies to emc1812_set_temp() for the internal channel (channel 0).

> +		case hwmon_temp_crit_hyst:
> +			/* critical temperature hysteresis is stored on 8bits */
> +			regval = DIV_ROUND_CLOSEST(val, 1000);
> +			return emc1812_set_hyst(data, channel, regval);

The same problem really applies to emc1812_set_hyst() as well. It clamps
the register value to (0, 256). 256 is truncated to 0. So either the clamp
in emc1812_set_hyst() is not needed (I didn't check possible value ranges)
or it is wrong.

...

> +	i2c_set_clientdata(client, data);

I don't immediately see why this would be needed.

Thanks,
Guenter