i2c-imc-skylake: instantiate the SPD EEPROMs at probe

Call i2c_register_spd() per adapter, like i801 and piix4, instead of
telling the user to echo into new_device. The helper counts populated
slots from DMI and probes upwards from 0x50 via i2c_new_scanned_device(),
so it claims no address that does not answer; each channel carries half
the DIMMs and the scan is per adapter, so each finds its own. Verified:
four clients on load, two per channel, none on the empty slots, and
decode-dimms works with no manual step.

This needed Receive Byte to be worth doing — without I2C_FUNC_SMBUS_BYTE
ee1004 would not have bound to what was instantiated.

The write_disable variant is deliberate. This part is DDR4 only, where
the two are equivalent (the split gates spd5118 on DDR5), and of two
equivalent calls the one that does not read as "this driver enables SPD
writes" is the better name to leave in a driver whose bus is the memory
bus. In-tree convention is write_enable; the cover letter offers to
switch.

Kconfig gains select I2C_SMBUS.

Out-of-tree, compat.h maps the call onto the older unsplit
i2c_register_spd() where that is what the target kernel has. It is force
included from the Makefile so i2c-imc-skylake.c stays byte-identical to
the submitted file, and the Makefile detects the API by grepping the
target kernel's header rather than testing a version number, which keeps
it correct on backported trees. Builds clean against both 6.8 and 7.0
headers; DKMS 1.0.2 installs on all four kernels here.

test-smoke.sh needed two fixes to keep testing what it claims:

  - A bound driver owns its address, so the raw transaction checks lost
    the SPDs the moment the driver started instantiating them. The script
    now unbinds ee1004 for the duration rather than deleting the clients:
    sysfs delete_device only removes clients that new_device created,
    and unbinding is sufficient because i2c-dev refuses an address only
    while a driver is bound (i2cdev_check: `return dev->driver ? -EBUSY
    : 0`). Rebound from an EXIT trap.

  - Those checks were guarded on an SPD address having been found, and
    when the lookup failed they vanished from the run silently: the suite
    reported "12 passed, 0 failed" while seven checks had not executed.
    Skipped checks are now counted and printed, which is how the above
    was noticed at all.

Smoke suite: 20 passed, 0 failed, 0 skipped. checkpatch --strict 0/0/0,
in-tree W=1 and Sparse clean.

Makefile

@@ -2,6 +2,16 @@ obj-m := i2c-imc-skylake.o
 
 KVER   ?= $(shell uname -r)
 KDIR   ?= /lib/modules/$(KVER)/build
+
+# Out-of-tree only. The driver targets current mainline and calls
+# i2c_register_spd_write_disable(); older kernels have the unsplit
+# i2c_register_spd(). compat.h bridges that, force included here so
+# i2c-imc-skylake.c is exactly the file sent upstream. Detected from the target
+# kernel's header rather than from a version number, which keeps it right on
+# backported and vendor trees.
+SPD_SPLIT := $(shell grep -qs i2c_register_spd_write_disable \
+	$(KDIR)/include/linux/i2c-smbus.h && echo -DHAVE_I2C_REGISTER_SPD_SPLIT)
+ccflags-y += -include $(src)/compat.h $(SPD_SPLIT)
 INSTALL_MOD_PATH ?= $(DESTDIR)
 INSTALL_MOD_DIR  ?= updates
 PWD    := $(shell pwd)

VERSION

@@ -1 +1 @@
-1.0.1
+1.0.2

compat.h

@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Out-of-tree build shim. NOT part of the upstream submission: the Makefile
+ * force includes it, i2c-imc-skylake.c never does, so the driver source stays
+ * free of build conditionals.
+ *
+ * i2c_register_spd() was split into i2c_register_spd_write_disable() and
+ * i2c_register_spd_write_enable() when DDR5 arrived and the caller had to say
+ * whether spd5118 should be instantiated at all. The driver targets current
+ * mainline and calls the split form; where only the old single function
+ * exists it has exactly the semantics this driver wants, since the split is
+ * about DDR5 and this part is DDR4 only.
+ *
+ * HAVE_I2C_REGISTER_SPD_SPLIT comes from the Makefile, which greps the target
+ * kernel's header for the symbol. Detecting the API beats guessing the version
+ * it appeared in, and it stays right on backported and vendor trees.
+ */
+
+#ifndef I2C_IMC_SKYLAKE_COMPAT_H
+#define I2C_IMC_SKYLAKE_COMPAT_H
+
+#include <linux/i2c-smbus.h>
+
+#ifndef HAVE_I2C_REGISTER_SPD_SPLIT
+static inline void i2c_register_spd_write_disable(struct i2c_adapter *adap)
+{
+	i2c_register_spd(adap);
+}
+
+static inline void i2c_register_spd_write_enable(struct i2c_adapter *adap)
+{
+	i2c_register_spd(adap);
+}
+#endif
+
+#endif /* I2C_IMC_SKYLAKE_COMPAT_H */

docs/submission/Kconfig.kernel

@@ -1,6 +1,7 @@
 config I2C_IMC_SKYLAKE
 	tristate "Intel Skylake-X iMC SMBus adapter"
 	depends on PCI && (X86_64 || COMPILE_TEST)
+	select I2C_SMBUS
 	help
 	  Say Y here if you want kernel support for the integrated memory
 	  controller (iMC) SMBus engine found in Intel Skylake-X / Cascade
@@ -10,6 +11,8 @@ config I2C_IMC_SKYLAKE
 	  Two I2C adapters are registered, one per hardware SMBus channel,
 	  allowing access to DDR4 DIMM SPD EEPROMs (0x50-0x57) and thermal
 	  sensors from userspace via standard i2c-tools and lm-sensors.
+	  SPD EEPROMs are instantiated automatically from DMI, so decode-dimms
+	  works without any manual step.
 
 	  Firmware, SMM, a BMC or CLTT may drive the same engine and no
 	  generic arbitration mechanism is known.  The driver therefore

docs/submission/cover-letter.txt

@@ -124,6 +124,22 @@ ee1004 consequently binds, and decode-dimms reads all four DIMMs on this
 system with both SPD CRCs valid on each.  jc42 binds where a sensor is
 populated.  i2cdetect scans in read mode.
 
+With Receive Byte in place the driver can also do what the other SMBus
+host drivers do and call i2c_register_spd() per adapter, so the SPD
+EEPROMs are instantiated at probe rather than by hand.  The helper counts
+the populated slots from DMI and probes upwards from 0x50, leaving alone
+any address that does not answer; each channel carries half the DIMMs and
+the scan is per adapter, so each finds its own.  Nothing else is
+instantiated, jc42 included: there is no firmware description of these
+buses and this driver does not scan for anything but the SPDs.
+
+I used the write_disable variant.  This part is DDR4 only, where the two
+are equivalent - the distinction gates spd5118 on DDR5 - and between two
+equivalent calls the one that does not read as "this driver enables SPD
+writes" seemed the better thing to leave in a memory-bus driver.  Happy
+to switch to write_enable for consistency with i801 and piix4 if you
+prefer the convention.
+
 SMBus Quick is still not implemented, and not only because the engine has
 no obvious encoding for it.  A write to the 0x30-0x37 range is how an
 EE1004 SPD is write-protected, permanently on many modules.  An adapter
@@ -156,6 +172,8 @@ Changes since v2
     not, ee1004 also needs Receive Byte and Send Byte.
   - Implement both, on the pointer-select bit, so ee1004 binds and
     decode-dimms works.
+  - Instantiate the SPD EEPROMs with i2c_register_spd() instead of
+    leaving it to the user.
 
 Validation
 ~~~~~~~~~~
@@ -170,11 +188,13 @@ Validation
     concurrent reads per channel, the default-off gate, and 20
     unload/reload cycles: 19 checks, 0 failures, no kernel warning or oops,
     no interference reported, every command word confirmed in the register.
-  - ee1004 bound on all four SPD addresses and decode-dimms decoded all
-    four modules, each with the CRC of bytes 0-125 and of bytes 128-253
-    valid.  The part number reads out of page 1, so the page select is
-    exercised end to end.  The four module serial numbers were identical
-    before and after every experiment in this series.
+  - i2c_register_spd() instantiated ee1004 on all four populated SPD
+    addresses at probe, two per channel, and claimed none of the empty
+    ones.  decode-dimms then decoded all four modules, each with the CRC
+    of bytes 0-125 and of bytes 128-253 valid.  The part number reads out
+    of page 1, so the page select is exercised end to end.  The four
+    module serial numbers were identical before and after every
+    experiment in this series.
   - Engine registers sampled every 2 s for one hour with the driver
     unloaded: 1791 samples, 0 changes.  No firmware activity while the
     system is up.

docs/submission/i2c-imc-skylake.rst

@@ -129,22 +129,27 @@ Word transfers use the standard SMBus byte order, so ``jc42`` reading through
 Instantiating clients
 ---------------------
 
-The adapter does not create clients by itself.  There is no firmware
-description of these buses and the driver does not scan.
+SPD EEPROMs are instantiated at probe with ``i2c_register_spd()``, like the
+other SMBus host drivers.  It counts the populated slots from DMI and probes
+from 0x50 upwards, so an address that does not answer is left alone.  Each
+channel carries half the DIMMs and the scan runs per adapter, so each finds
+its own::
 
-``jc42`` can be instantiated where a thermal sensor is populated::
+    i2c i2c-6: Successfully instantiated SPD at 0x50
+    ee1004 6-0050: 512 byte EE1004-compliant SPD EEPROM, read-only
+
+``decode-dimms`` therefore works with no manual step.
+
+Nothing else is instantiated: there is no firmware description of these buses
+and the driver does not scan for anything but the SPDs.  ``jc42`` has to be
+added by hand where a thermal sensor is populated::
 
     # echo jc42 0x18 > /sys/bus/i2c/devices/i2c-6/new_device
 
 Whether one is populated is visible in ``SMBCNTL.TSOD_PRESENT``; on the
 development system that mask is empty and no ``jc42`` binds.
 
-``ee1004`` binds on each populated SPD address::
-
-    # echo ee1004 0x50 > /sys/bus/i2c/devices/i2c-6/new_device
-    ee1004 6-0050: 512 byte EE1004-compliant SPD EEPROM, read-only
-
-Two things are worth knowing before instantiating it on this bus.
+Two things about the SPDs are worth knowing on this bus.
 
 A DDR4 SPD is a 512-byte array reached through a 256-byte window, and which
 half is visible is a property of the bus, not of the transfer.  ``ee1004``
@@ -154,11 +159,14 @@ reading SPD by hand::
 
     # i2cset -y 6 0x36 0x00 c
 
-And a bound ``ee1004`` owns those addresses, so userspace can no longer reach
-them through ``/dev/i2c-*``.  That matters more here than on a mainboard SMBus,
-because the tools that motivate this driver share the bus with the SPDs.  Bind
-``ee1004`` when you want ``decode-dimms``, and remove it when you want the
-addresses back::
+And ``ee1004`` owns those addresses once bound, so userspace cannot reach them
+through ``/dev/i2c-*`` any more.  This is ordinary I2C behaviour, but it is
+worth stating here because the userspace tools that motivate this driver share
+the bus with the SPDs.  Read the SPD through the driver instead::
+
+    $ cat /sys/bus/i2c/drivers/ee1004/6-0050/eeprom | hexdump -C
+
+or, if an address really is needed raw, release it::
 
     # echo 0x50 > /sys/bus/i2c/devices/i2c-6/delete_device
 

i2c-imc-skylake.c

@@ -47,6 +47,7 @@
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -680,6 +681,22 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 				"i2c_add_adapter ch%d failed: %d\n", i, ret);
 			return ret;
 		}
+
+		/*
+		 * Instantiate the SPD EEPROMs the same way the other SMBus
+		 * host drivers do.  This counts the populated slots from DMI
+		 * and probes 0x50 upwards, so no client is created for an
+		 * address that does not answer and userspace keeps the ones
+		 * that stay empty.  Each channel carries half the DIMMs, and
+		 * the scan is per adapter, so each finds its own.
+		 *
+		 * The write_disable variant: this part is DDR4 only, where the
+		 * two differ in nothing (the distinction gates spd5118 on
+		 * DDR5).  Where they are equivalent, the one that does not
+		 * suggest this driver enables SPD writes is the better name to
+		 * leave in a memory-bus driver.
+		 */
+		i2c_register_spd_write_disable(a);
 	}
 
 	dev_dbg(&pdev->dev, "registered 2 SMBus channels\n");

test-smoke.sh

@@ -4,6 +4,7 @@ set -euo pipefail
 
 PASS=0
 FAIL=0
+SKIP=0
 CH0=""
 CH1=""
 CH0_ADDR=""
@@ -53,6 +54,16 @@ _fail()
 	((FAIL++)) || true
 }
 
+# A check that could not run is not a check that passed. Several below are
+# guarded on an SPD address having been found, and when that lookup failed they
+# used to vanish from the run silently, leaving a shorter suite reporting
+# "0 failed" — which is how a regression hides.
+_skip()
+{
+	echo "SKIP: $*"
+	((SKIP++)) || true
+}
+
 _module_loaded()
 {
 	lsmod | awk '$1 == "i2c_imc_skylake" { found = 1 } END { exit !found }'
@@ -200,6 +211,43 @@ _load_unsafe()
 	"${SUDO[@]}" insmod "$MODULE_PATH" allow_unsafe_access=1
 }
 
+# The driver instantiates ee1004 on the populated SPD addresses at probe, and
+# the checks below need those addresses raw. Unbind rather than delete: the
+# sysfs delete_device only removes clients that sysfs new_device created, while
+# i2c_register_spd() uses i2c_new_scanned_device(). Unbinding is enough anyway,
+# because i2c-dev refuses an address only while a driver is *bound* to it
+# (i2cdev_check: `return dev->driver ? -EBUSY : 0`).
+SPD_UNBOUND=()
+
+_unbind_spd_clients()
+{
+	local dev name
+
+	for dev in /sys/bus/i2c/drivers/ee1004/*-*; do
+		[[ -e "$dev" ]] || continue
+		name=${dev##*/}
+		[[ "$name" == "$CH0-"* || "$name" == "$CH1-"* ]] || continue
+		if printf '%s\n' "$name" |
+			"${SUDO[@]}" tee /sys/bus/i2c/drivers/ee1004/unbind >/dev/null 2>&1; then
+			SPD_UNBOUND+=("$name")
+		fi
+	done
+	echo "unbound ${#SPD_UNBOUND[@]} SPD client(s) for the raw transaction checks"
+}
+
+# Put them back if the run ends early. On a clean run the reload cycles below
+# have already recreated and rebound them, so these writes just fail harmlessly.
+_rebind_spd_clients()
+{
+	local name
+
+	for name in ${SPD_UNBOUND[@]+"${SPD_UNBOUND[@]}"}; do
+		printf '%s\n' "$name" |
+			"${SUDO[@]}" tee /sys/bus/i2c/drivers/ee1004/bind >/dev/null 2>&1 || true
+	done
+}
+trap _rebind_spd_clients EXIT
+
 DMESG_START=$("${SUDO[@]}" dmesg | wc -l)
 "${SUDO[@]}" modprobe i2c-dev
 _module_loaded && "${SUDO[@]}" rmmod i2c-imc-skylake
@@ -210,6 +258,15 @@ _refresh_buses
 [[ -n "$CH0" ]] && _ok "ch0 adapter at i2c-$CH0" || _fail "ch0 adapter not found"
 [[ -n "$CH1" ]] && _ok "ch1 adapter at i2c-$CH1" || _fail "ch1 adapter not found"
 
+# The driver instantiates SPD clients at probe. Prove it did, then take the
+# addresses back so the raw transaction checks can use them.
+_spd_clients=$(ls -d /sys/bus/i2c/drivers/ee1004/"$CH0"-00* \
+	/sys/bus/i2c/drivers/ee1004/"$CH1"-00* 2>/dev/null | wc -l)
+((_spd_clients > 0)) &&
+	_ok "driver instantiated $_spd_clients SPD EEPROM(s) at probe" ||
+	_fail "no SPD EEPROM instantiated at probe"
+_unbind_spd_clients
+
 if [[ -n "$CH0" ]]; then
 	_scan_spd "$CH0" CH0_ADDR && _ok "SPD device present on ch0" || _fail "no SPD device on ch0"
 fi
@@ -220,30 +277,41 @@ fi
 echo "=== transfer semantics ==="
 if [[ -n "$CH0_ADDR" ]]; then
 	_word_test "$CH0" "$CH0_ADDR" && _ok "WORD_DATA byte order on ch0" || _fail "WORD_DATA test on ch0"
+else
+	_skip "WORD_DATA on ch0: no reachable SPD address"
 fi
 if [[ -n "$CH1_ADDR" ]]; then
 	_word_test "$CH1" "$CH1_ADDR" && _ok "WORD_DATA byte order on ch1" || _fail "WORD_DATA test on ch1"
-
+else
+	_skip "WORD_DATA on ch1: no reachable SPD address"
 fi
 if [[ -n "$CH0_ADDR" ]]; then
 	_receive_byte_test "$CH0" "$CH0_ADDR" &&
 		_ok "Receive Byte follows the EEPROM pointer on ch0" ||
 		_fail "Receive Byte test on ch0"
+else
+	_skip "Receive Byte follows the EEPROM pointer on ch0: no reachable SPD address"
 fi
 if [[ -n "$CH1_ADDR" ]]; then
 	_receive_byte_test "$CH1" "$CH1_ADDR" &&
 		_ok "Receive Byte follows the EEPROM pointer on ch1" ||
 		_fail "Receive Byte test on ch1"
+else
+	_skip "Receive Byte follows the EEPROM pointer on ch1: no reachable SPD address"
 fi
 if [[ -n "$CH0_ADDR" ]]; then
 	_send_byte_test "$CH0" "$CH0_ADDR" &&
 		_ok "Send Byte selects the SPD page on ch0" ||
 		_fail "Send Byte test on ch0"
+else
+	_skip "Send Byte selects the SPD page on ch0: no reachable SPD address"
 fi
 if [[ -n "$CH1_ADDR" ]]; then
 	_send_byte_test "$CH1" "$CH1_ADDR" &&
 		_ok "Send Byte selects the SPD page on ch1" ||
 		_fail "Send Byte test on ch1"
+else
+	_skip "Send Byte selects the SPD page on ch1: no reachable SPD address"
 fi
 if [[ -n "$CH0" ]]; then
 	_nack_test "$CH0" && _ok "absent-device ENXIO on ch0" || _fail "expected ENXIO at address 0x77 on ch0"
@@ -325,5 +393,9 @@ else
 	_ok "every command reached the register"
 fi
 
-echo "=== result: $PASS passed, $FAIL failed ==="
+if ((SKIP)); then
+	echo "=== result: $PASS passed, $FAIL failed, $SKIP skipped ==="
+else
+	echo "=== result: $PASS passed, $FAIL failed ==="
+fi
 ((FAIL == 0))