On 04/04/2017 04:10 PM, Russell King - ARM Linux wrote:
On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
The TVP5150 DT bindings specify a single output port (port 0) that
corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
---
I'm trying to get this to work with a TVP5150 analog TV decoder, and the
first problem is that this device doesn't have pad 0 as its single
output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
pads (input, video out, vbi out, audio out), and video out is pad 1,
whereas the device tree only defines a single port (0).
Looking at the patch, it's highlighted another review point with
Steve's driver.
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 17e2386a3ca3a..c52d6ca797965 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -267,6 +267,15 @@ static int imx_media_create_link(struct imx_media_dev *imxmd,
source_pad = link->local_pad;
sink_pad = link->remote_pad;
+ /*
+ * If the source subdev is an analog video decoder with a single source
+ * port, assume that this port 0 corresponds to the DEMOD_PAD_VID_OUT
+ * entity pad.
+ */
+ if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
+ local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
+ source_pad = DEMOD_PAD_VID_OUT;
+
v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
source->name, source_pad, sink->name, sink_pad);
What is "local" and what is "remote" here? It seems that, in the case of
a link being created with the sensor(etc), it's all back to front.
Eg, I see locally:
imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0
So here, "source" is the imx219 (the sensor), and sink is "imx6-mipi-csi2"
(part of the iMX6 capture.) However, this makes "local_sd" the subdev of
the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
totally back to front - this code is part of the iMX6 capture system,
so "local" implies that it should be part of that, and the "remote" thing
would be the sensor.
Hence, this seems completely confused. I'd suggest that:
(a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in imx_media_create_link()
is moved into imx_media_create_links(), and placed here instead:
for (j = 0; j < num_pads; j++) {
pad = &local_sd->pad[j];
if (pad->pad.flags & MEDIA_PAD_FL_SINK)
continue;
...
}
as the pad isn't going to spontaneously change this flag while we
consider each individual link.
Sure, I can do that. It would avoid iterating unnecessarily through the
pad's links if the pad is a sink.
However, maybe the test should be:
if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))
?
maybe that is more intuitive.
(b) the terms "local" and "remote" in imx_media_create_link() are
replaced with "source" and "sink" respectively, since this will
now be called with a guaranteed source pad.
Agreed. I'll change the arg and local var names.
As for Philipp's solution, I'm not sure what the correct solution for
something like this is. It depends how you view "hardware interface"
as defined by video-interfaces.txt, and whether the pads on the TVP5150
represent the hardware interfaces. If we take the view that the pads
do represent hardware interfaces, then using the reg= property on the
port node will solve this problem.
And the missing port nodes would have to actually be defined first.
According to Philipp they aren't, only a single output port 0.
If not, it would mean that we would have to have the iMX capture code
scan the pads on the source device, looking for outputs - but that
runs into a problem - if the source device has multiple outputs, does
the reg= property specify the output pad index or the pad number,
And how do we even know the data direction of a DT port. Is it an input,
an output, bidirectional? The OF graph parsing in imx-media-of.c can't
determine a port's direction if it encounters a device it doesn't
recognize that has multiple ports. For now that is not really a problem
because upstream from the video mux and csi-2 receiver it's expected
there will only be original sources of video with only one source port.
But it can become a limitation later. For example a device that has
multiple output bus interfaces, which would require multiple output
ports.
Steve
and what if one binding for a device specifies it one way and another
device's binding specifies it a different way.
There's lots of scope for making things really painful here, and
ending up with needing sensor-specific code in capture drivers to
work around different decisions on this.
I think someone needs to nail this down, if it's not already too late.
