Hi Jacopo,
Thank you for the patch,
On 11/12/2018 15:16, 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.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 6ce21542ed48..4d1aefc2c8d0 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
> + * @flags: Flags for the newly created link
> *
> * 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,
> + unsigned int flags)
> {
> - 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;
> -
Yup - that part certainly needs to go ...
> 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);
> + flags);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -68,24 +63,41 @@ 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 HDMI and AFE, 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.
> */
> - 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_txa(tx) && is_afe_enabled(state))
> + if (is_txa(tx)) {
> + if (is_hdmi_enabled(state)) {
> + ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> + &state->hdmi.sd,
> + ADV748X_HDMI_SOURCE,
> + MEDIA_LNK_FL_ENABLED);
> + if (ret)
> + return ret;
> + }
> +
> + if (is_afe_enabled(state)) {
> + ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> + &state->afe.sd,
> + ADV748X_AFE_SOURCE,
> + 0);
> + if (ret)
> + return ret;
> + }
> + } else if (is_afe_enabled(state))
I believe when adding braces to one side of an if statement, we are
supposed to add to the else clauses too ?
> return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> &state->afe.sd,
> - ADV748X_AFE_SOURCE);
> + ADV748X_AFE_SOURCE,
> + MEDIA_LNK_FL_ENABLED);
Won't this enable the AFE link for both TXA and TXB ?
Which one will win? Just the last one ? the first one ?
Does it error?
(It might not be a problem ... I can't recall what the behaviour is)
> +
There are a lot of nested if's above, and I think we can simplify
greatly if we move the logic for the flags inside
adv748x_csi2_register_link(), and adjust the checks on is_xxx_enabled()
What do you think about the following pseudo code?:
int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
struct v4l2_device *v4l2_dev,
struct v4l2_subdev *src,
unsigned int src_pad,
bool enable)
{
int flags = 0;
int ret;
if (!src->v4l2_dev) {
ret = v4l2_device_register_subdev(v4l2_dev, src)
if (ret) return ret;
}
if (enable)
flags = MEDIA_LNK_FL_ENABLED;
return media_create_pad_link(&src->entity, src_pad,
&tx->sd.entity, ADV748X_CSI2_SINK,
flags);
}
int adv748x_csi2_registered(struct v4l2_subdev *sd)
{
int ret;
if (is_afe_enabled(state) {
ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd,
ADV748X_AFE_SOURCE, !is_txa(tx));
if (ret)
return ret;
}
/* TX-B only supports AFE */
if (!is_txa(tx) || !(is_hdmi_enabled(state))
return 0;
return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
ADV748X_HDMI_SOURCE, true);
}
The above will for TXA:
register_link(..., AFE_SOURCE, enable = false );
register_link(..., HDMI_SOURCE, enable = true );
then TXB:
register_link(..., AFE_SOURCE, enable = true );
Does that meet our needs?
> return 0;
> }
>
> --
> 2.7.4
>
--
Regards
--
Kieran
