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

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

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

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,
  

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

[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,