[18/22] partial revert of "[media] tvp5150: add HW input connectors support"

Commit Message

Marco Felsch June 28, 2018, 4:20 p.m. UTC

From: Javier Martinez Canillas <[e-mail]>

Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
added input signals support for the tvp5150, but the approach was found
to be incorrect so the corresponding DT binding commit 82c2ffeb217a
("[media] tvp5150: document input connectors DT bindings") was reverted.

This left the driver with an undocumented (and wrong) DT parsing logic,
so lets get rid of this code as well until the input connectors support
is implemented properly.

It's a partial revert due other patches added on top of mentioned commit
not allowing the commit to be reverted cleanly anymore. But all the code
related to the DT parsing logic and input entities creation are removed.

Suggested-by: Laurent Pinchart <[e-mail]>
Signed-off-by: Javier Martinez Canillas <[e-mail]>
Acked-by: Laurent Pinchart <[e-mail]>
[[e-mail]: rm TVP5150_INPUT_NUM define]
Signed-off-by: Marco Felsch <[e-mail]>
---
 drivers/media/i2c/tvp5150.c         | 141 ----------------------------
 include/dt-bindings/media/tvp5150.h |   2 -
 2 files changed, 143 deletions(-)

Comments

Rob Herring (Arm) July 3, 2018, 11:19 p.m. UTC | #1

On Thu, Jun 28, 2018 at 06:20:50PM +0200, Marco Felsch wrote:
> From: Javier Martinez Canillas <[e-mail]>
> 
> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> added input signals support for the tvp5150, but the approach was found
> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> 
> This left the driver with an undocumented (and wrong) DT parsing logic,
> so lets get rid of this code as well until the input connectors support
> is implemented properly.
> 
> It's a partial revert due other patches added on top of mentioned commit
> not allowing the commit to be reverted cleanly anymore. But all the code
> related to the DT parsing logic and input entities creation are removed.
> 
> Suggested-by: Laurent Pinchart <[e-mail]>
> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> Acked-by: Laurent Pinchart <[e-mail]>
> [[e-mail]: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch <[e-mail]>
> ---
>  drivers/media/i2c/tvp5150.c         | 141 ----------------------------

>  include/dt-bindings/media/tvp5150.h |   2 -

Acked-by: Rob Herring <[e-mail]>

>  2 files changed, 143 deletions(-)

Mauro Carvalho Chehab July 30, 2018, 6:18 p.m. UTC | #2

Em Thu, 28 Jun 2018 18:20:50 +0200
Marco Felsch <[e-mail]> escreveu:

> From: Javier Martinez Canillas <[e-mail]>
> 
> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> added input signals support for the tvp5150, but the approach was found
> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> 
> This left the driver with an undocumented (and wrong) DT parsing logic,
> so lets get rid of this code as well until the input connectors support
> is implemented properly.
> 
> It's a partial revert due other patches added on top of mentioned commit
> not allowing the commit to be reverted cleanly anymore. But all the code
> related to the DT parsing logic and input entities creation are removed.
> 
> Suggested-by: Laurent Pinchart <[e-mail]>
> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> Acked-by: Laurent Pinchart <[e-mail]>
> [[e-mail]: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch <[e-mail]>
> ---

...

> -static int tvp5150_registered(struct v4l2_subdev *sd)
> -{
> -#ifdef CONFIG_MEDIA_CONTROLLER
> -	struct tvp5150 *decoder = to_tvp5150(sd);
> -	int ret = 0;
> -	int i;
> -
> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> -		struct media_entity *input = &decoder->input_ent[i];
> -		struct media_pad *pad = &decoder->input_pad[i];
> -
> -		if (!input->name)
> -			continue;
> -
> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> -
> -		ret = media_entity_pads_init(input, 1, pad);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = media_create_pad_link(input, 0, &sd->entity,
> -					    DEMOD_PAD_IF_INPUT, 0);
> -		if (ret < 0) {
> -			media_device_unregister_entity(input);
> -			return ret;
> -		}
> -	}
> -#endif

Hmm... I suspect that reverting this part may cause problems for drivers
like em28xx when compiled with MC, as they rely that the supported demods
will have 3 pads (DEMOD_NUM_PADS).

Thanks,
Mauro

Marco Felsch July 31, 2018, 7:01 a.m. UTC | #3

Hi Mauro,

On 18-07-30 15:18, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 18:20:50 +0200
> Marco Felsch <[e-mail]> escreveu:
> 
> > From: Javier Martinez Canillas <[e-mail]>
> > 
> > Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> > added input signals support for the tvp5150, but the approach was found
> > to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> > ("[media] tvp5150: document input connectors DT bindings") was reverted.
> > 
> > This left the driver with an undocumented (and wrong) DT parsing logic,
> > so lets get rid of this code as well until the input connectors support
> > is implemented properly.
> > 
> > It's a partial revert due other patches added on top of mentioned commit
> > not allowing the commit to be reverted cleanly anymore. But all the code
> > related to the DT parsing logic and input entities creation are removed.
> > 
> > Suggested-by: Laurent Pinchart <[e-mail]>
> > Signed-off-by: Javier Martinez Canillas <[e-mail]>
> > Acked-by: Laurent Pinchart <[e-mail]>
> > [[e-mail]: rm TVP5150_INPUT_NUM define]
> > Signed-off-by: Marco Felsch <[e-mail]>
> > ---
> 
> ...
> 
> > -static int tvp5150_registered(struct v4l2_subdev *sd)
> > -{
> > -#ifdef CONFIG_MEDIA_CONTROLLER
> > -	struct tvp5150 *decoder = to_tvp5150(sd);
> > -	int ret = 0;
> > -	int i;
> > -
> > -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> > -		struct media_entity *input = &decoder->input_ent[i];
> > -		struct media_pad *pad = &decoder->input_pad[i];
> > -
> > -		if (!input->name)
> > -			continue;
> > -
> > -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> > -
> > -		ret = media_entity_pads_init(input, 1, pad);
> > -		if (ret < 0)
> > -			return ret;
> > -
> > -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> > -		if (ret < 0)
> > -			return ret;
> > -
> > -		ret = media_create_pad_link(input, 0, &sd->entity,
> > -					    DEMOD_PAD_IF_INPUT, 0);
> > -		if (ret < 0) {
> > -			media_device_unregister_entity(input);
> > -			return ret;
> > -		}
> > -	}
> > -#endif
> 
> Hmm... I suspect that reverting this part may cause problems for drivers
> like em28xx when compiled with MC, as they rely that the supported demods
> will have 3 pads (DEMOD_NUM_PADS).

Please, can you test this for me? I have no such usb device.
Using the DEMOD_NUM_PADS looked wrong to me since the tvp5150 has more
than one input pad.

Thanks,
Marco

> Thanks,
> Mauro
>

Javier Martinez Canillas July 31, 2018, 8:52 a.m. UTC | #4

Hello Mauro,

On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 18:20:50 +0200
> Marco Felsch <[e-mail]> escreveu:
> 
>> From: Javier Martinez Canillas <[e-mail]>
>>
>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>> added input signals support for the tvp5150, but the approach was found
>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>
>> This left the driver with an undocumented (and wrong) DT parsing logic,
>> so lets get rid of this code as well until the input connectors support
>> is implemented properly.
>>
>> It's a partial revert due other patches added on top of mentioned commit
>> not allowing the commit to be reverted cleanly anymore. But all the code
>> related to the DT parsing logic and input entities creation are removed.
>>
>> Suggested-by: Laurent Pinchart <[e-mail]>
>> Signed-off-by: Javier Martinez Canillas <[e-mail]>
>> Acked-by: Laurent Pinchart <[e-mail]>
>> [[e-mail]: rm TVP5150_INPUT_NUM define]
>> Signed-off-by: Marco Felsch <[e-mail]>
>> ---
> 
> ...
> 
>> -static int tvp5150_registered(struct v4l2_subdev *sd)
>> -{
>> -#ifdef CONFIG_MEDIA_CONTROLLER
>> -	struct tvp5150 *decoder = to_tvp5150(sd);
>> -	int ret = 0;
>> -	int i;
>> -
>> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
>> -		struct media_entity *input = &decoder->input_ent[i];
>> -		struct media_pad *pad = &decoder->input_pad[i];
>> -
>> -		if (!input->name)
>> -			continue;
>> -
>> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
>> -
>> -		ret = media_entity_pads_init(input, 1, pad);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		ret = media_create_pad_link(input, 0, &sd->entity,
>> -					    DEMOD_PAD_IF_INPUT, 0);
>> -		if (ret < 0) {
>> -			media_device_unregister_entity(input);
>> -			return ret;
>> -		}
>> -	}
>> -#endif
> 
> Hmm... I suspect that reverting this part may cause problems for drivers
> like em28xx when compiled with MC, as they rely that the supported demods
> will have 3 pads (DEMOD_NUM_PADS).
>

I don't see how this change could affect em28xx and other drivers. The function
tvp5150_registered() being removed here, only register the media entity and add
a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
input connector as defined in the Device Tree file.

In other words, all the code removed by this patch is DT-only and isn't used by
any media driver that makes use of the tvp5151.

As mentioned in the commit message, this code has never been used (besides from
my testings) and should had been removed when the DT binding was reverted, but
for some reasons the first patch landed and the second didn't at the time.

> Thanks,
> Mauro
> 

Best regards,

Mauro Carvalho Chehab July 31, 2018, 10:06 a.m. UTC | #5

Em Tue, 31 Jul 2018 10:52:56 +0200
Javier Martinez Canillas <[e-mail]> escreveu:

> Hello Mauro,
> 
> On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Jun 2018 18:20:50 +0200
> > Marco Felsch <[e-mail]> escreveu:
> >   
> >> From: Javier Martinez Canillas <[e-mail]>
> >>
> >> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> >> added input signals support for the tvp5150, but the approach was found
> >> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> >> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> >>
> >> This left the driver with an undocumented (and wrong) DT parsing logic,
> >> so lets get rid of this code as well until the input connectors support
> >> is implemented properly.
> >>
> >> It's a partial revert due other patches added on top of mentioned commit
> >> not allowing the commit to be reverted cleanly anymore. But all the code
> >> related to the DT parsing logic and input entities creation are removed.
> >>
> >> Suggested-by: Laurent Pinchart <[e-mail]>
> >> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> >> Acked-by: Laurent Pinchart <[e-mail]>
> >> [[e-mail]: rm TVP5150_INPUT_NUM define]
> >> Signed-off-by: Marco Felsch <[e-mail]>
> >> ---  
> > 
> > ...
> >   
> >> -static int tvp5150_registered(struct v4l2_subdev *sd)
> >> -{
> >> -#ifdef CONFIG_MEDIA_CONTROLLER
> >> -	struct tvp5150 *decoder = to_tvp5150(sd);
> >> -	int ret = 0;
> >> -	int i;
> >> -
> >> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> >> -		struct media_entity *input = &decoder->input_ent[i];
> >> -		struct media_pad *pad = &decoder->input_pad[i];
> >> -
> >> -		if (!input->name)
> >> -			continue;
> >> -
> >> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> >> -
> >> -		ret = media_entity_pads_init(input, 1, pad);
> >> -		if (ret < 0)
> >> -			return ret;
> >> -
> >> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> >> -		if (ret < 0)
> >> -			return ret;
> >> -
> >> -		ret = media_create_pad_link(input, 0, &sd->entity,
> >> -					    DEMOD_PAD_IF_INPUT, 0);
> >> -		if (ret < 0) {
> >> -			media_device_unregister_entity(input);
> >> -			return ret;
> >> -		}
> >> -	}
> >> -#endif  
> > 
> > Hmm... I suspect that reverting this part may cause problems for drivers
> > like em28xx when compiled with MC, as they rely that the supported demods
> > will have 3 pads (DEMOD_NUM_PADS).
> >  
> 
> I don't see how this change could affect em28xx and other drivers. The function
> tvp5150_registered() being removed here, only register the media entity and add
> a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
> input connector as defined in the Device Tree file.
> 
> In other words, all the code removed by this patch is DT-only and isn't used by
> any media driver that makes use of the tvp5151.
> 
> As mentioned in the commit message, this code has never been used (besides from
> my testings) and should had been removed when the DT binding was reverted, but
> for some reasons the first patch landed and the second didn't at the time.

Short answer: 

Yeah, you're right. Yet, patch 19/22 will cause regressions.

Long answer:

That's easy enough to test.

Without this patch, a em28xx-based board (Terratec Grabster AV350) reports:

$ ./mc_nextgen_test -D
digraph board {
	rankdir=TB
	colorscheme=x11
	labelloc="t"
	label="Grabster AV 350
 driver:em28xx, bus: usb-0000:00:14.0-2
"
	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
	entity_1 [label="{{<pad_2> 0} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_3> 1 | <pad_4> 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
}

tvp5150 reports 3 pads (one input, two output pads), and media core
properly connects the source pads.

With patch 18/22, I got the same graph. So, yeah, applying this patch
won't cause regressions.

However, when we apply patch 19/22:

$ mc_nextgen_test -D
digraph board {
	rankdir=TB
	colorscheme=x11
	labelloc="t"
	label="Grabster AV 350
 driver:em28xx, bus: usb-0000:00:14.0-2
"
	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
}

The graph is not built correct, as it is linking tvp5150's input pads as
if they were output ones.

The problem is that now you need to teach drivers/media/v4l2-core/v4l2-mc.c
to do the proper wiring for tvp5150.

I suspect that fixing v4l2-mc for doing that is not hard, but it may
require changes at the other demods. Thankfully there aren't many
demod drivers, but such patch should be applied before patch 19/22.

In the specific case of demods that don't support sliced VBI (or
where sliced VBI is not coded), there should be just one source pad.

On demods with sliced VBI, there are actually two source pads,
although, for simplicity, maybe we could map them as just one.

If we map as just one source pad, it is probably easy to change the
code at v4l2-mc to do the right thing.

I'll do some tests here and try to code it.

Thanks,
Mauro

Javier Martinez Canillas July 31, 2018, 11:26 a.m. UTC | #6

Hi Mauro,

On 07/31/2018 12:06 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 31 Jul 2018 10:52:56 +0200
> Javier Martinez Canillas <[e-mail]> escreveu:
> 
>> Hello Mauro,
>>
>> On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
>>> Em Thu, 28 Jun 2018 18:20:50 +0200
>>> Marco Felsch <[e-mail]> escreveu:
>>>   
>>>> From: Javier Martinez Canillas <[e-mail]>
>>>>
>>>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>>>> added input signals support for the tvp5150, but the approach was found
>>>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>>>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>>>
>>>> This left the driver with an undocumented (and wrong) DT parsing logic,
>>>> so lets get rid of this code as well until the input connectors support
>>>> is implemented properly.
>>>>
>>>> It's a partial revert due other patches added on top of mentioned commit
>>>> not allowing the commit to be reverted cleanly anymore. But all the code
>>>> related to the DT parsing logic and input entities creation are removed.
>>>>
>>>> Suggested-by: Laurent Pinchart <[e-mail]>
>>>> Signed-off-by: Javier Martinez Canillas <[e-mail]>
>>>> Acked-by: Laurent Pinchart <[e-mail]>
>>>> [[e-mail]: rm TVP5150_INPUT_NUM define]
>>>> Signed-off-by: Marco Felsch <[e-mail]>
>>>> ---  
>>>
>>> ...
>>>   
>>>> -static int tvp5150_registered(struct v4l2_subdev *sd)
>>>> -{
>>>> -#ifdef CONFIG_MEDIA_CONTROLLER
>>>> -	struct tvp5150 *decoder = to_tvp5150(sd);
>>>> -	int ret = 0;
>>>> -	int i;
>>>> -
>>>> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
>>>> -		struct media_entity *input = &decoder->input_ent[i];
>>>> -		struct media_pad *pad = &decoder->input_pad[i];
>>>> -
>>>> -		if (!input->name)
>>>> -			continue;
>>>> -
>>>> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
>>>> -
>>>> -		ret = media_entity_pads_init(input, 1, pad);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -
>>>> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -
>>>> -		ret = media_create_pad_link(input, 0, &sd->entity,
>>>> -					    DEMOD_PAD_IF_INPUT, 0);
>>>> -		if (ret < 0) {
>>>> -			media_device_unregister_entity(input);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> -#endif  
>>>
>>> Hmm... I suspect that reverting this part may cause problems for drivers
>>> like em28xx when compiled with MC, as they rely that the supported demods
>>> will have 3 pads (DEMOD_NUM_PADS).
>>>  
>>
>> I don't see how this change could affect em28xx and other drivers. The function
>> tvp5150_registered() being removed here, only register the media entity and add
>> a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
>> input connector as defined in the Device Tree file.
>>
>> In other words, all the code removed by this patch is DT-only and isn't used by
>> any media driver that makes use of the tvp5151.
>>
>> As mentioned in the commit message, this code has never been used (besides from
>> my testings) and should had been removed when the DT binding was reverted, but
>> for some reasons the first patch landed and the second didn't at the time.
> 
> Short answer: 
> 
> Yeah, you're right. Yet, patch 19/22 will cause regressions.
>
> Long answer:
> 
> That's easy enough to test.
> 
> Without this patch, a em28xx-based board (Terratec Grabster AV350) reports:
> 
> $ ./mc_nextgen_test -D
> digraph board {
> 	rankdir=TB
> 	colorscheme=x11
> 	labelloc="t"
> 	label="Grabster AV 350
>  driver:em28xx, bus: usb-0000:00:14.0-2
> "
> 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> 	entity_1 [label="{{<pad_2> 0} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_3> 1 | <pad_4> 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> }
> 
> tvp5150 reports 3 pads (one input, two output pads), and media core
> properly connects the source pads.
> 
> With patch 18/22, I got the same graph. So, yeah, applying this patch
> won't cause regressions.
>

Yes, I didn't have time to review the other patches in the set yet. I was just
referring to patch 18/22 that it is really a standalone change and I've posted
it several times already. So I think that one is safe to merge.

> However, when we apply patch 19/22:
> 
> $ mc_nextgen_test -D
> digraph board {
> 	rankdir=TB
> 	colorscheme=x11
> 	labelloc="t"
> 	label="Grabster AV 350
>  driver:em28xx, bus: usb-0000:00:14.0-2
> "
> 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> 	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> }
> 
> The graph is not built correct, as it is linking tvp5150's input pads as
> if they were output ones.
> 
> The problem is that now you need to teach drivers/media/v4l2-core/v4l2-mc.c
> to do the proper wiring for tvp5150.
> 
> I suspect that fixing v4l2-mc for doing that is not hard, but it may
> require changes at the other demods. Thankfully there aren't many
> demod drivers, but such patch should be applied before patch 19/22.
> 
> In the specific case of demods that don't support sliced VBI (or
> where sliced VBI is not coded), there should be just one source pad.
> 
> On demods with sliced VBI, there are actually two source pads,
> although, for simplicity, maybe we could map them as just one.
> 
> If we map as just one source pad, it is probably easy to change the
> code at v4l2-mc to do the right thing.
> 
> I'll do some tests here and try to code it.
>

Yes, another thing that patch 19/22 should take into account is DTs that
don't have input connectors defined. So probably TVP5150_PORT_YOUT should
be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.

In other words, it should work both when input connectors are defined in
the DT and when these are not defined and only an output port is defined.

> Thanks,
> Mauro
> 

Best regards,

Marco Felsch July 31, 2018, 12:36 p.m. UTC | #7

Hi Javier,
Hi Mauro,

On 18-07-31 13:26, Javier Martinez Canillas wrote:
> Hi Mauro,
> 
> On 07/31/2018 12:06 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 31 Jul 2018 10:52:56 +0200
> > Javier Martinez Canillas <[e-mail]> escreveu:
> > 
> >> Hello Mauro,
> >>
> >> On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
> >>> Em Thu, 28 Jun 2018 18:20:50 +0200
> >>> Marco Felsch <[e-mail]> escreveu:
> >>>   
> >>>> From: Javier Martinez Canillas <[e-mail]>
> >>>>
> >>>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> >>>> added input signals support for the tvp5150, but the approach was found
> >>>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> >>>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> >>>>
> >>>> This left the driver with an undocumented (and wrong) DT parsing logic,
> >>>> so lets get rid of this code as well until the input connectors support
> >>>> is implemented properly.
> >>>>
> >>>> It's a partial revert due other patches added on top of mentioned commit
> >>>> not allowing the commit to be reverted cleanly anymore. But all the code
> >>>> related to the DT parsing logic and input entities creation are removed.
> >>>>
> >>>> Suggested-by: Laurent Pinchart <[e-mail]>
> >>>> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> >>>> Acked-by: Laurent Pinchart <[e-mail]>
> >>>> [[e-mail]: rm TVP5150_INPUT_NUM define]
> >>>> Signed-off-by: Marco Felsch <[e-mail]>
> >>>> ---  
> >>>
> >>> ...
> >>>   
> >>>> -static int tvp5150_registered(struct v4l2_subdev *sd)
> >>>> -{
> >>>> -#ifdef CONFIG_MEDIA_CONTROLLER
> >>>> -	struct tvp5150 *decoder = to_tvp5150(sd);
> >>>> -	int ret = 0;
> >>>> -	int i;
> >>>> -
> >>>> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> >>>> -		struct media_entity *input = &decoder->input_ent[i];
> >>>> -		struct media_pad *pad = &decoder->input_pad[i];
> >>>> -
> >>>> -		if (!input->name)
> >>>> -			continue;
> >>>> -
> >>>> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> >>>> -
> >>>> -		ret = media_entity_pads_init(input, 1, pad);
> >>>> -		if (ret < 0)
> >>>> -			return ret;
> >>>> -
> >>>> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> >>>> -		if (ret < 0)
> >>>> -			return ret;
> >>>> -
> >>>> -		ret = media_create_pad_link(input, 0, &sd->entity,
> >>>> -					    DEMOD_PAD_IF_INPUT, 0);
> >>>> -		if (ret < 0) {
> >>>> -			media_device_unregister_entity(input);
> >>>> -			return ret;
> >>>> -		}
> >>>> -	}
> >>>> -#endif  
> >>>
> >>> Hmm... I suspect that reverting this part may cause problems for drivers
> >>> like em28xx when compiled with MC, as they rely that the supported demods
> >>> will have 3 pads (DEMOD_NUM_PADS).
> >>>  
> >>
> >> I don't see how this change could affect em28xx and other drivers. The function
> >> tvp5150_registered() being removed here, only register the media entity and add
> >> a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
> >> input connector as defined in the Device Tree file.
> >>
> >> In other words, all the code removed by this patch is DT-only and isn't used by
> >> any media driver that makes use of the tvp5151.
> >>
> >> As mentioned in the commit message, this code has never been used (besides from
> >> my testings) and should had been removed when the DT binding was reverted, but
> >> for some reasons the first patch landed and the second didn't at the time.
> > 
> > Short answer: 
> > 
> > Yeah, you're right. Yet, patch 19/22 will cause regressions.
> >
> > Long answer:
> > 
> > That's easy enough to test.
> > 
> > Without this patch, a em28xx-based board (Terratec Grabster AV350) reports:
> > 
> > $ ./mc_nextgen_test -D
> > digraph board {
> > 	rankdir=TB
> > 	colorscheme=x11
> > 	labelloc="t"
> > 	label="Grabster AV 350
> >  driver:em28xx, bus: usb-0000:00:14.0-2
> > "
> > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > 	entity_1 [label="{{<pad_2> 0} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_3> 1 | <pad_4> 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> > 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > }
> > 
> > tvp5150 reports 3 pads (one input, two output pads), and media core
> > properly connects the source pads.
> > 
> > With patch 18/22, I got the same graph. So, yeah, applying this patch
> > won't cause regressions.
> >
> 
> Yes, I didn't have time to review the other patches in the set yet. I was just
> referring to patch 18/22 that it is really a standalone change and I've posted
> it several times already. So I think that one is safe to merge.
> 
> > However, when we apply patch 19/22:
> > 
> > $ mc_nextgen_test -D
> > digraph board {
> > 	rankdir=TB
> > 	colorscheme=x11
> > 	labelloc="t"
> > 	label="Grabster AV 350
> >  driver:em28xx, bus: usb-0000:00:14.0-2
> > "
> > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > 	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> > 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > }
> > 
> > The graph is not built correct, as it is linking tvp5150's input pads as
> > if they were output ones.

Maybe I misunderstand the mc-pads. I tought the pads represents the
physical ports. So I mapped these two things togehter.

> > 
> > The problem is that now you need to teach drivers/media/v4l2-core/v4l2-mc.c
> > to do the proper wiring for tvp5150.
> > 
> > I suspect that fixing v4l2-mc for doing that is not hard, but it may
> > require changes at the other demods. Thankfully there aren't many
> > demod drivers, but such patch should be applied before patch 19/22.
> > 
> > In the specific case of demods that don't support sliced VBI (or
> > where sliced VBI is not coded), there should be just one source pad.
> > 
> > On demods with sliced VBI, there are actually two source pads,
> > although, for simplicity, maybe we could map them as just one.
> > 
> > If we map as just one source pad, it is probably easy to change the
> > code at v4l2-mc to do the right thing.
> > 
> > I'll do some tests here and try to code it.
> >
> 
> Yes, another thing that patch 19/22 should take into account is DTs that
> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> 
> In other words, it should work both when input connectors are defined in
> the DT and when these are not defined and only an output port is defined.

Yes, it would be a approach to map the output port dynamicaly to the
highest port number. I tried to keep things easy by a static mapping.
Maybe a follow up patch can change this behaviour.

Anyway, input connectors aren't required. There must be at least one
port child node with a correct port-number in the DT.

Regards,
Marco

> > Thanks,
> > Mauro
> > 
> 
> Best regards,

Javier Martinez Canillas July 31, 2018, 12:52 p.m. UTC | #8

Hi Marco,

On 07/31/2018 02:36 PM, Marco Felsch wrote:

[snip]

>>
>> Yes, another thing that patch 19/22 should take into account is DTs that
>> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
>> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
>>
>> In other words, it should work both when input connectors are defined in
>> the DT and when these are not defined and only an output port is defined.
> 
> Yes, it would be a approach to map the output port dynamicaly to the
> highest port number. I tried to keep things easy by a static mapping.
> Maybe a follow up patch can change this behaviour.
> 
> Anyway, input connectors aren't required. There must be at least one
> port child node with a correct port-number in the DT.
>

Yes, that was my point. But your patch uses the port child reg property as
the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.

If there's only one port child (for the output) then the DT binding says
that the reg property isn't required, so this will be 0 and your patch will
wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
should be the first one in your enum tvp5150_ports and not the last one.

> Regards,
> Marco
> 

Best regards,

Mauro Carvalho Chehab July 31, 2018, 1:01 p.m. UTC | #9

Em Tue, 31 Jul 2018 14:36:52 +0200
Marco Felsch <[e-mail]> escreveu:

> Hi Javier,
> Hi Mauro,
> 
> On 18-07-31 13:26, Javier Martinez Canillas wrote:
> > Hi Mauro,
> > 
> > On 07/31/2018 12:06 PM, Mauro Carvalho Chehab wrote:  
> > > Em Tue, 31 Jul 2018 10:52:56 +0200
> > > Javier Martinez Canillas <[e-mail]> escreveu:
> > >   
> > >> Hello Mauro,
> > >>
> > >> On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:  
> > >>> Em Thu, 28 Jun 2018 18:20:50 +0200
> > >>> Marco Felsch <[e-mail]> escreveu:
> > >>>     
> > >>>> From: Javier Martinez Canillas <[e-mail]>
> > >>>>
> > >>>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> > >>>> added input signals support for the tvp5150, but the approach was found
> > >>>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> > >>>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> > >>>>
> > >>>> This left the driver with an undocumented (and wrong) DT parsing logic,
> > >>>> so lets get rid of this code as well until the input connectors support
> > >>>> is implemented properly.
> > >>>>
> > >>>> It's a partial revert due other patches added on top of mentioned commit
> > >>>> not allowing the commit to be reverted cleanly anymore. But all the code
> > >>>> related to the DT parsing logic and input entities creation are removed.
> > >>>>
> > >>>> Suggested-by: Laurent Pinchart <[e-mail]>
> > >>>> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> > >>>> Acked-by: Laurent Pinchart <[e-mail]>
> > >>>> [[e-mail]: rm TVP5150_INPUT_NUM define]
> > >>>> Signed-off-by: Marco Felsch <[e-mail]>
> > >>>> ---    
> > >>>
> > >>> ...
> > >>>     
> > >>>> -static int tvp5150_registered(struct v4l2_subdev *sd)
> > >>>> -{
> > >>>> -#ifdef CONFIG_MEDIA_CONTROLLER
> > >>>> -	struct tvp5150 *decoder = to_tvp5150(sd);
> > >>>> -	int ret = 0;
> > >>>> -	int i;
> > >>>> -
> > >>>> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> > >>>> -		struct media_entity *input = &decoder->input_ent[i];
> > >>>> -		struct media_pad *pad = &decoder->input_pad[i];
> > >>>> -
> > >>>> -		if (!input->name)
> > >>>> -			continue;
> > >>>> -
> > >>>> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> > >>>> -
> > >>>> -		ret = media_entity_pads_init(input, 1, pad);
> > >>>> -		if (ret < 0)
> > >>>> -			return ret;
> > >>>> -
> > >>>> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> > >>>> -		if (ret < 0)
> > >>>> -			return ret;
> > >>>> -
> > >>>> -		ret = media_create_pad_link(input, 0, &sd->entity,
> > >>>> -					    DEMOD_PAD_IF_INPUT, 0);
> > >>>> -		if (ret < 0) {
> > >>>> -			media_device_unregister_entity(input);
> > >>>> -			return ret;
> > >>>> -		}
> > >>>> -	}
> > >>>> -#endif    
> > >>>
> > >>> Hmm... I suspect that reverting this part may cause problems for drivers
> > >>> like em28xx when compiled with MC, as they rely that the supported demods
> > >>> will have 3 pads (DEMOD_NUM_PADS).
> > >>>    
> > >>
> > >> I don't see how this change could affect em28xx and other drivers. The function
> > >> tvp5150_registered() being removed here, only register the media entity and add
> > >> a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
> > >> input connector as defined in the Device Tree file.
> > >>
> > >> In other words, all the code removed by this patch is DT-only and isn't used by
> > >> any media driver that makes use of the tvp5151.
> > >>
> > >> As mentioned in the commit message, this code has never been used (besides from
> > >> my testings) and should had been removed when the DT binding was reverted, but
> > >> for some reasons the first patch landed and the second didn't at the time.  
> > > 
> > > Short answer: 
> > > 
> > > Yeah, you're right. Yet, patch 19/22 will cause regressions.
> > >
> > > Long answer:
> > > 
> > > That's easy enough to test.
> > > 
> > > Without this patch, a em28xx-based board (Terratec Grabster AV350) reports:
> > > 
> > > $ ./mc_nextgen_test -D
> > > digraph board {
> > > 	rankdir=TB
> > > 	colorscheme=x11
> > > 	labelloc="t"
> > > 	label="Grabster AV 350
> > >  driver:em28xx, bus: usb-0000:00:14.0-2
> > > "
> > > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > > 	entity_1 [label="{{<pad_2> 0} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_3> 1 | <pad_4> 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > > 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> > > 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> > > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > > }
> > > 
> > > tvp5150 reports 3 pads (one input, two output pads), and media core
> > > properly connects the source pads.
> > > 
> > > With patch 18/22, I got the same graph. So, yeah, applying this patch
> > > won't cause regressions.
> > >  
> > 
> > Yes, I didn't have time to review the other patches in the set yet. I was just
> > referring to patch 18/22 that it is really a standalone change and I've posted
> > it several times already. So I think that one is safe to merge.
> >   
> > > However, when we apply patch 19/22:
> > > 
> > > $ mc_nextgen_test -D
> > > digraph board {
> > > 	rankdir=TB
> > > 	colorscheme=x11
> > > 	labelloc="t"
> > > 	label="Grabster AV 350
> > >  driver:em28xx, bus: usb-0000:00:14.0-2
> > > "
> > > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > > 	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > > 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> > > 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> > > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > > }
> > > 
> > > The graph is not built correct, as it is linking tvp5150's input pads as
> > > if they were output ones.  
> 
> Maybe I misunderstand the mc-pads. I tought the pads represents the
> physical ports. So I mapped these two things togehter.

We had a long discussion about that on IRC and at the ML some time ago.

There are different ways to map it, and different opinions if either a
PAD is a physical or a logical port.

In the case of tvp5150, from the logical standpoint, there's just one
input port (as it can't stream from multiple ports at the same time)
and two physical ports (AIP1A and AIP1B).

Internally, tvp5150 has a switch that allows 3 possible configurations:
	- composite 0 - switching to AIP1A
	- composite 1 - switching to AIP1B
	- s-video - using both AIP1A and AIP1B

So, depending on the way you see, it may have 1, 2 or 3 pads.

We ended by mapping it to just 1 pad. The idea is that, when a link is
created from a connector to it, it will set the input switch.

We did a mistake at the mapping, as VBI and video is actually a single
output pad, with can be connected to two different entities: one that
does the video stream and the other one that filters just some rows of
the video, in order to stream it trough the vbi interface.

If we ever implement a sliced VBI interface - with is now possible with
the interrupt handler - then we'll have a second output pad with sliced
VBI output.

> > > 
> > > The problem is that now you need to teach drivers/media/v4l2-core/v4l2-mc.c
> > > to do the proper wiring for tvp5150.
> > > 
> > > I suspect that fixing v4l2-mc for doing that is not hard, but it may
> > > require changes at the other demods. Thankfully there aren't many
> > > demod drivers, but such patch should be applied before patch 19/22.
> > > 
> > > In the specific case of demods that don't support sliced VBI (or
> > > where sliced VBI is not coded), there should be just one source pad.
> > > 
> > > On demods with sliced VBI, there are actually two source pads,
> > > although, for simplicity, maybe we could map them as just one.
> > > 
> > > If we map as just one source pad, it is probably easy to change the
> > > code at v4l2-mc to do the right thing.
> > > 
> > > I'll do some tests here and try to code it.
> > >  
> > 
> > Yes, another thing that patch 19/22 should take into account is DTs that
> > don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> > be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> > 
> > In other words, it should work both when input connectors are defined in
> > the DT and when these are not defined and only an output port is defined.  
> 
> Yes, it would be a approach to map the output port dynamicaly to the
> highest port number. I tried to keep things easy by a static mapping.
> Maybe a follow up patch can change this behaviour.
> 
> Anyway, input connectors aren't required. There must be at least one
> port child node with a correct port-number in the DT.

If we want to switch the input connector via MC, then it is required.

There are some OMAP3-based boards with 2 composite inputs and tvp5151
(with is almost identical, from software standpoint, to tvp5150).

Thanks,
Mauro

Mauro Carvalho Chehab July 31, 2018, 1:22 p.m. UTC | #10

Em Tue, 31 Jul 2018 07:06:59 -0300
Mauro Carvalho Chehab <[e-mail]> escreveu:

> Em Tue, 31 Jul 2018 10:52:56 +0200
> Javier Martinez Canillas <[e-mail]> escreveu:
> 

> The graph is not built correct, as it is linking tvp5150's input pads as
> if they were output ones.
> 
> The problem is that now you need to teach drivers/media/v4l2-core/v4l2-mc.c
> to do the proper wiring for tvp5150.
> 
> I suspect that fixing v4l2-mc for doing that is not hard, but it may
> require changes at the other demods. Thankfully there aren't many
> demod drivers, but such patch should be applied before patch 19/22.
> 
> In the specific case of demods that don't support sliced VBI (or
> where sliced VBI is not coded), there should be just one source pad.
> 
> On demods with sliced VBI, there are actually two source pads,
> although, for simplicity, maybe we could map them as just one.
> 
> If we map as just one source pad, it is probably easy to change the
> code at v4l2-mc to do the right thing.
> 
> I'll do some tests here and try to code it.

Ok, did some coding. The way to make it more robust and allow having
a different number of PADs for different demods/tuners is with an
approach like the one below.

This is just a RFC sort of patch, as it is incomplete, not covering the
dvbdev.c pipeline setup logic.

Anyway, it should be useful for further discussions, but some work
is needed.

Regards,
Mauro


[RFC] media: v4l2: taint pads with the signal types for consumer devices

Consumer devices are provided with a wide diferent range of types
supported by the same driver, allowing different configutations.

In order to make easier to setup media controller links, "taint"
pads with the signal type it carries.

While here, get rid of DEMOD_PAD_VBI_OUT, as the signal it carries
is actually the same as the normal video output.

The difference happens at the video/VBI interface:
        - for VBI, only the hidden lines are streamed;
        - for video, the stream is usually cropped to hide the
          vbi lines.

Compile-tested only and incomplete: the dvbdev.c should have a similar
change like the one done at v4l2-mc.c.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

diff --git a/drivers/media/dvb-frontends/au8522_decoder.c b/drivers/media/dvb-frontends/au8522_decoder.c
index 343dc92ef54e..f4df9ab3d8b0 100644
--- a/drivers/media/dvb-frontends/au8522_decoder.c
+++ b/drivers/media/dvb-frontends/au8522_decoder.c
@@ -721,9 +721,11 @@ static int au8522_probe(struct i2c_client *client,
 #if defined(CONFIG_MEDIA_CONTROLLER)
 
 	state->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	state->pads[DEMOD_PAD_IF_INPUT].sig_type = PAD_SIGNAL_RF;
 	state->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	state->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	state->pads[DEMOD_PAD_VID_OUT].sig_type = PAD_SIGNAL_ATV_VIDEO;
 	state->pads[DEMOD_PAD_AUDIO_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	state->pads[DEMOD_PAD_AUDIO_OUT].sig_type = PAD_SIGNAL_AUDIO;
 	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
 
 	ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(state->pads),
diff --git a/drivers/media/i2c/msp3400-driver.c b/drivers/media/i2c/msp3400-driver.c
index 3db966db83eb..3b9c729fbd52 100644
--- a/drivers/media/i2c/msp3400-driver.c
+++ b/drivers/media/i2c/msp3400-driver.c
@@ -704,7 +704,9 @@ static int msp_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	state->pads[IF_AUD_DEC_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	state->pads[IF_AUD_DEC_PAD_IF_INPUT].sig_type = PAD_SIGNAL_AUDIO;
 	state->pads[IF_AUD_DEC_PAD_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	state->pads[IF_AUD_DEC_PAD_OUT].sig_type = PAD_SIGNAL_AUDIO;
 
 	sd->entity.function = MEDIA_ENT_F_IF_AUD_DECODER;
 
diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index b07114b5efb2..0b298aa34a7c 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -1835,8 +1835,9 @@ static int saa711x_probe(struct i2c_client *client,
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	state->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	state->pads[DEMOD_PAD_IF_INPUT].sig_type = PAD_SIGNAL_RF;
 	state->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	state->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	state->pads[DEMOD_PAD_VID_OUT].sig_type = PAD_SIGNAL_ATV_VIDEO;
 
 	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
 
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 1734ed4ede33..dab83a774e73 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1495,8 +1495,9 @@ static int tvp5150_probe(struct i2c_client *c,
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	core->pads[DEMOD_PAD_IF_INPUT].sig_type = PAD_SIGNAL_RF;
 	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	core->pads[DEMOD_PAD_VID_OUT].sig_type = PAD_SIGNAL_ATV_VIDEO;
 
 	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
 
diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
index 9e76de2411ae..322e2ac00066 100644
--- a/drivers/media/pci/saa7134/saa7134-core.c
+++ b/drivers/media/pci/saa7134/saa7134-core.c
@@ -846,8 +846,9 @@ static void saa7134_create_entities(struct saa7134_dev *dev)
 	if (!decoder) {
 		dev->demod.name = "saa713x";
 		dev->demod_pad[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+		dev->demod_pad[DEMOD_PAD_IF_INPUT].sig_type = PAD_SIGNAL_RF;
 		dev->demod_pad[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-		dev->demod_pad[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
+		dev->demod_pad[DEMOD_PAD_VID_OUT].sig_type = PAD_SIGNAL_ATV_VIDEO;
 		dev->demod.function = MEDIA_ENT_F_ATV_DECODER;
 
 		ret = media_entity_pads_init(&dev->demod, DEMOD_NUM_PADS,
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 9e34d31d724d..85e9ea9059a3 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -469,8 +469,11 @@ static int si2157_probe(struct i2c_client *client,
 		dev->ent.function = MEDIA_ENT_F_TUNER;
 
 		dev->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
+		dev->pad[TUNER_PAD_RF_INPUT].sig_type = PAD_SIGNAL_RF;
 		dev->pad[TUNER_PAD_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
+		dev->pad[TUNER_PAD_OUTPUT].sig_type = PAD_SIGNAL_CARRIERS;
 		dev->pad[TUNER_PAD_AUD_OUT].flags = MEDIA_PAD_FL_SOURCE;
+		dev->pad[TUNER_PAD_AUD_OUT].sig_type = PAD_SIGNAL_AUDIO;
 
 		ret = media_entity_pads_init(&dev->ent, TUNER_NUM_PADS,
 					     &dev->pad[0]);
diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
index 67953360fda5..9161064b7718 100644
--- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
+++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
@@ -893,7 +893,9 @@ static int mxl111sf_attach_tuner(struct dvb_usb_adapter *adap)
 	state->tuner.function = MEDIA_ENT_F_TUNER;
 	state->tuner.name = "mxl111sf tuner";
 	state->tuner_pads[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	state->tuner_pads[TUNER_PAD_RF_INPUT].sig_type = PAD_SIGNAL_RF;
 	state->tuner_pads[TUNER_PAD_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
+	state->tuner_pads[TUNER_PAD_OUTPUT].sig_type = PAD_SIGNAL_CARRIERS;
 
 	ret = media_entity_pads_init(&state->tuner,
 				     TUNER_NUM_PADS, state->tuner_pads);
diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index 7f858c39753c..4c09c30e6ea1 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -685,15 +685,20 @@ static int tuner_probe(struct i2c_client *client,
 	 */
 	if (t->type == TUNER_TDA9887) {
 		t->pad[IF_VID_DEC_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+		t->pad[IF_VID_DEC_PAD_IF_INPUT].sig_type = PAD_SIGNAL_RF;
 		t->pad[IF_VID_DEC_PAD_OUT].flags = MEDIA_PAD_FL_SOURCE;
+		t->pad[IF_VID_DEC_PAD_OUT].sig_type = PAD_SIGNAL_ATV_VIDEO;
 		ret = media_entity_pads_init(&t->sd.entity,
 					     IF_VID_DEC_PAD_NUM_PADS,
 					     &t->pad[0]);
 		t->sd.entity.function = MEDIA_ENT_F_IF_VID_DECODER;
 	} else {
 		t->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
+		t->pad[TUNER_PAD_RF_INPUT].sig_type = PAD_SIGNAL_RF;
 		t->pad[TUNER_PAD_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
+		t->pad[TUNER_PAD_OUTPUT].sig_type = PAD_SIGNAL_CARRIERS;
 		t->pad[TUNER_PAD_AUD_OUT].flags = MEDIA_PAD_FL_SOURCE;
+		t->pad[TUNER_PAD_AUD_OUT].sig_type = PAD_SIGNAL_AUDIO;
 		ret = media_entity_pads_init(&t->sd.entity, TUNER_NUM_PADS,
 					     &t->pad[0]);
 		t->sd.entity.function = MEDIA_ENT_F_TUNER;
diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index 0fc185a2ce90..982bab3530f6 100644
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -147,7 +147,7 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
 	}
 
 	if (io_vbi) {
-		ret = media_create_pad_link(decoder, DEMOD_PAD_VBI_OUT,
+		ret = media_create_pad_link(decoder, DEMOD_PAD_VID_OUT,
 					    io_vbi, 0,
 					    MEDIA_LNK_FL_ENABLED);
 		if (ret)
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index a732af1dbba0..bf0604d315ef 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -155,6 +155,38 @@ struct media_link {
 	bool is_backlink;
 };
 
+/**
+ * struct media_pad_signal_type - type of the signal inside a media pad
+ *
+ * @PAD_SIGNAL_DEFAULT
+ *	Default signal. Use this when all inputs or all outputs are
+ *	uniquely identified by just its number and all carries the same
+ *	signal type
+ * @PAD_SIGNAL_RF
+ *	The pad contains a Radio Frequency, Intermediate Frequency or
+ *	baseband signal.
+ *	All Tuner sinks should use it.
+ *	On tuner sources, this is used for digital TV demodulators and for
+ *	IF-PLL demodulator like tda9887.
+ * @PAD_SIGNAL_CARRIERS
+ *	The pad contains analog signals carrying either a digital or an analog
+ *	modulated (or baseband) signal. This is provided by tuner source
+ *	pads and used by analog TV standard decoders and by digital tv demods.
+ * @PAD_SIGNAL_ATV_VIDEO
+ *	Contains a bitstream of samples from an analog TV video source, with
+ *	usually contains the VBI data on it.
+ * @PAD_SIGNAL_AUDIO
+ *	Contains an Intermediate Frequency analog signal from an audio
+ *	sub-carrier or an audio bitstream. Provided by tuners and consumed by audio AM/FM decoders.
+ */
+enum media_pad_signal_type {
+	PAD_SIGNAL_DEFAULT = 0,
+	PAD_SIGNAL_RF,
+	PAD_SIGNAL_CARRIERS,
+	PAD_SIGNAL_ATV_VIDEO,
+	PAD_SIGNAL_AUDIO,
+};
+
 /**
  * struct media_pad - A media pad graph object.
  *
@@ -169,6 +201,7 @@ struct media_pad {
 	struct media_gobj graph_obj;	/* must be first field in struct */
 	struct media_entity *entity;
 	u16 index;
+	enum media_pad_signal_type sig_type;
 	unsigned long flags;
 };
 
diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
index 2634d9dc9916..7c9c781b16a9 100644
--- a/include/media/v4l2-mc.h
+++ b/include/media/v4l2-mc.h
@@ -89,14 +89,12 @@ enum if_aud_dec_pad_index {
  *
  * @DEMOD_PAD_IF_INPUT:	IF input sink pad.
  * @DEMOD_PAD_VID_OUT:	Video output source pad.
- * @DEMOD_PAD_VBI_OUT:	Vertical Blank Interface (VBI) output source pad.
  * @DEMOD_PAD_AUDIO_OUT: Audio output source pad.
  * @DEMOD_NUM_PADS:	Maximum number of output pads.
  */
 enum demod_pad_index {
 	DEMOD_PAD_IF_INPUT,
 	DEMOD_PAD_VID_OUT,
-	DEMOD_PAD_VBI_OUT,
 	DEMOD_PAD_AUDIO_OUT,
 	DEMOD_NUM_PADS
 };





Thanks,
Mauro

Marco Felsch July 31, 2018, 1:30 p.m. UTC | #11

Hi Javier,

On 18-07-31 14:52, Javier Martinez Canillas wrote:
> Hi Marco,
> 
> On 07/31/2018 02:36 PM, Marco Felsch wrote:
> 
> [snip]
> 
> >>
> >> Yes, another thing that patch 19/22 should take into account is DTs that
> >> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> >> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> >>
> >> In other words, it should work both when input connectors are defined in
> >> the DT and when these are not defined and only an output port is defined.
> > 
> > Yes, it would be a approach to map the output port dynamicaly to the
> > highest port number. I tried to keep things easy by a static mapping.
> > Maybe a follow up patch can change this behaviour.
> > 
> > Anyway, input connectors aren't required. There must be at least one
> > port child node with a correct port-number in the DT.
> >
> 
> Yes, that was my point. But your patch uses the port child reg property as
> the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
> 
> If there's only one port child (for the output) then the DT binding says
> that the reg property isn't required, so this will be 0 and your patch will
> wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
> should be the first one in your enum tvp5150_ports and not the last one.

Yes, now I got you. I implemted this in such a way in my first apporach.
But at the moment I don't know why I changed this. Maybe to keep the
decoder->input number in sync with the em28xx devices, which will set the
port by the s_routing() callback.

Let me check this.

Best Regards,
Marco

> > Regards,
> > Marco
> > 
> 
> Best regards,

Mauro Carvalho Chehab July 31, 2018, 7:56 p.m. UTC | #12

Em Tue, 31 Jul 2018 15:30:56 +0200
Marco Felsch <[e-mail]> escreveu:

> Hi Javier,
> 
> On 18-07-31 14:52, Javier Martinez Canillas wrote:
> > Hi Marco,
> > 
> > On 07/31/2018 02:36 PM, Marco Felsch wrote:
> > 
> > [snip]
> >   
> > >>
> > >> Yes, another thing that patch 19/22 should take into account is DTs that
> > >> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> > >> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> > >>
> > >> In other words, it should work both when input connectors are defined in
> > >> the DT and when these are not defined and only an output port is defined.  
> > > 
> > > Yes, it would be a approach to map the output port dynamicaly to the
> > > highest port number. I tried to keep things easy by a static mapping.
> > > Maybe a follow up patch can change this behaviour.
> > > 
> > > Anyway, input connectors aren't required. There must be at least one
> > > port child node with a correct port-number in the DT.
> > >  
> > 
> > Yes, that was my point. But your patch uses the port child reg property as
> > the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
> > 
> > If there's only one port child (for the output) then the DT binding says
> > that the reg property isn't required, so this will be 0 and your patch will
> > wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
> > should be the first one in your enum tvp5150_ports and not the last one.  
> 
> Yes, now I got you. I implemted this in such a way in my first apporach.
> But at the moment I don't know why I changed this. Maybe to keep the
> decoder->input number in sync with the em28xx devices, which will set the
> port by the s_routing() callback.
> 
> Let me check this.

Anyway, with the patchset I sent (with one fix), it will do the right
thing with regards to the pad output:
	https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150


$ mc_nextgen_test -D 
digraph board {
	rankdir=TB
	colorscheme=x11
	labelloc="t"
	label="Grabster AV 350
 driver:em28xx, bus: usb-0000:00:14.0-2
"
	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
	entity_1:pad_5 -> entity_6:pad_12 [color=blue]
	entity_1:pad_5 -> entity_9:pad_13 [color=blue]
	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
}

It won't do the right thing with regards to the input, though, as
the code at v4l2-mc.c expects just one input. So, both composite and
S-Video connectors (created outside tvp5150, based on the input entries
at em28xx cards table) are linked to pad 0. 

Thanks,
Mauro

Marco Felsch Aug. 1, 2018, 12:10 p.m. UTC | #13

Hi Mauro,

On 18-07-31 16:56, Mauro Carvalho Chehab wrote:
> Em Tue, 31 Jul 2018 15:30:56 +0200
> Marco Felsch <[e-mail]> escreveu:
> 
> > Hi Javier,
> > 
> > On 18-07-31 14:52, Javier Martinez Canillas wrote:
> > > Hi Marco,
> > > 
> > > On 07/31/2018 02:36 PM, Marco Felsch wrote:
> > > 
> > > [snip]
> > >   
> > > >>
> > > >> Yes, another thing that patch 19/22 should take into account is DTs that
> > > >> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> > > >> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> > > >>
> > > >> In other words, it should work both when input connectors are defined in
> > > >> the DT and when these are not defined and only an output port is defined.  
> > > > 
> > > > Yes, it would be a approach to map the output port dynamicaly to the
> > > > highest port number. I tried to keep things easy by a static mapping.
> > > > Maybe a follow up patch can change this behaviour.
> > > > 
> > > > Anyway, input connectors aren't required. There must be at least one
> > > > port child node with a correct port-number in the DT.
> > > >  
> > > 
> > > Yes, that was my point. But your patch uses the port child reg property as
> > > the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
> > > 
> > > If there's only one port child (for the output) then the DT binding says
> > > that the reg property isn't required, so this will be 0 and your patch will
> > > wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
> > > should be the first one in your enum tvp5150_ports and not the last one.  
> > 
> > Yes, now I got you. I implemted this in such a way in my first apporach.
> > But at the moment I don't know why I changed this. Maybe to keep the
> > decoder->input number in sync with the em28xx devices, which will set the
> > port by the s_routing() callback.
> > 
> > Let me check this.

I will prepare a follow up patch wich fix this behaviour, if possible.

> 
> Anyway, with the patchset I sent (with one fix), it will do the right
> thing with regards to the pad output:
> 	https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150

Thanks for your work :)
I have just one question. Is it correct to set the .sig_type only for the
tvp5150 'main' entity or should it be set for the dynamical connector
entities too?

> 
> $ mc_nextgen_test -D 
> digraph board {
> 	rankdir=TB
> 	colorscheme=x11
> 	labelloc="t"
> 	label="Grabster AV 350
>  driver:em28xx, bus: usb-0000:00:14.0-2
> "
> 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> 	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> 	entity_1:pad_5 -> entity_6:pad_12 [color=blue]
> 	entity_1:pad_5 -> entity_9:pad_13 [color=blue]
> 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> }
> 
> It won't do the right thing with regards to the input, though, as
> the code at v4l2-mc.c expects just one input. So, both composite and
> S-Video connectors (created outside tvp5150, based on the input entries
> at em28xx cards table) are linked to pad 0. 

Should we add comment for this behaviour in v4l2-mc.c? Since the
MEDIA_ENT_F_CONN_RF case updates the pad number.

> 
> Thanks,
> Mauro
> 

Regards,
Marco

Mauro Carvalho Chehab Aug. 1, 2018, 1:32 p.m. UTC | #14

Em Wed, 1 Aug 2018 14:10:47 +0200
Marco Felsch <[e-mail]> escreveu:

> Hi Mauro,
> 
> On 18-07-31 16:56, Mauro Carvalho Chehab wrote:
> > Em Tue, 31 Jul 2018 15:30:56 +0200
> > Marco Felsch <[e-mail]> escreveu:
> >   
> > > Hi Javier,
> > > 
> > > On 18-07-31 14:52, Javier Martinez Canillas wrote:  
> > > > Hi Marco,
> > > > 
> > > > On 07/31/2018 02:36 PM, Marco Felsch wrote:
> > > > 
> > > > [snip]
> > > >     
> > > > >>
> > > > >> Yes, another thing that patch 19/22 should take into account is DTs that
> > > > >> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> > > > >> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> > > > >>
> > > > >> In other words, it should work both when input connectors are defined in
> > > > >> the DT and when these are not defined and only an output port is defined.    
> > > > > 
> > > > > Yes, it would be a approach to map the output port dynamicaly to the
> > > > > highest port number. I tried to keep things easy by a static mapping.
> > > > > Maybe a follow up patch can change this behaviour.
> > > > > 
> > > > > Anyway, input connectors aren't required. There must be at least one
> > > > > port child node with a correct port-number in the DT.
> > > > >    
> > > > 
> > > > Yes, that was my point. But your patch uses the port child reg property as
> > > > the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
> > > > 
> > > > If there's only one port child (for the output) then the DT binding says
> > > > that the reg property isn't required, so this will be 0 and your patch will
> > > > wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
> > > > should be the first one in your enum tvp5150_ports and not the last one.    
> > > 
> > > Yes, now I got you. I implemted this in such a way in my first apporach.
> > > But at the moment I don't know why I changed this. Maybe to keep the
> > > decoder->input number in sync with the em28xx devices, which will set the
> > > port by the s_routing() callback.
> > > 
> > > Let me check this.  
> 
> I will prepare a follow up patch wich fix this behaviour, if possible.
> 
> > 
> > Anyway, with the patchset I sent (with one fix), it will do the right
> > thing with regards to the pad output:
> > 	https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150  
> 
> Thanks for your work :)
> I have just one question. Is it correct to set the .sig_type only for the
> tvp5150 'main' entity or should it be set for the dynamical connector
> entities too?

It should be needed only for entities with multiple pads, when
the pad index is not enough to uniquely identify what's there at
the pad. I don't think this applies to typical connectors
(although it might make sense on HDMI, where it may contain
different signals besides video, like CEC and ethernet).

> 
> > 
> > $ mc_nextgen_test -D 
> > digraph board {
> > 	rankdir=TB
> > 	colorscheme=x11
> > 	labelloc="t"
> > 	label="Grabster AV 350
> >  driver:em28xx, bus: usb-0000:00:14.0-2
> > "
> > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > 	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > 	entity_1:pad_5 -> entity_6:pad_12 [color=blue]
> > 	entity_1:pad_5 -> entity_9:pad_13 [color=blue]
> > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > }
> > 
> > It won't do the right thing with regards to the input, though, as
> > the code at v4l2-mc.c expects just one input. So, both composite and
> > S-Video connectors (created outside tvp5150, based on the input entries
> > at em28xx cards table) are linked to pad 0.   
> 
> Should we add comment for this behaviour in v4l2-mc.c? Since the
> MEDIA_ENT_F_CONN_RF case updates the pad number.

I don't think so... The stuff at v4l2-mc are there to help setting the
pipelines for devnode-based devices that also exposes their internal
wiring via MC. For those, it is up to the Kernel to create and manage
the pipelines. It is not used by MC-based devices, as, for those, 
the pipelines should be created by userspace via the MC device node.

Thanks,
Mauro

Marco Felsch Aug. 1, 2018, 3:49 p.m. UTC | #15

Hi Javier,

On 18-07-31 15:30, Marco Felsch wrote:
> Hi Javier,
> 
> On 18-07-31 14:52, Javier Martinez Canillas wrote:
> > Hi Marco,
> > 
> > On 07/31/2018 02:36 PM, Marco Felsch wrote:
> > 
> > [snip]
> > 
> > >>
> > >> Yes, another thing that patch 19/22 should take into account is DTs that
> > >> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> > >> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> > >>
> > >> In other words, it should work both when input connectors are defined in
> > >> the DT and when these are not defined and only an output port is defined.
> > > 
> > > Yes, it would be a approach to map the output port dynamicaly to the
> > > highest port number. I tried to keep things easy by a static mapping.
> > > Maybe a follow up patch can change this behaviour.
> > > 
> > > Anyway, input connectors aren't required. There must be at least one
> > > port child node with a correct port-number in the DT.
> > >
> > 
> > Yes, that was my point. But your patch uses the port child reg property as
> > the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
> > 
> > If there's only one port child (for the output) then the DT binding says
> > that the reg property isn't required, so this will be 0 and your patch will
> > wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
> > should be the first one in your enum tvp5150_ports and not the last one.
> 
> Yes, now I got you. I implemted this in such a way in my first apporach.
> But at the moment I don't know why I changed this. Maybe to keep the
> decoder->input number in sync with the em28xx devices, which will set the
> port by the s_routing() callback.
> 
> Let me check this.

I checked it again. Your're right, it should be doable but IMHO it isn't
the right solution. I checked some drivers which use of_graph and all of
them put the output at the end. So the tvp5150 will be the only one
which maps the out put to the first pad and it isn't intuitive.

I discused it with a colleague. We think a better solution would be to fix
the v4l2-core parser code to allow a independent dt-port<->pad mapping.
Since now the pad's correspond to the port number. This mapping should
be done by a driver callback, so each driver can do it's own custom
mapping.

Regards,
Marco

> 
> Best Regards,
> Marco
> 
> > > Regards,
> > > Marco
> > > 
> > 
> > Best regards,
>

Javier Martinez Canillas Aug. 1, 2018, 4:23 p.m. UTC | #16

Hi Marco,

On 08/01/2018 05:49 PM, Marco Felsch wrote:
> Hi Javier,
> 
> On 18-07-31 15:30, Marco Felsch wrote:
>> Hi Javier,
>>
>> On 18-07-31 14:52, Javier Martinez Canillas wrote:
>>> Hi Marco,
>>>
>>> On 07/31/2018 02:36 PM, Marco Felsch wrote:
>>>
>>> [snip]
>>>
>>>>>
>>>>> Yes, another thing that patch 19/22 should take into account is DTs that
>>>>> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
>>>>> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
>>>>>
>>>>> In other words, it should work both when input connectors are defined in
>>>>> the DT and when these are not defined and only an output port is defined.
>>>>
>>>> Yes, it would be a approach to map the output port dynamicaly to the
>>>> highest port number. I tried to keep things easy by a static mapping.
>>>> Maybe a follow up patch can change this behaviour.
>>>>
>>>> Anyway, input connectors aren't required. There must be at least one
>>>> port child node with a correct port-number in the DT.
>>>>
>>>
>>> Yes, that was my point. But your patch uses the port child reg property as
>>> the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
>>>
>>> If there's only one port child (for the output) then the DT binding says
>>> that the reg property isn't required, so this will be 0 and your patch will
>>> wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
>>> should be the first one in your enum tvp5150_ports and not the last one.
>>
>> Yes, now I got you. I implemted this in such a way in my first apporach.
>> But at the moment I don't know why I changed this. Maybe to keep the
>> decoder->input number in sync with the em28xx devices, which will set the
>> port by the s_routing() callback.
>>
>> Let me check this.
> 
> I checked it again. Your're right, it should be doable but IMHO it isn't
> the right solution. I checked some drivers which use of_graph and all of
> them put the output at the end. So the tvp5150 will be the only one
> which maps the out put to the first pad and it isn't intuitive.
> 

Ah, I didn't notice that the tvp5150 was the exception. I just mentioned due
DT maintainers always ask for driver changes to be DTB backward compatible.

> I discused it with a colleague. We think a better solution would be to fix
> the v4l2-core parser code to allow a independent dt-port<->pad mapping.
> Since now the pad's correspond to the port number. This mapping should
> be done by a driver callback, so each driver can do it's own custom
> mapping.
>

That sounds good to me.
 
> Regards,
> Marco
> 

Best regards,

Patch

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 3b51b6b67736..a6fec569a610 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -48,8 +48,6 @@  struct tvp5150 {
 	struct v4l2_subdev sd;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pads[DEMOD_NUM_PADS];
-	struct media_entity input_ent[TVP5150_INPUT_NUM];
-	struct media_pad input_pad[TVP5150_INPUT_NUM];
 #endif
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
@@ -1191,40 +1189,6 @@  static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-/****************************************************************************
-			Media entity ops
- ****************************************************************************/
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-static int tvp5150_link_setup(struct media_entity *entity,
-			      const struct media_pad *local,
-			      const struct media_pad *remote, u32 flags)
-{
-	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
-	struct tvp5150 *decoder = to_tvp5150(sd);
-	int i;
-
-	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-		if (remote->entity == &decoder->input_ent[i])
-			break;
-	}
-
-	/* Do nothing for entities that are not input connectors */
-	if (i == TVP5150_INPUT_NUM)
-		return 0;
-
-	decoder->input = i;
-
-	tvp5150_selmux(sd);
-
-	return 0;
-}
-
-static const struct media_entity_operations tvp5150_sd_media_ops = {
-	.link_setup = tvp5150_link_setup,
-};
-#endif
-
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
@@ -1369,42 +1333,6 @@  static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	return 0;
 }
 
-static int tvp5150_registered(struct v4l2_subdev *sd)
-{
-#ifdef CONFIG_MEDIA_CONTROLLER
-	struct tvp5150 *decoder = to_tvp5150(sd);
-	int ret = 0;
-	int i;
-
-	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-		struct media_entity *input = &decoder->input_ent[i];
-		struct media_pad *pad = &decoder->input_pad[i];
-
-		if (!input->name)
-			continue;
-
-		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
-
-		ret = media_entity_pads_init(input, 1, pad);
-		if (ret < 0)
-			return ret;
-
-		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
-		if (ret < 0)
-			return ret;
-
-		ret = media_create_pad_link(input, 0, &sd->entity,
-					    DEMOD_PAD_IF_INPUT, 0);
-		if (ret < 0) {
-			media_device_unregister_entity(input);
-			return ret;
-		}
-	}
-#endif
-
-	return 0;
-}
-
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1458,10 +1386,6 @@  static const struct v4l2_subdev_ops tvp5150_ops = {
 	.pad = &tvp5150_pad_ops,
 };
 
-static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
-	.registered = tvp5150_registered,
-};
-
 /****************************************************************************
 			I2C Client & Driver
  ****************************************************************************/
@@ -1614,12 +1538,6 @@  static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 {
 	struct v4l2_fwnode_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;
 
@@ -1643,63 +1561,6 @@  static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 
 	decoder->mbus_type = bus_cfg.bus_type;
 
-#ifdef CONFIG_MEDIA_CONTROLLER
-	connectors = of_get_child_by_name(np, "connectors");
-
-	if (!connectors)
-		goto err;
-
-	for_each_available_child_of_node(connectors, child) {
-		ret = of_property_read_u32(child, "input", &input_type);
-		if (ret) {
-			dev_err(decoder->sd.dev,
-				 "missing type property in node %s\n",
-				 child->name);
-			goto err_connector;
-		}
-
-		if (input_type >= TVP5150_INPUT_NUM) {
-			ret = -EINVAL;
-			goto err_connector;
-		}
-
-		input = &decoder->input_ent[input_type];
-
-		/* Each input connector can only be defined once */
-		if (input->name) {
-			dev_err(decoder->sd.dev,
-				 "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) {
-			dev_err(decoder->sd.dev,
-				 "missing label property in node %s\n",
-				 child->name);
-			goto err_connector;
-		}
-
-		input->name = name;
-	}
-
-err_connector:
-	of_node_put(connectors);
-#endif
 err:
 	of_node_put(ep);
 	return ret;
@@ -1751,7 +1612,6 @@  static int tvp5150_probe(struct i2c_client *c,
 	}
 
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
-	sd->internal_ops = &tvp5150_internal_ops;
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
@@ -1765,7 +1625,6 @@  static int tvp5150_probe(struct i2c_client *c,
 	if (res < 0)
 		return res;
 
-	sd->entity.ops = &tvp5150_sd_media_ops;
 #endif
 
 	res = tvp5150_detect_version(core);
diff --git a/include/dt-bindings/media/tvp5150.h b/include/dt-bindings/media/tvp5150.h
index c852a35e916e..8637910aae76 100644
--- a/include/dt-bindings/media/tvp5150.h
+++ b/include/dt-bindings/media/tvp5150.h
@@ -26,8 +26,6 @@ 
 #define TVP5150_COMPOSITE1 1
 #define TVP5150_SVIDEO     2
 
-#define TVP5150_INPUT_NUM  3
-
 /* TVP5150 HW outputs */
 #define TVP5150_NORMAL       0
 #define TVP5150_BLACK_SCREEN 1