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

[Alban <albeu@free.fr>]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]