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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

> +	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.

> +		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 ?

> +	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