Re: [PATCH 4/5] media: adv748x: Store the TX sink in HDMI/AFE

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

Hi Jacopo,

Thank you for the patch.

On Tuesday, 11 December 2018 17:16:12 EET Jacopo Mondi wrote:
> Both the AFE and HDMI s_stream routines (adv748x_afe_s_stream() and
> adv748x_hdmi_s_stream()) have to enable the CSI-2 TX they are streaming
> video data to.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, the video stream sink needs to be set at run time, and not statically
> selected as the s_stream functions are currently doing.
> 
> To fix this, store a reference to the active CSI-2 TX sink for both HDMI and
> AFE sources, and operate on it when starting/stopping the stream.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 19 ++++++++++++++-----
>  drivers/media/i2c/adv748x/adv748x-hdmi.c |  2 +-
>  drivers/media/i2c/adv748x/adv748x.h      |  4 ++++
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c
> b/drivers/media/i2c/adv748x/adv748x-afe.c index 71714634efb0..dbbb1e4d6363
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -282,7 +282,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd,
> int enable) goto unlock;
>  	}
> 
> -	ret = adv748x_tx_power(&state->txb, enable);
> +	ret = adv748x_tx_power(afe->tx, enable);
>  	if (ret)
>  		goto unlock;
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> b/drivers/media/i2c/adv748x/adv748x-csi2.c index 307966f4c736..0d6344a51795
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -85,6 +85,9 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
> +
> +			/* The default HDMI output is TXA. */
> +			state->hdmi.tx = tx;
>  		}
> 
>  		if (is_afe_enabled(state)) {
> @@ -95,11 +98,17 @@ static int adv748x_csi2_registered(struct v4l2_subdev
> *sd) if (ret)
>  				return ret;
>  		}
> -	} else if (is_afe_enabled(state))
> -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> -						  &state->afe.sd,
> -						  ADV748X_AFE_SOURCE,
> -						  MEDIA_LNK_FL_ENABLED);
> +	} else if (is_afe_enabled(state)) {
> +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> +						 &state->afe.sd,
> +						 ADV748X_AFE_SOURCE,
> +						 MEDIA_LNK_FL_ENABLED);
> +		if (ret)
> +			return ret;
> +
> +		/* The default AFE output is TXB. */
> +		state->afe.tx = tx;
> +	}
> 
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> b/drivers/media/i2c/adv748x/adv748x-hdmi.c index 35d027941482..c557f8fdf11a
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -358,7 +358,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd,
> int enable)
> 
>  	mutex_lock(&state->mutex);
> 
> -	ret = adv748x_tx_power(&state->txa, enable);
> +	ret = adv748x_tx_power(hdmi->tx, enable);
>  	if (ret)
>  		goto done;
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index 387002d6da65..0ee3b8d5c795
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -118,6 +118,8 @@ struct adv748x_hdmi {
>  	struct v4l2_dv_timings timings;
>  	struct v4l2_fract aspect_ratio;
> 
> +	struct adv748x_csi2 *tx;
> +
>  	struct {
>  		u8 edid[512];
>  		u32 present;
> @@ -148,6 +150,8 @@ struct adv748x_afe {
>  	struct v4l2_subdev sd;
>  	struct v4l2_mbus_framefmt format;
> 
> +	struct adv748x_csi2 *tx;
> +
>  	bool streaming;
>  	v4l2_std_id curr_norm;
>  	unsigned int input;

This may call for defining a common structure to store the common fields of 
adv748x_hdmi and adv748x_afe. Out of scope for this patch, but please keep it 
in mind.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart