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

Message ID 1521933899-362-2-git-send-email-albeu@free.fr
State Changes Requested
Delegated to: Boris Brezillon
Headers show
Series
  • mtd: Add support for reading MTD devices via the nvmem API
Related show

Commit Message

Alban March 24, 2018, 11:24 p.m.
Having the cells as subnodes of the provider device without any
compatible property might clash with other bindings. To avoid this
problem update the binding to have all the cells in a 'nvmem-cells'
subnode with a 'nvmem-cells' compatible string. This new binding
guarantee that we can turn any kind of device in a nvmem provider.

While discouraged for new uses the old scheme is still supported for
backward compatibility.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++-------
 drivers/nvmem/core.c                              | 10 +++++
 2 files changed, 48 insertions(+), 17 deletions(-)

Comments

Rob Herring April 16, 2018, 9:04 p.m. | #1
On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote:
> Having the cells as subnodes of the provider device without any
> compatible property might clash with other bindings. To avoid this
> problem update the binding to have all the cells in a 'nvmem-cells'
> subnode with a 'nvmem-cells' compatible string. This new binding
> guarantee that we can turn any kind of device in a nvmem provider.
> 
> While discouraged for new uses the old scheme is still supported for
> backward compatibility.
> 
> Signed-off-by: Alban Bedel <albeu@free.fr>
> ---
>  Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++-------
>  drivers/nvmem/core.c                              | 10 +++++
>  2 files changed, 48 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> index fd06c09..6b723e7 100644
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> @@ -11,14 +11,29 @@ these data from, and where they are stored on the storage device.
>  This document is here to document this.
>  
>  = Data providers =
> -Contains bindings specific to provider drivers and data cells as children
> -of this node.
> +A data provider should have a subnode named 'nvmem-cells' that contains
> +a subnodes for each data cells.
> +
> +For backward compatibility the nvmem data cells can be direct children
> +of the data provider. This use is discouraged as it can conflict with
> +other bindings.

I don't think we need to go this far. Whether this is necessary depends 
on the provider.

>  
>  Optional properties:
>   read-only: Mark the provider as read only.
>  
> += Data cells list =
> +The data cells list node should be named 'nvmem-cells' and have a
> +child node for each data cell.
> +
> +Required properties:
> + compatible: Must be "nvmem-cells"
> + #address-cells: <1> if the provider use 32 bit addressing,
> +                 <2> for 64 bits addressing
> + #size-cells: <1> if the provider use 32 bit sizes,
> +              <2> for 64 bits sizes
> +
>  = Data cells =
> -These are the child nodes of the provider which contain data cell
> +These are the child nodes of the nvmem-cells node which contain data cell
>  information like offset and size in nvmem provider.
Alban April 17, 2018, 12:31 p.m. | #2
On Mon, 16 Apr 2018 16:04:29 -0500
Rob Herring <robh@kernel.org> wrote:

> On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote:
> > Having the cells as subnodes of the provider device without any
> > compatible property might clash with other bindings. To avoid this
> > problem update the binding to have all the cells in a 'nvmem-cells'
> > subnode with a 'nvmem-cells' compatible string. This new binding
> > guarantee that we can turn any kind of device in a nvmem provider.
> > 
> > While discouraged for new uses the old scheme is still supported for
> > backward compatibility.
> > 
> > Signed-off-by: Alban Bedel <albeu@free.fr>
> > ---
> >  Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++-------
> >  drivers/nvmem/core.c                              | 10 +++++
> >  2 files changed, 48 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > index fd06c09..6b723e7 100644
> > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> > @@ -11,14 +11,29 @@ these data from, and where they are stored on the storage device.
> >  This document is here to document this.
> >  
> >  = Data providers =
> > -Contains bindings specific to provider drivers and data cells as children
> > -of this node.
> > +A data provider should have a subnode named 'nvmem-cells' that contains
> > +a subnodes for each data cells.
> > +
> > +For backward compatibility the nvmem data cells can be direct children
> > +of the data provider. This use is discouraged as it can conflict with
> > +other bindings.  
> 
> I don't think we need to go this far. Whether this is necessary depends 
> on the provider.

It depend more on the drivers implementation. Sure as long as the
driver only support the nvmem API it doesn't matter, both binding are
fine. But if it ever need to support another API the bindings might
clash and the whole device binding will need to be updated. So all in
all I see very few value in still allowing the old binding for new
devices, or do you seen any problem with the new binding?

However if the consensus is to keep both styles I will rewrite this
paragraph as needed.

Alban
Srinivas Kandagatla April 17, 2018, 12:54 p.m. | #3
On 24/03/18 23:24, Alban Bedel wrote:
> Having the cells as subnodes of the provider device without any
> compatible property might clash with other bindings. To avoid this
> problem update the binding to have all the cells in a 'nvmem-cells'
> subnode with a 'nvmem-cells' compatible string. This new binding
> guarantee that we can turn any kind of device in a nvmem provider.
> 
> While discouraged for new uses the old scheme is still supported for
> backward compatibility.

Am not sure if this a really good idea to change nvmem bindings based on 
provider requirements. This can be a beginning of other problems!!

Did you know that we can pass nvmem cells info via nvmem config ?

Why can't mtd-nvmem provider populate the nvmem_config->cells from its 
dt "nvmem-cells" subnode before it registers the provider?

Doing this way will make the binding very much specific to the provider 
rather than changing nvmem core bindings.

thanks,
srini
Alban April 17, 2018, 2:54 p.m. | #4
On Tue, 17 Apr 2018 13:54:07 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 24/03/18 23:24, Alban Bedel wrote:
> > Having the cells as subnodes of the provider device without any
> > compatible property might clash with other bindings. To avoid this
> > problem update the binding to have all the cells in a 'nvmem-cells'
> > subnode with a 'nvmem-cells' compatible string. This new binding
> > guarantee that we can turn any kind of device in a nvmem provider.
> > 
> > While discouraged for new uses the old scheme is still supported for
> > backward compatibility.  
> 
> Am not sure if this a really good idea to change nvmem bindings based on 
> provider requirements. This can be a beginning of other problems!!

I think you misunderstood something here, this proposed new binding
would be for all new nvmem bindings, not just mtd backed nvmem.

> Did you know that we can pass nvmem cells info via nvmem config ?
> 
> Why can't mtd-nvmem provider populate the nvmem_config->cells from
> its dt "nvmem-cells" subnode before it registers the provider?

The DT based lookup of nvmem-cells doesn't use nvmem_config->cells, so
that's not an option. In fact here the problem come from the MTD side
because it also had a similar binding using subnodes without compatible
string. Just to make things clear, here is an example of the clash
using the current nvmem binding on an unpartitioned MTD device:

flash@0 {
	#address-cells = <1>;
	#size-cells = <1>;
	compatible = "s25sl064a";
	reg = <0>;

	calibration: calib@404 {
		reg = <0x404 0x10>;
	};
};

This will not only allow reading the calibration data from nvmem, but
will also create a partition on the MTD device, which is not acceptable.
With my proposed binding this would become:

flash@0 {
	#address-cells = <1>;
	#size-cells = <1>;
	compatible = "s25sl064a";
	reg = <0>;

	nvmem-cells {
		compatible = "nvmem-cells";
		#address-cells = <1>;
		#address-cells = <1>;

		calibration: calib@404 {
			reg = <0x404 0x10>;
		};
	};
};

Which would work fine as the MTD code will ignore the nvmem-cells
subnode thanks to its compatible string.

IMHO subnodes without any compatible properties should never be used in
such generic bindings, as it is very likely that it will at some point
clash with another generic binding or with a device specific binding.

Alban
Srinivas Kandagatla April 17, 2018, 3:44 p.m. | #5
Thanks for explaining,

On 17/04/18 15:54, Alban wrote:
> This will not only allow reading the calibration data from nvmem, but
> will also create a partition on the MTD device, which is not acceptable.
> With my proposed binding this would become:
> 
> flash@0 {
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	compatible = "s25sl064a";
> 	reg = <0>;
> 
> 	nvmem-cells {
> 		compatible = "nvmem-cells";
> 		#address-cells = <1>;
> 		#address-cells = <1>;
> 
> 		calibration: calib@404 {
> 			reg = <0x404 0x10>;
> 		};
> 	};

Why can't we make nvmem-cells node a nvmem provider in this case?
Which should work!

--srini


> };
> 
> Which would work fine as the MTD code will ignore the nvmem-cells
> subnode thanks to its compatible string.
Alban April 17, 2018, 4 p.m. | #6
On Tue, 17 Apr 2018 16:44:01 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> Thanks for explaining,
> 
> On 17/04/18 15:54, Alban wrote:
> > This will not only allow reading the calibration data from nvmem, but
> > will also create a partition on the MTD device, which is not acceptable.
> > With my proposed binding this would become:
> > 
> > flash@0 {
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 	compatible = "s25sl064a";
> > 	reg = <0>;
> > 
> > 	nvmem-cells {
> > 		compatible = "nvmem-cells";
> > 		#address-cells = <1>;
> > 		#address-cells = <1>;
> > 
> > 		calibration: calib@404 {
> > 			reg = <0x404 0x10>;
> > 		};
> > 	};  
> 
> Why can't we make nvmem-cells node a nvmem provider in this case?
> Which should work!

TBH I just copied what have been done to fix the same problem with the
MTD partitions. But yes we could also just extend the current binding
to require a compatible string on each nvmem-cell, which would not
require any code change to support.

Alban
Alban April 18, 2018, 11:41 a.m. | #7
On Tue, 17 Apr 2018 18:00:40 +0200
Alban <albeu@free.fr> wrote:

> On Tue, 17 Apr 2018 16:44:01 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
> > Thanks for explaining,
> > 
> > On 17/04/18 15:54, Alban wrote:  
> > > This will not only allow reading the calibration data from nvmem, but
> > > will also create a partition on the MTD device, which is not acceptable.
> > > With my proposed binding this would become:
> > > 
> > > flash@0 {
> > > 	#address-cells = <1>;
> > > 	#size-cells = <1>;
> > > 	compatible = "s25sl064a";
> > > 	reg = <0>;
> > > 
> > > 	nvmem-cells {
> > > 		compatible = "nvmem-cells";
> > > 		#address-cells = <1>;
> > > 		#address-cells = <1>;
> > > 
> > > 		calibration: calib@404 {
> > > 			reg = <0x404 0x10>;
> > > 		};
> > > 	};    
> > 
> > Why can't we make nvmem-cells node a nvmem provider in this case?
> > Which should work!  
> 
> TBH I just copied what have been done to fix the same problem with the
> MTD partitions. But yes we could also just extend the current binding
> to require a compatible string on each nvmem-cell, which would not
> require any code change to support.

However this scheme will not work if the device node binding already
have subnodes with addresses. The addressing, as specified by
#address-cells and #size-cells, might be incompatible or might overlap.
Using the nvmem-cells subnode solve this problem.

Alban
Srinivas Kandagatla April 18, 2018, 12:12 p.m. | #8
On 18/04/18 12:41, Alban wrote:
> On Tue, 17 Apr 2018 18:00:40 +0200
> Alban <albeu@free.fr> wrote:
> 
>> On Tue, 17 Apr 2018 16:44:01 +0100
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
>>
>>> Thanks for explaining,
>>>
>>> On 17/04/18 15:54, Alban wrote:
>>>> This will not only allow reading the calibration data from nvmem, but
>>>> will also create a partition on the MTD device, which is not acceptable.
>>>> With my proposed binding this would become:
>>>>
>>>> flash@0 {
>>>> 	#address-cells = <1>;
>>>> 	#size-cells = <1>;
>>>> 	compatible = "s25sl064a";
>>>> 	reg = <0>;
>>>>
>>>> 	nvmem-cells {
>>>> 		compatible = "nvmem-cells";
>>>> 		#address-cells = <1>;
>>>> 		#address-cells = <1>;
>>>>
>>>> 		calibration: calib@404 {
>>>> 			reg = <0x404 0x10>;
>>>> 		};
>>>> 	};
>>>
>>> Why can't we make nvmem-cells node a nvmem provider in this case?
>>> Which should work!
>>
>> TBH I just copied what have been done to fix the same problem with the
>> MTD partitions. But yes we could also just extend the current binding
>> to require a compatible string on each nvmem-cell, which would not
>> require any code change to support.
> 
> However this scheme will not work if the device node binding already
> have subnodes with addresses. The addressing, as specified by
> #address-cells and #size-cells, might be incompatible or might overlap.
> Using the nvmem-cells subnode solve this problem.
> 

I was also suggesting you to use nvmem-cell subnode, but make it a 
proper nvmem provider device, rather than reusing its parent device.

You would end up some thing like this in dt.

flash@0 {
	#address-cells = <1>;
	#size-cells = <1>;
	compatible = "s25sl064a";
	reg = <0>;

	nvmem-cells {
		compatible = "mtd-nvmem";
		#address-cells = <1>;
		#size-cells = <1>;

		calibration: calib@404 {
			reg = <0x404 0x10>;
		};
	};
};

--srini

> Alban
>
Alban April 18, 2018, 12:32 p.m. | #9
On Wed, 18 Apr 2018 13:12:48 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 18/04/18 12:41, Alban wrote:
> > On Tue, 17 Apr 2018 18:00:40 +0200
> > Alban <albeu@free.fr> wrote:
> >   
> >> On Tue, 17 Apr 2018 16:44:01 +0100
> >> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> >>  
> >>> Thanks for explaining,
> >>>
> >>> On 17/04/18 15:54, Alban wrote:  
> >>>> This will not only allow reading the calibration data from nvmem, but
> >>>> will also create a partition on the MTD device, which is not acceptable.
> >>>> With my proposed binding this would become:
> >>>>
> >>>> flash@0 {
> >>>> 	#address-cells = <1>;
> >>>> 	#size-cells = <1>;
> >>>> 	compatible = "s25sl064a";
> >>>> 	reg = <0>;
> >>>>
> >>>> 	nvmem-cells {
> >>>> 		compatible = "nvmem-cells";
> >>>> 		#address-cells = <1>;
> >>>> 		#address-cells = <1>;
> >>>>
> >>>> 		calibration: calib@404 {
> >>>> 			reg = <0x404 0x10>;
> >>>> 		};
> >>>> 	};  
> >>>
> >>> Why can't we make nvmem-cells node a nvmem provider in this case?
> >>> Which should work!  
> >>
> >> TBH I just copied what have been done to fix the same problem with the
> >> MTD partitions. But yes we could also just extend the current binding
> >> to require a compatible string on each nvmem-cell, which would not
> >> require any code change to support.  
> > 
> > However this scheme will not work if the device node binding already
> > have subnodes with addresses. The addressing, as specified by
> > #address-cells and #size-cells, might be incompatible or might overlap.
> > Using the nvmem-cells subnode solve this problem.
> >   
> 
> I was also suggesting you to use nvmem-cell subnode, but make it a 
> proper nvmem provider device, rather than reusing its parent device.
> 
> You would end up some thing like this in dt.
> 
> flash@0 {
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	compatible = "s25sl064a";
> 	reg = <0>;
> 
> 	nvmem-cells {
> 		compatible = "mtd-nvmem";
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		calibration: calib@404 {
> 			reg = <0x404 0x10>;
> 		};
> 	};
> };

But the root cause is in the nvmem binding, this conflict could exists
with any device type, not just MTD. I don't understand why we would want
such workarounds instead of just fixing the problem once and for all.

Alban
Srinivas Kandagatla April 18, 2018, 12:53 p.m. | #10
On 18/04/18 13:32, Alban wrote:
>> I was also suggesting you to use nvmem-cell subnode, but make it a
>> proper nvmem provider device, rather than reusing its parent device.
>>
>> You would end up some thing like this in dt.
>>
>> flash@0 {
>> 	#address-cells = <1>;
>> 	#size-cells = <1>;
>> 	compatible = "s25sl064a";
>> 	reg = <0>;
>>
>> 	nvmem-cells {
>> 		compatible = "mtd-nvmem";
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>>
>> 		calibration: calib@404 {
>> 			reg = <0x404 0x10>;
>> 		};
>> 	};
>> };
> But the root cause is in the nvmem binding, this conflict could exists
No, the root cause is because of passing wrong device instance to nvmem 
core. And trying to workaround is the actual issue.

> with any device type, not just MTD. I don't understand why we would want
> such workarounds instead of just fixing the problem once and for all.
AFAIU, This is not a workaround, this is how nvmem provider bindings are 
and all providers should try to follow it.

I still do not understand what is the issue in making nvmem-cells node a 
proper nvmem provider device?

--srini
Alban April 18, 2018, 1:34 p.m. | #11
On Wed, 18 Apr 2018 13:53:56 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 18/04/18 13:32, Alban wrote:
> >> I was also suggesting you to use nvmem-cell subnode, but make it a
> >> proper nvmem provider device, rather than reusing its parent device.
> >>
> >> You would end up some thing like this in dt.
> >>
> >> flash@0 {
> >> 	#address-cells = <1>;
> >> 	#size-cells = <1>;
> >> 	compatible = "s25sl064a";
> >> 	reg = <0>;
> >>
> >> 	nvmem-cells {
> >> 		compatible = "mtd-nvmem";
> >> 		#address-cells = <1>;
> >> 		#size-cells = <1>;
> >>
> >> 		calibration: calib@404 {
> >> 			reg = <0x404 0x10>;
> >> 		};
> >> 	};
> >> };  
> > But the root cause is in the nvmem binding, this conflict could exists  
> No, the root cause is because of passing wrong device instance to nvmem 
> core. And trying to workaround is the actual issue.

The data is stored on the MTD, so the nvmem provider is the MTD device.
I don't think it is a good idea to have a virtual device in the DT to
accommodate the nvmem API.

> > with any device type, not just MTD. I don't understand why we would want
> > such workarounds instead of just fixing the problem once and for all.  
> AFAIU, This is not a workaround, this is how nvmem provider bindings are 
> and all providers should try to follow it.
> 
> I still do not understand what is the issue in making nvmem-cells node a 
> proper nvmem provider device?

It is doable, but beside making the code more complex, AFAIU that would
goes against DT best practice as no such device exists in the hardware.
The DT should only represent that this device contains data that can be
requested by a driver, and ideally in the same way for all kind of
storages.

Alban
Srinivas Kandagatla May 1, 2018, 4:49 p.m. | #12
On 18/04/18 14:34, Alban wrote:
> On Wed, 18 Apr 2018 13:53:56 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> On 18/04/18 13:32, Alban wrote:
>>>> I was also suggesting you to use nvmem-cell subnode, but make it a
>>>> proper nvmem provider device, rather than reusing its parent device.
>>>>
>>>> You would end up some thing like this in dt.
>>>>
>>>> flash@0 {
>>>> 	#address-cells = <1>;
>>>> 	#size-cells = <1>;
>>>> 	compatible = "s25sl064a";
>>>> 	reg = <0>;
>>>>
>>>> 	nvmem-cells {
>>>> 		compatible = "mtd-nvmem";
>>>> 		#address-cells = <1>;
>>>> 		#size-cells = <1>;
>>>>
>>>> 		calibration: calib@404 {
>>>> 			reg = <0x404 0x10>;
>>>> 		};
>>>> 	};
>>>> };
>>> But the root cause is in the nvmem binding, this conflict could exists
>> No, the root cause is because of passing wrong device instance to nvmem
>> core. And trying to workaround is the actual issue.
> 
> The data is stored on the MTD, so the nvmem provider is the MTD device.
> I don't think it is a good idea to have a virtual device in the DT to
> accommodate the nvmem API.
> 
Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem 
provider too.

However, I would like to see this moving forward.

I can think of one possible solution here, which is, adding 
"nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell. The 
problem you mentioned regarding #address-cells and #size-cells with 
provider need to be addressed in nvmem core.

Currently nvmem core only support offsets of 32 bits, if you are 
expecting a 64 bit offsets then we should add that as a feature to nvmem 
core.

nvmem core as it is today should work fine with 32 bit offsets for mtd 
cases.

what do you think?

thanks,
srini
Alban June 7, 2018, 4:41 p.m. | #13
On Tue, 1 May 2018 17:49:03 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 18/04/18 14:34, Alban wrote:
> > On Wed, 18 Apr 2018 13:53:56 +0100
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> >   
> >> On 18/04/18 13:32, Alban wrote:  
> >>>> I was also suggesting you to use nvmem-cell subnode, but make it a
> >>>> proper nvmem provider device, rather than reusing its parent device.
> >>>>
> >>>> You would end up some thing like this in dt.
> >>>>
> >>>> flash@0 {
> >>>> 	#address-cells = <1>;
> >>>> 	#size-cells = <1>;
> >>>> 	compatible = "s25sl064a";
> >>>> 	reg = <0>;
> >>>>
> >>>> 	nvmem-cells {
> >>>> 		compatible = "mtd-nvmem";
> >>>> 		#address-cells = <1>;
> >>>> 		#size-cells = <1>;
> >>>>
> >>>> 		calibration: calib@404 {
> >>>> 			reg = <0x404 0x10>;
> >>>> 		};
> >>>> 	};
> >>>> };  
> >>> But the root cause is in the nvmem binding, this conflict could exists  
> >> No, the root cause is because of passing wrong device instance to nvmem
> >> core. And trying to workaround is the actual issue.  
> > 
> > The data is stored on the MTD, so the nvmem provider is the MTD device.
> > I don't think it is a good idea to have a virtual device in the DT to
> > accommodate the nvmem API.
> >   
> Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem 
> provider too.
> 
> However, I would like to see this moving forward.
> 
> I can think of one possible solution here, which is, adding 
> "nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell.

I would definitely use "nvmem-cell", from the binding point of view it
doesn't matter what the underlaying storage is.

> The problem you mentioned regarding #address-cells and #size-cells with 
> provider need to be addressed in nvmem core.
> 
> Currently nvmem core only support offsets of 32 bits, if you are 
> expecting a 64 bit offsets then we should add that as a feature to nvmem 
> core.
> 
> nvmem core as it is today should work fine with 32 bit offsets for mtd 
> cases.

That's not what I meant, 32 bit should be more that enough for now.
What I meant is that if a binding already has children nodes using
unit-address, then we would end up with two different uses of the same
"address space".

> what do you think?

AFAIU the only thing that we disagree on now is if the nodes
representing the cells should be direct children of the provider
or in a dedicated subnode. For the MTD case both solution would solve
the binding clash. I would really appreciate if the DT people could
chip in so that we can settle this and get the MTD support merged.

Alban
Srinivas Kandagatla June 7, 2018, 5:03 p.m. | #14
On 07/06/18 17:41, Alban wrote:
> AFAIU the only thing that we disagree on now is if the nodes
> representing the cells should be direct children of the provider
> or in a dedicated subnode. For the MTD case both solution would solve
> the binding clash. I would really appreciate if the DT people could
Am reluctant in changing the nvmem generic bindings for a special case.

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,
   */
  struct nvmem_config {
         struct device           *dev;
+       struct device_node      *np;
         const char              *name;
         int                     id;
         struct module           *owner;

------------------------->cut<------------------------------------

thanks,
srini

> chip in so that we can settle this and get the MTD support merged.
Alban June 8, 2018, 10:59 a.m. | #15
On Thu, 7 Jun 2018 18:03:16 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 07/06/18 17:41, Alban wrote:
> > AFAIU the only thing that we disagree on now is if the nodes
> > representing the cells should be direct children of the provider
> > or in a dedicated subnode. For the MTD case both solution would solve
> > the binding clash. I would really appreciate if the DT people could  
> Am reluctant in changing the nvmem generic bindings for a special case.

Where I think the generic binding is fundamentally flawed, as this
problem will most probably appear again. But do note that my proposal
doesn't require updating the dts using the original binding, both are
still supported. I proposed deprecating the current binding because I
think it is flawed, but we could also "officially" support both style.

> 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.

Alban
Srinivas Kandagatla June 8, 2018, 11:34 a.m. | #16
On 08/06/18 11:59, Alban wrote:
> On Thu, 7 Jun 2018 18:03:16 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> On 07/06/18 17:41, Alban wrote:
>>> AFAIU the only thing that we disagree on now is if the nodes
>>> representing the cells should be direct children of the provider
>>> or in a dedicated subnode. For the MTD case both solution would solve
>>> the binding clash. I would really appreciate if the DT people could
>> Am reluctant in changing the nvmem generic bindings for a special case.
> 
> Where I think the generic binding is fundamentally flawed, as this
> problem will most probably appear again. But do note that my proposal

It depends from which side you are looking at this!!

There are more than 16 nvmem providers in kernel which use this bindings 
and they are fine!!

I obviously take some of your concerns w.r.t bindings and there is a 
scope of making the bindings much robust, most importantly the #address 
and #size-cells. But 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 
into this bindings. If single level of subnode works for your case and 
the below patch works for you then I would recommend lets move on and 
try to get this going.

> doesn't require updating the dts using the original binding, both are
> still supported. I proposed deprecating the current binding because I
> think it is flawed, but we could also "officially" support both style.
> 
>> 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!


thanks,
srini
> 
> Alban
>
Alban June 8, 2018, 5:07 p.m. | #17
On Fri, 8 Jun 2018 12:34:12 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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
Srinivas Kandagatla June 10, 2018, 10:32 a.m. | #18
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
>
Alban June 10, 2018, 11:36 a.m. | #19
On Sun, 10 Jun 2018 11:32:36 +0100
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> 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!

The problem is that DT should represent the hardware, not the OS API.
What should be represented is that other drivers can access data stored
on this device. It is my understanding that this wouldn't be an
acceptable binding as the nvmem provider node would only exists because
of how the NVMEM API currently works, a correct binding would just
directly reference the storage device without this extra node.

> > 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!

I fully understand you concern but I think they are overblown. First I
highly doubt that more layouts will ever be needed, using a compatible
string pretty much guarantee that we won't clash with another binding.
Furthermore even if you consider this extension "MTD specific" the
amount of code is very small, non intrusive and only run once at
registration time. I would understand if we were talking about pages of
code nesting in various place, but not really when it is a single small
if block with an obvious condition. And finally I don't see that as MTD
specific as any other device could use this feature without any code
change.

> 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.

It works from a code POV but it break the basic guidelines of DT
bindings. As I want to have this done, I'm going to do a patch as you
want, but I see a high chance that the binding is going to be rejected
by the DT maintainers and we'll be back here again.

Alban
Srinivas Kandagatla June 10, 2018, 1:28 p.m. | #20
On 10/06/18 12:36, Alban wrote:
> On Sun, 10 Jun 2018 11:32:36 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 
>> 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!
> 
> The problem is that DT should represent the hardware, not the OS API.
Exactly!! flash/mtd has nvmem provider which should be represented in 
the DT.

There is no change in DT side from your original patch vs the new 
approach. You still are going to have the same subnode.

Isn't it?

AFAIU, the new approach will make it explicit that there is a nvmem 
provider in the DT.
...
> What should be represented is that other drivers can access data stored
> on this device. It is my understanding that this wouldn't be an
> acceptable binding as the nvmem provider node would only exists because
> of how the NVMEM API currently works, a correct binding would just
> directly reference the storage device without this extra node.
> 
...

>> 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!
> 
> I fully understand you concern but I think they are overblown. First I
> highly doubt that more layouts will ever be needed, using a compatible
> string pretty much guarantee that we won't clash with another binding.
> Furthermore even if you consider this extension "MTD specific" the
> amount of code is very small, non intrusive and only run once at
> registration time. I would understand if we were talking about pages of
> code nesting in various place, but not really when it is a single small
> if block with an obvious condition. And finally I don't see that as MTD
> specific as any other device could use this feature without any code
> change.
> 
>> 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.
> 
> It works from a code POV but it break the basic guidelines of DT
> bindings. As I want to have this done, I'm going to do a patch as you
> want, but I see a high chance that the binding is going to be rejected
> by the DT maintainers and we'll be back here again.

If you think the sub node is going to be a problem from MTD point of 
view then that is worth discussing.
Adding subnode in nvmem bindings is not going to help or make the 
situation any better.

Lets see how this goes!

thanks,
srini

> 
> Alban
>

Patch

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
index fd06c09..6b723e7 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
@@ -11,14 +11,29 @@  these data from, and where they are stored on the storage device.
 This document is here to document this.
 
 = Data providers =
-Contains bindings specific to provider drivers and data cells as children
-of this node.
+A data provider should have a subnode named 'nvmem-cells' that contains
+a subnodes for each data cells.
+
+For backward compatibility the nvmem data cells can be direct children
+of the data provider. This use is discouraged as it can conflict with
+other bindings.
 
 Optional properties:
  read-only: Mark the provider as read only.
 
+= Data cells list =
+The data cells list node should be named 'nvmem-cells' and have a
+child node for each data cell.
+
+Required properties:
+ compatible: Must be "nvmem-cells"
+ #address-cells: <1> if the provider use 32 bit addressing,
+                 <2> for 64 bits addressing
+ #size-cells: <1> if the provider use 32 bit sizes,
+              <2> for 64 bits sizes
+
 = Data cells =
-These are the child nodes of the provider which contain data cell
+These are the child nodes of the nvmem-cells node which contain data cell
 information like offset and size in nvmem provider.
 
 Required properties:
@@ -37,24 +52,30 @@  For example:
 		...
 
 		/* Data cells */
-		tsens_calibration: calib@404 {
-			reg = <0x404 0x10>;
-		};
+		nvmem-cells {
+			compatible = "nvmem-cells";
+			#address-cells = <1>;
+			#size-cells = <1>;
 
-		tsens_calibration_bckp: calib_bckp@504 {
-			reg = <0x504 0x11>;
-			bits = <6 128>
-		};
+			tsens_calibration: calib@404 {
+				reg = <0x404 0x10>;
+			};
 
-		pvs_version: pvs-version@6 {
-			reg = <0x6 0x2>
-			bits = <7 2>
-		};
+			tsens_calibration_bckp: calib_bckp@504 {
+				reg = <0x504 0x11>;
+				bits = <6 128>
+			};
 
-		speed_bin: speed-bin@c{
-			reg = <0xc 0x1>;
-			bits = <2 3>;
+			pvs_version: pvs-version@6 {
+				reg = <0x6 0x2>
+				bits = <7 2>
+			};
 
+			speed_bin: speed-bin@c{
+				reg = <0xc 0x1>;
+				bits = <2 3>;
+
+			};
 		};
 		...
 	};
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 78051f0..a59195c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -783,6 +783,16 @@  struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 	if (!nvmem_np)
 		return ERR_PTR(-EINVAL);
 
+	/* Devices using the new binding have all the cells in
+	 * a subnode with compatible = "nvmem-cells". In this
+	 * case the device will be the parent of this node.
+	 */
+	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))