Hi Alban,
On Wed, 8 Mar 2017 16:20:01 +0100
Alban <al...@free.fr> wrote:
> On Tue, 7 Mar 2017 22:01:07 +0100
> Boris Brezillon <boris.brezil...@free-electrons.com> wrote:
>
> > On Tue, 7 Mar 2017 09:26:03 +0100
> > Alban <al...@free.fr> wrote:
> >
> > > Config data for drivers, like MAC addresses, is often stored in MTD.
> > > Add a binding that define how such data storage can be represented in
> > > device tree.
> > >
> > > Signed-off-by: Alban <al...@free.fr>
> > > ---
> > > Changelog:
> > > v2: * Added a "Required properties" section with the nvmem-provider
> > > property
> > > ---
> > > .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33
> > > ++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > > b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > > new file mode 100644
> > > index 0000000..8ed25e6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > > @@ -0,0 +1,33 @@
> > > += NVMEM in MTD =
> > > +
> > > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > > +This binding define how such data storage can be represented in device
> > > tree.
> > > +
> > > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> > > +property to their node.
> >
> > If everyone agrees that this is actually needed, then it should
> > definitely go in the nvmem binding doc, and we should patch all nvmem
> > providers to define this property (even if we keep supporting nodes
> > that are not defining it). I'm not fully convinced yet, but I might be
> > wrong.
>
> I really like to hear what the DT people think about this.
That was the plan.
>
> > I also think we should take the "nvmem under flash node without partitions"
> > into account now, or at least have a clear plan on how we want to represent
> > it.
> >
> > Something like that?
>
> Yes, but with the following extras:
>
> > flash {
> nvmem-provider;
> > partitions {
> > part@X {
> > nvmem {
> compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@Y {
> > };
> > };
> > };
> > };
> >
> > nvmem {
> compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@X {
> > };
> > };
> > };
> >
> > Note that patching nvmem core to support the subnode case should be
> > pretty easy (see below).
>
> This shouldn't be needed as nothing would change for the NVMEM devices,
> what could be added is a check for the "nvmem-provider" property.
> To support the proposed binding we would only need a minor change to
> of_nvmem_cell_get():
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 408b521ee520..6231ea27c9f4 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -444,6 +444,10 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> if (!config->dev)
> return ERR_PTR(-EINVAL);
>
> + if (config->dev->of_node &&
> + !of_property_read_bool(config->dev->of_node, "nvmem-provider"))
> + return ERR_PTR(-ENODEV);
> +
> nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
> if (!nvmem)
> return ERR_PTR(-ENOMEM);
> @@ -777,6 +781,15 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node
> *np,
> if (!nvmem_np)
> return ERR_PTR(-EINVAL);
>
> + /* handle the new cell binding */
> + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
> + nvmem_np = of_get_next_parent(cell_np);
> + if (!nvmem_np)
> + return ERR_PTR(-EINVAL);
> + if (!of_property_read_bool(nvmem_np, "nvmem-provider"))
> + return ERR_PTR(-ENODEV);
> + }
> +
> nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
> if (IS_ERR(nvmem))
> return ERR_CAST(nvmem);
>
Yep, works too. Let's wait for a DT review, before taking a decision.
Thanks,
Boris
