On 09/05/2017 03:05 PM, 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 to the
to -> in > 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 master notifier's complete callback is only called when all sub-device > notifiers are completed. > > Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 209 > ++++++++++++++++++++++++++++++----- > include/media/v4l2-async.h | 16 ++- > 2 files changed, 194 insertions(+), 31 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > b/drivers/media/v4l2-core/v4l2-async.c > index 79f216723a3f..620b2cd29fc3 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -53,6 +53,10 @@ static int v4l2_async_notifier_call_complete(struct > v4l2_async_notifier *n) > return n->ops->complete(n); > } > > +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *sd, > + struct v4l2_async_subdev *asd); > + > static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > #if IS_ENABLED(CONFIG_I2C) > @@ -129,14 +133,119 @@ static struct v4l2_async_subdev *v4l2_async_find_match( > return NULL; > } > > +/* Get the sub-device notifier registered by a sub-device driver. */ > +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier( > + struct v4l2_subdev *sd) > +{ > + struct v4l2_async_notifier *n; > + > + list_for_each_entry(n, ¬ifier_list, list) > + if (n->sd == sd) > + return n; > + > + return NULL; > +} > + > +/* Return true if all sub-device notifiers are complete, false otherwise. */ > +static bool v4l2_async_subdev_notifiers_complete( > + struct v4l2_async_notifier *notifier) > +{ > + struct v4l2_subdev *sd; > + > + if (!list_empty(¬ifier->waiting)) > + return false; > + > + list_for_each_entry(sd, ¬ifier->done, async_list) { > + struct v4l2_async_notifier *subdev_notifier = > + v4l2_async_get_subdev_notifier(sd); > + > + if (!subdev_notifier) > + continue; > + > + if (!v4l2_async_subdev_notifiers_complete(subdev_notifier)) > + return false; > + } > + > + return true; > +} > + > +/* Get v4l2_device related to the notifier if one can be found. */ > +static struct v4l2_device *v4l2_async_notifier_get_v4l2_dev( > + struct v4l2_async_notifier *notifier) > +{ > + while (notifier->master) > + notifier = notifier->master; > + > + return notifier->v4l2_dev; > +} > + > +/* 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_subdev *sd, *tmp; > + > + if (!v4l2_async_notifier_get_v4l2_dev(notifier)) > + return 0; > + > + 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, sd, asd); > + if (ret < 0) > + return ret; > + } > + > + return 0; > +} > + > +/* Try completing a notifier. */ > +static int v4l2_async_notifier_try_complete( > + struct v4l2_async_notifier *notifier) > +{ > + do { > + int ret; > + > + /* Any local async sub-devices left? */ > + if (!list_empty(¬ifier->waiting)) > + return 0; > + > + /* > + * Any sub-device notifiers waiting for async subdevs > + * to be bound? > + */ > + if (!v4l2_async_subdev_notifiers_complete(notifier)) > + return 0; > + > + /* Proceed completing the notifier */ > + ret = v4l2_async_notifier_call_complete(notifier); > + if (ret < 0) > + return ret; > + > + /* > + * Obtain notifier's master. If there is one, repeat > + * the process, otherwise we're done here. > + */ > + } while ((notifier = notifier->master)); I'd change this to: notifier = notifier->master; } while (notifier); > + > + return 0; > +} > + > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, > struct v4l2_async_subdev *asd) > { > + struct v4l2_async_notifier *subdev_notifier; > int ret; > > - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); > - if (ret < 0) > + ret = v4l2_device_register_subdev( > + v4l2_async_notifier_get_v4l2_dev(notifier), sd); > + if (ret) > return ret; > > ret = v4l2_async_notifier_call_bound(notifier, sd, asd); > @@ -153,10 +262,20 @@ static int v4l2_async_match_notify(struct > v4l2_async_notifier *notifier, > /* Move from the global subdevice list to notifier's done */ > list_move(&sd->async_list, ¬ifier->done); > > - if (list_empty(¬ifier->waiting)) > - return v4l2_async_notifier_call_complete(notifier); > + /* > + * See if the sub-device has a notifier. If it does, proceed > + * with checking for its async sub-devices. > + */ > + subdev_notifier = v4l2_async_get_subdev_notifier(sd); > + if (subdev_notifier && !subdev_notifier->master) { > + subdev_notifier->master = notifier; > + ret = v4l2_async_notifier_try_all_subdevs(subdev_notifier); > + if (ret) > + return ret; > + } > > - return 0; > + /* Try completing the notifier and its master(s). */ > + return v4l2_async_notifier_try_complete(notifier); > } > > static void v4l2_async_cleanup(struct v4l2_subdev *sd) > @@ -168,18 +287,17 @@ 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 || > + if (!notifier->v4l2_dev == !notifier->sd || !notifier->num_subdevs || With the changes suggested below this can be changed to: if (!notifier->num_subdevs || However, I have a question about that: why would it be wrong to call this with no subdevs in the list? It's perfectly valid to have no subdevs at all. There may be a fixed incoming video stream that is not controlled by a subdev. We have a case like that in fact. > notifier->num_subdevs > V4L2_MAX_SUBDEVS) > return -EINVAL; > > - notifier->v4l2_dev = v4l2_dev; > + INIT_LIST_HEAD(¬ifier->list); > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > > @@ -203,18 +321,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 */ > @@ -224,28 +334,67 @@ 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 (!v4l2_dev) I'd change this to: if (!v4l2_dev || notifier->sd) > + 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) > +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd, > + struct v4l2_async_notifier *notifier) > { > - struct v4l2_subdev *sd, *tmp; > + if (!sd) and this to: if (!sd || notifier->v4l2_dev) > + return -EINVAL; > > - if (!notifier->v4l2_dev) > - return; > + notifier->sd = sd; > > - mutex_lock(&list_lock); > + return __v4l2_async_notifier_register(notifier); > +} > +EXPORT_SYMBOL(v4l2_async_subdev_notifier_register); > > - list_del(¬ifier->list); > +/* 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, ¬ifier->done, async_list) { > + struct v4l2_async_notifier *subdev_notifier = > + v4l2_async_get_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); > + > + list_del(&sd->async_list); > + list_add(&sd->async_list, &subdev_list); > } > > - mutex_unlock(&list_lock); > + notifier->master = NULL; > +} > + > +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > +{ > + if (!notifier->v4l2_dev && !notifier->sd) > + return; > > - notifier->v4l2_dev = NULL; > + mutex_lock(&list_lock); > + > + v4l2_async_notifier_unbind_all_subdevs(notifier); > + > + list_del(¬ifier->list); > + > + mutex_unlock(&list_lock); > } > EXPORT_SYMBOL(v4l2_async_notifier_unregister); > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 3bc8a7c0d83f..12739be44bd1 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -102,7 +102,9 @@ struct v4l2_async_notifier_operations { > * @num_subdevs: number of subdevices used in the subdevs array > * @max_subdevs: number of subdevices allocated in the subdevs array > * @subdevs: array of pointers to subdevice descriptors > - * @v4l2_dev: pointer to struct v4l2_device > + * @v4l2_dev: v4l2_device of the master, for subdev notifiers NULL > + * @sd: sub-device that registered the notifier, NULL otherwise > + * @master: master notifier carrying @v4l2_dev I think this description is out of date. It is really the parent notifier, right? Should 'master' be renamed to 'parent'? Same problem with the description of @v4l2_dev: it's the v4l2_device of the root/top-level notifier. > * @waiting: list of struct v4l2_async_subdev, waiting for their drivers > * @done: list of struct v4l2_subdev, already probed > * @list: member in a global list of notifiers > @@ -113,6 +115,8 @@ struct v4l2_async_notifier { > unsigned int max_subdevs; > struct v4l2_async_subdev **subdevs; > struct v4l2_device *v4l2_dev; > + struct v4l2_subdev *sd; > + struct v4l2_async_notifier *master; > struct list_head waiting; > struct list_head done; > struct list_head list; > @@ -128,6 +132,16 @@ int v4l2_async_notifier_register(struct v4l2_device > *v4l2_dev, > struct v4l2_async_notifier *notifier); > > /** > + * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous > + * notifier for a sub-device > + * > + * @sd: pointer to &struct v4l2_subdev > + * @notifier: pointer to &struct v4l2_async_notifier > + */ > +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd, > + struct v4l2_async_notifier *notifier); > + > +/** > * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous > notifier > * > * @notifier: pointer to &struct v4l2_async_notifier > This v8 is much better and is getting close. Thanks! Hans