Re: [PATCH v13 14/25] v4l: async: Allow binding notifiers to sub-devices

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

Hi Laurent,

On Tue, Sep 19, 2017 at 08:52:56PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 18:17:32 EEST Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 04:52:29PM +0300, Laurent Pinchart wrote:
> > > On Friday, 15 September 2017 17:17:13 EEST Sakari Ailus wrote:
> > > > Registering a notifier has required the knowledge of struct v4l2_device
> > >> for the reason that sub-devices generally are registered to the
> > >> v4l2_device (as well as the media device, also available through
> > >> v4l2_device).
> > >> 
> > >> This information is not available for sub-device drivers at probe time.
> > >> 
> > >> What this patch does is that it allows registering notifiers without
> > >> having v4l2_device around. Instead the sub-device pointer is stored in
> > >> the notifier. Once the sub-device of the driver that registered the
> > >> notifier is registered, the notifier will gain the knowledge of the
> > >> v4l2_device, and the binding of async sub-devices from the sub-device
> > >> driver's notifier may proceed.
> > >> 
> > >> The root notifier's complete callback is only called when all sub-device
> > >> notifiers are completed.
> > > 
> > > This is a bit hard to review, shouldn't it be split in two patches, one
> > > that refactors the functions, and another one that allows binding
> > > notifiers to subdevs ?
> > > 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >> 
> > >>  drivers/media/v4l2-core/v4l2-async.c | 218 ++++++++++++++++++++++++-----
> > >>  include/media/v4l2-async.h           |  16 ++-
> > >>  2 files changed, 203 insertions(+), 31 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > >> b/drivers/media/v4l2-core/v4l2-async.c index 4be2f16af051..52fe22b9b6b4
> > >> 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> 
> [snip]
> 
> > >> +/* Unbind all sub-devices in the notifier tree. */
> > >> +static void v4l2_async_notifier_unbind_all_subdevs(
> > >> +	struct v4l2_async_notifier *notifier)
> > >> +{
> > >> +	struct v4l2_subdev *sd, *tmp;
> > >> 
> > >>  	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
> > >> 
> > >> +		struct v4l2_async_notifier *subdev_notifier =
> > >> +			v4l2_async_find_subdev_notifier(sd);
> > >> +
> > >> +		if (subdev_notifier)
> > >> +			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> > >> +
> > >>  		v4l2_async_cleanup(sd);
> > >>  		
> > >>  		v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> > >> -	}
> > >> 
> > >> -	mutex_unlock(&list_lock);
> > >> +		list_del(&sd->async_list);
> > >> +		list_add(&sd->async_list, &subdev_list);
> > > 
> > > How about list_move() ?
> > 
> > Yeah.
> > 
> > > This seems to be new code, and by the look of it, I wonder whether it
> > > doesn't belong in the reprobing removal patch.
> > 
> > This is not related to re-probing. Here we're moving an async sub-device
> > back to the global sub-device list when its notifier is going away.
> 
> In order to make the subdev bindable again when the notifier will be re-
> registered. This wasn't needed before, as reprobing took care of that.

Ah, I see what you mean now. That the async sub-device is returned to the
global list?

I also noticed the sd shouldn't be set NULL except to the notifier this is
directly called on. I'll fix that as well.

> 
> > >> +	}
> > >> 
> > >> +	notifier->parent = NULL;
> > >> +	notifier->sd = NULL;
> > >>  	notifier->v4l2_dev = NULL;
> > >>  }
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com