[PATCH v4] nvmem: core: skip child nodes not matching binding

[PATCH v4] nvmem: core: skip child nodes not matching binding

[Ahmad Fatoum]

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@pengutronix.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@kernel.org>
Cc: devicetree@vger.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

Re: [PATCH v4] nvmem: core: skip child nodes not matching binding

[Srinivas Kandagatla]

On 18/01/2021 13:24, Ahmad Fatoum wrote:
> 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@pengutronix.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@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>   drivers/nvmem/core.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)


Applied thanks,

--srini

> 
> 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;
>   		}
>