Hi Mauro,

On 18-07-31 16:56, Mauro Carvalho Chehab wrote:

Em Tue, 31 Jul 2018 15:30:56 +0200
Marco Felsch [off-list ref] escreveu:

quoted

Hi Javier,

On 18-07-31 14:52, Javier Martinez Canillas wrote:

quoted

Hi Marco,

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

[snip]
  

quoted

quoted

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