Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
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, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
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, >
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Wed, 1 Aug 2018 14:10:47 +0200
Marco Felsch escreveu:
> Hi Mauro,
>
> On 18-07-31 16:56, Mauro Carvalho Chehab wrote:
> > Em Tue, 31 Jul 2018 15:30:56 +0200
> > Marco Felsch 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-: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="{{ 0 | 1 | 2} | entity_1\nATV
> > decoder\ntvp5150 0-005c | { 3}}", shape=Mrecord, style=filled,
> > fillcolor=lightblue]
> > entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
> > shape=Mrecord, style=filled, fillcolor=aquamarine]
> > entity_9 [label="{{ 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 |
> > { 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > entity_16 [label="{entity_16\nunknown entity type\nS-Video | {
> > 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
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Hi Mauro,
On 18-07-31 16:56, Mauro Carvalho Chehab wrote:
> Em Tue, 31 Jul 2018 15:30:56 +0200
> Marco Felsch 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-: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="{{ 0 | 1 | 2} | entity_1\nATV
> decoder\ntvp5150 0-005c | { 3}}", shape=Mrecord, style=filled,
> fillcolor=lightblue]
> entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
> shape=Mrecord, style=filled, fillcolor=aquamarine]
> entity_9 [label="{{ 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 |
> { 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> entity_16 [label="{entity_16\nunknown entity type\nS-Video | {
> 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
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Tue, 31 Jul 2018 15:30:56 +0200
Marco Felsch 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-: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="{{ 0 | 1 | 2} | entity_1\nATV
decoder\ntvp5150 0-005c | { 3}}", shape=Mrecord, style=filled,
fillcolor=lightblue]
entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
shape=Mrecord, style=filled, fillcolor=aquamarine]
entity_9 [label="{{ 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 |
{ 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
entity_16 [label="{entity_16\nunknown entity type\nS-Video | {
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
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
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,
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Tue, 31 Jul 2018 07:06:59 -0300 Mauro Carvalho Chehab escreveu: > Em Tue, 31 Jul 2018 10:52:56 +0200 > Javier Martinez Canillas 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 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(>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; -
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Tue, 31 Jul 2018 14:36:52 +0200
Marco Felsch 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 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 escreveu:
> > >>>
> > From: Javier Martinez Canillas
> >
> > 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
> > Signed-off-by: Javier Martinez Canillas
> > Acked-by: Laurent Pinchart
> > [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> > Signed-off-by: Marco Felsch
> > ---
> > >>>
> > >>> ...
> > >>>
> > -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 = >input_ent[i];
> > - struct media_pad *pad = >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, >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-: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="{{ 0} | entity_1\nATV decoder\ntvp5150 0-005c |
> > > { 1 | 2}}", shape=Mrecord, style=filled,
> > > fillcolor=lightblue]
> > > entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
> > > shape=Mrecord, style=filled,
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
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, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
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 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 escreveu:
> >>>
> From: Javier Martinez Canillas
>
> 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
> Signed-off-by: Javier Martinez Canillas
> Acked-by: Laurent Pinchart
> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch
> ---
> >>>
> >>> ...
> >>>
> -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 = >input_ent[i];
> -struct media_pad *pad = >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, >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-: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="{{ 0} | entity_1\nATV decoder\ntvp5150 0-005c |
> > { 1 | 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
> > shape=Mrecord, style=filled, fillcolor=aquamarine]
> > entity_9 [label="{{ 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 |
> > { 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > entity_16 [label="{entity_16\nunknown entity type\nS-Video | {
> >
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
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 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 escreveu:
>>>
From: Javier Martinez Canillas
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
Signed-off-by: Javier Martinez Canillas
Acked-by: Laurent Pinchart
[m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
Signed-off-by: Marco Felsch
---
>>>
>>> ...
>>>
-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 = >input_ent[i];
- struct media_pad *pad = >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, >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-: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="{{ 0} | entity_1\nATV decoder\ntvp5150 0-005c |
> { 1 | 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
> shape=Mrecord, style=filled, fillcolor=aquamarine]
> entity_9 [label="{{ 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 |
> { 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> entity_16 [label="{entity_16\nunknown entity type\nS-Video | {
> 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
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Tue, 31 Jul 2018 10:52:56 +0200
Javier Martinez Canillas 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 escreveu:
> >
> >> From: Javier Martinez Canillas
> >>
> >> 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
> >> Signed-off-by: Javier Martinez Canillas
> >> Acked-by: Laurent Pinchart
> >> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> >> Signed-off-by: Marco Felsch
> >> ---
> >
> > ...
> >
> >> -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 = >input_ent[i];
> >> - struct media_pad *pad = >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, >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-: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="{{ 0} | entity_1\nATV decoder\ntvp5150 0-005c |
{ 1 | 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
entity_6 [label="{{ 0} | entity_6\nV4L I/O\n2-2:1.0 video}",
shape=Mrecord, style=filled, fillcolor=aquamarine]
entity_9 [label="{{ 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 |
{ 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
entity_16 [label="{entity_16\nunknown entity type\nS-Video | {
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,
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Hello Mauro,
On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 18:20:50 +0200
> Marco Felsch escreveu:
>
>> From: Javier Martinez Canillas
>>
>> 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
>> Signed-off-by: Javier Martinez Canillas
>> Acked-by: Laurent Pinchart
>> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
>> Signed-off-by: Marco Felsch
>> ---
>
> ...
>
>> -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 = >input_ent[i];
>> -struct media_pad *pad = >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, >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,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Hi Mauro,
On 18-07-30 15:18, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 18:20:50 +0200
> Marco Felsch escreveu:
>
> > From: Javier Martinez Canillas
> >
> > 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
> > Signed-off-by: Javier Martinez Canillas
> > Acked-by: Laurent Pinchart
> > [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> > Signed-off-by: Marco Felsch
> > ---
>
> ...
>
> > -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 = >input_ent[i];
> > - struct media_pad *pad = >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, >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
>
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Thu, 28 Jun 2018 18:20:50 +0200
Marco Felsch escreveu:
> From: Javier Martinez Canillas
>
> 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
> Signed-off-by: Javier Martinez Canillas
> Acked-by: Laurent Pinchart
> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch
> ---
...
> -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 = >input_ent[i];
> - struct media_pad *pad = >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, >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
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
On Thu, Jun 28, 2018 at 06:20:50PM +0200, Marco Felsch wrote:
> From: Javier Martinez Canillas
>
> 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
> Signed-off-by: Javier Martinez Canillas
> Acked-by: Laurent Pinchart
> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch
> ---
> drivers/media/i2c/tvp5150.c | 141
> include/dt-bindings/media/tvp5150.h | 2 -
Acked-by: Rob Herring
> 2 files changed, 143 deletions(-)
[PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
From: Javier Martinez Canillas
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
Signed-off-by: Javier Martinez Canillas
Acked-by: Laurent Pinchart
[m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
Signed-off-by: Marco Felsch
---
drivers/media/i2c/tvp5150.c | 141
include/dt-bindings/media/tvp5150.h | 2 -
2 files changed, 143 deletions(-)
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 == >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 = >input_ent[i];
- struct media_pad *pad = >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, >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 = _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
