[src/thorpej-i2c-spi-conf]: src/sys/dev/i2c The last change had an unfortunat...

[thorpej <thorpej%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/33639610dd2c
branches:  thorpej-i2c-spi-conf
changeset: 1020815:33639610dd2c
user:      thorpej <thorpej%NetBSD.org@localhost>
date:      Sun May 16 15:27:12 2021 +0000

description:
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.

diffstat:

 sys/dev/i2c/spdmem_i2c.c |  116 +++++++++++++++++++++++++++++-----------------
 1 files changed, 72 insertions(+), 44 deletions(-)

diffs (170 lines):

diff -r 84e09ac0585e -r 33639610dd2c sys/dev/i2c/spdmem_i2c.c
--- a/sys/dev/i2c/spdmem_i2c.c  Sun May 16 05:16:21 2021 +0000
+++ b/sys/dev/i2c/spdmem_i2c.c  Sun May 16 15:27:12 2021 +0000
@@ -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 @@
        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 @@
        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 @@
        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 @@
 {
        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 @@
        sc->sc_page1 = SPDCTL_SPA1;
        sc->sc_base.sc_read = spdmem_i2c_read;
 
+       /*
+        * 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");
-
-       spdmem_common_attach(&sc->sc_base, self);
 }
 
 static int