Hi
On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@xxxxxxxxxx> wrote:
>
> Hi Michael, Jagan, Hans,
>
> On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > Hi
> >
> > Down you have my tentative of connection
> >
> > I need to hack a bit to have tuner registered. I'm using imx-media
> >
> > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > <michael@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > Hi
> > >
> > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> > > >
> > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > Hi,
> > > > >
> > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > input data is converted into to digital composite using PAL decoder
> > > > > and it feed to adv7180, camera sensor.
> > > > >
> > > > > Analog input => Video PAL Decoder => ADV7180 => IPU-CSI0
> > > >
> > > > Just PAL? No NTSC support?
> > > >
> > > For now does not matter. I have registere the TUNER that support it
> > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > >
> > > Is this correct?
> > >
>
> media-types.rst reports:
>
> * - ``MEDIA_ENT_F_IF_VID_DECODER``
> - IF-PLL video decoder. It receives the IF from a PLL and decodes
> the analog TV video signal. This is commonly found on some very
> old analog tuners, like Philips MK3 designs. They all contain a
> tda9887 (or some software compatible similar chip, like tda9885).
> Those devices use a different I2C address than the tuner PLL.
>
> Is this what you were looking for?
>
> > > > >
> > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > bindings and the chip is
> > > > > detected fine.
> > > > >
> > > > > But we need to know, is this to be part of media control subdev
> > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > conventional pipeline or it should not to be part of media pipeline?
> > > >
> > > > Yes, I would say it should be part of the pipeline.
> > > >
> > >
> > > Ok I have created a draft patch to add the adv some new endpoint but
> > > is sufficient to declare tuner type in media control?
> > >
> > > Michael
> > >
> > > > >
> > > > > Please advise for best possible way to fit this into the design.
> > > > >
> > > > > Another observation is since the IPU has more than one sink, source
> > > > > pads, we source or sink the other components on the pipeline but look
> > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > not possible, If I'm not mistaken.
> > > >
> > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > to the video input connector on a board.
> > > >
> > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > we add support for connector entities.
> > > >
> > > > Regards,
> > > >
> > > > Hans
> > > >
> > > > >
> > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > it. is there any design similar to this in mainline?
> > > > >
> > > > > Please let us know if anyone has any suggestions on this.
> > > > >
> >
> > [ 3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > [ 3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > [ 3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > [ 3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > [ 3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > [ 3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > [ 3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > [ 3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > [ 3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > [ 3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > [ 3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > [ 3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > [ 3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> >
> > with
> > tuner: tuner@43 {
> > compatible = "tuner";
> > reg = <0x43>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_tuner>;
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > port@1 {
> > reg = <1>;
> >
> > tuner_in: endpoint {
>
> Nit: This is the tuner output, I would call this "tuner_out"
>
> > remote-endpoint = <&tuner_out>;
> > };
> > };
> > };
> > };
> >
> > adv7180: camera@20 {
> > compatible = "adi,adv7180";
>
> One minor thing first: look at the adv7180 bindings documentation, and
> you'll find out that only devices compatible with "adv7180cp" and
> "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> I don't see it enforced in driver's code, but still worth pointing it
> out from the very beginning)
Ok
>
> > reg = <0x20>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_adv7180>;
> > powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > port@1 {
> > reg = <1>;
> >
> > adv7180_to_ipu1_csi0_mux: endpoint {
> > remote-endpoint =
> > <&ipu1_csi0_mux_from_parallel_sensor>;
> > bus-width = <8>;
> > };
> > };
> >
> > port@0 {
> > reg = <0>;
> >
> > tuner_out: endpoint {
>
> Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
>
> > remote-endpoint = <&tuner_in>;
> > };
> > };
> > };
> > };
> >
> > Any help is appreciate
> >
>
> The adv7180(cp|st) bindings says the device can declare one (or more)
> input endpoints, but that's just to make possible to connect in device
> tree the 7180's device node with the video input port. You can see an
> example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> similar to what you've done here.
>
> The video input port does not show up in the media graph, as it is
> just a 'place holder' to describe an input port in DTs, the
> adv7180 driver does not register any sink pad, where to connect any
> video source to.
>
> Your proposed bindings here look almost correct, but to have it
> working for real you should add a sink pad to the adv7180 registered
> media entity (possibly only conditionally to the presence of an input
> endpoint in DTs...). You should then register a subdev-notifier, which
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -190,6 +190,12 @@ struct adv7180_state;
#define ADV7180_FLAG_MIPI_CSI2 BIT(2)
#define ADV7180_FLAG_I2P BIT(3)
+enum adv7180_pads {
+ ADV7180_PAD_IF_INPUT,
+ ADV7180_PAD_VID_OUT,
+ ADV7180_NUM_PADS
+};
+
struct adv7180_chip_info {
unsigned int flags;
unsigned int valid_input_mask;
@@ -201,7 +207,7 @@ struct adv7180_chip_info {
struct adv7180_state {
struct v4l2_ctrl_handler ctrl_hdl;
struct v4l2_subdev sd;
- struct media_pad pad;
+ struct media_pad pad[ADV7180_NUM_PADS];
struct mutex mutex; /* mutual excl. when accessing chip */
int irq;
struct gpio_desc *pwdn_gpio;
@@ -1360,9 +1366,12 @@ static int adv7180_probe(struct i2c_client *client,
if (ret)
goto err_unregister_vpp_client;
- state->pad.flags = MEDIA_PAD_FL_SOURCE;
+ state->pad[ADV7180_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+ state->pad[ADV7180_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
+ state->pad[ADV7180_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
+ state->pad[ADV7180_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
- ret = media_entity_pads_init(&sd->entity, 1, &state->pad);
+ ret = media_entity_pads_init(&sd->entity, ADV7180_NUM_PADS, state->pad);
if (ret)
goto err_free_ctrl;
> registers on an async-subdevice that uses the remote endpoint
> connected to your newly registered input pad to find out which device
> you're linked to; then at 'bound' (or possibly 'complete') time
> register a link between the two entities, on which you can operate on
> from userspace.
>
> Your tuner driver for tda9885 (which I don't see in mainline, so I
> assume it's downstream or custom code) should register an async subdevice,
tda988x is on mainline. Now I need to force somenthing in the config to have
registered as a subdev
Michael
> so that the adv7180 registered subdev-notifier gets matched and your
> callbacks invoked.
>
> If I were you, I would:
> 1) Add dt-parsing routine to tda7180, to find out if any input
> endpoint is registered in DT.
> 2) If it is, then register a SINK pad, along with the single SOURCE pad
> which is registered today.
> 3) When parsing DT, for input endpoints, get a reference to the remote
> endpoint connected to your input and register a subdev-notifier
> 4) Fill in the notifier 'bound' callback and register the link to
> your remote device there.
> 5) Make sure your tuner driver registers its subdevice with
> v4l2_async_register_subdev()
>
> A good example on how to register subdev notifier is provided in the
> rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> now uses subdev notifiers from v4.19 on too if you want to have a look
> there).
>
> -- Entering slippery territory here: you might want more inputs on this
>
> To make thing simpler&nicer (TM), if you blindly do what I've suggested
> here, you're going to break all current adv7180 users in mainline :(
>
> That's because the v4l2-async design 'completes' the root notifier,
> only if all subdev-notifiers connected to it have completed first.
> If you add a notifier for the adv7180 input ports unconditionally, and
> to the input port is connected a plain simple "composite-video-connector",
> as all DTs in mainline are doing right now, the newly registered
> subdev-notifier will never complete, as the "composite-video-connector"
> does not register any subdevice to match with. Bummer!
>
> A quick look in the code base, returns me that, in example:
> drivers/gpu/drm/meson/meson_drv.c filters on
> "composite-video-connector" and a few other compatible values. You
> might want to do the same, and register a notifier if and only if the
> remote input endpoint is one of those known not to register a
> subdevice. I'm sure there are other ways to deal with this issue, but
> that's the best I can come up with...
> ---
>
> Hope this is reasonably clear and that I'm not misleading you. I had to
> use adv7180 recently, and its single pad design confused me a bit as well :)
>
> Thanks
> j
>
> > Michael
> >
> > > > > Jagan.
> > > > >
> > > >
> > >
> > >
> > > --
--
| Michael Nazzareno Trimarchi Amarula Solutions BV |
| COO - Founder Cruquiuskade 47 |
| +31(0)851119172 Amsterdam 1018 AM NL |
| [`as] http://www.amarulasolutions.com |
