Re: [PATCH 2/2] i2c: i801: don't instantiate spd5118 if SPD Write Disable is set

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

On 4/23/25 20:35, Yo-Jung (Leo) Lin wrote:
> If SPD Write Disable bit in the SMBus is enabled, writing data to
> addresses from 0x50 to 0x57 is forbidden. This may lead to the
> following issues for spd5118 device:
> 
>    1) Writes to the sensor hwmon sysfs attributes will always result
>       in ENXIO.
> 
>    2) System-wide resume, errors may occur during regcache sync,
>       resulting in the following error messages:
> 
>       kernel: spd5118 1-0050: Failed to write b = 0: -6
>       kernel: spd5118 1-0050: PM: dpm_run_callback(): spd5118_resume [spd5118] returns -6
>       kernel: spd5118 1-0050: PM: failed to resume async: error -6
> 
>    3) nvmem won't be usable, because writing to the page selector becomes
>       impossible.
> 
> Also, spd5118 from some manufacturers may set the page to a value != 0

It is the BIOS vendor, not the spd5118 chip vendor.

> after a sensor reset. This will make the sensor not functional unless

board reset

> its MR11 register can be changed, which is impossible due to writes being
> disabled.
> 
> To address these issues, don't instantiate it at all if SPD Write Disable
> bit is set.
> 
> Signed-off-by: Yo-Jung (Leo) Lin <leo.lin@canonical.com>
> ---
>   drivers/i2c/i2c-smbus.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 97e833895dd7..cb7ef6a7a174 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -378,6 +378,7 @@ void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   	u16 handle;
>   	u8 common_mem_type = 0x0, mem_type;
>   	u64 mem_size;
> +	bool scan_needed = true;

I'd suggest to name the variable "instantiate", but that is really a nitpick.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

>   	const char *name;
>   
>   	while ((handle = dmi_memdev_handle(slot_count)) != 0xffff) {
> @@ -438,6 +439,7 @@ void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   	case 0x22:	/* DDR5 */
>   	case 0x23:	/* LPDDR5 */
>   		name = "spd5118";
> +		scan_needed = !write_disabled;
>   		break;
>   	default:
>   		dev_info(&adap->dev,
> @@ -461,6 +463,9 @@ void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   		addr_list[0] = 0x50 + n;
>   		addr_list[1] = I2C_CLIENT_END;
>   
> +		if (!scan_needed)
> +			continue;
> +
>   		if (!IS_ERR(i2c_new_scanned_device(adap, &info, addr_list, NULL))) {
>   			dev_info(&adap->dev,
>   				 "Successfully instantiated SPD at 0x%hx\n",
>