| Message ID | 20170911080008.21208-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS | expand |
On 09/11/2017 09:59 AM, 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. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 217 ++++++++++++++++++++++++++++++----- > include/media/v4l2-async.h | 16 ++- > 2 files changed, 202 insertions(+), 31 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 9ebc2e079d03..6f788b2e922a 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) > @@ -124,14 +128,128 @@ 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( I prefer to call this v4l2_async_find_subdev_notifier(). 'get' suggests a getter function, but this actually has to find it. I think this may have confused me during an earlier review of this code. The comment also needs updating: "Find the sub-device...". > + 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); Would it make sense to add a 'struct v4l2_async_notifier *subdev_notifier' field to struct v4l2_subdev? It's set when a subdev registers a notifier. That way you can just use sd->subdev_notifier here. I wonder if v4l2_async_get_subdev_notifier() is needed at all if you do this. > + > + 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->parent) > + notifier = notifier->parent; > + > + 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; > + > + if (!v4l2_async_notifier_get_v4l2_dev(notifier)) > + return 0; > + > +again: > + list_for_each_entry(sd, &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; > + > + /* > + * v4l2_async_match_notify() may lead to registering a > + * new notifier and thus changing the async subdevs > + * list. In order to proceed safely from here, restart > + * parsing the list from the beginning. > + */ > + goto again; > + } > + > + 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 parent. If there is one, repeat > + * the process, otherwise we're done here. > + */ > + } while ((notifier = notifier->parent)); > + > + 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); > @@ -148,10 +266,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->parent) { > + subdev_notifier->parent = notifier; > + ret = v4l2_async_notifier_try_all_subdevs(subdev_notifier); > + if (ret) > + return ret; > + } > > - return 0; > + /* Try completing the notifier and its parent(s). */ > + return v4l2_async_notifier_try_complete(notifier); > } > > static void v4l2_async_cleanup(struct v4l2_subdev *sd) > @@ -163,20 +291,18 @@ 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; > > if (!notifier->num_subdevs) > return v4l2_async_notifier_call_complete(notifier); > > - notifier->v4l2_dev = v4l2_dev; > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > > @@ -200,18 +326,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 */ > @@ -221,28 +339,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 || notifier->sd) Should this be a WARN_ON? > + 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 || notifier->v4l2_dev) Ditto. > + 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->parent = NULL; Shouldn't notifier->v4l2_dev and notifier->sd be set to NULL as well? I can't really tell. > +} > + > +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..cf409d45208c 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 root notifier, NULL otherwise > + * @sd: sub-device that registered the notifier, NULL otherwise > + * @parent: parent notifier carrying @v4l2_dev That's not correct, it only carries v4l2_dev if it is the root notifier. I think just 'parent notifier' is sufficient here. > * @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 *parent; > 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 > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2017 10:00 AM, Sakari Ailus wrote: > Add function v4l2_fwnode_reference_count() for counting external > references and v4l2_fwnode_reference_parse() for parsing them as async > sub-devices. > > This can be done on e.g. flash or lens async sub-devices that are not part > of but are associated with a sensor. > > struct v4l2_async_notifier.max_subdevs field is added to contain the > maximum number of sub-devices in a notifier to reflect the memory > allocated for the subdevs array. This paragraph appears to be out-of-date. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 47 +++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > index d978f2d714ca..4821c4989119 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -449,6 +449,53 @@ int v4l2_async_notifier_parse_fwnode_endpoints( > } > EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints); > > +static int v4l2_fwnode_reference_parse( > + struct device *dev, struct v4l2_async_notifier *notifier, > + const char *prop) > +{ > + struct fwnode_reference_args args; > + unsigned int index = 0; > + int ret; > + > + for (; !fwnode_property_get_reference_args( > + dev_fwnode(dev), prop, NULL, 0, index, &args); index++) > + fwnode_handle_put(args.fwnode); > + If nothing is found (i.e. index == 0), shouldn't you just return here? > + ret = v4l2_async_notifier_realloc(notifier, > + notifier->num_subdevs + index); > + if (ret) > + return -ENOMEM; > + > + for (ret = -ENOENT, index = 0; There is no reason for the 'ret = -ENOENT' to be in the for(), just set it before the 'for' statement. > + !fwnode_property_get_reference_args( > + dev_fwnode(dev), prop, NULL, 0, index, &args); > + index++) { > + struct v4l2_async_subdev *asd; > + > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { > + ret = -EINVAL; > + goto error; > + } > + > + asd = kzalloc(sizeof(*asd), GFP_KERNEL); > + if (!asd) { > + ret = -ENOMEM; > + goto error; > + } > + > + notifier->subdevs[notifier->num_subdevs] = asd; > + asd->match.fwnode.fwnode = args.fwnode; > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > + notifier->num_subdevs++; > + } If the loop doesn't find anything, then it still returns 0, not -ENOENT. So why set ret to ENOENT? Something weird going on here. I think you should also add a comment explaining this function. > + > + return 0; > + > +error: > + fwnode_handle_put(args.fwnode); > + return ret; > +} > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the review! On Mon, Sep 11, 2017 at 10:57:16AM +0200, Hans Verkuil wrote: > On 09/11/2017 09:59 AM, 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. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 217 ++++++++++++++++++++++++++++++----- > > include/media/v4l2-async.h | 16 ++- > > 2 files changed, 202 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 9ebc2e079d03..6f788b2e922a 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) > > @@ -124,14 +128,128 @@ 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( > > I prefer to call this v4l2_async_find_subdev_notifier(). 'get' suggests > a getter function, but this actually has to find it. I think this may have > confused me during an earlier review of this code. The comment also needs > updating: "Find the sub-device...". Yes, makes sense. Get also suggests that there would be reference counting which is not the case here. I made the corresponding change to v4l2_async_notifier_find_v4l2_dev() as well. > > > + 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); > > Would it make sense to add a 'struct v4l2_async_notifier *subdev_notifier' > field to struct v4l2_subdev? It's set when a subdev registers a notifier. > > That way you can just use sd->subdev_notifier here. > > I wonder if v4l2_async_get_subdev_notifier() is needed at all if you do > this. I thought of that, but ended up keeping the information in the notifier. As the information is already available elsewhere, I didn't end up adding a new field for the purpose. This is certainly not performance critical either. > > > + > > + 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->parent) > > + notifier = notifier->parent; > > + > > + 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; > > + > > + if (!v4l2_async_notifier_get_v4l2_dev(notifier)) > > + return 0; > > + > > +again: > > + list_for_each_entry(sd, &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; > > + > > + /* > > + * v4l2_async_match_notify() may lead to registering a > > + * new notifier and thus changing the async subdevs > > + * list. In order to proceed safely from here, restart > > + * parsing the list from the beginning. > > + */ > > + goto again; > > + } > > + > > + 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 parent. If there is one, repeat > > + * the process, otherwise we're done here. > > + */ > > + } while ((notifier = notifier->parent)); > > + > > + 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); > > @@ -148,10 +266,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->parent) { > > + subdev_notifier->parent = notifier; > > + ret = v4l2_async_notifier_try_all_subdevs(subdev_notifier); > > + if (ret) > > + return ret; > > + } > > > > - return 0; > > + /* Try completing the notifier and its parent(s). */ > > + return v4l2_async_notifier_try_complete(notifier); > > } > > > > static void v4l2_async_cleanup(struct v4l2_subdev *sd) > > @@ -163,20 +291,18 @@ 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; > > > > if (!notifier->num_subdevs) > > return v4l2_async_notifier_call_complete(notifier); > > > > - notifier->v4l2_dev = v4l2_dev; > > INIT_LIST_HEAD(¬ifier->waiting); > > INIT_LIST_HEAD(¬ifier->done); > > > > @@ -200,18 +326,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 */ > > @@ -221,28 +339,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 || notifier->sd) > > Should this be a WARN_ON? Added WARN_ON(). > > > + 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 || notifier->v4l2_dev) > > Ditto. Indeed. > > > + 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->parent = NULL; > > Shouldn't notifier->v4l2_dev and notifier->sd be set to NULL as well? > I can't really tell. Well spotted. Yes. This was actually broken by the patch; v4l2_dev used to be set NULL. Will fix. > > > +} > > + > > +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..cf409d45208c 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 root notifier, NULL otherwise > > + * @sd: sub-device that registered the notifier, NULL otherwise > > + * @parent: parent notifier carrying @v4l2_dev > > That's not correct, it only carries v4l2_dev if it is the root notifier. > I think just 'parent notifier' is sufficient here. Will change. > > > * @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 *parent; > > 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 > >
Typo in subject: interger -> integer On 09/11/2017 10:00 AM, Sakari Ailus wrote: > v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under > the device's own fwnode, Sorry, you lost me here. Which device are we talking about? > it will follow child fwnodes with the given > property -- value pair and return the resulting fwnode. property-value pair (easier readable that way). You only describe v4l2_fwnode_reference_parse_int_prop(), not v4l2_fwnode_reference_parse_int_props(). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > index 4821c4989119..56eee5bbd3b5 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse( > return ret; > } > > +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop( > + struct fwnode_handle *fwnode, const char *prop, unsigned int index, > + const char **props, unsigned int nprops) Need comments describing what this does. > +{ > + struct fwnode_reference_args fwnode_args; > + unsigned int *args = fwnode_args.args; > + struct fwnode_handle *child; > + int ret; > + > + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops, > + index, &fwnode_args); > + if (ret) > + return ERR_PTR(ret == -EINVAL ? -ENOENT : ret); Why map EINVAL to ENOENT? Needs a comment, either here or in the function description. > + > + for (fwnode = fwnode_args.fwnode; > + nprops; nprops--, fwnode = child, props++, args++) { I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args... It's hard to parse. I would make this a 'while (nprops)' and write out all the other assignments, increments and decrements. > + u32 val; > + > + fwnode_for_each_child_node(fwnode, child) { > + if (fwnode_property_read_u32(child, *props, &val)) > + continue; > + > + if (val == *args) > + break; I'm lost. This really needs comments and perhaps even an DT or ACPI example so you can see what exactly it is we're doing here. > + } > + > + fwnode_handle_put(fwnode); > + > + if (!child) { > + fwnode = ERR_PTR(-ENOENT); > + break; > + } > + } > + > + return fwnode; > +} > + > +static int v4l2_fwnode_reference_parse_int_props( > + struct device *dev, struct v4l2_async_notifier *notifier, > + const char *prop, const char **props, unsigned int nprops) Needs comments describing what this does. > +{ > + struct fwnode_handle *fwnode; > + unsigned int index = 0; > + int ret; > + > + while (!IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( > + dev_fwnode(dev), prop, index, props, > + nprops)))) { > + fwnode_handle_put(fwnode); > + index++; > + } > + > + if (PTR_ERR(fwnode) != -ENOENT) > + return PTR_ERR(fwnode); Missing 'if (index == 0)'? > + > + ret = v4l2_async_notifier_realloc(notifier, > + notifier->num_subdevs + index); > + if (ret) > + return -ENOMEM; > + > + for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( > + dev_fwnode(dev), prop, index, props, > + nprops))); ) { I'd add 'index++' in this for-loop. It's weird that it is missing. > + struct v4l2_async_subdev *asd; > + > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { > + ret = -EINVAL; > + goto error; > + } > + > + asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL); > + if (!asd) { > + ret = -ENOMEM; > + goto error; > + } > + > + notifier->subdevs[notifier->num_subdevs] = asd; > + asd->match.fwnode.fwnode = fwnode; > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > + notifier->num_subdevs++; > + > + fwnode_handle_put(fwnode); > + > + index++; > + } > + > + return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode); > + > +error: > + fwnode_handle_put(fwnode); > + return ret; > +} > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2017 10:00 AM, Sakari Ailus wrote: > Add v4l2_fwnode_parse_reference_sensor_common for parsing common > sensor properties that refer to adjacent devices such as flash or lens > driver chips. > > As this is an association only, there's little a regular driver needs to > know about these devices as such. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 35 +++++++++++++++++++++++++++++++++++ > include/media/v4l2-fwnode.h | 13 +++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > index 56eee5bbd3b5..b9e60a0e8f86 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -589,6 +589,41 @@ static int v4l2_fwnode_reference_parse_int_props( > return ret; > } > > +int v4l2_fwnode_reference_parse_sensor_common( > + struct device *dev, struct v4l2_async_notifier *notifier) > +{ > + static const char *led_props[] = { "led" }; > + static const struct { > + const char *name; > + const char **props; > + unsigned int nprops; > + } props[] = { > + { "flash-leds", led_props, ARRAY_SIZE(led_props) }, > + { "lens-focus", NULL, 0 }, > + }; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(props); i++) { > + int ret; > + > + if (props[i].props && is_acpi_node(dev_fwnode(dev))) > + ret = v4l2_fwnode_reference_parse_int_props( > + dev, notifier, props[i].name, > + props[i].props, props[i].nprops); > + else > + ret = v4l2_fwnode_reference_parse( > + dev, notifier, props[i].name); > + if (ret) { > + dev_warn(dev, "parsing property \"%s\" failed (%d)\n", > + props[i].name, ret); > + return ret; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse_sensor_common); > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > index 3819a73c3c8a..bcec1ce101dc 100644 > --- a/include/media/v4l2-fwnode.h > +++ b/include/media/v4l2-fwnode.h > @@ -257,4 +257,17 @@ int v4l2_async_notifier_parse_fwnode_endpoints( > struct v4l2_fwnode_endpoint *vep, > struct v4l2_async_subdev *asd)); > > +/** > + * v4l2_fwnode_reference_parse_sensor_common - parse common references on > + * sensors for async sub-devices > + * @dev: the device node the properties of which are parsed for references > + * @notifier: the async notifier where the async subdevs will be added > + * I think you should add a note that if this function returns 0 the caller should remember to call v4l2_async_notifier_release. That is not immediately obvious. Regards, Hans > + * Return: 0 on success > + * -ENOMEM if memory allocation failed > + * -EINVAL if property parsing failed > + */ > +int v4l2_fwnode_reference_parse_sensor_common( > + struct device *dev, struct v4l2_async_notifier *notifier); > + > #endif /* _V4L2_FWNODE_H */ > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2017 10:00 AM, Sakari Ailus wrote: > Parse async sub-devices by using > v4l2_subdev_fwnode_reference_parse_sensor_common(). > > These types devices aren't directly related to the sensor, but are > nevertheless handled by the smiapp driver due to the relationship of these > component to the main part of the camera module --- the sensor. > > This does not yet address providing the user space with information on how > to associate the sensor or lens devices but the kernel now has the > necessary information to do that. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > --- > drivers/media/i2c/smiapp/smiapp-core.c | 38 +++++++++++++++++++++++++++------- > drivers/media/i2c/smiapp/smiapp.h | 4 +++- > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c > index 700f433261d0..a65a839135d2 100644 > --- a/drivers/media/i2c/smiapp/smiapp-core.c > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > @@ -31,7 +31,7 @@ > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/smiapp.h> > -#include <linux/v4l2-mediabus.h> > +#include <media/v4l2-async.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-device.h> > > @@ -2887,17 +2887,24 @@ static int smiapp_probe(struct i2c_client *client, > v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops); > sensor->src->sd.internal_ops = &smiapp_internal_src_ops; > > + rval = v4l2_fwnode_reference_parse_sensor_common( > + &client->dev, &sensor->notifier); > + if (rval < 0) > + return rval; > + > sensor->vana = devm_regulator_get(&client->dev, "vana"); > if (IS_ERR(sensor->vana)) { > dev_err(&client->dev, "could not get regulator for vana\n"); > - return PTR_ERR(sensor->vana); > + rval = PTR_ERR(sensor->vana); > + goto out_release_async_notifier; > } > > sensor->ext_clk = devm_clk_get(&client->dev, NULL); > if (IS_ERR(sensor->ext_clk)) { > dev_err(&client->dev, "could not get clock (%ld)\n", > PTR_ERR(sensor->ext_clk)); > - return -EPROBE_DEFER; > + rval = -EPROBE_DEFER; > + goto out_release_async_notifier; > } > > rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk); > @@ -2905,17 +2912,19 @@ static int smiapp_probe(struct i2c_client *client, > dev_err(&client->dev, > "unable to set clock freq to %u\n", > sensor->hwcfg->ext_clk); > - return rval; > + goto out_release_async_notifier; > } > > sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown", > GPIOD_OUT_LOW); > - if (IS_ERR(sensor->xshutdown)) > - return PTR_ERR(sensor->xshutdown); > + if (IS_ERR(sensor->xshutdown)) { > + rval = PTR_ERR(sensor->xshutdown); > + goto out_release_async_notifier; > + } > > rval = smiapp_power_on(&client->dev); > if (rval < 0) > - return rval; > + goto out_release_async_notifier; > > rval = smiapp_identify_module(sensor); > if (rval) { > @@ -3092,9 +3101,14 @@ static int smiapp_probe(struct i2c_client *client, > if (rval < 0) > goto out_media_entity_cleanup; > > + rval = v4l2_async_subdev_notifier_register(&sensor->src->sd, > + &sensor->notifier); > + if (rval) > + goto out_media_entity_cleanup; > + > rval = v4l2_async_register_subdev(&sensor->src->sd); > if (rval < 0) > - goto out_media_entity_cleanup; > + goto out_unregister_async_notifier; > > pm_runtime_set_active(&client->dev); > pm_runtime_get_noresume(&client->dev); > @@ -3105,6 +3119,9 @@ static int smiapp_probe(struct i2c_client *client, > > return 0; > > +out_unregister_async_notifier: > + v4l2_async_notifier_unregister(&sensor->notifier); > + > out_media_entity_cleanup: > media_entity_cleanup(&sensor->src->sd.entity); > > @@ -3114,6 +3131,9 @@ static int smiapp_probe(struct i2c_client *client, > out_power_off: > smiapp_power_off(&client->dev); > > +out_release_async_notifier: > + v4l2_async_notifier_release(&sensor->notifier); > + > return rval; > } > > @@ -3124,6 +3144,8 @@ static int smiapp_remove(struct i2c_client *client) > unsigned int i; > > v4l2_async_unregister_subdev(subdev); > + v4l2_async_notifier_unregister(&sensor->notifier); > + v4l2_async_notifier_release(&sensor->notifier); > > pm_runtime_disable(&client->dev); > if (!pm_runtime_status_suspended(&client->dev)) > diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h > index f74d695018b9..be92cb5713f4 100644 > --- a/drivers/media/i2c/smiapp/smiapp.h > +++ b/drivers/media/i2c/smiapp/smiapp.h > @@ -20,9 +20,10 @@ > #define __SMIAPP_PRIV_H_ > > #include <linux/mutex.h> > +#include <media/i2c/smiapp.h> > +#include <media/v4l2-async.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-subdev.h> > -#include <media/i2c/smiapp.h> > > #include "smiapp-pll.h" > #include "smiapp-reg.h" > @@ -172,6 +173,7 @@ struct smiapp_subdev { > * struct smiapp_sensor - Main device structure > */ > struct smiapp_sensor { > + struct v4l2_async_notifier notifier; > /* > * "mutex" is used to serialise access to all fields here > * except v4l2_ctrls at the end of the struct. "mutex" is also > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2017 10:00 AM, Sakari Ailus wrote: > Parse async sub-devices by using > v4l2_subdev_fwnode_reference_parse_sensor_common(). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > --- > drivers/media/i2c/ov5670.c | 33 +++++++++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > index 6f7a1d6d2200..25970307dd75 100644 > --- a/drivers/media/i2c/ov5670.c > +++ b/drivers/media/i2c/ov5670.c > @@ -18,6 +18,7 @@ > #include <linux/pm_runtime.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > > #define OV5670_REG_CHIP_ID 0x300a > #define OV5670_CHIP_ID 0x005670 > @@ -1807,6 +1808,7 @@ static const struct ov5670_mode supported_modes[] = { > struct ov5670 { > struct v4l2_subdev sd; > struct media_pad pad; > + struct v4l2_async_notifier notifier; > > struct v4l2_ctrl_handler ctrl_handler; > /* V4L2 Controls */ > @@ -2473,11 +2475,13 @@ static int ov5670_probe(struct i2c_client *client) > return -EINVAL; > > ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > - if (!ov5670) { > - ret = -ENOMEM; > - err_msg = "devm_kzalloc() error"; > - goto error_print; > - } > + if (!ov5670) > + return -ENOMEM; > + > + ret = v4l2_fwnode_reference_parse_sensor_common( > + &client->dev, &ov5670->notifier); > + if (ret < 0) > + return ret; > > /* Initialize subdev */ > v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); > @@ -2486,7 +2490,7 @@ static int ov5670_probe(struct i2c_client *client) > ret = ov5670_identify_module(ov5670); > if (ret) { > err_msg = "ov5670_identify_module() error"; > - goto error_print; > + goto error_release_notifier; > } > > mutex_init(&ov5670->mutex); > @@ -2513,11 +2517,18 @@ static int ov5670_probe(struct i2c_client *client) > goto error_handler_free; > } > > + ret = v4l2_async_subdev_notifier_register(&ov5670->sd, > + &ov5670->notifier); > + if (ret) { > + err_msg = "can't register async notifier"; > + goto error_entity_cleanup; > + } > + > /* Async register for subdev */ > ret = v4l2_async_register_subdev(&ov5670->sd); > if (ret < 0) { > err_msg = "v4l2_async_register_subdev() error"; > - goto error_entity_cleanup; > + goto error_unregister_notifier; > } > > ov5670->streaming = false; > @@ -2533,6 +2544,9 @@ static int ov5670_probe(struct i2c_client *client) > > return 0; > > +error_unregister_notifier: > + v4l2_async_notifier_unregister(&ov5670->notifier); > + > error_entity_cleanup: > media_entity_cleanup(&ov5670->sd.entity); > > @@ -2542,7 +2556,8 @@ static int ov5670_probe(struct i2c_client *client) > error_mutex_destroy: > mutex_destroy(&ov5670->mutex); > > -error_print: > +error_release_notifier: > + v4l2_async_notifier_release(&ov5670->notifier); > dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret); > > return ret; > @@ -2554,6 +2569,8 @@ static int ov5670_remove(struct i2c_client *client) > struct ov5670 *ov5670 = to_ov5670(sd); > > v4l2_async_unregister_subdev(sd); > + v4l2_async_notifier_unregister(&ov5670->notifier); > + v4l2_async_notifier_release(&ov5670->notifier); > media_entity_cleanup(&sd->entity); > v4l2_ctrl_handler_free(sd->ctrl_handler); > mutex_destroy(&ov5670->mutex); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2017 10:00 AM, Sakari Ailus wrote: > Parse async sub-devices by using > v4l2_subdev_fwnode_reference_parse_sensor_common(). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > --- > drivers/media/i2c/ov13858.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c > index af7af0d14c69..0d60defc7492 100644 > --- a/drivers/media/i2c/ov13858.c > +++ b/drivers/media/i2c/ov13858.c > @@ -18,6 +18,7 @@ > #include <linux/pm_runtime.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > > #define OV13858_REG_VALUE_08BIT 1 > #define OV13858_REG_VALUE_16BIT 2 > @@ -1028,6 +1029,7 @@ static const struct ov13858_mode supported_modes[] = { > struct ov13858 { > struct v4l2_subdev sd; > struct media_pad pad; > + struct v4l2_async_notifier notifier; > > struct v4l2_ctrl_handler ctrl_handler; > /* V4L2 Controls */ > @@ -1715,6 +1717,11 @@ static int ov13858_probe(struct i2c_client *client, > if (!ov13858) > return -ENOMEM; > > + ret = v4l2_fwnode_reference_parse_sensor_common( > + &client->dev, &ov13858->notifier); > + if (ret < 0) > + return ret; > + > /* Initialize subdev */ > v4l2_i2c_subdev_init(&ov13858->sd, client, &ov13858_subdev_ops); > > @@ -1722,7 +1729,7 @@ static int ov13858_probe(struct i2c_client *client, > ret = ov13858_identify_module(ov13858); > if (ret) { > dev_err(&client->dev, "failed to find sensor: %d\n", ret); > - return ret; > + goto error_notifier_release; > } > > /* Set default mode to max resolution */ > @@ -1730,7 +1737,7 @@ static int ov13858_probe(struct i2c_client *client, > > ret = ov13858_init_controls(ov13858); > if (ret) > - return ret; > + goto error_notifier_release; > > /* Initialize subdev */ > ov13858->sd.internal_ops = &ov13858_internal_ops; > @@ -1746,9 +1753,14 @@ static int ov13858_probe(struct i2c_client *client, > goto error_handler_free; > } > > + ret = v4l2_async_subdev_notifier_register(&ov13858->sd, > + &ov13858->notifier); > + if (ret) > + goto error_media_entity; > + > ret = v4l2_async_register_subdev(&ov13858->sd); > if (ret < 0) > - goto error_media_entity; > + goto error_notifier_unregister; > > /* > * Device is already turned on by i2c-core with ACPI domain PM. > @@ -1761,11 +1773,17 @@ static int ov13858_probe(struct i2c_client *client, > > return 0; > > +error_notifier_unregister: > + v4l2_async_notifier_unregister(&ov13858->notifier); > + > error_media_entity: > media_entity_cleanup(&ov13858->sd.entity); > > error_handler_free: > ov13858_free_controls(ov13858); > + > +error_notifier_release: > + v4l2_async_notifier_release(&ov13858->notifier); > dev_err(&client->dev, "%s failed:%d\n", __func__, ret); > > return ret; > @@ -1777,6 +1795,8 @@ static int ov13858_remove(struct i2c_client *client) > struct ov13858 *ov13858 = to_ov13858(sd); > > v4l2_async_unregister_subdev(sd); > + v4l2_async_notifier_unregister(&ov13858->notifier); > + v4l2_async_notifier_release(&ov13858->notifier); > media_entity_cleanup(&sd->entity); > ov13858_free_controls(ov13858); > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2017 10:00 AM, Sakari Ailus wrote: > Add flash and indicator LED phandles to the sensor node. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > --- > arch/arm/boot/dts/omap3-n9.dts | 1 + > arch/arm/boot/dts/omap3-n950-n9.dtsi | 4 ++-- > arch/arm/boot/dts/omap3-n950.dts | 1 + > 3 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts > index b9e58c536afd..39e35f8b8206 100644 > --- a/arch/arm/boot/dts/omap3-n9.dts > +++ b/arch/arm/boot/dts/omap3-n9.dts > @@ -26,6 +26,7 @@ > clocks = <&isp 0>; > clock-frequency = <9600000>; > nokia,nvm-size = <(16 * 64)>; > + flash-leds = <&as3645a_flash &as3645a_indicator>; > port { > smia_1_1: endpoint { > link-frequencies = /bits/ 64 <199200000 210000000 499200000>; > diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi > index 1b0bd72945f2..12fbb3da5fce 100644 > --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi > +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi > @@ -271,14 +271,14 @@ > #size-cells = <0>; > reg = <0x30>; > compatible = "ams,as3645a"; > - flash@0 { > + as3645a_flash: flash@0 { > reg = <0x0>; > flash-timeout-us = <150000>; > flash-max-microamp = <320000>; > led-max-microamp = <60000>; > ams,input-max-microamp = <1750000>; > }; > - indicator@1 { > + as3645a_indicator: indicator@1 { > reg = <0x1>; > led-max-microamp = <10000>; > }; > diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts > index 646601a3ebd8..c354a1ed1e70 100644 > --- a/arch/arm/boot/dts/omap3-n950.dts > +++ b/arch/arm/boot/dts/omap3-n950.dts > @@ -60,6 +60,7 @@ > clocks = <&isp 0>; > clock-frequency = <9600000>; > nokia,nvm-size = <(16 * 64)>; > + flash-leds = <&as3645a_flash &as3645a_indicator>; > port { > smia_1_1: endpoint { > link-frequencies = /bits/ 64 <210000000 333600000 398400000>; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the review! On Mon, Sep 11, 2017 at 11:14:03AM +0200, Hans Verkuil wrote: > On 09/11/2017 10:00 AM, Sakari Ailus wrote: > > Add function v4l2_fwnode_reference_count() for counting external > > references and v4l2_fwnode_reference_parse() for parsing them as async > > sub-devices. > > > > This can be done on e.g. flash or lens async sub-devices that are not part > > of but are associated with a sensor. > > > > struct v4l2_async_notifier.max_subdevs field is added to contain the > > maximum number of sub-devices in a notifier to reflect the memory > > allocated for the subdevs array. > > This paragraph appears to be out-of-date. Will remove. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-fwnode.c | 47 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > > index d978f2d714ca..4821c4989119 100644 > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > @@ -449,6 +449,53 @@ int v4l2_async_notifier_parse_fwnode_endpoints( > > } > > EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints); > > > > +static int v4l2_fwnode_reference_parse( > > + struct device *dev, struct v4l2_async_notifier *notifier, > > + const char *prop) > > +{ > > + struct fwnode_reference_args args; > > + unsigned int index = 0; > > + int ret; > > + > > + for (; !fwnode_property_get_reference_args( > > + dev_fwnode(dev), prop, NULL, 0, index, &args); index++) > > + fwnode_handle_put(args.fwnode); > > + > > If nothing is found (i.e. index == 0), shouldn't you just return here? You could. yes. It would actually simplify the code, I could just return -ENOENT here. > > > + ret = v4l2_async_notifier_realloc(notifier, > > + notifier->num_subdevs + index); > > + if (ret) > > + return -ENOMEM; > > + > > + for (ret = -ENOENT, index = 0; > > There is no reason for the 'ret = -ENOENT' to be in the for(), just set it before > the 'for' statement. > > > + !fwnode_property_get_reference_args( > > + dev_fwnode(dev), prop, NULL, 0, index, &args); > > + index++) { > > + struct v4l2_async_subdev *asd; > > + > > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { > > + ret = -EINVAL; > > + goto error; > > + } > > + > > + asd = kzalloc(sizeof(*asd), GFP_KERNEL); > > + if (!asd) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + > > + notifier->subdevs[notifier->num_subdevs] = asd; > > + asd->match.fwnode.fwnode = args.fwnode; > > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > > + notifier->num_subdevs++; > > + } > > If the loop doesn't find anything, then it still returns 0, not -ENOENT. > So why set ret to ENOENT? Something weird going on here. :-) I think -ENOENT would make sense indeed if there are no entries found. > > I think you should also add a comment explaining this function. Yes. I had that in the header when it was exported, I'll add the same comments here. > > > + > > + return 0; > > + > > +error: > > + fwnode_handle_put(args.fwnode); > > + return ret; > > +} > > + > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > > >
On Mon, Sep 11, 2017 at 11:47:11AM +0200, Hans Verkuil wrote: > On 09/11/2017 10:00 AM, Sakari Ailus wrote: > > Add v4l2_fwnode_parse_reference_sensor_common for parsing common > > sensor properties that refer to adjacent devices such as flash or lens > > driver chips. > > > > As this is an association only, there's little a regular driver needs to > > know about these devices as such. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > drivers/media/v4l2-core/v4l2-fwnode.c | 35 +++++++++++++++++++++++++++++++++++ > > include/media/v4l2-fwnode.h | 13 +++++++++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > > index 56eee5bbd3b5..b9e60a0e8f86 100644 > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > @@ -589,6 +589,41 @@ static int v4l2_fwnode_reference_parse_int_props( > > return ret; > > } > > > > +int v4l2_fwnode_reference_parse_sensor_common( > > + struct device *dev, struct v4l2_async_notifier *notifier) > > +{ > > + static const char *led_props[] = { "led" }; > > + static const struct { > > + const char *name; > > + const char **props; > > + unsigned int nprops; > > + } props[] = { > > + { "flash-leds", led_props, ARRAY_SIZE(led_props) }, > > + { "lens-focus", NULL, 0 }, > > + }; > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(props); i++) { > > + int ret; > > + > > + if (props[i].props && is_acpi_node(dev_fwnode(dev))) > > + ret = v4l2_fwnode_reference_parse_int_props( > > + dev, notifier, props[i].name, > > + props[i].props, props[i].nprops); > > + else > > + ret = v4l2_fwnode_reference_parse( > > + dev, notifier, props[i].name); > > + if (ret) { > > + dev_warn(dev, "parsing property \"%s\" failed (%d)\n", > > + props[i].name, ret); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse_sensor_common); > > + > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > > index 3819a73c3c8a..bcec1ce101dc 100644 > > --- a/include/media/v4l2-fwnode.h > > +++ b/include/media/v4l2-fwnode.h > > @@ -257,4 +257,17 @@ int v4l2_async_notifier_parse_fwnode_endpoints( > > struct v4l2_fwnode_endpoint *vep, > > struct v4l2_async_subdev *asd)); > > > > +/** > > + * v4l2_fwnode_reference_parse_sensor_common - parse common references on > > + * sensors for async sub-devices > > + * @dev: the device node the properties of which are parsed for references > > + * @notifier: the async notifier where the async subdevs will be added > > + * > > I think you should add a note that if this function returns 0 the > caller should remember to call v4l2_async_notifier_release. That is not > immediately obvious. I'll add that, plus a note to v4l2_async_notifier_release() as well.
On Mon 2017-09-11 11:00:08, Sakari Ailus wrote: > Add flash and indicator LED phandles to the sensor node. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Pavel Machek <pavel@ucw.cz>
On Mon 2017-09-11 11:00:03, Sakari Ailus wrote: > Add v4l2_fwnode_parse_reference_sensor_common for parsing common > sensor properties that refer to adjacent devices such as flash or lens > driver chips. > > As this is an association only, there's little a regular driver needs to > know about these devices as such. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 35 +++++++++++++++++++++++++++++++++++ > include/media/v4l2-fwnode.h | 13 +++++++++++++ > 2 files changed, 48 insertions(+) > > > +/** > + * v4l2_fwnode_reference_parse_sensor_common - parse common references on > + * sensors for async sub-devices > + * @dev: the device node the properties of which are parsed for references > + * @notifier: the async notifier where the async subdevs will be added > + * > + * Return: 0 on success > + * -ENOMEM if memory allocation failed > + * -EINVAL if property parsing failed > + */ Returns: would sound more correct to me, but it seems kernel is split on that. Acked-by: Pavel Machek <pavel@ucw.cz>
Hi Hans, Thanks for the review. On Mon, Sep 11, 2017 at 11:38:58AM +0200, Hans Verkuil wrote: > Typo in subject: interger -> integer > > On 09/11/2017 10:00 AM, Sakari Ailus wrote: > > v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under > > the device's own fwnode, > > Sorry, you lost me here. Which device are we talking about? The fwnode related a struct device, in other words what dev_fwnode(dev) gives you. This is either struct device.fwnode or struct device.of_node.fwnode, depending on which firmware interface was used to create the device. I'll add a note of this. > > > it will follow child fwnodes with the given > > property -- value pair and return the resulting fwnode. > > property-value pair (easier readable that way). > > You only describe v4l2_fwnode_reference_parse_int_prop(), not > v4l2_fwnode_reference_parse_int_props(). Yes, I think I changed the naming but forgot to update the commit. I'll do that now. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 93 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > > index 4821c4989119..56eee5bbd3b5 100644 > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse( > > return ret; > > } > > > > +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop( > > + struct fwnode_handle *fwnode, const char *prop, unsigned int index, > > + const char **props, unsigned int nprops) > > Need comments describing what this does. Yes. I'll also rename it (get -> read) for consistency with the async changes. > > > +{ > > + struct fwnode_reference_args fwnode_args; > > + unsigned int *args = fwnode_args.args; > > + struct fwnode_handle *child; > > + int ret; > > + > > + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops, > > + index, &fwnode_args); > > + if (ret) > > + return ERR_PTR(ret == -EINVAL ? -ENOENT : ret); > > Why map EINVAL to ENOENT? Needs a comment, either here or in the function description. fwnode_property_get_reference_args() returns currently a little bit different error codes in ACPI / DT. This is worth documenting there and fixing as well. > > > + > > + for (fwnode = fwnode_args.fwnode; > > + nprops; nprops--, fwnode = child, props++, args++) { > > I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args... > It's hard to parse. Hmm. I'm not sure if that really helps; the function is just handling each entry in the array and related array pointers are changed accordingly. The fwnode = child assignment is there to move to the child node. I.e. what you need for handling the loop itself. I can change this though if you think it really makes a difference for better. > > I would make this a 'while (nprops)' and write out all the other assignments, > increments and decrements. > > > + u32 val; > > + > > + fwnode_for_each_child_node(fwnode, child) { > > + if (fwnode_property_read_u32(child, *props, &val)) > > + continue; > > + > > + if (val == *args) > > + break; > > I'm lost. This really needs comments and perhaps even an DT or ACPI example > so you can see what exactly it is we're doing here. I'll add comments to the code. A good example will be ACPI documentation for LEDs, see 17th patch in v9. That will go through the linux-pm tree so it won't be available in the same tree for a while. > > > + } > > + > > + fwnode_handle_put(fwnode); > > + > > + if (!child) { > > + fwnode = ERR_PTR(-ENOENT); > > + break; > > + } > > + } > > + > > + return fwnode; > > +} > > + > > +static int v4l2_fwnode_reference_parse_int_props( > > + struct device *dev, struct v4l2_async_notifier *notifier, > > + const char *prop, const char **props, unsigned int nprops) > > Needs comments describing what this does. Will add. > > > +{ > > + struct fwnode_handle *fwnode; > > + unsigned int index = 0; > > + int ret; > > + > > + while (!IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( > > + dev_fwnode(dev), prop, index, props, > > + nprops)))) { > > + fwnode_handle_put(fwnode); > > + index++; > > + } > > + > > + if (PTR_ERR(fwnode) != -ENOENT) > > + return PTR_ERR(fwnode); > > Missing 'if (index == 0)'? Yes, will add. > > > + > > + ret = v4l2_async_notifier_realloc(notifier, > > + notifier->num_subdevs + index); > > + if (ret) > > + return -ENOMEM; > > + > > + for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( > > + dev_fwnode(dev), prop, index, props, > > + nprops))); ) { > > I'd add 'index++' in this for-loop. It's weird that it is missing. Agreed, I'll move it there. > > > + struct v4l2_async_subdev *asd; > > + > > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { > > + ret = -EINVAL; > > + goto error; > > + } > > + > > + asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL); > > + if (!asd) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + > > + notifier->subdevs[notifier->num_subdevs] = asd; > > + asd->match.fwnode.fwnode = fwnode; > > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > > + notifier->num_subdevs++; > > + > > + fwnode_handle_put(fwnode); > > + > > + index++; > > + } > > + > > + return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode); > > + > > +error: > > + fwnode_handle_put(fwnode); > > + return ret; > > +} > > + > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > >
On 09/11/2017 02:28 PM, Sakari Ailus wrote: > Hi Hans, > > Thanks for the review. > > On Mon, Sep 11, 2017 at 11:38:58AM +0200, Hans Verkuil wrote: >> Typo in subject: interger -> integer >> >> On 09/11/2017 10:00 AM, Sakari Ailus wrote: >>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under >>> the device's own fwnode, >> >> Sorry, you lost me here. Which device are we talking about? > > The fwnode related a struct device, in other words what dev_fwnode(dev) > gives you. This is either struct device.fwnode or struct > device.of_node.fwnode, depending on which firmware interface was used to > create the device. > > I'll add a note of this. > >> >>> it will follow child fwnodes with the given >>> property -- value pair and return the resulting fwnode. >> >> property-value pair (easier readable that way). >> >> You only describe v4l2_fwnode_reference_parse_int_prop(), not >> v4l2_fwnode_reference_parse_int_props(). > > Yes, I think I changed the naming but forgot to update the commit. I'll do > that now. > >> >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 93 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c >>> index 4821c4989119..56eee5bbd3b5 100644 >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c >>> @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse( >>> return ret; >>> } >>> >>> +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop( >>> + struct fwnode_handle *fwnode, const char *prop, unsigned int index, >>> + const char **props, unsigned int nprops) >> >> Need comments describing what this does. > > Yes. I'll also rename it (get -> read) for consistency with the async > changes. Which async changes? Since the fwnode_handle that's returned is refcounted I wonder if 'get' isn't the right name in this case. > >> >>> +{ >>> + struct fwnode_reference_args fwnode_args; >>> + unsigned int *args = fwnode_args.args; >>> + struct fwnode_handle *child; >>> + int ret; >>> + >>> + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops, >>> + index, &fwnode_args); >>> + if (ret) >>> + return ERR_PTR(ret == -EINVAL ? -ENOENT : ret); >> >> Why map EINVAL to ENOENT? Needs a comment, either here or in the function description. > > fwnode_property_get_reference_args() returns currently a little bit > different error codes in ACPI / DT. This is worth documenting there and > fixing as well. > >> >>> + >>> + for (fwnode = fwnode_args.fwnode; >>> + nprops; nprops--, fwnode = child, props++, args++) { >> >> I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args... >> It's hard to parse. > > Hmm. I'm not sure if that really helps; the function is just handling each > entry in the array and related array pointers are changed accordingly. The > fwnode = child assignment is there to move to the child node. I.e. what you > need for handling the loop itself. > > I can change this though if you think it really makes a difference for > better. I think so, yes. I noticed you like complex for-loops :-) > >> >> I would make this a 'while (nprops)' and write out all the other assignments, >> increments and decrements. >> >>> + u32 val; >>> + >>> + fwnode_for_each_child_node(fwnode, child) { >>> + if (fwnode_property_read_u32(child, *props, &val)) >>> + continue; >>> + >>> + if (val == *args) >>> + break; >> >> I'm lost. This really needs comments and perhaps even an DT or ACPI example >> so you can see what exactly it is we're doing here. > > I'll add comments to the code. A good example will be ACPI documentation > for LEDs, see 17th patch in v9. That will go through the linux-pm tree so > it won't be available in the same tree for a while. Ideally an ACPI and an equivalent DT example would be nice to have, but I might be asking too much. I'm not that familiar with ACPI, so for me a DT example is easier. > >> >>> + } >>> + >>> + fwnode_handle_put(fwnode); >>> + >>> + if (!child) { >>> + fwnode = ERR_PTR(-ENOENT); >>> + break; >>> + } >>> + } >>> + >>> + return fwnode; >>> +} >>> + >>> +static int v4l2_fwnode_reference_parse_int_props( >>> + struct device *dev, struct v4l2_async_notifier *notifier, >>> + const char *prop, const char **props, unsigned int nprops) >> >> Needs comments describing what this does. > > Will add. > >> >>> +{ >>> + struct fwnode_handle *fwnode; >>> + unsigned int index = 0; >>> + int ret; >>> + >>> + while (!IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( >>> + dev_fwnode(dev), prop, index, props, >>> + nprops)))) { >>> + fwnode_handle_put(fwnode); >>> + index++; >>> + } >>> + >>> + if (PTR_ERR(fwnode) != -ENOENT) >>> + return PTR_ERR(fwnode); >> >> Missing 'if (index == 0)'? > > Yes, will add. > >> >>> + >>> + ret = v4l2_async_notifier_realloc(notifier, >>> + notifier->num_subdevs + index); >>> + if (ret) >>> + return -ENOMEM; >>> + >>> + for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( >>> + dev_fwnode(dev), prop, index, props, >>> + nprops))); ) { >> >> I'd add 'index++' in this for-loop. It's weird that it is missing. > > Agreed, I'll move it there. > >> >>> + struct v4l2_async_subdev *asd; >>> + >>> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { >>> + ret = -EINVAL; >>> + goto error; >>> + } >>> + >>> + asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL); >>> + if (!asd) { >>> + ret = -ENOMEM; >>> + goto error; >>> + } >>> + >>> + notifier->subdevs[notifier->num_subdevs] = asd; >>> + asd->match.fwnode.fwnode = fwnode; >>> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; >>> + notifier->num_subdevs++; >>> + >>> + fwnode_handle_put(fwnode); >>> + >>> + index++; >>> + } >>> + >>> + return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode); >>> + >>> +error: >>> + fwnode_handle_put(fwnode); >>> + return ret; >>> +} >>> + >>> MODULE_LICENSE("GPL"); >>> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); >>> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); >>> > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Mon, Sep 11, 2017 at 02:38:23PM +0200, Hans Verkuil wrote: > On 09/11/2017 02:28 PM, Sakari Ailus wrote: > > Hi Hans, > > > > Thanks for the review. > > > > On Mon, Sep 11, 2017 at 11:38:58AM +0200, Hans Verkuil wrote: > >> Typo in subject: interger -> integer > >> > >> On 09/11/2017 10:00 AM, Sakari Ailus wrote: > >>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under > >>> the device's own fwnode, > >> > >> Sorry, you lost me here. Which device are we talking about? > > > > The fwnode related a struct device, in other words what dev_fwnode(dev) > > gives you. This is either struct device.fwnode or struct > > device.of_node.fwnode, depending on which firmware interface was used to > > create the device. > > > > I'll add a note of this. > > > >> > >>> it will follow child fwnodes with the given > >>> property -- value pair and return the resulting fwnode. > >> > >> property-value pair (easier readable that way). > >> > >> You only describe v4l2_fwnode_reference_parse_int_prop(), not > >> v4l2_fwnode_reference_parse_int_props(). > > > > Yes, I think I changed the naming but forgot to update the commit. I'll do > > that now. > > > >> > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 93 insertions(+) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > >>> index 4821c4989119..56eee5bbd3b5 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c > >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > >>> @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse( > >>> return ret; > >>> } > >>> > >>> +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop( > >>> + struct fwnode_handle *fwnode, const char *prop, unsigned int index, > >>> + const char **props, unsigned int nprops) > >> > >> Need comments describing what this does. > > > > Yes. I'll also rename it (get -> read) for consistency with the async > > changes. > > Which async changes? Since the fwnode_handle that's returned is refcounted > I wonder if 'get' isn't the right name in this case. Right. True. I'll leave that as-is then. > > > > >> > >>> +{ > >>> + struct fwnode_reference_args fwnode_args; > >>> + unsigned int *args = fwnode_args.args; > >>> + struct fwnode_handle *child; > >>> + int ret; > >>> + > >>> + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops, > >>> + index, &fwnode_args); > >>> + if (ret) > >>> + return ERR_PTR(ret == -EINVAL ? -ENOENT : ret); > >> > >> Why map EINVAL to ENOENT? Needs a comment, either here or in the function description. > > > > fwnode_property_get_reference_args() returns currently a little bit > > different error codes in ACPI / DT. This is worth documenting there and > > fixing as well. > > > >> > >>> + > >>> + for (fwnode = fwnode_args.fwnode; > >>> + nprops; nprops--, fwnode = child, props++, args++) { > >> > >> I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args... > >> It's hard to parse. > > > > Hmm. I'm not sure if that really helps; the function is just handling each > > entry in the array and related array pointers are changed accordingly. The > > fwnode = child assignment is there to move to the child node. I.e. what you > > need for handling the loop itself. > > > > I can change this though if you think it really makes a difference for > > better. > > I think so, yes. I noticed you like complex for-loops :-) I don't really see a difference. The loop increment will just move at the end of the block inside the loop. > > > > >> > >> I would make this a 'while (nprops)' and write out all the other assignments, > >> increments and decrements. > >> > >>> + u32 val; > >>> + > >>> + fwnode_for_each_child_node(fwnode, child) { > >>> + if (fwnode_property_read_u32(child, *props, &val)) > >>> + continue; > >>> + > >>> + if (val == *args) > >>> + break; > >> > >> I'm lost. This really needs comments and perhaps even an DT or ACPI example > >> so you can see what exactly it is we're doing here. > > > > I'll add comments to the code. A good example will be ACPI documentation > > for LEDs, see 17th patch in v9. That will go through the linux-pm tree so > > it won't be available in the same tree for a while. > > Ideally an ACPI and an equivalent DT example would be nice to have, but I might > be asking too much. I'm not that familiar with ACPI, so for me a DT example > is easier. This won't be useful on DT although you could technically use it. In DT you can directly refer to any node but on ACPI you can just refer to devices, hence this. Would you be happy with the leds.txt example? I think it's a good example as it's directly related to this. > > > > >> > >>> + } > >>> + > >>> + fwnode_handle_put(fwnode); > >>> + > >>> + if (!child) { > >>> + fwnode = ERR_PTR(-ENOENT); > >>> + break; > >>> + } > >>> + } > >>> + > >>> + return fwnode; > >>> +} > >>> + > >>> +static int v4l2_fwnode_reference_parse_int_props( > >>> + struct device *dev, struct v4l2_async_notifier *notifier, > >>> + const char *prop, const char **props, unsigned int nprops) > >> > >> Needs comments describing what this does. > > > > Will add. > > > >> > >>> +{ > >>> + struct fwnode_handle *fwnode; > >>> + unsigned int index = 0; > >>> + int ret; > >>> + > >>> + while (!IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( > >>> + dev_fwnode(dev), prop, index, props, > >>> + nprops)))) { > >>> + fwnode_handle_put(fwnode); > >>> + index++; > >>> + } > >>> + > >>> + if (PTR_ERR(fwnode) != -ENOENT) > >>> + return PTR_ERR(fwnode); > >> > >> Missing 'if (index == 0)'? > > > > Yes, will add. > > > >> > >>> + > >>> + ret = v4l2_async_notifier_realloc(notifier, > >>> + notifier->num_subdevs + index); > >>> + if (ret) > >>> + return -ENOMEM; > >>> + > >>> + for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( > >>> + dev_fwnode(dev), prop, index, props, > >>> + nprops))); ) { > >> > >> I'd add 'index++' in this for-loop. It's weird that it is missing. > > > > Agreed, I'll move it there. > > > >> > >>> + struct v4l2_async_subdev *asd; > >>> + > >>> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { > >>> + ret = -EINVAL; > >>> + goto error; > >>> + } > >>> + > >>> + asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL); > >>> + if (!asd) { > >>> + ret = -ENOMEM; > >>> + goto error; > >>> + } > >>> + > >>> + notifier->subdevs[notifier->num_subdevs] = asd; > >>> + asd->match.fwnode.fwnode = fwnode; > >>> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > >>> + notifier->num_subdevs++; > >>> + > >>> + fwnode_handle_put(fwnode); > >>> + > >>> + index++; > >>> + } > >>> + > >>> + return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode); > >>> + > >>> +error: > >>> + fwnode_handle_put(fwnode); > >>> + return ret; > >>> +} > >>> + > >>> MODULE_LICENSE("GPL"); > >>> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>"); > >>> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > >>> > > > > Regards, > > Hans
On 09/11/2017 03:27 PM, Sakari Ailus wrote: > Hi Hans, > > On Mon, Sep 11, 2017 at 02:38:23PM +0200, Hans Verkuil wrote: >> On 09/11/2017 02:28 PM, Sakari Ailus wrote: >>> Hi Hans, >>> >>> Thanks for the review. >>> >>> On Mon, Sep 11, 2017 at 11:38:58AM +0200, Hans Verkuil wrote: >>>> Typo in subject: interger -> integer >>>> >>>> On 09/11/2017 10:00 AM, Sakari Ailus wrote: >>>>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under >>>>> the device's own fwnode, >>>> >>>> Sorry, you lost me here. Which device are we talking about? >>> >>> The fwnode related a struct device, in other words what dev_fwnode(dev) >>> gives you. This is either struct device.fwnode or struct >>> device.of_node.fwnode, depending on which firmware interface was used to >>> create the device. >>> >>> I'll add a note of this. >>> >>>> >>>>> it will follow child fwnodes with the given >>>>> property -- value pair and return the resulting fwnode. >>>> >>>> property-value pair (easier readable that way). >>>> >>>> You only describe v4l2_fwnode_reference_parse_int_prop(), not >>>> v4l2_fwnode_reference_parse_int_props(). >>> >>> Yes, I think I changed the naming but forgot to update the commit. I'll do >>> that now. >>> >>>> >>>>> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 93 insertions(+) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c >>>>> index 4821c4989119..56eee5bbd3b5 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c >>>>> @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse( >>>>> return ret; >>>>> } >>>>> >>>>> +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop( >>>>> + struct fwnode_handle *fwnode, const char *prop, unsigned int index, >>>>> + const char **props, unsigned int nprops) >>>> >>>> Need comments describing what this does. >>> >>> Yes. I'll also rename it (get -> read) for consistency with the async >>> changes. >> >> Which async changes? Since the fwnode_handle that's returned is refcounted >> I wonder if 'get' isn't the right name in this case. > > Right. True. I'll leave that as-is then. > >> >>> >>>> >>>>> +{ >>>>> + struct fwnode_reference_args fwnode_args; >>>>> + unsigned int *args = fwnode_args.args; >>>>> + struct fwnode_handle *child; >>>>> + int ret; >>>>> + >>>>> + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops, >>>>> + index, &fwnode_args); >>>>> + if (ret) >>>>> + return ERR_PTR(ret == -EINVAL ? -ENOENT : ret); >>>> >>>> Why map EINVAL to ENOENT? Needs a comment, either here or in the function description. >>> >>> fwnode_property_get_reference_args() returns currently a little bit >>> different error codes in ACPI / DT. This is worth documenting there and >>> fixing as well. >>> >>>> >>>>> + >>>>> + for (fwnode = fwnode_args.fwnode; >>>>> + nprops; nprops--, fwnode = child, props++, args++) { >>>> >>>> I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args... >>>> It's hard to parse. >>> >>> Hmm. I'm not sure if that really helps; the function is just handling each >>> entry in the array and related array pointers are changed accordingly. The >>> fwnode = child assignment is there to move to the child node. I.e. what you >>> need for handling the loop itself. >>> >>> I can change this though if you think it really makes a difference for >>> better. >> >> I think so, yes. I noticed you like complex for-loops :-) > > I don't really see a difference. The loop increment will just move at the > end of the block inside the loop. > >> >>> >>>> >>>> I would make this a 'while (nprops)' and write out all the other assignments, >>>> increments and decrements. >>>> >>>>> + u32 val; >>>>> + >>>>> + fwnode_for_each_child_node(fwnode, child) { >>>>> + if (fwnode_property_read_u32(child, *props, &val)) >>>>> + continue; >>>>> + >>>>> + if (val == *args) >>>>> + break; >>>> >>>> I'm lost. This really needs comments and perhaps even an DT or ACPI example >>>> so you can see what exactly it is we're doing here. >>> >>> I'll add comments to the code. A good example will be ACPI documentation >>> for LEDs, see 17th patch in v9. That will go through the linux-pm tree so >>> it won't be available in the same tree for a while. >> >> Ideally an ACPI and an equivalent DT example would be nice to have, but I might >> be asking too much. I'm not that familiar with ACPI, so for me a DT example >> is easier. > > This won't be useful on DT although you could technically use it. In DT you > can directly refer to any node but on ACPI you can just refer to devices, > hence this. So this function will effectively only be used with acpi? That should be documented. I think that explains some of my confusion since I was trying to map this code to a device tree, without much success. > Would you be happy with the leds.txt example? I think it's a good example > as it's directly related to this. Yes, that will work. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 11, 2017 at 03:34:16PM +0200, Hans Verkuil wrote: > >>>>> + u32 val; > >>>>> + > >>>>> + fwnode_for_each_child_node(fwnode, child) { > >>>>> + if (fwnode_property_read_u32(child, *props, &val)) > >>>>> + continue; > >>>>> + > >>>>> + if (val == *args) > >>>>> + break; > >>>> > >>>> I'm lost. This really needs comments and perhaps even an DT or ACPI example > >>>> so you can see what exactly it is we're doing here. > >>> > >>> I'll add comments to the code. A good example will be ACPI documentation > >>> for LEDs, see 17th patch in v9. That will go through the linux-pm tree so > >>> it won't be available in the same tree for a while. > >> > >> Ideally an ACPI and an equivalent DT example would be nice to have, but I might > >> be asking too much. I'm not that familiar with ACPI, so for me a DT example > >> is easier. > > > > This won't be useful on DT although you could technically use it. In DT you > > can directly refer to any node but on ACPI you can just refer to devices, > > hence this. > > So this function will effectively only be used with acpi? That should be > documented. I think that explains some of my confusion since I was trying > to map this code to a device tree, without much success. I'll add to the documentation of the function: * While it is technically possible to use this function on DT, it is only * meaningful on ACPI. On Device tree you can refer to any node in the tree but * on ACPI the references are limited to devices. > > > Would you be happy with the leds.txt example? I think it's a good example > > as it's directly related to this. > > Yes, that will work. I'll add a separate patch that I'll post later on. The ACPI documentation should get merged first.
On Mon, Sep 11, 2017 at 01:17:46PM +0200, Pavel Machek wrote: > On Mon 2017-09-11 11:00:03, Sakari Ailus wrote: > > Add v4l2_fwnode_parse_reference_sensor_common for parsing common > > sensor properties that refer to adjacent devices such as flash or lens > > driver chips. > > > > As this is an association only, there's little a regular driver needs to > > know about these devices as such. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > drivers/media/v4l2-core/v4l2-fwnode.c | 35 +++++++++++++++++++++++++++++++++++ > > include/media/v4l2-fwnode.h | 13 +++++++++++++ > > 2 files changed, 48 insertions(+) > > > > > > +/** > > + * v4l2_fwnode_reference_parse_sensor_common - parse common references on > > + * sensors for async sub-devices > > + * @dev: the device node the properties of which are parsed for references > > + * @notifier: the async notifier where the async subdevs will be added > > + * > > + * Return: 0 on success > > + * -ENOMEM if memory allocation failed > > + * -EINVAL if property parsing failed > > + */ > > Returns: would sound more correct to me, but it seems kernel is split > on that. I think in V4L2 there are roughly as many of each instances. I'll keep it as it is. > > Acked-by: Pavel Machek <pavel@ucw.cz> Thanks!
Hi, On Mon, Sep 11, 2017 at 9:59 AM, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi folks, > > We have a large influx of new, unmerged, drivers that are now parsing > fwnode endpoints and each one of them is doing this a little bit > differently. The needs are still exactly the same for the graph data > structure is device independent. This is still a non-trivial task and the > majority of the driver implementations are buggy, just buggy in different > ways. > > Facilitate parsing endpoints by adding a convenience function for parsing > the endpoints, and make the omap3isp and rcar-vin drivers use them as an > example. > > To show where we're getting with this, I've added support for async > sub-device notifier support that is notifiers that can be registered by > sub-device drivers as well as V4L2 fwnode improvements to make use of them > and the DTS changes for the Nokia N9. Some of these patches I've posted > previously in this set here: > > <URL:http://www.spinics.net/lists/linux-media/msg118764.html> > > Since that, the complete callback of the master notifier registering the > V4L2 device is only called once all sub-notifiers have been completed as > well. This way the device node creation can be postponed until all devices > have been successfully initialised. > > With this, the as3645a driver successfully registers a sub-device to the > media device created by the omap3isp driver. The kernel also has the > information it's related to the sensor driven by the smiapp driver but we > don't have a way to expose that information yet. I don't see core changes in this set, so I'm assuming it to be targeted at the users of endpoints etc. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On Tue, Oct 03, 2017 at 02:04:50AM +0200, Rafael J. Wysocki wrote: > Hi, > > On Mon, Sep 11, 2017 at 9:59 AM, Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > Hi folks, > > > > We have a large influx of new, unmerged, drivers that are now parsing > > fwnode endpoints and each one of them is doing this a little bit > > differently. The needs are still exactly the same for the graph data > > structure is device independent. This is still a non-trivial task and the > > majority of the driver implementations are buggy, just buggy in different > > ways. > > > > Facilitate parsing endpoints by adding a convenience function for parsing > > the endpoints, and make the omap3isp and rcar-vin drivers use them as an > > example. > > > > To show where we're getting with this, I've added support for async > > sub-device notifier support that is notifiers that can be registered by > > sub-device drivers as well as V4L2 fwnode improvements to make use of them > > and the DTS changes for the Nokia N9. Some of these patches I've posted > > previously in this set here: > > > > <URL:http://www.spinics.net/lists/linux-media/msg118764.html> > > > > Since that, the complete callback of the master notifier registering the > > V4L2 device is only called once all sub-notifiers have been completed as > > well. This way the device node creation can be postponed until all devices > > have been successfully initialised. > > > > With this, the as3645a driver successfully registers a sub-device to the > > media device created by the omap3isp driver. The kernel also has the > > information it's related to the sensor driven by the smiapp driver but we > > don't have a way to expose that information yet. > > I don't see core changes in this set, so I'm assuming it to be > targeted at the users of endpoints etc. Yes, this is dealing with V4L2 and how information from firmware is used mainly but there is also DT binding documentation.
Hi folks, We have a large influx of new, unmerged, drivers that are now parsing fwnode endpoints and each one of them is doing this a little bit differently. The needs are still exactly the same for the graph data structure is device independent. This is still a non-trivial task and the majority of the driver implementations are buggy, just buggy in different ways. Facilitate parsing endpoints by adding a convenience function for parsing the endpoints, and make the omap3isp and rcar-vin drivers use them as an example. To show where we're getting with this, I've added support for async sub-device notifier support that is notifiers that can be registered by sub-device drivers as well as V4L2 fwnode improvements to make use of them and the DTS changes for the Nokia N9. Some of these patches I've posted previously in this set here: <URL:http://www.spinics.net/lists/linux-media/msg118764.html> Since that, the complete callback of the master notifier registering the V4L2 device is only called once all sub-notifiers have been completed as well. This way the device node creation can be postponed until all devices have been successfully initialised. With this, the as3645a driver successfully registers a sub-device to the media device created by the omap3isp driver. The kernel also has the information it's related to the sensor driven by the smiapp driver but we don't have a way to expose that information yet. since v9: - Drop "as3645a: Switch to fwnode property API" and "ACPI: Document how to refer to LEDs from remote nodes" patches. They're better off separately from this set. - Address property documentation redundancy in smiapp DT binding documentation. - Add patches "ov5670: Add support for flash and lens devices" and "ov13858: Add support for flash and lens devices". since v8: - Improve terminology for notifiers. Instead of master / subdev, we have root, parent and subdev notifiers. - Renamed "flash" property as "flash-leds". There are many, and currently we make assumptions in a lot of places (e.g. LED bindings) that these are LEDs. While we don't have any other types of flashes supported right now (e.g. Xenon), it's safer to assume we might have them in the future. - Use ENOTCONN instead of EPERM to tell from driver's callback function that an endpoint is to be skipped but not handled as an error. - Avoid accessing notifier's subdevs array as well as num_subdevs field from rcar-vin driver. - Add a patch "v4l: async: Allow async notifier register call succeed with no subdevs", which allows, well, what the subject says. - Move checks for subdev / v4l2_dev from __v4l2_async_notifier_register() to v4l2_async_notifier_register() and v4l2_async_subdev_notifier_register(). - Don't initialise notifier->list. There was no need to do so, as this is the entry added to the list and not used otherwise. I.e. regarding this, the state before this patchset is restored. - Clean up error handling in v4l2_async_notifier_fwnode_parse_endpoint(). - WARN_ON() in v4l2_async_notifier_parse_fwnode_endpoints() if the asd_struct_size is smaller than size of struct v4l2_async_subdev. - Make v4l2_fwnode_reference_parse() static as there should be no need to use it outside the V4L2 fwnode framework. Also, remove the callback function as well as other arguments that always have the same value in current usage. (This can be changed later on if needed without affecting drivers.) - Add the patch "v4l: fwnode: Add a helper function to obtain device / interger references", which allows similar use than v4l2_fwnode_reference_parse() but is more useful on ACPI based systems --- on ACPI, you can only refer to device nodes (corresponding struct deice in Linux), not to data extension nodes under the devices. - Improve v4l2_fwnode_reference_parse_sensor_common() to work on ACPI based systems. - Add patch "ACPI: Document how to refer to LEDs from remote nodes" to document using and referring to LEDs on ACPI. - Rebase the set on AS3645A fixes I just sent ("AS3645A fixes") - In v4l2_fwnode_reference_parse_sensor_common(), tell if parsing a property failed. - Improved documentation for v4l2_async_notifier_parse_fwnode_endpoints(). - Fix v4l2_async_notifier_try_all_subdevs(); it is allowed that the list entry being iterated over is deleted but no other changes to the list are allowed. This could be the case if a sub-device driver's notifier binds a sub-device. Restart the loop whenever a match is found. - Add patch "as3645a: Switch to fwnode property API" which also adds ACPI support. since v7: - Added three more patches: v4l: async: Remove re-probing support v4l: async: Use more intuitive names for internal functions dt: bindings: smiapp: Document lens-focus and flash properties The last one was already sent previously after the rest of the patchset. - Removed re-probing support. This is hard to support and only useful in special cases. It can be reintroduced later on if there's really a need --- note that in e.g. omap3isp this was always broken and no-one ever complained. - Remove smiapp driver's async complete callback (and ops). It is redundant: the sub-device nodes are created through the master notifier. - Improve flash property documentation in video-interfaces.txt. - Introduce helper functions to call notifier operations, one for each operation. - Rename v4l2_async_test_notify as v4l2_async_match_notify and v4l2_async_belongs to v4l2_async_find_match. - v4l2_async_notifier_test_all_subdevs() renamed as v4l2_async_notifier_try_all_subdevs(). - Made notifier_v4l2_dev a function (it was a macro). - Registering subdev notifiers from sub-device drivers that control sub-devices created through sub-notifiers is now supported. In other words, subdev notifiers may be registered through other subdev notifiers. This is the source of the bulk of the changes between v7 and v8. - Add explanatory comments to helper functions used by V4L2 async framework. This should help understanding the internal workings of the framework. - Removed the "notifiers" list in struct v4l2_async_notifier. The information can be found from existing data structures. - Explicitly check that registering a non-subdev notifier has v4l2_dev and a subdev notifier has a sub-device pointer. - Unified several code paths between subdev notifiers and non-subdev notifiers. - Fixed v4l2_async_notifier_release() --- calling it on a notifier for which the driver had allocated the subdevs array would lead calling kvfree() on that array. Now notifier->max_subdevs is checked before proceeding. - Fixed a use-after-free issue in v4l2_async_notifier_fwnode_parse_endpoints(). - Small fixes to KernelDoc documentation for v4l2_async_notifier_parse_fwnode_endpoints(). since v6: - Drop the last patch that added variant for parsing endpoints given specific port and endpoints numbers. - Separate driver changes from the fwnode endpoint parser patch into two patches. rcar-vin driver is now using the name function. - Use -ENOTCONN to tell the parser that and endpoint (or a reference) is to be ignored. - parse_endpoint and parse_single callback functions are now optional and documented as such. - Added Laurent's patch adding notifier operations struct which I rebase on the fwnode parser patchset. I wrote another patch to call the notifier operations through macros. - Add DT bindings for flash and lens devices. - V4L2 fwnode parser for references (such as flash and lens). - Added smiapp driver support for async sub-devices (lens and flash). - Added a few fixes for omap3isp. since v5: - Use v4l2_async_ prefix for static functions as well (4th patch) - Use memcpy() to copy array rather than a loop - Document that the v4l2_async_subdev pointer in driver specific struct must be the first member - Improve documentation of the added functions (4th and 5th patches) - Arguments - More thorough explation of the purpose, usage and object lifetime - Added acks since v4: - Prepend the set with three documentation fixes. - The driver's async struct must begin with struct v4l2_async_subdev. Fix this for omap3isp and document it. - Improve documentation for new functions. - Don't use devm_ family of functions for allocating memory. Introduce v4l2_async_notifier_release() to release memory resources. - Rework both v4l2_async_notifier_fwnode_parse_endpoints() and v4l2_async_notifier_fwnode_parse_endpoint() and the local functions they call. This should make the code cleaner. Despite the name, for linking and typical usage reasons the functions remain in v4l2-fwnode.c. - Convert rcar-vin to use v4l2_async_notifier_fwnode_parse_endpoint(). - Use kvmalloc() for allocating the notifier's subdevs array. - max_subdevs argument for notifier_realloc is now the total maximum number of subdevs, not the number of available subdevs. - Use fwnode_device_is_available() to make sure the device actually exists. - Move the note telling v4l2_async_notifier_fwnode_parse_endpoints() should not be used by new drivers to the last patch adding v4l2_async_notifier_fwnode_parse_endpoint(). since v3: - Rebase on current mediatree master. since v2: - Rebase on CCP2 support patches. - Prepend a patch cleaning up omap3isp driver a little. since v1: - The first patch has been merged (it was a bugfix). - In anticipation that the parsing can take place over several iterations, take the existing number of async sub-devices into account when re-allocating an array of async sub-devices. - Rework the first patch to better anticipate parsing single endpoint at a time by a driver. - Add a second patch that adds a function for parsing endpoints one at a time based on port and endpoint numbers. Laurent Pinchart (1): v4l: async: Move async subdev notifier operations to a separate structure Sakari Ailus (23): v4l: fwnode: Move KernelDoc documentation to the header v4l: async: Remove re-probing support v4l: async: Use more intuitive names for internal functions v4l: async: Add V4L2 async documentation to the documentation build v4l: fwnode: Support generic parsing of graph endpoints in a device omap3isp: Use generic parser for parsing fwnode endpoints rcar-vin: Use generic parser for parsing fwnode endpoints omap3isp: Fix check for our own sub-devices omap3isp: Print the name of the entity where no source pads could be found v4l: async: Introduce helpers for calling async ops callbacks v4l: async: Register sub-devices before calling bound callback v4l: async: Allow async notifier register call succeed with no subdevs v4l: async: Allow binding notifiers to sub-devices dt: bindings: Add a binding for flash LED devices associated to a sensor dt: bindings: Add lens-focus binding for image sensors ACPI: Document how to refer to LEDs from remote nodes as3645a: Switch to fwnode property API v4l: fwnode: Add a helper function for parsing generic references v4l: fwnode: Add a helper function to obtain device / interger references v4l: fwnode: Add convenience function for parsing common external refs smiapp: Add support for flash and lens devices dt: bindings: smiapp: Document lens-focus and flash properties arm: dts: omap3: N9/N950: Add flash references to the camera Documentation/acpi/dsd/leds.txt | 92 +++++ .../devicetree/bindings/media/i2c/nokia,smia.txt | 2 + .../devicetree/bindings/media/video-interfaces.txt | 10 + Documentation/media/kapi/v4l2-async.rst | 3 + Documentation/media/kapi/v4l2-core.rst | 1 + arch/arm/boot/dts/omap3-n9.dts | 1 + arch/arm/boot/dts/omap3-n950-n9.dtsi | 4 +- arch/arm/boot/dts/omap3-n950.dts | 1 + drivers/leds/leds-as3645a.c | 81 +++-- drivers/media/i2c/smiapp/smiapp-core.c | 18 +- drivers/media/i2c/smiapp/smiapp.h | 4 +- drivers/media/platform/am437x/am437x-vpfe.c | 8 +- drivers/media/platform/atmel/atmel-isc.c | 10 +- drivers/media/platform/atmel/atmel-isi.c | 10 +- drivers/media/platform/davinci/vpif_capture.c | 8 +- drivers/media/platform/davinci/vpif_display.c | 8 +- drivers/media/platform/exynos4-is/media-dev.c | 8 +- drivers/media/platform/omap3isp/isp.c | 127 +++---- drivers/media/platform/omap3isp/isp.h | 5 +- drivers/media/platform/pxa_camera.c | 8 +- drivers/media/platform/qcom/camss-8x16/camss.c | 8 +- drivers/media/platform/rcar-vin/rcar-core.c | 122 +++---- drivers/media/platform/rcar-vin/rcar-dma.c | 10 +- drivers/media/platform/rcar-vin/rcar-v4l2.c | 14 +- drivers/media/platform/rcar-vin/rcar-vin.h | 4 +- drivers/media/platform/rcar_drif.c | 10 +- drivers/media/platform/soc_camera/soc_camera.c | 14 +- drivers/media/platform/stm32/stm32-dcmi.c | 10 +- drivers/media/platform/ti-vpe/cal.c | 8 +- drivers/media/platform/xilinx/xilinx-vipp.c | 8 +- drivers/media/v4l2-core/v4l2-async.c | 345 +++++++++++++----- drivers/media/v4l2-core/v4l2-fwnode.c | 386 +++++++++++++++++---- drivers/staging/media/imx/imx-media-dev.c | 8 +- include/media/v4l2-async.h | 68 +++- include/media/v4l2-fwnode.h | 150 +++++++- 35 files changed, 1135 insertions(+), 439 deletions(-) create mode 100644 Documentation/acpi/dsd/leds.txt create mode 100644 Documentation/media/kapi/v4l2-async.rst