Re: [PATCH] media: bindings: video-interfaces: Update the example

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo,

Thanks for your patch.

On 2019-08-22 23:04:33 +0200, Jacopo Mondi wrote:
> The example provided by the video-interface.txt file uses compatible
> values for drivers which are have been removed a long time ago. To avoid
> generating confusion, replace the existing example with a new one using
> upstream maintained and more modern devices.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> 
> ---
> This patch has been triggered by Simon's attempt to rename the bindings
> for the now removed soc-camera based sh-mobile-ceu device, which is used in
> this example:
> https://patchwork.kernel.org/patch/11101079/
> 
> As soon as that driver is not mentioned in the example anymore, its
> bindings documentation could be removed as well.
> ---
>  .../bindings/media/video-interfaces.txt       | 223 ++++++++++--------
>  1 file changed, 130 insertions(+), 93 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..cce80fd0ea13 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -153,123 +153,160 @@ Optional endpoint properties
>  Example
>  -------
> 
> -The example snippet below describes two data pipelines.  ov772x and imx074 are
> -camera sensors with a parallel and serial (MIPI CSI-2) video bus respectively.
> -Both sensors are on the I2C control bus corresponding to the i2c0 controller
> -node.  ov772x sensor is linked directly to the ceu0 video host interface.
> -imx074 is linked to ceu0 through the MIPI CSI-2 receiver (csi2). ceu0 has a
> -(single) DMA engine writing captured data to memory.  ceu0 node has a single
> -'port' node which may indicate that at any time only one of the following data
> -pipelines can be active: ov772x -> ceu0 or imx074 -> csi2 -> ceu0.
> -
> -	ceu0: ceu@fe910000 {
> -		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";
> -		};
> +Te example snippet below describes two data pipelines connected to a video

s/Te/The/

> +DMA engine (VIN4) which has a direct parallel video bus connection to an HDMI
> +video decoder at port@0 and a data path to a CSI-2 receiver connected to an
> +image sensor (imx074) at port@1.
> 
> -		port {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> +The parallel HDMI video decoder links directly to the VIN input port 0, and the
> +bus configuration at both ends is specified in each endpoint.
> 
> -			/* Parallel bus endpoint */
> -			ceu0_1: endpoint@1 {
> -				reg = <1>;		/* Local endpoint # */
> -				remote = <&ov772x_1_1>;	/* Remote phandle */
> -				bus-width = <8>;	/* Used data lines */
> -				data-shift = <2>;	/* Lines 9:2 are used */
> +The imx074 sensor connects to the CSI-2 receiver and the MIPI CSI-2 serial bus
> +configuration is specified in the respective endpoints as well. The CSI-2
> +receiver is then linked to the DMA engine through a direct data path which does
> +not require any endpoint configuration.
> 
> -				/* If hsync-active/vsync-active are missing,
> -				   embedded BT.656 sync is used */
> -				hsync-active = <0>;	/* Active low */
> -				vsync-active = <0>;	/* Active low */
> -				data-active = <1>;	/* Active high */
> -				pclk-sample = <1>;	/* Rising */
> -			};
> +i2c0: i2c@e6500000 {
> +
> +	hdmi-decoder@4c {
> +		compatible = "adi,adv7612";
> +		reg = <0x4c>;
> 
> -			/* MIPI CSI-2 bus endpoint */
> -			ceu0_0: endpoint@0 {
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
>  				reg = <0>;
> -				remote = <&csi2_2>;
> +				adv7612_in: endpoint {
> +					remote-endpoint = <&hdmi_con_in>;
> +				};
>  			};
> -		};
> -	};
> 
> -	i2c0: i2c@fff20000 {
> -		...
> -		ov772x_1: camera@21 {
> -			compatible = "ovti,ov772x";
> -			reg = <0x21>;
> -			vddio-supply = <&regulator1>;
> -			vddcore-supply = <&regulator2>;
> -
> -			clock-frequency = <20000000>;
> -			clocks = <&mclk 0>;
> -			clock-names = "xclk";
> -
> -			port {
> -				/* With 1 endpoint per port no need for addresses. */
> -				ov772x_1_1: endpoint {
> +			port@2 {
> +				reg = <2>;
> +				adv7612_out: endpoint {
> +					bus-type = 5;
>  					bus-width = <8>;
> -					remote-endpoint = <&ceu0_1>;
> -					hsync-active = <1>;
> -					vsync-active = <0>; /* Who came up with an
> +					pclk-sample = <0>;
> +					hsync-active = <0>;
> +					vsync-active = <1>; /* Who came up with an
>  							       inverter here ?... */
> -					data-active = <1>;
> -					pclk-sample = <1>;
> +					remote-endpoint = <&vin4_digital_in>;
>  				};
>  			};
>  		};
> + 	};
> 
> -		imx074: camera@1a {
> -			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 {
> -				imx074_1: endpoint {
> -					clock-lanes = <0>;
> -					data-lanes = <1 2>;
> -					remote-endpoint = <&csi2_1>;
> -				};
> +
> +	imx074: camera@1a {
> +		compatible = "sony,imx074";
> +		reg = <0x1a>;
> +
> +		rotation = <180>; /* The camera is mounted upside down! */
> +
> +		/* With a single port, use 'port' and not 'ports'. */
> +		port {
> +			/* With 1 endpoint per port no need for addresses. */
> +			imx074_1: endpoint {
> +				bus-type = 4;
> +				/* If lane re-ordering is not supported, no
> +				   need to tell where the clock lane is! */
> +				/* clock-lanes = <0>; */
> +				/* But the number of data lanes is important! */
> +				data-lanes = <1 2>;
> +				remote-endpoint = <&csi20_in>;
>  			};
>  		};
>  	};
> +};
> 
> -	csi2: csi2@ffc90000 {
> -		compatible = "renesas,sh-mobile-csi2";
> -		reg = <0xffc90000 0x1000>;
> -		interrupts = <0x17a0>;
> +csi20: csi2@fea80000 {
> +	compatible = "renesas,r8a7795-csi2";
> +	reg = <0 0xfea80000 0 0x10000>;
> +	interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&cpg CPG_MOD 714>;
> +	power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +	resets = <&cpg 714>;

Do we need all reg, interrupts, clocks, power-domains and resets in the 
example?

> +
> +	ports {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> 
> -		port@1 {
> -			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. */
> -			csi2_1: endpoint {
> -				clock-lanes = <0>;
> -				data-lanes = <2 1>;
> +		port@0 {
> +			reg = <0>;
> +
> +			csi20_in: endpoint {
> +				bus-type = 4;
> +				/* Use the same number of data lanes as the
> +				   one used by the remote endpoint! */

nit: Do this comment bring value, or is it confusing?

> +				data-lanes = <1 2>;
>  				remote-endpoint = <&imx074_1>;
>  			};
>  		};
> -		port@2 {
> -			reg = <2>;			/* port 2: link to the CEU */
> 
> -			csi2_2: endpoint {
> -				remote-endpoint = <&ceu0_0>;
> +		port@1 {
> +			reg = <1>;
> +
> +			/* Data path to the VIN4 DMA engine. */

Needed?

> +			csi20vin4: endpoint {
> +				remote-endpoint = <&vin4csi20>;
> +			};
> +		};
> +	};
> +};
> +
> +vin4: video@e6ef4000 {
> +	compatible = "renesas,vin-r8a7795";
> +	reg = <0 0xe6ef4000 0 0x1000>;
> +	interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&cpg CPG_MOD 807>;
> +	power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +	resets = <&cpg 807>;
> +	renesas,id = <4>;

Same comment as above, is all properties needed in the example?  
Specially renesas,id can be confusing as it's a driver specific binding 
needed to workaround a fun hardware design.

> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* Parallel input port: HDMI decoder */
> +		port@0 {
> +			reg = <0>;
> +
> +			vin4_digital_in: endpoint {
> +				bus-type = 5;
> +				bus-width = <8>;	/* Used data lines */
> +				data-shift = <2>;	/* Lines 9:2 are used */
> +				data-active = <1>;	/* Active high */
> +				pclk-sample = <0>;	/* Falling */
> +				/* If hsync-active/vsync-active are missing,
> +				 * embedded BT.656 sync is used */

I feel if this comment is to be kept it should be expanded.

> +				hsync-active = <0>;
> +				vsync-active = <0>;
> +				remote-endpoint = <&adv7612_out>;
> +			};
> +		};
> +
> +
> +		/* Data path to the MIPI CSI-2 receiver. */
> +		port@1 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			reg =<1>;
> +
> +			/* Need endpoint numbers when multiple endpoints are
> +			   present. */

I think this can be dropped.

> +			vin4csi20: endpoint@0 {
> +				reg = <0>;
> +				remote-endpoint = <&csi20vin4>;
> +			};
> +
> +			/* Not connected in this example. */
> +			vin4csi41: endpoint@3 {
> +				reg = <3>;
> +				remote-endpoint = <&csi41vin4>;
>  			};
>  		};
>  	};
> +};
> --
> 2.22.0
> 

-- 
Regards,
Niklas Söderlund



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux