| Message ID | 1544541373-30044-5-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: adv748x: Implement dynamic routing support | expand |
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@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); 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; >
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>
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;
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(-)