[PATCH v2 0/4] media: rcar-vin: Brush endpoint properties

[PATCH v2 0/4] media: rcar-vin: Brush endpoint properties

[Jacopo Mondi]

Hello,
   this series touches the bindings and the driver handling endpoint
properties for digital subdevices of the R-Car VIN driver.

The first patch simply documents what are the endpoint properties supported
at the moment while the second extends them with 'data-active' one.

As the VIN hardware allows to use HSYNC as data enable signal when the CLCKENB
pin is left unconnected, the 'data-active' property presence determinates
if HSYNC has to be used or not as data enable signal. Compared to v1, the
handling logic of this property has changed, and HSYNC is selected in its place
only when running V4L2_MBUS_PARALLEL media bus type. When running with
embedded synchronizations if the property is not specified, the default value
is used (this helps with retro-compatibility too).

I understand this may sound problematic. The property is optional, but when
not specified it may cause HSYNC to be used as data-active, which is not what
one may expect. Anyhow, the processor manual suggests this:

Note: When using ITU-R BT.601, BT.709, BT.1358 interface, and the
VIn_CLKENB pin is unused, the CHS bit must be set to 1.

So it seems reasonable to me.

Alternatively, a dedicated property, as 'renesas,chs' may be introduced, but
I don't particularly like this and I would prefer users to read bindings
documentation carefully, provided I wrote it clearly enough...

While this is still debated too, I left un-touched patch [4/4] that removes
un-used endpoint properties from all Gen-2 DTS that use them. Those properties
are not parsed by the driver nor documented by bindings, and their presence is
only confusing future users.

The series depends on:
[PATCH v3 0/9] rcar-vin: Add support for parallel input on Gen3

Available here for the interested:
git://jmondi.org d3/media-master/salvator-x-dts_csi2/d3-vin-driver-v3

Thanks
   j

v1 -> v2:
- Change data-active properties handling: the property is only considered
  when running with explicit synchronization signals.
- Drop patch 3/6 and 6/6 from v1 as they're not needed anymore as data-active
  handling logic changed.

Jacopo Mondi (4):
  dt-bindings: media: rcar-vin: Describe optional ep properties
  dt-bindings: media: rcar-vin: Document data-active
  media: rcar-vin: Handle CLOCKENB pin polarity
  ARM: dts: rcar-gen2: Remove unused VIN properties

 .../devicetree/bindings/media/rcar_vin.txt         | 19 +++++++++++++++-
 arch/arm/boot/dts/r8a7790-lager.dts                |  3 ---
 arch/arm/boot/dts/r8a7791-koelsch.dts              |  3 ---
 arch/arm/boot/dts/r8a7791-porter.dts               |  1 -
 arch/arm/boot/dts/r8a7793-gose.dts                 |  3 ---
 arch/arm/boot/dts/r8a7794-alt.dts                  |  1 -
 arch/arm/boot/dts/r8a7794-silk.dts                 |  1 -
 drivers/media/platform/rcar-vin/rcar-dma.c         | 25 ++++++++++++++--------
 8 files changed, 34 insertions(+), 22 deletions(-)

--
2.7.4

[PATCH v2 1/4] dt-bindings: media: rcar-vin: Describe optional ep properties

[Jacopo Mondi]

Describe the optional properties for endpoint nodes of port@0
and port@1 of the R-Car VIN driver device tree bindings documentation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 5c6f2a7..dab3118 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -54,6 +54,16 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
       from external SoC pins described in video-interfaces.txt[1].
       Describing more then one endpoint in port 0 is invalid. Only VIN
       instances that are connected to external pins should have port 0.
+
+      - Optional properties for endpoint nodes of port@0:
+        - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH
+	  respectively. Default is active high.
+        - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH
+	  respectively. Default is active high.
+
+	If both HSYNC and VSYNC polarities are not specified, embedded
+	synchronization is selected.
+
     - port 1 - sub-nodes describing one or more endpoints connected to
       the VIN from local SoC CSI-2 receivers. The endpoint numbers must
       use the following schema.
@@ -63,6 +73,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
         - Endpoint 2 - sub-node describing the endpoint connected to CSI40
         - Endpoint 3 - sub-node describing the endpoint connected to CSI41
 
+      Endpoint nodes of port@1 do not support any optional endpoint property.
+
 Device node example for Gen2 platforms
 --------------------------------------
 
@@ -113,7 +125,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
 
                 vin1ep0: endpoint {
                         remote-endpoint = <&adv7180>;
-                        bus-width = <8>;
                 };
         };
 };
-- 
2.7.4

Re: [PATCH v2 1/4] dt-bindings: media: rcar-vin: Describe optional ep properties

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-21 19:27:40 +0200, Jacopo Mondi wrote:
> Describe the optional properties for endpoint nodes of port@0
> and port@1 of the R-Car VIN driver device tree bindings documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index 5c6f2a7..dab3118 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -54,6 +54,16 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
>        from external SoC pins described in video-interfaces.txt[1].
>        Describing more then one endpoint in port 0 is invalid. Only VIN
>        instances that are connected to external pins should have port 0.
> +
> +      - Optional properties for endpoint nodes of port@0:
> +        - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH
> +	  respectively. Default is active high.
> +        - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH
> +	  respectively. Default is active high.
> +
> +	If both HSYNC and VSYNC polarities are not specified, embedded
> +	synchronization is selected.
> +
>      - port 1 - sub-nodes describing one or more endpoints connected to
>        the VIN from local SoC CSI-2 receivers. The endpoint numbers must
>        use the following schema.
> @@ -63,6 +73,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
>          - Endpoint 2 - sub-node describing the endpoint connected to CSI40
>          - Endpoint 3 - sub-node describing the endpoint connected to CSI41
>  
> +      Endpoint nodes of port@1 do not support any optional endpoint property.
> +
>  Device node example for Gen2 platforms
>  --------------------------------------
>  
> @@ -113,7 +125,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
>  
>                  vin1ep0: endpoint {
>                          remote-endpoint = <&adv7180>;
> -                        bus-width = <8>;

Until we have figured out if we should document or remove the bus-width 
parameter maybe move this to 4/4 ?


>                  };
>          };
>  };
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH v2 2/4] dt-bindings: media: rcar-vin: Document data-active

[Jacopo Mondi]

Document 'data-active' property in R-Car VIN device tree bindings.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

v1 -> v2:
- HSYNC is used in place of data enable signal only when running with
  explicit synchronizations.
- The property is no more mandatory when running with embedded
  synchronizations, and default is selected.
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index dab3118..2c144b4 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -64,6 +64,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
 	If both HSYNC and VSYNC polarities are not specified, embedded
 	synchronization is selected.
 
+        - data-active: data enable signal line polarity (CLKENB pin).
+          0/1 for LOW/HIGH respectively. If not specified and running with
+	  embedded synchronization, the default is active high. If not
+	  specified and running with explicit synchronization, HSYNC is used
+	  as data enable signal.
+
     - port 1 - sub-nodes describing one or more endpoints connected to
       the VIN from local SoC CSI-2 receivers. The endpoint numbers must
       use the following schema.
-- 
2.7.4

Re: [PATCH v2 2/4] dt-bindings: media: rcar-vin: Document data-active

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-21 19:27:41 +0200, Jacopo Mondi wrote:
> Document 'data-active' property in R-Car VIN device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> v1 -> v2:
> - HSYNC is used in place of data enable signal only when running with
>   explicit synchronizations.
> - The property is no more mandatory when running with embedded
>   synchronizations, and default is selected.
> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index dab3118..2c144b4 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -64,6 +64,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
>  	If both HSYNC and VSYNC polarities are not specified, embedded
>  	synchronization is selected.
>  
> +        - data-active: data enable signal line polarity (CLKENB pin).
> +          0/1 for LOW/HIGH respectively. If not specified and running with
> +	  embedded synchronization, the default is active high. If not
> +	  specified and running with explicit synchronization, HSYNC is used
> +	  as data enable signal.

This indentation looks funny :-)

If you check the rest of the rcar_vin.txt file only spaces are used for
indentation.

> +
>      - port 1 - sub-nodes describing one or more endpoints connected to
>        the VIN from local SoC CSI-2 receivers. The endpoint numbers must
>        use the following schema.
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH v2 3/4] media: rcar-vin: Handle CLOCKENB pin polarity

[Jacopo Mondi]

Handle CLOCKENB pin polarity, or use HSYNC in its place if polarity is
not specified and we're running on parallel data bus with explicit
synchronism signals.

While at there, simplify the media bus handling flags logic, inspecting
flags only if the system is running on parallel media bus type and ignore
flags when on CSI-2. Also change comments style to remove un-necessary
camel case and add a full stop at the end of sentences.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 34 ++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 17f291f..ffd3d62 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -123,6 +123,8 @@
 /* Video n Data Mode Register 2 bits */
 #define VNDMR2_VPS		(1 << 30)
 #define VNDMR2_HPS		(1 << 29)
+#define VNDMR2_CES		(1 << 28)
+#define VNDMR2_CHS		(1 << 23)
 #define VNDMR2_FTEV		(1 << 17)
 #define VNDMR2_VLV(n)		((n & 0xf) << 12)
 
@@ -684,21 +686,35 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	}
 
-	/* Enable VSYNC Field Toogle mode after one VSYNC input */
+	/* Enable VSYNC field toggle mode after one VSYNC input. */
 	if (vin->info->model == RCAR_GEN3)
 		dmr2 = VNDMR2_FTEV;
 	else
 		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
 
-	/* Hsync Signal Polarity Select */
-	if (!vin->is_csi &&
-	    !(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
-		dmr2 |= VNDMR2_HPS;
+	/* Synchronism signal polarities: only for parallel data bus. */
+	if (!vin->is_csi) {
+		/* Hsync signal polarity select. */
+		if (!(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
+			dmr2 |= VNDMR2_HPS;
 
-	/* Vsync Signal Polarity Select */
-	if (!vin->is_csi &&
-	    !(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
-		dmr2 |= VNDMR2_VPS;
+		/* Vsync signal polarity select. */
+		if (!(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+			dmr2 |= VNDMR2_VPS;
+
+		/*
+		 * Data enable signal polarity select.
+		 * Use HSYNC as data-enable if not specified and running
+		 * with explicit synchronizations; otherwise default 'high'
+		 * is selected.
+		 */
+		if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ACTIVE_LOW)
+			dmr2 |= VNDMR2_CES;
+		else if (!(vin->parallel->mbus_flags &
+			 V4L2_MBUS_DATA_ACTIVE_HIGH) &&
+			 vin->parallel->mbus_type == V4L2_MBUS_PARALLEL)
+			dmr2 |= VNDMR2_CHS;
+	}
 
 	/*
 	 * Output format
-- 
2.7.4

Re: [PATCH v2 3/4] media: rcar-vin: Handle CLOCKENB pin polarity

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-21 19:27:42 +0200, Jacopo Mondi wrote:
> Handle CLOCKENB pin polarity, or use HSYNC in its place if polarity is
> not specified and we're running on parallel data bus with explicit
> synchronism signals.
> 
> While at there, simplify the media bus handling flags logic, inspecting
> flags only if the system is running on parallel media bus type and ignore
> flags when on CSI-2. Also change comments style to remove un-necessary
> camel case and add a full stop at the end of sentences.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 34 ++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 17f291f..ffd3d62 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -123,6 +123,8 @@
>  /* Video n Data Mode Register 2 bits */
>  #define VNDMR2_VPS		(1 << 30)
>  #define VNDMR2_HPS		(1 << 29)
> +#define VNDMR2_CES		(1 << 28)
> +#define VNDMR2_CHS		(1 << 23)
>  #define VNDMR2_FTEV		(1 << 17)
>  #define VNDMR2_VLV(n)		((n & 0xf) << 12)
>  
> @@ -684,21 +686,35 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	}
>  
> -	/* Enable VSYNC Field Toogle mode after one VSYNC input */
> +	/* Enable VSYNC field toggle mode after one VSYNC input. */
>  	if (vin->info->model == RCAR_GEN3)
>  		dmr2 = VNDMR2_FTEV;
>  	else
>  		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
>  
> -	/* Hsync Signal Polarity Select */
> -	if (!vin->is_csi &&
> -	    !(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> -		dmr2 |= VNDMR2_HPS;
> +	/* Synchronism signal polarities: only for parallel data bus. */
> +	if (!vin->is_csi) {
> +		/* Hsync signal polarity select. */
> +		if (!(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> +			dmr2 |= VNDMR2_HPS;
>  
> -	/* Vsync Signal Polarity Select */
> -	if (!vin->is_csi &&
> -	    !(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> -		dmr2 |= VNDMR2_VPS;
> +		/* Vsync signal polarity select. */
> +		if (!(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> +			dmr2 |= VNDMR2_VPS;
> +
> +		/*
> +		 * Data enable signal polarity select.
> +		 * Use HSYNC as data-enable if not specified and running
> +		 * with explicit synchronizations; otherwise default 'high'
> +		 * is selected.
> +		 */
> +		if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ACTIVE_LOW)
> +			dmr2 |= VNDMR2_CES;
> +		else if (!(vin->parallel->mbus_flags &
> +			 V4L2_MBUS_DATA_ACTIVE_HIGH) &&
> +			 vin->parallel->mbus_type == V4L2_MBUS_PARALLEL)
> +			dmr2 |= VNDMR2_CHS;

As I stated in v1, I think this expression is more complex then is relly
needed here. Would not something like this be much more readable?

        /* Clock Enable signal polarity select. */
	if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ACTIVE_LOW)
                dmr2 |= VNDMR2_CES;

        /* Use HSYNC as clock enable if VIn_CLKENB is not available. */
        if (!(vin->parallel->mbus_flags & (V4L2_MBUS_DATA_ACTIVE_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH)))
                dmr2 |= VNDMR2_CHS;


> +	}
>  
>  	/*
>  	 * Output format
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH v2 4/4] ARM: dts: rcar-gen2: Remove unused VIN properties

[Jacopo Mondi]

The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
driver and only confuse users. Remove them in all Gen2 SoC that use
them.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm/boot/dts/r8a7790-lager.dts   | 3 ---
 arch/arm/boot/dts/r8a7791-koelsch.dts | 3 ---
 arch/arm/boot/dts/r8a7791-porter.dts  | 1 -
 arch/arm/boot/dts/r8a7793-gose.dts    | 3 ---
 arch/arm/boot/dts/r8a7794-alt.dts     | 1 -
 arch/arm/boot/dts/r8a7794-silk.dts    | 1 -
 6 files changed, 12 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index 092610e..9cdabfcf 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -885,10 +885,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};
@@ -904,7 +902,6 @@
 	port {
 		vin1ep0: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
index 8ab793d..033c9e3 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -857,10 +857,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};
@@ -875,7 +873,6 @@
 	port {
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
index a01101b..c16e870 100644
--- a/arch/arm/boot/dts/r8a7791-porter.dts
+++ b/arch/arm/boot/dts/r8a7791-porter.dts
@@ -388,7 +388,6 @@
 	port {
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/r8a7793-gose.dts b/arch/arm/boot/dts/r8a7793-gose.dts
index aa209f6..60aaddb 100644
--- a/arch/arm/boot/dts/r8a7793-gose.dts
+++ b/arch/arm/boot/dts/r8a7793-gose.dts
@@ -765,10 +765,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};
@@ -784,7 +782,6 @@
 	port {
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180_out>;
-			bus-width = <8>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/r8a7794-alt.dts b/arch/arm/boot/dts/r8a7794-alt.dts
index e170275..8ed7a71 100644
--- a/arch/arm/boot/dts/r8a7794-alt.dts
+++ b/arch/arm/boot/dts/r8a7794-alt.dts
@@ -388,7 +388,6 @@
 	port {
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/r8a7794-silk.dts b/arch/arm/boot/dts/r8a7794-silk.dts
index 7808aae..6adfcd6 100644
--- a/arch/arm/boot/dts/r8a7794-silk.dts
+++ b/arch/arm/boot/dts/r8a7794-silk.dts
@@ -477,7 +477,6 @@
 	port {
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
-- 
2.7.4

Re: [PATCH v2 4/4] ARM: dts: rcar-gen2: Remove unused VIN properties

[Simon Horman]

On Mon, May 21, 2018 at 07:27:43PM +0200, Jacopo Mondi wrote:

> The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> driver and only confuse users. Remove them in all Gen2 SoC that use
> them.

I think that the rational for removing properties (or not) is their
presence in the bindings as DT should describe the hardware and not the
current state of the driver implementation.

I see that 'bus-width' may be removed from the binding, as per discussion
in a different sub-thread. I'd like that discussion to reach a conclusion
before considering that part of this patch any further.

And I'd appreciate Niklas's feedback on the 'pclk-sample' portion.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm/boot/dts/r8a7790-lager.dts   | 3 ---
>  arch/arm/boot/dts/r8a7791-koelsch.dts | 3 ---
>  arch/arm/boot/dts/r8a7791-porter.dts  | 1 -
>  arch/arm/boot/dts/r8a7793-gose.dts    | 3 ---
>  arch/arm/boot/dts/r8a7794-alt.dts     | 1 -
>  arch/arm/boot/dts/r8a7794-silk.dts    | 1 -
>  6 files changed, 12 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index 092610e..9cdabfcf 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -885,10 +885,8 @@
>  	port {
>  		vin0ep2: endpoint {
>  			remote-endpoint = <&adv7612_out>;
> -			bus-width = <24>;
>  			hsync-active = <0>;
>  			vsync-active = <0>;
> -			pclk-sample = <1>;
>  			data-active = <1>;
>  		};
>  	};
> @@ -904,7 +902,6 @@
>  	port {
>  		vin1ep0: endpoint {
>  			remote-endpoint = <&adv7180>;
> -			bus-width = <8>;
>  		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
> index 8ab793d..033c9e3 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> @@ -857,10 +857,8 @@
>  	port {
>  		vin0ep2: endpoint {
>  			remote-endpoint = <&adv7612_out>;
> -			bus-width = <24>;
>  			hsync-active = <0>;
>  			vsync-active = <0>;
> -			pclk-sample = <1>;
>  			data-active = <1>;
>  		};
>  	};
> @@ -875,7 +873,6 @@
>  	port {
>  		vin1ep: endpoint {
>  			remote-endpoint = <&adv7180>;
> -			bus-width = <8>;
>  		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
> index a01101b..c16e870 100644
> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> @@ -388,7 +388,6 @@
>  	port {
>  		vin0ep: endpoint {
>  			remote-endpoint = <&adv7180>;
> -			bus-width = <8>;
>  		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/r8a7793-gose.dts b/arch/arm/boot/dts/r8a7793-gose.dts
> index aa209f6..60aaddb 100644
> --- a/arch/arm/boot/dts/r8a7793-gose.dts
> +++ b/arch/arm/boot/dts/r8a7793-gose.dts
> @@ -765,10 +765,8 @@
>  	port {
>  		vin0ep2: endpoint {
>  			remote-endpoint = <&adv7612_out>;
> -			bus-width = <24>;
>  			hsync-active = <0>;
>  			vsync-active = <0>;
> -			pclk-sample = <1>;
>  			data-active = <1>;
>  		};
>  	};
> @@ -784,7 +782,6 @@
>  	port {
>  		vin1ep: endpoint {
>  			remote-endpoint = <&adv7180_out>;
> -			bus-width = <8>;
>  		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/r8a7794-alt.dts b/arch/arm/boot/dts/r8a7794-alt.dts
> index e170275..8ed7a71 100644
> --- a/arch/arm/boot/dts/r8a7794-alt.dts
> +++ b/arch/arm/boot/dts/r8a7794-alt.dts
> @@ -388,7 +388,6 @@
>  	port {
>  		vin0ep: endpoint {
>  			remote-endpoint = <&adv7180>;
> -			bus-width = <8>;
>  		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/r8a7794-silk.dts b/arch/arm/boot/dts/r8a7794-silk.dts
> index 7808aae..6adfcd6 100644
> --- a/arch/arm/boot/dts/r8a7794-silk.dts
> +++ b/arch/arm/boot/dts/r8a7794-silk.dts
> @@ -477,7 +477,6 @@
>  	port {
>  		vin0ep: endpoint {
>  			remote-endpoint = <&adv7180>;
> -			bus-width = <8>;
>  		};
>  	};
>  };
> -- 
> 2.7.4
> 

Re: [PATCH v2 4/4] ARM: dts: rcar-gen2: Remove unused VIN properties

[Niklas Söderlund]

Hi Simon,

On 2018-06-04 11:53:09 +0200, Simon Horman wrote:
> On Mon, May 21, 2018 at 07:27:43PM +0200, Jacopo Mondi wrote:
> 
> > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> > driver and only confuse users. Remove them in all Gen2 SoC that use
> > them.
> 
> I think that the rational for removing properties (or not) is their
> presence in the bindings as DT should describe the hardware and not the
> current state of the driver implementation.
> 
> I see that 'bus-width' may be removed from the binding, as per discussion
> in a different sub-thread. I'd like that discussion to reach a conclusion
> before considering that part of this patch any further.
> 
> And I'd appreciate Niklas's feedback on the 'pclk-sample' portion.

My thoughts on 'pclk-sample' is the same as for 'bus-width', they 
describe the hardware. So we either should keep or remove both. As our 
discussion in the other thread I'm leaning towards that both should be 
kept.

> 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  arch/arm/boot/dts/r8a7790-lager.dts   | 3 ---
> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 3 ---
> >  arch/arm/boot/dts/r8a7791-porter.dts  | 1 -
> >  arch/arm/boot/dts/r8a7793-gose.dts    | 3 ---
> >  arch/arm/boot/dts/r8a7794-alt.dts     | 1 -
> >  arch/arm/boot/dts/r8a7794-silk.dts    | 1 -
> >  6 files changed, 12 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > index 092610e..9cdabfcf 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -885,10 +885,8 @@
> >  	port {
> >  		vin0ep2: endpoint {
> >  			remote-endpoint = <&adv7612_out>;
> > -			bus-width = <24>;
> >  			hsync-active = <0>;
> >  			vsync-active = <0>;
> > -			pclk-sample = <1>;
> >  			data-active = <1>;
> >  		};
> >  	};
> > @@ -904,7 +902,6 @@
> >  	port {
> >  		vin1ep0: endpoint {
> >  			remote-endpoint = <&adv7180>;
> > -			bus-width = <8>;
> >  		};
> >  	};
> >  };
> > diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > index 8ab793d..033c9e3 100644
> > --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> > +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> > @@ -857,10 +857,8 @@
> >  	port {
> >  		vin0ep2: endpoint {
> >  			remote-endpoint = <&adv7612_out>;
> > -			bus-width = <24>;
> >  			hsync-active = <0>;
> >  			vsync-active = <0>;
> > -			pclk-sample = <1>;
> >  			data-active = <1>;
> >  		};
> >  	};
> > @@ -875,7 +873,6 @@
> >  	port {
> >  		vin1ep: endpoint {
> >  			remote-endpoint = <&adv7180>;
> > -			bus-width = <8>;
> >  		};
> >  	};
> >  };
> > diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts
> > index a01101b..c16e870 100644
> > --- a/arch/arm/boot/dts/r8a7791-porter.dts
> > +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> > @@ -388,7 +388,6 @@
> >  	port {
> >  		vin0ep: endpoint {
> >  			remote-endpoint = <&adv7180>;
> > -			bus-width = <8>;
> >  		};
> >  	};
> >  };
> > diff --git a/arch/arm/boot/dts/r8a7793-gose.dts b/arch/arm/boot/dts/r8a7793-gose.dts
> > index aa209f6..60aaddb 100644
> > --- a/arch/arm/boot/dts/r8a7793-gose.dts
> > +++ b/arch/arm/boot/dts/r8a7793-gose.dts
> > @@ -765,10 +765,8 @@
> >  	port {
> >  		vin0ep2: endpoint {
> >  			remote-endpoint = <&adv7612_out>;
> > -			bus-width = <24>;
> >  			hsync-active = <0>;
> >  			vsync-active = <0>;
> > -			pclk-sample = <1>;
> >  			data-active = <1>;
> >  		};
> >  	};
> > @@ -784,7 +782,6 @@
> >  	port {
> >  		vin1ep: endpoint {
> >  			remote-endpoint = <&adv7180_out>;
> > -			bus-width = <8>;
> >  		};
> >  	};
> >  };
> > diff --git a/arch/arm/boot/dts/r8a7794-alt.dts b/arch/arm/boot/dts/r8a7794-alt.dts
> > index e170275..8ed7a71 100644
> > --- a/arch/arm/boot/dts/r8a7794-alt.dts
> > +++ b/arch/arm/boot/dts/r8a7794-alt.dts
> > @@ -388,7 +388,6 @@
> >  	port {
> >  		vin0ep: endpoint {
> >  			remote-endpoint = <&adv7180>;
> > -			bus-width = <8>;
> >  		};
> >  	};
> >  };
> > diff --git a/arch/arm/boot/dts/r8a7794-silk.dts b/arch/arm/boot/dts/r8a7794-silk.dts
> > index 7808aae..6adfcd6 100644
> > --- a/arch/arm/boot/dts/r8a7794-silk.dts
> > +++ b/arch/arm/boot/dts/r8a7794-silk.dts
> > @@ -477,7 +477,6 @@
> >  	port {
> >  		vin0ep: endpoint {
> >  			remote-endpoint = <&adv7180>;
> > -			bus-width = <8>;
> >  		};
> >  	};
> >  };
> > -- 
> > 2.7.4
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 4/4] ARM: dts: rcar-gen2: Remove unused VIN properties

[Simon Horman]

On Mon, Jun 04, 2018 at 01:31:54PM +0200, Niklas Söderlund wrote:
> Hi Simon,
> 
> On 2018-06-04 11:53:09 +0200, Simon Horman wrote:
> > On Mon, May 21, 2018 at 07:27:43PM +0200, Jacopo Mondi wrote:
> > 
> > > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> > > driver and only confuse users. Remove them in all Gen2 SoC that use
> > > them.
> > 
> > I think that the rational for removing properties (or not) is their
> > presence in the bindings as DT should describe the hardware and not the
> > current state of the driver implementation.
> > 
> > I see that 'bus-width' may be removed from the binding, as per discussion
> > in a different sub-thread. I'd like that discussion to reach a conclusion
> > before considering that part of this patch any further.
> > 
> > And I'd appreciate Niklas's feedback on the 'pclk-sample' portion.
> 
> My thoughts on 'pclk-sample' is the same as for 'bus-width', they 
> describe the hardware. So we either should keep or remove both. As our 
> discussion in the other thread I'm leaning towards that both should be 
> kept.

Thanks, that sounds reasonable to me.

I'm marking (v4 of) this as deferred pending a conclusion to that
conversation.

Re: [PATCH v2 4/4] ARM: dts: rcar-gen2: Remove unused VIN properties

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

Hi Niklas,

On Mon, Jun 04, 2018 at 02:25:15PM +0200, Simon Horman wrote:
> On Mon, Jun 04, 2018 at 01:31:54PM +0200, Niklas Söderlund wrote:
> > Hi Simon,
> >
> > On 2018-06-04 11:53:09 +0200, Simon Horman wrote:
> > > On Mon, May 21, 2018 at 07:27:43PM +0200, Jacopo Mondi wrote:
> > >
> > > > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> > > > driver and only confuse users. Remove them in all Gen2 SoC that use
> > > > them.
> > >
> > > I think that the rational for removing properties (or not) is their
> > > presence in the bindings as DT should describe the hardware and not the
> > > current state of the driver implementation.
> > >
> > > I see that 'bus-width' may be removed from the binding, as per discussion
> > > in a different sub-thread. I'd like that discussion to reach a conclusion
> > > before considering that part of this patch any further.
> > >
> > > And I'd appreciate Niklas's feedback on the 'pclk-sample' portion.
> >
> > My thoughts on 'pclk-sample' is the same as for 'bus-width', they
> > describe the hardware. So we either should keep or remove both. As our
> > discussion in the other thread I'm leaning towards that both should be
> > kept.

Am I wrong, or we discussed about the face there is no way to specify
the bus widht of the VIN parallel input interface?

>
> Thanks, that sounds reasonable to me.

So they should be documented if we want to keep them. Currently, they
are not.

And again, someone integrating a sensor may try to play around with those
properties, expecting them to change the interface behavior and wasting time
trying to figure out where the error is, on sensor side or VIN side, before
realizing those properties are actually ignored.

Anyway, the most important thing, if we want to keep them is to
provide a patch to document them as optional properties of the VIN
endpoint.

Thanks
   j

>
> I'm marking (v4 of) this as deferred pending a conclusion to that
> conversation.
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]