From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753363AbbGAXgh (ORCPT ); Wed, 1 Jul 2015 19:36:37 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:48914 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751300AbbGAXg1 (ORCPT ); Wed, 1 Jul 2015 19:36:27 -0400 From: "Rafael J. Wysocki" To: Tomeu Vizoso Cc: linux-kernel@vger.kernel.org, Mark Brown , linux-acpi@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-pwm@vger.kernel.org, alsa-devel@alsa-project.org, Greg Kroah-Hartman Subject: Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node Date: Thu, 02 Jul 2015 02:02:45 +0200 Message-ID: <2589152.el9mWEWqDg@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.1.0-rc5+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1435743667-11987-3-git-send-email-tomeu.vizoso@collabora.com> References: <1435743667-11987-1-git-send-email-tomeu.vizoso@collabora.com> <1435743667-11987-3-git-send-email-tomeu.vizoso@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday, July 01, 2015 11:40:57 AM Tomeu Vizoso wrote: > Adds API that allows callers to find out what other firmware nodes a > node depends on. > > Implementors of bindings documentation can register callbacks that > return the dependencies of a node. > > Dependency information can be used to change the order in which devices > are probed, or to print a warning when a device node is going to be > probed without all its dependencies fulfilled. > > Signed-off-by: Tomeu Vizoso I'd like to see a description of the new API in English in the changelog. > --- > > Changes in v2: > - Allow bindings implementations register a function instead of using > class callbacks, as not only subsystems implement firmware bindings. > > drivers/base/property.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/fwnode.h | 5 +++ > include/linux/property.h | 12 +++++++ > 3 files changed, 108 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 8ead1ba..9d38ede 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -19,7 +19,13 @@ > #include > #include > Please add a comment describing this structure. In particular, what it is supposed to be used for and how it is supposed to be used. > +struct dependency_parser { > + struct list_head parser; I'd rather call this "list_node" or something like that. > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps); > +}; > + > static bool fwnode_match_enable = false; > +static LIST_HEAD(dependency_parsers); > > /** > * device_add_property_set - Add a collection of properties to a device object. > @@ -553,6 +559,27 @@ bool device_dma_is_coherent(struct device *dev) > EXPORT_SYMBOL_GPL(device_dma_is_coherent); > > /** > + * fwnode_add_dependency - add firmware node to the passed dependency list > + * @fwnode: Firmware node to add to dependency list > + * @list: Dependency list to add the fwnode to > + */ > +void fwnode_add_dependency(struct fwnode_handle *fwnode, > + struct list_head *list) > +{ > + struct fwnode_dependency *dep; > + > + dep = kzalloc(sizeof(*dep), GFP_KERNEL); > + if (!dep) > + return; > + > + INIT_LIST_HEAD(&dep->dependency); > + dep->fwnode = fwnode; > + > + list_add_tail(&dep->dependency, list); > +} > +EXPORT_SYMBOL_GPL(fwnode_add_dependency); > + > +/** > * fwnode_get_parent - return the parent node of a device node > * @fwnode: Device node to find the parent node of > */ > @@ -600,6 +627,70 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible) > EXPORT_SYMBOL_GPL(fwnode_is_compatible); > > /** > + * fwnode_add_dependency_parser - register dependency parser > + * @func: Function that will be called to find out dependencies of a node > + * > + * Registers a callback that will be called when collecting the dependencies > + * of a firmware node. The callback should inspect the properties of the node > + * and call fwnode_add_dependency() for each dependency it recognizes, from > + * the bindings documentation. > + */ > +void fwnode_add_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)) > +{ > + struct dependency_parser *parser; > + > + parser = kzalloc(sizeof(*parser), GFP_KERNEL); > + if (!parser) > + return; > + > + INIT_LIST_HEAD(&parser->parser); > + parser->func = func; > + > + list_add_tail(&parser->parser, &dependency_parsers); We're modifying a global list here. Any locking needed? RCU? Whatever? > +} > +EXPORT_SYMBOL_GPL(fwnode_add_dependency_parser); > + > +/** > + * fwnode_remove_dependency_parser - unregister dependency parser > + * @func: Function that was to be called to find out dependencies of a node > + */ > +void fwnode_remove_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)) > +{ > + struct dependency_parser *parser, *tmp; > + > + list_for_each_entry_safe(parser, tmp, &dependency_parsers, parser) { > + if (parser->func == func) { > + list_del(&parser->parser); > + kfree(parser); > + return; > + } > + } > +} > +EXPORT_SYMBOL_GPL(fwnode_remove_dependency_parser); > + > +/** > + * fwnode_get_dependencies - find out what dependencies a firmware node has > + * @fwnode: firmware node to find its dependencies > + * @deps: list of struct fwnode_dependency in which dependencies will be placed > + */ > +void fwnode_get_dependencies(struct fwnode_handle *fwnode, > + struct list_head *deps) > +{ > + struct dependency_parser *parser; > + struct fwnode_handle *child; > + > + list_for_each_entry(parser, &dependency_parsers, parser) > + parser->func(fwnode, deps); > + > + /* Some device nodes will have dependencies in non-device sub-nodes */ > + fwnode_for_each_child_node(fwnode, child) > + if (!fwnode_property_present(child, "compatible")) This is a blatant OF-ism. We need to think about a generic way to express that. > + fwnode_get_dependencies(child, deps); > +} > + > +/** > * fwnode_driver_match_device - Tell if a driver matches a device. > * @drv: the device_driver structure to test > * @dev: the device structure to match against > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index 0408545..68ab558 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -24,4 +24,9 @@ struct fwnode_handle { > struct fwnode_handle *secondary; > }; > > +struct fwnode_dependency { > + struct fwnode_handle *fwnode; > + struct list_head dependency; So this is a node in the list of dependencies, right? I'd call that field "list_node", then. > +}; And fwnode is a firmware node that the owner of the list depends on, right? > + > #endif > diff --git a/include/linux/property.h b/include/linux/property.h > index 4e453c4..b8b86ea 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -86,6 +86,18 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible); > bool fwnode_driver_match_device(struct device *dev, > const struct device_driver *drv); > > +void fwnode_add_dependency(struct fwnode_handle *fwnode, > + struct list_head *list); > + > +void fwnode_add_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)); > + > +void fwnode_remove_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)); > + > +void fwnode_get_dependencies(struct fwnode_handle *fwnode, > + struct list_head *list); > + > unsigned int device_get_child_node_count(struct device *dev); > > static inline bool device_property_read_bool(struct device *dev, > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Date: Wed, 01 Jul 2015 23:36:42 +0000 Subject: Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node Message-Id: <2589152.el9mWEWqDg@vostro.rjw.lan> List-Id: References: <1435743667-11987-1-git-send-email-tomeu.vizoso@collabora.com> <1435743667-11987-3-git-send-email-tomeu.vizoso@collabora.com> In-Reply-To: <1435743667-11987-3-git-send-email-tomeu.vizoso@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Tomeu Vizoso Cc: linux-kernel@vger.kernel.org, Mark Brown , linux-acpi@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-pwm@vger.kernel.org, alsa-devel@alsa-project.org, Greg Kroah-Hartman On Wednesday, July 01, 2015 11:40:57 AM Tomeu Vizoso wrote: > Adds API that allows callers to find out what other firmware nodes a > node depends on. > > Implementors of bindings documentation can register callbacks that > return the dependencies of a node. > > Dependency information can be used to change the order in which devices > are probed, or to print a warning when a device node is going to be > probed without all its dependencies fulfilled. > > Signed-off-by: Tomeu Vizoso I'd like to see a description of the new API in English in the changelog. > --- > > Changes in v2: > - Allow bindings implementations register a function instead of using > class callbacks, as not only subsystems implement firmware bindings. > > drivers/base/property.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/fwnode.h | 5 +++ > include/linux/property.h | 12 +++++++ > 3 files changed, 108 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 8ead1ba..9d38ede 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -19,7 +19,13 @@ > #include > #include > Please add a comment describing this structure. In particular, what it is supposed to be used for and how it is supposed to be used. > +struct dependency_parser { > + struct list_head parser; I'd rather call this "list_node" or something like that. > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps); > +}; > + > static bool fwnode_match_enable = false; > +static LIST_HEAD(dependency_parsers); > > /** > * device_add_property_set - Add a collection of properties to a device object. > @@ -553,6 +559,27 @@ bool device_dma_is_coherent(struct device *dev) > EXPORT_SYMBOL_GPL(device_dma_is_coherent); > > /** > + * fwnode_add_dependency - add firmware node to the passed dependency list > + * @fwnode: Firmware node to add to dependency list > + * @list: Dependency list to add the fwnode to > + */ > +void fwnode_add_dependency(struct fwnode_handle *fwnode, > + struct list_head *list) > +{ > + struct fwnode_dependency *dep; > + > + dep = kzalloc(sizeof(*dep), GFP_KERNEL); > + if (!dep) > + return; > + > + INIT_LIST_HEAD(&dep->dependency); > + dep->fwnode = fwnode; > + > + list_add_tail(&dep->dependency, list); > +} > +EXPORT_SYMBOL_GPL(fwnode_add_dependency); > + > +/** > * fwnode_get_parent - return the parent node of a device node > * @fwnode: Device node to find the parent node of > */ > @@ -600,6 +627,70 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible) > EXPORT_SYMBOL_GPL(fwnode_is_compatible); > > /** > + * fwnode_add_dependency_parser - register dependency parser > + * @func: Function that will be called to find out dependencies of a node > + * > + * Registers a callback that will be called when collecting the dependencies > + * of a firmware node. The callback should inspect the properties of the node > + * and call fwnode_add_dependency() for each dependency it recognizes, from > + * the bindings documentation. > + */ > +void fwnode_add_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)) > +{ > + struct dependency_parser *parser; > + > + parser = kzalloc(sizeof(*parser), GFP_KERNEL); > + if (!parser) > + return; > + > + INIT_LIST_HEAD(&parser->parser); > + parser->func = func; > + > + list_add_tail(&parser->parser, &dependency_parsers); We're modifying a global list here. Any locking needed? RCU? Whatever? > +} > +EXPORT_SYMBOL_GPL(fwnode_add_dependency_parser); > + > +/** > + * fwnode_remove_dependency_parser - unregister dependency parser > + * @func: Function that was to be called to find out dependencies of a node > + */ > +void fwnode_remove_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)) > +{ > + struct dependency_parser *parser, *tmp; > + > + list_for_each_entry_safe(parser, tmp, &dependency_parsers, parser) { > + if (parser->func = func) { > + list_del(&parser->parser); > + kfree(parser); > + return; > + } > + } > +} > +EXPORT_SYMBOL_GPL(fwnode_remove_dependency_parser); > + > +/** > + * fwnode_get_dependencies - find out what dependencies a firmware node has > + * @fwnode: firmware node to find its dependencies > + * @deps: list of struct fwnode_dependency in which dependencies will be placed > + */ > +void fwnode_get_dependencies(struct fwnode_handle *fwnode, > + struct list_head *deps) > +{ > + struct dependency_parser *parser; > + struct fwnode_handle *child; > + > + list_for_each_entry(parser, &dependency_parsers, parser) > + parser->func(fwnode, deps); > + > + /* Some device nodes will have dependencies in non-device sub-nodes */ > + fwnode_for_each_child_node(fwnode, child) > + if (!fwnode_property_present(child, "compatible")) This is a blatant OF-ism. We need to think about a generic way to express that. > + fwnode_get_dependencies(child, deps); > +} > + > +/** > * fwnode_driver_match_device - Tell if a driver matches a device. > * @drv: the device_driver structure to test > * @dev: the device structure to match against > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index 0408545..68ab558 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -24,4 +24,9 @@ struct fwnode_handle { > struct fwnode_handle *secondary; > }; > > +struct fwnode_dependency { > + struct fwnode_handle *fwnode; > + struct list_head dependency; So this is a node in the list of dependencies, right? I'd call that field "list_node", then. > +}; And fwnode is a firmware node that the owner of the list depends on, right? > + > #endif > diff --git a/include/linux/property.h b/include/linux/property.h > index 4e453c4..b8b86ea 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -86,6 +86,18 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible); > bool fwnode_driver_match_device(struct device *dev, > const struct device_driver *drv); > > +void fwnode_add_dependency(struct fwnode_handle *fwnode, > + struct list_head *list); > + > +void fwnode_add_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)); > + > +void fwnode_remove_dependency_parser( > + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps)); > + > +void fwnode_get_dependencies(struct fwnode_handle *fwnode, > + struct list_head *list); > + > unsigned int device_get_child_node_count(struct device *dev); > > static inline bool device_property_read_bool(struct device *dev, > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.