Hi Jacopo,
On 06/01/2019 15:54, Jacopo Mondi wrote:
> The ADV748x chip supports routing AFE output to either TXA or TXB.
> In order to support run-time configuration of video stream path, create an
> additional (not enabled) "AFE:8->TXA:0" link, and remove the IMMUTABLE flag
> from existing ones.
Great - this looks good to me.
Small nit below - but it's not a big deal.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
> 1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index b6b5d8c7ea7c..9d391d6f752e 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> * @v4l2_dev: Video registration device
> * @src: Source subdevice to establish link
> * @src_pad: Pad number of source to link to this @tx
> + * @enable: Link enabled flag
> *
> * Ensure that the subdevice is registered against the v4l2_device, and link the
> * source pad to the sink pad of the CSI2 bus entity.
> @@ -34,17 +35,11 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> struct v4l2_device *v4l2_dev,
> struct v4l2_subdev *src,
> - unsigned int src_pad)
> + unsigned int src_pad,
> + bool enable)
> {
> - int enabled = MEDIA_LNK_FL_ENABLED;
> int ret;
>
> - /*
> - * Dynamic linking of the AFE is not supported.
> - * Register the links as immutable.
> - */
> - enabled |= MEDIA_LNK_FL_IMMUTABLE;
> -
> if (!src->v4l2_dev) {
> ret = v4l2_device_register_subdev(v4l2_dev, src);
> if (ret)
> @@ -53,7 +48,7 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>
> return media_create_pad_link(&src->entity, src_pad,
> &tx->sd.entity, ADV748X_CSI2_SINK,
> - enabled);
> + enable ? MEDIA_LNK_FL_ENABLED : 0);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -68,25 +63,32 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> {
> struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> struct adv748x_state *state = tx->state;
> + int ret;
>
> adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> sd->name);
>
> /*
> - * The adv748x hardware allows the AFE to route through the TXA, however
> - * this is not currently supported in this driver.
> + * Link TXA to AFE and HDMI, and TXB to AFE only as TXB cannot output
> + * HDMI.
> *
> - * Link HDMI->TXA, and AFE->TXB directly.
> + * The HDMI->TXA link is enabled by default, as the AFE->TXB is.
I might have worded this "as is the AFE->TXB." but it's understandable.
I can update this when applying if you wish.
> */
> - if (is_txa(tx) && is_hdmi_enabled(state))
> - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> - &state->hdmi.sd,
> - ADV748X_HDMI_SOURCE);
> - if (is_txb(tx) && is_afe_enabled(state))
> - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> - &state->afe.sd,
> - ADV748X_AFE_SOURCE);
> - return 0;
> + if (is_afe_enabled(state)) {
> + ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> + &state->afe.sd,
> + ADV748X_AFE_SOURCE,
> + is_txb(tx));
> + if (ret)
> + return ret;
> + }
> +
> + /* Register link to HDMI for TXA only. */
> + if (is_txb(tx) || !is_hdmi_enabled(state))
> + return 0;
> +
> + return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> + ADV748X_HDMI_SOURCE, true);
> }
>
> static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
>
--
Regards
--
Kieran
