Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[Jacopo Mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 5319 bytes --]

Hi Laurent, Kieran,
  thanks for the review

On Thu, Dec 13, 2018 at 11:40:00AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tuesday, 11 December 2018 17:16:13 EET Jacopo Mondi wrote:
> > When the adv748x driver is informed about a link being created from HDMI or
> > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > sure to implement proper routing management at link setup time, to route
> > the selected video stream to the desired TX output.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 63 ++++++++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >  2 files changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053
> > 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  /* ------------------------------------------------------------------------
> >   * Media Operations
> >   */
> > +static int adv748x_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *local,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > +	struct adv748x_csi2 *tx;
> > +	struct media_link *link;
> > +	u8 io10;
> > +
> > +	/*
> > +	 * For each link setup from [HDMI|AFE] to TX we receive two
> > +	 * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> > +	 *
> > +	 * Use the second notification form to make sure we're linking
> > +	 * to a TX and find out from where, to set up routing properly.
> > +	 */
>
> Why don't you implement the link handler just for the TX entities then ?
>

Done. Much better

> > +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> > +	    !(flags & MEDIA_LNK_FL_ENABLED))
>
> When disabling the link you should reset the ->source and ->tx pointers.
>

right

> > +		return 0;
> > +	tx = adv748x_sd_to_csi2(sd);
> > +
> > +	/*
> > +	 * Now that we're sure we're operating on one of the two TXs,
> > +	 * make sure there are no enabled links ending there from
> > +	 * either HDMI or AFE (this can only happens for TXA though).
> > +	 */
> > +	if (is_txa(tx))
> > +		list_for_each_entry(link, &entity->links, list)
> > +			if (link->sink->entity == entity &&
> > +			    link->flags & MEDIA_LNK_FL_ENABLED)
> > +				return -EINVAL;
>
> You can simplify this by checking if tx->source == NULL (after resetting tx-
> >source when disabling the link of course).
>
> > +	/* Change video stream routing, according to the newly created link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		state->afe.tx = tx;
> > +
> > +		/*
> > +		 * If AFE is routed to TXA, make sure TXB is off;
> > +		 * If AFE goes to TXB, we need TXA powered on.
> > +		 */
> > +		if (is_txa(tx)) {
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +			io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		} else {
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +		}
> > +	} else {
> > +		state->hdmi.tx = tx;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
>
> Is it guaranteed that the chip will be powered on at this point ? How about
> writing the register at stream on time instead ?

You know, I struggle to find a better place where to do so...
- Right now the register gets written at reset time only
- s_stream: Each subdev (afe, hdmi, txa and txb) has its own s_stream
  so it's hard to find a centralized place where to write this
  register that impacts both TXes
- afe/hdmi s_stream() ops call tx_power_up, but again this is per-TX
- There doesn't seems to be much concern in the driver in
  synchronizing register writes with power states already actually. Not
  that's an excuse, but when I played around and tried to power off
  the chip's PLLs for real I got into LP-11 state detection issues
  (again :) I would leave it here if that's fine with you :)

Thanks
   j

>
> > +	tx->rsd = rsd;
> > +
> > +	return 0;
> > +}
> >
> >  static const struct media_entity_operations adv748x_media_ops = {
> > -	.link_validate = v4l2_subdev_link_validate,
> > +	.link_setup	= adv748x_link_setup,
> > +	.link_validate	= v4l2_subdev_link_validate,
> >  };
> >
> >  /* ------------------------------------------------------------------------
> > -- diff --git a/drivers/media/i2c/adv748x/adv748x.h
> > b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169
> > 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -220,6 +220,7 @@ struct adv748x_state {
> >  #define ADV748X_IO_10_CSI4_EN		BIT(7)
> >  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> >  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	0x08
> >
> >  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
> >  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]