Re: [PATCH v14 14/28] v4l: async: Prepare for async sub-device notifiers

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 26/09/17 10:17, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Sep 26, 2017 at 10:12:50AM +0200, Hans Verkuil wrote:
>> On 26/09/17 00:25, Sakari Ailus wrote:
>>> Refactor the V4L2 async framework a little in preparation for async
>>> sub-device notifiers.
>>
>> Perhaps extend this a bit to also state the reason for these changes?
> 
> Well, the true reason is that Laurent felt the following patch was doing
> too much, but most of the changes in it are actually tightly
> interconnected.
> 
> How about adding:
> 
> This avoids making some structural changes in the patch actually
> implementing sub-device notifiers, making that patch easier to review.

That will do.

I preferred the original version, that way I could see WHY things were done,
even though the patch was larger.

Regards,

	Hans

> 
>>
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>
>> Anyway,
>>
>> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> 
> Thanks!
> 
>>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++++++++++++++++++-----------
>>>  1 file changed, 47 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index 77b9f851bfa9..1d4132305243 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>>>  }
>>>  
>>>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>>> +				   struct v4l2_device *v4l2_dev,
>>>  				   struct v4l2_subdev *sd,
>>>  				   struct v4l2_async_subdev *asd)
>>>  {
>>>  	int ret;
>>>  
>>> -	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
>>> +	ret = v4l2_device_register_subdev(v4l2_dev, sd);
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> @@ -154,6 +155,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>>>  	return 0;
>>>  }
>>>  
>>> +/* Test all async sub-devices in a notifier for a match. */
>>> +static int v4l2_async_notifier_try_all_subdevs(
>>> +	struct v4l2_async_notifier *notifier)
>>> +{
>>> +	struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
>>> +	struct v4l2_subdev *sd, *tmp;
>>> +
>>> +	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
>>> +		struct v4l2_async_subdev *asd;
>>> +		int ret;
>>> +
>>> +		asd = v4l2_async_find_match(notifier, sd);
>>> +		if (!asd)
>>> +			continue;
>>> +
>>> +		ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd);
>>> +		if (ret < 0) {
>>> +			mutex_unlock(&list_lock);
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
>>>  {
>>>  	v4l2_device_unregister_subdev(sd);
>>> @@ -163,17 +189,15 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
>>>  	sd->dev = NULL;
>>>  }
>>>  
>>> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>>> -				 struct v4l2_async_notifier *notifier)
>>> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>>>  {
>>> -	struct v4l2_subdev *sd, *tmp;
>>>  	struct v4l2_async_subdev *asd;
>>> +	int ret;
>>>  	int i;
>>>  
>>> -	if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>>> +	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>>>  		return -EINVAL;
>>>  
>>> -	notifier->v4l2_dev = v4l2_dev;
>>>  	INIT_LIST_HEAD(&notifier->waiting);
>>>  	INIT_LIST_HEAD(&notifier->done);
>>>  
>>> @@ -206,18 +230,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>>>  
>>>  	mutex_lock(&list_lock);
>>>  
>>> -	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
>>> -		int ret;
>>> -
>>> -		asd = v4l2_async_find_match(notifier, sd);
>>> -		if (!asd)
>>> -			continue;
>>> -
>>> -		ret = v4l2_async_match_notify(notifier, sd, asd);
>>> -		if (ret < 0) {
>>> -			mutex_unlock(&list_lock);
>>> -			return ret;
>>> -		}
>>> +	ret = v4l2_async_notifier_try_all_subdevs(notifier);
>>> +	if (ret) {
>>> +		mutex_unlock(&list_lock);
>>> +		return ret;
>>>  	}
>>>  
>>>  	/* Keep also completed notifiers on the list */
>>> @@ -227,6 +243,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>>>  
>>>  	return 0;
>>>  }
>>> +
>>> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>>> +				 struct v4l2_async_notifier *notifier)
>>> +{
>>> +	if (WARN_ON(!v4l2_dev))
>>> +		return -EINVAL;
>>> +
>>> +	notifier->v4l2_dev = v4l2_dev;
>>> +
>>> +	return __v4l2_async_notifier_register(notifier);
>>> +}
>>>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>>>  
>>>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>> @@ -303,7 +330,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>>>  		struct v4l2_async_subdev *asd = v4l2_async_find_match(notifier,
>>>  								      sd);
>>>  		if (asd) {
>>> -			int ret = v4l2_async_match_notify(notifier, sd, asd);
>>> +			int ret = v4l2_async_match_notify(
>>> +				notifier, notifier->v4l2_dev, sd, asd);
>>>  			mutex_unlock(&list_lock);
>>>  			return ret;
>>>  		}
>>>
>>
>