On Friday, July 10, 2015 03:14:38 PM Tomeu Vizoso wrote:
On 2 July 2015 at 02:02, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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 tomeu.vizoso@collabora.com
I'd like to see a description of the new API in English in the changelog.
Agreed.
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 <linux/platform_device.h> #include <linux/property.h>
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.
Ok.
+struct dependency_parser {
struct list_head parser;I'd rather call this "list_node" or something like that.
Ok.
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?
Oops.
+} +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.
My impression from reading the existing usage of fwnode in gpiolib and the examples in the link below was that we were going to share the bindings between DT and ACPI. Doesn't that extend to the meaning of the compatible property?
http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUI...
Not entirely.
An ACPI device object without the "compatible" property may still represent a valid device (and usually does), so it is not enough to check whether or not the "compatible" property is present for that in general.
Thanks, Rafael