Message-Id: <20210118132411.6268-1-a.fatoum@pengutronix.de>
Date:   Mon, 18 Jan 2021 14:24:11 +0100
From:   Ahmad Fatoum <a.fatoum@...gutronix.de>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     Kamel Bouhara <kamel.bouhara@...tlin.com>, ceggers@...i.de,
        kernel@...gutronix.de, Ahmad Fatoum <a.fatoum@...gutronix.de>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: [PATCH v4] nvmem: core: skip child nodes not matching binding

The nvmem cell binding applies to all eeprom child nodes matching
"^.*@[0-9a-f]+$" without taking a compatible into account.

Linux drivers, like at24, are even more extensive and assume
_all_ at24 eeprom child nodes to be nvmem cells since e888d445ac33
("nvmem: resolve cells from DT at registration time").

Since df5f3b6f5357 ("dt-bindings: nvmem: stm32: new property for
data access"), the additionalProperties: True means it's Ok to have
other properties as long as they don't match "^.*@[0-9a-f]+$".

The barebox bootloader extends the MTD partitions binding to
EEPROM and can fix up following device tree node:

  &eeprom {
    partitions {
      compatible = "fixed-partitions";
    };
  };

This is allowed binding-wise, but drivers using nvmem_register()
like at24 will fail to parse because the function expects all child
nodes to have a reg property present. This results in the whole
EEPROM driver probe failing despite the device tree being correct.

Fix this by skipping nodes lacking a reg property instead of
returning an error. This effectively makes the drivers adhere
to the binding because all nodes with a unit address must have
a reg property and vice versa.

Fixes: e888d445ac33 ("nvmem: resolve cells from DT at registration time").
Signed-off-by: Ahmad Fatoum <a.fatoum@...gutronix.de>

---
Previous Discussion: https://lore.kernel.org/lkml/20200428111829.2215-1-a.fatoum@pengutronix.de/
v1:
  - Ignore all nodes with a unit address (i.e. contain @)
v1 -> v2:
  - use ->full_name instead of ->name as to not break existing correct
    cells (Christian)
v2 -> v3:
  - use optional compatible property to weed out nodes instead of name
  - extend binding documentation (Srini)
v3 -> v4:
  - drop optional nvmem-cell compatible because it's redundant (Rob)
  - Make driver adhere to binding instead of changing binding

As review feedback on v3, Rob suggested moving nvmem cells into a
separate MTD partition. This sound good for people who want MTD
partitions and nvmem to coexist, but my problem described above where
MTD partitions are already fixed up top level into an EEPROM node isn't
solved by this. Revisiting the issue, I think the correct way forward is
along the lines of v1 & v2, where the driver is fixed to actually adhere
to the existing binding. Srini didn't like string matching for @ in driver
code, so I now check for presence of reg instead. They are equivalent
per device tree specification. From v0.3:

  "The unit-address must match the first address specified in the
  reg property of the node. If the node has no reg property, the
  @unit-address must be omitted"...

Cc: Rob Herring <robh+dt@...nel.org>
Cc: devicetree@...r.kernel.org
---
 drivers/nvmem/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 177f5bf27c6d..f114df55f403 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -682,7 +682,9 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
 
 	for_each_child_of_node(parent, child) {
 		addr = of_get_property(child, "reg", &len);
-		if (!addr || (len < 2 * sizeof(u32))) {
+		if (!addr)
+			continue;
+		if (len < 2 * sizeof(u32)) {
 			dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
 			return -EINVAL;
 		}
-- 
2.30.0

Powered by blists - more mailing lists