Re: [RFC v5] V4L DT bindings

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/05/2012 04:57 AM, Guennadi Liakhovetski wrote:
> Hi all
> 
> Version 5 of this RFC is a result of a discussion of its version 4, which 
> took place during the recent Linux Plumbers conference in San Diego. 
> Changes are:
> 
> (1) remove bus-width properties from device (bridge and client) top level. 
> This has actually already been decided upon during our discussions with 
> Sylwester, I just forgot to actually remove them, sorry.
> 
> (2) links are now grouped under "ports." This should help better describe 
> device connection topology by making clearer, which interfaces links are 
> attached to. (help needed: in my notes I see "port" optional if only one 
> port is present, but I seem to remember, that the final decision was - 
> make ports compulsory for uniformity. Which one is true?)

I'd tend to make the port node compulsory.

A related question: What code is going to parse all the port/link
sub-nodes in a device? And, how does it know which sub-nodes are ports,
and which are something else entirely? Perhaps the algorithm is that all
port nodes must be named "port"?

If there were (optionally) no port node, I think the answer to that
question would be a lot more complex, hence why I advocate for it always
being there.

> (3) "videolink" is renamed to just "link."
> 
> (4) "client" is renamed to "remote" and is now used on both sides of 
> links.
> 
> (5) clock-names in clock consumer nodes (e.g., camera sensors) should 
> reflect clock input pin names from respective datasheets
> 
> (6) also serial bus description should be placed under respective link 
> nodes.
> 
> (7) reference remote link DT nodes in "remote" properties, not to the 
> parent.
> 
> (8) use standard names for "SoC-external" (e.g., i2c) devices on their 
> respective busses. "Sensor" has been proposed, maybe "camera" is a better 
> match though.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 
> // Describe an imaginary configuration: a CEU serving either a parallel ov7725
> // sensor, or a serial imx074 sensor, connected over a CSI-2 SoC interface
> 
> 	ceu0: ceu@0xfe910000 {
> 		compatible = "renesas,sh-mobile-ceu";
> 		reg = <0xfe910000 0xa0>;
> 		interrupts = <0x880>;
> 
> 		mclk: master_clock {
> 			compatible = "renesas,ceu-clock";
> 			#clock-cells = <1>;
> 			clock-frequency = <50000000>;	/* max clock frequency */
> 			clock-output-names = "mclk";
> 		};
> 
> 		...
> 		port@0 {

Since there's only 1 port node here, you can drop the "@0" here.

> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			ov772x_1_1: link@1 {

This isn't a comment on the binding definition, but on the example
itself. The label names ("ov772x_1_1" here and "csi2_0" below) feel
backwards to me; I'd personally use label names that describe the object
being labelled, rather than the object that's linked to the node being
labelled. In other words, "ceu0_1" here, and "ov772x_1" at the far end
of this link. But, these are just arbitrary names, so you can name/label
everything whatever you want and it'll still work.

> 				reg = <1>;		/* local pad # */
> 				remote = <&ceu0_1>;	/* remote phandle and pad # */
> 				bus-width = <8>;	/* used data lines */
> 				data-shift = <0>;	/* lines 7:0 are used */
> 	
> 				/* If [hv]sync-active are missing, embedded bt.605 sync is used */
> 				hsync-active = <1>;	/* active high */
> 				vsync-active = <1>;	/* active high */
> 				pclk-sample = <1>;	/* rising */
> 			};
> 
> 			csi2_0: link@0 {
> 				reg = <0>;
> 				remote = <&ceu0_2>;
> 				immutable;
> 			};
> 		};
> 	};
> 
> 	i2c0: i2c@0xfff20000 {
> 		...
> 		ov772x_1: camera@0x21 {
> 			compatible = "omnivision,ov772x";
> 			reg = <0x21>;
> 			vddio-supply = <&regulator1>;
> 			vddcore-supply = <&regulator2>;
> 
> 			clock-frequency = <20000000>;
> 			clocks = <&mclk 0>;
> 			clock-names = "xclk";
> 
> 			...
> 			port {
> 				/* With 1 link per port no need in addresses */
> 				ceu0_1: link@0 {

You can drop "@0" there too.

> 					bus-width = <8>;
> 					remote = <&ov772x_1_1>;
> 					hsync-active = <1>;
> 					hsync-active = <0>;	/* who came up with an inverter here?... */
> 					pclk-sample = <1>;
> 				};
> 			};
> 		};
> 
> 		imx074: camera@0x1a {
> 			compatible = "sony,imx074";
> 			reg = <0x1a>;
> 			vddio-supply = <&regulator1>;
> 			vddcore-supply = <&regulator2>;
> 
> 			clock-frequency = <30000000>;	/* shared clock with ov772x_1 */
> 			clocks = <&mclk 0>;
> 			clock-names = "sysclk";		/* assuming this is the name in the datasheet */
> 			...
> 			port {
> 				csi2_1: link@0 {

You can drop "@0" there too.

> 					clock-lanes = <0>;
> 					data-lanes = <1>, <2>;
> 					remote = <&imx074_1>;
> 				};
> 			};
> 		};
> 		...
> 	};
> 
> 	csi2: csi2@0xffc90000 {
> 		compatible = "renesas,sh-mobile-csi2";
> 		reg = <0xffc90000 0x1000>;
> 		interrupts = <0x17a0>;
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		...
> 		port {
> 			compatible = "renesas,csi2c";	/* one of CSI2I and CSI2C */
> 			reg = <1>;			/* CSI-2 PHY #1 of 2: PHY_S, PHY_M has port address 0, is unused */
> 
> 			imx074_1: link@1 {

You can drop "@1" there too.

> 				client = <&imx074 0>;
> 				clock-lanes = <0>;
> 				data-lanes = <2>, <1>;
> 				remote = <&csi2_1>;
> 			};
> 		};
> 		port {

There are two nodes named "port" here; I think they should be "port@1"
and "port@2" based on the reg properties.

> 			reg = <2>;			/* port 2: link to the CEU */
> 			ceu0_2: link@0 {

You can drop "@0" there too.

> 				immutable;
> 				remote = <&csi2_0>;
> 			};
> 		};
> 	};

Aside from those minor comments, I think the overall structure of the
bindings looks good.