Hi Jacopo, On 11/12/2018 15:16, 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. Keeping these link references certainly feels like a useful thing to do. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> > --- > 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); And makes that much better... > 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; It's a shame that we haven't got a 'base class' for the AFE/HDMI so that this could be set independently... > } > > 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; Also - Can this be simplified into the pseudo code I suggested earlier. I haven't given that much thought here as it's late - and I don't know if you'll accept my pseudo code change yet :) > + } > > 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); Certainly beneficial here :) > 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; > -- Regards -- Kieran