* Re: [PATCH v6 01/25] rcar-vin: add Gen3 devicetree bindings documentation
2017-08-22 23:26 ` [PATCH v6 01/25] rcar-vin: add Gen3 devicetree bindings documentation Niklas Söderlund
@ 2017-08-23 8:14 ` Laurent Pinchart
0 siblings, 0 replies; 2+ messages in thread
From: Laurent Pinchart @ 2017-08-23 8:14 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Hans Verkuil, Kieran Bingham, Sakari Ailus, tomoharu.fukawa.eb,
linux-media, linux-renesas-soc, Rob Herring, devicetree
Hi Niklas,
On Wednesday, 23 August 2017 02:26:16 EEST Niklas Söderlund wrote:
> Document the devicetree bindings for the CSI-2 inputs available on Gen3.
>
> There is a need to add a custom property 'renesas,id' and to define
> which CSI-2 input is described in which endpoint under the port@1 node.
> This information is needed since there are a set of predefined routes
> between each VIN and CSI-2 block. This routing table will be kept
> inside the driver but in order for it to act on it it must know which
> VIN and CSI-2 is which.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> .../devicetree/bindings/media/rcar_vin.txt | 106 ++++++++++++++++--
> 1 file changed, 96 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
> b/Documentation/devicetree/bindings/media/rcar_vin.txt index
> 6e4ef8caf759e5d3..be38ad89d71ad05d 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -2,8 +2,12 @@ Renesas R-Car Video Input driver (rcar_vin)
> -------------------------------------------
>
> The rcar_vin device provides video input capabilities for the Renesas R-Car
> -family of devices. The current blocks are always slaves and suppot one
> input -channel which can be either RGB, YUYV or BT656.
> +family of devices.
> +
> +On Gen2 the current blocks are always slaves and support one input channel
> +which can be either RGB, YUYV or BT656.
What do you mean by "are always slaves" ?
> On Gen3 the current blocks are
> +always slaves and support multiple input channels which can be either RGB,
> +YUVU, BT656 or CSI-2.
Strictly speaking VIN on Gen3 doesn't handle CSI-2, the CSI-2 receiver
deserializes the video stream and produces a parallel input.
You could word this as follows.
Each VIN instance has a single parallel input that supports RGB and YUV video,
with both external synchronization and BT.656 synchronization for the latter.
Depending on the instance the VIN input is connected to external SoC pins, or
on Gen3 to a CSI-2 receiver.
> - compatible: Must be one or more of the following
> - "renesas,vin-r8a7795" for the R8A7795 device
> @@ -28,7 +32,7 @@ channel which can be either RGB, YUYV or BT656.
> Additionally, an alias named vinX will need to be created to specify
> which video input device this is.
>
> -The per-board settings:
> +The per-board settings Gen2:
> - port sub-node describing a single endpoint connected to the vin
> as described in video-interfaces.txt[1]. Only the first one will
> be considered as each vin interface has one input port.
> @@ -36,13 +40,21 @@ The per-board settings:
> These settings are used to work out video input format and widths
> into the system.
Not related to this patch, but I don't understand how that sentence is related
to the previous one.
> +The per-board settings Gen3:
> +- renesas,id - ID number of the VIN
You should define what the ID is.
> +- Port 0 - Digital video source (same as port node on Gen2)
> +- Port 1 - CSI-2 video sources
> + - Endpoint 0 - sub-node describing the endpoint which is CSI20
> + - Endpoint 1 - sub-node describing the endpoint which is CSI21
> + - Endpoint 2 - sub-node describing the endpoint which is CSI40
> + - Endpoint 3 - sub-node describing the endpoint which is CSI41
Given that the parallel input and CSI-2 input are mutually exclusive,
shouldn't the VIN have a single port ?
I think nodes and endpoints need slightly more detailed documentation.
> -Device node example
> --------------------
> +Device node example Gen2
> +------------------------
>
> - aliases {
> - vin0 = &vin0;
> - };
> + aliases {
> + vin0 = &vin0;
> + };
Do we need the aliases ?
> vin0: vin@0xe6ef0000 {
> compatible = "renesas,vin-r8a7790",
> "renesas,rcar-gen2-vin"; @@ -52,8 +64,8 @@ Device node example
> status = "disabled";
> };
>
> -Board setup example (vin1 composite video input)
> -------------------------------------------------
> +Board setup example Gen2 (vin1 composite video input)
> +-----------------------------------------------------
>
> &i2c2 {
> status = "ok";
> @@ -92,6 +104,80 @@ Board setup example (vin1 composite video input)
> };
> };
>
> +Device node example Gen3
> +------------------------
> +
> + vin0: video@e6ef0000 {
> + compatible = "renesas,vin-r8a7795";
> + reg = <0 0xe6ef0000 0 0x1000>;
> + interrupts = <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cpg CPG_MOD 811>;
> + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> + status = "disabled";
> +
> + renesas,id = <0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reg = <1>;
> +
> + vin0csi20: endpoint@0 {
> + reg = <0>;
> + remote-endpoint= <&csi20vin0>;
> + };
> + vin0csi21: endpoint@1 {
> + reg = <1>;
> + remote-endpoint= <&csi21vin0>;
> + };
> + vin0csi40: endpoint@2 {
> + reg = <2>;
> + remote-endpoint= <&csi40vin0>;
> + };
> + };
> + };
> + };
> +
> + csi20: csi2@fea80000 {
> + compatible = "renesas,r8a7795-csi2",
> "renesas,rcar-gen3-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>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
Wrong indentation.
> +
> + reg = <0>;
> + csi20_in: endpoint@0 {
Do you need to number the endpoint ? If not you could omit the #address-cells
and #size-cells properties. Otherwise you need a reg property here.
> + clock-lanes = <0>;
> + data-lanes = <1>;
> + remote-endpoint = <&adv7482_txb>;
> + };
> + };
>
> + port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reg = <1>;
> +
> + csi20vin0: endpoint@0 {
I assume this one needs to be numbered as will have multiple endpoints (one
per remote VIN).
> + remote-endpoint = <&vin0csi20>;
> + };
> + };
> + };
> + };
>
> [1] video-interfaces.txt common video media interface
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 2+ messages in thread