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 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.

> + *
> + * TODO: Create data links if the notifier's function is
> + * MEDIA_ENT_F_VID_IF_BRIDGE and the subdev's is MEDIA_ENT_F_CAM_SENSOR.
> + */
> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier,
> +				       struct v4l2_subdev *sd)

How about calling this v4l2_async_create_ancillary_links()?

> +{
> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> +
> +	if (!notifier->sd)
> +		return 0;
> +
> +	switch (notifier->sd->entity.function) {
> +	case MEDIA_ENT_F_CAM_SENSOR:
> +		return __v4l2_async_create_ancillary_link(notifier, sd);
> +	}
> +
> +#endif
> +	return 0;
> +}
> +
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_device *v4l2_dev,
>  				   struct v4l2_subdev *sd,
> @@ -293,6 +337,18 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  		return ret;
>  	}
>  
> +	/*
> +	 * Depending of the function of the entities involved, we may want to
> +	 * create links between them (for example between a sensor and its lens
> +	 * or between a sensor's source pad and the connected device's sink
> +	 * pad).
> +	 */
> +	ret = v4l2_async_try_create_links(notifier, sd);
> +	if (ret) {

The notifier's bound function has been called already. The unbound function
needs to be thus called here, too.

> +		v4l2_device_unregister_subdev(sd);
> +		return ret;
> +	}
> +
>  	/* Remove from the waiting list */
>  	list_del(&asd->list);
>  	sd->asd = asd;

-- 
Kind regards,

Sakari Ailus