lkml.org 
[lkml]   [2018]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
From
Date


On 08/06/18 18:07, Alban wrote:
> On Fri, 8 Jun 2018 12:34:12 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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
>

\
 
 \ /
  Last update: 2018-06-10 12:33    [W:0.086 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site