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@xxxxxxxxxx>
> >>> ---
> >>>
> >>> 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]
--
Regards,
Laurent Pinchart
