| Message ID | 20190106155413.30666-7-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: adv748x: Implement dynamic routing support | expand |
Hi Jacopo, On 06/01/2019 15:54, Jacopo Mondi wrote: > When the adv748x driver is informed about a link being created from HDMI or > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > sure to implement proper routing management at link setup time, to route > the selected video stream to the desired TX output. Overall this looks like the right approach - but I feel like the handling of the io10 register might need some consideration, because it's value depends on the condition of both CSI2 transmitters, not just the currently parsed link. I had a go at some pseudo - uncompiled/untested code inline as a suggestion. If you think it's better - feel free to rework it in ... or not as you see fit. Regards Kieran > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > drivers/media/i2c/adv748x/adv748x.h | 2 + > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > index 200e00f93546..a586bf393558 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > /* ----------------------------------------------------------------------------- > * Media Operations > */ > +static int adv748x_link_setup(struct media_entity *entity, > + const struct media_pad *local, > + const struct media_pad *remote, u32 flags) > +{ > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > + bool enable = flags & MEDIA_LNK_FL_ENABLED; > + u8 io10; > + > + /* Refuse to enable multiple links to the same TX at the same time. */ > + if (enable && tx->src) > + return -EINVAL; > + > + /* Set or clear the source (HDMI or AFE) and the current TX. */ > + if (rsd == &state->afe.sd) > + state->afe.tx = enable ? tx : NULL; > + else > + state->hdmi.tx = enable ? tx : NULL; > + > + tx->src = enable ? rsd : NULL; > + > + if (!enable) > + return 0; Don't we potentially want to take any action on disable to power down links below ? > + > + /* Change video stream routing, according to the newly enabled link. */ > + io10 = io_read(state, ADV748X_IO_10); > + if (rsd == &state->afe.sd) { > + /* > + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. > + * if AFE goes to TXB, we need both TXA and TXB powered on. > + */ > + io10 &= ~ADV748X_IO_10_CSI1_EN; > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > + if (is_txa(tx)) > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; Shouldn't the CSI4 be enabled here too? or are we assuming it's already (/always) enabled? io10 |= ADV748X_IO_10_CSI4_EN; > + else > + io10 |= ADV748X_IO_10_CSI4_EN | > + ADV748X_IO_10_CSI1_EN; > + } else { > + /* Clear AFE->TXA routing and power up TXA. */ > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > + io10 |= ADV748X_IO_10_CSI4_EN; But if we assume it's already enabled ... do we need this? Perhaps it might be better to be explicit on this? > + } > + io_write(state, ADV748X_IO_10, io10); Would it be any cleaner to use io_clrset() here? Hrm ... also it feels like this register really should be set depending upon the complete state of ... &state->... So perhaps it deserves it's own function which should be called after csi_registered() callback and any link change. /me has a quick go at some psuedo codeishness...: int adv74x_io_10(struct adv748x_state *state); u8 bits = 0; u8 mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | ADV748X_IO_10_CSI4_IN_SEL_AFE; if (state->afe.tx) { /* AFE Requires TXA enabled, even when output to TXB */ bits |= ADV748X_IO_10_CSI4_EN; if (is_txa(state->afe.tx)) bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE else bits |= ADV748X_IO_10_CSI1_EN; } if (state->hdmi.tx) { bits |= ADV748X_IO_10_CSI4_EN; } return io_clrset(state, ADV748X_IO_10, mask, bits); } How does that look ? (is it even correct first?) > + > + return 0; > +} > + > +static const struct media_entity_operations adv748x_tx_media_ops = { > + .link_setup = adv748x_link_setup, > + .link_validate = v4l2_subdev_link_validate, > +}; > > static const struct media_entity_operations adv748x_media_ops = { > .link_validate = v4l2_subdev_link_validate, > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state, > state->client->addr, ident); > > sd->entity.function = function; > - sd->entity.ops = &adv748x_media_ops; > + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > + &adv748x_tx_media_ops : &adv748x_media_ops; Aha - yes that's a neat solution to ensure that only the TX links generate link_setup calls :) > } > > static int adv748x_parse_csi2_lanes(struct adv748x_state *state, > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > index 6eb2e4a95eed..eb19c6cbbb4e 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -93,6 +93,7 @@ struct adv748x_csi2 { > > #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL) > #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab) > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx)) > #define is_txa(_tx) __is_tx(_tx, a) > #define is_txb(_tx) __is_tx(_tx, b) > > @@ -224,6 +225,7 @@ struct adv748x_state { > #define ADV748X_IO_10_CSI4_EN BIT(7) > #define ADV748X_IO_10_CSI1_EN BIT(6) > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE BIT(3) > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > #define ADV748X_IO_CHIP_REV_ID_2 0xe0 >
Hello, On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote: > On 06/01/2019 15:54, Jacopo Mondi wrote: > > When the adv748x driver is informed about a link being created from HDMI > > or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > > sure to implement proper routing management at link setup time, to route > > the selected video stream to the desired TX output. > > Overall this looks like the right approach - but I feel like the > handling of the io10 register might need some consideration, because > it's value depends on the condition of both CSI2 transmitters, not just > the currently parsed link. > > I had a go at some pseudo - uncompiled/untested code inline as a suggestion. > > If you think it's better - feel free to rework it in ... or not as you > see fit. > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > > > drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > > drivers/media/i2c/adv748x/adv748x.h | 2 + > > 2 files changed, 58 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c > > b/drivers/media/i2c/adv748x/adv748x-core.c index > > 200e00f93546..a586bf393558 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool > > on) > > /* ---------------------------------------------------------------------- > > * Media Operations > > */ > > +static int adv748x_link_setup(struct media_entity *entity, > > + const struct media_pad *local, > > + const struct media_pad *remote, u32 flags) > > +{ > > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > > + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > + bool enable = flags & MEDIA_LNK_FL_ENABLED; > > + u8 io10; > > + > > + /* Refuse to enable multiple links to the same TX at the same time. */ > > + if (enable && tx->src) > > + return -EINVAL; > > + > > + /* Set or clear the source (HDMI or AFE) and the current TX. */ > > + if (rsd == &state->afe.sd) > > + state->afe.tx = enable ? tx : NULL; > > + else > > + state->hdmi.tx = enable ? tx : NULL; > > + > > + tx->src = enable ? rsd : NULL; > > + > > + if (!enable) > > + return 0; > > Don't we potentially want to take any action on disable to power down > links below ? > > > + > > + /* Change video stream routing, according to the newly enabled link. */ > > + io10 = io_read(state, ADV748X_IO_10); > > + if (rsd == &state->afe.sd) { > > + /* > > + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. > > + * if AFE goes to TXB, we need both TXA and TXB powered on. > > + */ > > + io10 &= ~ADV748X_IO_10_CSI1_EN; > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + if (is_txa(tx)) > > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > Shouldn't the CSI4 be enabled here too? or are we assuming it's already > (/always) enabled? > io10 |= ADV748X_IO_10_CSI4_EN; > > > + else > > + io10 |= ADV748X_IO_10_CSI4_EN | > > + ADV748X_IO_10_CSI1_EN; > > + } else { > > + /* Clear AFE->TXA routing and power up TXA. */ > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + io10 |= ADV748X_IO_10_CSI4_EN; > > But if we assume it's already enabled ... do we need this? > Perhaps it might be better to be explicit on this? > > > + } > > + io_write(state, ADV748X_IO_10, io10); > > Would it be any cleaner to use io_clrset() here? > > Hrm ... also it feels like this register really should be set depending > upon the complete state of ... &state->... > > So perhaps it deserves it's own function which should be called after > csi_registered() callback and any link change. > > /me has a quick go at some psuedo codeishness...: > > int adv74x_io_10(struct adv748x_state *state); > u8 bits = 0; > u8 mask = ADV748X_IO_10_CSI1_EN > > | ADV748X_IO_10_CSI4_EN > | ADV748X_IO_10_CSI4_IN_SEL_AFE; > > if (state->afe.tx) { > /* AFE Requires TXA enabled, even when output to TXB */ > bits |= ADV748X_IO_10_CSI4_EN; > > if (is_txa(state->afe.tx)) > bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE > else > bits |= ADV748X_IO_10_CSI1_EN; > } > > if (state->hdmi.tx) { > bits |= ADV748X_IO_10_CSI4_EN; > } > > return io_clrset(state, ADV748X_IO_10, mask, bits); > } > > How does that look ? (is it even correct first?) > > > + > > + return 0; > > +} > > + > > +static const struct media_entity_operations adv748x_tx_media_ops = { > > + .link_setup = adv748x_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > > > static const struct media_entity_operations adv748x_media_ops = { > > .link_validate = v4l2_subdev_link_validate, > > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, > > struct adv748x_state *state, > > state->client->addr, ident); > > > > sd->entity.function = function; > > - sd->entity.ops = &adv748x_media_ops; > > + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > > + &adv748x_tx_media_ops : &adv748x_media_ops; > > Aha - yes that's a neat solution to ensure that only the TX links > generate link_setup calls :) Another option would be to bail out from adv748x_link_setup() if the entity is not a TX*. > > } [snip]
On 09/01/2019 00:15, Laurent Pinchart wrote: > Hello, > > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote: >> On 06/01/2019 15:54, Jacopo Mondi wrote: >>> When the adv748x driver is informed about a link being created from HDMI >>> or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make >>> sure to implement proper routing management at link setup time, to route >>> the selected video stream to the desired TX output. >> >> Overall this looks like the right approach - but I feel like the >> handling of the io10 register might need some consideration, because >> it's value depends on the condition of both CSI2 transmitters, not just >> the currently parsed link. >> >> I had a go at some pseudo - uncompiled/untested code inline as a suggestion. >> >> If you think it's better - feel free to rework it in ... or not as you >> see fit. >> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> >>> drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- >>> drivers/media/i2c/adv748x/adv748x.h | 2 + >>> 2 files changed, 58 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c >>> b/drivers/media/i2c/adv748x/adv748x-core.c index >>> 200e00f93546..a586bf393558 100644 >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c >>> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool >>> on) >>> /* ---------------------------------------------------------------------- >>> * Media Operations >>> */ >>> +static int adv748x_link_setup(struct media_entity *entity, >>> + const struct media_pad *local, >>> + const struct media_pad *remote, u32 flags) >>> +{ >>> + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); >>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); >>> + struct adv748x_state *state = v4l2_get_subdevdata(sd); >>> + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); >>> + bool enable = flags & MEDIA_LNK_FL_ENABLED; >>> + u8 io10; >>> + >>> + /* Refuse to enable multiple links to the same TX at the same time. */ >>> + if (enable && tx->src) >>> + return -EINVAL; >>> + >>> + /* Set or clear the source (HDMI or AFE) and the current TX. */ >>> + if (rsd == &state->afe.sd) >>> + state->afe.tx = enable ? tx : NULL; >>> + else >>> + state->hdmi.tx = enable ? tx : NULL; >>> + >>> + tx->src = enable ? rsd : NULL; >>> + >>> + if (!enable) >>> + return 0; >> >> Don't we potentially want to take any action on disable to power down >> links below ? >> >>> + >>> + /* Change video stream routing, according to the newly enabled link. */ >>> + io10 = io_read(state, ADV748X_IO_10); >>> + if (rsd == &state->afe.sd) { >>> + /* >>> + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. >>> + * if AFE goes to TXB, we need both TXA and TXB powered on. >>> + */ >>> + io10 &= ~ADV748X_IO_10_CSI1_EN; >>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> + if (is_txa(tx)) >>> + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >> >> Shouldn't the CSI4 be enabled here too? or are we assuming it's already >> (/always) enabled? >> io10 |= ADV748X_IO_10_CSI4_EN; >> >>> + else >>> + io10 |= ADV748X_IO_10_CSI4_EN | >>> + ADV748X_IO_10_CSI1_EN; >>> + } else { >>> + /* Clear AFE->TXA routing and power up TXA. */ >>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> + io10 |= ADV748X_IO_10_CSI4_EN; >> >> But if we assume it's already enabled ... do we need this? >> Perhaps it might be better to be explicit on this? >> >>> + } >>> + io_write(state, ADV748X_IO_10, io10); >> >> Would it be any cleaner to use io_clrset() here? >> >> Hrm ... also it feels like this register really should be set depending >> upon the complete state of ... &state->... >> >> So perhaps it deserves it's own function which should be called after >> csi_registered() callback and any link change. >> >> /me has a quick go at some psuedo codeishness...: >> >> int adv74x_io_10(struct adv748x_state *state); >> u8 bits = 0; >> u8 mask = ADV748X_IO_10_CSI1_EN >> >> | ADV748X_IO_10_CSI4_EN >> | ADV748X_IO_10_CSI4_IN_SEL_AFE; >> >> if (state->afe.tx) { >> /* AFE Requires TXA enabled, even when output to TXB */ >> bits |= ADV748X_IO_10_CSI4_EN; >> >> if (is_txa(state->afe.tx)) >> bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE >> else >> bits |= ADV748X_IO_10_CSI1_EN; >> } >> >> if (state->hdmi.tx) { >> bits |= ADV748X_IO_10_CSI4_EN; >> } >> >> return io_clrset(state, ADV748X_IO_10, mask, bits); >> } >> >> How does that look ? (is it even correct first?) >> >>> + >>> + return 0; >>> +} >>> + >>> +static const struct media_entity_operations adv748x_tx_media_ops = { >>> + .link_setup = adv748x_link_setup, >>> + .link_validate = v4l2_subdev_link_validate, >>> +}; >>> >>> static const struct media_entity_operations adv748x_media_ops = { >>> .link_validate = v4l2_subdev_link_validate, >>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, >>> struct adv748x_state *state, >>> state->client->addr, ident); >>> >>> sd->entity.function = function; >>> - sd->entity.ops = &adv748x_media_ops; >>> + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? >>> + &adv748x_tx_media_ops : &adv748x_media_ops; >> >> Aha - yes that's a neat solution to ensure that only the TX links >> generate link_setup calls :) > > Another option would be to bail out from adv748x_link_setup() if the entity is > not a TX*. > I suggested this in v1 - but Jacopo objected with the following: > Checking for is_txa() and is_txb() would require to call > 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually > represent a csi2_tx. I would keep it as it is. Now I look at the implementation here, I see this is precisely what it is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on an unknown pointer type (which I still believe is a valid thing to do in this instance) So yes, I think this would be simpler having the check at the top of the adv748x_link_setup() call, and thus then there is no need to add a second adv_media_ops structure. >>> } > > [snip] >
Hi Jacopo, One more comment below: On 07/01/2019 12:36, Kieran Bingham wrote: > Hi Jacopo, > > On 06/01/2019 15:54, Jacopo Mondi wrote: >> When the adv748x driver is informed about a link being created from HDMI or >> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make >> sure to implement proper routing management at link setup time, to route >> the selected video stream to the desired TX output.> <snip> >> static int adv748x_parse_csi2_lanes(struct adv748x_state *state, >> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h >> index 6eb2e4a95eed..eb19c6cbbb4e 100644 >> --- a/drivers/media/i2c/adv748x/adv748x.h >> +++ b/drivers/media/i2c/adv748x/adv748x.h >> @@ -93,6 +93,7 @@ struct adv748x_csi2 { >> >> #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL) >> #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab) >> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx)) I'd put this /after/ is_txa/is_txb so that the use is after the declarations. -- KB >> #define is_txa(_tx) __is_tx(_tx, a) >> #define is_txb(_tx) __is_tx(_tx, b) >> >> @@ -224,6 +225,7 @@ struct adv748x_state { >> #define ADV748X_IO_10_CSI4_EN BIT(7) >> #define ADV748X_IO_10_CSI1_EN BIT(6) >> #define ADV748X_IO_10_PIX_OUT_EN BIT(5) >> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE BIT(3) >> >> #define ADV748X_IO_CHIP_REV_ID_1 0xdf >> #define ADV748X_IO_CHIP_REV_ID_2 0xe0 >> >
Hi Laurent, On Wed, Jan 09, 2019 at 02:15:04AM +0200, Laurent Pinchart wrote: > Hello, > > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote: > > On 06/01/2019 15:54, Jacopo Mondi wrote: > > > When the adv748x driver is informed about a link being created from HDMI > > > or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > > > sure to implement proper routing management at link setup time, to route > > > the selected video stream to the desired TX output. > > > > Overall this looks like the right approach - but I feel like the > > handling of the io10 register might need some consideration, because > > it's value depends on the condition of both CSI2 transmitters, not just > > the currently parsed link. > > > > I had a go at some pseudo - uncompiled/untested code inline as a suggestion. > > > > If you think it's better - feel free to rework it in ... or not as you > > see fit. > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > > > > drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > > > drivers/media/i2c/adv748x/adv748x.h | 2 + > > > 2 files changed, 58 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c > > > b/drivers/media/i2c/adv748x/adv748x-core.c index > > > 200e00f93546..a586bf393558 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool > > > on) > > > /* ---------------------------------------------------------------------- > > > * Media Operations > > > */ > > > +static int adv748x_link_setup(struct media_entity *entity, > > > + const struct media_pad *local, > > > + const struct media_pad *remote, u32 flags) > > > +{ > > > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > > > + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > > + bool enable = flags & MEDIA_LNK_FL_ENABLED; > > > + u8 io10; > > > + > > > + /* Refuse to enable multiple links to the same TX at the same time. */ > > > + if (enable && tx->src) > > > + return -EINVAL; > > > + > > > + /* Set or clear the source (HDMI or AFE) and the current TX. */ > > > + if (rsd == &state->afe.sd) > > > + state->afe.tx = enable ? tx : NULL; > > > + else > > > + state->hdmi.tx = enable ? tx : NULL; > > > + > > > + tx->src = enable ? rsd : NULL; > > > + > > > + if (!enable) > > > + return 0; > > > > Don't we potentially want to take any action on disable to power down > > links below ? > > > > > + > > > + /* Change video stream routing, according to the newly enabled link. */ > > > + io10 = io_read(state, ADV748X_IO_10); > > > + if (rsd == &state->afe.sd) { > > > + /* > > > + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. > > > + * if AFE goes to TXB, we need both TXA and TXB powered on. > > > + */ > > > + io10 &= ~ADV748X_IO_10_CSI1_EN; > > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > > + if (is_txa(tx)) > > > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > > > Shouldn't the CSI4 be enabled here too? or are we assuming it's already > > (/always) enabled? > > io10 |= ADV748X_IO_10_CSI4_EN; > > > > > + else > > > + io10 |= ADV748X_IO_10_CSI4_EN | > > > + ADV748X_IO_10_CSI1_EN; > > > + } else { > > > + /* Clear AFE->TXA routing and power up TXA. */ > > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > > + io10 |= ADV748X_IO_10_CSI4_EN; > > > > But if we assume it's already enabled ... do we need this? > > Perhaps it might be better to be explicit on this? > > > > > + } > > > + io_write(state, ADV748X_IO_10, io10); > > > > Would it be any cleaner to use io_clrset() here? > > > > Hrm ... also it feels like this register really should be set depending > > upon the complete state of ... &state->... > > > > So perhaps it deserves it's own function which should be called after > > csi_registered() callback and any link change. > > > > /me has a quick go at some psuedo codeishness...: > > > > int adv74x_io_10(struct adv748x_state *state); > > u8 bits = 0; > > u8 mask = ADV748X_IO_10_CSI1_EN > > > > | ADV748X_IO_10_CSI4_EN > > | ADV748X_IO_10_CSI4_IN_SEL_AFE; > > > > if (state->afe.tx) { > > /* AFE Requires TXA enabled, even when output to TXB */ > > bits |= ADV748X_IO_10_CSI4_EN; > > > > if (is_txa(state->afe.tx)) > > bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE > > else > > bits |= ADV748X_IO_10_CSI1_EN; > > } > > > > if (state->hdmi.tx) { > > bits |= ADV748X_IO_10_CSI4_EN; > > } > > > > return io_clrset(state, ADV748X_IO_10, mask, bits); > > } > > > > How does that look ? (is it even correct first?) > > > > > + > > > + return 0; > > > +} > > > + > > > +static const struct media_entity_operations adv748x_tx_media_ops = { > > > + .link_setup = adv748x_link_setup, > > > + .link_validate = v4l2_subdev_link_validate, > > > +}; > > > > > > static const struct media_entity_operations adv748x_media_ops = { > > > .link_validate = v4l2_subdev_link_validate, > > > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, > > > struct adv748x_state *state, > > > state->client->addr, ident); > > > > > > sd->entity.function = function; > > > - sd->entity.ops = &adv748x_media_ops; > > > + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > > > + &adv748x_tx_media_ops : &adv748x_media_ops; > > > > Aha - yes that's a neat solution to ensure that only the TX links > > generate link_setup calls :) > > Another option would be to bail out from adv748x_link_setup() if the entity is > not a TX*. > If I'm not wrong you suggested me to register a set of operations with the .link_setup callback only for TX entities, and I agree it is much better, so I'm leaning to leave it as it is in this series. Thanks j > > > } > > [snip] > > -- > Regards, > > Laurent Pinchart > > >
On Wed, Jan 09, 2019 at 02:15:33PM +0000, Kieran Bingham wrote: > On 09/01/2019 00:15, Laurent Pinchart wrote: > > Hello, > > > > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote: > >> On 06/01/2019 15:54, Jacopo Mondi wrote: > >>> When the adv748x driver is informed about a link being created from HDMI > >>> or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > >>> sure to implement proper routing management at link setup time, to route > >>> the selected video stream to the desired TX output. > >> > >> Overall this looks like the right approach - but I feel like the > >> handling of the io10 register might need some consideration, because > >> it's value depends on the condition of both CSI2 transmitters, not just > >> the currently parsed link. > >> > >> I had a go at some pseudo - uncompiled/untested code inline as a suggestion. > >> > >> If you think it's better - feel free to rework it in ... or not as you > >> see fit. > >> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> > >>> drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > >>> drivers/media/i2c/adv748x/adv748x.h | 2 + > >>> 2 files changed, 58 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c > >>> b/drivers/media/i2c/adv748x/adv748x-core.c index > >>> 200e00f93546..a586bf393558 100644 > >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c > >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c > >>> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool > >>> on) > >>> /* ---------------------------------------------------------------------- > >>> * Media Operations > >>> */ > >>> +static int adv748x_link_setup(struct media_entity *entity, > >>> + const struct media_pad *local, > >>> + const struct media_pad *remote, u32 flags) > >>> +{ > >>> + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > >>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > >>> + struct adv748x_state *state = v4l2_get_subdevdata(sd); > >>> + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > >>> + bool enable = flags & MEDIA_LNK_FL_ENABLED; > >>> + u8 io10; > >>> + > >>> + /* Refuse to enable multiple links to the same TX at the same time. */ > >>> + if (enable && tx->src) > >>> + return -EINVAL; > >>> + > >>> + /* Set or clear the source (HDMI or AFE) and the current TX. */ > >>> + if (rsd == &state->afe.sd) > >>> + state->afe.tx = enable ? tx : NULL; > >>> + else > >>> + state->hdmi.tx = enable ? tx : NULL; > >>> + > >>> + tx->src = enable ? rsd : NULL; > >>> + > >>> + if (!enable) > >>> + return 0; > >> > >> Don't we potentially want to take any action on disable to power down > >> links below ? > >> > >>> + > >>> + /* Change video stream routing, according to the newly enabled link. */ > >>> + io10 = io_read(state, ADV748X_IO_10); > >>> + if (rsd == &state->afe.sd) { > >>> + /* > >>> + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. > >>> + * if AFE goes to TXB, we need both TXA and TXB powered on. > >>> + */ > >>> + io10 &= ~ADV748X_IO_10_CSI1_EN; > >>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > >>> + if (is_txa(tx)) > >>> + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > >> > >> Shouldn't the CSI4 be enabled here too? or are we assuming it's already > >> (/always) enabled? > >> io10 |= ADV748X_IO_10_CSI4_EN; > >> > >>> + else > >>> + io10 |= ADV748X_IO_10_CSI4_EN | > >>> + ADV748X_IO_10_CSI1_EN; > >>> + } else { > >>> + /* Clear AFE->TXA routing and power up TXA. */ > >>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > >>> + io10 |= ADV748X_IO_10_CSI4_EN; > >> > >> But if we assume it's already enabled ... do we need this? > >> Perhaps it might be better to be explicit on this? > >> > >>> + } > >>> + io_write(state, ADV748X_IO_10, io10); > >> > >> Would it be any cleaner to use io_clrset() here? > >> > >> Hrm ... also it feels like this register really should be set depending > >> upon the complete state of ... &state->... > >> > >> So perhaps it deserves it's own function which should be called after > >> csi_registered() callback and any link change. > >> > >> /me has a quick go at some psuedo codeishness...: > >> > >> int adv74x_io_10(struct adv748x_state *state); > >> u8 bits = 0; > >> u8 mask = ADV748X_IO_10_CSI1_EN > >> > >> | ADV748X_IO_10_CSI4_EN > >> | ADV748X_IO_10_CSI4_IN_SEL_AFE; > >> > >> if (state->afe.tx) { > >> /* AFE Requires TXA enabled, even when output to TXB */ > >> bits |= ADV748X_IO_10_CSI4_EN; > >> > >> if (is_txa(state->afe.tx)) > >> bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE > >> else > >> bits |= ADV748X_IO_10_CSI1_EN; > >> } > >> > >> if (state->hdmi.tx) { > >> bits |= ADV748X_IO_10_CSI4_EN; > >> } > >> > >> return io_clrset(state, ADV748X_IO_10, mask, bits); > >> } > >> > >> How does that look ? (is it even correct first?) > >> > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static const struct media_entity_operations adv748x_tx_media_ops = { > >>> + .link_setup = adv748x_link_setup, > >>> + .link_validate = v4l2_subdev_link_validate, > >>> +}; > >>> > >>> static const struct media_entity_operations adv748x_media_ops = { > >>> .link_validate = v4l2_subdev_link_validate, > >>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, > >>> struct adv748x_state *state, > >>> state->client->addr, ident); > >>> > >>> sd->entity.function = function; > >>> - sd->entity.ops = &adv748x_media_ops; > >>> + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > >>> + &adv748x_tx_media_ops : &adv748x_media_ops; > >> > >> Aha - yes that's a neat solution to ensure that only the TX links > >> generate link_setup calls :) > > > > Another option would be to bail out from adv748x_link_setup() if the entity is > > not a TX*. > > > > I suggested this in v1 - but Jacopo objected with the following: > > > Checking for is_txa() and is_txb() would require to call > > 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually > > represent a csi2_tx. I would keep it as it is. > That was at the time where the .link_setup() callback was called for TXs and non-TXs. What you proposed was to call: #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd) on variables that we don't have any guarantee that are of type 'struct adv748x_csi2'. I still think it is dangerous and should be avoided and I worked it around in v1 as: + if ((sd != &state->txa.sd && sd != &state->txb.sd) || > Now I look at the implementation here, I see this is precisely what it > is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on > an unknown pointer type > (which I still believe is a valid thing to do in this instance) It's not unknown, .link_setup() is only registered for TXs. If it gets called, we know we're dealing with a TX. > > So yes, I think this would be simpler having the check at the top of the > adv748x_link_setup() call, and thus then there is no need to add a > second adv_media_ops structure. That was what I did in v1, didn't I ? The current implementation looks better imho, but if the both of you prefer something similar to v1 I will consider that. Thanks j > > > >>> } > > > > [snip] > > > > -- > Regards > -- > Kieran
Hi Jacopo, On Thursday, 10 January 2019 10:51:00 EET Jacopo Mondi wrote: > On Wed, Jan 09, 2019 at 02:15:04AM +0200, Laurent Pinchart wrote: > > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote: > >> On 06/01/2019 15:54, Jacopo Mondi wrote: > >>> When the adv748x driver is informed about a link being created from > >>> HDMI or AFE to a CSI-2 TX output, the 'link_setup()' callback is > >>> invoked. Make sure to implement proper routing management at link setup > >>> time, to route the selected video stream to the desired TX output. > >> > >> Overall this looks like the right approach - but I feel like the > >> handling of the io10 register might need some consideration, because > >> it's value depends on the condition of both CSI2 transmitters, not just > >> the currently parsed link. > >> > >> I had a go at some pseudo - uncompiled/untested code inline as a > >> suggestion. > >> > >> If you think it's better - feel free to rework it in ... or not as you > >> see fit. > >> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> > >>> drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > >>> drivers/media/i2c/adv748x/adv748x.h | 2 + > >>> 2 files changed, 58 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c > >>> b/drivers/media/i2c/adv748x/adv748x-core.c index > >>> 200e00f93546..a586bf393558 100644 > >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c > >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c [snip] > >>> +static const struct media_entity_operations adv748x_tx_media_ops = { > >>> + .link_setup = adv748x_link_setup, > >>> + .link_validate = v4l2_subdev_link_validate, > >>> +}; > >>> > >>> static const struct media_entity_operations adv748x_media_ops = { > >>> .link_validate = v4l2_subdev_link_validate, > >>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, > >>> struct adv748x_state *state, > >>> state->client->addr, ident); > >>> > >>> sd->entity.function = function; > >>> - sd->entity.ops = &adv748x_media_ops; > >>> + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > >>> + &adv748x_tx_media_ops : &adv748x_media_ops; > >> > >> Aha - yes that's a neat solution to ensure that only the TX links > >> generate link_setup calls :) > > > > Another option would be to bail out from adv748x_link_setup() if the > > entity is not a TX*. > > If I'm not wrong you suggested me to register a set of operations with > the .link_setup callback only for TX entities, and I agree it is much > better, so I'm leaning to leave it as it is in this series. Sorry, I should have made it clear that this wasn't a request for a change, just pointing out another potential option. Your implementation is fine with me. > >>> } > > > > [snip]
Hi Kieran, On Mon, Jan 07, 2019 at 12:36:28PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 06/01/2019 15:54, Jacopo Mondi wrote: > > When the adv748x driver is informed about a link being created from HDMI or > > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > > sure to implement proper routing management at link setup time, to route > > the selected video stream to the desired TX output. > > Overall this looks like the right approach - but I feel like the > handling of the io10 register might need some consideration, because > it's value depends on the condition of both CSI2 transmitters, not just > the currently parsed link. > > I had a go at some pseudo - uncompiled/untested code inline as a suggestion. > > If you think it's better - feel free to rework it in ... or not as you > see fit. > > Regards > > Kieran > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > > drivers/media/i2c/adv748x/adv748x.h | 2 + > > 2 files changed, 58 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > > index 200e00f93546..a586bf393558 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > > /* ----------------------------------------------------------------------------- > > * Media Operations > > */ > > +static int adv748x_link_setup(struct media_entity *entity, > > + const struct media_pad *local, > > + const struct media_pad *remote, u32 flags) > > +{ > > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > > + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > + bool enable = flags & MEDIA_LNK_FL_ENABLED; > > + u8 io10; > > + > > + /* Refuse to enable multiple links to the same TX at the same time. */ > > + if (enable && tx->src) > > + return -EINVAL; > > + > > + /* Set or clear the source (HDMI or AFE) and the current TX. */ > > + if (rsd == &state->afe.sd) > > + state->afe.tx = enable ? tx : NULL; > > + else > > + state->hdmi.tx = enable ? tx : NULL; > > + > > + tx->src = enable ? rsd : NULL; > > + > > + if (!enable) > > + return 0; > > Don't we potentially want to take any action on disable to power down > links below ? > You know, I had a thought about it, and I was not sure it was worth it. The chip stays powered, and even if a TX stays active when it is not linked, this is not worse than what it was before, and considering... > > + > > + /* Change video stream routing, according to the newly enabled link. */ > > + io10 = io_read(state, ADV748X_IO_10); > > + if (rsd == &state->afe.sd) { > > + /* > > + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. > > + * if AFE goes to TXB, we need both TXA and TXB powered on. > > + */ ... this I should not simply disable, say TXA, because the HDMI->TXA link has been disabled, but I should inspect the global state everytime. > > + io10 &= ~ADV748X_IO_10_CSI1_EN; > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + if (is_txa(tx)) > > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > Shouldn't the CSI4 be enabled here too? or are we assuming it's already > (/always) enabled? > io10 |= ADV748X_IO_10_CSI4_EN; Correct, I wrongly assumed it stayed on, but it could be not. > > > + else > > + io10 |= ADV748X_IO_10_CSI4_EN | > > + ADV748X_IO_10_CSI1_EN; > > + } else { > > + /* Clear AFE->TXA routing and power up TXA. */ > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + io10 |= ADV748X_IO_10_CSI4_EN; > > But if we assume it's already enabled ... do we need this? > Perhaps it might be better to be explicit on this? > > > + } > > + io_write(state, ADV748X_IO_10, io10); > > > Would it be any cleaner to use io_clrset() here? Not sure if it is cleaner, but I checked if it was safe to write the whole IO_10 register, and the only fields not touched by this are the ones controlling output redirection to the TTL port, which should stay zeros. Bits [1:0] are not documented though.. > > Hrm ... also it feels like this register really should be set depending > upon the complete state of ... &state->... > Possibly, that would help catching cases where a TX (or both TXs) could be shut down, if we consider it worthy. > So perhaps it deserves it's own function which should be called after > csi_registered() callback and any link change. > Not sure if it makes sense to touch this register when a subdevice gets registered... we create links at that time, but they're not enabled, so the only place where this configuration changes is actually this function. I'm not sure it is worth breaking this out, but I agree the full state should probably be inspected, and not just the last changed route. I'll run some tests and see how it works. Thanks for the comments j > /me has a quick go at some psuedo codeishness...: > > int adv74x_io_10(struct adv748x_state *state); > u8 bits = 0; > u8 mask = ADV748X_IO_10_CSI1_EN > | ADV748X_IO_10_CSI4_EN > | ADV748X_IO_10_CSI4_IN_SEL_AFE; > > if (state->afe.tx) { > /* AFE Requires TXA enabled, even when output to TXB */ > bits |= ADV748X_IO_10_CSI4_EN; > > if (is_txa(state->afe.tx)) > bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE > else > bits |= ADV748X_IO_10_CSI1_EN; > } > > if (state->hdmi.tx) { > bits |= ADV748X_IO_10_CSI4_EN; > } > > return io_clrset(state, ADV748X_IO_10, mask, bits); > } > > How does that look ? (is it even correct first?) > > > + > > + return 0; > > +} > > + > > +static const struct media_entity_operations adv748x_tx_media_ops = { > > + .link_setup = adv748x_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > > > static const struct media_entity_operations adv748x_media_ops = { > > .link_validate = v4l2_subdev_link_validate, > > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state, > > state->client->addr, ident); > > > > sd->entity.function = function; > > - sd->entity.ops = &adv748x_media_ops; > > + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > > + &adv748x_tx_media_ops : &adv748x_media_ops; > > Aha - yes that's a neat solution to ensure that only the TX links > generate link_setup calls :) > > > > > } > > > > static int adv748x_parse_csi2_lanes(struct adv748x_state *state, > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > index 6eb2e4a95eed..eb19c6cbbb4e 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -93,6 +93,7 @@ struct adv748x_csi2 { > > > > #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL) > > #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab) > > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx)) > > #define is_txa(_tx) __is_tx(_tx, a) > > #define is_txb(_tx) __is_tx(_tx, b) > > > > @@ -224,6 +225,7 @@ struct adv748x_state { > > #define ADV748X_IO_10_CSI4_EN BIT(7) > > #define ADV748X_IO_10_CSI1_EN BIT(6) > > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE BIT(3) > > > > > > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > > #define ADV748X_IO_CHIP_REV_ID_2 0xe0 > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 10/01/2019 08:58, Jacopo Mondi wrote: > On Wed, Jan 09, 2019 at 02:15:33PM +0000, Kieran Bingham wrote: >> On 09/01/2019 00:15, Laurent Pinchart wrote: >>> Hello, >>> >>> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote: >>>> On 06/01/2019 15:54, Jacopo Mondi wrote: >>>>> When the adv748x driver is informed about a link being created from HDMI >>>>> or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make >>>>> sure to implement proper routing management at link setup time, to route >>>>> the selected video stream to the desired TX output. >>>> >>>> Overall this looks like the right approach - but I feel like the >>>> handling of the io10 register might need some consideration, because >>>> it's value depends on the condition of both CSI2 transmitters, not just >>>> the currently parsed link. >>>> >>>> I had a go at some pseudo - uncompiled/untested code inline as a suggestion. >>>> >>>> If you think it's better - feel free to rework it in ... or not as you >>>> see fit. >>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>> --- >>>>> >>>>> drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- >>>>> drivers/media/i2c/adv748x/adv748x.h | 2 + >>>>> 2 files changed, 58 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c >>>>> b/drivers/media/i2c/adv748x/adv748x-core.c index >>>>> 200e00f93546..a586bf393558 100644 >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c >>>>> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool >>>>> on) >>>>> /* ---------------------------------------------------------------------- >>>>> * Media Operations >>>>> */ >>>>> +static int adv748x_link_setup(struct media_entity *entity, >>>>> + const struct media_pad *local, >>>>> + const struct media_pad *remote, u32 flags) >>>>> +{ >>>>> + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); >>>>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); >>>>> + struct adv748x_state *state = v4l2_get_subdevdata(sd); >>>>> + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); >>>>> + bool enable = flags & MEDIA_LNK_FL_ENABLED; >>>>> + u8 io10; >>>>> + >>>>> + /* Refuse to enable multiple links to the same TX at the same time. */ >>>>> + if (enable && tx->src) >>>>> + return -EINVAL; >>>>> + >>>>> + /* Set or clear the source (HDMI or AFE) and the current TX. */ >>>>> + if (rsd == &state->afe.sd) >>>>> + state->afe.tx = enable ? tx : NULL; >>>>> + else >>>>> + state->hdmi.tx = enable ? tx : NULL; >>>>> + >>>>> + tx->src = enable ? rsd : NULL; >>>>> + >>>>> + if (!enable) >>>>> + return 0; >>>> >>>> Don't we potentially want to take any action on disable to power down >>>> links below ? >>>> >>>>> + >>>>> + /* Change video stream routing, according to the newly enabled link. */ >>>>> + io10 = io_read(state, ADV748X_IO_10); >>>>> + if (rsd == &state->afe.sd) { >>>>> + /* >>>>> + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. >>>>> + * if AFE goes to TXB, we need both TXA and TXB powered on. >>>>> + */ >>>>> + io10 &= ~ADV748X_IO_10_CSI1_EN; >>>>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>>> + if (is_txa(tx)) >>>>> + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>> >>>> Shouldn't the CSI4 be enabled here too? or are we assuming it's already >>>> (/always) enabled? >>>> io10 |= ADV748X_IO_10_CSI4_EN; >>>> >>>>> + else >>>>> + io10 |= ADV748X_IO_10_CSI4_EN | >>>>> + ADV748X_IO_10_CSI1_EN; >>>>> + } else { >>>>> + /* Clear AFE->TXA routing and power up TXA. */ >>>>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>>> + io10 |= ADV748X_IO_10_CSI4_EN; >>>> >>>> But if we assume it's already enabled ... do we need this? >>>> Perhaps it might be better to be explicit on this? >>>> >>>>> + } >>>>> + io_write(state, ADV748X_IO_10, io10); >>>> >>>> Would it be any cleaner to use io_clrset() here? >>>> >>>> Hrm ... also it feels like this register really should be set depending >>>> upon the complete state of ... &state->... >>>> >>>> So perhaps it deserves it's own function which should be called after >>>> csi_registered() callback and any link change. >>>> >>>> /me has a quick go at some psuedo codeishness...: >>>> >>>> int adv74x_io_10(struct adv748x_state *state); >>>> u8 bits = 0; >>>> u8 mask = ADV748X_IO_10_CSI1_EN >>>> >>>> | ADV748X_IO_10_CSI4_EN >>>> | ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>> >>>> if (state->afe.tx) { >>>> /* AFE Requires TXA enabled, even when output to TXB */ >>>> bits |= ADV748X_IO_10_CSI4_EN; >>>> >>>> if (is_txa(state->afe.tx)) >>>> bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE >>>> else >>>> bits |= ADV748X_IO_10_CSI1_EN; >>>> } >>>> >>>> if (state->hdmi.tx) { >>>> bits |= ADV748X_IO_10_CSI4_EN; >>>> } >>>> >>>> return io_clrset(state, ADV748X_IO_10, mask, bits); >>>> } >>>> >>>> How does that look ? (is it even correct first?) >>>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct media_entity_operations adv748x_tx_media_ops = { >>>>> + .link_setup = adv748x_link_setup, >>>>> + .link_validate = v4l2_subdev_link_validate, >>>>> +}; >>>>> >>>>> static const struct media_entity_operations adv748x_media_ops = { >>>>> .link_validate = v4l2_subdev_link_validate, >>>>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, >>>>> struct adv748x_state *state, >>>>> state->client->addr, ident); >>>>> >>>>> sd->entity.function = function; >>>>> - sd->entity.ops = &adv748x_media_ops; >>>>> + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? >>>>> + &adv748x_tx_media_ops : &adv748x_media_ops; >>>> >>>> Aha - yes that's a neat solution to ensure that only the TX links >>>> generate link_setup calls :) >>> >>> Another option would be to bail out from adv748x_link_setup() if the entity is >>> not a TX*. >>> >> >> I suggested this in v1 - but Jacopo objected with the following: >> >>> Checking for is_txa() and is_txb() would require to call >>> 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually >>> represent a csi2_tx. I would keep it as it is. >> > > That was at the time where the .link_setup() callback was called for > TXs and non-TXs. What you proposed was to call: > > #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd) > > on variables that we don't have any guarantee that are of type 'struct > adv748x_csi2'. I still think it is dangerous and should be avoided and > I worked it around in v1 as: > > + if ((sd != &state->txa.sd && sd != &state->txb.sd) || > >> Now I look at the implementation here, I see this is precisely what it >> is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on >> an unknown pointer type >> (which I still believe is a valid thing to do in this instance) > > It's not unknown, .link_setup() is only registered for TXs. If it gets > called, we know we're dealing with a TX. > >> >> So yes, I think this would be simpler having the check at the top of the >> adv748x_link_setup() call, and thus then there is no need to add a >> second adv_media_ops structure. > > That was what I did in v1, didn't I ? > > The current implementation looks better imho, but if the both of you > prefer something similar to v1 I will consider that. Given the extra clarification above, I'll not object to keeping it this way. I still think it's fine to use container of and then check the pointers for failure. The is_tx() would perform the type-validation :) - but lets stick with the one that you prefer. Its your patch :) -- Kieran
Hi Kieran, On Mon, Jan 07, 2019 at 12:36:28PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 06/01/2019 15:54, Jacopo Mondi wrote: > > When the adv748x driver is informed about a link being created from HDMI or > > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > > sure to implement proper routing management at link setup time, to route > > the selected video stream to the desired TX output. > > Overall this looks like the right approach - but I feel like the > handling of the io10 register might need some consideration, because > it's value depends on the condition of both CSI2 transmitters, not just > the currently parsed link. > > I had a go at some pseudo - uncompiled/untested code inline as a suggestion. > > If you think it's better - feel free to rework it in ... or not as you > see fit. > > Regards > > Kieran > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- > > drivers/media/i2c/adv748x/adv748x.h | 2 + > > 2 files changed, 58 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > > index 200e00f93546..a586bf393558 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > > /* ----------------------------------------------------------------------------- > > * Media Operations > > */ > > +static int adv748x_link_setup(struct media_entity *entity, > > + const struct media_pad *local, > > + const struct media_pad *remote, u32 flags) > > +{ > > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > > + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > + bool enable = flags & MEDIA_LNK_FL_ENABLED; > > + u8 io10; > > + > > + /* Refuse to enable multiple links to the same TX at the same time. */ > > + if (enable && tx->src) > > + return -EINVAL; > > + > > + /* Set or clear the source (HDMI or AFE) and the current TX. */ > > + if (rsd == &state->afe.sd) > > + state->afe.tx = enable ? tx : NULL; > > + else > > + state->hdmi.tx = enable ? tx : NULL; > > + > > + tx->src = enable ? rsd : NULL; > > + > > + if (!enable) > > + return 0; > > Don't we potentially want to take any action on disable to power down > links below ? > > > + > > + /* Change video stream routing, according to the newly enabled link. */ > > + io10 = io_read(state, ADV748X_IO_10); > > + if (rsd == &state->afe.sd) { > > + /* > > + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. > > + * if AFE goes to TXB, we need both TXA and TXB powered on. > > + */ > > + io10 &= ~ADV748X_IO_10_CSI1_EN; > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + if (is_txa(tx)) > > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > Shouldn't the CSI4 be enabled here too? or are we assuming it's already > (/always) enabled? > io10 |= ADV748X_IO_10_CSI4_EN; > > > + else > > + io10 |= ADV748X_IO_10_CSI4_EN | > > + ADV748X_IO_10_CSI1_EN; > > + } else { > > + /* Clear AFE->TXA routing and power up TXA. */ > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + io10 |= ADV748X_IO_10_CSI4_EN; > > But if we assume it's already enabled ... do we need this? > Perhaps it might be better to be explicit on this? > > > + } > > + io_write(state, ADV748X_IO_10, io10); > > > Would it be any cleaner to use io_clrset() here? > > Hrm ... also it feels like this register really should be set depending > upon the complete state of ... &state->... > > So perhaps it deserves it's own function which should be called after > csi_registered() callback and any link change. > > /me has a quick go at some psuedo codeishness...: > > int adv74x_io_10(struct adv748x_state *state); > u8 bits = 0; > u8 mask = ADV748X_IO_10_CSI1_EN > | ADV748X_IO_10_CSI4_EN > | ADV748X_IO_10_CSI4_IN_SEL_AFE; > > if (state->afe.tx) { > /* AFE Requires TXA enabled, even when output to TXB */ > bits |= ADV748X_IO_10_CSI4_EN; > > if (is_txa(state->afe.tx)) > bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE > else > bits |= ADV748X_IO_10_CSI1_EN; > } > > if (state->hdmi.tx) { > bits |= ADV748X_IO_10_CSI4_EN; > } > > return io_clrset(state, ADV748X_IO_10, mask, bits); > } > > How does that look ? (is it even correct first?) > Thanks, I've now updated the implementation to use something very similar to what you proposed here! Thanks j > > + > > + return 0; > > +} > > + > > +static const struct media_entity_operations adv748x_tx_media_ops = { > > + .link_setup = adv748x_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > > > static const struct media_entity_operations adv748x_media_ops = { > > .link_validate = v4l2_subdev_link_validate, > > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state, > > state->client->addr, ident); > > > > sd->entity.function = function; > > - sd->entity.ops = &adv748x_media_ops; > > + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? > > + &adv748x_tx_media_ops : &adv748x_media_ops; > > Aha - yes that's a neat solution to ensure that only the TX links > generate link_setup calls :) > > > > > } > > > > static int adv748x_parse_csi2_lanes(struct adv748x_state *state, > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > index 6eb2e4a95eed..eb19c6cbbb4e 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -93,6 +93,7 @@ struct adv748x_csi2 { > > > > #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL) > > #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab) > > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx)) > > #define is_txa(_tx) __is_tx(_tx, a) > > #define is_txb(_tx) __is_tx(_tx, b) > > > > @@ -224,6 +225,7 @@ struct adv748x_state { > > #define ADV748X_IO_10_CSI4_EN BIT(7) > > #define ADV748X_IO_10_CSI1_EN BIT(6) > > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE BIT(3) > > > > > > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > > #define ADV748X_IO_CHIP_REV_ID_2 0xe0 > > > > -- > Regards > -- > Kieran
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c index 200e00f93546..a586bf393558 100644 --- a/drivers/media/i2c/adv748x/adv748x-core.c +++ b/drivers/media/i2c/adv748x/adv748x-core.c @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) /* ----------------------------------------------------------------------------- * Media Operations */ +static int adv748x_link_setup(struct media_entity *entity, + const struct media_pad *local, + const struct media_pad *remote, u32 flags) +{ + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); + struct adv748x_state *state = v4l2_get_subdevdata(sd); + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); + bool enable = flags & MEDIA_LNK_FL_ENABLED; + u8 io10; + + /* Refuse to enable multiple links to the same TX at the same time. */ + if (enable && tx->src) + return -EINVAL; + + /* Set or clear the source (HDMI or AFE) and the current TX. */ + if (rsd == &state->afe.sd) + state->afe.tx = enable ? tx : NULL; + else + state->hdmi.tx = enable ? tx : NULL; + + tx->src = enable ? rsd : NULL; + + if (!enable) + return 0; + + /* Change video stream routing, according to the newly enabled link. */ + io10 = io_read(state, ADV748X_IO_10); + if (rsd == &state->afe.sd) { + /* + * Set AFE->TXA routing and power off TXB if AFE goes to TXA. + * if AFE goes to TXB, we need both TXA and TXB powered on. + */ + io10 &= ~ADV748X_IO_10_CSI1_EN; + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; + if (is_txa(tx)) + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; + else + io10 |= ADV748X_IO_10_CSI4_EN | + ADV748X_IO_10_CSI1_EN; + } else { + /* Clear AFE->TXA routing and power up TXA. */ + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; + io10 |= ADV748X_IO_10_CSI4_EN; + } + io_write(state, ADV748X_IO_10, io10); + + return 0; +} + +static const struct media_entity_operations adv748x_tx_media_ops = { + .link_setup = adv748x_link_setup, + .link_validate = v4l2_subdev_link_validate, +}; static const struct media_entity_operations adv748x_media_ops = { .link_validate = v4l2_subdev_link_validate, @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state, state->client->addr, ident); sd->entity.function = function; - sd->entity.ops = &adv748x_media_ops; + sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ? + &adv748x_tx_media_ops : &adv748x_media_ops; } static int adv748x_parse_csi2_lanes(struct adv748x_state *state, diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h index 6eb2e4a95eed..eb19c6cbbb4e 100644 --- a/drivers/media/i2c/adv748x/adv748x.h +++ b/drivers/media/i2c/adv748x/adv748x.h @@ -93,6 +93,7 @@ struct adv748x_csi2 { #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL) #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab) +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx)) #define is_txa(_tx) __is_tx(_tx, a) #define is_txb(_tx) __is_tx(_tx, b) @@ -224,6 +225,7 @@ struct adv748x_state { #define ADV748X_IO_10_CSI4_EN BIT(7) #define ADV748X_IO_10_CSI1_EN BIT(6) #define ADV748X_IO_10_PIX_OUT_EN BIT(5) +#define ADV748X_IO_10_CSI4_IN_SEL_AFE BIT(3) #define ADV748X_IO_CHIP_REV_ID_1 0xdf #define ADV748X_IO_CHIP_REV_ID_2 0xe0
When the adv748x driver is informed about a link being created from HDMI or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make sure to implement proper routing management at link setup time, to route the selected video stream to the desired TX output. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++- drivers/media/i2c/adv748x/adv748x.h | 2 + 2 files changed, 58 insertions(+), 1 deletion(-)