Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Mark Rutland]

Hi,

On Thu, Apr 28, 2016 at 08:44:05AM +0200, Boris Brezillon wrote:
> On Wed, 27 Apr 2016 16:07:38 +0100
> Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Wed, Apr 27, 2016 at 04:35:54PM +0200, Boris Brezillon wrote:
> > > +	ebi: ebi@10000000 {
> > > +		compatible = "atmel,sama5d3-ebi", "simple-bus";  
> > > +		#address-cells = <2>;
> > > +		#size-cells = <1>;
> > > +		atmel,smc = <&hsmc>;
> > > +		atmel,matrix = <&matrix>;
> > > +		reg = <0x10000000 0x10000000
> > > +		       0x40000000 0x30000000>;
> > > +		ranges = <0x0 0x0 0x10000000 0x10000000
> > > +			  0x1 0x0 0x40000000 0x10000000
> > > +			  0x2 0x0 0x50000000 0x10000000
> > > +			  0x3 0x0 0x60000000 0x10000000>;
> > > +		clocks = <&mck>;
> > > +
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_ebi_addr>;
> > > +
> > > +		cs@0 {  
> > > +			#address-cells = <2>;
> > > +			#size-cells = <1>;
> > > +			ranges;
> > > +			atmel,generic-dev;  
> > > +			atmel,read-mode = "nrd";
> > > +			atmel,write-mode = "nwe";
> > > +			atmel,bus-width = <16>;
> > > +			atmel,ncs-rd-setup-ns = <0>;
> > > +			atmel,ncs-wr-setup-ns = <0>;
> > > +			atmel,nwe-setup-ns = <8>;
> > > +			atmel,nrd-setup-ns = <16>;
> > > +			atmel,ncs-rd-pulse-ns = <84>;
> > > +			atmel,ncs-wr-pulse-ns = <84>;
> > > +			atmel,nrd-pulse-ns = <76>;
> > > +			atmel,nwe-pulse-ns = <76>;
> > > +			atmel,nrd-cycle-ns = <107>;
> > > +			atmel,nwe-cycle-ns = <84>;
> > > +			atmel,tdf-ns = <16>;
> > > +
> > > +			nor: flash@0,0 {
> > > +				compatible = "cfi-flash";
> > > +				#address-cells = <1>;
> > > +				#size-cells = <1>;
> > > +				reg = <0x0 0x0 0x1000000>;  
> > 
> > It feels odd that in the node for chipselect N, sub-devices have to
> > encode the chipselect number in their reg, when it's obvious from their
> > container. It may make more sense for the cs node to have a non-empty
> > reg (or somehow to make that translation/truncation implicit).
> 
> Would you agree with the following representation?
> 
> 	ebi: ebi@10000000 {
> 		compatible = "atmel,sama5d3-ebi";
> 		#address-cells = <2>;
> 		#size-cells = <1>;
> 		atmel,smc = <&hsmc>;
> 		atmel,matrix = <&matrix>;
> 		reg = <0x10000000 0x10000000
> 		       0x40000000 0x30000000>;
> 		ranges = <0x0 0x0 0x10000000 0x10000000
> 			  0x1 0x0 0x40000000 0x10000000
> 			  0x2 0x0 0x50000000 0x10000000
> 			  0x3 0x0 0x60000000 0x10000000>;
> 		clocks = <&mck>;
> 
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&pinctrl_ebi_addr>;
> 
> 		configs {
> 			config-0 {
> 				atmel,read-mode = "nrd";
> 				atmel,write-mode = "nwe";
> 				atmel,bus-width = <16>;
> 				atmel,ncs-rd-setup-ns = <0>;
> 				atmel,ncs-wr-setup-ns = <0>;
> 				atmel,nwe-setup-ns = <8>;
> 				atmel,nrd-setup-ns = <16>;
> 				atmel,ncs-rd-pulse-ns = <84>;
> 				atmel,ncs-wr-pulse-ns = <84>;
> 				atmel,nrd-pulse-ns = <76>;
> 				atmel,nwe-pulse-ns = <76>;
> 				atmel,nrd-cycle-ns = <107>;
> 				atmel,nwe-cycle-ns = <84>;
> 				atmel,tdf-ns = <16>;
> 			};
> 		};
> 
> 		nor: flash@0,0 {
> 			compatible = "cfi-flash";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			reg = <0x0 0x0 0x1000000>;
> 			bank-width = <2>;
> 		};
> 	};

Something of that sort looks good to me, yes.

Thanks,
Mark.