Re: [PATCH v2 6/6] media: v4l2-async: Create links during v4l2_async_match_notify()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Daniel,

Thanks for the ping.

On Wed, Feb 02, 2022 at 09:48:56PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 02/02/2022 16:38, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > Thanks for the update.
> >
> > On Sun, Jan 30, 2022 at 11:58:21PM +0000, Daniel Scally wrote:
> >> Upon an async fwnode match, there's some typical behaviour that the
> >> notifier and matching subdev will want to do. For example, a notifier
> >> representing a sensor matching to an async subdev representing its
> >> VCM will want to create an ancillary link to expose that relationship
> >> to userspace.
> >>
> >> To avoid lots of code in individual drivers, try to build these links
> >> within v4l2 core.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> Changes since v1:
> >>
> >> 	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
> >> 	- Some spelling and nomenclature cleanup (Laurent)
> >>
> >> Changes since the rfc:
> >>
> >> 	- None
> >>
> >>  drivers/media/v4l2-core/v4l2-async.c | 56 ++++++++++++++++++++++++++++
> >>  1 file changed, 56 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >> index 0404267f1ae4..8980534e755e 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >> @@ -275,6 +275,50 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
> >>  static int
> >>  v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
> >>  
> >> +static int
> >> +__v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
> >> +				   struct v4l2_subdev *sd)
> >> +{
> >> +	struct media_link *link = NULL;
> >> +
> >> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> >> +
> >> +	if (sd->entity.function != MEDIA_ENT_F_LENS &&
> >> +	    sd->entity.function != MEDIA_ENT_F_FLASH)
> >> +		return -EINVAL;
> >> +
> >> +	link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity);
> >> +
> >> +#endif
> >> +
> >> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
> >> +}
> >> +
> >> +/*
> >> + * Create links on behalf of the notifier and subdev, where it's obvious what
> >> + * should be done. At the moment, we only support cases where the notifier
> >> + * is a camera sensor and the subdev is a lens controller.
> > I think I'd rather change this so that ancillary links are created for lens
> > and flash subdevs, independently of the function of the notifier subdev.
> >
> > Are there cases where this would go wrong currently, or in the future? I
> > can't think of any right now at least. I guess we could add more
> > information in the future to convey here if needed.
> I don't think doing that would go wrong anyhow...at least not that I
> could think of at the minute. My plan was to add a new function like
> __v4l2_async_create_data_links() and call that (from
> v4l2_async_try_create_links()) where the function of the notifier subdev
> was MEDIA_ENT_F_VID_IF_BRIDGE...you think we shouldn't be doing that? Or
> just that it should be separate?

I'm not sure the function of the subdev should be involved with this.

The function is mainly used by the user space and different drivers tend to
use different functions. Of course there could (and should) be alignment on
this, but as you can have only a single function, there are bound to be
cases where you have to pick one that fits the best but does not entirely
match what the device is.

I see no problem doing this automatically, as long as it does not clash
with what drivers create by themselves. The local pad where the data link
is connected often comes from the driver --- same for the flags btw. --- so
some way needs to be provided for the driver to provide this information.

There's a callback for connecting endpoint with a pad the transmitter
drivers use, maybe that could be helpful?

-- 
Kind regards,

Sakari Ailus