From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Alban <albeu@free.fr>
Cc: linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
David Woodhouse <dwmw2@infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
Boris Brezillon <boris.brezillon@free-electrons.com>,
Marek Vasut <marek.vasut@gmail.com>,
Richard Weinberger <richard@nod.at>,
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
devicetree@vger.kernel.org, linux-mtd@lists.infradead.org
Subject: Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list
Date: Sun, 10 Jun 2018 11:32:36 +0100 [thread overview]
Message-ID: <02d3cba5-01a3-4d8f-55fc-9c7b7fd5e5c1@linaro.org> (raw)
In-Reply-To: <20180608190717.55cb185c@tock>
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
>
next prev parent reply other threads:[~2018-06-10 10:32 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-24 23:24 [PATCH v3 0/3] mtd: Add support for reading MTD devices via the nvmem API Alban Bedel
2018-03-24 23:24 ` [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list Alban Bedel
2018-04-16 21:04 ` Rob Herring
2018-04-17 12:31 ` Alban
2018-04-17 12:54 ` Srinivas Kandagatla
2018-04-17 14:54 ` Alban
2018-04-17 15:44 ` Srinivas Kandagatla
2018-04-17 16:00 ` Alban
2018-04-18 11:41 ` Alban
2018-04-18 12:12 ` Srinivas Kandagatla
2018-04-18 12:32 ` Alban
2018-04-18 12:53 ` Srinivas Kandagatla
2018-04-18 13:34 ` Alban
2018-05-01 16:49 ` Srinivas Kandagatla
2018-06-07 16:41 ` Alban
2018-06-07 17:03 ` Srinivas Kandagatla
2018-06-08 10:59 ` Alban
2018-06-08 11:34 ` Srinivas Kandagatla
2018-06-08 17:07 ` Alban
2018-06-10 10:32 ` Srinivas Kandagatla [this message]
2018-06-10 11:36 ` Alban
2018-06-10 13:28 ` Srinivas Kandagatla
2018-03-24 23:24 ` [PATCH v3 2/3] doc: bindings: Add bindings documentation for mtd nvmem Alban Bedel
2018-04-16 21:08 ` Rob Herring
2018-04-17 12:44 ` Alban
2018-03-24 23:24 ` [PATCH v3 3/3] mtd: Add support for reading MTD devices via the nvmem API Alban Bedel
2019-04-18 13:36 ` Reading MAC addresses with NVMEM under MTD partition [Was: Re: [PATCH v3 1/3] nvmem: Update the OF binding to use a subnode for the cells list] Petr Štetiar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=02d3cba5-01a3-4d8f-55fc-9c7b7fd5e5c1@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=albeu@free.fr \
--cc=boris.brezillon@free-electrons.com \
--cc=computersforpeace@gmail.com \
--cc=cyrille.pitchen@wedev4u.fr \
--cc=devicetree@vger.kernel.org \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=mark.rutland@arm.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).