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

[Srinivas Kandagatla]

On 08/06/18 18:07, Alban wrote:
> On Fri, 8 Jun 2018 12:34:12 +0100
> Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> wrote:
...
> 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.
NO. This just looks fine!
nvmem-cells is the nvmem provider node without which you would not have 
any provider instance.
All this looks as expected!
Am not sure what is the problem here!


> 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?
Let me repeat what I have said in my previous emails:

Having a subnode still sounds very fragile to me,
and this could be much specific case of MTD provider. We might have
instances where this could be sub-sub node of the the original provider
for other providers. Also I do not want to bring in Provider specifics 
layout into nvmem bindings.

I can not make myself any clearer than this, Its going to be a NAK from 
my side for the above reasons!

Also, patch I shared should give enough flexibility to various range of 
providers which have different child node layouts without touching the 
nvmem bindings. If it works please use it.


thanks,
srini
> Alban