Hi Laurent, Kieran,
thanks for the review
On Thu, Dec 13, 2018 at 11:40:00AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tuesday, 11 December 2018 17:16:13 EET 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.
> > + */
>
> Why don't you implement the link handler just for the TX entities then ?
>
Done. Much better
> > + if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> > + !(flags & MEDIA_LNK_FL_ENABLED))
>
> When disabling the link you should reset the ->source and ->tx pointers.
>
right
> > + return 0;
> > + 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;
>
> You can simplify this by checking if tx->source == NULL (after resetting tx-
> >source when disabling the link of course).
>
> > + /* 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);
>
> Is it guaranteed that the chip will be powered on at this point ? How about
> writing the register at stream on time instead ?
You know, I struggle to find a better place where to do so...
- Right now the register gets written at reset time only
- s_stream: Each subdev (afe, hdmi, txa and txb) has its own s_stream
so it's hard to find a centralized place where to write this
register that impacts both TXes
- afe/hdmi s_stream() ops call tx_power_up, but again this is per-TX
- There doesn't seems to be much concern in the driver in
synchronizing register writes with power states already actually. Not
that's an excuse, but when I played around and tried to power off
the chip's PLLs for real I got into LP-11 state detection issues
(again :) I would leave it here if that's fine with you :)
Thanks
j
>
> > + 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
> >
> > #define ADV748X_IO_CHIP_REV_ID_1 0xdf
> > #define ADV748X_IO_CHIP_REV_ID_2 0xe0
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
signature.asc
Description: PGP signature
