Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list

[Alban]

On Fri, 8 Jun 2018 12:34:12 +0100
Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> wrote:

> >> Can you try this with your original subnode proposal:
> >> just pass the subnode node pointer in np of nvmem_config:
> >>  
> >> ------------------------->cut<------------------------------------  
> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >> index b05aa8e81303..c9621632bbfb 100644
> >> --- a/drivers/nvmem/core.c
> >> +++ b/drivers/nvmem/core.c
> >> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct
> >> nvmem_config *config)
> >>           nvmem->priv = config->priv;
> >>           nvmem->reg_read = config->reg_read;
> >>           nvmem->reg_write = config->reg_write;
> >> -       nvmem->dev.of_node = config->dev->of_node;
> >> +
> >> +       if (config->np)
> >> +               nvmem->dev.of_node = config->np;
> >> +       else
> >> +               nvmem->dev.of_node = config->dev->of_node;
> >>
> >>           if (config->id == -1 && config->name) {
> >>                   dev_set_name(&nvmem->dev, "%s", config->name);
> >> diff --git a/include/linux/nvmem-provider.h
> >> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8
> >> 100644 --- a/include/linux/nvmem-provider.h
> >> +++ b/include/linux/nvmem-provider.h
> >> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv,
> >> unsigned int offset,nvmem_device_get(
> >>     */
> >>    struct nvmem_config {
> >>           struct device           *dev;
> >> +       struct device_node      *np;
> >>           const char              *name;
> >>           int                     id;
> >>           struct module           *owner;
> >>  
> >> ------------------------->cut<------------------------------------  
> > 
> > That should work just fine to allow next to any kind of binding.
> > I'll do a new patch using this approach for the code side and leaving
> > the generic binding as it is.  
> Sure!!
> This will give more flexibility to other provider drivers!

I looked into this. It would work fine for the cells but not so nicely
for the nvmem device API. The phandle for the nvmem device would have
to reference the node passed here and not the real device. We would end
up with a DT like this:

flash@0 {
	compatible = "mtd";
	...
	nvmem_dev: nvmem-cells {
		compatible = "nvmem-cells";
		...
        };
};

other-device@10 {
	...
	nvmem = <&nvmem_dev>;
};

Now if there is no cell defined we have this empty child node that make
very little sense, it is just there to accommodate the nvmem API.

What I would suggest now is to just change the wording. We don't
deprecate the current binding, but we extend it to allow grouping the
cells in a child node if required. The code to support this is trivial,
(4 lines including error handling) so even if we expect very few
bindings to make use of it it is not going to be maintenance drag.
That would look like this:

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
index fd06c09..085d042 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
@@ -19,7 +19,10 @@ Optional properties:
 
 = Data cells =
 These are the child nodes of the provider which contain data cell
-information like offset and size in nvmem provider.
+information like offset and size in nvmem provider. Alternatively the data
+cells can be grouped in a node that has a compatible property set to
+"nvmem-cells".
+
 
 Required properties:
 reg:   specifies the offset in byte within the storage device.
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4e94a78..3e1369c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
        if (!nvmem_np)
                return ERR_PTR(-EINVAL);
 
+       /* bindings that already have anonymous child nodes can instead put
+        * their cells in a child node with an nvmem-cells compatible. */
+       if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
+               nvmem_np = of_get_next_parent(nvmem_np);
+               if (!nvmem_np)
+                       return ERR_PTR(-EINVAL);
+       }
+
        nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
        of_node_put(nvmem_np);
        if (IS_ERR(nvmem))

What about it?

Alban

Attachment: pgpzZkvXTGv6T.pgp
Description: OpenPGP digital signature