Re: [PATCH 11/12] hwmon: spd5118: Add I3C support
From: Guenter Roeck
Date: Thu Mar 19 2026 - 10:45:06 EST
On 3/18/26 21:35, Akhil R wrote:
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 <akhilrajeev@xxxxxxxxxx>
---
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/20250419161356.2528986-6-linux@xxxxxxxxxxxx/
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, ®val);
+ 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, ®val);
+ 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?
Testing of the 16-bit code was limited: I had to set the SPD on a system
manually to 16-bit mode to get it working, and that only worked until the system
was reset. Its whole point was to prepare for I3C mode. If that fails, the entire
16-bit code in the driver is potentially wrong and should be pulled out before
adding I3C code. It can be added back later if/when a system actually utilizing
it is found.
Thanks,
Guenter
+}
+
+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 <rene@xxxxxxxxxxxx>");
MODULE_AUTHOR("Guenter Roeck <linux@xxxxxxxxxxxx>");
Best Regards,
Akhil