media: rcar-vin: Brush endpoint properties
Subsystems (4)
v3 cover media: rcar-vin: Brush endpoint properties
Hello,
3rd version of VIN endpoint brushing series.
Slightly enlarged the linux-media receiver list, as this new version introduces a common property in 'video-interfaces.txt'.
Quite some changes compared to v1/v2:
- First patch in the series changed to include Niklas' comments on using spaces for indent over tabs in documentation and as suggested by Rob and Laurent to refer to 'video-interfaces.txt' in property description.
- As suggested by Rob this series introduces a new property 'data-enable-active' to describe data enable signal polarity instead of using 'data-active' which refers to the data lanes polarity.
- Use this new property to control CLKENB pin polarity in VIN driver
- Introduce a new custom property to describe the 'use HSYNC as data-enable' function provided by VIN. In previous iterations I used the presence of 'data-active' to enable/disable this functionality. That was confusing and not correct as it used the wrong property. As this is a VIN specificity, I thought a custom property is more suited.
- Last patch is still there and I know it is debated. My opinion is that it is still needed, as the presence of those un-documented and un-parsed properties confuses users which may expect changing those properties value to reflect on the video interface configuration, which does not happens instead.
Individual changelog per patch when relevant.
Thanks
j
Jacopo Mondi (8):
dt-bindings: media: rcar-vin: Describe optional ep properties
dt-bindings: media: Document data-enable-active property
media: v4l2-fwnode: parse 'data-enable-active' prop
dt-bindings: media: rcar-vin: Add 'data-enable-active'
media: rcar-vin: Handle data-enable polarity
dt-bindings: rcar-vin: Add 'hsync-as-de' custom prop
media: rcar-vin: Handle 'hsync-as-de' property
ARM: dts: rcar-gen2: Remove unused VIN properties
Documentation/devicetree/bindings/media/rcar_vin.txt | 16 +++++++++++++++- .../devicetree/bindings/media/video-interfaces.txt | 2 ++ 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-core.c | 6 ++++++ drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++++++++++ drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++ drivers/media/v4l2-core/v4l2-fwnode.c | 5 +++++ include/media/v4l2-mediabus.h | 2 ++ 13 files changed, 43 insertions(+), 13 deletions(-)
--
2.7.4
PATCH v3 1/8 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.
---
v2 -> v3:
- Do not repeat property description, just reference video-interfaces.txt
- Indent with spaces, not tabs as the rest of the document
- Do not remove (yet) the 'bus-width' property from example
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 10 ++++++++++ 1 file changed, 10 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +10 -0
@@ -54,6 +54,14 @@ 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: see [1] for description. Default is active high. + - vsync-active: see [1] for description. 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 +71,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 --------------------------------------
--
2.7.4
Rob Herring
maintainerPATCH v3 1/8 dt-bindings: media: rcar-vin: Describe optional ep properties
Quoted 6 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 8 lines
---
v2 -> v3:
- Do not repeat property description, just reference video-interfaces.txt
- Indent with spaces, not tabs as the rest of the document
- Do not remove (yet) the 'bus-width' property from example--- Documentation/devicetree/bindings/media/rcar_vin.txt | 10 ++++++++++ 1 file changed, 10 insertions(+)
PATCH v3 2/8 dt-bindings: media: Document data-enable-active property
In reply to Jacopo MondiAdd 'data-enable-active' property to endpoint node properties list.
The property allows to specify the polarity of the data-enable signal, which when in active state determinates when data lanes have to sampled for valid pixel data.
---
v3:
- new patch
--- Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0
@@ -109,6 +109,8 @@ Optional endpoint properties Note, that if HSYNC and VSYNC polarities are not specified, embedded synchronization may be required, where supported. - data-active: similar to HSYNC and VSYNC, specifies data line polarity. +- data-enable-active: similar to HSYNC and VSYNC, specifies the data enable + signal polarity. - field-even-active: field signal level during the even field data transmission. - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock signal.
--
2.7.4
Rob Herring
maintainerPATCH v3 2/8 dt-bindings: media: Document data-enable-active property
Quoted 6 lines · Jacopo Mondi
Add 'data-enable-active' property to endpoint node properties list.
The property allows to specify the polarity of the data-enable signal, which when in active state determinates when data lanes have to sampled for valid pixel data.
Quoted 7 lines
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch--- Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++ 1 file changed, 2 insertions(+)
Sakari Ailus
maintainerPATCH v3 2/8 dt-bindings: media: Document data-enable-active property
In reply to Jacopo MondiHi Jacopo,
Thanks for the patch.
Quoted 5 lines · Jacopo Mondi
Add 'data-enable-active' property to endpoint node properties list.
The property allows to specify the polarity of the data-enable signal, which when in active state determinates when data lanes have to sampled for valid pixel data.
Lanes or lines? I understand this is forthe parallel interface.
Quoted 22 lines
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch--- Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/media/video-interfaces.txt +2
-0@@ -109,6 +109,8 @@ Optional endpoint properties Note, that if HSYNC and VSYNC polarities are not specified, embedded synchronization may be required, where supported. - data-active: similar to HSYNC and VSYNC, specifies data line polarity. +- data-enable-active: similar to HSYNC and VSYNC, specifies the data enable + signal polarity. - field-even-active: field signal level during the even field data transmission. - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock signal.
Signature · 2 lines
Sakari Ailus sakari.ailus at linux.intel.com
PATCH v3 2/8 dt-bindings: media: Document data-enable-active property
Hi Sakari,
Quoted 4 lines · Sakari Ailus
Hi Jacopo,
Thanks for the patch.
Quoted 9 lines
Quoted 5 lines · Jacopo Mondi
Add 'data-enable-active' property to endpoint node properties list.
The property allows to specify the polarity of the data-enable signal, which when in active state determinates when data lanes have to sampled for valid pixel data.
Lanes or lines? I understand this is forthe parallel interface.
Now I'm confused. Are the parallel data 'lines' and the CSI-2 one 'lanes'? I thought 'lanes' applies to both.. am I wrong?
Thanks
j
Quoted 18 lines
Quoted 18 lines
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch--- Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/media/video-interfaces.txt +1
-0@@ -109,6 +109,8 @@ Optional endpoint properties Note, that if HSYNC and VSYNC polarities are not specified, embedded synchronization may be required, where supported. - data-active: similar to HSYNC and VSYNC, specifies data line polarity. +- data-enable-active: similar to HSYNC and VSYNC, specifies the data enable
Quoted 8 lines
Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 116
Quoted 4 lines
Documentation/devicetree/bindings/media/video-interfaces.txt +1
-0+ signal polarity. - field-even-active: field signal level during the even field data transmission. - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock signal.--
Sakari Ailus
sakari.ailus at linux.intel.com
-------------- 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/20180601/da16dd9b/attachment.sig>
Sakari Ailus
maintainerPATCH v3 2/8 dt-bindings: media: Document data-enable-active property
Quoted 14 lines · jacopo mondi
Hi Sakari,
Quoted 11 lines · Sakari Ailus
Hi Jacopo,
Thanks for the patch.
Quoted 5 lines · Jacopo Mondi
Add 'data-enable-active' property to endpoint node properties list.
The property allows to specify the polarity of the data-enable signal, which when in active state determinates when data lanes have to sampled for valid pixel data.
Quoted 5 lines
Quoted 2 lines
Lanes or lines? I understand this is forthe parallel interface.
Now I'm confused. Are the parallel data 'lines' and the CSI-2 one 'lanes'? I thought 'lanes' applies to both.. am I wrong?
"Lane" is conventionally refer to a differential pair of wires on a serial bus such as CSI-2. "Line" is used of a wire on a parallel bus.
Signature · 2 lines
Sakari Ailus sakari.ailus at linux.intel.com
PATCH v3 3/8 media: v4l2-fwnode: parse 'data-enable-active' prop
In reply to Jacopo MondiParse the newly defined 'data-enable-active' property in parallel endpoint parsing function.
---
v3:
- new patch
--- drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++++ include/media/v4l2-mediabus.h | 2 ++ 2 files changed, 6 insertions(+)
drivers/media/v4l2-core/v4l2-fwnode.c +4 -0
@@ -154,6 +154,10 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus( flags |= v ?V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH :V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW ; +if (!fwnode_property_read_u32 (fwnode,"data-enable-active" , &v)) + flags |= v ?V4L2_MBUS_DATA_ENABLE_HIGH : +V4L2_MBUS_DATA_ENABLE_LOW ; + bus->flags = flags; }
include/media/v4l2-mediabus.h +2 -0
@@ -45,6 +45,8 @@/* Active state of Sync-on-green (SoG) signal, 0/1 for LOW/HIGH respectively. */ #define V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH BIT(12)#define V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW BIT(13) +#define V4L2_MBUS_DATA_ENABLE_HIGH BIT(14) +#define V4L2_MBUS_DATA_ENABLE_LOW BIT(15)/* Serial flags */ /* How many lanes the client can use */
--
2.7.4
PATCH v3 3/8 media: v4l2-fwnode: parse 'data-enable-active' prop
Hi Jacopo,
Thanks for your patch.
Quoted 4 lines · Jacopo Mondi
Parse the newly defined 'data-enable-active' property in parallel endpoint parsing function.
Signed-off-by Jacopo Mondi <[email protected]>
Quoted 40 lines
---
v3:
- new patch--- drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++++ include/media/v4l2-mediabus.h | 2 ++ 2 files changed, 6 insertions(+)drivers/media/v4l2-core/v4l2-fwnode.c +4
-0@@ -154,6 +154,10 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus( flags |= v ?V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH :V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW ; +if (!fwnode_property_read_u32 (fwnode,"data-enable-active" , &v)) + flags |= v ?V4L2_MBUS_DATA_ENABLE_HIGH : +V4L2_MBUS_DATA_ENABLE_LOW ; + bus->flags = flags; }include/media/v4l2-mediabus.h +2
-1@@ -45,6 +45,8 @@/* Active state of Sync-on-green (SoG) signal, 0/1 for LOW/HIGH respectively. */ #define V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH BIT(12)#define V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW BIT(13) +#define V4L2_MBUS_DATA_ENABLE_HIGH BIT(14) +#define V4L2_MBUS_DATA_ENABLE_LOW BIT(15)/* Serial flags */ /* How many lanes the client can use */ --2.7.4
Signature · 2 lines
Regards, Niklas S?derlund
PATCH v3 4/8 dt-bindings: media: rcar-vin: Add 'data-enable-active'
In reply to Jacopo MondiDescribe optional endpoint property 'data-enable-active' for R-Car VIN.
---
v3:
- new patch
---
Documentation/devicetree/bindings/media/rcar_vin.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +2 -0
@@ -58,6 +58,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC. - Optional properties for endpoint nodes of port at 0: - hsync-active: see [1] for description. Default is active high. - vsync-active: see [1] for description. Default is active high. + - data-enable-active: polarity of CLKENB signal, see [1] for + description. Default is active high. If both HSYNC and VSYNC polarities are not specified, embedded synchronization is selected.
--
2.7.4
Rob Herring
maintainerPATCH v3 4/8 dt-bindings: media: rcar-vin: Add 'data-enable-active'
Quoted 10 lines · Jacopo Mondi
Describe optional endpoint property 'data-enable-active' for R-Car VIN.
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch
---Documentation/devicetree/bindings/media/rcar_vin.txt | 2 ++ 1 file changed, 2 insertions(+)
PATCH v3 4/8 dt-bindings: media: rcar-vin: Add 'data-enable-active'
In reply to Jacopo MondiHi Jacopo,
Thanks for your patch.
Quoted 3 lines · Jacopo Mondi
Describe optional endpoint property 'data-enable-active' for R-Car VIN.
Signed-off-by Jacopo Mondi <[email protected]>
Quoted 24 lines
---
v3:
- new patch
---Documentation/devicetree/bindings/media/rcar_vin.txt | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/media/rcar_vin.txt +2
-1@@ -58,6 +58,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC. - Optional properties for endpoint nodes of port at 0: - hsync-active: see [1] for description. Default is active high. - vsync-active: see [1] for description. Default is active high. + - data-enable-active: polarity of CLKENB signal, see [1] for + description. Default is active high. If both HSYNC and VSYNC polarities are not specified, embedded synchronization is selected. --2.7.4
Signature · 2 lines
Regards, Niklas S?derlund
PATCH v3 5/8 media: rcar-vin: Handle data-enable polarity
In reply to Jacopo MondiHandle data-enable signal polarity. If the polarity is not specifically requested to be active low, use the active high default.
---
v3:
- use new property to set the CES bit
--- drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/media/platform/rcar-vin/rcar-dma.c +5 -0
@@ -123,6 +123,7 @@/* Video n Data Mode Register 2 bits */ #define VNDMR2_VPS (1 << 30)#define VNDMR2_HPS (1 << 29) +#define VNDMR2_CES (1 << 28)#define VNDMR2_FTEV (1 << 17)#define VNDMR2_VLV (n) ((n & 0xf) << 12)
@@ -698,6 +699,10 @@ static int rvin_setup(struct rvin_dev *vin)/* Vsync Signal Polarity Select */ if (!(vin->parallel->mbus_flags &V4L2_MBUS_VSYNC_ACTIVE_LOW )) dmr2 |=VNDMR2_VPS ; + +/* Data Enable Polarity Select */ +if (vin->parallel->mbus_flags &V4L2_MBUS_DATA_ENABLE_LOW ) + dmr2 |=VNDMR2_CES ; }/*
--
2.7.4
PATCH v3 5/8 media: rcar-vin: Handle data-enable polarity
Hi Jacopo,
Thanks for your work.
Quoted 4 lines · Jacopo Mondi
Handle data-enable signal polarity. If the polarity is not specifically requested to be active low, use the active high default.
Signed-off-by Jacopo Mondi <[email protected]>
Quoted 33 lines
---
v3:
- use new property to set the CES bit--- drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/media/platform/rcar-vin/rcar-dma.c +5
-1@@ -123,6 +123,7 @@/* Video n Data Mode Register 2 bits */ #define VNDMR2_VPS (1 << 30)#define VNDMR2_HPS (1 << 29) +#define VNDMR2_CES (1 << 28)#define VNDMR2_FTEV (1 << 17)#define VNDMR2_VLV (n) ((n & 0xf) << 12)@@ -698,6 +699,10 @@ static int rvin_setup(struct rvin_dev *vin)/* Vsync Signal Polarity Select */ if (!(vin->parallel->mbus_flags &V4L2_MBUS_VSYNC_ACTIVE_LOW )) dmr2 |=VNDMR2_VPS ; + +/* Data Enable Polarity Select */ +if (vin->parallel->mbus_flags &V4L2_MBUS_DATA_ENABLE_LOW ) + dmr2 |=VNDMR2_CES ; }/* -- 2.7.4
Signature · 2 lines
Regards, Niklas S?derlund
PATCH v3 6/8 dt-bindings: rcar-vin: Add 'hsync-as-de' custom prop
In reply to Jacopo MondiDocument the boolean custom property 'renesas,hsync-as-de' that indicates that the HSYNC signal is internally used as data-enable, when the CLKENB signal is not connected.
As this is a VIN specificity create a custom property specific to the R-Car VIN driver.
---
v3:
- new patch
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 3 +++ 1 file changed, 3 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +3 -0
@@ -60,6 +60,9 @@ from local SoC CSI-2 receivers (port1) depending on SoC. - vsync-active: see [1] for description. Default is active high. - data-enable-active: polarity of CLKENB signal, see [1] for description. Default is active high. + - renesas,hsync-as-de: a boolean property to indicate that HSYNC signal + is internally used as data-enable when the CLKENB signal is + not available. If both HSYNC and VSYNC polarities are not specified, embedded synchronization is selected.
--
2.7.4
Rob Herring
maintainerPATCH v3 6/8 dt-bindings: rcar-vin: Add 'hsync-as-de' custom prop
Quoted 7 lines · Jacopo Mondi
Document the boolean custom property 'renesas,hsync-as-de' that indicates that the HSYNC signal is internally used as data-enable, when the CLKENB signal is not connected.
As this is a VIN specificity create a custom property specific to the R-Car VIN driver.
Quoted 7 lines
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch--- Documentation/devicetree/bindings/media/rcar_vin.txt | 3 +++ 1 file changed, 3 insertions(+)
PATCH v3 6/8 dt-bindings: rcar-vin: Add 'hsync-as-de' custom prop
In reply to Jacopo MondiHi Jacopo,
Thanks for your work.
Quoted 20 lines · Jacopo Mondi
Document the boolean custom property 'renesas,hsync-as-de' that indicates that the HSYNC signal is internally used as data-enable, when the CLKENB signal is not connected.
As this is a VIN specificity create a custom property specific to the R-Car VIN driver.
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch--- Documentation/devicetree/bindings/media/rcar_vin.txt | 3 +++ 1 file changed, 3 insertions(+)Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0
Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 65
Quoted 6 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +3
-0@@ -60,6 +60,9 @@ from local SoC CSI-2 receivers (port1) depending on SoC. - vsync-active: see [1] for description. Default is active high. - data-enable-active: polarity of CLKENB signal, see [1] for description. Default is active high. + - renesas,hsync-as-de: a boolean property to indicate that HSYNC signal + is internally used as data-enable when the CLKENB signal is + not available.
I'm not sure I like this, is there really a need to add a custom property for this? The datasheet states that when the CLKENB pin is not connected the driver should enable 'Clock Enable Hsync Select (CHS)'. With the new generic property 'data-enable-active' which describes the polarity of the CLKENB pin we also gain the knowledge if the CLKENB pin is connected or not.
I propose we drop this custom property and instead let the driver check if the CLKENB polarity is described or not and use that to determine if CHS bit should be set or not. IMHO that is much simpler then having two properties describing the same pin.
Quoted 6 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +0
-1If both HSYNC and VSYNC polarities are not specified, embedded synchronization is selected. --2.7.4
Signature · 2 lines
Regards, Niklas S?derlund
PATCH v3 6/8 dt-bindings: rcar-vin: Add 'hsync-as-de' custom prop
Hi Niklas,
Quoted 30 lines · Niklas S?derlund
Hi Jacopo,
Thanks for your work.
Quoted 25 lines · Jacopo Mondi
Document the boolean custom property 'renesas,hsync-as-de' that indicates that the HSYNC signal is internally used as data-enable, when the CLKENB signal is not connected.
As this is a VIN specificity create a custom property specific to the R-Car VIN driver.
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- new patch--- Documentation/devicetree/bindings/media/rcar_vin.txt | 3 +++ 1 file changed, 3 insertions(+)Documentation/devicetree/bindings/media/rcar_vin.txt +2
-0@@ -60,6 +60,9 @@ from local SoC CSI-2 receivers (port1) depending on SoC. - vsync-active: see [1] for description. Default is active high. - data-enable-active: polarity of CLKENB signal, see [1] for description. Default is active high. + - renesas,hsync-as-de: a boolean property to indicate that HSYNC signal + is internally used as data-enable when the CLKENB signal is
Quoted 8 lines
Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 65
Quoted 1 line
Documentation/devicetree/bindings/media/rcar_vin.txt +1
-0+ not available.I'm not sure I like this, is there really a need to add a custom property for this? The datasheet states that when the CLKENB pin is not connected the driver should enable 'Clock Enable Hsync Select (CHS)'. With the new generic property 'data-enable-active' which describes the polarity of the CLKENB pin we also gain the knowledge if the CLKENB pin is connected or not.
That was my first proposal, we discussed that in
Re: [PATCH 3/6] media: rcar-vin: Handle data-active property
Re: [PATCH 2/6] dt-bindings: media: rcar-vin: Document data-active
Let's sum it up in this way:
Instead of having to deal again with the "what happens if there is no data-enable-active and we're running on BT.656 where there is no HSYNC signal"[1] I decided to go with a custom property.
Quoted 6 lines
I propose we drop this custom property and instead let the driver check if the CLKENB polarity is described or not and use that to determine if CHS bit should be set or not. IMHO that is much simpler then having two properties describing the same pin.
It is my understanding that both Gen2 and Gen3 boards CVBS input are BT.656 and none of them has CLKENB input. So 'data-enable-active' is never there but in this case we should not set CHS. So the absence of 'data-enable-active' has different consequences if we're running on parallel or BT.656 bus, and that feels confusing to me, so I decided to make it an explicit property.
Also, as the interface manual describes the "use HSYNC in place of CLKENB" when on parallel bus as a design choice, that should imo be documented.
Again, this is very minor stuff. I'll leave this out from next version, maybe we can talk about this f2f next week.
Thanks
j
Quoted 10 lines
Quoted 6 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +0
-1If both HSYNC and VSYNC polarities are not specified, embedded synchronization is selected. --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/20180612/d1d7273b/attachment.sig>
PATCH v3 7/8 media: rcar-vin: Handle 'hsync-as-de' property
In reply to Jacopo MondiParse and handle 'hsync-as-de' custom property and set the CHS flag during the VIN interface setup.
---
v3:
- new patch
- use the new custom property to set the CHS bit
--- drivers/media/platform/rcar-vin/rcar-core.c | 6 ++++++ drivers/media/platform/rcar-vin/rcar-dma.c | 6 ++++++ drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++ 3 files changed, 14 insertions(+)
drivers/media/platform/rcar-vin/rcar-core.c +6 -0
@@ -589,6 +589,7 @@ static int rvin_parallel_parse_v4l2(struct device *dev,struct rvin_dev *vin =dev_get_drvdata (dev);struct rvin_parallel_entity *rvpe = container_of(asd, struct rvin_parallel_entity, asd); +const struct fwnode_handle *fwnode = vep->base.local_fwnode;if (vep->base.port || vep->base.id)return -ENOTCONN ;
@@ -610,6 +611,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,return -EINVAL ; } +if (fwnode_property_read_bool (fwnode,"renesas,hsync-as-de" )) + vin->parallel->chs = true; +else + vin->parallel->chs = false; +return 0 ; }
drivers/media/platform/rcar-vin/rcar-dma.c +6 -0
@@ -124,6 +124,7 @@#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)
@@ -703,6 +704,11 @@ static int rvin_setup(struct rvin_dev *vin)/* Data Enable Polarity Select */ if (vin->parallel->mbus_flags &V4L2_MBUS_DATA_ENABLE_LOW ) dmr2 |=VNDMR2_CES ; + +/* Use HSYNC as data-enable signal */ +if (vin->parallel->mbus_type ==V4L2_MBUS_PARALLEL && + vin->parallel->chs) + dmr2 |=VNDMR2_CHS ; }/*
drivers/media/platform/rcar-vin/rcar-vin.h +2 -0
@@ -78,6 +78,7 @@ struct rvin_video_format {* @subdev: subdevice matched using async framework * @mbus_type: media bus type * @mbus_flags: media bus configuration flags +* @chs: use HSYNC as data-enable flag * @source_pad: source pad of remote subdevice * @sink_pad: sink pad of remote subdevice *
@@ -88,6 +89,7 @@ struct rvin_parallel_entity {enum v4l2_mbus_type mbus_type;unsigned int mbus_flags; +bool chs;unsigned int source_pad;unsigned int sink_pad;
--
2.7.4
PATCH v3 8/8 ARM: dts: rcar-gen2: Remove unused VIN properties
In reply to Jacopo MondiThe '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.
---
v3:
- remove bus-width from dt-bindings example
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 1 - 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 - 7 files changed, 13 deletions(-)
Documentation/devicetree/bindings/media/rcar_vin.txt +0 -1
@@ -128,7 +128,6 @@ Board setup example for Gen2 platforms (vin1 composite video input) vin1ep0: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };
arch/arm/boot/dts/r8a7790-lager.dts +0 -3
@@ -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>; }; }; };
arch/arm/boot/dts/r8a7791-koelsch.dts +0 -3
@@ -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>; }; }; };
arch/arm/boot/dts/r8a7791-porter.dts +0 -1
@@ -388,7 +388,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };
arch/arm/boot/dts/r8a7793-gose.dts +0 -3
@@ -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>; }; }; };
arch/arm/boot/dts/r8a7794-alt.dts +0 -1
@@ -388,7 +388,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };
arch/arm/boot/dts/r8a7794-silk.dts +0 -1
@@ -477,7 +477,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };
--
2.7.4
Rob Herring
maintainerPATCH v3 8/8 ARM: dts: rcar-gen2: Remove unused VIN properties
Quoted 8 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 use them.
Signed-off-by Jacopo Mondi <[email protected]>---
v3:
- remove bus-width from dt-bindings example
Quoted 9 lines
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 1 - 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 - 7 files changed, 13 deletions(-)
PATCH v3 8/8 ARM: dts: rcar-gen2: Remove unused VIN properties
In reply to Jacopo MondiHi Jacopo,
Thanks for your work.
Quoted 5 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 use them.
Signed-off-by Jacopo Mondi <[email protected]>
The more I think about this the more I lean towards that this patch should be dropped. The properties accurately describes the hardware and I think there is value in that. That the driver currently don't parse or make use of them don't in my view reduce there value. Maybe you should break out this patch to a separate series?
Quoted 133 lines
---
v3:
- remove bus-width from dt-bindings example--- Documentation/devicetree/bindings/media/rcar_vin.txt | 1 - 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 - 7 files changed, 13 deletions(-)Documentation/devicetree/bindings/media/rcar_vin.txt +0
-1@@ -128,7 +128,6 @@ Board setup example for Gen2 platforms (vin1 composite video input) vin1ep0: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };arch/arm/boot/dts/r8a7790-lager.dts +0
-3@@ -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>; }; }; };arch/arm/boot/dts/r8a7791-koelsch.dts +0
-3@@ -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>; }; }; };arch/arm/boot/dts/r8a7791-porter.dts +0
-1@@ -388,7 +388,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };arch/arm/boot/dts/r8a7793-gose.dts +0
-3@@ -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>; }; }; };arch/arm/boot/dts/r8a7794-alt.dts +0
-1@@ -388,7 +388,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };arch/arm/boot/dts/r8a7794-silk.dts +0
-2@@ -477,7 +477,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; }; --2.7.4
Signature · 2 lines
Regards, Niklas S?derlund
PATCH v3 8/8 ARM: dts: rcar-gen2: Remove unused VIN properties
Quoted 9 lines · Niklas S?derlund
Hi Jacopo,
Thanks for your work.
Quoted 4 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 use them.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jacopo Mondi <[email protected]>The more I think about this the more I lean towards that this patch should be dropped. The properties accurately describes the hardware and I think there is value in that. That the driver currently don't parse or make use of them don't in my view reduce there value. Maybe you should break out this patch to a separate series?
I also think there is value in describing the hardware not the state of the driver at this time. Is there any missmatch between these properties and the bindings?
PATCH v3 8/8 ARM: dts: rcar-gen2: Remove unused VIN properties
Hi Simon,
Quoted 18 lines · Simon Horman
Quoted 9 lines · Niklas S?derlund
Hi Jacopo,
Thanks for your work.
Quoted 4 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 use them.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jacopo Mondi <[email protected]>The more I think about this the more I lean towards that this patch should be dropped. The properties accurately describes the hardware and I think there is value in that. That the driver currently don't parse or make use of them don't in my view reduce there value. Maybe you should break out this patch to a separate series?
Quoted 3 lines
I also think there is value in describing the hardware not the state of the driver at this time. Is there any missmatch between these properties and the bindings?
Niklas and I discussed a bit offline on this yesterday. My main concern, and sorry for being pedant on this, is that changing those properties value does not change the interface behaviour, and this could cause troubles when integrating image sensor not known to be working on the VIN interface.
This said, the documentation of those (and all other) properties is in the generic "video-interfaces.txt" file and it is my understanding, but I think Laurent and Rob agree on this as well from their replies to my previous series, that each driver should list which properties it actually supports, as some aspects are very implementation specific, like default values and what happens if the property is not specified [1]. Nonetheless, all properties describing hardware features and documented in the generic file should be accepted in DTS, as those aims to be OS-independent and even independent from the single driver implementation.
A possible middle-ground would be documenting in the VIN device tree bindings description properties whose values actually affect the VIN interface configuration and state in bindings that all generic properties described in 'video-interfaces.txt' are valid ones but if not explicitly listed there their value won't affect the interface configuration. Niklas suggested this, and I quite like the fact it makes clear that, say, changing the 'pclk-sample' property won't change how the VIN interface would sample data but at the same time allows for extensive description of the hardware.
Would something like this work in your opinion?
As a consequence, this patch should be dropped as Niklas and you suggested.
Thanks
j
[1] Ie. An interface that supports BT.656 encoding will use it if
'hsync-active' and 'vsync-active' are not specified. One that does not
support embedded synchronization would instead use defaults for those
two properties. This is specific to the single interface (or even the
single driver implementation actually) and should be listed in the single
driver's bindings description imo.
-------------- 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/20180605/5a7706b3/attachment.sig>
PATCH v3 8/8 ARM: dts: rcar-gen2: Remove unused VIN properties
Hi Jacopo,
Quoted 25 lines · jacopo mondi
Quoted 14 lines · Simon Horman
Quoted 5 lines · Niklas S?derlund
Quoted 4 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 use them.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jacopo Mondi <[email protected]>The more I think about this the more I lean towards that this patch should be dropped. The properties accurately describes the hardware and I think there is value in that. That the driver currently don't parse or make use of them don't in my view reduce there value. Maybe you should break out this patch to a separate series?
Quoted 3 lines
I also think there is value in describing the hardware not the state of the driver at this time. Is there any missmatch between these properties and the bindings?
Niklas and I discussed a bit offline on this yesterday. My main concern, and sorry for being pedant on this, is that changing those properties value does not change the interface behaviour, and this could cause troubles when integrating image sensor not known to be working on the VIN interface.
Quoted 4 lines
This said, the documentation of those (and all other) properties is in the generic "video-interfaces.txt" file and it is my understanding, but I think Laurent and Rob agree on this as well from their replies to my previous series, that each driver should list which properties it actually supports, as
s/driver/device-specific binding/
Quoted 2 lines
some aspects are very implementation specific, like default values and what happens if the property is not specified [1]. Nonetheless, all
In se defaults are not (Linux) implementation-specific, but fixed in the DT bindings.
Quoted 3 lines
properties describing hardware features and documented in the generic file should be accepted in DTS, as those aims to be OS-independent and even independent from the single driver implementation.
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds