| Message ID | 20210118132411.6268-1-a.fatoum@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 0445efacec75b85c2a3c176957ee050ba9be53f0 |
| Headers | show |
| Series | [v4] nvmem: core: skip child nodes not matching binding | expand |
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; > } >
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; }
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(-)