[{"id":1763845,"web_url":"http://patchwork.ozlabs.org/comment/1763845/","msgid":"<255d6d56-e6b8-0919-4e22-a38255d15e3d@xs4all.nl>","list_archive_url":null,"date":"2017-09-06T07:00:30","subject":"Re: [PATCH v8 02/21] v4l: async: Remove re-probing support","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Remove V4L2 async re-probing support. The re-probing support has been\n> there to support cases where the sub-devices require resources provided by\n> the main driver's hardware to function, such as clocks.\n> \n> Reprobing has allowed unbinding and again binding the main driver without\n> explicilty unbinding the sub-device drivers. This is certainly not a\n> common need, and the responsibility will be the user's going forward.\n> \n> An alternative could have been to introduce notifier specific locks.\n> Considering the complexity of the re-probing and that it isn't really a\n> solution to a problem but a workaround, remove re-probing instead.\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> ---\n> drivers/media/v4l2-core/v4l2-async.c | 54 ------------------------------------\n> 1 file changed, 54 deletions(-)\n> \n> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> index 851f128eba22..f50a82767863 100644\n> --- a/drivers/media/v4l2-core/v4l2-async.c\n> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> @@ -203,78 +203,24 @@ EXPORT_SYMBOL(v4l2_async_notifier_register);\n> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> {\n> \tstruct v4l2_subdev *sd, *tmp;\n> -\tunsigned int notif_n_subdev = notifier->num_subdevs;\n> -\tunsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);\n> -\tstruct device **dev;\n> -\tint i = 0;\n> \n> \tif (!notifier->v4l2_dev)\n> \t\treturn;\n> \n> -\tdev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);\n> -\tif (!dev) {\n> -\t\tdev_err(notifier->v4l2_dev->dev,\n> -\t\t\t\"Failed to allocate device cache!\\n\");\n> -\t}\n> -\n> \tmutex_lock(&list_lock);\n> \n> \tlist_del(¬ifier->list);\n> \n> \tlist_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {\n> -\t\tstruct device *d;\n> -\n> -\t\td = get_device(sd->dev);\n> -\n> \t\tv4l2_async_cleanup(sd);\n> \n> -\t\t/* If we handled USB devices, we'd have to lock the parent too */\n> -\t\tdevice_release_driver(d);\n> -\n> \t\tif (notifier->unbind)\n> \t\t\tnotifier->unbind(notifier, sd, sd->asd);\n> -\n> -\t\t/*\n> -\t\t * Store device at the device cache, in order to call\n> -\t\t * put_device() on the final step\n> -\t\t */\n> -\t\tif (dev)\n> -\t\t\tdev[i++] = d;\n> -\t\telse\n> -\t\t\tput_device(d);\n> \t}\n> \n> \tmutex_unlock(&list_lock);\n> \n> -\t/*\n> -\t * Call device_attach() to reprobe devices\n> -\t *\n> -\t * NOTE: If dev allocation fails, i is 0, and the whole loop won't be\n> -\t * executed.\n> -\t */\n> -\twhile (i--) {\n> -\t\tstruct device *d = dev[i];\n> -\n> -\t\tif (d && device_attach(d) < 0) {\n> -\t\t\tconst char *name = \"(none)\";\n> -\t\t\tint lock = device_trylock(d);\n> -\n> -\t\t\tif (lock && d->driver)\n> -\t\t\t\tname = d->driver->name;\n> -\t\t\tdev_err(d, \"Failed to re-probe to %s\\n\", name);\n> -\t\t\tif (lock)\n> -\t\t\t\tdevice_unlock(d);\n> -\t\t}\n> -\t\tput_device(d);\n> -\t}\n> -\tkvfree(dev);\n> -\n> \tnotifier->v4l2_dev = NULL;\n> -\n> -\t/*\n> -\t * Don't care about the waiting list, it is initialised and populated\n> -\t * upon notifier registration.\n> -\t */\n> }\n> EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n> \n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnDxm3N4Mz9t3f\n\tfor ;\n\tWed, 6 Sep 2017 17:00:44 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751870AbdIFHAm (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:00:42 -0400","from lb3-smtp-cloud7.xs4all.net ([194.109.24.31]:34891 \"EHLO\n\tlb3-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751837AbdIFHAm (ORCPT\n\t); Wed, 6 Sep 2017 03:00:42 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pUK6dg9Jub2snpUKAdomTw; Wed, 06 Sep 2017 09:00:40 +0200"],"Subject":"Re: [PATCH v8 02/21] v4l: async: Remove re-probing support","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-3-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"<255d6d56-e6b8-0919-4e22-a38255d15e3d@xs4all.nl>","Date":"Wed, 6 Sep 2017 09:00:30 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-3-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfOASmXMkfwGhdbmWFl9GcpB8mvTgqqZx8AUskZ5K6W4sbtDZC+i2a+dFEODLmcmekWvWwf1LcQHWe3ccfs5/DB8huyF5SNRqGCXdMTPJhPAu4JpnvHeO\n\tdmTlnMzdQzxHH5N4nLXH16U7FTbPgW9Oal/dRqbFtjxwfPjwLfokVQjjfNp5+IvEZjR/OxI/zEJrxyU+5mKXsl23pNQIRJqwS6LqHbCgVacr78GDEGkW7SCz\n\tTdMrwa0DugzIPeiBB2vXdgq1qwmwnq/sB9wNzTc78ckeWtRoY/rXfZT0PZmNSQ3Mfe1YBEHdHQJhlINyNMNqgUIEYPTNnP6Vr6LxRR7TlZ7y1kgcrea7hra3\n\thqhKETu6qNdW0Zcgp0458T8dEi3nvzIjOWzjhJRzOvm9tkzwZZd2ZEgPJhZtj7aP58MFjrGK9HDivi7gcfgTE/Fqrx4bNs+f6vVC1Qh6rTXBTa8GB4LouIaV\n\t2EzxU4Ey3eMIUrzl","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763846,"web_url":"http://patchwork.ozlabs.org/comment/1763846/","msgid":"<7e466832-2c73-a552-1396-e9c282409272@xs4all.nl>","list_archive_url":null,"date":"2017-09-06T07:01:27","subject":"Re: [PATCH v8 03/21] v4l: async: Use more intuitive names for\n\tinternal functions","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Rename internal functions to make the names of the functions better\n> describe what they do.\n> \n> \tOld name\t\t\tNew name\n> \tv4l2_async_test_notify\tv4l2_async_match_notify\n> \tv4l2_async_belongs\tv4l2_async_find_match\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> ---\n> drivers/media/v4l2-core/v4l2-async.c | 19 ++++++++++---------\n> 1 file changed, 10 insertions(+), 9 deletions(-)\n> \n> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> index f50a82767863..3d81ff6a496f 100644\n> --- a/drivers/media/v4l2-core/v4l2-async.c\n> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> @@ -65,8 +65,8 @@ static LIST_HEAD(subdev_list);\n> static LIST_HEAD(notifier_list);\n> static DEFINE_MUTEX(list_lock);\n> \n> -static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,\n> -\t\t\t\t\t\t struct v4l2_subdev *sd)\n> +static struct v4l2_async_subdev *v4l2_async_find_match(\n> +\tstruct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)\n> {\n> \tbool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);\n> \tstruct v4l2_async_subdev *asd;\n> @@ -100,9 +100,9 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *\n> \treturn NULL;\n> }\n> \n> -static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,\n> -\t\t\t\t struct v4l2_subdev *sd,\n> -\t\t\t\t struct v4l2_async_subdev *asd)\n> +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> +\t\t\t\t struct v4l2_subdev *sd,\n> +\t\t\t\t struct v4l2_async_subdev *asd)\n> {\n> \tint ret;\n> \n> @@ -180,11 +180,11 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> \tlist_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {\n> \t\tint ret;\n> \n> -\t\tasd = v4l2_async_belongs(notifier, sd);\n> +\t\tasd = v4l2_async_find_match(notifier, sd);\n> \t\tif (!asd)\n> \t\t\tcontinue;\n> \n> -\t\tret = v4l2_async_test_notify(notifier, sd, asd);\n> +\t\tret = v4l2_async_match_notify(notifier, sd, asd);\n> \t\tif (ret < 0) {\n> \t\t\tmutex_unlock(&list_lock);\n> \t\t\treturn ret;\n> @@ -241,9 +241,10 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)\n> \tINIT_LIST_HEAD(&sd->async_list);\n> \n> \tlist_for_each_entry(notifier, ¬ifier_list, list) {\n> -\t\tstruct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd);\n> +\t\tstruct v4l2_async_subdev *asd = v4l2_async_find_match(notifier,\n> +\t\t\t\t\t\t\t\t sd);\n> \t\tif (asd) {\n> -\t\t\tint ret = v4l2_async_test_notify(notifier, sd, asd);\n> +\t\t\tint ret = v4l2_async_match_notify(notifier, sd, asd);\n> \t\t\tmutex_unlock(&list_lock);\n> \t\t\treturn ret;\n> \t\t}\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnDyk40gwz9sRV\n\tfor ;\n\tWed, 6 Sep 2017 17:01:34 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751880AbdIFHBd (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:01:33 -0400","from lb2-smtp-cloud7.xs4all.net ([194.109.24.28]:55277 \"EHLO\n\tlb2-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751837AbdIFHBc (ORCPT\n\t); Wed, 6 Sep 2017 03:01:32 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pUL1dg9r5b2snpUL4domon; Wed, 06 Sep 2017 09:01:31 +0200"],"Subject":"Re: [PATCH v8 03/21] v4l: async: Use more intuitive names for\n\tinternal functions","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-4-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"<7e466832-2c73-a552-1396-e9c282409272@xs4all.nl>","Date":"Wed, 6 Sep 2017 09:01:27 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-4-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfKDixqLCwNcayodTvR1lAXkw7L3dBqjeyECQ9amorIAP3RL7JzPCadv0420/OsQbmUGv5jYneaOSjWx2ueICcirL+KELavotBXne5QjgnGTFKkaPxMvR\n\t4kEslHa556wf9McUoMJCIavj+aANubw9ev5VQ7w192exRvIpDo+nF41GMwUADYd4EODnnSqvF1S8gYzDBDsJKzts/h/PYdKqr8jjL2vW0957V6PwW/EAXyMu\n\tzGvwx7TE0vZNrX+focdd2uSp1Xi6svZNGvcKDzaphefpRdqbRni/4HQkYMnExYYq2AwUrhikqzCRwniyMofgFYe/GhcTSgVUr0k+3q0ICCZPxeQwGVmE2kC/\n\tSbPNMMkLWljcNwVbPEw/rOoWMJx6ZySwzwxtaKttX5iYXMj9uqrHS7cOaXPy0E/SUGVwdcIdYugnCoocRbrTobTSsJYThF2bSW7PPTo+Czxd0PCNx2sWEiet\n\tPDQtKIHPb3sBpfpI","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763867,"web_url":"http://patchwork.ozlabs.org/comment/1763867/","msgid":"","list_archive_url":null,"date":"2017-09-06T07:41:40","subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> The current practice is that drivers iterate over their endpoints and\n> parse each endpoint separately. This is very similar in a number of\n> drivers, implement a generic function for the job. Driver specific matters\n> can be taken into account in the driver specific callback.\n> \n> Signed-off-by: Sakari Ailus \n> ---\n> drivers/media/v4l2-core/v4l2-async.c | 19 +++++\n> drivers/media/v4l2-core/v4l2-fwnode.c | 140 ++++++++++++++++++++++++++++++++++\n> include/media/v4l2-async.h | 24 +++++-\n> include/media/v4l2-fwnode.h | 53 +++++++++++++\n> 4 files changed, 234 insertions(+), 2 deletions(-)\n> \n> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> index 3d81ff6a496f..7bd595c4094a 100644\n> --- a/drivers/media/v4l2-core/v4l2-async.c\n> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> @@ -22,6 +22,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n> @@ -224,6 +225,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> }\n> EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n> \n> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)\n> +{\n> +\tunsigned int i;\n> +\n> +\tif (!notifier->max_subdevs)\n> +\t\treturn;\n> +\n> +\tfor (i = 0; i < notifier->num_subdevs; i++)\n> +\t\tkfree(notifier->subdevs[i]);\n> +\n> +\tnotifier->max_subdevs = 0;\n> +\tnotifier->num_subdevs = 0;\n> +\n> +\tkvfree(notifier->subdevs);\n> +\tnotifier->subdevs = NULL;\n> +}\n> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);\n> +\n> int v4l2_async_register_subdev(struct v4l2_subdev *sd)\n> {\n> \tstruct v4l2_async_notifier *notifier;\n> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c\n> index 706f9e7b90f1..e6932d7d47b6 100644\n> --- a/drivers/media/v4l2-core/v4l2-fwnode.c\n> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c\n> @@ -19,6 +19,7 @@\n> */\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -26,6 +27,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> enum v4l2_fwnode_bus_type {\n> @@ -313,6 +315,144 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)\n> }\n> EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);\n> \n> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,\n> +\t\t\t\t unsigned int max_subdevs)\n> +{\n> +\tstruct v4l2_async_subdev **subdevs;\n> +\n> +\tif (max_subdevs <= notifier->max_subdevs)\n> +\t\treturn 0;\n> +\n> +\tsubdevs = kvmalloc_array(\n> +\t\tmax_subdevs, sizeof(*notifier->subdevs),\n> +\t\tGFP_KERNEL | __GFP_ZERO);\n> +\tif (!subdevs)\n> +\t\treturn -ENOMEM;\n> +\n> +\tif (notifier->subdevs) {\n> +\t\tmemcpy(subdevs, notifier->subdevs,\n> +\t\t sizeof(*subdevs) * notifier->num_subdevs);\n> +\n> +\t\tkvfree(notifier->subdevs);\n> +\t}\n> +\n> +\tnotifier->subdevs = subdevs;\n> +\tnotifier->max_subdevs = max_subdevs;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int v4l2_async_notifier_fwnode_parse_endpoint(\n> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> +\tstruct fwnode_handle *endpoint, unsigned int asd_struct_size,\n> +\tint (*parse_endpoint)(struct device *dev,\n> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> +\t\t\t struct v4l2_async_subdev *asd))\n> +{\n> +\tstruct v4l2_async_subdev *asd;\n> +\tstruct v4l2_fwnode_endpoint *vep;\n> +\tstruct fwnode_endpoint ep;\n> +\tint ret = 0;\n> +\n> +\tasd = kzalloc(asd_struct_size, GFP_KERNEL);\n> +\tif (!asd)\n> +\t\treturn -ENOMEM;\n> +\n> +\tasd->match.fwnode.fwnode =\n> +\t\tfwnode_graph_get_remote_port_parent(endpoint);\n> +\tif (!asd->match.fwnode.fwnode) {\n> +\t\tdev_warn(dev, \"bad remote port parent\\n\");\n> +\t\tret = -EINVAL;\n> +\t\tgoto out_err;\n> +\t}\n> +\n> +\t/* Ignore endpoints the parsing of which failed. */\n> +\tvep = v4l2_fwnode_endpoint_alloc_parse(endpoint);\n> +\tif (IS_ERR(vep)) {\n> +\t\tret = PTR_ERR(vep);\n> +\t\tdev_warn(dev, \"unable to parse V4L2 fwnode endpoint (%d)\\n\",\n> +\t\t\t ret);\n> +\t\tgoto out_err;\n> +\t}\n> +\n> +\tep = vep->base;\n> +\n> +\tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n> +\tv4l2_fwnode_endpoint_free(vep);\n> +\tif (ret == -ENOTCONN) {\n> +\t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", ep.port, ep.id);\n> +\t\tkfree(asd);\n\nShouldn't there be a call to fwnode_handle_put()?\n\n> +\t\treturn 0;\n> +\t} else if (ret < 0) {\n> +\t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n> +\t\t\t ep.port, ep.id, ret);\n> +\t\tgoto out_err;\n> +\t}\n\nI think this would be better and it avoids the need for the ep local variable:\n\n\tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n\tif (ret == -ENOTCONN)\n\t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", vep->base.port, vep->base.id);\n\telse if (ret < 0)\n\t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n\t\t\t vep->base.port, vep->base.id, ret);\n\tv4l2_fwnode_endpoint_free(vep);\n\tif (ret < 0)\n\t\tgoto out_err;\n\nAnd the 'return ret;' below would become:\n\n\treturn ret == -ENOTCONN ? 0 : ret;\n\n> +\n> +\tasd->match_type = V4L2_ASYNC_MATCH_FWNODE;\n> +\tnotifier->subdevs[notifier->num_subdevs] = asd;\n> +\tnotifier->num_subdevs++;\n> +\n> +\treturn 0;\n> +\n> +out_err:\n> +\tfwnode_handle_put(asd->match.fwnode.fwnode);\n> +\tkfree(asd);\n> +\n> +\treturn ret;\n> +}\n> +\n> +int v4l2_async_notifier_parse_fwnode_endpoints(\n> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> +\tsize_t asd_struct_size,\n> +\tint (*parse_endpoint)(struct device *dev,\n> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> +\t\t\t struct v4l2_async_subdev *asd))\n> +{\n> +\tstruct fwnode_handle *fwnode = NULL;\n> +\tunsigned int max_subdevs = notifier->max_subdevs;\n> +\tint ret;\n> +\n> +\tif (asd_struct_size < sizeof(struct v4l2_async_subdev))\n\nI think this can be a WARN_ON or a WARN_ON_ONCE. Up to you, though.\n\n> +\t\treturn -EINVAL;\n> +\n> +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n> +\t\t\t\t dev_fwnode(dev), fwnode)); )\n> +\t\tif (fwnode_device_is_available(\n> +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n> +\t\t\tmax_subdevs++;\n\nI think I would prefer this as a simple for (;;):\n\n\tfor (;;) {\n\t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n\t\tif (fwnode == NULL)\n\t\t\tbreak;\n\t\t...\n\t}\n\nOr alternatively as a do...while:\n\n\tdo {\n\t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n\t\tif (fwnode && fwnode_device_is_available(\n\t\t\t fwnode_graph_get_port_parent(fwnode)))\n\t\t\tmax_subdevs++;\n\t} while (fwnode);\n\nBoth are IMHO a bit more readable. I leave it up to you, though.\n\n> +\n> +\t/* No subdevs to add? Return here. */\n> +\tif (max_subdevs == notifier->max_subdevs)\n> +\t\treturn 0;\n> +\n> +\tret = v4l2_async_notifier_realloc(notifier, max_subdevs);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n> +\t\t\t\t dev_fwnode(dev), fwnode)); ) {\n\nSame comment as above.\n\n> +\t\tif (!fwnode_device_is_available(\n> +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {\n> +\t\t\tret = -EINVAL;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tret = v4l2_async_notifier_fwnode_parse_endpoint(\n> +\t\t\tdev, notifier, fwnode, asd_struct_size, parse_endpoint);\n> +\t\tif (ret < 0)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tfwnode_handle_put(fwnode);\n> +\n> +\treturn ret;\n> +}\n> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);\n> +\n> MODULE_LICENSE(\"GPL\");\n> MODULE_AUTHOR(\"Sakari Ailus \");\n> MODULE_AUTHOR(\"Sylwester Nawrocki \");\n> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n> index c69d8c8a66d0..96fa1afc00dd 100644\n> --- a/include/media/v4l2-async.h\n> +++ b/include/media/v4l2-async.h\n> @@ -18,7 +18,6 @@ struct device;\n> struct device_node;\n> struct v4l2_device;\n> struct v4l2_subdev;\n> -struct v4l2_async_notifier;\n> \n> /* A random max subdevice number, used to allocate an array on stack */\n> #define V4L2_MAX_SUBDEVS 128U\n> @@ -50,6 +49,10 @@ enum v4l2_async_match_type {\n> * @match:\tunion of per-bus type matching data sets\n> * @list:\tused to link struct v4l2_async_subdev objects, waiting to be\n> *\t\tprobed, to a notifier->waiting list\n> + *\n> + * When this struct is used as a member in a driver specific struct,\n> + * the driver specific struct shall contain the @struct\n> + * v4l2_async_subdev as its first member.\n> */\n> struct v4l2_async_subdev {\n> \tenum v4l2_async_match_type match_type;\n> @@ -78,7 +81,8 @@ struct v4l2_async_subdev {\n> /**\n> * struct v4l2_async_notifier - v4l2_device notifier data\n> *\n> - * @num_subdevs: number of subdevices\n> + * @num_subdevs: number of subdevices used in the subdevs array\n> + * @max_subdevs: number of subdevices allocated in the subdevs array\n> * @subdevs:\tarray of pointers to subdevice descriptors\n> * @v4l2_dev:\tpointer to struct v4l2_device\n> * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n> @@ -90,6 +94,7 @@ struct v4l2_async_subdev {\n> */\n> struct v4l2_async_notifier {\n> \tunsigned int num_subdevs;\n> +\tunsigned int max_subdevs;\n> \tstruct v4l2_async_subdev **subdevs;\n> \tstruct v4l2_device *v4l2_dev;\n> \tstruct list_head waiting;\n> @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);\n> \n> /**\n> + * v4l2_async_notifier_release - release notifier resources\n> + * @notifier: the notifier the resources of which are to be released\n> + *\n> + * Release memory resources related to a notifier, including the async\n> + * sub-devices allocated for the purposes of the notifier. The user is\n> + * responsible for releasing the notifier's resources after calling\n> + * @v4l2_async_notifier_parse_fwnode_endpoints.\n> + *\n> + * There is no harm from calling v4l2_async_notifier_release in other\n> + * cases as long as its memory has been zeroed after it has been\n> + * allocated.\n> + */\n> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);\n> +\n> +/**\n> * v4l2_async_register_subdev - registers a sub-device to the asynchronous\n> * \tsubdevice framework\n> *\n> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h\n> index 68eb22ba571b..6d125f26ec84 100644\n> --- a/include/media/v4l2-fwnode.h\n> +++ b/include/media/v4l2-fwnode.h\n> @@ -25,6 +25,8 @@\n> #include \n> \n> struct fwnode_handle;\n> +struct v4l2_async_notifier;\n> +struct v4l2_async_subdev;\n> \n> #define V4L2_FWNODE_CSI2_MAX_DATA_LANES\t4\n> \n> @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,\n> */\n> void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);\n> \n> +/**\n> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a\n> + *\t\t\t\t\t\tdevice node\n> + * @dev: the device the endpoints of which are to be parsed\n> + * @notifier: notifier for @dev\n> + * @asd_struct_size: size of the driver's async sub-device struct, including\n> + *\t\t sizeof(struct v4l2_async_subdev). The &struct\n> + *\t\t v4l2_async_subdev shall be the first member of\n> + *\t\t the driver's async sub-device struct, i.e. both\n> + *\t\t begin at the same memory address.\n> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode\n> + *\t\t endpoint. Optional.\n> + *\t\t Return: %0 on success\n> + *\t\t\t %-ENOTCONN if the endpoint is to be skipped but this\n> + *\t\t\t\t should not be considered as an error\n> + *\t\t\t %-EINVAL if the endpoint configuration is invalid\n> + *\n> + * Parse the fwnode endpoints of the @dev device and populate the async sub-\n> + * devices array of the notifier. The @parse_endpoint callback function is\n> + * called for each endpoint with the corresponding async sub-device pointer to\n> + * let the caller initialize the driver-specific part of the async sub-device\n> + * structure.\n> + *\n> + * The notifier memory shall be zeroed before this function is called on the\n> + * notifier.\n> + *\n> + * This function may not be called on a registered notifier and may be called on\n> + * a notifier only once. When using this function, the user may not access the\n> + * notifier's subdevs array nor change notifier's num_subdevs field, these are\n> + * reserved for the framework's internal use only.\n\nThe rvin_digital_graph_init() function accesses the subdevs array.\n\nI still don't like this sentence, and I still think it should be dropped. Either that\nor completely rewritten.\n\n> + *\n> + * The @struct v4l2_fwnode_endpoint passed to the callback function\n> + * @parse_endpoint is released once the function is finished. If there is a need\n> + * to retain that configuration, the user needs to allocate memory for it.\n> + *\n> + * Any notifier populated using this function must be released with a call to\n> + * v4l2_async_notifier_release() after it has been unregistered and the async\n> + * sub-devices are no longer in use.\n> + *\n> + * Return: %0 on success, including when no async sub-devices are found\n> + *\t %-ENOMEM if memory allocation failed\n> + *\t %-EINVAL if graph or endpoint parsing failed\n> + *\t Other error codes as returned by @parse_endpoint\n> + */\n> +int v4l2_async_notifier_parse_fwnode_endpoints(\n> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> +\tsize_t asd_struct_size,\n> +\tint (*parse_endpoint)(struct device *dev,\n> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> +\t\t\t struct v4l2_async_subdev *asd));\n> +\n> #endif /* _V4L2_FWNODE_H */\n> \n\nRegards,\n\n\tHans\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnFs8572wz9sNd\n\tfor ;\n\tWed, 6 Sep 2017 17:41:48 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751646AbdIFHlr (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:41:47 -0400","from lb2-smtp-cloud7.xs4all.net ([194.109.24.28]:48642 \"EHLO\n\tlb2-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751021AbdIFHlq (ORCPT\n\t); Wed, 6 Sep 2017 03:41:46 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pUxwdgYiKb2snpUy0dp2KB; Wed, 06 Sep 2017 09:41:45 +0200"],"Subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-7-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"","Date":"Wed, 6 Sep 2017 09:41:40 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-7-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfP85BQoQRcv5KEaajxUA8LHOH2O2zlOTcgHRQErCpM1Wo1PAyIYUTb6ZlqooxuIfr+02ftQraN+kIFGGsKakUm6r6Vr4PzNVYJmu55zKjo2sjALgqvYS\n\tituKI5R0skLZm9puY+aodHq3EjCLvDdvQblmWB5l7SXuImVkFLmj2QRLss+YDCPPV+pCq7nMVmEgQANkNr4uEE23XEPRX0UqkplTwVG+BgkLO9HofQYQzdBK\n\t5fSlS0G0ys6EOlUFqSnsk1T/VJsyRpV5sjcKmvBxqCJuFnwVcz25n4P/SA3k55/HcBbvZK6xN0F62R80gXgVNt4QwvIBcxofw99JvwpxBV4nWyx9j06FInYY\n\tS+s1y8+zJGN5oXVS+/Q4ZQ9isWIwoyNqDE+sFoHLG7oDoidd3iMRC8VgVSyWww+a5jar9Zl+6oYczfp+6dqjT7/0Eu5wlt0Q9SfdiE8YRDg6RrYA97vKyZry\n\tCkZHuA6IW+zMiuPL","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763868,"web_url":"http://patchwork.ozlabs.org/comment/1763868/","msgid":"<2d01200b-7d63-c548-a3c8-97ab9a147bee@xs4all.nl>","list_archive_url":null,"date":"2017-09-06T07:42:24","subject":"Re: [PATCH v8 07/21] omap3isp: Use generic parser for parsing fwnode\n\tendpoints","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Instead of using driver implementation, use\n> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints\n> of the device.\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> ---\n> drivers/media/platform/omap3isp/isp.c | 115 +++++++++++-----------------------\n> drivers/media/platform/omap3isp/isp.h | 5 +-\n> 2 files changed, 37 insertions(+), 83 deletions(-)\n> \n> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c\n> index 1a428fe9f070..a546cf774d40 100644\n> --- a/drivers/media/platform/omap3isp/isp.c\n> +++ b/drivers/media/platform/omap3isp/isp.c\n> @@ -2001,6 +2001,7 @@ static int isp_remove(struct platform_device *pdev)\n> \t__omap3isp_put(isp, false);\n> \n> \tmedia_entity_enum_cleanup(&isp->crashed);\n> +\tv4l2_async_notifier_release(&isp->notifier);\n> \n> \treturn 0;\n> }\n> @@ -2011,44 +2012,41 @@ enum isp_of_phy {\n> \tISP_OF_PHY_CSIPHY2,\n> };\n> \n> -static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,\n> -\t\t\t struct isp_async_subdev *isd)\n> +static int isp_fwnode_parse(struct device *dev,\n> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> +\t\t\t struct v4l2_async_subdev *asd)\n> {\n> +\tstruct isp_async_subdev *isd =\n> +\t\tcontainer_of(asd, struct isp_async_subdev, asd);\n> \tstruct isp_bus_cfg *buscfg = &isd->bus;\n> -\tstruct v4l2_fwnode_endpoint vep;\n> -\tunsigned int i;\n> -\tint ret;\n> \tbool csi1 = false;\n> -\n> -\tret = v4l2_fwnode_endpoint_parse(fwnode, &vep);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tunsigned int i;\n> \n> \tdev_dbg(dev, \"parsing endpoint %pOF, interface %u\\n\",\n> -\t\tto_of_node(fwnode), vep.base.port);\n> +\t\tto_of_node(vep->base.local_fwnode), vep->base.port);\n> \n> -\tswitch (vep.base.port) {\n> +\tswitch (vep->base.port) {\n> \tcase ISP_OF_PHY_PARALLEL:\n> \t\tbuscfg->interface = ISP_INTERFACE_PARALLEL;\n> \t\tbuscfg->bus.parallel.data_lane_shift =\n> -\t\t\tvep.bus.parallel.data_shift;\n> +\t\t\tvep->bus.parallel.data_shift;\n> \t\tbuscfg->bus.parallel.clk_pol =\n> -\t\t\t!!(vep.bus.parallel.flags\n> +\t\t\t!!(vep->bus.parallel.flags\n> \t\t\t & V4L2_MBUS_PCLK_SAMPLE_FALLING);\n> \t\tbuscfg->bus.parallel.hs_pol =\n> -\t\t\t!!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);\n> +\t\t\t!!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);\n> \t\tbuscfg->bus.parallel.vs_pol =\n> -\t\t\t!!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);\n> +\t\t\t!!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);\n> \t\tbuscfg->bus.parallel.fld_pol =\n> -\t\t\t!!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);\n> +\t\t\t!!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);\n> \t\tbuscfg->bus.parallel.data_pol =\n> -\t\t\t!!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);\n> -\t\tbuscfg->bus.parallel.bt656 = vep.bus_type == V4L2_MBUS_BT656;\n> +\t\t\t!!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);\n> +\t\tbuscfg->bus.parallel.bt656 = vep->bus_type == V4L2_MBUS_BT656;\n> \t\tbreak;\n> \n> \tcase ISP_OF_PHY_CSIPHY1:\n> \tcase ISP_OF_PHY_CSIPHY2:\n> -\t\tswitch (vep.bus_type) {\n> +\t\tswitch (vep->bus_type) {\n> \t\tcase V4L2_MBUS_CCP2:\n> \t\tcase V4L2_MBUS_CSI1:\n> \t\t\tdev_dbg(dev, \"CSI-1/CCP-2 configuration\\n\");\n> @@ -2060,11 +2058,11 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tdev_err(dev, \"unsupported bus type %u\\n\",\n> -\t\t\t\tvep.bus_type);\n> +\t\t\t\tvep->bus_type);\n> \t\t\treturn -EINVAL;\n> \t\t}\n> \n> -\t\tswitch (vep.base.port) {\n> +\t\tswitch (vep->base.port) {\n> \t\tcase ISP_OF_PHY_CSIPHY1:\n> \t\t\tif (csi1)\n> \t\t\t\tbuscfg->interface = ISP_INTERFACE_CCP2B_PHY1;\n> @@ -2080,47 +2078,47 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,\n> \t\t}\n> \t\tif (csi1) {\n> \t\t\tbuscfg->bus.ccp2.lanecfg.clk.pos =\n> -\t\t\t\tvep.bus.mipi_csi1.clock_lane;\n> +\t\t\t\tvep->bus.mipi_csi1.clock_lane;\n> \t\t\tbuscfg->bus.ccp2.lanecfg.clk.pol =\n> -\t\t\t\tvep.bus.mipi_csi1.lane_polarity[0];\n> +\t\t\t\tvep->bus.mipi_csi1.lane_polarity[0];\n> \t\t\tdev_dbg(dev, \"clock lane polarity %u, pos %u\\n\",\n> \t\t\t\tbuscfg->bus.ccp2.lanecfg.clk.pol,\n> \t\t\t\tbuscfg->bus.ccp2.lanecfg.clk.pos);\n> \n> \t\t\tbuscfg->bus.ccp2.lanecfg.data[0].pos =\n> -\t\t\t\tvep.bus.mipi_csi1.data_lane;\n> +\t\t\t\tvep->bus.mipi_csi1.data_lane;\n> \t\t\tbuscfg->bus.ccp2.lanecfg.data[0].pol =\n> -\t\t\t\tvep.bus.mipi_csi1.lane_polarity[1];\n> +\t\t\t\tvep->bus.mipi_csi1.lane_polarity[1];\n> \n> \t\t\tdev_dbg(dev, \"data lane polarity %u, pos %u\\n\",\n> \t\t\t\tbuscfg->bus.ccp2.lanecfg.data[0].pol,\n> \t\t\t\tbuscfg->bus.ccp2.lanecfg.data[0].pos);\n> \n> \t\t\tbuscfg->bus.ccp2.strobe_clk_pol =\n> -\t\t\t\tvep.bus.mipi_csi1.clock_inv;\n> -\t\t\tbuscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe;\n> +\t\t\t\tvep->bus.mipi_csi1.clock_inv;\n> +\t\t\tbuscfg->bus.ccp2.phy_layer = vep->bus.mipi_csi1.strobe;\n> \t\t\tbuscfg->bus.ccp2.ccp2_mode =\n> -\t\t\t\tvep.bus_type == V4L2_MBUS_CCP2;\n> +\t\t\t\tvep->bus_type == V4L2_MBUS_CCP2;\n> \t\t\tbuscfg->bus.ccp2.vp_clk_pol = 1;\n> \n> \t\t\tbuscfg->bus.ccp2.crc = 1;\n> \t\t} else {\n> \t\t\tbuscfg->bus.csi2.lanecfg.clk.pos =\n> -\t\t\t\tvep.bus.mipi_csi2.clock_lane;\n> +\t\t\t\tvep->bus.mipi_csi2.clock_lane;\n> \t\t\tbuscfg->bus.csi2.lanecfg.clk.pol =\n> -\t\t\t\tvep.bus.mipi_csi2.lane_polarities[0];\n> +\t\t\t\tvep->bus.mipi_csi2.lane_polarities[0];\n> \t\t\tdev_dbg(dev, \"clock lane polarity %u, pos %u\\n\",\n> \t\t\t\tbuscfg->bus.csi2.lanecfg.clk.pol,\n> \t\t\t\tbuscfg->bus.csi2.lanecfg.clk.pos);\n> \n> \t\t\tbuscfg->bus.csi2.num_data_lanes =\n> -\t\t\t\tvep.bus.mipi_csi2.num_data_lanes;\n> +\t\t\t\tvep->bus.mipi_csi2.num_data_lanes;\n> \n> \t\t\tfor (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {\n> \t\t\t\tbuscfg->bus.csi2.lanecfg.data[i].pos =\n> -\t\t\t\t\tvep.bus.mipi_csi2.data_lanes[i];\n> +\t\t\t\t\tvep->bus.mipi_csi2.data_lanes[i];\n> \t\t\t\tbuscfg->bus.csi2.lanecfg.data[i].pol =\n> -\t\t\t\t\tvep.bus.mipi_csi2.lane_polarities[i + 1];\n> +\t\t\t\t\tvep->bus.mipi_csi2.lane_polarities[i + 1];\n> \t\t\t\tdev_dbg(dev,\n> \t\t\t\t\t\"data lane %u polarity %u, pos %u\\n\", i,\n> \t\t\t\t\tbuscfg->bus.csi2.lanecfg.data[i].pol,\n> @@ -2137,57 +2135,13 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,\n> \n> \tdefault:\n> \t\tdev_warn(dev, \"%pOF: invalid interface %u\\n\",\n> -\t\t\t to_of_node(fwnode), vep.base.port);\n> +\t\t\t to_of_node(vep->base.local_fwnode), vep->base.port);\n> \t\treturn -EINVAL;\n> \t}\n> \n> \treturn 0;\n> }\n> \n> -static int isp_fwnodes_parse(struct device *dev,\n> -\t\t\t struct v4l2_async_notifier *notifier)\n> -{\n> -\tstruct fwnode_handle *fwnode = NULL;\n> -\n> -\tnotifier->subdevs = devm_kcalloc(\n> -\t\tdev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);\n> -\tif (!notifier->subdevs)\n> -\t\treturn -ENOMEM;\n> -\n> -\twhile (notifier->num_subdevs < ISP_MAX_SUBDEVS &&\n> -\t (fwnode = fwnode_graph_get_next_endpoint(\n> -\t\t\tof_fwnode_handle(dev->of_node), fwnode))) {\n> -\t\tstruct isp_async_subdev *isd;\n> -\n> -\t\tisd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);\n> -\t\tif (!isd)\n> -\t\t\tgoto error;\n> -\n> -\t\tif (isp_fwnode_parse(dev, fwnode, isd)) {\n> -\t\t\tdevm_kfree(dev, isd);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tnotifier->subdevs[notifier->num_subdevs] = &isd->asd;\n> -\n> -\t\tisd->asd.match.fwnode.fwnode =\n> -\t\t\tfwnode_graph_get_remote_port_parent(fwnode);\n> -\t\tif (!isd->asd.match.fwnode.fwnode) {\n> -\t\t\tdev_warn(dev, \"bad remote port parent\\n\");\n> -\t\t\tgoto error;\n> -\t\t}\n> -\n> -\t\tisd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;\n> -\t\tnotifier->num_subdevs++;\n> -\t}\n> -\n> -\treturn notifier->num_subdevs;\n> -\n> -error:\n> -\tfwnode_handle_put(fwnode);\n> -\treturn -EINVAL;\n> -}\n> -\n> static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)\n> {\n> \tstruct isp_device *isp = container_of(async, struct isp_device,\n> @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);\n> +\tret = v4l2_async_notifier_parse_fwnode_endpoints(\n> +\t\t&pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),\n> +\t\tisp_fwnode_parse);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> @@ -2407,6 +2363,7 @@ static int isp_probe(struct platform_device *pdev)\n> \t__omap3isp_put(isp, false);\n> error:\n> \tmutex_destroy(&isp->isp_mutex);\n> +\tv4l2_async_notifier_release(&isp->notifier);\n> \n> \treturn ret;\n> }\n> diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h\n> index e528df6efc09..8b9043db94b3 100644\n> --- a/drivers/media/platform/omap3isp/isp.h\n> +++ b/drivers/media/platform/omap3isp/isp.h\n> @@ -220,14 +220,11 @@ struct isp_device {\n> \n> \tunsigned int sbl_resources;\n> \tunsigned int subclk_resources;\n> -\n> -#define ISP_MAX_SUBDEVS\t\t8\n> -\tstruct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];\n> };\n> \n> struct isp_async_subdev {\n> -\tstruct isp_bus_cfg bus;\n> \tstruct v4l2_async_subdev asd;\n> +\tstruct isp_bus_cfg bus;\n> };\n> \n> #define v4l2_subdev_to_bus_cfg(sd) \\\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnFsz5jLHz9sNd\n\tfor ;\n\tWed, 6 Sep 2017 17:42:31 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751626AbdIFHma (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:42:30 -0400","from lb2-smtp-cloud7.xs4all.net ([194.109.24.28]:40901 \"EHLO\n\tlb2-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751345AbdIFHm3 (ORCPT\n\t); Wed, 6 Sep 2017 03:42:29 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pUyedgZ6Kb2snpUyhdp2YN; Wed, 06 Sep 2017 09:42:28 +0200"],"Subject":"Re: [PATCH v8 07/21] omap3isp: Use generic parser for parsing fwnode\n\tendpoints","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-8-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"<2d01200b-7d63-c548-a3c8-97ab9a147bee@xs4all.nl>","Date":"Wed, 6 Sep 2017 09:42:24 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-8-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfDKMrtPCpUDFLbLABKAA151gfNffGQb/AsjYtGqliYTuNHgRMsSfy7cK1lx2TsP1QgSFBrv70/D3jV/8uKCJ021ok/0MtSXONjqYtVm4pzGauqPmllfD\n\tgiEJxolxxj/2Xu00obtzaLk10iFQJrjzstbxPBYaQcpvdQG299TTQI5DaY84IdMatptSfnTzbZAIRpfiBnjwK2NdoPF9Zr6p5PPYTYUyVFxAOtKS8NKW1ADE\n\thtS0KfqHVMJz21e0tklZfXvgZNxMtKl10ickVND0UP0MJGJ95xz93lujGNG6OjEmFbGhdJB81IV/0cvptmArluKx/p4+i9U+KQ59WeoFS1kkRHUJ+YuBazML\n\tf9PZGSv26AwlFBahmOiAOA5MMIgDhn090UrxQ1//OVptXA2MyIgg6ezi8UMyl+iCFDOxVsflNbKOHTad+BDIIDGo+2coVDTLrobJHftk3MJk/LoVlhrAYWyt\n\tohtXLflIHixhuQ4x","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763869,"web_url":"http://patchwork.ozlabs.org/comment/1763869/","msgid":"","list_archive_url":null,"date":"2017-09-06T07:44:32","subject":"Re: [PATCH v8 08/21] rcar-vin: Use generic parser for parsing fwnode\n\tendpoints","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Instead of using driver implementation, use\n> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints\n> of the device.\n> \n> Signed-off-by: Sakari Ailus \n> ---\n> drivers/media/platform/rcar-vin/rcar-core.c | 112 +++++++++-------------------\n> drivers/media/platform/rcar-vin/rcar-dma.c | 10 +--\n> drivers/media/platform/rcar-vin/rcar-v4l2.c | 14 ++--\n> drivers/media/platform/rcar-vin/rcar-vin.h | 4 +-\n> 4 files changed, 48 insertions(+), 92 deletions(-)\n> \n> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c\n> index 142de447aaaa..bd551f0be213 100644\n> --- a/drivers/media/platform/rcar-vin/rcar-core.c\n> +++ b/drivers/media/platform/rcar-vin/rcar-core.c\n> @@ -21,6 +21,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"rcar-vin.h\"\n> @@ -77,14 +78,14 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)\n> \tint ret;\n> \n> \t/* Verify subdevices mbus format */\n> -\tif (!rvin_mbus_supported(&vin->digital)) {\n> +\tif (!rvin_mbus_supported(vin->digital)) {\n> \t\tvin_err(vin, \"Unsupported media bus format for %s\\n\",\n> -\t\t\tvin->digital.subdev->name);\n> +\t\t\tvin->digital->subdev->name);\n> \t\treturn -EINVAL;\n> \t}\n> \n> \tvin_dbg(vin, \"Found media bus format for %s: %d\\n\",\n> -\t\tvin->digital.subdev->name, vin->digital.code);\n> +\t\tvin->digital->subdev->name, vin->digital->code);\n> \n> \tret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);\n> \tif (ret < 0) {\n> @@ -103,7 +104,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,\n> \n> \tvin_dbg(vin, \"unbind digital subdev %s\\n\", subdev->name);\n> \trvin_v4l2_remove(vin);\n> -\tvin->digital.subdev = NULL;\n> +\tvin->digital->subdev = NULL;\n> }\n> \n> static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,\n> @@ -120,117 +121,70 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,\n> \tret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);\n> \tif (ret < 0)\n> \t\treturn ret;\n> -\tvin->digital.source_pad = ret;\n> +\tvin->digital->source_pad = ret;\n> \n> \tret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);\n> -\tvin->digital.sink_pad = ret < 0 ? 0 : ret;\n> +\tvin->digital->sink_pad = ret < 0 ? 0 : ret;\n> \n> -\tvin->digital.subdev = subdev;\n> +\tvin->digital->subdev = subdev;\n> \n> \tvin_dbg(vin, \"bound subdev %s source pad: %u sink pad: %u\\n\",\n> -\t\tsubdev->name, vin->digital.source_pad,\n> -\t\tvin->digital.sink_pad);\n> +\t\tsubdev->name, vin->digital->source_pad,\n> +\t\tvin->digital->sink_pad);\n> \n> \treturn 0;\n> }\n> \n> -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,\n> -\t\t\t\t struct device_node *ep,\n> -\t\t\t\t struct v4l2_mbus_config *mbus_cfg)\n> +static int rvin_digital_parse_v4l2(struct device *dev,\n> +\t\t\t\t struct v4l2_fwnode_endpoint *vep,\n> +\t\t\t\t struct v4l2_async_subdev *asd)\n> {\n> -\tstruct v4l2_fwnode_endpoint v4l2_ep;\n> -\tint ret;\n> +\tstruct rvin_dev *vin = dev_get_drvdata(dev);\n> +\tstruct rvin_graph_entity *rvge =\n> +\t\tcontainer_of(asd, struct rvin_graph_entity, asd);\n> \n> -\tret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);\n> -\tif (ret) {\n> -\t\tvin_err(vin, \"Could not parse v4l2 endpoint\\n\");\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tif (vep->base.port || vep->base.id)\n> +\t\treturn -EPERM;\n> \n> -\tmbus_cfg->type = v4l2_ep.bus_type;\n> +\trvge->mbus_cfg.type = vep->bus_type;\n> \n> -\tswitch (mbus_cfg->type) {\n> +\tswitch (rvge->mbus_cfg.type) {\n> \tcase V4L2_MBUS_PARALLEL:\n> \t\tvin_dbg(vin, \"Found PARALLEL media bus\\n\");\n> -\t\tmbus_cfg->flags = v4l2_ep.bus.parallel.flags;\n> +\t\trvge->mbus_cfg.flags = vep->bus.parallel.flags;\n> \t\tbreak;\n> \tcase V4L2_MBUS_BT656:\n> \t\tvin_dbg(vin, \"Found BT656 media bus\\n\");\n> -\t\tmbus_cfg->flags = 0;\n> +\t\trvge->mbus_cfg.flags = 0;\n> \t\tbreak;\n> \tdefault:\n> \t\tvin_err(vin, \"Unknown media bus type\\n\");\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\treturn 0;\n> -}\n> -\n> -static int rvin_digital_graph_parse(struct rvin_dev *vin)\n> -{\n> -\tstruct device_node *ep, *np;\n> -\tint ret;\n> -\n> -\tvin->digital.asd.match.fwnode.fwnode = NULL;\n> -\tvin->digital.subdev = NULL;\n> -\n> -\t/*\n> -\t * Port 0 id 0 is local digital input, try to get it.\n> -\t * Not all instances can or will have this, that is OK\n> -\t */\n> -\tep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);\n> -\tif (!ep)\n> -\t\treturn 0;\n> -\n> -\tnp = of_graph_get_remote_port_parent(ep);\n> -\tif (!np) {\n> -\t\tvin_err(vin, \"No remote parent for digital input\\n\");\n> -\t\tof_node_put(ep);\n> -\t\treturn -EINVAL;\n> -\t}\n> -\tof_node_put(np);\n> -\n> -\tret = rvin_digitial_parse_v4l2(vin, ep, &vin->digital.mbus_cfg);\n> -\tof_node_put(ep);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tvin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);\n> -\tvin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;\n> +\tvin->digital = rvge;\n> \n> \treturn 0;\n> }\n> \n> static int rvin_digital_graph_init(struct rvin_dev *vin)\n> {\n> -\tstruct v4l2_async_subdev **subdevs = NULL;\n> \tint ret;\n> \n> -\tret = rvin_digital_graph_parse(vin);\n> +\tret = v4l2_async_notifier_parse_fwnode_endpoints(\n> +\t\tvin->dev, &vin->notifier,\n> +\t\tsizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tif (!vin->digital.asd.match.fwnode.fwnode) {\n> -\t\tvin_dbg(vin, \"No digital subdevice found\\n\");\n> -\t\treturn -ENODEV;\n> -\t}\n> -\n> -\t/* Register the subdevices notifier. */\n> -\tsubdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);\n> -\tif (subdevs == NULL)\n> -\t\treturn -ENOMEM;\n> -\n> -\tsubdevs[0] = &vin->digital.asd;\n> -\n> -\tvin_dbg(vin, \"Found digital subdevice %pOF\\n\",\n> -\t\tto_of_node(subdevs[0]->match.fwnode.fwnode));\n> +\tif (vin->notifier.num_subdevs > 0)\n> +\t\tvin_dbg(vin, \"Found digital subdevice %pOF\\n\",\n> +\t\t\tto_of_node(\n> +\t\t\t\tvin->notifier.subdevs[0]->match.fwnode.fwnode));\n\nAs mentioned in my review of patch 6/21, this violates the documentation of the\nv4l2_async_notifier_parse_fwnode_endpoints function.\n\nHowever, I think the problem is with the documentation and not with this code,\nso:\n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> \n> -\tvin->notifier.num_subdevs = 1;\n> -\tvin->notifier.subdevs = subdevs;\n> \tvin->notifier.bound = rvin_digital_notify_bound;\n> \tvin->notifier.unbind = rvin_digital_notify_unbind;\n> \tvin->notifier.complete = rvin_digital_notify_complete;\n> -\n> \tret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);\n> \tif (ret < 0) {\n> \t\tvin_err(vin, \"Notifier registration failed\\n\");\n> @@ -290,6 +244,8 @@ static int rcar_vin_probe(struct platform_device *pdev)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tplatform_set_drvdata(pdev, vin);\n> +\n> \tret = rvin_digital_graph_init(vin);\n> \tif (ret < 0)\n> \t\tgoto error;\n> @@ -297,11 +253,10 @@ static int rcar_vin_probe(struct platform_device *pdev)\n> \tpm_suspend_ignore_children(&pdev->dev, true);\n> \tpm_runtime_enable(&pdev->dev);\n> \n> -\tplatform_set_drvdata(pdev, vin);\n> -\n> \treturn 0;\n> error:\n> \trvin_dma_remove(vin);\n> +\tv4l2_async_notifier_release(&vin->notifier);\n> \n> \treturn ret;\n> }\n> @@ -313,6 +268,7 @@ static int rcar_vin_remove(struct platform_device *pdev)\n> \tpm_runtime_disable(&pdev->dev);\n> \n> \tv4l2_async_notifier_unregister(&vin->notifier);\n> +\tv4l2_async_notifier_release(&vin->notifier);\n> \n> \trvin_dma_remove(vin);\n> \n> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c\n> index b136844499f6..23fdff7a7370 100644\n> --- a/drivers/media/platform/rcar-vin/rcar-dma.c\n> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c\n> @@ -183,7 +183,7 @@ static int rvin_setup(struct rvin_dev *vin)\n> \t/*\n> \t * Input interface\n> \t */\n> -\tswitch (vin->digital.code) {\n> +\tswitch (vin->digital->code) {\n> \tcase MEDIA_BUS_FMT_YUYV8_1X16:\n> \t\t/* BT.601/BT.1358 16bit YCbCr422 */\n> \t\tvnmc |= VNMC_INF_YUV16;\n> @@ -191,7 +191,7 @@ static int rvin_setup(struct rvin_dev *vin)\n> \t\tbreak;\n> \tcase MEDIA_BUS_FMT_UYVY8_2X8:\n> \t\t/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */\n> -\t\tvnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?\n> +\t\tvnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?\n> \t\t\tVNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;\n> \t\tinput_is_yuv = true;\n> \t\tbreak;\n> @@ -200,7 +200,7 @@ static int rvin_setup(struct rvin_dev *vin)\n> \t\tbreak;\n> \tcase MEDIA_BUS_FMT_UYVY10_2X10:\n> \t\t/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */\n> -\t\tvnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?\n> +\t\tvnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?\n> \t\t\tVNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;\n> \t\tinput_is_yuv = true;\n> \t\tbreak;\n> @@ -212,11 +212,11 @@ static int rvin_setup(struct rvin_dev *vin)\n> \tdmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);\n> \n> \t/* Hsync Signal Polarity Select */\n> -\tif (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))\n> +\tif (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))\n> \t\tdmr2 |= VNDMR2_HPS;\n> \n> \t/* Vsync Signal Polarity Select */\n> -\tif (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))\n> +\tif (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))\n> \t\tdmr2 |= VNDMR2_VPS;\n> \n> \t/*\n> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c\n> index dd37ea811680..b479b882da12 100644\n> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c\n> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c\n> @@ -111,7 +111,7 @@ static int rvin_reset_format(struct rvin_dev *vin)\n> \tstruct v4l2_mbus_framefmt *mf = &fmt.format;\n> \tint ret;\n> \n> -\tfmt.pad = vin->digital.source_pad;\n> +\tfmt.pad = vin->digital->source_pad;\n> \n> \tret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);\n> \tif (ret)\n> @@ -172,13 +172,13 @@ static int __rvin_try_format_source(struct rvin_dev *vin,\n> \n> \tsd = vin_to_source(vin);\n> \n> -\tv4l2_fill_mbus_format(&format.format, pix, vin->digital.code);\n> +\tv4l2_fill_mbus_format(&format.format, pix, vin->digital->code);\n> \n> \tpad_cfg = v4l2_subdev_alloc_pad_config(sd);\n> \tif (pad_cfg == NULL)\n> \t\treturn -ENOMEM;\n> \n> -\tformat.pad = vin->digital.source_pad;\n> +\tformat.pad = vin->digital->source_pad;\n> \n> \tfield = pix->field;\n> \n> @@ -555,7 +555,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,\n> \tif (timings->pad)\n> \t\treturn -EINVAL;\n> \n> -\ttimings->pad = vin->digital.sink_pad;\n> +\ttimings->pad = vin->digital->sink_pad;\n> \n> \tret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);\n> \n> @@ -607,7 +607,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,\n> \tif (cap->pad)\n> \t\treturn -EINVAL;\n> \n> -\tcap->pad = vin->digital.sink_pad;\n> +\tcap->pad = vin->digital->sink_pad;\n> \n> \tret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);\n> \n> @@ -625,7 +625,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)\n> \tif (edid->pad)\n> \t\treturn -EINVAL;\n> \n> -\tedid->pad = vin->digital.sink_pad;\n> +\tedid->pad = vin->digital->sink_pad;\n> \n> \tret = v4l2_subdev_call(sd, pad, get_edid, edid);\n> \n> @@ -643,7 +643,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)\n> \tif (edid->pad)\n> \t\treturn -EINVAL;\n> \n> -\tedid->pad = vin->digital.sink_pad;\n> +\tedid->pad = vin->digital->sink_pad;\n> \n> \tret = v4l2_subdev_call(sd, pad, set_edid, edid);\n> \n> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h\n> index 9bfb5a7c4dc4..5382078143fb 100644\n> --- a/drivers/media/platform/rcar-vin/rcar-vin.h\n> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h\n> @@ -126,7 +126,7 @@ struct rvin_dev {\n> \tstruct v4l2_device v4l2_dev;\n> \tstruct v4l2_ctrl_handler ctrl_handler;\n> \tstruct v4l2_async_notifier notifier;\n> -\tstruct rvin_graph_entity digital;\n> +\tstruct rvin_graph_entity *digital;\n> \n> \tstruct mutex lock;\n> \tstruct vb2_queue queue;\n> @@ -145,7 +145,7 @@ struct rvin_dev {\n> \tstruct v4l2_rect compose;\n> };\n> \n> -#define vin_to_source(vin)\t\tvin->digital.subdev\n> +#define vin_to_source(vin)\t\t((vin)->digital->subdev)\n> \n> /* Debug */\n> #define vin_dbg(d, fmt, arg...)\t\tdev_dbg(d->dev, fmt, ##arg)\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnFwR1KmNz9sCZ\n\tfor ;\n\tWed, 6 Sep 2017 17:44:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751836AbdIFHoi (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:44:38 -0400","from lb3-smtp-cloud7.xs4all.net ([194.109.24.31]:37875 \"EHLO\n\tlb3-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751021AbdIFHoh (ORCPT\n\t); Wed, 6 Sep 2017 03:44:37 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pV0idgaahb2snpV0ldp3X3; Wed, 06 Sep 2017 09:44:36 +0200"],"Subject":"Re: [PATCH v8 08/21] rcar-vin: Use generic parser for parsing fwnode\n\tendpoints","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-9-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"","Date":"Wed, 6 Sep 2017 09:44:32 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-9-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfIVPe3HuOT7xmxZzM018HNS6WyzmLdc7UGWagvLe6v/xMZWRPY5DVG909v4JD2Yn0Gqz7YeRmmxIFPTWh+hdsZ7xWz+2W2169mXsDOw+o6gALbegxzIN\n\tZuwQB2vetODmMMXsxXPl5kTez44Fujg8M3AE4J0EQ5i2v8T7fyHpMrDY4RnSmft/QC6WmYE0SHSRW3EubfnDz4E7KpEerKv83ptDWx35bl/RFeN0g3RCAJlg\n\tibNXEVNrNFYfgAj+M7p/QoFvNAq8QmgNa4Mo3TkT/sQib+Opl0tZHhXOj0E+1SDTanwv4Lz3nEtps2Npn7SLblkYEiw09Zyh9R4NwRcHhNh4G3Z17+2gqMlI\n\tAw7Q+R7SSE2dj5+WtYjnwY/4xCIBSywKXNYzGeDy/QfHVn+7g/jJnSVZ5d3hf8IfMTbLiotaYOSHK6M7LYGfw/K4fX0Qsyn54u86kfz450v/I6ATHTHOyXxs\n\tdKt43huJ91eeBF08","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763870,"web_url":"http://patchwork.ozlabs.org/comment/1763870/","msgid":"<36648f59-4ea1-8137-8e7a-5c8e497bc664@xs4all.nl>","list_archive_url":null,"date":"2017-09-06T07:45:14","subject":"Re: [PATCH v8 09/21] omap3isp: Fix check for our own sub-devices","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> We only want to link sub-devices that were bound to the async notifier the\n> isp driver registered but there may be other sub-devices in the\n> v4l2_device as well. Check for the correct async notifier.\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> ---\n> drivers/media/platform/omap3isp/isp.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c\n> index a546cf774d40..3b1a9cd0e591 100644\n> --- a/drivers/media/platform/omap3isp/isp.c\n> +++ b/drivers/media/platform/omap3isp/isp.c\n> @@ -2155,7 +2155,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)\n> \t\treturn ret;\n> \n> \tlist_for_each_entry(sd, &v4l2_dev->subdevs, list) {\n> -\t\tif (!sd->asd)\n> +\t\tif (sd->notifier != &isp->notifier)\n> \t\t\tcontinue;\n> \n> \t\tret = isp_link_entity(isp, &sd->entity,\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnFxD6Gxnz9sCZ\n\tfor ;\n\tWed, 6 Sep 2017 17:45:20 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751675AbdIFHpT (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:45:19 -0400","from lb3-smtp-cloud7.xs4all.net ([194.109.24.31]:42504 \"EHLO\n\tlb3-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751021AbdIFHpS (ORCPT\n\t); Wed, 6 Sep 2017 03:45:18 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pV1OdgbURb2snpV1Rdp4Tt; Wed, 06 Sep 2017 09:45:17 +0200"],"Subject":"Re: [PATCH v8 09/21] omap3isp: Fix check for our own sub-devices","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-10-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"<36648f59-4ea1-8137-8e7a-5c8e497bc664@xs4all.nl>","Date":"Wed, 6 Sep 2017 09:45:14 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-10-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfAExMvMsKxVcoV3cuWvnwwa5p0D0/yA5v4XxCoIDK2SN0WnhKxuiBv1NwWxvzmSqC+NLjaI5GIiY4yNV8VEFRykgpqAU/XukU1bdDmI9Jd6N5KNQ2qDs\n\tjiZw1WP/K7fIF9Vsb7lKg8836oimdrmgWH5b1RPOWEevwK9Pfqp6uGD6LVvNdimBmXqcg9dhb+UZNRsyd0nlLn0KJQBgLPH6uBNaMe2W1gWyqXtoSWWO70Np\n\ttm6e3oTOwhouOn+zbekjHLLP+6/SeT7zP67nNpiOi//qe6fv5mMXVjvH9FF6P93E47/UzIWdo9i5XPs84MADBDJ9gZnA8devXN+jzEDk0i/14C4LAz1vCKX4\n\tkJMlZvfrouWERpn3J+1BtIG3g2odF+urAhRG42K2ynLdVQ+QziJ5cnxbtrYhS+hFG9rDh+8JuKwTpt+8AQ3ZYPh6MH4SzzumZTHJ2dXASvYbDfgpZw3HV0/B\n\tirM+NJ0/2q2BjVcA","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763871,"web_url":"http://patchwork.ozlabs.org/comment/1763871/","msgid":"","list_archive_url":null,"date":"2017-09-06T07:45:29","subject":"Re: [PATCH v8 10/21] omap3isp: Print the name of the entity where no\n\tsource pads could be found","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> If no source pads are found in an entity, print the name of the entity.\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> ---\n> drivers/media/platform/omap3isp/isp.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c\n> index 3b1a9cd0e591..9a694924e46e 100644\n> --- a/drivers/media/platform/omap3isp/isp.c\n> +++ b/drivers/media/platform/omap3isp/isp.c\n> @@ -1669,8 +1669,8 @@ static int isp_link_entity(\n> \t\t\tbreak;\n> \t}\n> \tif (i == entity->num_pads) {\n> -\t\tdev_err(isp->dev, \"%s: no source pad in external entity\\n\",\n> -\t\t\t__func__);\n> +\t\tdev_err(isp->dev, \"%s: no source pad in external entity %s\\n\",\n> +\t\t\t__func__, entity->name);\n> \t\treturn -EINVAL;\n> \t}\n> \n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnFxW3Svfz9t2R\n\tfor ;\n\tWed, 6 Sep 2017 17:45:35 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751626AbdIFHpe (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:45:34 -0400","from lb1-smtp-cloud7.xs4all.net ([194.109.24.24]:46913 \"EHLO\n\tlb1-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751021AbdIFHpe (ORCPT\n\t); Wed, 6 Sep 2017 03:45:34 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pV1ddgbyAb2snpV1gdp50q; Wed, 06 Sep 2017 09:45:32 +0200"],"Subject":"Re: [PATCH v8 10/21] omap3isp: Print the name of the entity where no\n\tsource pads could be found","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-11-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"","Date":"Wed, 6 Sep 2017 09:45:29 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-11-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfBe0mPps72B0IERVCWalxECpCuIyLFZYmxywZ1goa19oEPWNSffdLzlJMxv9aRkUJj2C5ZbRBtUoyHQh/r2vsdQXe075nJX6CPaSBorN9wj/ZU1cXVHq\n\t4RTo34bPBKzDQ3zrZugMyhM+E8RUMO4wKBO6hwdUgtYTCVhiZlSpJesu5JutbwMlf/hEcGITxla4O+5fHBOmFe5L0746XCs8ssMc+6JryEAOqE7y7ZjPx/m9\n\txJ1vbrkCb23UBO2ChrGglBJZL0Y4sPaBZwZ5VBDbyEqDJX7U/vfJMxVuPU18oRNEhrJaGEfUXYZrRqdr/0RBF/KTa9NmQxcNdatESP+LUSjqOXOI/oqlBw1a\n\tegeUCOKKG3P3ru3YSPHsubSSFM2ii1Y7C/7nLyPsk7jI59pIzoC/vPFqcCy2nS/IhP+/kyNLhf4ao+r51uS79URFyRhIpFZr1m1BSYAIwUQaZxutS0RXs1nJ\n\tSSWJDOeo8zYQuTH/","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763875,"web_url":"http://patchwork.ozlabs.org/comment/1763875/","msgid":"","list_archive_url":null,"date":"2017-09-06T07:50:36","subject":"Re: [PATCH v8 12/21] v4l: async: Introduce helpers for calling async\n\tops callbacks","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Add three helper functions to call async operations callbacks. Besides\n> simplifying callbacks, this allows async notifiers to have no ops set,\n> i.e. it can be left NULL.\n\nWhat is the use-case of that?\n\nAnyway:\n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> \n> Signed-off-by: Sakari Ailus \n> ---\n> drivers/media/v4l2-core/v4l2-async.c | 49 ++++++++++++++++++++++++++----------\n> include/media/v4l2-async.h | 1 +\n> 2 files changed, 37 insertions(+), 13 deletions(-)\n> \n> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> index f7eb3713207a..baee95eacbba 100644\n> --- a/drivers/media/v4l2-core/v4l2-async.c\n> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> @@ -25,6 +25,34 @@\n> #include \n> #include \n> \n> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,\n> +\t\t\t\t\t struct v4l2_subdev *subdev,\n> +\t\t\t\t\t struct v4l2_async_subdev *asd)\n> +{\n> +\tif (!n->ops || !n->ops->bound)\n> +\t\treturn 0;\n> +\n> +\treturn n->ops->bound(n, subdev, asd);\n> +}\n> +\n> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,\n> +\t\t\t\t\t struct v4l2_subdev *subdev,\n> +\t\t\t\t\t struct v4l2_async_subdev *asd)\n> +{\n> +\tif (!n->ops || !n->ops->unbind)\n> +\t\treturn;\n> +\n> +\tn->ops->unbind(n, subdev, asd);\n> +}\n> +\n> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)\n> +{\n> +\tif (!n->ops || !n->ops->complete)\n> +\t\treturn 0;\n> +\n> +\treturn n->ops->complete(n);\n> +}\n> +\n> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n> {\n> #if IS_ENABLED(CONFIG_I2C)\n> @@ -107,16 +135,13 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> {\n> \tint ret;\n> \n> -\tif (notifier->ops->bound) {\n> -\t\tret = notifier->ops->bound(notifier, sd, asd);\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\t}\n> +\tret = v4l2_async_notifier_call_bound(notifier, sd, asd);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> \n> \tret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);\n> \tif (ret < 0) {\n> -\t\tif (notifier->ops->unbind)\n> -\t\t\tnotifier->ops->unbind(notifier, sd, asd);\n> +\t\tv4l2_async_notifier_call_unbind(notifier, sd, asd);\n> \t\treturn ret;\n> \t}\n> \n> @@ -128,8 +153,8 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> \t/* Move from the global subdevice list to notifier's done */\n> \tlist_move(&sd->async_list, ¬ifier->done);\n> \n> -\tif (list_empty(¬ifier->waiting) && notifier->ops->complete)\n> -\t\treturn notifier->ops->complete(notifier);\n> +\tif (list_empty(¬ifier->waiting))\n> +\t\treturn v4l2_async_notifier_call_complete(notifier);\n> \n> \treturn 0;\n> }\n> @@ -215,8 +240,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> \tlist_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {\n> \t\tv4l2_async_cleanup(sd);\n> \n> -\t\tif (notifier->ops->unbind)\n> -\t\t\tnotifier->ops->unbind(notifier, sd, sd->asd);\n> +\t\tv4l2_async_notifier_call_unbind(notifier, sd, sd->asd);\n> \t}\n> \n> \tmutex_unlock(&list_lock);\n> @@ -294,8 +318,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)\n> \n> \tv4l2_async_cleanup(sd);\n> \n> -\tif (notifier->ops->unbind)\n> -\t\tnotifier->ops->unbind(notifier, sd, sd->asd);\n> +\tv4l2_async_notifier_call_unbind(notifier, sd, sd->asd);\n> \n> \tmutex_unlock(&list_lock);\n> }\n> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n> index 3c48f8b66d12..3bc8a7c0d83f 100644\n> --- a/include/media/v4l2-async.h\n> +++ b/include/media/v4l2-async.h\n> @@ -164,4 +164,5 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);\n> * @sd: pointer to &struct v4l2_subdev\n> */\n> void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);\n> +\n> #endif\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnG3S2bPdz9sNd\n\tfor ;\n\tWed, 6 Sep 2017 17:50:44 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751778AbdIFHum (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:50:42 -0400","from lb2-smtp-cloud7.xs4all.net ([194.109.24.28]:57604 \"EHLO\n\tlb2-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751646AbdIFHul (ORCPT\n\t); Wed, 6 Sep 2017 03:50:41 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pV6adgfT1b2snpV6edp7Me; Wed, 06 Sep 2017 09:50:40 +0200"],"Subject":"Re: [PATCH v8 12/21] v4l: async: Introduce helpers for calling async\n\tops callbacks","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-13-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"","Date":"Wed, 6 Sep 2017 09:50:36 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-13-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfHwkxGuOfMCFQdtJH+812h84N5J4jJYK1RfhK+IDEIcdIAhKU+CXz1sbdDoD7GbQFnksAHN+lj5Vcx6EeOvzd4ptKNflM+jOcWSsJmglJosvRJ95TB7L\n\tiObQKm1RwF5OPRj2FOu4bFKt9kD+AR8SyEv7HkO55u75ROGAHX7foh16S0E3EQzEX1Y1lMDikSSxLmw0XBh/DiXLV0C+xN0FkNoMgCBNszYsRu0RFjHc9YRY\n\tWilo6T0FC5UqCPsgHUOewR4Q1zIUUvgPcrlPj8BE3ogtx16ss8c7IFWT7h/HIV6dqr89HuN1Zgc8gRDbxq1exUoKkr1UWb1xi5Zr4FfpYkSCQjHoixgYRudV\n\t9Xyj0Bc2cM7JwV29CbY6urO2nm6WvlL7XuTXccQMBl8hsmu0C/Vkfk7SQCMdp3Ns2Ut2Qk0pOSKnBsJE+MdD6gvP8ZPoIyed2B1cl4HugynQbGHynVfB7bHc\n\tiYqDe8ocHTlInKba","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763876,"web_url":"http://patchwork.ozlabs.org/comment/1763876/","msgid":"","list_archive_url":null,"date":"2017-09-06T07:51:26","subject":"Re: [PATCH v8 13/21] v4l: async: Register sub-devices before calling\n\tbound callback","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Register the sub-device before calling the notifier's bound callback.\n> Doing this the other way around is problematic as the struct v4l2_device\n> has not assigned for the sub-device yet and may be required by the bound\n> callback.\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Hans Verkuil \n\nRegards,\n\n\tHans\n\n> ---\n> drivers/media/v4l2-core/v4l2-async.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> index baee95eacbba..79f216723a3f 100644\n> --- a/drivers/media/v4l2-core/v4l2-async.c\n> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> @@ -135,13 +135,13 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> {\n> \tint ret;\n> \n> -\tret = v4l2_async_notifier_call_bound(notifier, sd, asd);\n> +\tret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);\n> +\tret = v4l2_async_notifier_call_bound(notifier, sd, asd);\n> \tif (ret < 0) {\n> -\t\tv4l2_async_notifier_call_unbind(notifier, sd, asd);\n> +\t\tv4l2_device_unregister_subdev(sd);\n> \t\treturn ret;\n> \t}\n> \n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnG4N3k5Qz9sNd\n\tfor ;\n\tWed, 6 Sep 2017 17:51:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751836AbdIFHvb (ORCPT\n\t);\n\tWed, 6 Sep 2017 03:51:31 -0400","from lb2-smtp-cloud7.xs4all.net ([194.109.24.28]:33916 \"EHLO\n\tlb2-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751675AbdIFHva (ORCPT\n\t); Wed, 6 Sep 2017 03:51:30 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pV7Odgfzfb2snpV7Rdp7f4; Wed, 06 Sep 2017 09:51:29 +0200"],"Subject":"Re: [PATCH v8 13/21] v4l: async: Register sub-devices before calling\n\tbound callback","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-14-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"","Date":"Wed, 6 Sep 2017 09:51:26 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-14-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfI/PXVYNzVXIWCydBC6nP9RjYUnh9FY0KmRfbZtZU5RClz7jeJO9QxQXtV+9yTsEyNI1cVksuRxy7o7n1Zk9Crv9HGPJGpyuDDu22t3BNlLlvG4LNLhk\n\tydAvRh4Y8D2p7isPPQW3fCupZIMgeXFVrTOKt+GVwTCoZlpzEko0ELX4ydvD5SIOlSYSgwofVKqHEjTIgHfGlMXmCsE2YjjCbm3WemhO33UeugXGzK+3pntO\n\t4NT0sb1nQdoyAeqp3oBk7FV4xaZccQe0gz2Ygg+rhsD9XNVAaAjAoIvghD8zIG8rNDb3OzK269vu1TbBAqA9+bYUfCXUVk/zzlE+be/5SyHpri+evsctBFap\n\tp2Dheo9h3liIAwlyZdDgUY3gk9+e1SfGmaSXYwkXtOzgzyN9StHyZvtUWhW+1Obra0OwRaYO81zi+NePFT5XVYsFM4TFDEOgAdhfNNcz77fGM2zTIfArW0Ry\n\t2yQ2UwHDlceWBQlz","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1763920,"web_url":"http://patchwork.ozlabs.org/comment/1763920/","msgid":"<910b3d01-a8a4-2363-4b24-ed0edd1e1f4d@xs4all.nl>","list_archive_url":null,"date":"2017-09-06T08:46:31","subject":"Re: [PATCH v8 14/21] v4l: async: Allow binding notifiers to\n\tsub-devices","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> Registering a notifier has required the knowledge of struct v4l2_device\n> for the reason that sub-devices generally are registered to the\n> v4l2_device (as well as the media device, also available through\n> v4l2_device).\n> \n> This information is not available for sub-device drivers at probe time.\n> \n> What this patch does is that it allows registering notifiers without\n> having v4l2_device around. Instead the sub-device pointer is stored to the\n\nto -> in\n\n> notifier. Once the sub-device of the driver that registered the notifier\n> is registered, the notifier will gain the knowledge of the v4l2_device,\n> and the binding of async sub-devices from the sub-device driver's notifier\n> may proceed.\n> \n> The master notifier's complete callback is only called when all sub-device\n> notifiers are completed.\n> \n> Signed-off-by: Sakari Ailus \n> ---\n> drivers/media/v4l2-core/v4l2-async.c | 209 ++++++++++++++++++++++++++++++-----\n> include/media/v4l2-async.h | 16 ++-\n> 2 files changed, 194 insertions(+), 31 deletions(-)\n> \n> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> index 79f216723a3f..620b2cd29fc3 100644\n> --- a/drivers/media/v4l2-core/v4l2-async.c\n> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> @@ -53,6 +53,10 @@ static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)\n> \treturn n->ops->complete(n);\n> }\n> \n> +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> +\t\t\t\t struct v4l2_subdev *sd,\n> +\t\t\t\t struct v4l2_async_subdev *asd);\n> +\n> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n> {\n> #if IS_ENABLED(CONFIG_I2C)\n> @@ -129,14 +133,119 @@ static struct v4l2_async_subdev *v4l2_async_find_match(\n> \treturn NULL;\n> }\n> \n> +/* Get the sub-device notifier registered by a sub-device driver. */\n> +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(\n> +\tstruct v4l2_subdev *sd)\n> +{\n> +\tstruct v4l2_async_notifier *n;\n> +\n> +\tlist_for_each_entry(n, ¬ifier_list, list)\n> +\t\tif (n->sd == sd)\n> +\t\t\treturn n;\n> +\n> +\treturn NULL;\n> +}\n> +\n> +/* Return true if all sub-device notifiers are complete, false otherwise. */\n> +static bool v4l2_async_subdev_notifiers_complete(\n> +\tstruct v4l2_async_notifier *notifier)\n> +{\n> +\tstruct v4l2_subdev *sd;\n> +\n> +\tif (!list_empty(¬ifier->waiting))\n> +\t\treturn false;\n> +\n> +\tlist_for_each_entry(sd, ¬ifier->done, async_list) {\n> +\t\tstruct v4l2_async_notifier *subdev_notifier =\n> +\t\t\tv4l2_async_get_subdev_notifier(sd);\n> +\n> +\t\tif (!subdev_notifier)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (!v4l2_async_subdev_notifiers_complete(subdev_notifier))\n> +\t\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +/* Get v4l2_device related to the notifier if one can be found. */\n> +static struct v4l2_device *v4l2_async_notifier_get_v4l2_dev(\n> +\tstruct v4l2_async_notifier *notifier)\n> +{\n> +\twhile (notifier->master)\n> +\t\tnotifier = notifier->master;\n> +\n> +\treturn notifier->v4l2_dev;\n> +}\n> +\n> +/* Test all async sub-devices in a notifier for a match. */\n> +static int v4l2_async_notifier_try_all_subdevs(\n> +\tstruct v4l2_async_notifier *notifier)\n> +{\n> +\tstruct v4l2_subdev *sd, *tmp;\n> +\n> +\tif (!v4l2_async_notifier_get_v4l2_dev(notifier))\n> +\t\treturn 0;\n> +\n> +\tlist_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {\n> +\t\tstruct v4l2_async_subdev *asd;\n> +\t\tint ret;\n> +\n> +\t\tasd = v4l2_async_find_match(notifier, sd);\n> +\t\tif (!asd)\n> +\t\t\tcontinue;\n> +\n> +\t\tret = v4l2_async_match_notify(notifier, sd, asd);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/* Try completing a notifier. */\n> +static int v4l2_async_notifier_try_complete(\n> +\tstruct v4l2_async_notifier *notifier)\n> +{\n> +\tdo {\n> +\t\tint ret;\n> +\n> +\t\t/* Any local async sub-devices left? */\n> +\t\tif (!list_empty(¬ifier->waiting))\n> +\t\t\treturn 0;\n> +\n> +\t\t/*\n> +\t\t * Any sub-device notifiers waiting for async subdevs\n> +\t\t * to be bound?\n> +\t\t */\n> +\t\tif (!v4l2_async_subdev_notifiers_complete(notifier))\n> +\t\t\treturn 0;\n> +\n> +\t\t/* Proceed completing the notifier */\n> +\t\tret = v4l2_async_notifier_call_complete(notifier);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\t/*\n> +\t\t * Obtain notifier's master. If there is one, repeat\n> +\t\t * the process, otherwise we're done here.\n> +\t\t */\n> +\t} while ((notifier = notifier->master));\n\nI'd change this to:\n\n\t\tnotifier = notifier->master;\n\t} while (notifier);\n\n> +\n> +\treturn 0;\n> +}\n> +\n> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> \t\t\t\t struct v4l2_subdev *sd,\n> \t\t\t\t struct v4l2_async_subdev *asd)\n> {\n> +\tstruct v4l2_async_notifier *subdev_notifier;\n> \tint ret;\n> \n> -\tret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);\n> -\tif (ret < 0)\n> +\tret = v4l2_device_register_subdev(\n> +\t\tv4l2_async_notifier_get_v4l2_dev(notifier), sd);\n> +\tif (ret)\n> \t\treturn ret;\n> \n> \tret = v4l2_async_notifier_call_bound(notifier, sd, asd);\n> @@ -153,10 +262,20 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> \t/* Move from the global subdevice list to notifier's done */\n> \tlist_move(&sd->async_list, ¬ifier->done);\n> \n> -\tif (list_empty(¬ifier->waiting))\n> -\t\treturn v4l2_async_notifier_call_complete(notifier);\n> +\t/*\n> +\t * See if the sub-device has a notifier. If it does, proceed\n> +\t * with checking for its async sub-devices.\n> +\t */\n> +\tsubdev_notifier = v4l2_async_get_subdev_notifier(sd);\n> +\tif (subdev_notifier && !subdev_notifier->master) {\n> +\t\tsubdev_notifier->master = notifier;\n> +\t\tret = v4l2_async_notifier_try_all_subdevs(subdev_notifier);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> \n> -\treturn 0;\n> +\t/* Try completing the notifier and its master(s). */\n> +\treturn v4l2_async_notifier_try_complete(notifier);\n> }\n> \n> static void v4l2_async_cleanup(struct v4l2_subdev *sd)\n> @@ -168,18 +287,17 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)\n> \tsd->dev = NULL;\n> }\n> \n> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> -\t\t\t\t struct v4l2_async_notifier *notifier)\n> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)\n> {\n> -\tstruct v4l2_subdev *sd, *tmp;\n> \tstruct v4l2_async_subdev *asd;\n> +\tint ret;\n> \tint i;\n> \n> -\tif (!v4l2_dev || !notifier->num_subdevs ||\n> +\tif (!notifier->v4l2_dev == !notifier->sd || !notifier->num_subdevs ||\n\nWith the changes suggested below this can be changed to:\n\n\tif (!notifier->num_subdevs ||\n\nHowever, I have a question about that: why would it be wrong to call this with\nno subdevs in the list?\n\nIt's perfectly valid to have no subdevs at all. There may be a fixed incoming video\nstream that is not controlled by a subdev. We have a case like that in fact.\n\n> \t notifier->num_subdevs > V4L2_MAX_SUBDEVS)\n> \t\treturn -EINVAL;\n> \n> -\tnotifier->v4l2_dev = v4l2_dev;\n> +\tINIT_LIST_HEAD(¬ifier->list);\n> \tINIT_LIST_HEAD(¬ifier->waiting);\n> \tINIT_LIST_HEAD(¬ifier->done);\n> \n> @@ -203,18 +321,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> \n> \tmutex_lock(&list_lock);\n> \n> -\tlist_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {\n> -\t\tint ret;\n> -\n> -\t\tasd = v4l2_async_find_match(notifier, sd);\n> -\t\tif (!asd)\n> -\t\t\tcontinue;\n> -\n> -\t\tret = v4l2_async_match_notify(notifier, sd, asd);\n> -\t\tif (ret < 0) {\n> -\t\t\tmutex_unlock(&list_lock);\n> -\t\t\treturn ret;\n> -\t\t}\n> +\tret = v4l2_async_notifier_try_all_subdevs(notifier);\n> +\tif (ret) {\n> +\t\tmutex_unlock(&list_lock);\n> +\t\treturn ret;\n> \t}\n> \n> \t/* Keep also completed notifiers on the list */\n> @@ -224,28 +334,67 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> \n> \treturn 0;\n> }\n> +\n> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> +\t\t\t\t struct v4l2_async_notifier *notifier)\n> +{\n> +\tif (!v4l2_dev)\n\nI'd change this to:\n\n\tif (!v4l2_dev || notifier->sd)\n\n> +\t\treturn -EINVAL;\n> +\n> +\tnotifier->v4l2_dev = v4l2_dev;\n> +\n> +\treturn __v4l2_async_notifier_register(notifier);\n> +}\n> EXPORT_SYMBOL(v4l2_async_notifier_register);\n> \n> -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,\n> +\t\t\t\t\tstruct v4l2_async_notifier *notifier)\n> {\n> -\tstruct v4l2_subdev *sd, *tmp;\n> +\tif (!sd)\n\nand this to:\n\n\tif (!sd || notifier->v4l2_dev)\n\n> +\t\treturn -EINVAL;\n> \n> -\tif (!notifier->v4l2_dev)\n> -\t\treturn;\n> +\tnotifier->sd = sd;\n> \n> -\tmutex_lock(&list_lock);\n> +\treturn __v4l2_async_notifier_register(notifier);\n> +}\n> +EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);\n> \n> -\tlist_del(¬ifier->list);\n> +/* Unbind all sub-devices in the notifier tree. */\n> +static void v4l2_async_notifier_unbind_all_subdevs(\n> +\tstruct v4l2_async_notifier *notifier)\n> +{\n> +\tstruct v4l2_subdev *sd, *tmp;\n> \n> \tlist_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {\n> +\t\tstruct v4l2_async_notifier *subdev_notifier =\n> +\t\t\tv4l2_async_get_subdev_notifier(sd);\n> +\n> +\t\tif (subdev_notifier)\n> +\t\t\tv4l2_async_notifier_unbind_all_subdevs(subdev_notifier);\n> +\n> \t\tv4l2_async_cleanup(sd);\n> \n> \t\tv4l2_async_notifier_call_unbind(notifier, sd, sd->asd);\n> +\n> +\t\tlist_del(&sd->async_list);\n> +\t\tlist_add(&sd->async_list, &subdev_list);\n> \t}\n> \n> -\tmutex_unlock(&list_lock);\n> +\tnotifier->master = NULL;\n> +}\n> +\n> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> +{\n> +\tif (!notifier->v4l2_dev && !notifier->sd)\n> +\t\treturn;\n> \n> -\tnotifier->v4l2_dev = NULL;\n> +\tmutex_lock(&list_lock);\n> +\n> +\tv4l2_async_notifier_unbind_all_subdevs(notifier);\n> +\n> +\tlist_del(¬ifier->list);\n> +\n> +\tmutex_unlock(&list_lock);\n> }\n> EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n> \n> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n> index 3bc8a7c0d83f..12739be44bd1 100644\n> --- a/include/media/v4l2-async.h\n> +++ b/include/media/v4l2-async.h\n> @@ -102,7 +102,9 @@ struct v4l2_async_notifier_operations {\n> * @num_subdevs: number of subdevices used in the subdevs array\n> * @max_subdevs: number of subdevices allocated in the subdevs array\n> * @subdevs:\tarray of pointers to subdevice descriptors\n> - * @v4l2_dev:\tpointer to struct v4l2_device\n> + * @v4l2_dev:\tv4l2_device of the master, for subdev notifiers NULL\n> + * @sd:\t\tsub-device that registered the notifier, NULL otherwise\n> + * @master:\tmaster notifier carrying @v4l2_dev\n\nI think this description is out of date. It is really the parent notifier,\nright? Should 'master' be renamed to 'parent'?\n\nSame problem with the description of @v4l2_dev: it's the v4l2_device of the\nroot/top-level notifier.\n\n> * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n> * @done:\tlist of struct v4l2_subdev, already probed\n> * @list:\tmember in a global list of notifiers\n> @@ -113,6 +115,8 @@ struct v4l2_async_notifier {\n> \tunsigned int max_subdevs;\n> \tstruct v4l2_async_subdev **subdevs;\n> \tstruct v4l2_device *v4l2_dev;\n> +\tstruct v4l2_subdev *sd;\n> +\tstruct v4l2_async_notifier *master;\n> \tstruct list_head waiting;\n> \tstruct list_head done;\n> \tstruct list_head list;\n> @@ -128,6 +132,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> \t\t\t\t struct v4l2_async_notifier *notifier);\n> \n> /**\n> + * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous\n> + *\t\t\t\t\t notifier for a sub-device\n> + *\n> + * @sd: pointer to &struct v4l2_subdev\n> + * @notifier: pointer to &struct v4l2_async_notifier\n> + */\n> +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,\n> +\t\t\t\t\tstruct v4l2_async_notifier *notifier);\n> +\n> +/**\n> * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier\n> *\n> * @notifier: pointer to &struct v4l2_async_notifier\n> \n\nThis v8 is much better and is getting close.\n\nThanks!\n\n\tHans\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnHJ051Ycz9sBZ\n\tfor ;\n\tWed, 6 Sep 2017 18:46:40 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752038AbdIFIqi (ORCPT\n\t);\n\tWed, 6 Sep 2017 04:46:38 -0400","from lb3-smtp-cloud7.xs4all.net ([194.109.24.31]:35542 \"EHLO\n\tlb3-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1750832AbdIFIqg (ORCPT\n\t); Wed, 6 Sep 2017 04:46:36 -0400","from [192.168.2.10] ([212.251.195.8])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pVyhdhFOUb2snpVyldpUfU; Wed, 06 Sep 2017 10:46:35 +0200"],"Subject":"Re: [PATCH v8 14/21] v4l: async: Allow binding notifiers to\n\tsub-devices","To":"Sakari Ailus , linux-media@vger.kernel.org","Cc":"niklas.soderlund@ragnatech.se, robh@kernel.org,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tpavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-15-sakari.ailus@linux.intel.com>","From":"Hans Verkuil ","Message-ID":"<910b3d01-a8a4-2363-4b24-ed0edd1e1f4d@xs4all.nl>","Date":"Wed, 6 Sep 2017 10:46:31 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170905130553.1332-15-sakari.ailus@linux.intel.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfAaTcS/kathuvbvsK5cv1jQxoNpVYLmZYA77fQV6Y0oCAdchFRvjIyHvBULuwF20Ve0/krKQQa+HhIDAdaVPVfwRvU1ImSqVPd/Va4NvlDYjxPE+hjBx\n\tDVB5XpThI/Y4QzKP/+n9khZ4pTvkggRJ8yV+OQQ4o9SbEZgGn+s2Y1DKk4Ma4wCAuFa87xR3XRfm0GkDFXgzkIDdaXZ2nWkhY6PW0Ddgwjoj2MjKy+lUzUA7\n\tFIpZ3jqWjGZFttrWa5tKCXmikygooAFJlpRrspADyvvGbE8XAAim53Qxm+YQuNSm2LO2WSuPEQXGrLrNTaE62sFtYjKyRLWpBLgLZJFNW8i0rh7+W9LfJWA3\n\tOMK5LcklaPQrOWoBHUWa3uWaR3my70QUTWT1eqEh2mtesYQ58P3MTgesQNH6t4w2R5G1hFaxtMl1rHf9cpbqDk9pe6jRuJWYtWaN7FAy6IbxfiU16WQ/DlOr\n\t7k/4vs9ii0pao9a0","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764556,"web_url":"http://patchwork.ozlabs.org/comment/1764556/","msgid":"<20170907073446.ajxsscekwrbfbtgk@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-09-07T07:34:46","subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Hans,\n\nOn Wed, Sep 06, 2017 at 09:41:40AM +0200, Hans Verkuil wrote:\n> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> > The current practice is that drivers iterate over their endpoints and\n> > parse each endpoint separately. This is very similar in a number of\n> > drivers, implement a generic function for the job. Driver specific matters\n> > can be taken into account in the driver specific callback.\n> > \n> > Signed-off-by: Sakari Ailus \n> > ---\n> > drivers/media/v4l2-core/v4l2-async.c | 19 +++++\n> > drivers/media/v4l2-core/v4l2-fwnode.c | 140 ++++++++++++++++++++++++++++++++++\n> > include/media/v4l2-async.h | 24 +++++-\n> > include/media/v4l2-fwnode.h | 53 +++++++++++++\n> > 4 files changed, 234 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> > index 3d81ff6a496f..7bd595c4094a 100644\n> > --- a/drivers/media/v4l2-core/v4l2-async.c\n> > +++ b/drivers/media/v4l2-core/v4l2-async.c\n> > @@ -22,6 +22,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n> > @@ -224,6 +225,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> > }\n> > EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n> > \n> > +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)\n> > +{\n> > +\tunsigned int i;\n> > +\n> > +\tif (!notifier->max_subdevs)\n> > +\t\treturn;\n> > +\n> > +\tfor (i = 0; i < notifier->num_subdevs; i++)\n> > +\t\tkfree(notifier->subdevs[i]);\n> > +\n> > +\tnotifier->max_subdevs = 0;\n> > +\tnotifier->num_subdevs = 0;\n> > +\n> > +\tkvfree(notifier->subdevs);\n> > +\tnotifier->subdevs = NULL;\n> > +}\n> > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);\n> > +\n> > int v4l2_async_register_subdev(struct v4l2_subdev *sd)\n> > {\n> > \tstruct v4l2_async_notifier *notifier;\n> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c\n> > index 706f9e7b90f1..e6932d7d47b6 100644\n> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c\n> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c\n> > @@ -19,6 +19,7 @@\n> > */\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -26,6 +27,7 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > #include \n> > \n> > enum v4l2_fwnode_bus_type {\n> > @@ -313,6 +315,144 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)\n> > }\n> > EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);\n> > \n> > +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,\n> > +\t\t\t\t unsigned int max_subdevs)\n> > +{\n> > +\tstruct v4l2_async_subdev **subdevs;\n> > +\n> > +\tif (max_subdevs <= notifier->max_subdevs)\n> > +\t\treturn 0;\n> > +\n> > +\tsubdevs = kvmalloc_array(\n> > +\t\tmax_subdevs, sizeof(*notifier->subdevs),\n> > +\t\tGFP_KERNEL | __GFP_ZERO);\n> > +\tif (!subdevs)\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\tif (notifier->subdevs) {\n> > +\t\tmemcpy(subdevs, notifier->subdevs,\n> > +\t\t sizeof(*subdevs) * notifier->num_subdevs);\n> > +\n> > +\t\tkvfree(notifier->subdevs);\n> > +\t}\n> > +\n> > +\tnotifier->subdevs = subdevs;\n> > +\tnotifier->max_subdevs = max_subdevs;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int v4l2_async_notifier_fwnode_parse_endpoint(\n> > +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> > +\tstruct fwnode_handle *endpoint, unsigned int asd_struct_size,\n> > +\tint (*parse_endpoint)(struct device *dev,\n> > +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> > +\t\t\t struct v4l2_async_subdev *asd))\n> > +{\n> > +\tstruct v4l2_async_subdev *asd;\n> > +\tstruct v4l2_fwnode_endpoint *vep;\n> > +\tstruct fwnode_endpoint ep;\n> > +\tint ret = 0;\n> > +\n> > +\tasd = kzalloc(asd_struct_size, GFP_KERNEL);\n> > +\tif (!asd)\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\tasd->match.fwnode.fwnode =\n> > +\t\tfwnode_graph_get_remote_port_parent(endpoint);\n> > +\tif (!asd->match.fwnode.fwnode) {\n> > +\t\tdev_warn(dev, \"bad remote port parent\\n\");\n> > +\t\tret = -EINVAL;\n> > +\t\tgoto out_err;\n> > +\t}\n> > +\n> > +\t/* Ignore endpoints the parsing of which failed. */\n> > +\tvep = v4l2_fwnode_endpoint_alloc_parse(endpoint);\n> > +\tif (IS_ERR(vep)) {\n> > +\t\tret = PTR_ERR(vep);\n> > +\t\tdev_warn(dev, \"unable to parse V4L2 fwnode endpoint (%d)\\n\",\n> > +\t\t\t ret);\n> > +\t\tgoto out_err;\n> > +\t}\n> > +\n> > +\tep = vep->base;\n> > +\n> > +\tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n> > +\tv4l2_fwnode_endpoint_free(vep);\n> > +\tif (ret == -ENOTCONN) {\n> > +\t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", ep.port, ep.id);\n> > +\t\tkfree(asd);\n> \n> Shouldn't there be a call to fwnode_handle_put()?\n\nActually no. But your hunch is right in the sense that I think there are\nissues. The fwnode_endpoint (as of_endpoint) contains the fwnode which is\nreferenced, but no reference is taken here. One couldn't be released either\nlater on, as the fwnode field is const.\n\nI guess this is almost fine as long as the fwnode field is used for pointer\ncomparison and nothing else but you can never be sure what drivers actually\ndo.\n\nThis actually should be addressed for both OF and fwnode but it's mostly\nindependent of this patchset. Luckily there are not *that* many users of\nthis. The V4L2 fwnode interface that allocates and releases endpoints makes\nthis quite a bit easier actually.\n\n> \n> > +\t\treturn 0;\n> > +\t} else if (ret < 0) {\n> > +\t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n> > +\t\t\t ep.port, ep.id, ret);\n> > +\t\tgoto out_err;\n> > +\t}\n> \n> I think this would be better and it avoids the need for the ep local variable:\n> \n> \tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n> \tif (ret == -ENOTCONN)\n> \t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", vep->base.port, vep->base.id);\n> \telse if (ret < 0)\n> \t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n> \t\t\t vep->base.port, vep->base.id, ret);\n> \tv4l2_fwnode_endpoint_free(vep);\n> \tif (ret < 0)\n> \t\tgoto out_err;\n> \n> And the 'return ret;' below would become:\n> \n> \treturn ret == -ENOTCONN ? 0 : ret;\n\nLooks good to me, I'll use it.\n\n> \n> > +\n> > +\tasd->match_type = V4L2_ASYNC_MATCH_FWNODE;\n> > +\tnotifier->subdevs[notifier->num_subdevs] = asd;\n> > +\tnotifier->num_subdevs++;\n> > +\n> > +\treturn 0;\n> > +\n> > +out_err:\n> > +\tfwnode_handle_put(asd->match.fwnode.fwnode);\n> > +\tkfree(asd);\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +int v4l2_async_notifier_parse_fwnode_endpoints(\n> > +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> > +\tsize_t asd_struct_size,\n> > +\tint (*parse_endpoint)(struct device *dev,\n> > +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> > +\t\t\t struct v4l2_async_subdev *asd))\n> > +{\n> > +\tstruct fwnode_handle *fwnode = NULL;\n> > +\tunsigned int max_subdevs = notifier->max_subdevs;\n> > +\tint ret;\n> > +\n> > +\tif (asd_struct_size < sizeof(struct v4l2_async_subdev))\n> \n> I think this can be a WARN_ON or a WARN_ON_ONCE. Up to you, though.\n\nIt'd be a driver bug, so either seems appropriate. I'll use WARN_ON().\n\n> \n> > +\t\treturn -EINVAL;\n> > +\n> > +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n> > +\t\t\t\t dev_fwnode(dev), fwnode)); )\n> > +\t\tif (fwnode_device_is_available(\n> > +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n> > +\t\t\tmax_subdevs++;\n> \n> I think I would prefer this as a simple for (;;):\n> \n> \tfor (;;) {\n\nI think for (;;) is misuse of the for loop. :-D\n\n> \t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n> \t\tif (fwnode == NULL)\n> \t\t\tbreak;\n> \t\t...\n> \t}\n> \n> Or alternatively as a do...while:\n> \n> \tdo {\n> \t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n> \t\tif (fwnode && fwnode_device_is_available(\n> \t\t\t fwnode_graph_get_port_parent(fwnode)))\n> \t\t\tmax_subdevs++;\n> \t} while (fwnode);\n> \n> Both are IMHO a bit more readable. I leave it up to you, though.\n> \n> > +\n> > +\t/* No subdevs to add? Return here. */\n> > +\tif (max_subdevs == notifier->max_subdevs)\n> > +\t\treturn 0;\n> > +\n> > +\tret = v4l2_async_notifier_realloc(notifier, max_subdevs);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n> > +\t\t\t\t dev_fwnode(dev), fwnode)); ) {\n> \n> Same comment as above.\n\nWhat I like in the original code is that managing looping over all\nendpoints takes place directly in the for () statement and does not spill\nover to where the individual endpoints are being handled.\n\nWhat would you think of adding a macro to do this? Say, we could write\nthis as:\n\nfwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) {\n\t...\n}\n\nThat'd have to go through linux-pm tree though, and we could change the\ncode here later on.\n\n> \n> > +\t\tif (!fwnode_device_is_available(\n> > +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {\n> > +\t\t\tret = -EINVAL;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tret = v4l2_async_notifier_fwnode_parse_endpoint(\n> > +\t\t\tdev, notifier, fwnode, asd_struct_size, parse_endpoint);\n> > +\t\tif (ret < 0)\n> > +\t\t\tbreak;\n> > +\t}\n> > +\n> > +\tfwnode_handle_put(fwnode);\n> > +\n> > +\treturn ret;\n> > +}\n> > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);\n> > +\n> > MODULE_LICENSE(\"GPL\");\n> > MODULE_AUTHOR(\"Sakari Ailus \");\n> > MODULE_AUTHOR(\"Sylwester Nawrocki \");\n> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n> > index c69d8c8a66d0..96fa1afc00dd 100644\n> > --- a/include/media/v4l2-async.h\n> > +++ b/include/media/v4l2-async.h\n> > @@ -18,7 +18,6 @@ struct device;\n> > struct device_node;\n> > struct v4l2_device;\n> > struct v4l2_subdev;\n> > -struct v4l2_async_notifier;\n> > \n> > /* A random max subdevice number, used to allocate an array on stack */\n> > #define V4L2_MAX_SUBDEVS 128U\n> > @@ -50,6 +49,10 @@ enum v4l2_async_match_type {\n> > * @match:\tunion of per-bus type matching data sets\n> > * @list:\tused to link struct v4l2_async_subdev objects, waiting to be\n> > *\t\tprobed, to a notifier->waiting list\n> > + *\n> > + * When this struct is used as a member in a driver specific struct,\n> > + * the driver specific struct shall contain the @struct\n> > + * v4l2_async_subdev as its first member.\n> > */\n> > struct v4l2_async_subdev {\n> > \tenum v4l2_async_match_type match_type;\n> > @@ -78,7 +81,8 @@ struct v4l2_async_subdev {\n> > /**\n> > * struct v4l2_async_notifier - v4l2_device notifier data\n> > *\n> > - * @num_subdevs: number of subdevices\n> > + * @num_subdevs: number of subdevices used in the subdevs array\n> > + * @max_subdevs: number of subdevices allocated in the subdevs array\n> > * @subdevs:\tarray of pointers to subdevice descriptors\n> > * @v4l2_dev:\tpointer to struct v4l2_device\n> > * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n> > @@ -90,6 +94,7 @@ struct v4l2_async_subdev {\n> > */\n> > struct v4l2_async_notifier {\n> > \tunsigned int num_subdevs;\n> > +\tunsigned int max_subdevs;\n> > \tstruct v4l2_async_subdev **subdevs;\n> > \tstruct v4l2_device *v4l2_dev;\n> > \tstruct list_head waiting;\n> > @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);\n> > \n> > /**\n> > + * v4l2_async_notifier_release - release notifier resources\n> > + * @notifier: the notifier the resources of which are to be released\n> > + *\n> > + * Release memory resources related to a notifier, including the async\n> > + * sub-devices allocated for the purposes of the notifier. The user is\n> > + * responsible for releasing the notifier's resources after calling\n> > + * @v4l2_async_notifier_parse_fwnode_endpoints.\n> > + *\n> > + * There is no harm from calling v4l2_async_notifier_release in other\n> > + * cases as long as its memory has been zeroed after it has been\n> > + * allocated.\n> > + */\n> > +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);\n> > +\n> > +/**\n> > * v4l2_async_register_subdev - registers a sub-device to the asynchronous\n> > * \tsubdevice framework\n> > *\n> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h\n> > index 68eb22ba571b..6d125f26ec84 100644\n> > --- a/include/media/v4l2-fwnode.h\n> > +++ b/include/media/v4l2-fwnode.h\n> > @@ -25,6 +25,8 @@\n> > #include \n> > \n> > struct fwnode_handle;\n> > +struct v4l2_async_notifier;\n> > +struct v4l2_async_subdev;\n> > \n> > #define V4L2_FWNODE_CSI2_MAX_DATA_LANES\t4\n> > \n> > @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,\n> > */\n> > void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);\n> > \n> > +/**\n> > + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a\n> > + *\t\t\t\t\t\tdevice node\n> > + * @dev: the device the endpoints of which are to be parsed\n> > + * @notifier: notifier for @dev\n> > + * @asd_struct_size: size of the driver's async sub-device struct, including\n> > + *\t\t sizeof(struct v4l2_async_subdev). The &struct\n> > + *\t\t v4l2_async_subdev shall be the first member of\n> > + *\t\t the driver's async sub-device struct, i.e. both\n> > + *\t\t begin at the same memory address.\n> > + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode\n> > + *\t\t endpoint. Optional.\n> > + *\t\t Return: %0 on success\n> > + *\t\t\t %-ENOTCONN if the endpoint is to be skipped but this\n> > + *\t\t\t\t should not be considered as an error\n> > + *\t\t\t %-EINVAL if the endpoint configuration is invalid\n> > + *\n> > + * Parse the fwnode endpoints of the @dev device and populate the async sub-\n> > + * devices array of the notifier. The @parse_endpoint callback function is\n> > + * called for each endpoint with the corresponding async sub-device pointer to\n> > + * let the caller initialize the driver-specific part of the async sub-device\n> > + * structure.\n> > + *\n> > + * The notifier memory shall be zeroed before this function is called on the\n> > + * notifier.\n> > + *\n> > + * This function may not be called on a registered notifier and may be called on\n> > + * a notifier only once. When using this function, the user may not access the\n> > + * notifier's subdevs array nor change notifier's num_subdevs field, these are\n> > + * reserved for the framework's internal use only.\n> \n> The rvin_digital_graph_init() function accesses the subdevs array.\n> \n> I still don't like this sentence, and I still think it should be dropped. Either that\n> or completely rewritten.\n\nHow about:\n\nWhen using this function on a notifier, the user is not allowed to change\nthe notifier's subdevs array, take references to the subdevs array itself\nnor change the notifier's num_subdevs field. This is because the function\nallocates and reallocates the subdevs array based on parsing endpoints.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnsfh0dzbz9sRY\n\tfor ;\n\tThu, 7 Sep 2017 17:34:52 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753794AbdIGHeu (ORCPT\n\t);\n\tThu, 7 Sep 2017 03:34:50 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:60972 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1751398AbdIGHet (ORCPT\n\t); Thu, 7 Sep 2017 03:34:49 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id 4D8FA600E7;\n\tThu, 7 Sep 2017 10:34:47 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dprKo-0002pq-SZ; Thu, 07 Sep 2017 10:34:46 +0300"],"Date":"Thu, 7 Sep 2017 10:34:46 +0300","From":"Sakari Ailus ","To":"Hans Verkuil ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","Subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","Message-ID":"<20170907073446.ajxsscekwrbfbtgk@valkosipuli.retiisi.org.uk>","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-7-sakari.ailus@linux.intel.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764563,"web_url":"http://patchwork.ozlabs.org/comment/1764563/","msgid":"<20170907075128.2rhfymo6jnzdt33d@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-09-07T07:51:29","subject":"Re: [PATCH v8 12/21] v4l: async: Introduce helpers for calling async\n\tops callbacks","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"On Wed, Sep 06, 2017 at 09:50:36AM +0200, Hans Verkuil wrote:\n> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> > Add three helper functions to call async operations callbacks. Besides\n> > simplifying callbacks, this allows async notifiers to have no ops set,\n> > i.e. it can be left NULL.\n> \n> What is the use-case of that?\n\nGoing forward, the sub-notifiers that are just for binding associated\ndevices (later in the patchset, 17th and 18th patches) there is no need for\ncallbacks. This allows having no ops either.\n\n> \n> Anyway:\n> \n> Acked-by: Hans Verkuil \n\nThanks!","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnt1z3VgBz9s82\n\tfor ;\n\tThu, 7 Sep 2017 17:51:35 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754509AbdIGHvc (ORCPT\n\t);\n\tThu, 7 Sep 2017 03:51:32 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:32966 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1754350AbdIGHvb (ORCPT\n\t); Thu, 7 Sep 2017 03:51:31 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id C6AC0600E7;\n\tThu, 7 Sep 2017 10:51:29 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dpraz-0002py-AS; Thu, 07 Sep 2017 10:51:29 +0300"],"Date":"Thu, 7 Sep 2017 10:51:29 +0300","From":"Sakari Ailus ","To":"Hans Verkuil ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","Subject":"Re: [PATCH v8 12/21] v4l: async: Introduce helpers for calling async\n\tops callbacks","Message-ID":"<20170907075128.2rhfymo6jnzdt33d@valkosipuli.retiisi.org.uk>","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-13-sakari.ailus@linux.intel.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764584,"web_url":"http://patchwork.ozlabs.org/comment/1764584/","msgid":"<20170907083209.3xtaxwhrfmgrtpfz@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-09-07T08:32:10","subject":"Re: [PATCH v8 14/21] v4l: async: Allow binding notifiers to\n\tsub-devices","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Hans,\n\nOn Wed, Sep 06, 2017 at 10:46:31AM +0200, Hans Verkuil wrote:\n> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> > Registering a notifier has required the knowledge of struct v4l2_device\n> > for the reason that sub-devices generally are registered to the\n> > v4l2_device (as well as the media device, also available through\n> > v4l2_device).\n> > \n> > This information is not available for sub-device drivers at probe time.\n> > \n> > What this patch does is that it allows registering notifiers without\n> > having v4l2_device around. Instead the sub-device pointer is stored to the\n> \n> to -> in\n\nFixed.\n\n> \n> > notifier. Once the sub-device of the driver that registered the notifier\n> > is registered, the notifier will gain the knowledge of the v4l2_device,\n> > and the binding of async sub-devices from the sub-device driver's notifier\n> > may proceed.\n> > \n> > The master notifier's complete callback is only called when all sub-device\n> > notifiers are completed.\n> > \n> > Signed-off-by: Sakari Ailus \n> > ---\n> > drivers/media/v4l2-core/v4l2-async.c | 209 ++++++++++++++++++++++++++++++-----\n> > include/media/v4l2-async.h | 16 ++-\n> > 2 files changed, 194 insertions(+), 31 deletions(-)\n> > \n> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> > index 79f216723a3f..620b2cd29fc3 100644\n> > --- a/drivers/media/v4l2-core/v4l2-async.c\n> > +++ b/drivers/media/v4l2-core/v4l2-async.c\n> > @@ -53,6 +53,10 @@ static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)\n> > \treturn n->ops->complete(n);\n> > }\n> > \n> > +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> > +\t\t\t\t struct v4l2_subdev *sd,\n> > +\t\t\t\t struct v4l2_async_subdev *asd);\n> > +\n> > static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n> > {\n> > #if IS_ENABLED(CONFIG_I2C)\n> > @@ -129,14 +133,119 @@ static struct v4l2_async_subdev *v4l2_async_find_match(\n> > \treturn NULL;\n> > }\n> > \n> > +/* Get the sub-device notifier registered by a sub-device driver. */\n> > +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(\n> > +\tstruct v4l2_subdev *sd)\n> > +{\n> > +\tstruct v4l2_async_notifier *n;\n> > +\n> > +\tlist_for_each_entry(n, ¬ifier_list, list)\n> > +\t\tif (n->sd == sd)\n> > +\t\t\treturn n;\n> > +\n> > +\treturn NULL;\n> > +}\n> > +\n> > +/* Return true if all sub-device notifiers are complete, false otherwise. */\n> > +static bool v4l2_async_subdev_notifiers_complete(\n> > +\tstruct v4l2_async_notifier *notifier)\n> > +{\n> > +\tstruct v4l2_subdev *sd;\n> > +\n> > +\tif (!list_empty(¬ifier->waiting))\n> > +\t\treturn false;\n> > +\n> > +\tlist_for_each_entry(sd, ¬ifier->done, async_list) {\n> > +\t\tstruct v4l2_async_notifier *subdev_notifier =\n> > +\t\t\tv4l2_async_get_subdev_notifier(sd);\n> > +\n> > +\t\tif (!subdev_notifier)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (!v4l2_async_subdev_notifiers_complete(subdev_notifier))\n> > +\t\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/* Get v4l2_device related to the notifier if one can be found. */\n> > +static struct v4l2_device *v4l2_async_notifier_get_v4l2_dev(\n> > +\tstruct v4l2_async_notifier *notifier)\n> > +{\n> > +\twhile (notifier->master)\n> > +\t\tnotifier = notifier->master;\n> > +\n> > +\treturn notifier->v4l2_dev;\n> > +}\n> > +\n> > +/* Test all async sub-devices in a notifier for a match. */\n> > +static int v4l2_async_notifier_try_all_subdevs(\n> > +\tstruct v4l2_async_notifier *notifier)\n> > +{\n> > +\tstruct v4l2_subdev *sd, *tmp;\n> > +\n> > +\tif (!v4l2_async_notifier_get_v4l2_dev(notifier))\n> > +\t\treturn 0;\n> > +\n> > +\tlist_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {\n> > +\t\tstruct v4l2_async_subdev *asd;\n> > +\t\tint ret;\n> > +\n> > +\t\tasd = v4l2_async_find_match(notifier, sd);\n> > +\t\tif (!asd)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tret = v4l2_async_match_notify(notifier, sd, asd);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/* Try completing a notifier. */\n> > +static int v4l2_async_notifier_try_complete(\n> > +\tstruct v4l2_async_notifier *notifier)\n> > +{\n> > +\tdo {\n> > +\t\tint ret;\n> > +\n> > +\t\t/* Any local async sub-devices left? */\n> > +\t\tif (!list_empty(¬ifier->waiting))\n> > +\t\t\treturn 0;\n> > +\n> > +\t\t/*\n> > +\t\t * Any sub-device notifiers waiting for async subdevs\n> > +\t\t * to be bound?\n> > +\t\t */\n> > +\t\tif (!v4l2_async_subdev_notifiers_complete(notifier))\n> > +\t\t\treturn 0;\n> > +\n> > +\t\t/* Proceed completing the notifier */\n> > +\t\tret = v4l2_async_notifier_call_complete(notifier);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/*\n> > +\t\t * Obtain notifier's master. If there is one, repeat\n> > +\t\t * the process, otherwise we're done here.\n> > +\t\t */\n> > +\t} while ((notifier = notifier->master));\n> \n> I'd change this to:\n> \n> \t\tnotifier = notifier->master;\n> \t} while (notifier);\n\nI prefer the original for the same reason as in related case: the loop\ncondition as well as obtaining the next entry is better separated from\nwhere the real work on the entries takes place.\n\n> \n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> > \t\t\t\t struct v4l2_subdev *sd,\n> > \t\t\t\t struct v4l2_async_subdev *asd)\n> > {\n> > +\tstruct v4l2_async_notifier *subdev_notifier;\n> > \tint ret;\n> > \n> > -\tret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);\n> > -\tif (ret < 0)\n> > +\tret = v4l2_device_register_subdev(\n> > +\t\tv4l2_async_notifier_get_v4l2_dev(notifier), sd);\n> > +\tif (ret)\n> > \t\treturn ret;\n> > \n> > \tret = v4l2_async_notifier_call_bound(notifier, sd, asd);\n> > @@ -153,10 +262,20 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,\n> > \t/* Move from the global subdevice list to notifier's done */\n> > \tlist_move(&sd->async_list, ¬ifier->done);\n> > \n> > -\tif (list_empty(¬ifier->waiting))\n> > -\t\treturn v4l2_async_notifier_call_complete(notifier);\n> > +\t/*\n> > +\t * See if the sub-device has a notifier. If it does, proceed\n> > +\t * with checking for its async sub-devices.\n> > +\t */\n> > +\tsubdev_notifier = v4l2_async_get_subdev_notifier(sd);\n> > +\tif (subdev_notifier && !subdev_notifier->master) {\n> > +\t\tsubdev_notifier->master = notifier;\n> > +\t\tret = v4l2_async_notifier_try_all_subdevs(subdev_notifier);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t}\n> > \n> > -\treturn 0;\n> > +\t/* Try completing the notifier and its master(s). */\n> > +\treturn v4l2_async_notifier_try_complete(notifier);\n> > }\n> > \n> > static void v4l2_async_cleanup(struct v4l2_subdev *sd)\n> > @@ -168,18 +287,17 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)\n> > \tsd->dev = NULL;\n> > }\n> > \n> > -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> > -\t\t\t\t struct v4l2_async_notifier *notifier)\n> > +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)\n> > {\n> > -\tstruct v4l2_subdev *sd, *tmp;\n> > \tstruct v4l2_async_subdev *asd;\n> > +\tint ret;\n> > \tint i;\n> > \n> > -\tif (!v4l2_dev || !notifier->num_subdevs ||\n> > +\tif (!notifier->v4l2_dev == !notifier->sd || !notifier->num_subdevs ||\n> \n> With the changes suggested below this can be changed to:\n> \n> \tif (!notifier->num_subdevs ||\n> \n> However, I have a question about that: why would it be wrong to call this with\n> no subdevs in the list?\n> \n> It's perfectly valid to have no subdevs at all. There may be a fixed incoming video\n> stream that is not controlled by a subdev. We have a case like that in fact.\n\nI added a separate patch for that earlier in the set.\n\n> \n> > \t notifier->num_subdevs > V4L2_MAX_SUBDEVS)\n> > \t\treturn -EINVAL;\n> > \n> > -\tnotifier->v4l2_dev = v4l2_dev;\n> > +\tINIT_LIST_HEAD(¬ifier->list);\n> > \tINIT_LIST_HEAD(¬ifier->waiting);\n> > \tINIT_LIST_HEAD(¬ifier->done);\n> > \n> > @@ -203,18 +321,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> > \n> > \tmutex_lock(&list_lock);\n> > \n> > -\tlist_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {\n> > -\t\tint ret;\n> > -\n> > -\t\tasd = v4l2_async_find_match(notifier, sd);\n> > -\t\tif (!asd)\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tret = v4l2_async_match_notify(notifier, sd, asd);\n> > -\t\tif (ret < 0) {\n> > -\t\t\tmutex_unlock(&list_lock);\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > +\tret = v4l2_async_notifier_try_all_subdevs(notifier);\n> > +\tif (ret) {\n> > +\t\tmutex_unlock(&list_lock);\n> > +\t\treturn ret;\n> > \t}\n> > \n> > \t/* Keep also completed notifiers on the list */\n> > @@ -224,28 +334,67 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> > \n> > \treturn 0;\n> > }\n> > +\n> > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> > +\t\t\t\t struct v4l2_async_notifier *notifier)\n> > +{\n> > +\tif (!v4l2_dev)\n> \n> I'd change this to:\n> \n> \tif (!v4l2_dev || notifier->sd)\n\nDone.\n\n> \n> > +\t\treturn -EINVAL;\n> > +\n> > +\tnotifier->v4l2_dev = v4l2_dev;\n> > +\n> > +\treturn __v4l2_async_notifier_register(notifier);\n> > +}\n> > EXPORT_SYMBOL(v4l2_async_notifier_register);\n> > \n> > -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> > +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,\n> > +\t\t\t\t\tstruct v4l2_async_notifier *notifier)\n> > {\n> > -\tstruct v4l2_subdev *sd, *tmp;\n> > +\tif (!sd)\n> \n> and this to:\n> \n> \tif (!sd || notifier->v4l2_dev)\n\nDone as well. And removed the interesting-looking check from\n__v4l2_async_subdev_notifier_register(). :-)\n\n> \n> > +\t\treturn -EINVAL;\n> > \n> > -\tif (!notifier->v4l2_dev)\n> > -\t\treturn;\n> > +\tnotifier->sd = sd;\n> > \n> > -\tmutex_lock(&list_lock);\n> > +\treturn __v4l2_async_notifier_register(notifier);\n> > +}\n> > +EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);\n> > \n> > -\tlist_del(¬ifier->list);\n> > +/* Unbind all sub-devices in the notifier tree. */\n> > +static void v4l2_async_notifier_unbind_all_subdevs(\n> > +\tstruct v4l2_async_notifier *notifier)\n> > +{\n> > +\tstruct v4l2_subdev *sd, *tmp;\n> > \n> > \tlist_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {\n> > +\t\tstruct v4l2_async_notifier *subdev_notifier =\n> > +\t\t\tv4l2_async_get_subdev_notifier(sd);\n> > +\n> > +\t\tif (subdev_notifier)\n> > +\t\t\tv4l2_async_notifier_unbind_all_subdevs(subdev_notifier);\n> > +\n> > \t\tv4l2_async_cleanup(sd);\n> > \n> > \t\tv4l2_async_notifier_call_unbind(notifier, sd, sd->asd);\n> > +\n> > +\t\tlist_del(&sd->async_list);\n> > +\t\tlist_add(&sd->async_list, &subdev_list);\n> > \t}\n> > \n> > -\tmutex_unlock(&list_lock);\n> > +\tnotifier->master = NULL;\n> > +}\n> > +\n> > +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> > +{\n> > +\tif (!notifier->v4l2_dev && !notifier->sd)\n> > +\t\treturn;\n> > \n> > -\tnotifier->v4l2_dev = NULL;\n> > +\tmutex_lock(&list_lock);\n> > +\n> > +\tv4l2_async_notifier_unbind_all_subdevs(notifier);\n> > +\n> > +\tlist_del(¬ifier->list);\n> > +\n> > +\tmutex_unlock(&list_lock);\n> > }\n> > EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n> > \n> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n> > index 3bc8a7c0d83f..12739be44bd1 100644\n> > --- a/include/media/v4l2-async.h\n> > +++ b/include/media/v4l2-async.h\n> > @@ -102,7 +102,9 @@ struct v4l2_async_notifier_operations {\n> > * @num_subdevs: number of subdevices used in the subdevs array\n> > * @max_subdevs: number of subdevices allocated in the subdevs array\n> > * @subdevs:\tarray of pointers to subdevice descriptors\n> > - * @v4l2_dev:\tpointer to struct v4l2_device\n> > + * @v4l2_dev:\tv4l2_device of the master, for subdev notifiers NULL\n> > + * @sd:\t\tsub-device that registered the notifier, NULL otherwise\n> > + * @master:\tmaster notifier carrying @v4l2_dev\n> \n> I think this description is out of date. It is really the parent notifier,\n> right? Should 'master' be renamed to 'parent'?\n\nYou could view it as one, yes. What is known is that the notifier is\nrelated, and through which the v4l2_dev can be found. I'll rename it as\n\"parent\".\n\nI'll use root in the commit message as well.\n\n> \n> Same problem with the description of @v4l2_dev: it's the v4l2_device of the\n> root/top-level notifier.\n\n\"v4l2_device of the root notifier, NULL otherwise\"?\n\n> \n> > * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n> > * @done:\tlist of struct v4l2_subdev, already probed\n> > * @list:\tmember in a global list of notifiers\n> > @@ -113,6 +115,8 @@ struct v4l2_async_notifier {\n> > \tunsigned int max_subdevs;\n> > \tstruct v4l2_async_subdev **subdevs;\n> > \tstruct v4l2_device *v4l2_dev;\n> > +\tstruct v4l2_subdev *sd;\n> > +\tstruct v4l2_async_notifier *master;\n> > \tstruct list_head waiting;\n> > \tstruct list_head done;\n> > \tstruct list_head list;\n> > @@ -128,6 +132,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> > \t\t\t\t struct v4l2_async_notifier *notifier);\n> > \n> > /**\n> > + * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous\n> > + *\t\t\t\t\t notifier for a sub-device\n> > + *\n> > + * @sd: pointer to &struct v4l2_subdev\n> > + * @notifier: pointer to &struct v4l2_async_notifier\n> > + */\n> > +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,\n> > +\t\t\t\t\tstruct v4l2_async_notifier *notifier);\n> > +\n> > +/**\n> > * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier\n> > *\n> > * @notifier: pointer to &struct v4l2_async_notifier\n> > \n> \n> This v8 is much better and is getting close.\n\nThanks!","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xntwx5G5mz9sRV\n\tfor ;\n\tThu, 7 Sep 2017 18:32:17 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754776AbdIGIcO (ORCPT\n\t);\n\tThu, 7 Sep 2017 04:32:14 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:33396 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1754073AbdIGIcN (ORCPT\n\t); Thu, 7 Sep 2017 04:32:13 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id E2941600F2;\n\tThu, 7 Sep 2017 11:32:10 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dpsEM-0002sO-B6; Thu, 07 Sep 2017 11:32:10 +0300"],"Date":"Thu, 7 Sep 2017 11:32:10 +0300","From":"Sakari Ailus ","To":"Hans Verkuil ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","Subject":"Re: [PATCH v8 14/21] v4l: async: Allow binding notifiers to\n\tsub-devices","Message-ID":"<20170907083209.3xtaxwhrfmgrtpfz@valkosipuli.retiisi.org.uk>","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-15-sakari.ailus@linux.intel.com>\n\t<910b3d01-a8a4-2363-4b24-ed0edd1e1f4d@xs4all.nl>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<910b3d01-a8a4-2363-4b24-ed0edd1e1f4d@xs4all.nl>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764592,"web_url":"http://patchwork.ozlabs.org/comment/1764592/","msgid":"<85733523-7f6b-4aaf-55ca-e60e76719874@xs4all.nl>","list_archive_url":null,"date":"2017-09-07T08:51:21","subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/07/17 09:34, Sakari Ailus wrote:\n> Hi Hans,\n> \n> On Wed, Sep 06, 2017 at 09:41:40AM +0200, Hans Verkuil wrote:\n>> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n>>> The current practice is that drivers iterate over their endpoints and\n>>> parse each endpoint separately. This is very similar in a number of\n>>> drivers, implement a generic function for the job. Driver specific matters\n>>> can be taken into account in the driver specific callback.\n>>>\n>>> Signed-off-by: Sakari Ailus \n>>> ---\n>>> drivers/media/v4l2-core/v4l2-async.c | 19 +++++\n>>> drivers/media/v4l2-core/v4l2-fwnode.c | 140 ++++++++++++++++++++++++++++++++++\n>>> include/media/v4l2-async.h | 24 +++++-\n>>> include/media/v4l2-fwnode.h | 53 +++++++++++++\n>>> 4 files changed, 234 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n>>> index 3d81ff6a496f..7bd595c4094a 100644\n>>> --- a/drivers/media/v4l2-core/v4l2-async.c\n>>> +++ b/drivers/media/v4l2-core/v4l2-async.c\n>>> @@ -22,6 +22,7 @@\n>>> \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> \n>>> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n>>> @@ -224,6 +225,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n>>> }\n>>> EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n>>> \n>>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)\n>>> +{\n>>> +\tunsigned int i;\n>>> +\n>>> +\tif (!notifier->max_subdevs)\n>>> +\t\treturn;\n>>> +\n>>> +\tfor (i = 0; i < notifier->num_subdevs; i++)\n>>> +\t\tkfree(notifier->subdevs[i]);\n>>> +\n>>> +\tnotifier->max_subdevs = 0;\n>>> +\tnotifier->num_subdevs = 0;\n>>> +\n>>> +\tkvfree(notifier->subdevs);\n>>> +\tnotifier->subdevs = NULL;\n>>> +}\n>>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);\n>>> +\n>>> int v4l2_async_register_subdev(struct v4l2_subdev *sd)\n>>> {\n>>> \tstruct v4l2_async_notifier *notifier;\n>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c\n>>> index 706f9e7b90f1..e6932d7d47b6 100644\n>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c\n>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c\n>>> @@ -19,6 +19,7 @@\n>>> */\n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -26,6 +27,7 @@\n>>> #include \n>>> #include \n>>> \n>>> +#include \n>>> #include \n>>> \n>>> enum v4l2_fwnode_bus_type {\n>>> @@ -313,6 +315,144 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)\n>>> }\n>>> EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);\n>>> \n>>> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,\n>>> +\t\t\t\t unsigned int max_subdevs)\n>>> +{\n>>> +\tstruct v4l2_async_subdev **subdevs;\n>>> +\n>>> +\tif (max_subdevs <= notifier->max_subdevs)\n>>> +\t\treturn 0;\n>>> +\n>>> +\tsubdevs = kvmalloc_array(\n>>> +\t\tmax_subdevs, sizeof(*notifier->subdevs),\n>>> +\t\tGFP_KERNEL | __GFP_ZERO);\n>>> +\tif (!subdevs)\n>>> +\t\treturn -ENOMEM;\n>>> +\n>>> +\tif (notifier->subdevs) {\n>>> +\t\tmemcpy(subdevs, notifier->subdevs,\n>>> +\t\t sizeof(*subdevs) * notifier->num_subdevs);\n>>> +\n>>> +\t\tkvfree(notifier->subdevs);\n>>> +\t}\n>>> +\n>>> +\tnotifier->subdevs = subdevs;\n>>> +\tnotifier->max_subdevs = max_subdevs;\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +static int v4l2_async_notifier_fwnode_parse_endpoint(\n>>> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n>>> +\tstruct fwnode_handle *endpoint, unsigned int asd_struct_size,\n>>> +\tint (*parse_endpoint)(struct device *dev,\n>>> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n>>> +\t\t\t struct v4l2_async_subdev *asd))\n>>> +{\n>>> +\tstruct v4l2_async_subdev *asd;\n>>> +\tstruct v4l2_fwnode_endpoint *vep;\n>>> +\tstruct fwnode_endpoint ep;\n>>> +\tint ret = 0;\n>>> +\n>>> +\tasd = kzalloc(asd_struct_size, GFP_KERNEL);\n>>> +\tif (!asd)\n>>> +\t\treturn -ENOMEM;\n>>> +\n>>> +\tasd->match.fwnode.fwnode =\n>>> +\t\tfwnode_graph_get_remote_port_parent(endpoint);\n>>> +\tif (!asd->match.fwnode.fwnode) {\n>>> +\t\tdev_warn(dev, \"bad remote port parent\\n\");\n>>> +\t\tret = -EINVAL;\n>>> +\t\tgoto out_err;\n>>> +\t}\n>>> +\n>>> +\t/* Ignore endpoints the parsing of which failed. */\n>>> +\tvep = v4l2_fwnode_endpoint_alloc_parse(endpoint);\n>>> +\tif (IS_ERR(vep)) {\n>>> +\t\tret = PTR_ERR(vep);\n>>> +\t\tdev_warn(dev, \"unable to parse V4L2 fwnode endpoint (%d)\\n\",\n>>> +\t\t\t ret);\n>>> +\t\tgoto out_err;\n>>> +\t}\n>>> +\n>>> +\tep = vep->base;\n>>> +\n>>> +\tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n>>> +\tv4l2_fwnode_endpoint_free(vep);\n>>> +\tif (ret == -ENOTCONN) {\n>>> +\t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", ep.port, ep.id);\n>>> +\t\tkfree(asd);\n>>\n>> Shouldn't there be a call to fwnode_handle_put()?\n> \n> Actually no. But your hunch is right in the sense that I think there are\n> issues. The fwnode_endpoint (as of_endpoint) contains the fwnode which is\n> referenced, but no reference is taken here. One couldn't be released either\n> later on, as the fwnode field is const.\n> \n> I guess this is almost fine as long as the fwnode field is used for pointer\n> comparison and nothing else but you can never be sure what drivers actually\n> do.\n> \n> This actually should be addressed for both OF and fwnode but it's mostly\n> independent of this patchset. Luckily there are not *that* many users of\n> this. The V4L2 fwnode interface that allocates and releases endpoints makes\n> this quite a bit easier actually.\n\nI think a comment or two here would be helpful. If I tripped over this, so\nwill others.\n\n> \n>>\n>>> +\t\treturn 0;\n>>> +\t} else if (ret < 0) {\n>>> +\t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n>>> +\t\t\t ep.port, ep.id, ret);\n>>> +\t\tgoto out_err;\n>>> +\t}\n>>\n>> I think this would be better and it avoids the need for the ep local variable:\n>>\n>> \tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n>> \tif (ret == -ENOTCONN)\n>> \t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", vep->base.port, vep->base.id);\n>> \telse if (ret < 0)\n>> \t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n>> \t\t\t vep->base.port, vep->base.id, ret);\n>> \tv4l2_fwnode_endpoint_free(vep);\n>> \tif (ret < 0)\n>> \t\tgoto out_err;\n>>\n>> And the 'return ret;' below would become:\n>>\n>> \treturn ret == -ENOTCONN ? 0 : ret;\n> \n> Looks good to me, I'll use it.\n> \n>>\n>>> +\n>>> +\tasd->match_type = V4L2_ASYNC_MATCH_FWNODE;\n>>> +\tnotifier->subdevs[notifier->num_subdevs] = asd;\n>>> +\tnotifier->num_subdevs++;\n>>> +\n>>> +\treturn 0;\n>>> +\n>>> +out_err:\n>>> +\tfwnode_handle_put(asd->match.fwnode.fwnode);\n\nJust a reminder: this put() should now check if ret != -ENOTCONN.\n\n>>> +\tkfree(asd);\n>>> +\n>>> +\treturn ret;\n>>> +}\n>>> +\n>>> +int v4l2_async_notifier_parse_fwnode_endpoints(\n>>> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n>>> +\tsize_t asd_struct_size,\n>>> +\tint (*parse_endpoint)(struct device *dev,\n>>> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n>>> +\t\t\t struct v4l2_async_subdev *asd))\n>>> +{\n>>> +\tstruct fwnode_handle *fwnode = NULL;\n>>> +\tunsigned int max_subdevs = notifier->max_subdevs;\n>>> +\tint ret;\n>>> +\n>>> +\tif (asd_struct_size < sizeof(struct v4l2_async_subdev))\n>>\n>> I think this can be a WARN_ON or a WARN_ON_ONCE. Up to you, though.\n> \n> It'd be a driver bug, so either seems appropriate. I'll use WARN_ON().\n> \n>>\n>>> +\t\treturn -EINVAL;\n>>> +\n>>> +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n>>> +\t\t\t\t dev_fwnode(dev), fwnode)); )\n>>> +\t\tif (fwnode_device_is_available(\n>>> +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n>>> +\t\t\tmax_subdevs++;\n>>\n>> I think I would prefer this as a simple for (;;):\n>>\n>> \tfor (;;) {\n> \n> I think for (;;) is misuse of the for loop. :-D\n> \n>> \t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n>> \t\tif (fwnode == NULL)\n>> \t\t\tbreak;\n>> \t\t...\n>> \t}\n>>\n>> Or alternatively as a do...while:\n>>\n>> \tdo {\n>> \t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n>> \t\tif (fwnode && fwnode_device_is_available(\n>> \t\t\t fwnode_graph_get_port_parent(fwnode)))\n>> \t\t\tmax_subdevs++;\n>> \t} while (fwnode);\n>>\n>> Both are IMHO a bit more readable. I leave it up to you, though.\n>>\n>>> +\n>>> +\t/* No subdevs to add? Return here. */\n>>> +\tif (max_subdevs == notifier->max_subdevs)\n>>> +\t\treturn 0;\n>>> +\n>>> +\tret = v4l2_async_notifier_realloc(notifier, max_subdevs);\n>>> +\tif (ret)\n>>> +\t\treturn ret;\n>>> +\n>>> +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n>>> +\t\t\t\t dev_fwnode(dev), fwnode)); ) {\n>>\n>> Same comment as above.\n> \n> What I like in the original code is that managing looping over all\n> endpoints takes place directly in the for () statement and does not spill\n> over to where the individual endpoints are being handled.\n> \n> What would you think of adding a macro to do this? Say, we could write\n> this as:\n> \n> fwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) {\n> \t...\n> }\n> \n> That'd have to go through linux-pm tree though, and we could change the\n> code here later on.\n\nI think that's a good idea. Let's do that.\n\n> \n>>\n>>> +\t\tif (!fwnode_device_is_available(\n>>> +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n>>> +\t\t\tcontinue;\n>>> +\n>>> +\t\tif (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {\n>>> +\t\t\tret = -EINVAL;\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\n>>> +\t\tret = v4l2_async_notifier_fwnode_parse_endpoint(\n>>> +\t\t\tdev, notifier, fwnode, asd_struct_size, parse_endpoint);\n>>> +\t\tif (ret < 0)\n>>> +\t\t\tbreak;\n>>> +\t}\n>>> +\n>>> +\tfwnode_handle_put(fwnode);\n>>> +\n>>> +\treturn ret;\n>>> +}\n>>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);\n>>> +\n>>> MODULE_LICENSE(\"GPL\");\n>>> MODULE_AUTHOR(\"Sakari Ailus \");\n>>> MODULE_AUTHOR(\"Sylwester Nawrocki \");\n>>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n>>> index c69d8c8a66d0..96fa1afc00dd 100644\n>>> --- a/include/media/v4l2-async.h\n>>> +++ b/include/media/v4l2-async.h\n>>> @@ -18,7 +18,6 @@ struct device;\n>>> struct device_node;\n>>> struct v4l2_device;\n>>> struct v4l2_subdev;\n>>> -struct v4l2_async_notifier;\n>>> \n>>> /* A random max subdevice number, used to allocate an array on stack */\n>>> #define V4L2_MAX_SUBDEVS 128U\n>>> @@ -50,6 +49,10 @@ enum v4l2_async_match_type {\n>>> * @match:\tunion of per-bus type matching data sets\n>>> * @list:\tused to link struct v4l2_async_subdev objects, waiting to be\n>>> *\t\tprobed, to a notifier->waiting list\n>>> + *\n>>> + * When this struct is used as a member in a driver specific struct,\n>>> + * the driver specific struct shall contain the @struct\n>>> + * v4l2_async_subdev as its first member.\n>>> */\n>>> struct v4l2_async_subdev {\n>>> \tenum v4l2_async_match_type match_type;\n>>> @@ -78,7 +81,8 @@ struct v4l2_async_subdev {\n>>> /**\n>>> * struct v4l2_async_notifier - v4l2_device notifier data\n>>> *\n>>> - * @num_subdevs: number of subdevices\n>>> + * @num_subdevs: number of subdevices used in the subdevs array\n>>> + * @max_subdevs: number of subdevices allocated in the subdevs array\n>>> * @subdevs:\tarray of pointers to subdevice descriptors\n>>> * @v4l2_dev:\tpointer to struct v4l2_device\n>>> * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n>>> @@ -90,6 +94,7 @@ struct v4l2_async_subdev {\n>>> */\n>>> struct v4l2_async_notifier {\n>>> \tunsigned int num_subdevs;\n>>> +\tunsigned int max_subdevs;\n>>> \tstruct v4l2_async_subdev **subdevs;\n>>> \tstruct v4l2_device *v4l2_dev;\n>>> \tstruct list_head waiting;\n>>> @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n>>> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);\n>>> \n>>> /**\n>>> + * v4l2_async_notifier_release - release notifier resources\n>>> + * @notifier: the notifier the resources of which are to be released\n>>> + *\n>>> + * Release memory resources related to a notifier, including the async\n>>> + * sub-devices allocated for the purposes of the notifier. The user is\n>>> + * responsible for releasing the notifier's resources after calling\n>>> + * @v4l2_async_notifier_parse_fwnode_endpoints.\n>>> + *\n>>> + * There is no harm from calling v4l2_async_notifier_release in other\n>>> + * cases as long as its memory has been zeroed after it has been\n>>> + * allocated.\n>>> + */\n>>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);\n>>> +\n>>> +/**\n>>> * v4l2_async_register_subdev - registers a sub-device to the asynchronous\n>>> * \tsubdevice framework\n>>> *\n>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h\n>>> index 68eb22ba571b..6d125f26ec84 100644\n>>> --- a/include/media/v4l2-fwnode.h\n>>> +++ b/include/media/v4l2-fwnode.h\n>>> @@ -25,6 +25,8 @@\n>>> #include \n>>> \n>>> struct fwnode_handle;\n>>> +struct v4l2_async_notifier;\n>>> +struct v4l2_async_subdev;\n>>> \n>>> #define V4L2_FWNODE_CSI2_MAX_DATA_LANES\t4\n>>> \n>>> @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,\n>>> */\n>>> void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);\n>>> \n>>> +/**\n>>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a\n>>> + *\t\t\t\t\t\tdevice node\n>>> + * @dev: the device the endpoints of which are to be parsed\n>>> + * @notifier: notifier for @dev\n>>> + * @asd_struct_size: size of the driver's async sub-device struct, including\n>>> + *\t\t sizeof(struct v4l2_async_subdev). The &struct\n>>> + *\t\t v4l2_async_subdev shall be the first member of\n>>> + *\t\t the driver's async sub-device struct, i.e. both\n>>> + *\t\t begin at the same memory address.\n>>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode\n>>> + *\t\t endpoint. Optional.\n>>> + *\t\t Return: %0 on success\n>>> + *\t\t\t %-ENOTCONN if the endpoint is to be skipped but this\n>>> + *\t\t\t\t should not be considered as an error\n>>> + *\t\t\t %-EINVAL if the endpoint configuration is invalid\n>>> + *\n>>> + * Parse the fwnode endpoints of the @dev device and populate the async sub-\n>>> + * devices array of the notifier. The @parse_endpoint callback function is\n>>> + * called for each endpoint with the corresponding async sub-device pointer to\n>>> + * let the caller initialize the driver-specific part of the async sub-device\n>>> + * structure.\n>>> + *\n>>> + * The notifier memory shall be zeroed before this function is called on the\n>>> + * notifier.\n>>> + *\n>>> + * This function may not be called on a registered notifier and may be called on\n>>> + * a notifier only once. When using this function, the user may not access the\n>>> + * notifier's subdevs array nor change notifier's num_subdevs field, these are\n>>> + * reserved for the framework's internal use only.\n>>\n>> The rvin_digital_graph_init() function accesses the subdevs array.\n>>\n>> I still don't like this sentence, and I still think it should be dropped. Either that\n>> or completely rewritten.\n> \n> How about:\n> \n> When using this function on a notifier, the user is not allowed to change\n> the notifier's subdevs array, take references to the subdevs array itself\n> nor change the notifier's num_subdevs field. This is because the function\n> allocates and reallocates the subdevs array based on parsing endpoints.\n> \n\nHow about this:\n\n\"Do not change the notifier's subdevs array, take references to the subdevs\narray itself or change the notifier's num_subdevs field. This is because\nthis function allocates and reallocates the subdevs array based on parsing\nendpoints.\"\n\nIt's the \"When using this function\" part that really trips me up. It's\nodd phrasing.\n\nRegards,\n\n\tHans\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnvM91ld0z9s8J\n\tfor ;\n\tThu, 7 Sep 2017 18:51:33 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754068AbdIGIvb (ORCPT\n\t);\n\tThu, 7 Sep 2017 04:51:31 -0400","from lb1-smtp-cloud9.xs4all.net ([194.109.24.22]:58629 \"EHLO\n\tlb1-smtp-cloud9.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1754073AbdIGIva (ORCPT\n\t); Thu, 7 Sep 2017 04:51:30 -0400","from [IPv6:2001:420:44c1:2579:b476:a112:f655:173c]\n\t([IPv6:2001:420:44c1:2579:b476:a112:f655:173c])\n\tby smtp-cloud9.xs4all.net with ESMTPA\n\tid psWwdQkbfdRLjpsWzdhycS; Thu, 07 Sep 2017 10:51:28 +0200"],"Subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","To":"Sakari Ailus ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-7-sakari.ailus@linux.intel.com>\n\t\n\t<20170907073446.ajxsscekwrbfbtgk@valkosipuli.retiisi.org.uk>","From":"Hans Verkuil ","Message-ID":"<85733523-7f6b-4aaf-55ca-e60e76719874@xs4all.nl>","Date":"Thu, 7 Sep 2017 10:51:21 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170907073446.ajxsscekwrbfbtgk@valkosipuli.retiisi.org.uk>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfOYMLkDPMAW9GRB+lxcJVI3Xdy33zERq7w1iwCplmWY2Sc2wO9pcHLWl1xJtCWjyk7pZVC060MUlu9TA1o4O0+s0har49Psy0q9/gCqdAXRxPsGqkKmC\n\tIpc8Njz4r5ORpKu/o5LLkkAGYKxctJ6NO6sL3yNPoNFQEwa1bOalv9bNGA9X7v+3XMj/aSERg1PIn8MUtb8GmVGAQ8mpU+Jzr+wJ30twYhmshiCCCz+DR9f1\n\t101PXRVniA/CoPw/9F30g1kcmq6Kqhil7HVHw/gN2/44kdHlM3CCRqByEKDMVZp1cvI002jt/K0WH/HFVV0fhBCRxBZoCBwvkqRAOYehd0t/a2oO8oeA4Th2\n\tVeDAezS8TCNGfNhOWjcTbm4otm/fLSKER5vpTqO24bmNhHG/RN/IxzVKJYvuOHvJ1tqiagrG9WJiPq1PoU+3slFALdXEYQ9o8AUFvQaxHn4Syft6okpmD8+6\n\ta0dAI01RvjPNuJhqBZzAFJ4w+I5y02INSpFtsuZ3gmF8mHXKxr30GNScVWQ=","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764625,"web_url":"http://patchwork.ozlabs.org/comment/1764625/","msgid":"<20170907095850.m7mlag3tofwbj2jc@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-09-07T09:58:50","subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Hans,\n\nOn Thu, Sep 07, 2017 at 10:51:21AM +0200, Hans Verkuil wrote:\n> On 09/07/17 09:34, Sakari Ailus wrote:\n> > Hi Hans,\n> > \n> > On Wed, Sep 06, 2017 at 09:41:40AM +0200, Hans Verkuil wrote:\n> >> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n> >>> The current practice is that drivers iterate over their endpoints and\n> >>> parse each endpoint separately. This is very similar in a number of\n> >>> drivers, implement a generic function for the job. Driver specific matters\n> >>> can be taken into account in the driver specific callback.\n> >>>\n> >>> Signed-off-by: Sakari Ailus \n> >>> ---\n> >>> drivers/media/v4l2-core/v4l2-async.c | 19 +++++\n> >>> drivers/media/v4l2-core/v4l2-fwnode.c | 140 ++++++++++++++++++++++++++++++++++\n> >>> include/media/v4l2-async.h | 24 +++++-\n> >>> include/media/v4l2-fwnode.h | 53 +++++++++++++\n> >>> 4 files changed, 234 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n> >>> index 3d81ff6a496f..7bd595c4094a 100644\n> >>> --- a/drivers/media/v4l2-core/v4l2-async.c\n> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c\n> >>> @@ -22,6 +22,7 @@\n> >>> \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> \n> >>> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n> >>> @@ -224,6 +225,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n> >>> }\n> >>> EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n> >>> \n> >>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)\n> >>> +{\n> >>> +\tunsigned int i;\n> >>> +\n> >>> +\tif (!notifier->max_subdevs)\n> >>> +\t\treturn;\n> >>> +\n> >>> +\tfor (i = 0; i < notifier->num_subdevs; i++)\n> >>> +\t\tkfree(notifier->subdevs[i]);\n> >>> +\n> >>> +\tnotifier->max_subdevs = 0;\n> >>> +\tnotifier->num_subdevs = 0;\n> >>> +\n> >>> +\tkvfree(notifier->subdevs);\n> >>> +\tnotifier->subdevs = NULL;\n> >>> +}\n> >>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);\n> >>> +\n> >>> int v4l2_async_register_subdev(struct v4l2_subdev *sd)\n> >>> {\n> >>> \tstruct v4l2_async_notifier *notifier;\n> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c\n> >>> index 706f9e7b90f1..e6932d7d47b6 100644\n> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c\n> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c\n> >>> @@ -19,6 +19,7 @@\n> >>> */\n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -26,6 +27,7 @@\n> >>> #include \n> >>> #include \n> >>> \n> >>> +#include \n> >>> #include \n> >>> \n> >>> enum v4l2_fwnode_bus_type {\n> >>> @@ -313,6 +315,144 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)\n> >>> }\n> >>> EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);\n> >>> \n> >>> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,\n> >>> +\t\t\t\t unsigned int max_subdevs)\n> >>> +{\n> >>> +\tstruct v4l2_async_subdev **subdevs;\n> >>> +\n> >>> +\tif (max_subdevs <= notifier->max_subdevs)\n> >>> +\t\treturn 0;\n> >>> +\n> >>> +\tsubdevs = kvmalloc_array(\n> >>> +\t\tmax_subdevs, sizeof(*notifier->subdevs),\n> >>> +\t\tGFP_KERNEL | __GFP_ZERO);\n> >>> +\tif (!subdevs)\n> >>> +\t\treturn -ENOMEM;\n> >>> +\n> >>> +\tif (notifier->subdevs) {\n> >>> +\t\tmemcpy(subdevs, notifier->subdevs,\n> >>> +\t\t sizeof(*subdevs) * notifier->num_subdevs);\n> >>> +\n> >>> +\t\tkvfree(notifier->subdevs);\n> >>> +\t}\n> >>> +\n> >>> +\tnotifier->subdevs = subdevs;\n> >>> +\tnotifier->max_subdevs = max_subdevs;\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +static int v4l2_async_notifier_fwnode_parse_endpoint(\n> >>> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> >>> +\tstruct fwnode_handle *endpoint, unsigned int asd_struct_size,\n> >>> +\tint (*parse_endpoint)(struct device *dev,\n> >>> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> >>> +\t\t\t struct v4l2_async_subdev *asd))\n> >>> +{\n> >>> +\tstruct v4l2_async_subdev *asd;\n> >>> +\tstruct v4l2_fwnode_endpoint *vep;\n> >>> +\tstruct fwnode_endpoint ep;\n> >>> +\tint ret = 0;\n> >>> +\n> >>> +\tasd = kzalloc(asd_struct_size, GFP_KERNEL);\n> >>> +\tif (!asd)\n> >>> +\t\treturn -ENOMEM;\n> >>> +\n> >>> +\tasd->match.fwnode.fwnode =\n> >>> +\t\tfwnode_graph_get_remote_port_parent(endpoint);\n> >>> +\tif (!asd->match.fwnode.fwnode) {\n> >>> +\t\tdev_warn(dev, \"bad remote port parent\\n\");\n> >>> +\t\tret = -EINVAL;\n> >>> +\t\tgoto out_err;\n> >>> +\t}\n> >>> +\n> >>> +\t/* Ignore endpoints the parsing of which failed. */\n> >>> +\tvep = v4l2_fwnode_endpoint_alloc_parse(endpoint);\n> >>> +\tif (IS_ERR(vep)) {\n> >>> +\t\tret = PTR_ERR(vep);\n> >>> +\t\tdev_warn(dev, \"unable to parse V4L2 fwnode endpoint (%d)\\n\",\n> >>> +\t\t\t ret);\n> >>> +\t\tgoto out_err;\n> >>> +\t}\n> >>> +\n> >>> +\tep = vep->base;\n> >>> +\n> >>> +\tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n> >>> +\tv4l2_fwnode_endpoint_free(vep);\n> >>> +\tif (ret == -ENOTCONN) {\n> >>> +\t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", ep.port, ep.id);\n> >>> +\t\tkfree(asd);\n> >>\n> >> Shouldn't there be a call to fwnode_handle_put()?\n> > \n> > Actually no. But your hunch is right in the sense that I think there are\n> > issues. The fwnode_endpoint (as of_endpoint) contains the fwnode which is\n> > referenced, but no reference is taken here. One couldn't be released either\n> > later on, as the fwnode field is const.\n> > \n> > I guess this is almost fine as long as the fwnode field is used for pointer\n> > comparison and nothing else but you can never be sure what drivers actually\n> > do.\n> > \n> > This actually should be addressed for both OF and fwnode but it's mostly\n> > independent of this patchset. Luckily there are not *that* many users of\n> > this. The V4L2 fwnode interface that allocates and releases endpoints makes\n> > this quite a bit easier actually.\n> \n> I think a comment or two here would be helpful. If I tripped over this, so\n> will others.\n\nReading the code again, I noticed I didn't read far enough the last time.\nWhile v4l2_fwnode_endpoint_alloc_parse() won't take a reference,\nfwnode_graph_get_remote_port_parent() does. That reference indeed isn't\nreleased right now.\n\nIt shouldn't be too hard to do that properly in\nv4l2_async_notifier_release(). I guess I'll do just that actually. By that\ntime, there are no users left anymore.\n\nGood catch.\n\n> \n> > \n> >>\n> >>> +\t\treturn 0;\n> >>> +\t} else if (ret < 0) {\n> >>> +\t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n> >>> +\t\t\t ep.port, ep.id, ret);\n> >>> +\t\tgoto out_err;\n> >>> +\t}\n> >>\n> >> I think this would be better and it avoids the need for the ep local variable:\n> >>\n> >> \tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n> >> \tif (ret == -ENOTCONN)\n> >> \t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", vep->base.port, vep->base.id);\n> >> \telse if (ret < 0)\n> >> \t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n> >> \t\t\t vep->base.port, vep->base.id, ret);\n> >> \tv4l2_fwnode_endpoint_free(vep);\n> >> \tif (ret < 0)\n> >> \t\tgoto out_err;\n> >>\n> >> And the 'return ret;' below would become:\n> >>\n> >> \treturn ret == -ENOTCONN ? 0 : ret;\n> > \n> > Looks good to me, I'll use it.\n> > \n> >>\n> >>> +\n> >>> +\tasd->match_type = V4L2_ASYNC_MATCH_FWNODE;\n> >>> +\tnotifier->subdevs[notifier->num_subdevs] = asd;\n> >>> +\tnotifier->num_subdevs++;\n> >>> +\n> >>> +\treturn 0;\n> >>> +\n> >>> +out_err:\n> >>> +\tfwnode_handle_put(asd->match.fwnode.fwnode);\n> \n> Just a reminder: this put() should now check if ret != -ENOTCONN.\n\nShould it? Even if ret == -ENOTCONN, the reference is there and needs to be\nreleased as the endpoint is not used (just as in the case of an error).\n\n> \n> >>> +\tkfree(asd);\n> >>> +\n> >>> +\treturn ret;\n> >>> +}\n> >>> +\n> >>> +int v4l2_async_notifier_parse_fwnode_endpoints(\n> >>> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n> >>> +\tsize_t asd_struct_size,\n> >>> +\tint (*parse_endpoint)(struct device *dev,\n> >>> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n> >>> +\t\t\t struct v4l2_async_subdev *asd))\n> >>> +{\n> >>> +\tstruct fwnode_handle *fwnode = NULL;\n> >>> +\tunsigned int max_subdevs = notifier->max_subdevs;\n> >>> +\tint ret;\n> >>> +\n> >>> +\tif (asd_struct_size < sizeof(struct v4l2_async_subdev))\n> >>\n> >> I think this can be a WARN_ON or a WARN_ON_ONCE. Up to you, though.\n> > \n> > It'd be a driver bug, so either seems appropriate. I'll use WARN_ON().\n> > \n> >>\n> >>> +\t\treturn -EINVAL;\n> >>> +\n> >>> +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n> >>> +\t\t\t\t dev_fwnode(dev), fwnode)); )\n> >>> +\t\tif (fwnode_device_is_available(\n> >>> +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n> >>> +\t\t\tmax_subdevs++;\n> >>\n> >> I think I would prefer this as a simple for (;;):\n> >>\n> >> \tfor (;;) {\n> > \n> > I think for (;;) is misuse of the for loop. :-D\n> > \n> >> \t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n> >> \t\tif (fwnode == NULL)\n> >> \t\t\tbreak;\n> >> \t\t...\n> >> \t}\n> >>\n> >> Or alternatively as a do...while:\n> >>\n> >> \tdo {\n> >> \t\tfwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));\n> >> \t\tif (fwnode && fwnode_device_is_available(\n> >> \t\t\t fwnode_graph_get_port_parent(fwnode)))\n> >> \t\t\tmax_subdevs++;\n> >> \t} while (fwnode);\n> >>\n> >> Both are IMHO a bit more readable. I leave it up to you, though.\n> >>\n> >>> +\n> >>> +\t/* No subdevs to add? Return here. */\n> >>> +\tif (max_subdevs == notifier->max_subdevs)\n> >>> +\t\treturn 0;\n> >>> +\n> >>> +\tret = v4l2_async_notifier_realloc(notifier, max_subdevs);\n> >>> +\tif (ret)\n> >>> +\t\treturn ret;\n> >>> +\n> >>> +\tfor (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(\n> >>> +\t\t\t\t dev_fwnode(dev), fwnode)); ) {\n> >>\n> >> Same comment as above.\n> > \n> > What I like in the original code is that managing looping over all\n> > endpoints takes place directly in the for () statement and does not spill\n> > over to where the individual endpoints are being handled.\n> > \n> > What would you think of adding a macro to do this? Say, we could write\n> > this as:\n> > \n> > fwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) {\n> > \t...\n> > }\n> > \n> > That'd have to go through linux-pm tree though, and we could change the\n> > code here later on.\n> \n> I think that's a good idea. Let's do that.\n\nAck. I'll submit a patch on that in the near future.\n\n> \n> > \n> >>\n> >>> +\t\tif (!fwnode_device_is_available(\n> >>> +\t\t\t fwnode_graph_get_port_parent(fwnode)))\n> >>> +\t\t\tcontinue;\n> >>> +\n> >>> +\t\tif (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {\n> >>> +\t\t\tret = -EINVAL;\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tret = v4l2_async_notifier_fwnode_parse_endpoint(\n> >>> +\t\t\tdev, notifier, fwnode, asd_struct_size, parse_endpoint);\n> >>> +\t\tif (ret < 0)\n> >>> +\t\t\tbreak;\n> >>> +\t}\n> >>> +\n> >>> +\tfwnode_handle_put(fwnode);\n> >>> +\n> >>> +\treturn ret;\n> >>> +}\n> >>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);\n> >>> +\n> >>> MODULE_LICENSE(\"GPL\");\n> >>> MODULE_AUTHOR(\"Sakari Ailus \");\n> >>> MODULE_AUTHOR(\"Sylwester Nawrocki \");\n> >>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n> >>> index c69d8c8a66d0..96fa1afc00dd 100644\n> >>> --- a/include/media/v4l2-async.h\n> >>> +++ b/include/media/v4l2-async.h\n> >>> @@ -18,7 +18,6 @@ struct device;\n> >>> struct device_node;\n> >>> struct v4l2_device;\n> >>> struct v4l2_subdev;\n> >>> -struct v4l2_async_notifier;\n> >>> \n> >>> /* A random max subdevice number, used to allocate an array on stack */\n> >>> #define V4L2_MAX_SUBDEVS 128U\n> >>> @@ -50,6 +49,10 @@ enum v4l2_async_match_type {\n> >>> * @match:\tunion of per-bus type matching data sets\n> >>> * @list:\tused to link struct v4l2_async_subdev objects, waiting to be\n> >>> *\t\tprobed, to a notifier->waiting list\n> >>> + *\n> >>> + * When this struct is used as a member in a driver specific struct,\n> >>> + * the driver specific struct shall contain the @struct\n> >>> + * v4l2_async_subdev as its first member.\n> >>> */\n> >>> struct v4l2_async_subdev {\n> >>> \tenum v4l2_async_match_type match_type;\n> >>> @@ -78,7 +81,8 @@ struct v4l2_async_subdev {\n> >>> /**\n> >>> * struct v4l2_async_notifier - v4l2_device notifier data\n> >>> *\n> >>> - * @num_subdevs: number of subdevices\n> >>> + * @num_subdevs: number of subdevices used in the subdevs array\n> >>> + * @max_subdevs: number of subdevices allocated in the subdevs array\n> >>> * @subdevs:\tarray of pointers to subdevice descriptors\n> >>> * @v4l2_dev:\tpointer to struct v4l2_device\n> >>> * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n> >>> @@ -90,6 +94,7 @@ struct v4l2_async_subdev {\n> >>> */\n> >>> struct v4l2_async_notifier {\n> >>> \tunsigned int num_subdevs;\n> >>> +\tunsigned int max_subdevs;\n> >>> \tstruct v4l2_async_subdev **subdevs;\n> >>> \tstruct v4l2_device *v4l2_dev;\n> >>> \tstruct list_head waiting;\n> >>> @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n> >>> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);\n> >>> \n> >>> /**\n> >>> + * v4l2_async_notifier_release - release notifier resources\n> >>> + * @notifier: the notifier the resources of which are to be released\n> >>> + *\n> >>> + * Release memory resources related to a notifier, including the async\n> >>> + * sub-devices allocated for the purposes of the notifier. The user is\n> >>> + * responsible for releasing the notifier's resources after calling\n> >>> + * @v4l2_async_notifier_parse_fwnode_endpoints.\n> >>> + *\n> >>> + * There is no harm from calling v4l2_async_notifier_release in other\n> >>> + * cases as long as its memory has been zeroed after it has been\n> >>> + * allocated.\n> >>> + */\n> >>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);\n> >>> +\n> >>> +/**\n> >>> * v4l2_async_register_subdev - registers a sub-device to the asynchronous\n> >>> * \tsubdevice framework\n> >>> *\n> >>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h\n> >>> index 68eb22ba571b..6d125f26ec84 100644\n> >>> --- a/include/media/v4l2-fwnode.h\n> >>> +++ b/include/media/v4l2-fwnode.h\n> >>> @@ -25,6 +25,8 @@\n> >>> #include \n> >>> \n> >>> struct fwnode_handle;\n> >>> +struct v4l2_async_notifier;\n> >>> +struct v4l2_async_subdev;\n> >>> \n> >>> #define V4L2_FWNODE_CSI2_MAX_DATA_LANES\t4\n> >>> \n> >>> @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,\n> >>> */\n> >>> void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);\n> >>> \n> >>> +/**\n> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a\n> >>> + *\t\t\t\t\t\tdevice node\n> >>> + * @dev: the device the endpoints of which are to be parsed\n> >>> + * @notifier: notifier for @dev\n> >>> + * @asd_struct_size: size of the driver's async sub-device struct, including\n> >>> + *\t\t sizeof(struct v4l2_async_subdev). The &struct\n> >>> + *\t\t v4l2_async_subdev shall be the first member of\n> >>> + *\t\t the driver's async sub-device struct, i.e. both\n> >>> + *\t\t begin at the same memory address.\n> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode\n> >>> + *\t\t endpoint. Optional.\n> >>> + *\t\t Return: %0 on success\n> >>> + *\t\t\t %-ENOTCONN if the endpoint is to be skipped but this\n> >>> + *\t\t\t\t should not be considered as an error\n> >>> + *\t\t\t %-EINVAL if the endpoint configuration is invalid\n> >>> + *\n> >>> + * Parse the fwnode endpoints of the @dev device and populate the async sub-\n> >>> + * devices array of the notifier. The @parse_endpoint callback function is\n> >>> + * called for each endpoint with the corresponding async sub-device pointer to\n> >>> + * let the caller initialize the driver-specific part of the async sub-device\n> >>> + * structure.\n> >>> + *\n> >>> + * The notifier memory shall be zeroed before this function is called on the\n> >>> + * notifier.\n> >>> + *\n> >>> + * This function may not be called on a registered notifier and may be called on\n> >>> + * a notifier only once. When using this function, the user may not access the\n> >>> + * notifier's subdevs array nor change notifier's num_subdevs field, these are\n> >>> + * reserved for the framework's internal use only.\n> >>\n> >> The rvin_digital_graph_init() function accesses the subdevs array.\n> >>\n> >> I still don't like this sentence, and I still think it should be dropped. Either that\n> >> or completely rewritten.\n> > \n> > How about:\n> > \n> > When using this function on a notifier, the user is not allowed to change\n> > the notifier's subdevs array, take references to the subdevs array itself\n> > nor change the notifier's num_subdevs field. This is because the function\n> > allocates and reallocates the subdevs array based on parsing endpoints.\n> > \n> \n> How about this:\n> \n> \"Do not change the notifier's subdevs array, take references to the subdevs\n> array itself or change the notifier's num_subdevs field. This is because\n> this function allocates and reallocates the subdevs array based on parsing\n> endpoints.\"\n\nI'll use this in the next version.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnwrv5JCkz9t2R\n\tfor ;\n\tThu, 7 Sep 2017 19:58:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753996AbdIGJ6y (ORCPT\n\t);\n\tThu, 7 Sep 2017 05:58:54 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:34396 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1753905AbdIGJ6x (ORCPT\n\t); Thu, 7 Sep 2017 05:58:53 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id 38020600E0;\n\tThu, 7 Sep 2017 12:58:51 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dptaE-0002si-OJ; Thu, 07 Sep 2017 12:58:50 +0300"],"Date":"Thu, 7 Sep 2017 12:58:50 +0300","From":"Sakari Ailus ","To":"Hans Verkuil ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","Subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","Message-ID":"<20170907095850.m7mlag3tofwbj2jc@valkosipuli.retiisi.org.uk>","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-7-sakari.ailus@linux.intel.com>\n\t\n\t<20170907073446.ajxsscekwrbfbtgk@valkosipuli.retiisi.org.uk>\n\t<85733523-7f6b-4aaf-55ca-e60e76719874@xs4all.nl>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<85733523-7f6b-4aaf-55ca-e60e76719874@xs4all.nl>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764680,"web_url":"http://patchwork.ozlabs.org/comment/1764680/","msgid":"<62199083-9d61-daa5-f16a-c3d29ed94407@xs4all.nl>","list_archive_url":null,"date":"2017-09-07T12:02:14","subject":"Re: [PATCH v8 14/21] v4l: async: Allow binding notifiers to\n\tsub-devices","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/07/17 10:32, Sakari Ailus wrote:\n> Hi Hans,\n> \n> On Wed, Sep 06, 2017 at 10:46:31AM +0200, Hans Verkuil wrote:\n>> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n\n>>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h\n>>> index 3bc8a7c0d83f..12739be44bd1 100644\n>>> --- a/include/media/v4l2-async.h\n>>> +++ b/include/media/v4l2-async.h\n>>> @@ -102,7 +102,9 @@ struct v4l2_async_notifier_operations {\n>>> * @num_subdevs: number of subdevices used in the subdevs array\n>>> * @max_subdevs: number of subdevices allocated in the subdevs array\n>>> * @subdevs:\tarray of pointers to subdevice descriptors\n>>> - * @v4l2_dev:\tpointer to struct v4l2_device\n>>> + * @v4l2_dev:\tv4l2_device of the master, for subdev notifiers NULL\n>>> + * @sd:\t\tsub-device that registered the notifier, NULL otherwise\n>>> + * @master:\tmaster notifier carrying @v4l2_dev\n>>\n>> I think this description is out of date. It is really the parent notifier,\n>> right? Should 'master' be renamed to 'parent'?\n> \n> You could view it as one, yes. What is known is that the notifier is\n> related, and through which the v4l2_dev can be found. I'll rename it as\n> \"parent\".\n> \n> I'll use root in the commit message as well.\n> \n>>\n>> Same problem with the description of @v4l2_dev: it's the v4l2_device of the\n>> root/top-level notifier.\n> \n> \"v4l2_device of the root notifier, NULL otherwise\"?\n\nAck.\n\n\tHans\n\n> \n>>\n>>> * @waiting:\tlist of struct v4l2_async_subdev, waiting for their drivers\n>>> * @done:\tlist of struct v4l2_subdev, already probed\n>>> * @list:\tmember in a global list of notifiers\n>>> @@ -113,6 +115,8 @@ struct v4l2_async_notifier {\n>>> \tunsigned int max_subdevs;\n>>> \tstruct v4l2_async_subdev **subdevs;\n>>> \tstruct v4l2_device *v4l2_dev;\n>>> +\tstruct v4l2_subdev *sd;\n>>> +\tstruct v4l2_async_notifier *master;\n>>> \tstruct list_head waiting;\n>>> \tstruct list_head done;\n>>> \tstruct list_head list;\n>>> @@ -128,6 +132,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,\n>>> \t\t\t\t struct v4l2_async_notifier *notifier);\n>>> \n>>> /**\n>>> + * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous\n>>> + *\t\t\t\t\t notifier for a sub-device\n>>> + *\n>>> + * @sd: pointer to &struct v4l2_subdev\n>>> + * @notifier: pointer to &struct v4l2_async_notifier\n>>> + */\n>>> +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,\n>>> +\t\t\t\t\tstruct v4l2_async_notifier *notifier);\n>>> +\n>>> +/**\n>>> * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier\n>>> *\n>>> * @notifier: pointer to &struct v4l2_async_notifier\n>>>\n>>\n>> This v8 is much better and is getting close.\n> \n> Thanks!\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnzbQ11NJz9sRV\n\tfor ;\n\tThu, 7 Sep 2017 22:02:26 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755114AbdIGMCY (ORCPT\n\t);\n\tThu, 7 Sep 2017 08:02:24 -0400","from lb2-smtp-cloud7.xs4all.net ([194.109.24.28]:42039 \"EHLO\n\tlb2-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1754796AbdIGMCY (ORCPT\n\t); Thu, 7 Sep 2017 08:02:24 -0400","from [IPv6:2001:420:44c1:2579:9dea:9e9e:3419:5bfe]\n\t([IPv6:2001:420:44c1:2579:9dea:9e9e:3419:5bfe])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pvVfds7aLb2snpvVidvZoW; Thu, 07 Sep 2017 14:02:22 +0200"],"Subject":"Re: [PATCH v8 14/21] v4l: async: Allow binding notifiers to\n\tsub-devices","To":"Sakari Ailus ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-15-sakari.ailus@linux.intel.com>\n\t<910b3d01-a8a4-2363-4b24-ed0edd1e1f4d@xs4all.nl>\n\t<20170907083209.3xtaxwhrfmgrtpfz@valkosipuli.retiisi.org.uk>","From":"Hans Verkuil ","Message-ID":"<62199083-9d61-daa5-f16a-c3d29ed94407@xs4all.nl>","Date":"Thu, 7 Sep 2017 14:02:14 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170907083209.3xtaxwhrfmgrtpfz@valkosipuli.retiisi.org.uk>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfAICxVSD9nnSdjyrEK4RpwDz2m/UDwRrGl6CqvJKKq0SNHLuyprnphrWNYnFuTUtY62pq/X5AaMd+KC4RaAM4yhWbELje+UoRGfup5LG554p5Ce5g6DE\n\tI1Yp361nu3//ypMwZ2+AZWhwBgdAa5Pb0GN6nJQ4ZNWtJ3XFZMkTkNyzz5zI16TeRKJU+AebW8NBvwCHeGues6qVcQHrh5HUE3MHqH3MsWkaKBJJXwOGFLl+\n\tC2jLajy7ud1OK+DbxNbJEfVvtdotJHQELY3nRfyJK9HURv7XA+oAEzpGxdZeItyWZ5A+f2sdnBC8nJGNqw3mQ3vJvFjkUxKMGhr0JmlsCiHrPs9SZlvfEiZo\n\tfxLtH+BDZPUjasEociNssIauN1gfRTbn/K/HwrY7UgYtaIz9EbJFOBDbtoDzx59lvX5qHDLOtZ01pk7rCfuLX98rRaboJuPb8D0KX84DPqKFPqzKqpqdz7ka\n\tnknyH389N2JMTpS8v04Pd9sTiyzW0CwbZBzO5DeeTdqVcTKIcU2dHOtIdc4=","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764681,"web_url":"http://patchwork.ozlabs.org/comment/1764681/","msgid":"<8123f8c5-9045-2a22-c97f-09d36e504e2e@xs4all.nl>","list_archive_url":null,"date":"2017-09-07T12:03:57","subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/07/17 11:58, Sakari Ailus wrote:\n> Hi Hans,\n> \n> On Thu, Sep 07, 2017 at 10:51:21AM +0200, Hans Verkuil wrote:\n>> On 09/07/17 09:34, Sakari Ailus wrote:\n>>> Hi Hans,\n>>>\n>>> On Wed, Sep 06, 2017 at 09:41:40AM +0200, Hans Verkuil wrote:\n>>>> On 09/05/2017 03:05 PM, Sakari Ailus wrote:\n>>>>> The current practice is that drivers iterate over their endpoints and\n>>>>> parse each endpoint separately. This is very similar in a number of\n>>>>> drivers, implement a generic function for the job. Driver specific matters\n>>>>> can be taken into account in the driver specific callback.\n>>>>>\n>>>>> Signed-off-by: Sakari Ailus \n>>>>> ---\n>>>>> drivers/media/v4l2-core/v4l2-async.c | 19 +++++\n>>>>> drivers/media/v4l2-core/v4l2-fwnode.c | 140 ++++++++++++++++++++++++++++++++++\n>>>>> include/media/v4l2-async.h | 24 +++++-\n>>>>> include/media/v4l2-fwnode.h | 53 +++++++++++++\n>>>>> 4 files changed, 234 insertions(+), 2 deletions(-)\n>>>>>\n>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c\n>>>>> index 3d81ff6a496f..7bd595c4094a 100644\n>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c\n>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c\n>>>>> @@ -22,6 +22,7 @@\n>>>>> \n>>>>> #include \n>>>>> #include \n>>>>> +#include \n>>>>> #include \n>>>>> \n>>>>> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)\n>>>>> @@ -224,6 +225,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)\n>>>>> }\n>>>>> EXPORT_SYMBOL(v4l2_async_notifier_unregister);\n>>>>> \n>>>>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)\n>>>>> +{\n>>>>> +\tunsigned int i;\n>>>>> +\n>>>>> +\tif (!notifier->max_subdevs)\n>>>>> +\t\treturn;\n>>>>> +\n>>>>> +\tfor (i = 0; i < notifier->num_subdevs; i++)\n>>>>> +\t\tkfree(notifier->subdevs[i]);\n>>>>> +\n>>>>> +\tnotifier->max_subdevs = 0;\n>>>>> +\tnotifier->num_subdevs = 0;\n>>>>> +\n>>>>> +\tkvfree(notifier->subdevs);\n>>>>> +\tnotifier->subdevs = NULL;\n>>>>> +}\n>>>>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);\n>>>>> +\n>>>>> int v4l2_async_register_subdev(struct v4l2_subdev *sd)\n>>>>> {\n>>>>> \tstruct v4l2_async_notifier *notifier;\n>>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c\n>>>>> index 706f9e7b90f1..e6932d7d47b6 100644\n>>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c\n>>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c\n>>>>> @@ -19,6 +19,7 @@\n>>>>> */\n>>>>> #include \n>>>>> #include \n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> @@ -26,6 +27,7 @@\n>>>>> #include \n>>>>> #include \n>>>>> \n>>>>> +#include \n>>>>> #include \n>>>>> \n>>>>> enum v4l2_fwnode_bus_type {\n>>>>> @@ -313,6 +315,144 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)\n>>>>> }\n>>>>> EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);\n>>>>> \n>>>>> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,\n>>>>> +\t\t\t\t unsigned int max_subdevs)\n>>>>> +{\n>>>>> +\tstruct v4l2_async_subdev **subdevs;\n>>>>> +\n>>>>> +\tif (max_subdevs <= notifier->max_subdevs)\n>>>>> +\t\treturn 0;\n>>>>> +\n>>>>> +\tsubdevs = kvmalloc_array(\n>>>>> +\t\tmax_subdevs, sizeof(*notifier->subdevs),\n>>>>> +\t\tGFP_KERNEL | __GFP_ZERO);\n>>>>> +\tif (!subdevs)\n>>>>> +\t\treturn -ENOMEM;\n>>>>> +\n>>>>> +\tif (notifier->subdevs) {\n>>>>> +\t\tmemcpy(subdevs, notifier->subdevs,\n>>>>> +\t\t sizeof(*subdevs) * notifier->num_subdevs);\n>>>>> +\n>>>>> +\t\tkvfree(notifier->subdevs);\n>>>>> +\t}\n>>>>> +\n>>>>> +\tnotifier->subdevs = subdevs;\n>>>>> +\tnotifier->max_subdevs = max_subdevs;\n>>>>> +\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> +\n>>>>> +static int v4l2_async_notifier_fwnode_parse_endpoint(\n>>>>> +\tstruct device *dev, struct v4l2_async_notifier *notifier,\n>>>>> +\tstruct fwnode_handle *endpoint, unsigned int asd_struct_size,\n>>>>> +\tint (*parse_endpoint)(struct device *dev,\n>>>>> +\t\t\t struct v4l2_fwnode_endpoint *vep,\n>>>>> +\t\t\t struct v4l2_async_subdev *asd))\n>>>>> +{\n>>>>> +\tstruct v4l2_async_subdev *asd;\n>>>>> +\tstruct v4l2_fwnode_endpoint *vep;\n>>>>> +\tstruct fwnode_endpoint ep;\n>>>>> +\tint ret = 0;\n>>>>> +\n>>>>> +\tasd = kzalloc(asd_struct_size, GFP_KERNEL);\n>>>>> +\tif (!asd)\n>>>>> +\t\treturn -ENOMEM;\n>>>>> +\n>>>>> +\tasd->match.fwnode.fwnode =\n>>>>> +\t\tfwnode_graph_get_remote_port_parent(endpoint);\n>>>>> +\tif (!asd->match.fwnode.fwnode) {\n>>>>> +\t\tdev_warn(dev, \"bad remote port parent\\n\");\n>>>>> +\t\tret = -EINVAL;\n>>>>> +\t\tgoto out_err;\n>>>>> +\t}\n>>>>> +\n>>>>> +\t/* Ignore endpoints the parsing of which failed. */\n>>>>> +\tvep = v4l2_fwnode_endpoint_alloc_parse(endpoint);\n>>>>> +\tif (IS_ERR(vep)) {\n>>>>> +\t\tret = PTR_ERR(vep);\n>>>>> +\t\tdev_warn(dev, \"unable to parse V4L2 fwnode endpoint (%d)\\n\",\n>>>>> +\t\t\t ret);\n>>>>> +\t\tgoto out_err;\n>>>>> +\t}\n>>>>> +\n>>>>> +\tep = vep->base;\n>>>>> +\n>>>>> +\tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n>>>>> +\tv4l2_fwnode_endpoint_free(vep);\n>>>>> +\tif (ret == -ENOTCONN) {\n>>>>> +\t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", ep.port, ep.id);\n>>>>> +\t\tkfree(asd);\n>>>>\n>>>> Shouldn't there be a call to fwnode_handle_put()?\n>>>\n>>> Actually no. But your hunch is right in the sense that I think there are\n>>> issues. The fwnode_endpoint (as of_endpoint) contains the fwnode which is\n>>> referenced, but no reference is taken here. One couldn't be released either\n>>> later on, as the fwnode field is const.\n>>>\n>>> I guess this is almost fine as long as the fwnode field is used for pointer\n>>> comparison and nothing else but you can never be sure what drivers actually\n>>> do.\n>>>\n>>> This actually should be addressed for both OF and fwnode but it's mostly\n>>> independent of this patchset. Luckily there are not *that* many users of\n>>> this. The V4L2 fwnode interface that allocates and releases endpoints makes\n>>> this quite a bit easier actually.\n>>\n>> I think a comment or two here would be helpful. If I tripped over this, so\n>> will others.\n> \n> Reading the code again, I noticed I didn't read far enough the last time.\n> While v4l2_fwnode_endpoint_alloc_parse() won't take a reference,\n> fwnode_graph_get_remote_port_parent() does. That reference indeed isn't\n> released right now.\n> \n> It shouldn't be too hard to do that properly in\n> v4l2_async_notifier_release(). I guess I'll do just that actually. By that\n> time, there are no users left anymore.\n> \n> Good catch.\n> \n>>\n>>>\n>>>>\n>>>>> +\t\treturn 0;\n>>>>> +\t} else if (ret < 0) {\n>>>>> +\t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n>>>>> +\t\t\t ep.port, ep.id, ret);\n>>>>> +\t\tgoto out_err;\n>>>>> +\t}\n>>>>\n>>>> I think this would be better and it avoids the need for the ep local variable:\n>>>>\n>>>> \tret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;\n>>>> \tif (ret == -ENOTCONN)\n>>>> \t\tdev_dbg(dev, \"ignoring endpoint %u,%u\\n\", vep->base.port, vep->base.id);\n>>>> \telse if (ret < 0)\n>>>> \t\tdev_warn(dev, \"driver could not parse endpoint %u,%u (%d)\\n\",\n>>>> \t\t\t vep->base.port, vep->base.id, ret);\n>>>> \tv4l2_fwnode_endpoint_free(vep);\n>>>> \tif (ret < 0)\n>>>> \t\tgoto out_err;\n>>>>\n>>>> And the 'return ret;' below would become:\n>>>>\n>>>> \treturn ret == -ENOTCONN ? 0 : ret;\n>>>\n>>> Looks good to me, I'll use it.\n>>>\n>>>>\n>>>>> +\n>>>>> +\tasd->match_type = V4L2_ASYNC_MATCH_FWNODE;\n>>>>> +\tnotifier->subdevs[notifier->num_subdevs] = asd;\n>>>>> +\tnotifier->num_subdevs++;\n>>>>> +\n>>>>> +\treturn 0;\n>>>>> +\n>>>>> +out_err:\n>>>>> +\tfwnode_handle_put(asd->match.fwnode.fwnode);\n>>\n>> Just a reminder: this put() should now check if ret != -ENOTCONN.\n> \n> Should it? Even if ret == -ENOTCONN, the reference is there and needs to be\n> released as the endpoint is not used (just as in the case of an error).\n\nWell, in the previous reply you mentioned that the put wasn't necessary, hence\nmy comment. Since it is now needed again you can ignore this.\n\nRegards,\n\n\tHans\n\n> \n>>\n>>>>> +\tkfree(asd);\n>>>>> +\n>>>>> +\treturn ret;\n>>>>> +}\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnzdJ0PGlz9sMN\n\tfor ;\n\tThu, 7 Sep 2017 22:04:04 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754796AbdIGMED (ORCPT\n\t);\n\tThu, 7 Sep 2017 08:04:03 -0400","from lb1-smtp-cloud7.xs4all.net ([194.109.24.24]:34527 \"EHLO\n\tlb1-smtp-cloud7.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1754606AbdIGMEC (ORCPT\n\t); Thu, 7 Sep 2017 08:04:02 -0400","from [IPv6:2001:420:44c1:2579:9dea:9e9e:3419:5bfe]\n\t([IPv6:2001:420:44c1:2579:9dea:9e9e:3419:5bfe])\n\tby smtp-cloud7.xs4all.net with ESMTPA\n\tid pvXJds8ekb2snpvXMdvahb; Thu, 07 Sep 2017 14:04:00 +0200"],"Subject":"Re: [PATCH v8 06/21] v4l: fwnode: Support generic parsing of graph\n\tendpoints in a device","To":"Sakari Ailus ","Cc":"Sakari Ailus ,\n\tlinux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, laurent.pinchart@ideasonboard.com,\n\tdevicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-7-sakari.ailus@linux.intel.com>\n\t\n\t<20170907073446.ajxsscekwrbfbtgk@valkosipuli.retiisi.org.uk>\n\t<85733523-7f6b-4aaf-55ca-e60e76719874@xs4all.nl>\n\t<20170907095850.m7mlag3tofwbj2jc@valkosipuli.retiisi.org.uk>","From":"Hans Verkuil ","Message-ID":"<8123f8c5-9045-2a22-c97f-09d36e504e2e@xs4all.nl>","Date":"Thu, 7 Sep 2017 14:03:57 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170907095850.m7mlag3tofwbj2jc@valkosipuli.retiisi.org.uk>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfLcRU6JeDheWwUQBafYxgImUy1iuuwS9uXY6Bc7UYISDBWHh79iBMQmbq6eXkT3QhnrSadqbCwXh/BUIUlvyzakKVWpE3DdAyCaBE/XF35FLv4v1MRRG\n\tjNvmVTPTcRLAZKOAkChASvR2d/PdbSlCEBVIT7EBacSyFdOBdv1T9NAAPlxgp832e/NK/VEfekGfVgYbI83+1CanWCKf6GYgLkVOBIPWdZ1yXz6naIGGpcV8\n\td2sf1d9ZzjITl8g4eT7MCRIv4JubusBBk2C9NLqkkmyvxx5234s+iqTOS1QXF5xhEOx0QtE8lWIDkcnWG9nsJQMQxndjbVk5650e1EGZJBkYDBdUvMTq13UW\n\twAxR24H3kSatNKN4izMq1EwJCUj0XzQ+pJyQfSuet8Vuo9VCtBKnh+LaIw+brq8tte2P+z7if7ljQK3ip/0VduN5n1oWfSTscmKd0ktsXLqiX4CQ9mLCcuFH\n\t0zGIcGjk2WFA4lZzcdHMqvuAnxjnZRhXAW5LzGZbXu/dJpaQMgmR0QOUfyA=","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1765296,"web_url":"http://patchwork.ozlabs.org/comment/1765296/","msgid":"<20170908123914.GN18365@amd>","list_archive_url":null,"date":"2017-09-08T12:39:14","subject":"Re: [PATCH v8 01/21] v4l: fwnode: Move KernelDoc documentation to\n\tthe header","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"On Tue 2017-09-05 16:05:33, Sakari Ailus wrote:\n> In V4L2 the practice is to have the KernelDoc documentation in the header\n> and not in .c source code files. This consequientally makes the V4L2\n> fwnode function documentation part of the Media documentation build.\n> \n> Also correct the link related function and argument naming in\n> documentation.\n> \n> Signed-off-by: Sakari Ailus \n> Reviewed-by: Niklas Söderlund \n> Acked-by: Hans Verkuil \n\nAcked-by: Pavel Machek ","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpcMZ2LMZz9tY3\n\tfor ;\n\tFri, 8 Sep 2017 22:39:22 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754792AbdIHMjQ (ORCPT\n\t);\n\tFri, 8 Sep 2017 08:39:16 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:40576 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753645AbdIHMjP (ORCPT\n\t); Fri, 8 Sep 2017 08:39:15 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 876F5824F1; Fri, 8 Sep 2017 14:39:14 +0200 (CEST)"],"Date":"Fri, 8 Sep 2017 14:39:14 +0200","From":"Pavel Machek ","To":"Sakari Ailus ","Cc":"linux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, hverkuil@xs4all.nl,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tsre@kernel.org","Subject":"Re: [PATCH v8 01/21] v4l: fwnode: Move KernelDoc documentation to\n\tthe header","Message-ID":"<20170908123914.GN18365@amd>","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-2-sakari.ailus@linux.intel.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vDEbda84Uy/oId5W\"","Content-Disposition":"inline","In-Reply-To":"<20170905130553.1332-2-sakari.ailus@linux.intel.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1765298,"web_url":"http://patchwork.ozlabs.org/comment/1765298/","msgid":"<20170908124010.GO18365@amd>","list_archive_url":null,"date":"2017-09-08T12:40:10","subject":"Re: [PATCH v8 03/21] v4l: async: Use more intuitive names for\n\tinternal functions","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"On Tue 2017-09-05 16:05:35, Sakari Ailus wrote:\n> Rename internal functions to make the names of the functions better\n> describe what they do.\n> \n> \tOld name\t\t\tNew name\n> \tv4l2_async_test_notify\tv4l2_async_match_notify\n> \tv4l2_async_belongs\tv4l2_async_find_match\n> \n> Signed-off-by: Sakari Ailus \n\nAcked-by: Pavel Machek ","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpcNb4tqsz9sQl\n\tfor ;\n\tFri, 8 Sep 2017 22:40:13 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755020AbdIHMkM (ORCPT\n\t);\n\tFri, 8 Sep 2017 08:40:12 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:40642 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754951AbdIHMkL (ORCPT\n\t); Fri, 8 Sep 2017 08:40:11 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 6242A81FC8; Fri, 8 Sep 2017 14:40:10 +0200 (CEST)"],"Date":"Fri, 8 Sep 2017 14:40:10 +0200","From":"Pavel Machek ","To":"Sakari Ailus ","Cc":"linux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,\n\trobh@kernel.org, hverkuil@xs4all.nl,\n\tlaurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,\n\tsre@kernel.org","Subject":"Re: [PATCH v8 03/21] v4l: async: Use more intuitive names for\n\tinternal functions","Message-ID":"<20170908124010.GO18365@amd>","References":"<20170905130553.1332-1-sakari.ailus@linux.intel.com>\n\t<20170905130553.1332-4-sakari.ailus@linux.intel.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"XVTPT6MZt3zd/C+/\"","Content-Disposition":"inline","In-Reply-To":"<20170905130553.1332-4-sakari.ailus@linux.intel.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]