Hi Jacopo,
On 11/12/2018 15:16, 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.
>
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 63
> +++++++++++++++++++++++++++++++-
> drivers/media/i2c/adv748x/adv748x.h | 1 +
> 2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index f3aabbccdfb5..08dc0e89b053 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,9 +335,70 @@ 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;
> + struct media_link *link;
> + u8 io10;
> +
> + /*
> + * For each link setup from [HDMI|AFE] to TX we receive two
> + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> + *
> + * Use the second notification form to make sure we're linking
> + * to a TX and find out from where, to set up routing properly.
> + */
> + if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
I'm starting to think an 'is_txb(tx)' would help clean up some code ...
Then we could do the assignment of tx above, and then this line would read
if ( (!(is_txa(tx) && !(is_txb(tx)))
|| !(flags & MEDIA_LNK_FL_ENABLED) )
It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on
non-TX SD's as they will then simply fail to match the above is_txa/is_txb.
> + !(flags & MEDIA_LNK_FL_ENABLED))
> + return 0;
Don't we need to clear some local references when disabling links?
(or actually perhaps it doesn't matter if we keep stale references in a
disabled object, because it's disabled)
> + tx = adv748x_sd_to_csi2(sd);
> +
> + /*
> + * Now that we're sure we're operating on one of the two TXs,
> + * make sure there are no enabled links ending there from
> + * either HDMI or AFE (this can only happens for TXA though).
> + */
> + if (is_txa(tx))
> + list_for_each_entry(link, &entity->links, list)
> + if (link->sink->entity == entity &&
> + link->flags & MEDIA_LNK_FL_ENABLED)
> + return -EINVAL;
> +
What does this protect?
Doesn't this code read as:
if (is TXA)
for each entity
Check all links - and if any are enabled, -EINVAL
Don't we ever want a link to be enabled on TXA?
(I must surely be mis-reading this - and it's nearly mid-night - so I'm
going to say I'm confused and it's time for me to stop and go to bed :D)
> + /* Change video stream routing, according to the newly created link. */
> + io10 = io_read(state, ADV748X_IO_10);
> + if (rsd == &state->afe.sd) {
> + state->afe.tx = tx;
> +
> + /*
> + * If AFE is routed to TXA, make sure TXB is off;
> + * If AFE goes to TXB, we need TXA powered on.
> + */
> + if (is_txa(tx)) {
> + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> + io10 &= ~ADV748X_IO_10_CSI1_EN;
> + } else {
> + io10 |= ADV748X_IO_10_CSI4_EN |
> + ADV748X_IO_10_CSI1_EN;
> + }
> + } else {
> + state->hdmi.tx = tx;
> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> + }
> + io_write(state, ADV748X_IO_10, io10);
> +
> + tx->rsd = rsd;
> +
> + return 0;
> +}
>
> static const struct media_entity_operations adv748x_media_ops = {
> - .link_validate = v4l2_subdev_link_validate,
> + .link_setup = adv748x_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> };
>
> /*
> -----------------------------------------------------------------------------
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h
> index 0ee3b8d5c795..63a17c31c169 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -220,6 +220,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 0x08
Should this be BIT(3)?
>
> #define ADV748X_IO_CHIP_REV_ID_1 0xdf
> #define ADV748X_IO_CHIP_REV_ID_2 0xe0
>
--
Regards
--
Kieran
