Hi Sakari
On 02/02/2022 21:48, 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@xxxxxxxxx>
>>> ---
>>> 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(¬ifier->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?
Gentle ping for this part :)
>>> + *
>>> + * 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()?
>
> Sure
>
>>> +{
>>> +#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.
>
> Thank you, I'll do that in the next version
>
>>> + v4l2_device_unregister_subdev(sd);
>>> + return ret;
>>> + }
>>> +
>>> /* Remove from the waiting list */
>>> list_del(&asd->list);
>>> sd->asd = asd;
