Re: [PATCH 11/12] hwmon: spd5118: Add I3C support

On Wed, 18 Mar 2026 11:53:49 -0700, Guenter Roeck wrote:
> On 3/18/26 10:27, Akhil R wrote:
>> Add a regmap config and a probe function to support for I3C based
>> communication to SPD5118 devices.
>> 
>> On an I3C bus, SPD5118 are enumerated via SETAASA and always require an
>> ACPI or device tree entry. The device matching is hence through the OF
>> match tables only and do not need an I3C class match table. The device
>> identity is verified in the type registers before proceeding to the
>> common probe function.
>> 
>> Signed-off-by: Akhil R <[email protected]>
>> ---
>>   drivers/hwmon/Kconfig   |  7 +++--
>>   drivers/hwmon/spd5118.c | 66 ++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 70 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 8af80e17d25e..23604c05ad22 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2300,10 +2300,13 @@ config SENSORS_SPD5118
>>   	tristate "SPD5118 Compliant Temperature Sensors"
>>   	depends on I2C
>>   	select REGMAP_I2C
> 
> I also had
> 	depends on I3C || I3C=n
> in my version at
> 
> https://patchwork.kernel.org/project/linux-hwmon/patch/[email protected]/
> 
> which I guess matches the more recent "depends on I3C_OR_I2C".

Ack. Will update.

> 
>> +	select REGMAP_I3C if I3C
>>   	help
>>   	  If you say yes here you get support for SPD5118 (JEDEC JESD300)
>> -	  compliant temperature sensors. Such sensors are found on DDR5 memory
>> -	  modules.
>> +	  compliant temperature sensors using I2C or I3C bus interface.
>> +	  Such sensors are found on DDR5 memory modules.
>> +
>> +	  This driver supports both I2C and I3C interfaces.
>>   
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called spd5118.
>> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
>> index 5da44571b6a0..d70123e10616 100644
>> --- a/drivers/hwmon/spd5118.c
>> +++ b/drivers/hwmon/spd5118.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/bits.h>
>>   #include <linux/err.h>
>>   #include <linux/i2c.h>
>> +#include <linux/i3c/device.h>
>>   #include <linux/hwmon.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>> @@ -482,6 +483,25 @@ static const struct regmap_config spd5118_regmap16_config = {
>>   	.cache_type = REGCACHE_MAPLE,
>>   };
>>   
>> +/*
>> + * I3C uses 2-byte register addressing -
>> + *   Byte 1: MemReg | BlkAddr[0] | Address[5:0]
>> + *   Byte 2: 0000   | BlkAddr[4:1]
>> + *
>> + * The low byte carries the register/NVM address and the high byte carries the
>> + * upper block address bits, so little-endian format is required. No range
>> + * config is needed since I3C does not use MR11 page switching.
>> + */
>> +static const struct regmap_config spd5118_regmap_i3c_config = {
>> +	.reg_bits = 16,
>> +	.val_bits = 8,
>> +	.max_register = 0x7ff,
>> +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> 
> Should this be added to spd5118_regmap16_config instead, or is there reason
> to assume that I2C 16-bit addressing differs from I3C addressing ?

I did not see any difference for I2C in the specification, but I assumed the
existing format would have been working and I thought not to change them.
Changing the I2C format would also require a change in the is_16bit nvmem_read
formula.

> 
>> +	.writeable_reg = spd5118_writeable_reg,
>> +	.volatile_reg = spd5118_volatile_reg,
>> +	.cache_type = REGCACHE_MAPLE,
>> +};
>> +
>>   static int spd5118_suspend(struct device *dev)
>>   {
>>   	struct spd5118_data *data = dev_get_drvdata(dev);
>> @@ -770,7 +790,51 @@ static struct i2c_driver spd5118_i2c_driver = {
>>   	.address_list	= IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? normal_i2c : NULL,
>>   };
>>   
>> -module_i2c_driver(spd5118_i2c_driver);
>> +/* I3C */
>> +
>> +static int spd5118_i3c_probe(struct i3c_device *i3cdev)
>> +{
>> +	struct device *dev = i3cdev_to_dev(i3cdev);
>> +	struct regmap *regmap;
>> +	unsigned int regval;
>> +	int err;
>> +
>> +	regmap = devm_regmap_init_i3c(i3cdev, &spd5118_regmap_i3c_config);
>> +	if (IS_ERR(regmap))
>> +		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
>> +
>> +	/* Verify this is a SPD5118 device */
>> +	err = regmap_read(regmap, SPD5118_REG_TYPE, &regval);
>> +	if (err)
>> +		return err;
>> +
>> +	if (regval != 0x51) {
>> +		dev_err(dev, "unexpected device type 0x%02x, expected 0x51\n", regval);
>> +		return -ENODEV;
>> +	}
>> +
>> +	err = regmap_read(regmap, SPD5118_REG_TYPE + 1, &regval);
>> +	if (err)
>> +		return err;
>> +
>> +	if (regval != 0x18) {
>> +		dev_err(dev, "unexpected device type 0x%02x, expected 0x18\n", regval);
>> +		return -ENODEV;
>> +	}
>> +
> 
> I don't think this should dump error messages. Also, it might be desirable
> to use a single regmap operation to read both values.

Ack. Will use regmap_bulk_read() and will remove the error dump.

> 
>> +	return spd5118_common_probe(dev, regmap, false);
> 
> Why is_16bit=false ?

We don't need the encoding formula for the nvmem address with I3C. Since it
uses little-endian, (page * 0x100 + SPD5118_EEPROM_BASE) translates to the
correct address. Or did I overlook something?

> 
>> +}
>> +
>> +static struct i3c_driver spd5118_i3c_driver = {
>> +	.driver = {
>> +		.name	= "spd5118_i3c",
>> +		.of_match_table = spd5118_of_ids,
>> +		.pm = pm_sleep_ptr(&spd5118_pm_ops),
>> +	},
>> +	.probe		= spd5118_i3c_probe,
>> +};
>> +
>> +module_i3c_i2c_driver(spd5118_i3c_driver, &spd5118_i2c_driver);
>>   
>>   MODULE_AUTHOR("René Rebe <[email protected]>");
>>   MODULE_AUTHOR("Guenter Roeck <[email protected]>");

Best Regards,
Akhil