Re: [PATCH v4 4/6] hwmon: (lm63) Add Sensylink CTF2301 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] TEMP12_TO_REG and software caching logic fail to align with 12-bit hardware truncation, causing sysfs cache inconsistency.
--

commit c4469f3bd5935eebd0d31fe1d782461f5b52c354
Author: Troy Mitchell <troy.mitchell@linux.dev>

hwmon: (lm63) Add Sensylink CTF2301 support

This commit adds support for the Sensylink CTF2301 to the LM63 driver. It
registers an optional thermal cooling device when the device tree node
provides #cooling-cells and implements the specific temperature scaling for
the CTF2301 chip.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
[ ... ]
> @@ -121,6 +126,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>  							    127000), 1000)
>  #define TEMP8U_TO_REG(val)	DIV_ROUND_CLOSEST(clamp_val((val), 0, \
>  							    255000), 1000)
> +#define TEMP12_FROM_REG(reg)	DIV_ROUND_CLOSEST((s16)(reg) * 1000, 256)
> +#define TEMP12_TO_REG(val)	DIV_ROUND_CLOSEST(clamp_val((val), -128000, \
> +							     127937) * 256, 1000)
>  #define TEMP11_FROM_REG(reg)	((reg) / 32 * 125)
>  #define TEMP11_TO_REG(val)	(DIV_ROUND_CLOSEST(clamp_val((val), -128000, \
>  							     127875), 125) * 32)
[ ... ]
> @@ -594,6 +692,21 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>  		return err;
>  
>  	mutex_lock(&data->update_lock);
> +	if (data->kind == ctf2301 && nr == 1) {
> +		temp = TEMP12_TO_REG(val);
> +		err = i2c_smbus_write_byte_data(client, LM63_REG_LOCAL_HIGH,
> +						temp >> 8);
> +		if (!err)
> +			err = i2c_smbus_write_byte_data(client,
> +							CTF2301_REG_LOCAL_HIGH_LSB,
> +							temp & 0xf0);
> +		if (!err)
> +			data->temp1_max = temp;

[Severity: Medium]
Does this sequence cause the sysfs cache to become inconsistent with the
hardware state?

The TEMP12_TO_REG macro scales the value but does not align or quantize it
to a 12-bit hardware boundary, leaving fractional bits present in the lowest
4 bits. Here in set_temp8, those lowest 4 bits of temp are masked out with
0xf0 when writing to the hardware register, but the unmasked temp is saved in
data->temp1_max.

If a user writes a temperature limit that is not a perfect multiple of 0.0625
degrees Celsius, will this cause the sysfs interface to return the
un-truncated value immediately after the write, but later shift to the
truncated value upon the next device poll?

> +		mutex_unlock(&data->update_lock);
> +
> +		return err ? err : count;
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-ctl2301-v4-0-b335d52a0570@linux.dev?part=4