[RFC PATCH v2 0/2] [media] tvp515p: Proposal for MC input connector support

[RFC PATCH v2 0/2] [media] tvp515p: Proposal for MC input connector support

[Javier Martinez Canillas]

Hello,

This is a second version of an RFC patch series that adds MC input connector
support to the tvp5150 driver. The first RFC version was [0].

The patches are RFC because a previous version was merged and later reverted
since the approach was found to be inadequate. So I preferred to post this
approach as RFC to discuss it first.

The main difference with v1 is that a single sink pad is used for the tvp5150
(instead of using a pad per each input pin) as suggested by Mauro and Hans.

The mc_nextgen_test dot output after applying the series can be found at [1]
and the graph png generated using the dot tool is at [2].

I tested these patches on an IGEPv2 by capturing using both Composite inputs.

[0]: https://www.mail-archive.com/linux-media@vger.kernel.org/msg95389.html
[1]: http://hastebin.com/yiduhonome.tex
[2]: http://i.imgur.com/EyFtVtJ.png?1

Best regards,
Javier

Changes in v2:
- Remove from the changelog a mention of devices that multiplex the
  physical RCA connectors to be used for the S-Video Y and C signals
  since it's a special case and it doesn't really work on the IGEPv2.
- Use a single sink pad for the demod and map the connectors as entities
  so the mux is made via links. Suggested by Mauro and Hans.

Javier Martinez Canillas (2):
  [media] tvp5150: Add input connectors DT bindings
  [media] tvp5150: Replace connector support according to DT binding

 .../devicetree/bindings/media/i2c/tvp5150.txt      |  59 ++++++++
 drivers/media/i2c/tvp5150.c                        | 155 +++++++++++++++------
 2 files changed, 170 insertions(+), 44 deletions(-)

-- 
2.5.5

[RFC PATCH v2 1/2] [media] tvp5150: Add input connectors DT bindings

[Javier Martinez Canillas]

The tvp5150 and tvp5151 decoders support different video input source
connections to their AIP1A and AIP1B pins. Either two Composite or a
S-Video input signals are supported.

The possible configurations are as follows:

- Analog Composite signal connected to AIP1A.
- Analog Composite signal connected to AIP1B.
- Analog S-Video Y (luminance) and C (chrominance)
  signals connected to AIP1A and AIP1B respectively.

This patch extends the Device Tree binding documentation to describe
how the input connectors for these devices should be defined in a DT.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Hello,

The DT binding assumes that there is a 1:1 map between physical connectors
and connections, so there will be a connector described in the DT for each
connection.

There is also the question about how the DT bindings will be extended to
support other attributes (color/position/group) using the properties API.

But I believe that can be done as a follow-up, once the properties API is
in mainline.

Best regards,
Javier

Changes in v2:
- Remove from the changelog a mention of devices that multiplex the
  physical RCA connectors to be used for the S-Video Y and C signals
  since it's a special case and it doesn't really work on the IGEPv2.

 .../devicetree/bindings/media/i2c/tvp5150.txt      | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 8c0fc1a26bf0..df555650b0b4 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -26,8 +26,46 @@ Required Endpoint Properties for parallel synchronization:
 If none of hsync-active, vsync-active and field-even-active is specified,
 the endpoint is assumed to use embedded BT.656 synchronization.
 
+-Optional nodes:
+- connectors: The list of tvp5150 input connectors available on a given
+  board. The node should contain a child 'port' node for each connector.
+
+  The tvp5150 has support for three possible connectors: 2 Composite and
+  1 S-video. The "reg" property is used to specify which input connector
+  is associated with each 'port', using the following possible values:
+
+  0: Composite0
+  1: Composite1
+  2: S-Video
+
+  The ports should have an endpoint subnode that is linked to a connector
+  node defined in Documentation/devicetree/bindings/display/connector/.
+  The linked connector compatible string should match the connector type.
+
 Example:
 
+composite0: connector@0 {
+	compatible = "composite-video-connector";
+	label = "Composite0";
+
+	port {
+		comp0_out: endpoint {
+			remote-endpoint = <&tvp5150_comp0_in>;
+		};
+	};
+};
+
+svideo: connector@1 {
+	compatible = "composite-video-connector";
+	label = "S-Video";
+
+	port {
+		svideo_out: endpoint {
+			remote-endpoint = <&tvp5150_svideo_in>;
+		};
+	};
+};
+
 &i2c2 {
 	...
 	tvp5150@5c {
@@ -36,6 +74,27 @@ Example:
 		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
 		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
 
+		connectors {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/* Composite0 input */
+			port@0 {
+				reg = <0>;
+				tvp5150_comp0_in: endpoint {
+					remote-endpoint = <&comp0_out>;
+				};
+			};
+
+			/* S-Video input */
+			port@2 {
+				reg = <2>;
+				tvp5150_svideo_in: endpoint {
+					remote-endpoint = <&svideo_out>;
+				};
+			};
+		};
+
 		port {
 			tvp5150_1: endpoint {
 				remote-endpoint = <&ccdc_ep>;
-- 
2.5.5

[RFC PATCH v2 2/2] [media] tvp5150: Replace connector support according to DT binding

[Javier Martinez Canillas]

The tvp5150 and tvp5151 decoders support different video input source
connections to their AIP1A and AIP1B pins. Either two Composite or a
S-Video input signals are supported.

The input connector support was added before to the device DT binding
with commit 82c2ffeb217a ("[media] tvp5150: document input connectors
DT bindings"), but the binding was found to be wrong so was reverted.

A new binding documentation was added that uses OF graph endpoint and
ports for the input connectors, so this patch replaces the old logic
to be aligned to what is described in the latest tvp5150 binding doc.

The DT binding only describes the type of the input connectors and not
how the signals are routed to the chip, that information is inferred
by the driver that knows what are the possible input configurations.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

Changes in v2:
- Use a single sink pad for the demod and map the connectors as entities
  so the mux is made via links. Suggested by Mauro and Hans.

 drivers/media/i2c/tvp5150.c | 155 +++++++++++++++++++++++++++++++-------------
 1 file changed, 111 insertions(+), 44 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index ff18444e19e4..e5003d94f262 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1338,15 +1338,112 @@ static int tvp5150_init(struct i2c_client *c)
 	return 0;
 }
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+static int tvp5150_parse_connector_node(struct tvp5150 *decoder,
+					struct device_node *port)
+{
+	struct v4l2_of_endpoint endpoint;
+	struct media_entity *input;
+	struct device_node *ep, *rp;
+	unsigned int input_type;
+	const char *name;
+	int ret;
+
+	/* tvp5150 connector ports can have only one endpoint. */
+	ep = of_get_next_child(port, NULL);
+	if (!ep) {
+		v4l2_err(&decoder->sd, "Endpoint not found for connector %s\n",
+			 port->full_name);
+		return -EINVAL;
+	}
+
+	ret = v4l2_of_parse_endpoint(ep, &endpoint);
+	if (ret) {
+		v4l2_err(&decoder->sd, "Connector %s parse endpoint failed %d\n",
+			 port->full_name, ret);
+		goto err_ep;
+	}
+
+	input_type = endpoint.base.port;
+
+	if (input_type >= TVP5150_INPUT_NUM) {
+		v4l2_err(&decoder->sd,
+			 "Connector %s port address %u is not a valid one\n",
+			 port->full_name, input_type);
+		ret = -EINVAL;
+		goto err_ep;
+	}
+
+	input = &decoder->input_ent[input_type];
+
+	/* Each input connector can only be defined once */
+	if (input->name) {
+		v4l2_err(&decoder->sd,
+			 "Connector %s with same type already exists\n",
+			 input->name);
+		ret = -EINVAL;
+		goto err_ep;
+	}
+
+	rp = of_graph_get_remote_port_parent(ep);
+	if (!rp) {
+		v4l2_err(&decoder->sd, "Port %s remote parent not found\n",
+			 ep->full_name);
+		ret = -EINVAL;
+		goto err_ep;
+	}
+
+	switch (input_type) {
+	case TVP5150_COMPOSITE0:
+	case TVP5150_COMPOSITE1:
+		if (!of_device_is_compatible(rp, "composite-video-connector")) {
+			v4l2_err(&decoder->sd, "Wrong compatible for port %s\n",
+				 rp->full_name);
+			ret = -EINVAL;
+			goto err_rp;
+		}
+
+		input->function = MEDIA_ENT_F_CONN_COMPOSITE;
+		break;
+	case TVP5150_SVIDEO:
+		if (!of_device_is_compatible(rp, "svideo-connector")) {
+			v4l2_err(&decoder->sd, "Wrong compatible for port %s\n",
+				 rp->full_name);
+			ret = -EINVAL;
+			goto err_rp;
+		}
+
+		input->function = MEDIA_ENT_F_CONN_SVIDEO;
+		break;
+	}
+
+	input->flags = MEDIA_ENT_FL_CONNECTOR;
+
+	ret = of_property_read_string(rp, "label", &name);
+	if (ret < 0) {
+		v4l2_err(&decoder->sd,
+			 "Missing label property in port %s\n",
+			 rp->full_name);
+		goto err_rp;
+	}
+
+	input->name = name;
+
+err_rp:
+	of_node_put(rp);
+err_ep:
+	of_node_put(ep);
+	return ret;
+
+}
+#endif
+
 static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 {
 	struct v4l2_of_endpoint bus_cfg;
 	struct device_node *ep;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct device_node *connectors, *child;
-	struct media_entity *input;
-	const char *name;
-	u32 input_type;
 #endif
 	unsigned int flags;
 	int ret = 0;
@@ -1377,52 +1474,22 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 	if (!connectors)
 		goto err;
 
+	if (of_get_child_count(connectors) > TVP5150_INPUT_NUM) {
+		v4l2_err(&decoder->sd,
+			 "Maximum number of connectors is %d\n",
+			 TVP5150_INPUT_NUM);
+		ret = -EINVAL;
+		goto err_connector;
+	}
+
 	for_each_available_child_of_node(connectors, child) {
-		ret = of_property_read_u32(child, "input", &input_type);
+		ret = tvp5150_parse_connector_node(decoder, child);
 		if (ret) {
 			v4l2_err(&decoder->sd,
-				 "missing type property in node %s\n",
-				 child->name);
-			goto err_connector;
-		}
-
-		if (input_type >= TVP5150_INPUT_NUM) {
-			ret = -EINVAL;
+				 "Connector %s parse failed\n", child->name);
+			of_node_put(child);
 			goto err_connector;
 		}
-
-		input = &decoder->input_ent[input_type];
-
-		/* Each input connector can only be defined once */
-		if (input->name) {
-			v4l2_err(&decoder->sd,
-				 "input %s with same type already exists\n",
-				 input->name);
-			ret = -EINVAL;
-			goto err_connector;
-		}
-
-		switch (input_type) {
-		case TVP5150_COMPOSITE0:
-		case TVP5150_COMPOSITE1:
-			input->function = MEDIA_ENT_F_CONN_COMPOSITE;
-			break;
-		case TVP5150_SVIDEO:
-			input->function = MEDIA_ENT_F_CONN_SVIDEO;
-			break;
-		}
-
-		input->flags = MEDIA_ENT_FL_CONNECTOR;
-
-		ret = of_property_read_string(child, "label", &name);
-		if (ret < 0) {
-			v4l2_err(&decoder->sd,
-				 "missing label property in node %s\n",
-				 child->name);
-			goto err_connector;
-		}
-
-		input->name = name;
 	}
 
 err_connector:
-- 
2.5.5

Re: [RFC PATCH v2 0/2] [media] tvp515p: Proposal for MC input connector support

[Javier Martinez Canillas]

On 04/12/2016 06:42 PM, Javier Martinez Canillas wrote:
> Hello,
> 
> This is a second version of an RFC patch series that adds MC input connector
> support to the tvp5150 driver. The first RFC version was [0].
> 
> The patches are RFC because a previous version was merged and later reverted
> since the approach was found to be inadequate. So I preferred to post this
> approach as RFC to discuss it first.
> 
> The main difference with v1 is that a single sink pad is used for the tvp5150
> (instead of using a pad per each input pin) as suggested by Mauro and Hans.
> 
> The mc_nextgen_test dot output after applying the series can be found at [1]
> and the graph png generated using the dot tool is at [2].
> 
> I tested these patches on an IGEPv2 by capturing using both Composite inputs.
> 
> [0]: https://www.mail-archive.com/linux-media@vger.kernel.org/msg95389.html
> [1]: http://hastebin.com/yiduhonome.tex
> [2]: http://i.imgur.com/EyFtVtJ.png?1
> 
> Best regards,
> Javier
> 
> Changes in v2:
> - Remove from the changelog a mention of devices that multiplex the
>   physical RCA connectors to be used for the S-Video Y and C signals
>   since it's a special case and it doesn't really work on the IGEPv2.
> - Use a single sink pad for the demod and map the connectors as entities
>   so the mux is made via links. Suggested by Mauro and Hans.
> 
> Javier Martinez Canillas (2):
>   [media] tvp5150: Add input connectors DT bindings
>   [media] tvp5150: Replace connector support according to DT binding
> 
>  .../devicetree/bindings/media/i2c/tvp5150.txt      |  59 ++++++++
>  drivers/media/i2c/tvp5150.c                        | 155 +++++++++++++++------
>  2 files changed, 170 insertions(+), 44 deletions(-)
> 

Any comments about this series?

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH v2 1/2] [media] tvp5150: Add input connectors DT bindings

[Laurent Pinchart]

Hi Javier,

Thank you for the patch.

On Tuesday 12 Apr 2016 18:42:52 Javier Martinez Canillas wrote:
> The tvp5150 and tvp5151 decoders support different video input source
> connections to their AIP1A and AIP1B pins. Either two Composite or a
> S-Video input signals are supported.
> 
> The possible configurations are as follows:
> 
> - Analog Composite signal connected to AIP1A.
> - Analog Composite signal connected to AIP1B.
> - Analog S-Video Y (luminance) and C (chrominance)
>   signals connected to AIP1A and AIP1B respectively.
> 
> This patch extends the Device Tree binding documentation to describe
> how the input connectors for these devices should be defined in a DT.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> Hello,
> 
> The DT binding assumes that there is a 1:1 map between physical connectors
> and connections, so there will be a connector described in the DT for each
> connection.
> 
> There is also the question about how the DT bindings will be extended to
> support other attributes (color/position/group) using the properties API.

I foresee lots of bikeshedding on that particular topic, but I don't think it 
will be a blocker. We need a volunteer to quickstart a discussion on the 
devicetree (or possible devicetree-spec) mailing list :-)

> But I believe that can be done as a follow-up, once the properties API is
> in mainline.
> 
> Best regards,
> Javier
> 
> Changes in v2:
> - Remove from the changelog a mention of devices that multiplex the
>   physical RCA connectors to be used for the S-Video Y and C signals
>   since it's a special case and it doesn't really work on the IGEPv2.
> 
>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
> :
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt index
> 8c0fc1a26bf0..df555650b0b4 100644
> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> @@ -26,8 +26,46 @@ Required Endpoint Properties for parallel
> synchronization: If none of hsync-active, vsync-active and
> field-even-active is specified, the endpoint is assumed to use embedded
> BT.656 synchronization.
> 
> +-Optional nodes:
> +- connectors: The list of tvp5150 input connectors available on a given
> +  board. The node should contain a child 'port' node for each connector.

I had understood this as meaning that connectors should be fully described in 
the connectors subnode, until I read through the whole patch and saw that 
dedicated DT nodes are needed for the connectors. I thus believe the paragraph 
should be reworded to avoid the ambiguity.

This being said, why do you need a connectors subnode ? Can't we just add the 
port nodes for the input ports directly in the tvp5150 node (or possibly in a 
ports subnode, as defined in the OF graph bindings).

> +  The tvp5150 has support for three possible connectors: 2 Composite and
> +  1 S-video. The "reg" property is used to specify which input connector
> +  is associated with each 'port', using the following possible values:
> +
> +  0: Composite0
> +  1: Composite1
> +  2: S-Video
> +
> +  The ports should have an endpoint subnode that is linked to a connector
> +  node defined in Documentation/devicetree/bindings/display/connector/.
> +  The linked connector compatible string should match the connector type.
> +
>  Example:
> 
> +composite0: connector@0 {
> +	compatible = "composite-video-connector";
> +	label = "Composite0";
> +
> +	port {
> +		comp0_out: endpoint {
> +			remote-endpoint = <&tvp5150_comp0_in>;
> +		};
> +	};
> +};
> +
> +svideo: connector@1 {
> +	compatible = "composite-video-connector";
> +	label = "S-Video";
> +
> +	port {
> +		svideo_out: endpoint {
> +			remote-endpoint = <&tvp5150_svideo_in>;
> +		};
> +	};
> +};
> +
>  &i2c2 {
>  	...
>  	tvp5150@5c {
> @@ -36,6 +74,27 @@ Example:
>  		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
>  		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> 
> +		connectors {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			/* Composite0 input */
> +			port@0 {
> +				reg = <0>;
> +				tvp5150_comp0_in: endpoint {
> +					remote-endpoint = <&comp0_out>;
> +				};
> +			};
> +
> +			/* S-Video input */
> +			port@2 {
> +				reg = <2>;
> +				tvp5150_svideo_in: endpoint {
> +					remote-endpoint = <&svideo_out>;
> +				};
> +			};
> +		};
> +
>  		port {
>  			tvp5150_1: endpoint {
>  				remote-endpoint = <&ccdc_ep>;

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 1/2] [media] tvp5150: Add input connectors DT bindings

[Javier Martinez Canillas]

Hello Laurent,

Thanks a lot for your feedback.

On 04/27/2016 10:29 AM, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Tuesday 12 Apr 2016 18:42:52 Javier Martinez Canillas wrote:
>> The tvp5150 and tvp5151 decoders support different video input source
>> connections to their AIP1A and AIP1B pins. Either two Composite or a
>> S-Video input signals are supported.
>>
>> The possible configurations are as follows:
>>
>> - Analog Composite signal connected to AIP1A.
>> - Analog Composite signal connected to AIP1B.
>> - Analog S-Video Y (luminance) and C (chrominance)
>>   signals connected to AIP1A and AIP1B respectively.
>>
>> This patch extends the Device Tree binding documentation to describe
>> how the input connectors for these devices should be defined in a DT.
>>
>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>> Hello,
>>
>> The DT binding assumes that there is a 1:1 map between physical connectors
>> and connections, so there will be a connector described in the DT for each
>> connection.
>>
>> There is also the question about how the DT bindings will be extended to
>> support other attributes (color/position/group) using the properties API.
> 
> I foresee lots of bikeshedding on that particular topic, but I don't think it 
> will be a blocker. We need a volunteer to quickstart a discussion on the 
> devicetree (or possible devicetree-spec) mailing list :-)
>

Yes, I plan to extend this binding once we have the properties API in mainline
but that can be done as a follow-up since it should just be more properties on
top of compatible, label and port that will be supported in the meantime.
 
>> But I believe that can be done as a follow-up, once the properties API is
>> in mainline.
>>
>> Best regards,
>> Javier
>>
>> Changes in v2:
>> - Remove from the changelog a mention of devices that multiplex the
>>   physical RCA connectors to be used for the S-Video Y and C signals
>>   since it's a special case and it doesn't really work on the IGEPv2.
>>
>>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 59 +++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>> :
>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>> b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt index
>> 8c0fc1a26bf0..df555650b0b4 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>> @@ -26,8 +26,46 @@ Required Endpoint Properties for parallel
>> synchronization: If none of hsync-active, vsync-active and
>> field-even-active is specified, the endpoint is assumed to use embedded
>> BT.656 synchronization.
>>
>> +-Optional nodes:
>> +- connectors: The list of tvp5150 input connectors available on a given
>> +  board. The node should contain a child 'port' node for each connector.
> 
> I had understood this as meaning that connectors should be fully described in 
> the connectors subnode, until I read through the whole patch and saw that 
> dedicated DT nodes are needed for the connectors. I thus believe the paragraph 
> should be reworded to avoid the ambiguity.
>

I see what you mean, OK I'll make it clear that this only is the list of ports
and that connectors should be described somewhere else (i.e: the root node).

> This being said, why do you need a connectors subnode ? Can't we just add the 
> port nodes for the input ports directly in the tvp5150 node (or possibly in a 
> ports subnode, as defined in the OF graph bindings).
>

Yes we could, I went with a "connectors" subnode because the video decoders
will have another port node to point to the bridge device node endpoint. So
I thought it could be more clear to make a distinction between those ports.

We can go with the "ports" subnode instead of "connectors" but then again it
could be confusing to differentiate between bridge and connectors ports both
for users writing/reading DTS and the drivers parsing the DT.

I used as an inspiration the regulators binding where regulators are usually
described under a "regulators" subnode.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH v2 1/2] [media] tvp5150: Add input connectors DT bindings

[Hans Verkuil]

This old mail came up in a discussion today so let me comment on this:

On 04/27/2016 09:12 PM, Javier Martinez Canillas wrote:
> Hello Laurent,
> 
> Thanks a lot for your feedback.
> 
> On 04/27/2016 10:29 AM, Laurent Pinchart wrote:
>> Hi Javier,
>>
>> Thank you for the patch.
>>
>> On Tuesday 12 Apr 2016 18:42:52 Javier Martinez Canillas wrote:
>>> The tvp5150 and tvp5151 decoders support different video input source
>>> connections to their AIP1A and AIP1B pins. Either two Composite or a
>>> S-Video input signals are supported.
>>>
>>> The possible configurations are as follows:
>>>
>>> - Analog Composite signal connected to AIP1A.
>>> - Analog Composite signal connected to AIP1B.
>>> - Analog S-Video Y (luminance) and C (chrominance)
>>>   signals connected to AIP1A and AIP1B respectively.
>>>
>>> This patch extends the Device Tree binding documentation to describe
>>> how the input connectors for these devices should be defined in a DT.
>>>
>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>
>>> ---
>>> Hello,
>>>
>>> The DT binding assumes that there is a 1:1 map between physical connectors
>>> and connections, so there will be a connector described in the DT for each
>>> connection.
>>>
>>> There is also the question about how the DT bindings will be extended to
>>> support other attributes (color/position/group) using the properties API.
>>
>> I foresee lots of bikeshedding on that particular topic, but I don't think it 
>> will be a blocker. We need a volunteer to quickstart a discussion on the 
>> devicetree (or possible devicetree-spec) mailing list :-)
>>
> 
> Yes, I plan to extend this binding once we have the properties API in mainline
> but that can be done as a follow-up since it should just be more properties on
> top of compatible, label and port that will be supported in the meantime.
>  
>>> But I believe that can be done as a follow-up, once the properties API is
>>> in mainline.
>>>
>>> Best regards,
>>> Javier
>>>
>>> Changes in v2:
>>> - Remove from the changelog a mention of devices that multiplex the
>>>   physical RCA connectors to be used for the S-Video Y and C signals
>>>   since it's a special case and it doesn't really work on the IGEPv2.
>>>
>>>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 59 +++++++++++++++++++
>>>  1 file changed, 59 insertions(+)
>>> :
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>> b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt index
>>> 8c0fc1a26bf0..df555650b0b4 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>> @@ -26,8 +26,46 @@ Required Endpoint Properties for parallel
>>> synchronization: If none of hsync-active, vsync-active and
>>> field-even-active is specified, the endpoint is assumed to use embedded
>>> BT.656 synchronization.
>>>
>>> +-Optional nodes:
>>> +- connectors: The list of tvp5150 input connectors available on a given
>>> +  board. The node should contain a child 'port' node for each connector.
>>
>> I had understood this as meaning that connectors should be fully described in 
>> the connectors subnode, until I read through the whole patch and saw that 
>> dedicated DT nodes are needed for the connectors. I thus believe the paragraph 
>> should be reworded to avoid the ambiguity.
>>
> 
> I see what you mean, OK I'll make it clear that this only is the list of ports
> and that connectors should be described somewhere else (i.e: the root node).
> 
>> This being said, why do you need a connectors subnode ? Can't we just add the 
>> port nodes for the input ports directly in the tvp5150 node (or possibly in a 
>> ports subnode, as defined in the OF graph bindings).
>>
> 
> Yes we could, I went with a "connectors" subnode because the video decoders
> will have another port node to point to the bridge device node endpoint. So
> I thought it could be more clear to make a distinction between those ports.
> 
> We can go with the "ports" subnode instead of "connectors" but then again it
> could be confusing to differentiate between bridge and connectors ports both
> for users writing/reading DTS and the drivers parsing the DT.
> 
> I used as an inspiration the regulators binding where regulators are usually
> described under a "regulators" subnode.

I am inclined to go with Laurent on this. In the end these are just pins and while
they usually will be hooked up to connectors, that doesn't have to be the case.
There may be another component in between, so I don't really like it that it is
called 'connector'. In my mind it is just another (input) port. It is really
only after looking at the remote endpoint that you see that there is a connector
connected to the pin(s).

Regards,

	Hans

Re: [RFC PATCH v2 1/2] [media] tvp5150: Add input connectors DT bindings

[Javier Martinez Canillas]

[adding Kevin as cc since he was interested in this feature as well]

Hello Hans,

On 11/11/2016 01:11 PM, Hans Verkuil wrote:
> This old mail came up in a discussion today so let me comment on this:
>

Thanks a lot for the feedback.
 
> On 04/27/2016 09:12 PM, Javier Martinez Canillas wrote:
>> Hello Laurent,
>>
>> Thanks a lot for your feedback.
>>
>> On 04/27/2016 10:29 AM, Laurent Pinchart wrote:
>>> Hi Javier,
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday 12 Apr 2016 18:42:52 Javier Martinez Canillas wrote:
>>>> The tvp5150 and tvp5151 decoders support different video input source
>>>> connections to their AIP1A and AIP1B pins. Either two Composite or a
>>>> S-Video input signals are supported.
>>>>
>>>> The possible configurations are as follows:
>>>>
>>>> - Analog Composite signal connected to AIP1A.
>>>> - Analog Composite signal connected to AIP1B.
>>>> - Analog S-Video Y (luminance) and C (chrominance)
>>>>   signals connected to AIP1A and AIP1B respectively.
>>>>
>>>> This patch extends the Device Tree binding documentation to describe
>>>> how the input connectors for these devices should be defined in a DT.
>>>>
>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>>
>>>> ---
>>>> Hello,
>>>>
>>>> The DT binding assumes that there is a 1:1 map between physical connectors
>>>> and connections, so there will be a connector described in the DT for each
>>>> connection.
>>>>
>>>> There is also the question about how the DT bindings will be extended to
>>>> support other attributes (color/position/group) using the properties API.
>>>
>>> I foresee lots of bikeshedding on that particular topic, but I don't think it 
>>> will be a blocker. We need a volunteer to quickstart a discussion on the 
>>> devicetree (or possible devicetree-spec) mailing list :-)
>>>
>>
>> Yes, I plan to extend this binding once we have the properties API in mainline
>> but that can be done as a follow-up since it should just be more properties on
>> top of compatible, label and port that will be supported in the meantime.
>>  
>>>> But I believe that can be done as a follow-up, once the properties API is
>>>> in mainline.
>>>>
>>>> Best regards,
>>>> Javier
>>>>
>>>> Changes in v2:
>>>> - Remove from the changelog a mention of devices that multiplex the
>>>>   physical RCA connectors to be used for the S-Video Y and C signals
>>>>   since it's a special case and it doesn't really work on the IGEPv2.
>>>>
>>>>  .../devicetree/bindings/media/i2c/tvp5150.txt      | 59 +++++++++++++++++++
>>>>  1 file changed, 59 insertions(+)
>>>> :
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>>> b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt index
>>>> 8c0fc1a26bf0..df555650b0b4 100644
>>>> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>>> @@ -26,8 +26,46 @@ Required Endpoint Properties for parallel
>>>> synchronization: If none of hsync-active, vsync-active and
>>>> field-even-active is specified, the endpoint is assumed to use embedded
>>>> BT.656 synchronization.
>>>>
>>>> +-Optional nodes:
>>>> +- connectors: The list of tvp5150 input connectors available on a given
>>>> +  board. The node should contain a child 'port' node for each connector.
>>>
>>> I had understood this as meaning that connectors should be fully described in 
>>> the connectors subnode, until I read through the whole patch and saw that 
>>> dedicated DT nodes are needed for the connectors. I thus believe the paragraph 
>>> should be reworded to avoid the ambiguity.
>>>
>>
>> I see what you mean, OK I'll make it clear that this only is the list of ports
>> and that connectors should be described somewhere else (i.e: the root node).
>>
>>> This being said, why do you need a connectors subnode ? Can't we just add the 
>>> port nodes for the input ports directly in the tvp5150 node (or possibly in a 
>>> ports subnode, as defined in the OF graph bindings).
>>>
>>
>> Yes we could, I went with a "connectors" subnode because the video decoders
>> will have another port node to point to the bridge device node endpoint. So
>> I thought it could be more clear to make a distinction between those ports.
>>
>> We can go with the "ports" subnode instead of "connectors" but then again it
>> could be confusing to differentiate between bridge and connectors ports both
>> for users writing/reading DTS and the drivers parsing the DT.
>>
>> I used as an inspiration the regulators binding where regulators are usually
>> described under a "regulators" subnode.
> 
> I am inclined to go with Laurent on this. In the end these are just pins and while
> they usually will be hooked up to connectors, that doesn't have to be the case.
> There may be another component in between, so I don't really like it that it is
> called 'connector'. In my mind it is just another (input) port. It is really
> only after looking at the remote endpoint that you see that there is a connector
> connected to the pin(s).
>

Ok, I'll re-spin the patches then and do the change according to Laurent's and your
suggestions. It may take some time though since I'm currently busy with other tasks.
 
> Regards,
> 
> 	Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America