media: rcar-vin: Brush endpoint properties

PATCH Superseded Jacopo Mondi 7 participants 25 messages May 16, 2018 – May 23, 2018

Subsystems (3)

Device tree bindings Media (V4L/DVB) Arm/RISC-V/Renesas architecture

lore

Share & tools

lore

Jacopo Mondi

author

2018-05-16 16:32 UTC #0 lore raw

cover media: rcar-vin: Brush endpoint properties

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, then the second one extends them with 'data-active'.

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. As a consequence, when running with embedded synchronism, and there is not HSYNC signal, it becomes mandatory to specify 'data-active' polarity in DTS.

To address this, all Gen-2 boards featuring a composite video input and running with embedded synchronization, now need that property to be specified in DTS. Before adding it, remove un-used properties as 'pclk-sample' and 'bus-width' from the Gen-2 bindings, as they are not parsed by the VIN driver and only confuse users.

Niklas, as you already know I don't have any Gen2 board. Could you give this a spin on Koelsch if you like the series?

Thanks
j

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

 Documentation/devicetree/bindings/media/rcar_vin.txt | 18 +++++++++++++++++-
 arch/arm/boot/dts/r8a7790-lager.dts                  |  4 +---
 arch/arm/boot/dts/r8a7791-koelsch.dts                |  4 +---
 arch/arm/boot/dts/r8a7791-porter.dts                 |  2 +-
 arch/arm/boot/dts/r8a7793-gose.dts                   |  4 +---
 arch/arm/boot/dts/r8a7794-alt.dts                    |  2 +-
 arch/arm/boot/dts/r8a7794-silk.dts                   |  2 +-
 drivers/media/platform/rcar-vin/rcar-core.c          | 10 ++++++++--
 drivers/media/platform/rcar-vin/rcar-dma.c           | 11 +++++++++++
 9 files changed, 42 insertions(+), 15 deletions(-)

--
2.7.4

Jacopo Mondi

author

2018-05-16 16:32 UTC #1 lore raw

PATCH 1/6 dt-bindings: media: rcar-vin: Describe optional ep properties

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

Signed-off-by Jacopo Mondi <[email protected]>

---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/media/rcar_vin.txt +12 -1

index a19517e1..c53ce4e 100644
@@ -53,6 +53,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 at 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.

@@ -62,6 +72,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 at 1 do not support any optional endpoint property.
+
 Device node example for Gen2 platforms
 --------------------------------------

@@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
 
                 vin1ep0: endpoint {
                         remote-endpoint = <&adv7180>;
-                        bus-width = <8>;
                 };
         };
 };

--
2.7.4

Niklas Söderlund

2018-05-16 21:18 UTC #2 lore raw

PATCH 1/6 dt-bindings: media: rcar-vin: Describe optional ep properties

Hi Jacopo,

Thanks for your patch.

Quoted 4 lines · Jacopo Mondi

Describe the optional endpoint properties for endpoint nodes of port at 0 and port at 1 of the R-Car VIN driver device tree bindings documentation.
Signed-off-by Jacopo Mondi <[email protected]>

Acked-by Niklas S?derlund <[email protected]>

Quoted 45 lines

---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/media/rcar_vin.txt +12 -2

index a19517e1..c53ce4e 100644
@@ -53,6 +53,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 at 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.

@@ -62,6 +72,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 at 1 do not support any optional endpoint property.
+
 Device node example for Gen2 platforms
 --------------------------------------

@@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
 
                 vin1ep0: endpoint {
                         remote-endpoint = <&adv7180>;
-                        bus-width = <8>;
                 };
         };
 };
--

2.7.4

Signature · 2 lines

Regards,
Niklas S?derlund

Rob Herring

maintainer

2018-05-23 16:29 UTC #3 lore raw

PATCH 1/6 dt-bindings: media: rcar-vin: Describe optional ep properties

In reply to Jacopo Mondi

Quoted 15 lines · Jacopo Mondi

Describe the optional endpoint properties for endpoint nodes of port at 0 and port at 1 of the R-Car VIN driver device tree bindings documentation.
Signed-off-by Jacopo Mondi <[email protected]>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0
index a19517e1..c53ce4e 100644
@@ -53,6 +53,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

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 64

Quoted 10 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +9 -0

       instances that are connected to external pins should have port 0.
+
+      - Optional properties for endpoint nodes of port at 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.

No need to copy-n-paste from video-interfaces.txt. Just "see video-interfaces.txt" for the description is fine.

Quoted 24 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +3 -2

+
     - 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.

@@ -62,6 +72,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 at 1 do not support any optional endpoint property.
+
 Device node example for Gen2 platforms
 --------------------------------------

@@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
 
                 vin1ep0: endpoint {
                         remote-endpoint = <&adv7180>;
-                        bus-width = <8>;
                 };
         };
 };
--

2.7.4

Laurent Pinchart

2018-05-23 19:38 UTC #4 lore raw

PATCH 1/6 dt-bindings: media: rcar-vin: Describe optional ep properties

Hi Rob,

Quoted 22 lines · On Wednesday, 23 May 2018 19:29:47 EEST Rob Herring wrote:

Quoted 21 lines · Jacopo Mondi
Describe the optional endpoint properties for endpoint nodes of port at 0 and port at 1 of the R-Car VIN driver device tree bindings documentation.
Signed-off-by Jacopo Mondi <[email protected]>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/media/rcar_vin.txt index a19517e1..c53ce4e 100644
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0
@@ -53,6 +53,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.

Quoted 14 lines

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 64
Quoted 11 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +3 -0
+
+      - Optional properties for endpoint nodes of port at 0:
+        - hsync-active: active state of the HSYNC signal, 0/1 for
LOW/HIGH
Documentation/devicetree/bindings/media/rcar_vin.txt +2 -0
+	  respectively. Default is active high.
+        - vsync-active: active state of the VSYNC signal, 0/1 for
LOW/HIGH
Documentation/devicetree/bindings/media/rcar_vin.txt +4 -0
+	  respectively. Default is active high.
+
+	If both HSYNC and VSYNC polarities are not specified, embedded
+	synchronization is selected.
No need to copy-n-paste from video-interfaces.txt. Just "see video-interfaces.txt" for the description is fine.

I would still explicitly list the properties that apply to this binding. I agree that there's no need to copy & paste the description of those properties though.

Quoted 33 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +2 -0

+
 
     - 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.

@@ -62,6 +72,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 at 1 do not support any optional endpoint

property. +

Device node example for Gen2 platforms
--------------------------------------

Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0

@@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite

video input)>

                 vin1ep0: endpoint {
                 
                         remote-endpoint = <&adv7180>;

- bus-width = <8>;

                 };
         
         };

};

Signature · 3 lines

Regards,

Laurent Pinchart

Rob Herring

maintainer

2018-05-23 19:55 UTC #5 lore raw

PATCH 1/6 dt-bindings: media: rcar-vin: Describe optional ep properties

Quoted 40 lines · Laurent Pinchart

Hi Rob,
Quoted 22 lines · On Wednesday, 23 May 2018 19:29:47 EEST Rob Herring wrote:
Quoted 21 lines · Jacopo Mondi
Describe the optional endpoint properties for endpoint nodes of port at 0 and port at 1 of the R-Car VIN driver device tree bindings documentation.
Signed-off-by Jacopo Mondi <[email protected]>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/media/rcar_vin.txt index a19517e1..c53ce4e 100644
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0
@@ -53,6 +53,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.
Quoted 14 lines
Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 64
Quoted 11 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +3 -0
+
+      - Optional properties for endpoint nodes of port at 0:
+        - hsync-active: active state of the HSYNC signal, 0/1 for
LOW/HIGH
Documentation/devicetree/bindings/media/rcar_vin.txt +2 -0
+     respectively. Default is active high.
+        - vsync-active: active state of the VSYNC signal, 0/1 for
LOW/HIGH
Documentation/devicetree/bindings/media/rcar_vin.txt +4 -0
+     respectively. Default is active high.
+
+   If both HSYNC and VSYNC polarities are not specified, embedded
+   synchronization is selected.
No need to copy-n-paste from video-interfaces.txt. Just "see video-interfaces.txt" for the description is fine.

Quoted 3 lines

I would still explicitly list the properties that apply to this binding. I agree that there's no need to copy & paste the description of those properties though.

Yes, that's what I meant. List each property with "see video-interfaces.txt" for the description of each.

Rob

Jacopo Mondi

author

2018-05-16 16:32 UTC #6 lore raw

PATCH 2/6 dt-bindings: media: rcar-vin: Document data-active

In reply to Jacopo Mondi

Document 'data-active' property in R-Car VIN device tree bindings. The property is optional when running with explicit synchronization (eg. BT.601) but mandatory when embedded synchronization is in use (eg. BT.656) as specified by the hardware manual.

Signed-off-by Jacopo Mondi <[email protected]>

---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/media/rcar_vin.txt +5 -0

index c53ce4e..17eac8a 100644
@@ -63,6 +63,11 @@ 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: active state of data enable signal (CLOCKENB pin).
+          0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
+          data enable signal. When using embedded synchronization this
+          property is mandatory.
+
     - 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

Niklas Söderlund

2018-05-16 21:55 UTC #7 lore raw

PATCH 2/6 dt-bindings: media: rcar-vin: Document data-active

Hi Jacopo,

Thanks for your work.

Quoted 15 lines · Jacopo Mondi

Document 'data-active' property in R-Car VIN device tree bindings. The property is optional when running with explicit synchronization (eg. BT.601) but mandatory when embedded synchronization is in use (eg. BT.656) as specified by the hardware manual.
Signed-off-by Jacopo Mondi <[email protected]>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0
index c53ce4e..17eac8a 100644

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 66

Quoted 4 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +1 -0

@@ -63,6 +63,11 @@ 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: active state of data enable signal (CLOCKENB pin).

I'm not sure what you mean by active state here. video-interfaces.txt defines data-active as 'similar to HSYNC and VSYNC, specifies data line polarity' so I assume this is the polarity of the CLOCKENB pin?

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 69

Quoted 3 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +3 -0

+          0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
+          data enable signal. When using embedded synchronization this
+          property is mandatory.

I'm confused, why is this mandatory if we have no embedded sync (that is hsync-active and vsync-active not defined)? I can't find any reference to this in the Gen2 datasheet but I'm sure I'm just missing it :-)

Quoted 7 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +1 -1

+
     - 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

Signature · 2 lines

Regards,
Niklas S?derlund

jacopo mondi

2018-05-17 08:25 UTC #8 lore raw

PATCH 2/6 dt-bindings: media: rcar-vin: Document data-active

Hi Niklas,

Quoted 23 lines · Niklas S?derlund

Hi Jacopo,
Thanks for your work.
Quoted 18 lines · Jacopo Mondi
Document 'data-active' property in R-Car VIN device tree bindings. The property is optional when running with explicit synchronization (eg. BT.601) but mandatory when embedded synchronization is in use (eg. BT.656) as specified by the hardware manual.
Signed-off-by Jacopo Mondi <[email protected]>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0
index c53ce4e..17eac8a 100644
@@ -63,6 +63,11 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
 	If both HSYNC and VSYNC polarities are not specified, embedded
 	synchronization is selected.
 

Quoted 5 lines

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 66
Quoted 1 line
Documentation/devicetree/bindings/media/rcar_vin.txt +1 -0
+        - data-active: active state of data enable signal (CLOCKENB pin).
I'm not sure what you mean by active state here. video-interfaces.txt defines data-active as 'similar to HSYNC and VSYNC, specifies data line polarity' so I assume this is the polarity of the CLOCKENB pin?

Yes, I can change this if it feels confusing to you.

Quoted 9 lines

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 69
Quoted 4 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +3 -0
+          0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
+          data enable signal. When using embedded synchronization this
+          property is mandatory.
I'm confused, why is this mandatory if we have no embedded sync (that is hsync-active and vsync-active not defined)? I can't find any reference to this in the Gen2 datasheet but I'm sure I'm just missing it :-)

Not exactly, it becomes mandatory IF we have embedded sync. Here it is my reasoning:

In the documentation of CHS bit of Vn_DMR2 register [1] the following is specified:

"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."

And setting the CHS bit to 1:

"HSYNC signal (VIn_HSYNC#) input from the pin is internally used as the clock enable signal"

So, if 'data-active' property is not specified I assume CLCKENB is not used, and set the CHS bit. What if we are using BT656 and there is no HSYNC? Then specifying 'data-active' becomes mandatory, as otherwise we set the CHS bit and wait for HSYNC pin transitions that won't happen.

This is probably wrong, as in the Koelsch case, there is no guarantee that CLKENB is connected, and what I should have done is probably set the CHS bit only when running on V4L2_MBUS_PARALLEL, and leave CHS (and CES, if 'data-active' is not specified) untouched, as we're doing today when running on V4L2_MBUS_BT656. Does this work better in your opinion?

This also makes patch [6/6] (where I was adding 'data-active' to Gen-2 boards) not required.

Thanks
j

[1] 26.2.18 Video n Data Mode Register 2 (VnDMR2) Datasheet version, R19UH0105EJ0100 Rev.1.00 Apr 30, 2018

Quoted 11 lines

Quoted 7 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +1 -1

+
     - 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180517/6971752f/attachment.sig>

Rob Herring

maintainer

2018-05-23 16:37 UTC #9 lore raw

PATCH 2/6 dt-bindings: media: rcar-vin: Document data-active

In reply to Jacopo Mondi

Quoted 15 lines · Jacopo Mondi

Document 'data-active' property in R-Car VIN device tree bindings. The property is optional when running with explicit synchronization (eg. BT.601) but mandatory when embedded synchronization is in use (eg. BT.656) as specified by the hardware manual.
Signed-off-by Jacopo Mondi <[email protected]>
---
 Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -0
index c53ce4e..17eac8a 100644

Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 69

Quoted 7 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +4 -0

@@ -63,6 +63,11 @@ 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: active state of data enable signal (CLOCKENB pin).
+          0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
+          data enable signal. When using embedded synchronization this
+          property is mandatory.

This doesn't match the common property's definition which AIUI is for the data lines' polarity. You need a new property. Perhaps a common one.

Quoted 7 lines

Documentation/devicetree/bindings/media/rcar_vin.txt +1 -1

+
     - 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

Jacopo Mondi

author

2018-05-16 16:32 UTC #10 lore raw

PATCH 3/6 media: rcar-vin: Handle data-active property

In reply to Jacopo Mondi

The data-active property has to be specified when running with embedded synchronization. The VIN peripheral can use HSYNC in place of CLOCKENB when the CLOCKENB pin is not connected, this requires explicit synchronization to be in use.

Now that the driver supports 'data-active' property, it makes not sense to zero the mbus configuration flags when running with implicit synch (V4L2_MBUS_BT656).

Signed-off-by Jacopo Mondi <[email protected]>

---
 drivers/media/platform/rcar-vin/rcar-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/media/platform/rcar-vin/rcar-core.c +8 -2

index d3072e1..075d08f 100644
@@ -531,15 +531,21 @@ static int rvin_digital_parse_v4l2(struct device *dev,
 		return -ENOTCONN;
 
 	vin->mbus_cfg.type = vep->bus_type;
+	vin->mbus_cfg.flags = vep->bus.parallel.flags;
 
 	switch (vin->mbus_cfg.type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");
-		vin->mbus_cfg.flags = vep->bus.parallel.flags;
 		break;
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found BT656 media bus\n");
-		vin->mbus_cfg.flags = 0;
+
+		if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH) &&
+		    !(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)) {
+			vin_err(vin,
+				"Missing data enable signal polarity property\n");
+			return -EINVAL;
+		}
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");

Signature · 1 line

2.7.4

Niklas Söderlund

2018-05-16 21:58 UTC #11 lore raw

Re: [PATCH 3/6] media: rcar-vin: Handle data-active property

Hi Jacopo,

Thanks for your work.

Quoted 4 lines · Jacopo Mondi

The data-active property has to be specified when running with embedded synchronization. The VIN peripheral can use HSYNC in place of CLOCKENB when the CLOCKENB pin is not connected, this requires explicit synchronization to be in use.

Is this really the intent of the data-active property? I read the video-interfaces.txt document as such as if no hsync-active, vsync-active and data-active we should use the embedded synchronization else set the polarity for the requested pins. What am I not understanding here?

Quoted 23 lines

Now that the driver supports 'data-active' property, it makes not sense to zero the mbus configuration flags when running with implicit synch (V4L2_MBUS_BT656).
Signed-off-by Jacopo Mondi <[email protected]>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/media/platform/rcar-vin/rcar-core.c +1 -0
index d3072e1..075d08f 100644
@@ -531,15 +531,21 @@ static int rvin_digital_parse_v4l2(struct device *dev,
 		return -ENOTCONN;
 
 	vin->mbus_cfg.type = vep->bus_type;
+	vin->mbus_cfg.flags = vep->bus.parallel.flags;
 
 	switch (vin->mbus_cfg.type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");

drivers/media/platform/rcar-vin/rcar-core.c static int rvin_digital_parse_v4l2(struct device *dev, · line 546

Quoted 10 lines

drivers/media/platform/rcar-vin/rcar-core.c +5 -2

-		vin->mbus_cfg.flags = vep->bus.parallel.flags;
 		break;
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found BT656 media bus\n");
-		vin->mbus_cfg.flags = 0;
+
+		if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH) &&
+		    !(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)) {
+			vin_err(vin,
+				"Missing data enable signal polarity property\n");

I fear this can't be an error as that would break backward comp ability with existing dtb's.

Quoted 8 lines

drivers/media/platform/rcar-vin/rcar-core.c +2 -0
+			return -EINVAL;
+		}
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");
Signature · 1 line
2.7.4

Signature · 2 lines

Regards,
Niklas Sï¿½derlund

jacopo mondi

2018-05-17 08:30 UTC #12 lore raw

Re: [PATCH 3/6] media: rcar-vin: Handle data-active property

Hi Niklas,

Quoted 10 lines · Niklas Söderlund

Hi Jacopo,
Thanks for your work.
Quoted 4 lines · Jacopo Mondi
The data-active property has to be specified when running with embedded synchronization. The VIN peripheral can use HSYNC in place of CLOCKENB when the CLOCKENB pin is not connected, this requires explicit synchronization to be in use.

Quoted 5 lines

Is this really the intent of the data-active property? I read the video-interfaces.txt document as such as if no hsync-active, vsync-active and data-active we should use the embedded synchronization else set the polarity for the requested pins. What am I not understanding here?

Almost correct.

The presence of hsync-active, vsync-active and field-evev-active properties determinate the bus type we're running on. If none of the is specified, the bus is marked 'BT656' and we assume the system is using embedded synchronization.

data-active does not take part in the bus identification, and my reasoning was the other way around as explained in reply to your comment on [2/6], and as explained there my reasoning is probably wrong, and we should set CHS -only- when running with explicit synchronization, instead of making it mandatory when running with embedded syncs.

Thanks and sorry for my confusion.

Quoted 50 lines

Quoted 24 lines
Now that the driver supports 'data-active' property, it makes not sense to zero the mbus configuration flags when running with implicit synch (V4L2_MBUS_BT656).
Signed-off-by Jacopo Mondi <[email protected]>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/media/platform/rcar-vin/rcar-core.c +1 -0
index d3072e1..075d08f 100644
@@ -531,15 +531,21 @@ static int rvin_digital_parse_v4l2(struct device *dev,
 		return -ENOTCONN;
 
 	vin->mbus_cfg.type = vep->bus_type;
+	vin->mbus_cfg.flags = vep->bus.parallel.flags;
 
 	switch (vin->mbus_cfg.type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");
drivers/media/platform/rcar-vin/rcar-core.c static int rvin_digital_parse_v4l2(struct device *dev, · line 546
Quoted 10 lines
drivers/media/platform/rcar-vin/rcar-core.c +5 -2
-		vin->mbus_cfg.flags = vep->bus.parallel.flags;
 		break;
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found BT656 media bus\n");
-		vin->mbus_cfg.flags = 0;
+
+		if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH) &&
+		    !(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)) {
+			vin_err(vin,
+				"Missing data enable signal polarity property\n");
I fear this can't be an error as that would break backward comp ability with existing dtb's.
Quoted 8 lines
drivers/media/platform/rcar-vin/rcar-core.c +2 -1
+			return -EINVAL;
+		}
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");
--
2.7.4
--
Regards,
Niklas Söderlund

Sergei Shtylyov

2018-05-17 08:48 UTC #13 lore raw

Re: [PATCH 3/6] media: rcar-vin: Handle data-active property

In reply to Jacopo Mondi

Hello!

Quoted 1 line · Jacopo Mondi

The data-active property has to be specified when running with embedded

Prop names are typically enclosed in "".

Quoted 1 line

synchronization. The VIN peripheral can use HSYNC in place of CLOCKENB

CLKENB, maybe?

Quoted 4 lines

when the CLOCKENB pin is not connected, this requires explicit synchronization to be in use.
Now that the driver supports 'data-active' property, it makes not sense

"data-active", s/not/no/.

Quoted 1 line

to zero the mbus configuration flags when running with implicit synch

Sync is better. :-)

Quoted 3 lines

(V4L2_MBUS_BT656).
Signed-off-by Jacopo Mondi <[email protected]>

[...]

MBR, Sergei

Jacopo Mondi

author

2018-05-16 16:32 UTC #14 lore raw

PATCH 4/6 media: rcar-vin: Handle CLOCKENB pin polarity

In reply to Jacopo Mondi

Handle CLOCKENB pin polarity, or use HSYNC in its place if polarity is not specified.

Signed-off-by Jacopo Mondi <[email protected]>

---
 drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/media/platform/rcar-vin/rcar-dma.c +11 -0

index ac07f99..7a84eae 100644
@@ -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)

@@ -691,6 +693,15 @@ static int rvin_setup(struct rvin_dev *vin)
 		dmr2 |= VNDMR2_VPS;
 
 	/*
+	 * Clock-enable active level select.
+	 * Use HSYNC as enable if not specified
+	 */
+	if (vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)
+		dmr2 |= VNDMR2_CES;
+	else if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH))
+		dmr2 |= VNDMR2_CHS;
+
+	/*
 	 * Output format
 	 */
 	switch (vin->format.pixelformat) {

Signature · 1 line

2.7.4

Niklas Söderlund

2018-05-16 22:11 UTC #15 lore raw

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

Hi Jacopo,

Thanks for your patch.

I'm happy that you dig into this as it clearly needs doing!

Quoted 22 lines · Jacopo Mondi

Handle CLOCKENB pin polarity, or use HSYNC in its place if polarity is not specified.

Signed-off-by Jacopo Mondi <[email protected]>

---
 drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/media/platform/rcar-vin/rcar-dma.c +2 -0

index ac07f99..7a84eae 100644
@@ -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)

drivers/media/platform/rcar-vin/rcar-dma.c static int rvin_setup(struct rvin_dev *vin) · line 702

Quoted 10 lines

drivers/media/platform/rcar-vin/rcar-dma.c +7 -0

@@ -691,6 +693,15 @@ static int rvin_setup(struct rvin_dev *vin)
 		dmr2 |= VNDMR2_VPS;
 
 	/*
+	 * Clock-enable active level select.
+	 * Use HSYNC as enable if not specified
+	 */
+	if (vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)
+		dmr2 |= VNDMR2_CES;
+	else if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH))
+		dmr2 |= VNDMR2_CHS;

After studying the datasheet for a while I'm getting more and more convinced this should be (with context leftout in this patch context) something like this:

	/* Hsync Signal Polarity Select */
	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
	        dmr2 |= VNDMR2_HPS;

	/* Vsync Signal Polarity Select */
	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
	        dmr2 |= VNDMR2_VPS;

	/* Clock Enable Signal Polarity Select */
	if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW))
	        dmr2 |= VNDMR2_CES;

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

Or am I misunderstanding something?

Quoted 8 lines

drivers/media/platform/rcar-vin/rcar-dma.c +2 -0
+
+	/*
 	 * Output format
 	 */
 	switch (vin->format.pixelformat) {
Signature · 1 line
2.7.4

Signature · 2 lines

Regards,
Niklas Söderlund

jacopo mondi

2018-05-17 08:41 UTC #16 lore raw

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

Hi Niklas,

Quoted 44 lines · Niklas Söderlund

Hi Jacopo,
Thanks for your patch.
I'm happy that you dig into this as it clearly needs doing!
Quoted 22 lines · Jacopo Mondi
Handle CLOCKENB pin polarity, or use HSYNC in its place if polarity is not specified.
Signed-off-by Jacopo Mondi <[email protected]>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/media/platform/rcar-vin/rcar-dma.c +2 -0
index ac07f99..7a84eae 100644
@@ -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)
 
drivers/media/platform/rcar-vin/rcar-dma.c static int rvin_setup(struct rvin_dev *vin) · line 702
Quoted 10 lines
drivers/media/platform/rcar-vin/rcar-dma.c +7 -0
@@ -691,6 +693,15 @@ static int rvin_setup(struct rvin_dev *vin)
 		dmr2 |= VNDMR2_VPS;
 
 	/*
+	 * Clock-enable active level select.
+	 * Use HSYNC as enable if not specified
+	 */
+	if (vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)
+		dmr2 |= VNDMR2_CES;
+	else if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH))
+		dmr2 |= VNDMR2_CHS;
After studying the datasheet for a while I'm getting more and more convinced this should be (with context leftout in this patch context) something like this:

Quoted 11 lines

	/* Hsync Signal Polarity Select */
	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
	        dmr2 |= VNDMR2_HPS;

	/* Vsync Signal Polarity Select */
	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
	        dmr2 |= VNDMR2_VPS;

	/* Clock Enable Signal Polarity Select */
	if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW))
	        dmr2 |= VNDMR2_CES;

No, set CES if V4L2_MBUS_DATA_ACTIVE_LOW is specified, not the other way around. See the CES bit description:

Clock Enable Signal Polarity Select
This bit specifies the polarity of the input clock enable signal in the ITU- R BT.601.
0: Active high
1: Active low

Quoted 6 lines

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

Or am I misunderstanding something?

Isn't that what my code is doing?

if (flags & LOW)

        dmr2 |= CES;
else if (!(flags & HIGH)) // if we get here, LOW is not set too
        dmr2 |= CHS;

Anyway, if you agree with my previous replies, we should set CHS only when running with explicit syncs (V4L2_MBUS_PARALLEL).

Thanks
j

Quoted 13 lines

Quoted 9 lines
drivers/media/platform/rcar-vin/rcar-dma.c +2 -1
+
+	/*
 	 * Output format
 	 */
 	switch (vin->format.pixelformat) {
--
2.7.4
--
Regards,
Niklas Söderlund

Jacopo Mondi

author

2018-05-16 16:32 UTC #17 lore raw

PATCH 5/6 ARM: dts: rcar-gen2: Remove unused VIN properties

In reply to 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 used them.

Signed-off-by Jacopo Mondi <[email protected]>

---
 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(-)

arch/arm/boot/dts/r8a7790-lager.dts +0 -3

index 063fdb6..b56b309 100644
@@ -873,10 +873,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -895,7 +893,6 @@
 
 		vin1ep0: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-koelsch.dts +0 -3

index f40321a..9967666 100644
@@ -849,10 +849,8 @@
 
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -870,7 +868,6 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-porter.dts +0 -1

index c14e6fe..055a7f1 100644
@@ -391,7 +391,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7793-gose.dts +0 -3

index 9ed6961..9d3fba2 100644
@@ -759,10 +759,8 @@
 
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -781,7 +779,6 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180_out>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-alt.dts +0 -1

index 26a8834..4bbb9cc 100644
@@ -380,7 +380,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-silk.dts +0 -1

index 351cb3b..c0c5d31 100644
@@ -480,7 +480,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

--
2.7.4

Niklas Söderlund

2018-05-16 22:13 UTC #18 lore raw

PATCH 5/6 ARM: dts: rcar-gen2: Remove unused VIN properties

Hi Jacopo,

Thanks for your work.

Quoted 19 lines · 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 used them.
Signed-off-by Jacopo Mondi <[email protected]>
---
 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(-)
arch/arm/boot/dts/r8a7790-lager.dts +0 -0
index 063fdb6..b56b309 100644

arch/arm/boot/dts/r8a7790-lager.dts line 876

Quoted 4 lines

arch/arm/boot/dts/r8a7790-lager.dts +0 -1

@@ -873,10 +873,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;

I can't really make up my mind if this is a good thing or not. Device tree describes the hardware and not what the drivers make use of. And the fact is that this bus is 24 bits wide. So I'm not sure we should remove these properties. But I would love to hear what others think about this.

Quoted 99 lines

arch/arm/boot/dts/r8a7790-lager.dts +0 -2

 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -895,7 +893,6 @@
 
 		vin1ep0: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-koelsch.dts +0 -3

index f40321a..9967666 100644
@@ -849,10 +849,8 @@
 
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -870,7 +868,6 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-porter.dts +0 -1

index c14e6fe..055a7f1 100644
@@ -391,7 +391,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7793-gose.dts +0 -3

index 9ed6961..9d3fba2 100644
@@ -759,10 +759,8 @@
 
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -781,7 +779,6 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180_out>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-alt.dts +0 -1

index 26a8834..4bbb9cc 100644
@@ -380,7 +380,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-silk.dts +0 -2

index 351cb3b..c0c5d31 100644
@@ -480,7 +480,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
--

2.7.4

Signature · 2 lines

Regards,
Niklas S?derlund

jacopo mondi

2018-05-17 09:01 UTC #19 lore raw

PATCH 5/6 ARM: dts: rcar-gen2: Remove unused VIN properties

Hi Niklas,

Quoted 19 lines · Niklas S?derlund

Hi Jacopo,
Thanks for your work.
Quoted 14 lines · 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 used them.
Signed-off-by Jacopo Mondi <[email protected]>
---
 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(-)

Quoted 16 lines

arch/arm/boot/dts/r8a7790-lager.dts line 876
Quoted 9 lines
arch/arm/boot/dts/r8a7790-lager.dts +0 -1
index 063fdb6..b56b309 100644
@@ -873,10 +873,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
I can't really make up my mind if this is a good thing or not. Device tree describes the hardware and not what the drivers make use of. And the fact is that this bus is 24 bits wide. So I'm not sure we should remove these properties. But I would love to hear what others think about this.

Just to point out those properties are not even documented in rcar-vin bindings (actually, none of them was).

I feel it's wrong to have them here, as someone may think that changing their value should actually change the VIN interface behavior, which it's not true, leading to massive confusion and quite some code digging for no reason (and they will get mad at us at some point, probably :)

Thanks
j

Quoted 90 lines

arch/arm/boot/dts/r8a7790-lager.dts +0 -2

 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -895,7 +893,6 @@
 
 		vin1ep0: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-koelsch.dts +0 -3

index f40321a..9967666 100644
@@ -849,10 +849,8 @@
 
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -870,7 +868,6 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-porter.dts +0 -1

index c14e6fe..055a7f1 100644
@@ -391,7 +391,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7793-gose.dts +0 -3

index 9ed6961..9d3fba2 100644
@@ -759,10 +759,8 @@
 
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
 			hsync-active = <0>;
 			vsync-active = <0>;
-			pclk-sample = <1>;
 			data-active = <1>;
 		};
 	};

@@ -781,7 +779,6 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180_out>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-alt.dts +0 -1

index 26a8834..4bbb9cc 100644
@@ -380,7 +380,6 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-silk.dts +0 -0

index 351cb3b..c0c5d31 100644
@@ -480,7 +480,6 @@

Quoted 13 lines

Quoted 9 lines
arch/arm/boot/dts/r8a7794-silk.dts +0 -2
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
 		};
 	};
 };
--
2.7.4
--
Regards,
Niklas S?derlund

Simon Horman

2018-05-22 08:18 UTC #20 lore raw

PATCH 5/6 ARM: dts: rcar-gen2: Remove unused VIN properties

Quoted 39 lines · jacopo mondi

Hi Niklas,
Quoted 19 lines · Niklas S?derlund
Hi Jacopo,
Thanks for your work.
Quoted 14 lines · 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 used them.
Signed-off-by Jacopo Mondi <[email protected]>
---
 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(-)
Quoted 16 lines
arch/arm/boot/dts/r8a7790-lager.dts line 876
Quoted 9 lines
arch/arm/boot/dts/r8a7790-lager.dts +0 -1
index 063fdb6..b56b309 100644
@@ -873,10 +873,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
I can't really make up my mind if this is a good thing or not. Device tree describes the hardware and not what the drivers make use of. And the fact is that this bus is 24 bits wide. So I'm not sure we should remove these properties. But I would love to hear what others think about this.

Quoted 7 lines

Just to point out those properties are not even documented in rcar-vin bindings (actually, none of them was).
I feel it's wrong to have them here, as someone may think that changing their value should actually change the VIN interface behavior, which it's not true, leading to massive confusion and quite some code digging for no reason (and they will get mad at us at some point, probably :)

I think its fine that the driver doesn't implement something described in DT - we are describing the hardware not the implementation. But I think its not fine that DT includes properties not described in the binding.

So I think we should either
a) Fix the binding documentation, but perhaps it is already correct in which case we should;
b) Apply this patch

Once we have decided what is the correct description of the hardware we can consider implications on the driver implementation.

Rob Herring

maintainer

2018-05-23 16:33 UTC #21 lore raw

PATCH 5/6 ARM: dts: rcar-gen2: Remove unused VIN properties

In reply to Niklas Söderlund

Quoted 19 lines · Niklas S?derlund

Hi Jacopo,
Thanks for your work.
Quoted 14 lines · 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 used them.
Signed-off-by Jacopo Mondi <[email protected]>
---
 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(-)

Quoted 15 lines

arch/arm/boot/dts/r8a7790-lager.dts line 876
Quoted 9 lines
arch/arm/boot/dts/r8a7790-lager.dts +0 -1
index 063fdb6..b56b309 100644
@@ -873,10 +873,8 @@
 	port {
 		vin0ep2: endpoint {
 			remote-endpoint = <&adv7612_out>;
-			bus-width = <24>;
I can't really make up my mind if this is a good thing or not. Device tree describes the hardware and not what the drivers make use of. And the fact is that this bus is 24 bits wide. So I'm not sure we should remove these properties. But I would love to hear what others think about this.

IMO, this property should only be present when all the pins are not connected. And by "all", I mean the minimum of what each end of the graph can support.

Rob

Jacopo Mondi

author

2018-05-16 16:32 UTC #22 lore raw

PATCH 6/6 ARM: dts: rcar-gen2: Add 'data-active' property

In reply to Jacopo Mondi

The 'data-active' property needs to be specified when using embedded synchronization. Add it to the Gen-2 boards using composite video input.

Signed-off-by Jacopo Mondi <[email protected]>

---
 arch/arm/boot/dts/r8a7790-lager.dts   | 1 +
 arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
 arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
 arch/arm/boot/dts/r8a7793-gose.dts    | 1 +
 arch/arm/boot/dts/r8a7794-alt.dts     | 1 +
 arch/arm/boot/dts/r8a7794-silk.dts    | 1 +
 6 files changed, 6 insertions(+)

arch/arm/boot/dts/r8a7790-lager.dts +1 -0

index b56b309..48fcb44 100644
@@ -893,6 +893,7 @@
 
 		vin1ep0: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-koelsch.dts +1 -0

index 9967666..fa0b25f 100644
@@ -868,6 +868,7 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7791-porter.dts +1 -0

index 055a7f1..96b9605 100644
@@ -391,6 +391,7 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7793-gose.dts +1 -0

index 9d3fba2..80b4fa8 100644
@@ -779,6 +779,7 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180_out>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-alt.dts +1 -0

index 4bbb9cc..00df605d 100644
@@ -380,6 +380,7 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-silk.dts +1 -0

index c0c5d31..ed17376 100644
@@ -480,6 +480,7 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

--
2.7.4

Niklas Söderlund

2018-05-16 22:15 UTC #23 lore raw

PATCH 6/6 ARM: dts: rcar-gen2: Add 'data-active' property

Hi Jacopo,

Thanks for your work.

Quoted 30 lines · Jacopo Mondi

The 'data-active' property needs to be specified when using embedded synchronization. Add it to the Gen-2 boards using composite video input.
Signed-off-by Jacopo Mondi <[email protected]>
---
 arch/arm/boot/dts/r8a7790-lager.dts   | 1 +
 arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
 arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
 arch/arm/boot/dts/r8a7793-gose.dts    | 1 +
 arch/arm/boot/dts/r8a7794-alt.dts     | 1 +
 arch/arm/boot/dts/r8a7794-silk.dts    | 1 +
 6 files changed, 6 insertions(+)
arch/arm/boot/dts/r8a7790-lager.dts +1 -0
index b56b309..48fcb44 100644
@@ -893,6 +893,7 @@
 
 		vin1ep0: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };
arch/arm/boot/dts/r8a7791-koelsch.dts +0 -0
index 9967666..fa0b25f 100644

arch/arm/boot/dts/r8a7791-koelsch.dts line 871

Quoted 4 lines

arch/arm/boot/dts/r8a7791-koelsch.dts +1 -0

@@ -868,6 +868,7 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;

Depending on how we interpret the data-active property this can be good or bad. But if we interpret it as the polarity of the VIn_CLKENB pin this is not good as this is not connected for the adv7180 on Koelsch.

I have not checked all the Gen2 schematics as I'm still not sure how to interpret the property.

Quoted 54 lines

arch/arm/boot/dts/r8a7791-koelsch.dts +0 -0

 		};
 	};
 };

arch/arm/boot/dts/r8a7791-porter.dts +1 -0

index 055a7f1..96b9605 100644
@@ -391,6 +391,7 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7793-gose.dts +1 -0

index 9d3fba2..80b4fa8 100644
@@ -779,6 +779,7 @@
 
 		vin1ep: endpoint {
 			remote-endpoint = <&adv7180_out>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-alt.dts +1 -0

index 4bbb9cc..00df605d 100644
@@ -380,6 +380,7 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };

arch/arm/boot/dts/r8a7794-silk.dts +1 -1

index c0c5d31..ed17376 100644
@@ -480,6 +480,7 @@
 
 		vin0ep: endpoint {
 			remote-endpoint = <&adv7180>;
+			data-active = <1>;
 		};
 	};
 };
--

2.7.4

Signature · 2 lines

Regards,
Niklas S?derlund

Niklas Söderlund

2018-05-16 21:16 UTC #24 lore raw

In reply to Jacopo Mondi

Hi Jacopo,

Quoted 13 lines · 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, then the second one extends them with 'data-active'.
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. As a consequence, when running with embedded synchronism, and there is not HSYNC signal, it becomes mandatory to specify 'data-active' polarity in DTS.

Quoted 8 lines

To address this, all Gen-2 boards featuring a composite video input and running with embedded synchronization, now need that property to be specified in DTS. Before adding it, remove un-used properties as 'pclk-sample' and 'bus-width' from the Gen-2 bindings, as they are not parsed by the VIN driver and only confuse users.
Niklas, as you already know I don't have any Gen2 board. Could you give this a spin on Koelsch if you like the series?

I tested this on my Koelsch and capture is still working :-)

Quoted 26 lines

Thanks
j
Jacopo Mondi (6):
dt-bindings: media: rcar-vin: Describe optional ep properties
dt-bindings: media: rcar-vin: Document data-active
media: rcar-vin: Handle data-active property
media: rcar-vin: Handle CLOCKENB pin polarity
ARM: dts: rcar-gen2: Remove unused VIN properties
ARM: dts: rcar-gen2: Add 'data-active' property
 Documentation/devicetree/bindings/media/rcar_vin.txt | 18 +++++++++++++++++-
 arch/arm/boot/dts/r8a7790-lager.dts                  |  4 +---
 arch/arm/boot/dts/r8a7791-koelsch.dts                |  4 +---
 arch/arm/boot/dts/r8a7791-porter.dts                 |  2 +-
 arch/arm/boot/dts/r8a7793-gose.dts                   |  4 +---
 arch/arm/boot/dts/r8a7794-alt.dts                    |  2 +-
 arch/arm/boot/dts/r8a7794-silk.dts                   |  2 +-
 drivers/media/platform/rcar-vin/rcar-core.c          | 10 ++++++++--
 drivers/media/platform/rcar-vin/rcar-dma.c           | 11 +++++++++++
 9 files changed, 42 insertions(+), 15 deletions(-)
--
2.7.4

Signature · 2 lines

Regards,
Niklas Söderlund