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

["Troy Mitchell" <troy.mitchell@linux.dev>]

[-- Attachment #1.1: Type: text/plain, Size: 2670 bytes --]

On Mon, Aug 31, 2026 at 04:32:56PM +0000, sashiko-bot@kernel.org wrote:
> 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.
>
> [...]
>
> @@ -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?

This is a bug. TEMP12_TO_REG() currently produces an unaligned 8.8
fixed-point value. The register write discards bits 3:0, while the cache
retains them, so an immediate sysfs read can differ from the value read
back from the device after the next update.

I will fix this in the next version by quantizing the converted value to
the CTF2301's 0.0625 degree C resolution before using it for both the
register writes and the cache update.

                                            - Troy

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv