| Message ID | 1544541373-30044-3-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, 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@jmondi.org> > --- > 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 >
Hi Kieran, On Tue, Dec 11, 2018 at 11:07:09PM +0000, Kieran Bingham wrote: > 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@jmondi.org> > > --- > > 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 ? Correct > > > 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) > > The AFE->TXA link is created as not enabled (see the 0 as last argument in the adv748x_csi2_register_link() call above here, in the 'is_txa(tx)' case > > + > > 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? > Yes it does, and it looks better. Thanks! > > > > > return 0; > > } > > > > -- > > 2.7.4 > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 12/12/2018 08:21, jacopo mondi wrote: > Hi Kieran, > > On Tue, Dec 11, 2018 at 11:07:09PM +0000, Kieran Bingham wrote: >> 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@jmondi.org> >>> --- >>> 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 ? > > Correct > >> >>> 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) >> >> > > The AFE->TXA link is created as not enabled (see the 0 as last > argument in the adv748x_csi2_register_link() call above here, in the > 'is_txa(tx)' case Ugh - of course it is ... I was clearly way too tired last night. Oops :D > >>> + >> >> 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? >> > > Yes it does, and it looks better. Thanks! Great - something useful came out of my non-sleeping sleepiness :) >>> return 0; >>> } >>> >>> -- >>> 2.7.4 >>> >> >> -- >> Regards >> -- >> Kieran
Hi Jacopo, Thank you for the patch. On Wednesday, 12 December 2018 10:21:01 EET jacopo mondi wrote: > On Tue, Dec 11, 2018 at 11:07:09PM +0000, Kieran Bingham wrote: > > 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@jmondi.org> > >> --- > >> > >> 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 You may want to name this link_flags, up to you. > >> * > >> * Ensure that the subdevice is registered against the v4l2_device, and > >> link the > >> * source pad to the sink pad of the CSI2 bus entity. [snip] > >> @@ -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; > >> + } I wonder whether in the case where both HDMI and TXB are disabled we shouldn't create this link as immutable and enabled ? This would require a big refactoring, possibly including deep changes in the v4l2-async and MC code, so it's likely out of scope for this patch (unless you see an easy way to do so). > >> + } 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 ? > > Correct And I would write this } else { if (is_afe_enabled(state)) ... } To more clearly visualize the TXA and TXB code. > >> 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) > > The AFE->TXA link is created as not enabled (see the 0 as last > argument in the adv748x_csi2_register_link() call above here, in the > 'is_txa(tx)' case > > >> + > > > > 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; > > I would add /* The AFE can be routed to both TXA and TXB. */ > > 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); > > } Looks better than my above proposal :-) > > 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? > > Yes it does, and it looks better. Thanks! > > >> return 0; > >> } With Kieran's proposed change, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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; - 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)) return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd, - ADV748X_AFE_SOURCE); + ADV748X_AFE_SOURCE, + MEDIA_LNK_FL_ENABLED); + return 0; }
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@jmondi.org> --- drivers/media/i2c/adv748x/adv748x-csi2.c | 48 ++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 18 deletions(-) -- 2.7.4