Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 12 May 2020 09:18:34 -0500
From:   Rob Herring <robh@kernel.org>
To:     Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc:     linux-kernel@vger.kernel.org, kernel@pengutronix.de,
        ceggers@arri.de, devicetree@vger.kernel.org
Subject: Re: [PATCH v3 0/2] nvmem: skip nodes with compatibles other than
 "nvmem-cell"
Message-ID: <20200512141834.GA3023@bogus>
References: <20200428111829.2215-1-a.fatoum@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200428111829.2215-1-a.fatoum@pengutronix.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 28, 2020 at 01:18:25PM +0200, Ahmad Fatoum wrote:
> The nvmem cell binding applies to all objects which match "^.*@[0-9a-f]+$",
> without taking a compatible into account. This precludes extension of e.g.
> eeprom nodes by any child nodes other than nvmem. Consider following example:
> 
> 	eeprom@0 {
> 		reg = <0 64>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		partitions {
> 			compatible = "fixed-partitions";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			bits = <64 64 64>; /* to verify it's skipped */
> 
> 			part@0 {
> 				reg = <0x00 16>;
> 			};
> 		};
> 
> 		no-cell@10 {
> 			compatible = "not-nvmem-cell";
> 			reg = <0x10 4>;
> 			bits = <64 64 64>; /* to verify it's skipped */
> 		};
> 
> 		cell-old@14 {
> 			reg = <0x14 0x2>;
> 		};
> 
> 		cell-new@16 {
> 			compatible = "nvmem-cell";
> 			reg = <0x16 4>;
> 		};
> 	};
> 
> Without this series, the NVMEM driver interprets all direct children of eeprom@0
> as NVMEM cells and driver probe fails, because the partitions node lacks a reg
> property, e.g.:
> 
>   nvmem 0-00000: nvmem: invalid reg on /eeprom@0
> 
> Running dtbs_check on the snippet will skip partitions (it doesn't match above
> regex), but will flag no-cell@10 and cell-new@16 as invalid.
> 
> With this series applied, the driver will skip partitions and no-cell@10,
> because they have a compatible but it's not "nvmem-cell".

Because you have to support no compatible (forever), there's no point 
adding this compatible.

> Both cell-old@14 and cell-new@16 will be interpreted as cells.
> 
> Likewise, running dtbs_check on the snippet will skip partitions (compatible
> doesn't match and regex doesn't either) and no-cell@10, but accept the other two.
> 
> This series resolves an existing clash between this nvmem-cell binding and
> the barebox bootloader binding that extends the fixed-partitions MTD
> binding to EEPROMs[1]. It's also a building block for getting nvmem cells and
> partitions in MTD devices to co-exist in the same device tree node[2].

This violates having multiple nodes at the same address because you are 
independently overlaying partitions and nvmem cells on same address 
ranges. It also seems seems pretty fragile if you want to update 
partitions.

I think instead, nvmem cells should be contained within a partition. 
The partition should then have a compatible to indicate it contains 
nvmem cells.

Rob