hwmon: Add support for SPD5118 compliant temperature sensors

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30 (Corsair Venegance DDR5).

René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as author.

  Documentation/hwmon/index.rst   |   1 +
  Documentation/hwmon/spd5118.rst |  60 ++++
  drivers/hwmon/Kconfig           |  12 +
  drivers/hwmon/Makefile          |   1 +
  drivers/hwmon/spd5118.c         | 482 ++++++++++++++++++++++++++++++++
  5 files changed, 556 insertions(+)

create mode 100644 Documentation/hwmon/spd5118.rst create mode 100644 drivers/hwmon/spd5118.c

Documentation/hwmon/index.rst +1 -0

index 03d313af469a..6e7b8726b60c 100644
@@ -215,6 +215,7 @@ Hardware Monitoring Kernel Drivers
     smsc47m192
     smsc47m1
     sparx5-temp
+   spd5118
     stpddc60
     surface_fan
     sy7636a-hwmon

Documentation/hwmon/spd5118.rst new file +60 -0

new file mode 100644
index 000000000000..67e990551a8a
@@ -0,0 +1,60 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver spd5118
+=====================
+
+Supported chips:
+
+  * SPD5118 (JEDEC JESD300-5B.01) compliant temperature sensor chips
+
+    JEDEC standard download:
+	https://www.jedec.org/standards-documents/docs/jesd300-5b01
+	(account required)
+
+
+    Prefix: 'spd5118'
+
+    Addresses scanned: I2C 0x50 - 0x57
+
+Author:
+	Guenter Roeck <[email protected]>
+
+
+Description
+-----------
+
+This driver implements support for SPD5118 (JEDEC JESD300-5B.01) compliant
+temperature sensors, which are used on many DDR5 memory modules. Some systems
+use the sensor to prevent memory overheating by automatically throttling
+the memory controller.
+
+The driver auto-detects SPD5118 compliant chips, but can also be instantiated
+using devicetree/firmware nodes.
+
+A SPD5118 compliant chip supports a single temperature sensor. Critical minimum,
+minimum, maximum, and critical temperature can be configured. There are alarms
+for low critical, low, high, and critical thresholds.
+
+
+PEC configuration
+-----------------
+
+If the I2C/SMBus controller supports PEC, it can be enabled or disabled using
+the 'pec' sysfs attribute attached to the i2c device.
+
+
+Hardware monitoring sysfs entries
+---------------------------------
+
+======================= ==================================
+temp1_input		Temperature (RO)
+temp1_lcrit		Low critical high temperature (RW)
+temp1_min		Minimum temperature (RW)
+temp1_max		Maximum temperature (RW)
+temp1_crit		Critical high temperature (RW)
+
+temp1_lcrit_alarm	Temperature low critical alarm
+temp1_min_alarm		Temperature low alarm
+temp1_max_alarm		Temperature high alarm
+temp1_crit_alarm	Temperature critical alarm
+======================= ===========================================

drivers/hwmon/Kconfig +12 -0

index 7f384a2494c9..111d05718b89 100644
@@ -2182,6 +2182,18 @@ config SENSORS_INA3221
  	  This driver can also be built as a module. If so, the module
  	  will be called ina3221.
 
+config SENSORS_SPD5118
+	tristate "SPD5118 Compliant Temperature Sensors"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for SPD5118 (JEDEC JESD300-5B)
+	  compliant temperature sensors. Such sensors are found on DDR5 memory
+	  modules.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called spd5118.
+
  config SENSORS_TC74
  	tristate "Microchip TC74"
  	depends on I2C

drivers/hwmon/Makefile +0 -0

index e3f25475d1f0..07c593fae9a3 100644

drivers/hwmon/Makefile +0 -0

  obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
  obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
  obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o

drivers/hwmon/spd5118.c new file +482 -0

new file mode 100644
index 000000000000..440503d09d13
@@ -0,0 +1,482 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Jedec 5118 compliant temperature sensors
+ *
+ * Derived from https://github.com/Steve-Tech/SPD5118-DKMS
+ * Originally from T/2 driver at https://t2sde.org/packages/linux
+ *	Copyright (c) 2023 René Rebe, ExactCODE GmbH; Germany.
+ *
+ * Copyright (c) 2024 Guenter Roeck
+ *
+ * Inspired by ee1004.c and jc42.c.
+ *
+ * SPD5118 compliant temperature sensors are typically used on DDR5
+ * memory modules.
+ */
+
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = {
+	0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, I2C_CLIENT_END };
+
+/* SPD5118 registers. */
+#define SPD5118_REG_TYPE		0x00	/* MR0:MR1 */
+#define SPD5118_REG_REVISION		0x02	/* MR2 */
+#define SPD5118_REG_VENDOR		0x03	/* MR3:MR4 */
+#define SPD5118_REG_CAPABILITY		0x05	/* MR5 */
+#define SPD5118_REG_I2C_LEGACY_MODE	0x0B	/* MR11 */
+#define SPD5118_REG_TEMP_CLR		0x13	/* MR19 */
+#define SPD5118_REG_ERROR_CLR		0x14	/* MR20 */
+#define SPD5118_REG_TEMP_CONFIG		0x1A	/* MR26 */
+#define SPD5118_REG_TEMP_MAX		0x1c	/* MR28:MR29 */
+#define SPD5118_REG_TEMP_MIN		0x1e	/* MR30:MR31 */
+#define SPD5118_REG_TEMP_CRIT		0x20	/* MR32:MR33 */
+#define SPD5118_REG_TEMP_LCRIT		0x22	/* MR34:MR35 */
+#define SPD5118_REG_TEMP		0x31	/* MR49:MR50 */
+#define SPD5118_REG_TEMP_STATUS		0x33	/* MR51 */
+
+#define SPD5118_TEMP_STATUS_HIGH	BIT(0)
+#define SPD5118_TEMP_STATUS_LOW		BIT(1)
+#define SPD5118_TEMP_STATUS_CRIT	BIT(2)
+#define SPD5118_TEMP_STATUS_LCRIT	BIT(3)
+
+#define SPD5118_CAP_TS_SUPPORT		BIT(1)	/* temperature sensor support */
+
+#define SPD5118_TS_DISABLE		BIT(0)	/* temperature sensor disable */
+
+/* Temperature unit in millicelsius */
+#define SPD5118_TEMP_UNIT		(MILLIDEGREE_PER_DEGREE / 4)
+/* Representable temperature range in millicelsius */
+#define SPD5118_TEMP_RANGE_MIN		-256000
+#define SPD5118_TEMP_RANGE_MAX		255750
+
+static int spd5118_temp_from_reg(u16 reg)
+{
+	int temp = sign_extend32(reg >> 2, 10);
+
+	return temp * SPD5118_TEMP_UNIT;
+}
+
+static u16 spd5118_temp_to_reg(long temp)
+{
+	temp = clamp_val(temp, SPD5118_TEMP_RANGE_MIN, SPD5118_TEMP_RANGE_MAX);
+	return (DIV_ROUND_CLOSEST(temp, SPD5118_TEMP_UNIT) & 0x7ff) << 2;
+}
+
+static int spd5118_read_temp(struct regmap *regmap, u32 attr, long *val)
+{
+	int reg, err;
+	u8 regval[2];
+	u16 temp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = SPD5118_REG_TEMP;
+		break;
+	case hwmon_temp_max:
+		reg = SPD5118_REG_TEMP_MAX;
+		break;
+	case hwmon_temp_min:
+		reg = SPD5118_REG_TEMP_MIN;
+		break;
+	case hwmon_temp_crit:
+		reg = SPD5118_REG_TEMP_CRIT;
+		break;
+	case hwmon_temp_lcrit:
+		reg = SPD5118_REG_TEMP_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_bulk_read(regmap, reg, regval, 2);
+	if (err)
+		return err;
+
+	temp = (regval[1] << 8) | regval[0];
+
+	*val = spd5118_temp_from_reg(temp);
+	return 0;
+}
+
+static int spd5118_read_alarm(struct regmap *regmap, u32 attr, long *val)
+{
+	unsigned int mask, regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_temp_max_alarm:
+		mask = SPD5118_TEMP_STATUS_HIGH;
+		break;
+	case hwmon_temp_min_alarm:
+		mask = SPD5118_TEMP_STATUS_LOW;
+		break;
+	case hwmon_temp_crit_alarm:
+		mask = SPD5118_TEMP_STATUS_CRIT;
+		break;
+	case hwmon_temp_lcrit_alarm:
+		mask = SPD5118_TEMP_STATUS_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(regmap, SPD5118_REG_TEMP_STATUS, &regval);
+	if (err < 0)
+		return err;
+	*val = !!(regval & mask);
+	if (*val)
+		return regmap_write(regmap, SPD5118_REG_TEMP_CLR, mask);
+	return 0;
+}
+
+static int spd5118_read_enable(struct regmap *regmap, u32 attr, long *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
+	if (err < 0)
+		return err;
+	*val = !(regval & SPD5118_TS_DISABLE);
+	return 0;
+}
+
+static int spd5118_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_lcrit:
+		return spd5118_read_temp(regmap, attr, val);
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_lcrit_alarm:
+		return spd5118_read_alarm(regmap, attr, val);
+	case hwmon_temp_enable:
+		return spd5118_read_enable(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int spd5118_write_temp(struct regmap *regmap, u32 attr, long val)
+{
+	u8 regval[2];
+	u16 temp;
+	int reg;
+
+	switch (attr) {
+	case hwmon_temp_max:
+		reg = SPD5118_REG_TEMP_MAX;
+		break;
+	case hwmon_temp_min:
+		reg = SPD5118_REG_TEMP_MIN;
+		break;
+	case hwmon_temp_crit:
+		reg = SPD5118_REG_TEMP_CRIT;
+		break;
+	case hwmon_temp_lcrit:
+		reg = SPD5118_REG_TEMP_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	temp = spd5118_temp_to_reg(val);
+	regval[0] = temp & 0xff;
+	regval[1] = temp >> 8;
+
+	return regmap_bulk_write(regmap, reg, regval, 2);
+}
+
+static int spd5118_write_enable(struct regmap *regmap, u32 attr, long val)
+{
+	if (val && val != 1)
+		return -EINVAL;
+
+	return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
+				  SPD5118_TS_DISABLE,
+				  val ? 0 : SPD5118_TS_DISABLE);
+}
+
+static int spd5118_temp_write(struct regmap *regmap, u32 attr, long val)
+{
+	switch (attr) {
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_lcrit:
+		return spd5118_write_temp(regmap, attr, val);
+	case hwmon_temp_enable:
+		return spd5118_write_enable(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		return spd5118_temp_write(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	if (type != hwmon_temp)
+		return 0;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		return 0444;
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+	case hwmon_temp_lcrit:
+	case hwmon_temp_crit:
+	case hwmon_temp_enable:
+		return 0644;
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_lcrit_alarm:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static inline bool spd5118_parity8(u8 w)
+{
+	w ^= w >> 4;
+	return (0x6996 >> (w & 0xf)) & 1;
+}
+
+/*
+ * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
+ * Vendor IDs 0 and 0x7f are invalid.
+ * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs.
+ */
+static bool spd5118_vendor_valid(u8 bank, u8 id)
+{
+	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
+		return false;
+
+	id &= 0x7f;
+	return id && id != 0x7f;
+}
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int regval;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+	if (regval != 0x5118)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
+	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
+	if (regval < 0)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
+	if (regval)
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
+	if (regval)
+		return -ENODEV;
+
+	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
+	if (regval < 0 || (regval & 0xc1))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
+	if (regval < 0)
+		return -ENODEV;
+	if (regval & ~SPD5118_TS_DISABLE)
+		return -ENODEV;
+
+	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
+	return 0;
+}
+
+static const struct hwmon_channel_info *spd5118_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT |
+			   HWMON_T_LCRIT | HWMON_T_LCRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+			   HWMON_T_ENABLE),
+	NULL
+};
+
+static const struct hwmon_ops spd5118_hwmon_ops = {
+	.is_visible = spd5118_is_visible,
+	.read = spd5118_read,
+	.write = spd5118_write,
+};
+
+static const struct hwmon_chip_info spd5118_chip_info = {
+	.ops = &spd5118_hwmon_ops,
+	.info = spd5118_info,
+};
+
+static bool spd5118_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SPD5118_REG_TEMP_CLR:
+	case SPD5118_REG_TEMP_CONFIG:
+	case SPD5118_REG_TEMP_MAX:
+	case SPD5118_REG_TEMP_MAX + 1:
+	case SPD5118_REG_TEMP_MIN:
+	case SPD5118_REG_TEMP_MIN + 1:
+	case SPD5118_REG_TEMP_CRIT:
+	case SPD5118_REG_TEMP_CRIT + 1:
+	case SPD5118_REG_TEMP_LCRIT:
+	case SPD5118_REG_TEMP_LCRIT + 1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SPD5118_REG_TEMP_CLR:
+	case SPD5118_REG_ERROR_CLR:
+	case SPD5118_REG_TEMP:
+	case SPD5118_REG_TEMP + 1:
+	case SPD5118_REG_TEMP_STATUS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config spd5118_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = SPD5118_REG_TEMP_STATUS,
+	.writeable_reg = spd5118_writeable_reg,
+	.volatile_reg = spd5118_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int spd5118_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	unsigned int regval, revision, vendor, bank;
+	struct device *hwmon_dev;
+	struct regmap *regmap;
+	int err;
+
+	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
+
+	err = regmap_read(regmap, SPD5118_REG_CAPABILITY, &regval);
+	if (err)
+		return err;
+	if (!(regval & SPD5118_CAP_TS_SUPPORT))
+		return -ENODEV;
+
+	err = regmap_read(regmap, SPD5118_REG_REVISION, &revision);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, SPD5118_REG_VENDOR, &bank);
+	if (err)
+		return err;
+	err = regmap_read(regmap, SPD5118_REG_VENDOR + 1, &vendor);
+	if (err)
+		return err;
+	if (!spd5118_vendor_valid(bank, vendor))
+		return -ENODEV;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
+							 regmap, &spd5118_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	/*
+	 * From JESD300-5B
+	 *   MR2 bits [5:4]: Major revision, 1..4
+	 *   MR2 bits [3:1]: Minor revision, 0..8? Probably a typo, assume 1..8
+	 */
+	dev_info(dev, "DDR5 temperature sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
+		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1, ((revision >> 1) & 0x07) + 1);
+
+	return 0;
+}
+
+static const struct i2c_device_id spd5118_id[] = {
+	{ "spd5118", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, spd5118_id);
+
+static const struct of_device_id spd5118_of_ids[] = {
+	{ .compatible = "jedec,spd5118", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, spd5118_of_ids);
+
+static struct i2c_driver spd5118_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "spd5118",
+		.of_match_table = spd5118_of_ids,
+	},
+	.probe		= spd5118_probe,
+	.id_table	= spd5118_id,
+	.detect		= spd5118_detect,
+	.address_list	= normal_i2c,
+};
+
+module_i2c_driver(spd5118_driver);
+
+MODULE_AUTHOR("René Rebe <[email protected]>");
+MODULE_AUTHOR("Guenter Roeck <[email protected]>");
+MODULE_DESCRIPTION("SPD 5118 driver");
+MODULE_LICENSE("GPL");

Quoted 1 line

+obj-$(CONFIG_SENSORS_SPD51118)    += spd5118.o

Hi,

thank you for working on this, i am currently testing the driver on my machine. I already noticed the kconfig option is wrong, the correct one would be CONFIG_SENSORS_SPD5118.

On 5/30/24 01:08, Armin Wolf wrote:
[ ... ]

Quoted 2 lines

Quoted 1 line

+obj-$(CONFIG_SENSORS_SPD51118)    += spd5118.o

Quoted 5 lines

Hi,

thank you for working on this, i am currently testing the driver on my machine. I already noticed the kconfig option is wrong, the correct one would be CONFIG_SENSORS_SPD5118.

Oops. Thanks for noticing!

Quoted 6 lines · Armin Wolf

[ ... ]

Quoted 4 lines

Quoted 1 line

+obj-$(CONFIG_SENSORS_SPD51118)    += spd5118.o

Hi,

Quoted 8 lines

Quoted 5 lines

thank you for working on this, i am currently testing the driver on my machine.
I already noticed the kconfig option is wrong, the correct one would be CONFIG_SENSORS_SPD5118.

Oops. Thanks for noticing!

I fixed this up. I'll send v2 in a couple of days, probably early next week.

If it is not too much trouble, could you send me a register dump ?

Thanks,
Guenter

Quoted 1 line

# i2cdump 1 0x51
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 51 18 0a 86 32 03 32 00 00 00 00 00 ff 3c 00 00    Q???2?2......<..
10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00    ............p?..
20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00    P?..............
30: 00 c0 01 00 00 00 00 00 00 00 00 00 00 00 00 00    .??.............
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

80: 30 10 12 02 04 00 40 42 00 00 00 00 b2 12 0d 00    0????.@B....???.
90: 00 00 00 00 a0 01 f2 03 7a 0d 00 00 00 00 80 3e    ....????z?....?>
a0: 80 3e 80 3e 00 7d 80 bb 30 75 27 01 a0 00 82 00    ?>?>.}??0u'??.?.
b0: 00 00 00 00 00 00 d4 00 00 00 d4 00 00 00 d4 00    ......?...?...?.
c0: 00 00 d4 00 00 00 88 13 08 88 13 08 20 4e 20 10    ..?...?????? N ?
d0: 27 10 1a 41 28 10 27 10 c4 09 04 4c 1d 0c 00 00    '??A(?'????L??..
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

Hi Armin,

On 5/30/24 09:45, Armin Wolf wrote:
[ ... ]

Quoted 11 lines

Quoted 1 line

# i2cdump 1 0x51
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
00: 51 18 0a 86 32 03 32 00 00 00 00 00 ff 3c 00 00 Q???2?2......<..
10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
30: 00 c0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................

Quoted 9 lines

80: 30 10 12 02 04 00 40 42 00 00 00 00 b2 12 0d 00 0????.@B....???.
90: 00 00 00 00 a0 01 f2 03 7a 0d 00 00 00 00 80 3e ....????z?....?>
a0: 80 3e 80 3e 00 7d 80 bb 30 75 27 01 a0 00 82 00 ?>?>.}??0u'??.?.
b0: 00 00 00 00 00 00 d4 00 00 00 d4 00 00 00 d4 00 ......?...?...?.
c0: 00 00 d4 00 00 00 88 13 08 88 13 08 20 4e 20 10 ..?...?????? N ?
d0: 27 10 1a 41 28 10 27 10 c4 09 04 4c 1d 0c 00 00 '??A(?'????L??..
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................

Thanks a lot. This is again Montage Technology's M88SPD5118. What is your DDR module vendor ?

Thanks,
Guenter

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---

drivers/hwmon/spd5118.c new file +21 -0

+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int regval;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+	if (regval != 0x5118)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
+	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
+	if (regval < 0)
+		return -ENODEV;

drivers/hwmon/spd5118.c new file +14 -0

+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
+	if (regval < 0 || (regval & 0xc1))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
+	if (regval < 0)
+		return -ENODEV;
+	if (regval & ~SPD5118_TS_DISABLE)
+		return -ENODEV;
+
+	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
+	return 0;
+}

Quoted 6 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---

<snip>

Quoted 21 lines

drivers/hwmon/spd5118.c new file +21 -0

+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int regval;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+	if (regval != 0x5118)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
+	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
+	if (regval < 0)
+		return -ENODEV;

drivers/hwmon/spd5118.c line 323

Quoted 10 lines

drivers/hwmon/spd5118.c new file +10 -0

+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
+	if (regval)
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
+	if (regval)
+		return -ENODEV;
+
+	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
+		return -ENODEV;

This breaks automatic detection for me.

I think the test should after the read of SPD5118_REG_CAPABILITY and test that register, similar on how it is done in _probe().

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30 (Corsair Venegance DDR5).

René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as author.

 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/spd5118.rst |  60 ++++
 drivers/hwmon/Kconfig           |  12 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/spd5118.c         | 482 ++++++++++++++++++++++++++++++++
 5 files changed, 556 insertions(+)
 create mode 100644 Documentation/hwmon/spd5118.rst
 create mode 100644 drivers/hwmon/spd5118.c

Quoted 13 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30 (Corsair Venegance DDR5).

René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as author.

Quoted 8 lines

  Documentation/hwmon/index.rst   |   1 +
  Documentation/hwmon/spd5118.rst |  60 ++++
  drivers/hwmon/Kconfig           |  12 +
  drivers/hwmon/Makefile          |   1 +
  drivers/hwmon/spd5118.c         | 482 ++++++++++++++++++++++++++++++++
  5 files changed, 556 insertions(+)

create mode 100644 Documentation/hwmon/spd5118.rst create mode 100644 drivers/hwmon/spd5118.c

With the Makefile and detect callback fixed:

Reviewed-by Thomas Weißschuh <[email protected]>

Tested-by Thomas Weißschuh <[email protected]>

Quoted 25 lines · Thomas Weißschuh

Quoted 13 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30 (Corsair Venegance DDR5).

René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as author.

Quoted 8 lines

  Documentation/hwmon/index.rst   |   1 +
  Documentation/hwmon/spd5118.rst |  60 ++++
  drivers/hwmon/Kconfig           |  12 +
  drivers/hwmon/Makefile          |   1 +
  drivers/hwmon/spd5118.c         | 482 ++++++++++++++++++++++++++++++++
  5 files changed, 556 insertions(+)

create mode 100644 Documentation/hwmon/spd5118.rst create mode 100644 drivers/hwmon/spd5118.c

With the Makefile and detect callback fixed:

Quoted 2 lines

Reviewed-by Thomas Weißschuh <[email protected]>

Tested-by Thomas Weißschuh <[email protected]>

Thanks a lot for the feedback!

If it is not too much trouble, could you send me a register dump ? The one I have is from Montage Technology M88SPD5118, and I'd like to get a few more to improve my module test script.

Quoted 29 lines · Guenter Roeck

Quoted 23 lines · Thomas Weißschuh

Quoted 13 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30 (Corsair Venegance DDR5).

René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as author.

Quoted 8 lines

   Documentation/hwmon/index.rst   |   1 +
   Documentation/hwmon/spd5118.rst |  60 ++++
   drivers/hwmon/Kconfig           |  12 +
   drivers/hwmon/Makefile          |   1 +
   drivers/hwmon/spd5118.c         | 482 ++++++++++++++++++++++++++++++++
   5 files changed, 556 insertions(+)

create mode 100644 Documentation/hwmon/spd5118.rst create mode 100644 drivers/hwmon/spd5118.c

Quoted 4 lines

With the Makefile and detect callback fixed:

Reviewed-by Thomas Weißschuh <[email protected]>

Tested-by Thomas Weißschuh <[email protected]>

Quoted 7 lines

Thanks a lot for the feedback!

If it is not too much trouble, could you send me a register dump ? The one I have is from Montage Technology M88SPD5118, and I'd like to get a few more to improve my module test script.

From a Kingston KF556S40-32:

# i2cdump 20 0x50
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 51 18 0a 86 32 03 32 00 00 00 00 07 ff 3c 00 00    Q???2?2....?.<..
10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00    ............p?..
20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00    P?..............
30: 00 f0 01 00 00 00 00 00 00 00 00 00 00 00 00 00    .??.............
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Cc René Rebe <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30 (Corsair Venegance DDR5).

René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as author.

  Documentation/hwmon/index.rst   |   1 +
  Documentation/hwmon/spd5118.rst |  60 ++++
  drivers/hwmon/Kconfig           |  12 +
  drivers/hwmon/Makefile          |   1 +
  drivers/hwmon/spd5118.c         | 482 ++++++++++++++++++++++++++++++++
  5 files changed, 556 insertions(+)

create mode 100644 Documentation/hwmon/spd5118.rst create mode 100644 drivers/hwmon/spd5118.c

Documentation/hwmon/index.rst +1 -0

index 03d313af469a..6e7b8726b60c 100644
@@ -215,6 +215,7 @@ Hardware Monitoring Kernel Drivers
     smsc47m192
     smsc47m1
     sparx5-temp
+   spd5118
     stpddc60
     surface_fan
     sy7636a-hwmon

Documentation/hwmon/spd5118.rst new file +34 -0

new file mode 100644
index 000000000000..67e990551a8a
@@ -0,0 +1,60 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver spd5118
+=====================
+
+Supported chips:
+
+  * SPD5118 (JEDEC JESD300-5B.01) compliant temperature sensor chips
+
+    JEDEC standard download:
+	https://www.jedec.org/standards-documents/docs/jesd300-5b01
+	(account required)
+
+
+    Prefix: 'spd5118'
+
+    Addresses scanned: I2C 0x50 - 0x57
+
+Author:
+	Guenter Roeck <[email protected]>
+
+
+Description
+-----------
+
+This driver implements support for SPD5118 (JEDEC JESD300-5B.01) compliant
+temperature sensors, which are used on many DDR5 memory modules. Some systems
+use the sensor to prevent memory overheating by automatically throttling
+the memory controller.
+
+The driver auto-detects SPD5118 compliant chips, but can also be instantiated
+using devicetree/firmware nodes.
+
+A SPD5118 compliant chip supports a single temperature sensor. Critical minimum,

Documentation/hwmon/spd5118.rst new file +5 -0

+
+Hardware monitoring sysfs entries
+---------------------------------
+
+======================= ==================================

Documentation/hwmon/spd5118.rst new file +1 -0

+======================= ===========================================

drivers/hwmon/Kconfig +12 -0

index 7f384a2494c9..111d05718b89 100644
@@ -2182,6 +2182,18 @@ config SENSORS_INA3221
  	  This driver can also be built as a module. If so, the module
  	  will be called ina3221.
 
+config SENSORS_SPD5118
+	tristate "SPD5118 Compliant Temperature Sensors"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for SPD5118 (JEDEC JESD300-5B)
+	  compliant temperature sensors. Such sensors are found on DDR5 memory
+	  modules.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called spd5118.
+
  config SENSORS_TC74
  	tristate "Microchip TC74"
  	depends on I2C

drivers/hwmon/Makefile +1 -0

index e3f25475d1f0..07c593fae9a3 100644
@@ -207,6 +207,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
  obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
+obj-$(CONFIG_SENSORS_SPD51118)	+= spd5118.o
  obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
  obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
  obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o

drivers/hwmon/spd5118.c new file +207 -0

new file mode 100644
index 000000000000..440503d09d13
@@ -0,0 +1,482 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Jedec 5118 compliant temperature sensors
+ *
+ * Derived from https://github.com/Steve-Tech/SPD5118-DKMS
+ * Originally from T/2 driver at https://t2sde.org/packages/linux
+ *	Copyright (c) 2023 René Rebe, ExactCODE GmbH; Germany.
+ *
+ * Copyright (c) 2024 Guenter Roeck
+ *
+ * Inspired by ee1004.c and jc42.c.
+ *
+ * SPD5118 compliant temperature sensors are typically used on DDR5
+ * memory modules.
+ */
+
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = {
+	0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, I2C_CLIENT_END };
+
+/* SPD5118 registers. */
+#define SPD5118_REG_TYPE		0x00	/* MR0:MR1 */
+#define SPD5118_REG_REVISION		0x02	/* MR2 */
+#define SPD5118_REG_VENDOR		0x03	/* MR3:MR4 */
+#define SPD5118_REG_CAPABILITY		0x05	/* MR5 */
+#define SPD5118_REG_I2C_LEGACY_MODE	0x0B	/* MR11 */
+#define SPD5118_REG_TEMP_CLR		0x13	/* MR19 */
+#define SPD5118_REG_ERROR_CLR		0x14	/* MR20 */
+#define SPD5118_REG_TEMP_CONFIG		0x1A	/* MR26 */
+#define SPD5118_REG_TEMP_MAX		0x1c	/* MR28:MR29 */
+#define SPD5118_REG_TEMP_MIN		0x1e	/* MR30:MR31 */
+#define SPD5118_REG_TEMP_CRIT		0x20	/* MR32:MR33 */
+#define SPD5118_REG_TEMP_LCRIT		0x22	/* MR34:MR35 */
+#define SPD5118_REG_TEMP		0x31	/* MR49:MR50 */
+#define SPD5118_REG_TEMP_STATUS		0x33	/* MR51 */
+
+#define SPD5118_TEMP_STATUS_HIGH	BIT(0)
+#define SPD5118_TEMP_STATUS_LOW		BIT(1)
+#define SPD5118_TEMP_STATUS_CRIT	BIT(2)
+#define SPD5118_TEMP_STATUS_LCRIT	BIT(3)
+
+#define SPD5118_CAP_TS_SUPPORT		BIT(1)	/* temperature sensor support */
+
+#define SPD5118_TS_DISABLE		BIT(0)	/* temperature sensor disable */
+
+/* Temperature unit in millicelsius */
+#define SPD5118_TEMP_UNIT		(MILLIDEGREE_PER_DEGREE / 4)
+/* Representable temperature range in millicelsius */
+#define SPD5118_TEMP_RANGE_MIN		-256000
+#define SPD5118_TEMP_RANGE_MAX		255750
+
+static int spd5118_temp_from_reg(u16 reg)
+{
+	int temp = sign_extend32(reg >> 2, 10);
+
+	return temp * SPD5118_TEMP_UNIT;
+}
+
+static u16 spd5118_temp_to_reg(long temp)
+{
+	temp = clamp_val(temp, SPD5118_TEMP_RANGE_MIN, SPD5118_TEMP_RANGE_MAX);
+	return (DIV_ROUND_CLOSEST(temp, SPD5118_TEMP_UNIT) & 0x7ff) << 2;
+}
+
+static int spd5118_read_temp(struct regmap *regmap, u32 attr, long *val)
+{
+	int reg, err;
+	u8 regval[2];
+	u16 temp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = SPD5118_REG_TEMP;
+		break;
+	case hwmon_temp_max:
+		reg = SPD5118_REG_TEMP_MAX;
+		break;
+	case hwmon_temp_min:
+		reg = SPD5118_REG_TEMP_MIN;
+		break;
+	case hwmon_temp_crit:
+		reg = SPD5118_REG_TEMP_CRIT;
+		break;
+	case hwmon_temp_lcrit:
+		reg = SPD5118_REG_TEMP_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_bulk_read(regmap, reg, regval, 2);
+	if (err)
+		return err;
+
+	temp = (regval[1] << 8) | regval[0];
+
+	*val = spd5118_temp_from_reg(temp);
+	return 0;
+}
+
+static int spd5118_read_alarm(struct regmap *regmap, u32 attr, long *val)
+{
+	unsigned int mask, regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_temp_max_alarm:
+		mask = SPD5118_TEMP_STATUS_HIGH;
+		break;
+	case hwmon_temp_min_alarm:
+		mask = SPD5118_TEMP_STATUS_LOW;
+		break;
+	case hwmon_temp_crit_alarm:
+		mask = SPD5118_TEMP_STATUS_CRIT;
+		break;
+	case hwmon_temp_lcrit_alarm:
+		mask = SPD5118_TEMP_STATUS_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(regmap, SPD5118_REG_TEMP_STATUS, &regval);
+	if (err < 0)
+		return err;
+	*val = !!(regval & mask);
+	if (*val)
+		return regmap_write(regmap, SPD5118_REG_TEMP_CLR, mask);
+	return 0;
+}
+
+static int spd5118_read_enable(struct regmap *regmap, u32 attr, long *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
+	if (err < 0)
+		return err;
+	*val = !(regval & SPD5118_TS_DISABLE);
+	return 0;
+}
+
+static int spd5118_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_lcrit:
+		return spd5118_read_temp(regmap, attr, val);
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_lcrit_alarm:
+		return spd5118_read_alarm(regmap, attr, val);
+	case hwmon_temp_enable:
+		return spd5118_read_enable(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int spd5118_write_temp(struct regmap *regmap, u32 attr, long val)
+{
+	u8 regval[2];
+	u16 temp;
+	int reg;
+
+	switch (attr) {
+	case hwmon_temp_max:
+		reg = SPD5118_REG_TEMP_MAX;
+		break;
+	case hwmon_temp_min:
+		reg = SPD5118_REG_TEMP_MIN;
+		break;
+	case hwmon_temp_crit:
+		reg = SPD5118_REG_TEMP_CRIT;
+		break;
+	case hwmon_temp_lcrit:
+		reg = SPD5118_REG_TEMP_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	temp = spd5118_temp_to_reg(val);
+	regval[0] = temp & 0xff;
+	regval[1] = temp >> 8;
+
+	return regmap_bulk_write(regmap, reg, regval, 2);

drivers/hwmon/spd5118.c new file +242 -0

+}
+
+static int spd5118_temp_write(struct regmap *regmap, u32 attr, long val)
+{
+	switch (attr) {
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_lcrit:
+		return spd5118_write_temp(regmap, attr, val);
+	case hwmon_temp_enable:
+		return spd5118_write_enable(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		return spd5118_temp_write(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	if (type != hwmon_temp)
+		return 0;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		return 0444;
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+	case hwmon_temp_lcrit:
+	case hwmon_temp_crit:
+	case hwmon_temp_enable:
+		return 0644;
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_lcrit_alarm:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static inline bool spd5118_parity8(u8 w)
+{
+	w ^= w >> 4;
+	return (0x6996 >> (w & 0xf)) & 1;
+}
+
+/*
+ * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
+ * Vendor IDs 0 and 0x7f are invalid.
+ * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs.
+ */
+static bool spd5118_vendor_valid(u8 bank, u8 id)
+{
+	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
+		return false;
+
+	id &= 0x7f;
+	return id && id != 0x7f;
+}
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int regval;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+	if (regval != 0x5118)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
+	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
+	if (regval < 0)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
+	if (regval)
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
+	if (regval)
+		return -ENODEV;
+
+	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
+	if (regval < 0 || (regval & 0xc1))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
+	if (regval < 0)
+		return -ENODEV;
+	if (regval & ~SPD5118_TS_DISABLE)
+		return -ENODEV;
+
+	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
+	return 0;
+}
+
+static const struct hwmon_channel_info *spd5118_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT |
+			   HWMON_T_LCRIT | HWMON_T_LCRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+			   HWMON_T_ENABLE),
+	NULL
+};
+
+static const struct hwmon_ops spd5118_hwmon_ops = {
+	.is_visible = spd5118_is_visible,
+	.read = spd5118_read,
+	.write = spd5118_write,
+};
+
+static const struct hwmon_chip_info spd5118_chip_info = {
+	.ops = &spd5118_hwmon_ops,
+	.info = spd5118_info,
+};
+
+static bool spd5118_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SPD5118_REG_TEMP_CLR:
+	case SPD5118_REG_TEMP_CONFIG:
+	case SPD5118_REG_TEMP_MAX:
+	case SPD5118_REG_TEMP_MAX + 1:
+	case SPD5118_REG_TEMP_MIN:
+	case SPD5118_REG_TEMP_MIN + 1:
+	case SPD5118_REG_TEMP_CRIT:
+	case SPD5118_REG_TEMP_CRIT + 1:
+	case SPD5118_REG_TEMP_LCRIT:
+	case SPD5118_REG_TEMP_LCRIT + 1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SPD5118_REG_TEMP_CLR:
+	case SPD5118_REG_ERROR_CLR:
+	case SPD5118_REG_TEMP:
+	case SPD5118_REG_TEMP + 1:
+	case SPD5118_REG_TEMP_STATUS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config spd5118_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = SPD5118_REG_TEMP_STATUS,
+	.writeable_reg = spd5118_writeable_reg,
+	.volatile_reg = spd5118_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int spd5118_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	unsigned int regval, revision, vendor, bank;
+	struct device *hwmon_dev;
+	struct regmap *regmap;
+	int err;
+
+	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
+
+	err = regmap_read(regmap, SPD5118_REG_CAPABILITY, &regval);
+	if (err)
+		return err;
+	if (!(regval & SPD5118_CAP_TS_SUPPORT))
+		return -ENODEV;
+
+	err = regmap_read(regmap, SPD5118_REG_REVISION, &revision);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, SPD5118_REG_VENDOR, &bank);
+	if (err)
+		return err;
+	err = regmap_read(regmap, SPD5118_REG_VENDOR + 1, &vendor);
+	if (err)
+		return err;
+	if (!spd5118_vendor_valid(bank, vendor))
+		return -ENODEV;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
+							 regmap, &spd5118_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	/*
+	 * From JESD300-5B
+	 *   MR2 bits [5:4]: Major revision, 1..4
+	 *   MR2 bits [3:1]: Minor revision, 0..8? Probably a typo, assume 1..8
+	 */
+	dev_info(dev, "DDR5 temperature sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
+		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1, ((revision >> 1) & 0x07) + 1);
+
+	return 0;
+}
+
+static const struct i2c_device_id spd5118_id[] = {
+	{ "spd5118", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, spd5118_id);
+
+static const struct of_device_id spd5118_of_ids[] = {

drivers/hwmon/spd5118.c new file +13 -0

+	},
+	.probe		= spd5118_probe,
+	.id_table	= spd5118_id,
+	.detect		= spd5118_detect,
+	.address_list	= normal_i2c,
+};
+
+module_i2c_driver(spd5118_driver);
+
+MODULE_AUTHOR("René Rebe <[email protected]>");
+MODULE_AUTHOR("Guenter Roeck <[email protected]>");
+MODULE_DESCRIPTION("SPD 5118 driver");
+MODULE_LICENSE("GPL");

Am 29.05.24 um 22:52 schrieb Guenter Roeck:

Documentation/hwmon/spd5118.rst line 59

Quoted 5 lines

Documentation/hwmon/spd5118.rst new file +5 -0

+
+temp1_lcrit_alarm    Temperature low critical alarm
+temp1_min_alarm        Temperature low alarm
+temp1_max_alarm        Temperature high alarm
+temp1_crit_alarm    Temperature critical alarm

Maybe it would be a good idea to tell users that the alarm attributes are sticky.

drivers/hwmon/spd5118.c line 217

Quoted 8 lines

drivers/hwmon/spd5118.c new file +8 -0

+static int spd5118_write_enable(struct regmap *regmap, u32 attr, long val)
+{
+    if (val && val != 1)
+        return -EINVAL;
+
+    return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
+                  SPD5118_TS_DISABLE,
+                  val ? 0 : SPD5118_TS_DISABLE);

The spd5118 spec says that we have to wait 10ms after enabling the sensors before we start reading temperature values, maybe we need a delay + locking here?

drivers/hwmon/spd5118.c line 469

Quoted 6 lines

drivers/hwmon/spd5118.c new file +6 -0

+
+static struct i2c_driver spd5118_driver = {
+    .class        = I2C_CLASS_HWMON,
+    .driver = {
+        .name    = "spd5118",
+        .of_match_table = spd5118_of_ids,

The driver is missing suspend support, without it hibernation/S4 sleep will cause the limit and config registers to be out of sync with the regmap cache.

Quoted 2 lines

Am 29.05.24 um 22:52 schrieb Guenter Roeck:

[ ... ]

Quoted 9 lines

Documentation/hwmon/spd5118.rst line 59

Quoted 5 lines

Documentation/hwmon/spd5118.rst new file +5 -0

+
+temp1_lcrit_alarm    Temperature low critical alarm
+temp1_min_alarm        Temperature low alarm
+temp1_max_alarm        Temperature high alarm
+temp1_crit_alarm    Temperature critical alarm

Maybe it would be a good idea to tell users that the alarm attributes are sticky.

The driver auto-clears them after read, so they are only sticky in the sense
that they will remain active until read. This is quite common for hardware
monitoring devices. However, sure, I'll add a note.

[ ... ]

Quoted 6 lines

Quoted 6 lines

+static int spd5118_write_enable(struct regmap *regmap, u32 attr, long val)

drivers/hwmon/spd5118.c new file +4 -0

+{
+    if (val && val != 1)
+        return -EINVAL;
+

Quoted 9 lines

Quoted 3 lines

+    return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, +                  SPD5118_TS_DISABLE,
+                  val ? 0 : SPD5118_TS_DISABLE);

The spd5118 spec says that we have to wait 10ms after enabling the sensors before
we start reading temperature values, maybe we need a delay + locking here?

I don't think that would add much if any value but a lot of complexity for little gain. I find it acceptable that the sensor returns 0 for a few ms
after enabling it. Pretty much all chips have the same problem, so I am really not concerned about it.

Quoted 11 lines

drivers/hwmon/spd5118.c line 469

Quoted 6 lines

drivers/hwmon/spd5118.c new file +6 -0

+
+static struct i2c_driver spd5118_driver = {
+    .class        = I2C_CLASS_HWMON,
+    .driver = {
+        .name    = "spd5118",
+        .of_match_table = spd5118_of_ids,

The driver is missing suspend support, without it hibernation/S4 sleep will cause the
limit and config registers to be out of sync with the regmap cache.

Good point. Do you have a means to test this if I add suspend support ? I have not been able to figure out how to put my system into suspend.

Thanks,
Guenter

Documentation/hwmon/spd5118.rst line 44

Quoted 8 lines

Documentation/hwmon/spd5118.rst new file +8 -0

+
+
+PEC configuration
+-----------------
+
+If the I2C/SMBus controller supports PEC, it can be enabled or disabled using
+the 'pec' sysfs attribute attached to the i2c device.
+

Hi,

the spd5118 only supports PEC when in i3c basic mode, so afaik we cannot use PEC here.

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

Quoted 2 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

I can get the module to automatically probe with this change:

drivers/i2c/i2c-smbus.c +4 -0

index 97f338b123b1..8d9218f755d7 100644
@@ -382,6 +386,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
         case 0x1E:      /* LPDDR4 */
                 name = "ee1004";
                 break;
+       case 0x22:      /* DDR5 */
+       case 0x23:      /* LPDDR5 */
+               name = "spd5118";
+               break;
         default:
                 dev_info(&adap->dev,
                          "Memory type 0x%02x not supported yet, not instantiating SPD\n",

(Credits go to Paul Menzel [0])

Maybe you can add that to your series.

To also work with my PIIX4 I2C bus, I also need:

drivers/i2c/busses/Kconfig +1 -0

index fe6e8a1bb607..ff66e883b348 100644
@@ -195,6 +195,7 @@ config I2C_ISMT
  config I2C_PIIX4
         tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
         depends on PCI && HAS_IOPORT
+       select I2C_SMBUS
         help
           If you say yes to this option, support will be included for the Intel
           PIIX4 family of mainboard I2C interfaces.  Specifically, the following

drivers/i2c/busses/i2c-piix4.c +3 -0

index 6a0392172b2f..f8d81f8c0cb3 100644
@@ -29,6 +29,7 @@
  #include <linux/stddef.h>
  #include <linux/ioport.h>
  #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
  #include <linux/slab.h>
  #include <linux/dmi.h>
  #include <linux/acpi.h>

@@ -982,6 +983,8 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
                 return retval;
         }
 
+       i2c_register_spd(adap);
+
         *padap = adap;
         return 0;
  }

Though I guess it's not the right place to call i2c_register_sdp(), I'll look at it some more and then submit it.

Quoted 24 lines · Thomas Weißschuh

Quoted 2 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

I can get the module to automatically probe with this change:

drivers/i2c/i2c-smbus.c +4 -0

index 97f338b123b1..8d9218f755d7 100644
@@ -382,6 +386,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
         case 0x1E:      /* LPDDR4 */
                 name = "ee1004";
                 break;
+       case 0x22:      /* DDR5 */
+       case 0x23:      /* LPDDR5 */
+               name = "spd5118";
+               break;
         default:
                 dev_info(&adap->dev,
                          "Memory type 0x%02x not supported yet, not instantiating SPD\n",

(Credits go to Paul Menzel [0])

Quoted 2 lines

Maybe you can add that to your series.

That is specifically for SPD (eeprom) support, which I didn't provide in the driver. It does not register the equivalent jc42.4 temperature sensor either. Given that, using the code to register a temperature sensor seems inappropriate.

I didn't include accessing the SPD eeprom to the driver because I don't have a use case. I don't mind adding it, though, if others think that it is important.

Quoted 36 lines

To also work with my PIIX4 I2C bus, I also need:

drivers/i2c/busses/Kconfig +1 -0

index fe6e8a1bb607..ff66e883b348 100644
@@ -195,6 +195,7 @@ config I2C_ISMT
  config I2C_PIIX4
         tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
         depends on PCI && HAS_IOPORT
+       select I2C_SMBUS
         help
           If you say yes to this option, support will be included for the Intel
           PIIX4 family of mainboard I2C interfaces.  Specifically, the following

drivers/i2c/busses/i2c-piix4.c +3 -0

index 6a0392172b2f..f8d81f8c0cb3 100644
@@ -29,6 +29,7 @@
  #include <linux/stddef.h>
  #include <linux/ioport.h>
  #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
  #include <linux/slab.h>
  #include <linux/dmi.h>
  #include <linux/acpi.h>

@@ -982,6 +983,8 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
                 return retval;
         }
 
+       i2c_register_spd(adap);
+
         *padap = adap;
         return 0;
  }

Quoted 3 lines

Though I guess it's not the right place to call i2c_register_sdp(), I'll look at it some more and then submit it.

Hmm, I didn't find a better place though.

Please copy me when you submit a patch; I can test it on an AMD system with DDR4.

Quoted 25 lines · Guenter Roeck

Quoted 24 lines · Thomas Weißschuh

Quoted 2 lines · Guenter Roeck

Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature sensors. Such sensors are typically found on DDR5 memory modules.

I can get the module to automatically probe with this change:

drivers/i2c/i2c-smbus.c +4 -0

index 97f338b123b1..8d9218f755d7 100644
@@ -382,6 +386,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
          case 0x1E:      /* LPDDR4 */
                  name = "ee1004";
                  break;
+       case 0x22:      /* DDR5 */
+       case 0x23:      /* LPDDR5 */
+               name = "spd5118";
+               break;
          default:
                  dev_info(&adap->dev,
                           "Memory type 0x%02x not supported yet, not instantiating SPD\n",

(Credits go to Paul Menzel [0])

Quoted 7 lines

Quoted 2 lines

Maybe you can add that to your series.

That is specifically for SPD (eeprom) support, which I didn't provide in the driver. It does not register the equivalent jc42.4 temperature sensor either. Given that, using the code to register a temperature sensor seems inappropriate.

I see, I wasn't aware about the specifics of SPD.

It felt like a nice way to get automatic probing.
(I was wondering about that today before)

Quoted 3 lines

I didn't include accessing the SPD eeprom to the driver because I don't have a use case. I don't mind adding it, though, if others think that it is important.

Wolfgang seems to think it's important:
https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Quoted 1 line

Wolfgang seems to think it's important:

Quoted 2 lines

https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Ok, but that doesn't explain the reason. Wolfram, Paul, why do you think this is needed ? Note that I am not opposed to adding spd eeprom support, but I'd like to know why I am doing it before I spend time on it.

Hi all,

Quoted 1 line

Quoted 1 line

Wolfgang seems to think it's important:

Wolfram, please.

Quoted 7 lines

Quoted 2 lines

https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Ok, but that doesn't explain the reason. Wolfram, Paul, why do you think this is needed ? Note that I am not opposed to adding spd eeprom support, but I'd like to know why I am doing it before I spend time on it.

A working eeprom driver is needed to get 'decode-dimms' from the i2c-tools package working. Jean reported that EEPROM access for DDR5 is different from DDR4, so it needs a separate driver. And i2c_register_spd() then needs to be updated to use the new driver for DDR5.

Quoted 3 lines · Wolfram Sang

Quoted 1 line

Quoted 1 line

Wolfgang seems to think it's important:

Wolfram, please.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Ok, but that doesn't explain the reason. Wolfram, Paul, why do you think this is needed ? Note that I am not opposed to adding spd eeprom support, but I'd like to know why I am doing it before I spend time on it.

Quoted 5 lines

A working eeprom driver is needed to get 'decode-dimms' from the i2c-tools package working. Jean reported that EEPROM access for DDR5 is different from DDR4, so it needs a separate driver. And i2c_register_spd() then needs to be updated to use the new driver for DDR5.

Well my original downstream driver already had eeprom access:

	https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch

Note there are some surrounding -2, and parity patches around this patch.

Hi,

Quoted 15 lines · Wolfram Sang

Hi all,

Quoted 1 line

Quoted 1 line

Wolfgang seems to think it's important:

Wolfram, please.

Quoted 7 lines

Quoted 2 lines

https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Ok, but that doesn't explain the reason. Wolfram, Paul, why do you think this is needed ? Note that I am not opposed to adding spd eeprom support, but I'd like to know why I am doing it before I spend time on it.

Quoted 5 lines

A working eeprom driver is needed to get 'decode-dimms' from the i2c-tools package working. Jean reported that EEPROM access for DDR5 is different from DDR4, so it needs a separate driver. And i2c_register_spd() then needs to be updated to use the new driver for DDR5.

Well my original downstream driver already had eeprom access:

	https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch

Hi René,

Quoted 22 lines · René Rebe

Hi,

Quoted 20 lines · Wolfram Sang

Hi all,

Quoted 1 line

Quoted 1 line

Wolfgang seems to think it's important:

Wolfram, please.

Quoted 7 lines

Quoted 2 lines

https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Ok, but that doesn't explain the reason. Wolfram, Paul, why do you think this is needed ? Note that I am not opposed to adding spd eeprom support, but I'd like to know why I am doing it before I spend time on it.

A working eeprom driver is needed to get 'decode-dimms' from the i2c-tools package working. Jean reported that EEPROM access for DDR5 is different from DDR4, so it needs a separate driver. And i2c_register_spd() then needs to be updated to use the new driver for DDR5.

Quoted 2 lines

Well my original downstream driver already had eeprom access:
https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch

Yes, but you didn't send it upstream, so I took it, fixed a couple of bugs,

dropped eeprom support since that is secondary for my use case as well as the

out-of-tree parity code, and submitted it. I'd be more than happy to let you take over if you like.

Hi,

Quoted 30 lines · Guenter Roeck

Hi René,

Quoted 24 lines · René Rebe

Hi,

Quoted 15 lines · Wolfram Sang

Hi all,

Quoted 1 line

Quoted 1 line

Wolfgang seems to think it's important:

Wolfram, please.

Quoted 7 lines

Quoted 2 lines

https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/

Ok, but that doesn't explain the reason. Wolfram, Paul, why do you think this is needed ? Note that I am not opposed to adding spd eeprom support, but I'd like to know why I am doing it before I spend time on it.

Quoted 5 lines

A working eeprom driver is needed to get 'decode-dimms' from the i2c-tools package working. Jean reported that EEPROM access for DDR5 is different from DDR4, so it needs a separate driver. And i2c_register_spd() then needs to be updated to use the new driver for DDR5.

Well my original downstream driver already had eeprom access:
https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch

Quoted 1 line

Yes, but you didn't send it upstream, so I took it, fixed a couple of bugs,

And I appreciate that!

Quoted 1 line

dropped eeprom support since that is secondary for my use case as well as the

I only said the original code had this implemented if someone wants to re-add it to save them some time not having to re-write it from scratch ;-)

Quoted 2 lines

out-of-tree parity code, and submitted it. I'd be more than happy to let you take over if you like.

I’m mostly out of time, so I appreciate you starting the upstream process.