Re: [PATCH v2 6/6] media: adv748x: Implement TX link_setup callback

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: kieran.bingham@ideasonboard.com,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	niklas.soderlund+renesas@ragnatech.se,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2 6/6] media: adv748x: Implement TX link_setup callback
Date: Thu, 10 Jan 2019 11:27:57 +0200	[thread overview]
Message-ID: <1672065.jg9h0FcvVr@avalon> (raw)
In-Reply-To: <20190110085100.l4dwjxdkx23jfhfg@uno.localdomain>

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]

-- 
Regards,

Laurent Pinchart