media: rcar-vin: Brush endpoint properties
Subsystems (4)
v4 cover media: rcar-vin: Brush endpoint properties
Hello,
4th round for the VIN endpoint brushing series.
Slightly enlarged the linux-media receiver list, as this new version introduces a common property in 'video-interfaces.txt'.
Compared to v3 I have dropped the last controversial parts:
- The custom 'renesas,hsync-as-de' property has been dropped: do not handle CHS bit for the moment.
- Do not remove properties not parsed by the driver and not listed in the interface bindings from Gen2 boards. As this lead to a long discussion, I have now proposed a patch to clearly state that properties not listed in the interface bindings can be optionally specified, but they don't affect the interface behaviour. To avoid more discussions this patch may be dropped and things will stay the way they are now.
For the common 'data-enable-active' property, I guess with Rob's ack we should be fine there and I hope the rest of the series won't slow down its acceptance.
Thanks
j
Jacopo Mondi (6):
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: media: rcar-vin: Clarify optional props
Documentation/devicetree/bindings/media/rcar_vin.txt | 18 ++++++++++++++++++ .../devicetree/bindings/media/video-interfaces.txt | 2 ++ drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++ drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++++ include/media/v4l2-mediabus.h | 2 ++ 5 files changed, 31 insertions(+)
--
2.7.4
PATCH v4 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.
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 10 ++++++++++ 1 file changed, 10 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +10 -0
@@ -53,6 +53,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.
@@ -62,6 +70,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 --------------------------------------
Signature · 1 line
2.7.4
PATCH v4 2/6 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 lines have to sampled for valid pixel data.
--- 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 · 1 line
2.7.4
PATCH v4 3/6 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.
--- 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 */
Signature · 1 line
2.7.4
PATCH v4 4/6 dt-bindings: media: rcar-vin: Add 'data-enable-active'
In reply to Jacopo MondiDescribe optional endpoint property 'data-enable-active' for R-Car VIN.
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +2 -0
@@ -57,6 +57,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC. - Optional properties for endpoint nodes of port@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.
Signature · 1 line
2.7.4
PATCH v4 5/6 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.
--- 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 ; }/*
Signature · 1 line
2.7.4
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
In reply to Jacopo MondiAdd a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
--- Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/media/rcar_vin.txt +6 -0
@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC. instances that are connected to external pins should have port 0. - Optional properties for endpoint nodes of port at 0: + + All properties described in [1] and which apply to the selected + media bus type could be optionally listed here to better describe + the current hardware configuration, but only the following ones do + actually modify the VIN interface behaviour: + - 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
Signature · 1 line
2.7.4
Sakari Ailus
maintainerPATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
Hi Jacopo,
Quoted 15 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>--- Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 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 63
Quoted 9 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +6
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC. instances that are connected to external pins should have port 0. - Optional properties for endpoint nodes of port at 0: + + All properties described in [1] and which apply to the selected + media bus type could be optionally listed here to better describe + the current hardware configuration, but only the following ones do + actually modify the VIN interface behaviour: +
I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
Quoted 6 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +0
-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] forSignature · 1 line
2.7.4
Signature · 2 lines
Sakari Ailus sakari.ailus at linux.intel.com
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
Hello,
Quoted 22 lines · On Tuesday, 12 June 2018 18:45:53 EEST Sakari Ailus wrote:
Quoted 21 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port@0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>---
Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)(quoted diff) +0
-0
b/Documentation/devicetree/bindings/media/rcar_vin.txtindex 8130849..03544c7 100644Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending onSoC.
instances that are connected to external pins should have port 0.
Quoted 11 lines
Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 63
Quoted 8 lines
- Optional properties for endpoint nodes of port@0:
Documentation/devicetree/bindings/media/rcar_vin.txt +4
-0+ + All properties described in [1] and which apply to the selected + media bus type could be optionally listed here to better describe + the current hardware configuration, but only the following onesdo
Documentation/devicetree/bindings/media/rcar_vin.txt +2
-0+ actually modify the VIN interface behaviour: +I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
I agree. Please list explicitly all properties that are applicable for this device, and don't tell which one(s) are used by the driver.
Quoted 3 lines
Quoted 3 lines
- 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
Signature · 3 lines
Regards, Laurent Pinchart
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
In reply to Sakari AilusHi Sakari,
Quoted 20 lines · Sakari Ailus
Hi Jacopo,
Quoted 17 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>--- Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC. instances that are connected to external pins should have port 0.
Quoted 11 lines
Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 63
Quoted 7 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +6
-0- Optional properties for endpoint nodes of port at 0: + + All properties described in [1] and which apply to the selected + media bus type could be optionally listed here to better describe + the current hardware configuration, but only the following ones do + actually modify the VIN interface behaviour: +I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
There has been quite some debate on this, and please bear with me here for re-proposing it: I started by removing properties in some DT files for older Renesas board which listed endpoint properties not documented in the VIN's bindings and not parsed by the VIN driver [1] Niklas (but Simon and Geert seems to agree here) opposed to that patch, as those properties where described in 'video-interfaces.txt' and even if not parsed by the current driver implementation, they actually describe hardware. I rebated that only properties listed in the device bindings documentation should actually be used, and having properties not parsed by the driver confuses users, which may expect changing them modifies the interface configuration, which does not happens at the moment.
This came out as a middle ground from a discussion with Niklas. As stated in the cover letter if this patch makes someone uncomfortable, feel free to drop it not to hold back the rest of the series which has been well received instead.
Thanks
j
[1] https://www.spinics.net/lists/arm-kernel/msg656302.html
Quoted 10 lines
Quoted 6 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +0
-1- 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 --2.7.4
--
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/20180613/e8557938/attachment.sig>
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
Hi Jacopo, Sakari and Laurent,
Jacopo, thanks for your patch and continued work on resolving this.
Quoted 23 lines · Jacopo Mondi
Hi Sakari,
Quoted 20 lines · Sakari Ailus
Hi Jacopo,
Quoted 17 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>--- Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC. instances that are connected to external pins should have port 0.
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/media/rcar_vin.txt from local SoC CSI-2 receivers (port1) depending on SoC. · line 63
Quoted 7 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +6
-0- Optional properties for endpoint nodes of port at 0: + + All properties described in [1] and which apply to the selected + media bus type could be optionally listed here to better describe + the current hardware configuration, but only the following ones do + actually modify the VIN interface behaviour: +
I'm not sure the description have to be as explicit to that the properties in 'video-interfaces.txt' are not currently used by the driver. I find it not relevant which ones are used or not, what is important for me is that all properties in 'video-interfaces.txt' which can be used to describe the specific bus are valid for the DT description.
On a side note, in rcar_vin.txt we have this section describing the Gen2 bindings:
The per-board settings Gen2 platforms:
- port sub-node describing a single endpoint connected to the vin as described in video-interfaces.txt[1]. Only the first one will be considered as each vin interface has one input port.
This whole series only deals with documenting the Gen3 optional properties and not the Gen2. Maybe with parallel input support for Gen3 patches on there way to making it upstream this series should be extended to in a good way merge the port at 0 optional properties for both generations of hardware?
Quoted 18 lines
Quoted 4 lines
I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
There has been quite some debate on this, and please bear with me here for re-proposing it: I started by removing properties in some DT files for older Renesas board which listed endpoint properties not documented in the VIN's bindings and not parsed by the VIN driver [1] Niklas (but Simon and Geert seems to agree here) opposed to that patch, as those properties where described in 'video-interfaces.txt' and even if not parsed by the current driver implementation, they actually describe hardware. I rebated that only properties listed in the device bindings documentation should actually be used, and having properties not parsed by the driver confuses users, which may expect changing them modifies the interface configuration, which does not happens at the moment.
Quoted 4 lines
This came out as a middle ground from a discussion with Niklas. As stated in the cover letter if this patch makes someone uncomfortable, feel free to drop it not to hold back the rest of the series which has been well received instead.
What I don't agree with and sparked this debate from my side was the deletion of properties in dts files which correctly does describe the bus but which are not currently parsed by the driver. To me that is decreasing the value of the dts. If on the other hand the goal is to deprecate a property from the video-interfaces.txt by slowly removing them from dts where the driver don't use them I'm all for it. But I don't think this is the case here right?
Quoted 16 lines
Thanks
j[1] https://www.spinics.net/lists/arm-kernel/msg656302.html
Quoted 10 lines
Quoted 6 lines
Documentation/devicetree/bindings/media/rcar_vin.txt +0
-1- 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 --2.7.4
--
Sakari Ailus
sakari.ailus at linux.intel.com
Signature · 2 lines
Regards, Niklas S?derlund
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
Hi Niklas,
Quoted 34 lines · On Wednesday, 27 June 2018 08:24:31 EEST Niklas S?derlund wrote:
Quoted 33 lines · Jacopo Mondi
Quoted 32 lines · Sakari Ailus
Quoted 31 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>---
Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)(quoted diff) +0
-0
b/Documentation/devicetree/bindings/media/rcar_vin.txtindex 8130849..03544c7 100644Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending onSoC.
instances that are connected to external pins should have port 0.
- Optional properties for endpoint nodes of port at 0:
Documentation/devicetree/bindings/media/rcar_vin.txt +2
-0+ + All properties described in [1] and which apply to theselected
+ media bus type could be optionally listed here to better
describe
+ the current hardware configuration, but only the following
ones do
+ actually modify the VIN interface behaviour:
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
+
I'm not sure the description have to be as explicit to that the properties in 'video-interfaces.txt' are not currently used by the driver. I find it not relevant which ones are used or not, what is important for me is that all properties in 'video-interfaces.txt' which can be used to describe the specific bus are valid for the DT description.
I agree with you. The driver is irrelevant in this context. What matters is which properties are applicable to the bus. For instance, if the VIN parallel input supports configurable polarities for the h/v sync signals, hsync-active and vsync-active should be listed in the bindings. On the other hand, if the polarities are fixed, then the properties are not needed.
Quoted 8 lines
On a side note, in rcar_vin.txt we have this section describing the Gen2 bindings:
The per-board settings Gen2 platforms:
- port sub-node describing a single endpoint connected to the vin as described in video-interfaces.txt[1]. Only the first one will be considered as each vin interface has one input port.
Quoted 5 lines
This whole series only deals with documenting the Gen3 optional properties and not the Gen2. Maybe with parallel input support for Gen3 patches on there way to making it upstream this series should be extended to in a good way merge the port at 0 optional properties for both generations of hardware?
That would be nice too :-)
Quoted 21 lines
Quoted 16 lines
Quoted 2 lines
I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
There has been quite some debate on this, and please bear with me here for re-proposing it: I started by removing properties in some DT files for older Renesas board which listed endpoint properties not documented in the VIN's bindings and not parsed by the VIN driver [1] Niklas (but Simon and Geert seems to agree here) opposed to that patch, as those properties where described in 'video-interfaces.txt' and even if not parsed by the current driver implementation, they actually describe hardware. I rebated that only properties listed in the device bindings documentation should actually be used, and having properties not parsed by the driver confuses users, which may expect changing them modifies the interface configuration, which does not happens at the moment.
Quoted 4 lines
This came out as a middle ground from a discussion with Niklas. As stated in the cover letter if this patch makes someone uncomfortable, feel free to drop it not to hold back the rest of the series which has been well received instead.
Quoted 7 lines
What I don't agree with and sparked this debate from my side was the deletion of properties in dts files which correctly does describe the bus but which are not currently parsed by the driver. To me that is decreasing the value of the dts. If on the other hand the goal is to deprecate a property from the video-interfaces.txt by slowly removing them from dts where the driver don't use them I'm all for it. But I don't think this is the case here right?
I think you're right, I don't think that's the case.
We should not remove properties from the dts files when they correctly describe the hardware and have an added-value. On the other hand, if a property correctly describes the hardware, but is constrained to a single value due to hardware limitations, then we can omit it.
Quoted 5 lines
Quoted 5 lines
[1] https://www.spinics.net/lists/arm-kernel/msg656302.html
Quoted 3 lines
Quoted 3 lines
- 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
Signature · 3 lines
Regards, Laurent Pinchart
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
Hi Laurent, Niklas,
Quoted 46 lines · Laurent Pinchart
Hi Niklas,
Quoted 34 lines · On Wednesday, 27 June 2018 08:24:31 EEST Niklas S?derlund wrote:
Quoted 33 lines · Jacopo Mondi
Quoted 32 lines · Sakari Ailus
Quoted 31 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>---
Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)(quoted diff) +0
-0
b/Documentation/devicetree/bindings/media/rcar_vin.txtindex 8130849..03544c7 100644Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending onSoC.
instances that are connected to external pins should have port 0.
- Optional properties for endpoint nodes of port at 0:
Documentation/devicetree/bindings/media/rcar_vin.txt +2
-0+ + All properties described in [1] and which apply to theselected
+ media bus type could be optionally listed here to better
describe
+ the current hardware configuration, but only the following
ones do
+ actually modify the VIN interface behaviour:Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
+
I'm not sure the description have to be as explicit to that the properties in 'video-interfaces.txt' are not currently used by the driver. I find it not relevant which ones are used or not, what is important for me is that all properties in 'video-interfaces.txt' which can be used to describe the specific bus are valid for the DT description.
Quoted 6 lines
I agree with you. The driver is irrelevant in this context. What matters is which properties are applicable to the bus. For instance, if the VIN parallel input supports configurable polarities for the h/v sync signals, hsync-active and vsync-active should be listed in the bindings. On the other hand, if the polarities are fixed, then the properties are not needed.
Fine, I'll surrender then, you're too many to fight against :p
Joking aside, I see your points, and I'll resend dropping this last bit and documenting which properties applies to the interface.
I can list:
- vsync-active (added by this series)
- hsync-active (added by this series)
- data-enable-active (added by this series)
- field-active-even (to be added)
I haven't find a way to configure the plck-sample value in the VINs registers. Should we skip it?
I also think 'bus-width' and 'data-shift' are not directly configurable in registers but depends on SoC, the selected input interface, and the input image format (see tables at 26.3.1 datasheet revision 1.00), so it makes sense list them as acceptable properties.
Do I have your ack on this so I can re-spin?
Quoted 8 lines
Quoted 8 lines
On a side note, in rcar_vin.txt we have this section describing the Gen2 bindings:
The per-board settings Gen2 platforms:
- port sub-node describing a single endpoint connected to the vin as described in video-interfaces.txt[1]. Only the first one will be considered as each vin interface has one input port.
Quoted 7 lines
Quoted 5 lines
This whole series only deals with documenting the Gen3 optional properties and not the Gen2. Maybe with parallel input support for Gen3 patches on there way to making it upstream this series should be extended to in a good way merge the port at 0 optional properties for both generations of hardware?
That would be nice too :-)
I see..
Well, I would keep the Gen2/Gen3 sections separate, as they have a different layout of port nodes (Gen2 has a single 'port' node, while Gen3 could have several 'port' nodes enclosed in a 'ports' one).
But I could indeed reference the optional endpoint properties of 'port at 0' of Gen3 bindings in Gen2 ones. Or the other way around. Do you have any preference?
Thanks
j
Quoted 39 lines
Quoted 22 lines
Quoted 17 lines
Quoted 3 lines
I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
There has been quite some debate on this, and please bear with me here for re-proposing it: I started by removing properties in some DT files for older Renesas board which listed endpoint properties not documented in the VIN's bindings and not parsed by the VIN driver [1] Niklas (but Simon and Geert seems to agree here) opposed to that patch, as those properties where described in 'video-interfaces.txt' and even if not parsed by the current driver implementation, they actually describe hardware. I rebated that only properties listed in the device bindings documentation should actually be used, and having properties not parsed by the driver confuses users, which may expect changing them modifies the interface configuration, which does not happens at the moment.
Quoted 4 lines
This came out as a middle ground from a discussion with Niklas. As stated in the cover letter if this patch makes someone uncomfortable, feel free to drop it not to hold back the rest of the series which has been well received instead.
Quoted 7 lines
What I don't agree with and sparked this debate from my side was the deletion of properties in dts files which correctly does describe the bus but which are not currently parsed by the driver. To me that is decreasing the value of the dts. If on the other hand the goal is to deprecate a property from the video-interfaces.txt by slowly removing them from dts where the driver don't use them I'm all for it. But I don't think this is the case here right?
I think you're right, I don't think that's the case.
We should not remove properties from the dts files when they correctly describe the hardware and have an added-value. On the other hand, if a property correctly describes the hardware, but is constrained to a single value due to hardware limitations, then we can omit it.
Quoted 2 lines
Quoted 2 lines
Quoted 11 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
- 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--
Regards,Laurent Pinchart
-------------- 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/20180703/77435bfe/attachment.sig>
PATCH v4 6/6 dt-bindings: media: rcar-vin: Clarify optional props
Hi Jacopo,
Thanks for not giving up on this work :-)
Quoted 61 lines · Jacopo Mondi
Hi Laurent, Niklas,
Quoted 46 lines · Laurent Pinchart
Hi Niklas,
Quoted 34 lines · On Wednesday, 27 June 2018 08:24:31 EEST Niklas S?derlund wrote:
Quoted 33 lines · Jacopo Mondi
Quoted 32 lines · Sakari Ailus
Quoted 31 lines · Jacopo Mondi
Add a note to the R-Car VIN interface bindings to clarify that all properties listed as generic properties in video-interfaces.txt can be included in port at 0 endpoint, but if not explicitly listed in the interface bindings documentation, they do not modify it behaviour.
Signed-off-by Jacopo Mondi <[email protected]>---
Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++ 1 file changed, 6 insertions(+)(quoted diff) +0
-0
b/Documentation/devicetree/bindings/media/rcar_vin.txtindex 8130849..03544c7 100644Documentation/devicetree/bindings/media/rcar_vin.txt +0
-0@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending onSoC.
instances that are connected to external pins should have port 0.
- Optional properties for endpoint nodes of port at 0:
Documentation/devicetree/bindings/media/rcar_vin.txt +2
-0+ + All properties described in [1] and which apply to theselected
+ media bus type could be optionally listed here to better
describe
+ the current hardware configuration, but only the following
ones do
+ actually modify the VIN interface behaviour:Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
+
I'm not sure the description have to be as explicit to that the properties in 'video-interfaces.txt' are not currently used by the driver. I find it not relevant which ones are used or not, what is important for me is that all properties in 'video-interfaces.txt' which can be used to describe the specific bus are valid for the DT description.
Quoted 6 lines
I agree with you. The driver is irrelevant in this context. What matters is which properties are applicable to the bus. For instance, if the VIN parallel input supports configurable polarities for the h/v sync signals, hsync-active and vsync-active should be listed in the bindings. On the other hand, if the polarities are fixed, then the properties are not needed.
Fine, I'll surrender then, you're too many to fight against :p
Joking aside, I see your points, and I'll resend dropping this last bit and documenting which properties applies to the interface.
Quoted 8 lines
I can list:
- vsync-active (added by this series)
- hsync-active (added by this series)
- data-enable-active (added by this series)
- field-active-even (to be added)I haven't find a way to configure the plck-sample value in the VINs registers. Should we skip it?
IIRC my findings agree with yours, no register to control plck-sample in the VIN register set.
Quoted 7 lines
I also think 'bus-width' and 'data-shift' are not directly configurable in registers but depends on SoC, the selected input interface, and the input image format (see tables at 26.3.1 datasheet revision 1.00), so it makes sense list them as acceptable properties.
Do I have your ack on this so I can re-spin?
I think I understand your plan and I think it sounds ok, please try it :-)
Quoted 19 lines
Quoted 9 lines
Quoted 9 lines
On a side note, in rcar_vin.txt we have this section describing the Gen2 bindings:
The per-board settings Gen2 platforms:
- port sub-node describing a single endpoint connected to the vin as described in video-interfaces.txt[1]. Only the first one will be considered as each vin interface has one input port.Quoted 7 lines
Quoted 5 lines
This whole series only deals with documenting the Gen3 optional properties and not the Gen2. Maybe with parallel input support for Gen3 patches on there way to making it upstream this series should be extended to in a good way merge the port at 0 optional properties for both generations of hardware?
That would be nice too :-)
I see..
Quoted 7 lines
Well, I would keep the Gen2/Gen3 sections separate, as they have a different layout of port nodes (Gen2 has a single 'port' node, while Gen3 could have several 'port' nodes enclosed in a 'ports' one).
But I could indeed reference the optional endpoint properties of 'port at 0' of Gen3 bindings in Gen2 ones. Or the other way around. Do you have any preference?
My preference would be to list the properties in the Gen2 section and in the Gen3 port at 0 section refer to the Gen2 optional properties.
Quoted 55 lines
Thanks
jQuoted 50 lines
Quoted 22 lines
Quoted 17 lines
Quoted 3 lines
I don't think this should be needed. You should only have properties that describe the hardware configuration in a given system.
There has been quite some debate on this, and please bear with me here for re-proposing it: I started by removing properties in some DT files for older Renesas board which listed endpoint properties not documented in the VIN's bindings and not parsed by the VIN driver [1] Niklas (but Simon and Geert seems to agree here) opposed to that patch, as those properties where described in 'video-interfaces.txt' and even if not parsed by the current driver implementation, they actually describe hardware. I rebated that only properties listed in the device bindings documentation should actually be used, and having properties not parsed by the driver confuses users, which may expect changing them modifies the interface configuration, which does not happens at the moment.
Quoted 4 lines
This came out as a middle ground from a discussion with Niklas. As stated in the cover letter if this patch makes someone uncomfortable, feel free to drop it not to hold back the rest of the series which has been well received instead.
Quoted 7 lines
What I don't agree with and sparked this debate from my side was the deletion of properties in dts files which correctly does describe the bus but which are not currently parsed by the driver. To me that is decreasing the value of the dts. If on the other hand the goal is to deprecate a property from the video-interfaces.txt by slowly removing them from dts where the driver don't use them I'm all for it. But I don't think this is the case here right?
I think you're right, I don't think that's the case.
We should not remove properties from the dts files when they correctly describe the hardware and have an added-value. On the other hand, if a property correctly describes the hardware, but is constrained to a single value due to hardware limitations, then we can omit it.
Quoted 5 lines
Quoted 5 lines
[1] https://www.spinics.net/lists/arm-kernel/msg656302.html
Quoted 3 lines
Quoted 3 lines
- 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--
Regards,Laurent Pinchart
Signature · 2 lines
Regards, Niklas S?derlund