Module Name:    src
Committed By:   thorpej
Date:           Sun May 16 15:27:12 UTC 2021

Modified Files:
        src/sys/dev/i2c [thorpej-i2c-spi-conf]: spdmem_i2c.c

Log Message:
The last change had an unfortunate side-effect on empty DIMM slots, so
roll that back.  Instead, if we used direct config, then probe for the
module in the attach routine and report if the module is not present,
rather than assuming that it is.

Encapsulate the direct config logic for SPD into one place for clarity.


To generate a diff of this commit:
cvs rdiff -u -r1.21.4.1 -r1.21.4.2 src/sys/dev/i2c/spdmem_i2c.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/dev/i2c/spdmem_i2c.c
diff -u src/sys/dev/i2c/spdmem_i2c.c:1.21.4.1 src/sys/dev/i2c/spdmem_i2c.c:1.21.4.2
--- src/sys/dev/i2c/spdmem_i2c.c:1.21.4.1	Sun May 16 05:16:21 2021
+++ src/sys/dev/i2c/spdmem_i2c.c	Sun May 16 15:27:12 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: spdmem_i2c.c,v 1.21.4.1 2021/05/16 05:16:21 thorpej Exp $ */
+/* $NetBSD: spdmem_i2c.c,v 1.21.4.2 2021/05/16 15:27:12 thorpej Exp $ */
 
 /*
  * Copyright (c) 2007 Nicolas Joly
@@ -40,7 +40,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: spdmem_i2c.c,v 1.21.4.1 2021/05/16 05:16:21 thorpej Exp $");
+__KERNEL_RCSID(0, "$NetBSD: spdmem_i2c.c,v 1.21.4.2 2021/05/16 15:27:12 thorpej Exp $");
 
 #include <sys/param.h>
 #include <sys/device.h>
@@ -178,6 +178,48 @@ static const struct device_compatible_en
 	DEVICE_COMPAT_EOL
 };
 
+/*
+ * Some device trees don't have real "compatible" entries, so we
+ * end up having to match by name.
+ */
+static const struct device_compatible_entry name_data[] = {
+	{ .compat = "dimm-spd" },
+	{ .compat = "dimm" },
+	DEVICE_COMPAT_EOL
+};
+
+static bool
+spdmem_i2c_use_name_match(const struct i2c_attach_args *ia, int *match_resultp)
+{
+	const char *name = ia->ia_name;
+
+	if (name != NULL) {
+		*match_resultp = device_compatible_match(&name, 1, name_data)
+		    ? I2C_MATCH_DIRECT_COMPATIBLE
+		    : 0;
+		return true;
+	}
+	return false;
+}
+
+static bool
+spdmem_i2c_use_direct_match(const struct i2c_attach_args *ia, const cfdata_t cf,
+			    const struct device_compatible_entry *cdata,
+			    int *match_resultp)
+{
+	if (iic_use_direct_match(ia, cf, cdata, match_resultp))
+		return true;
+
+	/*
+	 * Matching by name is not ideal, but some device trees only
+	 * have a name and no "compatible" property.
+	 */
+	if (spdmem_i2c_use_name_match(ia, match_resultp))
+		return true;
+
+	return false;
+}
+
 static int
 spdmem_i2c_match(device_t parent, cfdata_t match, void *aux)
 {
@@ -185,45 +227,13 @@ spdmem_i2c_match(device_t parent, cfdata
 	struct spdmem_i2c_softc sc;
 	int match_result;
 
-	/*
-	 * SPD stands for "Serial Presence Detect".  This implies that
-	 * if we're using direct configuration that we should treat
-	 * that as a *hint*... it's entirely possible that a device
-	 * tree lists locations where SPD memory can be found, not
-	 * necessarily where memory is known to be present.
-	 *
-	 * Accordingly, if we get a direct configuration match based
-	 * on compatible data or device name, we still check to see
-	 * if the device is there.
-	 */
-
-	if (iic_use_direct_match(ia, match, compat_data, &match_result)) {
-		if (match_result != 0) {
-			goto do_probe;
-		}
-		return 0;
-	}
+	if (spdmem_i2c_use_direct_match(ia, match, compat_data, &match_result))
+		return match_result;
 
-	if (ia->ia_name) {
-		/* add other names as we find more firmware variations */
-		if (strcmp(ia->ia_name, "dimm-spd") == 0 ||
-		    strcmp(ia->ia_name, "dimm") == 0) {
-			match_result = I2C_MATCH_DIRECT_SPECIFIC;
-			goto do_probe;
-		}
+	/* Filter out by address when not using direct config. */
+	if ((ia->ia_addr & SPDMEM_I2C_ADDRMASK) != SPDMEM_I2C_ADDR)
 		return 0;
-	}
 
-	/* As a last resort, filter out invalid addresses. */
-	if ((ia->ia_addr & SPDMEM_I2C_ADDRMASK) == SPDMEM_I2C_ADDR) {
-		match_result = I2C_MATCH_ADDRESS_AND_PROBE;
-		goto do_probe;
-	}
-	
-	/* Not a candidate address. */
-	return 0;
-
- do_probe:
 	sc.sc_tag = ia->ia_tag;
 	sc.sc_addr = ia->ia_addr;
 	sc.sc_page0 = SPDCTL_SPA0;
@@ -234,10 +244,9 @@ spdmem_i2c_match(device_t parent, cfdata
 	if (spdmem_reset_page(&sc) != 0)
 		return 0;
 
-	if (spdmem_common_probe(&sc.sc_base)) {
-		return match_result;
-	}
-	return 0;
+	return spdmem_common_probe(&sc.sc_base)
+	    ? I2C_MATCH_ADDRESS_AND_PROBE
+	    : 0;
 }
 
 static void
@@ -245,6 +254,7 @@ spdmem_i2c_attach(device_t parent, devic
 {
 	struct spdmem_i2c_softc *sc = device_private(self);
 	struct i2c_attach_args *ia = aux;
+	int match_result;
 
 	sc->sc_tag = ia->ia_tag;
 	sc->sc_addr = ia->ia_addr;
@@ -252,10 +262,28 @@ spdmem_i2c_attach(device_t parent, devic
 	sc->sc_page1 = SPDCTL_SPA1;
 	sc->sc_base.sc_read = spdmem_i2c_read;
 
-	if (!pmf_device_register(self, NULL, NULL))
-		aprint_error_dev(self, "couldn't establish power handler\n");
+	/*
+	 * SPD stands for "Serial Presence Detect".  If we're using
+	 * direct configuration, the device tree may have given us
+	 * a location where and SPD memory modudle *could* be found,
+	 * not necessarily where it is known to be present.
+	 *
+	 * Accordingly, if we get a direct configuration match based
+	 * on compatible data or device name, we still check to see
+	 * if the device is there.
+	 */
+	if (spdmem_i2c_use_direct_match(ia, device_cfdata(self), compat_data,
+					&match_result)) {
+		if (! spdmem_common_probe(&sc->sc_base)) {
+			aprint_normal(": module not present\n");
+			return;
+		}
+	}
 
 	spdmem_common_attach(&sc->sc_base, self);
+
+	if (!pmf_device_register(self, NULL, NULL))
+		aprint_error_dev(self, "couldn't establish power handler\n");
 }
 
 static int

Reply via email to