[alsa-devel] [PATCH v2 0/12] Discover and probe dependencies
Hi,
this is version 2 of a series that probes devices in dependency order so as to avoid deferred probes. While deferred probing is a powerful solution that makes sure that you eventually get a working system at the end of the boot, can make it very time consuming to find out why a device didn't probe and can also introduce big delays in when a device actually probes by sending it to the end of the deferred queue.
So far I have only tested on a Tegra124 Chromebook.
Thanks,
Tomeu
Changes in v2: - Instead of delaying all probes until late_initcall, only delay matches of platform devices that have a firmware node attached. - Allow bindings implementations register a function instead of using class callbacks, as not only subsystems implement firmware bindings. - Move strends to string.h - Document that consumers of backlight devices can use the 'backlight' property to hold a phandle to the backlight device. - Allocate the list of dependencies and pass it to the function that fills it.
Tomeu Vizoso (12): device: property: delay device-driver matches device: property: find dependencies of a firmware node string: Introduce strends() gpio: register dependency parser for firmware nodes gpu: host1x: register dependency parser for firmware nodes backlight: Document consumers of backlight nodes backlight: register dependency parser for firmware nodes USB: EHCI: register dependency parser for firmware nodes regulator: register dependency parser for firmware nodes pwm: register dependency parser for firmware nodes ASoC: tegra: register dependency parser for firmware nodes driver-core: probe dependencies before probing
.../bindings/video/backlight/backlight.txt | 22 ++++ drivers/base/dd.c | 139 +++++++++++++++++++++ drivers/base/property.c | 120 ++++++++++++++++++ drivers/gpio/gpiolib.c | 54 ++++++++ drivers/gpu/host1x/dev.c | 26 ++++ drivers/pwm/core.c | 28 +++++ drivers/regulator/core.c | 27 ++++ drivers/usb/host/ehci-tegra.c | 16 +++ drivers/video/backlight/backlight.c | 16 +++ include/linux/fwnode.h | 5 + include/linux/property.h | 12 ++ include/linux/string.h | 13 ++ sound/soc/tegra/tegra_max98090.c | 42 ++++++- 13 files changed, 519 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/video/backlight/backlight.txt
Delay matches of platform devices until late_initcall, when we are sure that all built-in drivers have been registered already. This is needed to prevent deferred probes because of some dependencies' drivers not having registered yet.
This reduces the total amount of work that the kernel does during boot because it won't try to match devices to drivers when built-in drivers are still registering but also reduces some parallelism, so total boot time might slightly increase or decrease depending on the platform and kernel configuration.
This change will make make possible to prevent any deferred probes once devices are probed in dependency order.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: - Instead of delaying all probes until late_initcall, only delay matches of platform devices that have a firmware node attached.
drivers/base/property.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c index 8528eb9..8ead1ba 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -16,8 +16,11 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/platform_device.h> #include <linux/property.h>
+static bool fwnode_match_enable = false; + /** * device_add_property_set - Add a collection of properties to a device object. * @dev: Device to add properties to. @@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible); bool fwnode_driver_match_device(struct device *dev, const struct device_driver *drv) { + /* + * Delay matches of platform devices until late_initcall, when we are + * sure that all built-in drivers have been registered already. This + * is needed to prevent deferred probes because of some drivers + * not having registered yet. + */ + if(dev->bus == &platform_bus_type && !fwnode_match_enable) + return false; + if (is_of_node(dev->fwnode)) return of_driver_match_device(dev, drv); else if (is_acpi_node(dev->fwnode)) @@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev, return false; } EXPORT_SYMBOL_GPL(fwnode_driver_match_device); + +static int __device_attach(struct device *dev, void *data) +{ + device_initial_probe(dev); + + return 0; +} + +static int fwnode_match_initcall(void) +{ + fwnode_match_enable = true; + + bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach); + + return 0; +} +late_initcall(fwnode_match_initcall);
On Wednesday, July 01, 2015 11:40:56 AM Tomeu Vizoso wrote:
Delay matches of platform devices until late_initcall, when we are sure that all built-in drivers have been registered already. This is needed to prevent deferred probes because of some dependencies' drivers not having registered yet.
This reduces the total amount of work that the kernel does during boot because it won't try to match devices to drivers when built-in drivers are still registering but also reduces some parallelism, so total boot time might slightly increase or decrease depending on the platform and kernel configuration.
This change will make make possible to prevent any deferred probes once devices are probed in dependency order.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com
Changes in v2:
- Instead of delaying all probes until late_initcall, only delay matches of platform devices that have a firmware node attached.
drivers/base/property.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c index 8528eb9..8ead1ba 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -16,8 +16,11 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/platform_device.h> #include <linux/property.h>
+static bool fwnode_match_enable = false;
/**
- device_add_property_set - Add a collection of properties to a device object.
- @dev: Device to add properties to.
@@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible); bool fwnode_driver_match_device(struct device *dev, const struct device_driver *drv) {
- /*
* Delay matches of platform devices until late_initcall, when we are* sure that all built-in drivers have been registered already. This* is needed to prevent deferred probes because of some drivers* not having registered yet.*/- if(dev->bus == &platform_bus_type && !fwnode_match_enable)
return false;
I'm not particularly enthusiastic about referring to specific bus types in generic code like that.
What about having a special version of fwnode_driver_match_device() specifically for the platform bus type that will do the check?
- if (is_of_node(dev->fwnode)) return of_driver_match_device(dev, drv); else if (is_acpi_node(dev->fwnode))
@@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev, return false; } EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
+static int __device_attach(struct device *dev, void *data) +{
- device_initial_probe(dev);
- return 0;
+}
+static int fwnode_match_initcall(void) +{
- fwnode_match_enable = true;
- bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);
- return 0;
+} +late_initcall(fwnode_match_initcall);
On 2 July 2015 at 01:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Wednesday, July 01, 2015 11:40:56 AM Tomeu Vizoso wrote:
Delay matches of platform devices until late_initcall, when we are sure that all built-in drivers have been registered already. This is needed to prevent deferred probes because of some dependencies' drivers not having registered yet.
This reduces the total amount of work that the kernel does during boot because it won't try to match devices to drivers when built-in drivers are still registering but also reduces some parallelism, so total boot time might slightly increase or decrease depending on the platform and kernel configuration.
This change will make make possible to prevent any deferred probes once devices are probed in dependency order.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com
Changes in v2:
- Instead of delaying all probes until late_initcall, only delay matches of platform devices that have a firmware node attached.
drivers/base/property.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c index 8528eb9..8ead1ba 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -16,8 +16,11 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/platform_device.h> #include <linux/property.h>
+static bool fwnode_match_enable = false;
/**
- device_add_property_set - Add a collection of properties to a device object.
- @dev: Device to add properties to.
@@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible); bool fwnode_driver_match_device(struct device *dev, const struct device_driver *drv) {
/** Delay matches of platform devices until late_initcall, when we are* sure that all built-in drivers have been registered already. This* is needed to prevent deferred probes because of some drivers* not having registered yet.*/if(dev->bus == &platform_bus_type && !fwnode_match_enable)return false;I'm not particularly enthusiastic about referring to specific bus types in generic code like that.
Agreed.
What about having a special version of fwnode_driver_match_device() specifically for the platform bus type that will do the check?
Have moved all this code to base/platform.c instead, as I don't see any reason to have it in property.c.
Thanks,
Tomeu
if (is_of_node(dev->fwnode)) return of_driver_match_device(dev, drv); else if (is_acpi_node(dev->fwnode))@@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev, return false; } EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
+static int __device_attach(struct device *dev, void *data) +{
device_initial_probe(dev);return 0;+}
+static int fwnode_match_initcall(void) +{
fwnode_match_enable = true;bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);return 0;+} +late_initcall(fwnode_match_initcall);
-- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Jul 01, 2015 at 11:40:56AM +0200, Tomeu Vizoso wrote:
Delay matches of platform devices until late_initcall, when we are sure that all built-in drivers have been registered already. This is needed to prevent deferred probes because of some dependencies' drivers not having registered yet.
I have to say I'm still not 100% clear that special casing platform devices makes sense here - I can see that platform devices are usually the first devices to instantiate but there are other kinds of devices and it's not obvious what the benefit of specifically picking out platform devices as opposed to just deferring all devices is.
Hi Mark,
On Thu, Jul 16, 2015 at 10:23 PM, Mark Brown broonie@kernel.org wrote:
On Wed, Jul 01, 2015 at 11:40:56AM +0200, Tomeu Vizoso wrote:
Delay matches of platform devices until late_initcall, when we are sure that all built-in drivers have been registered already. This is needed to prevent deferred probes because of some dependencies' drivers not having registered yet.
I have to say I'm still not 100% clear that special casing platform devices makes sense here - I can see that platform devices are usually the first devices to instantiate but there are other kinds of devices and it's not obvious what the benefit of specifically picking out platform devices as opposed to just deferring all devices is.
Some existing devices cannot be deferred without redesigning things quite a bit.
What I was talking about, though, was to use an opt-in mechanism for that which could be set for all platform devices, for example, by default, but it might be set for other bus types too if that's useful.
Thanks, Rafael
On Fri, Jul 17, 2015 at 01:41:16AM +0200, Rafael J. Wysocki wrote:
On Thu, Jul 16, 2015 at 10:23 PM, Mark Brown broonie@kernel.org wrote:
On Wed, Jul 01, 2015 at 11:40:56AM +0200, Tomeu Vizoso wrote:
I have to say I'm still not 100% clear that special casing platform devices makes sense here - I can see that platform devices are usually the first devices to instantiate but there are other kinds of devices and it's not obvious what the benefit of specifically picking out platform devices as opposed to just deferring all devices is.
Some existing devices cannot be deferred without redesigning things quite a bit.
OK, that should go in the changelog then - right now it's just a bit obtuse why we're doing this (and as you say it's a bit awkward). Now you mention this I'm thinking that some of the affected devices might be platform devices on some systems, IOMMUs spring to mind for example... they're one of the main bits of the system I'm aware of that still rely on probe ordering and they do tend to be platform devices.
What I was talking about, though, was to use an opt-in mechanism for that which could be set for all platform devices, for example, by default, but it might be set for other bus types too if that's useful.
Sure, I got that and do agree with you that a mechanism like you suggest would be good. I just wasn't clear why we were targetting platform devices in the first place.
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 ---
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>
+struct dependency_parser { + struct list_head parser; + 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); +} +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")) + 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; +}; + #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,
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.
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.
+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,
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...
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.
Right, fwnode_dependency is just there so we can have a list of fwnodes.
+};
And fwnode is a firmware node that the owner of the list depends on, right?
Yes, will make it clearer in the next revision.
Thanks,
Tomeu
#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. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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
To avoid duplicating code in upcoming patches that will check for postfixes in strings, add strends().
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: - Move strends to string.h
include/linux/string.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index d5dfe3e..4244363 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -146,6 +146,19 @@ static inline bool strstarts(const char *str, const char *prefix) return strncmp(str, prefix, strlen(prefix)) == 0; }
+/** + * strends - does @str end with @postfix? + * @str: string to examine + * @postfix: postfix to look for + */ +static inline bool strends(const char *str, const char *postfix) +{ + if (strlen(str) < strlen(postfix)) + return false; + + return strcmp(str + strlen(str) - strlen(postfix), postfix) == 0; +} + size_t memweight(const void *ptr, size_t bytes); void memzero_explicit(void *s, size_t count);
So the GPIO subsystem can be queried about the dependencies of nodes that consume GPIOs, as specified in bindings/gpio/gpio.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
drivers/gpio/gpiolib.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index bf4bd1d..6a3e83f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2388,4 +2388,58 @@ static int __init gpiolib_debugfs_init(void) } subsys_initcall(gpiolib_debugfs_init);
+static void gpio_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + struct device_node *np; + struct property *pp; + struct of_phandle_args pspec; + int count, i, ret; + + np = to_of_node(fwnode); + if (!np) + return; + + for_each_property_of_node(np, pp) { + if (strcmp(pp->name, "gpio") && + strcmp(pp->name, "gpios") && + !strends(pp->name, "-gpios") && + !strends(pp->name, "-gpio")) + continue; + + count = of_count_phandle_with_args(np, pp->name, + "#gpio-cells"); + for (i = 0; i < count; i++) { + ret = of_parse_phandle_with_args(np, pp->name, + "#gpio-cells", i, + &pspec); + if (ret || !pspec.np) + continue; + + fwnode_add_dependency(&pspec.np->fwnode, deps); + + of_node_put(pspec.np); + } + } + + for (i = 0;; i++) { + ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, + i, &pspec); + if (ret) + break; + + fwnode_add_dependency(&pspec.np->fwnode, deps); + + of_node_put(pspec.np); + } +} + +static int __init gpiolib_init(void) +{ + fwnode_add_dependency_parser(gpio_get_dependencies); + + return 0; +} +device_initcall(gpiolib_init); + #endif /* DEBUG_FS */
So others can find out dependencies of host1x clients, as specified in bindings/gpu/nvidia,tegra20-host1x.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
drivers/gpu/host1x/dev.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index 53d3d1d..5bb10b8 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -212,6 +212,29 @@ static struct platform_driver tegra_host1x_driver = { .remove = host1x_remove, };
+static void add_dependency(struct fwnode_handle *fwnode, + const char *property, + struct list_head *deps) +{ + struct device_node *np; + + np = of_parse_phandle(to_of_node(fwnode), property, 0); + if (!np) + return; + + fwnode_add_dependency(&np->fwnode, deps); +} + +static void host1x_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + add_dependency(fwnode, "nvidia,dpaux", deps); + add_dependency(fwnode, "nvidia,panel", deps); + add_dependency(fwnode, "nvidia,ddc-i2c-bus", deps); + add_dependency(fwnode, "nvidia,hpd-gpio", deps); + add_dependency(fwnode, "ddc-i2c-bus", deps); +} + static int __init tegra_host1x_init(void) { int err; @@ -228,6 +251,8 @@ static int __init tegra_host1x_init(void) if (err < 0) goto unregister_host1x;
+ fwnode_add_dependency_parser(host1x_get_dependencies); + return 0;
unregister_host1x: @@ -240,6 +265,7 @@ module_init(tegra_host1x_init);
static void __exit tegra_host1x_exit(void) { + fwnode_remove_dependency_parser(host1x_get_dependencies); platform_driver_unregister(&tegra_mipi_driver); platform_driver_unregister(&tegra_host1x_driver); bus_unregister(&host1x_bus_type);
Add a small note that makes explicit that properties named 'backlight' contain phandles to backlight nodes.
This is needed so that we can automatically extract dependencies on backlight devices by assuming that a property with that name contains a phandle to such a device.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: - Document that consumers of backlight devices can use the 'backlight' property to hold a phandle to the backlight device.
.../bindings/video/backlight/backlight.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/backlight/backlight.txt
diff --git a/Documentation/devicetree/bindings/video/backlight/backlight.txt b/Documentation/devicetree/bindings/video/backlight/backlight.txt new file mode 100644 index 0000000..309949d --- /dev/null +++ b/Documentation/devicetree/bindings/video/backlight/backlight.txt @@ -0,0 +1,22 @@ +Specifying backlight information for devices +============================================ + +Backlight user nodes +-------------------- + +Nodes such as display panels that refer to backlight devices can do so by simply having a property named 'backlight' that contains a phandle to a backlight node. + +Example: + + backlight: backlight { + compatible = "gpio-backlight"; + gpios = <&gpio3 4 GPIO_ACTIVE_HIGH>; + }; + + [...] + + panel: panel { + compatible = "cptt,claa101wb01"; + + backlight = <&backlight>; + };
So others can find out what depends on backlight devices, as specified in bindings/video/backlight/backlight.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
drivers/video/backlight/backlight.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index bddc8b1..ab8f5e7 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -566,8 +566,22 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node) EXPORT_SYMBOL(of_find_backlight_by_node); #endif
+static void backlight_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + struct device_node *np; + + np = of_parse_phandle(to_of_node(fwnode), "backlight", 0); + if (!np) + return; + + fwnode_add_dependency(&np->fwnode, deps); +} + static void __exit backlight_class_exit(void) { + fwnode_remove_dependency_parser(backlight_get_dependencies); + class_destroy(backlight_class); }
@@ -586,6 +600,8 @@ static int __init backlight_class_init(void) mutex_init(&backlight_dev_list_mutex); BLOCKING_INIT_NOTIFIER_HEAD(&backlight_notifier);
+ fwnode_add_dependency_parser(backlight_get_dependencies); + return 0; }
So others can find out whether a firmware node depends on a phy as specified in bindings/usb/nvidia,tegra20-ehci.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
drivers/usb/host/ehci-tegra.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c index 4031b37..3665eaa 100644 --- a/drivers/usb/host/ehci-tegra.c +++ b/drivers/usb/host/ehci-tegra.c @@ -589,6 +589,18 @@ static const struct ehci_driver_overrides tegra_overrides __initconst = { .reset = tegra_ehci_reset, };
+static void tegra_ehci_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + struct device_node *np; + + np = of_parse_phandle(to_of_node(fwnode), "nvidia,phy", 0); + if (!np) + return; + + fwnode_add_dependency(&np->fwnode, deps); +} + static int __init ehci_tegra_init(void) { if (usb_disabled()) @@ -611,6 +623,8 @@ static int __init ehci_tegra_init(void) tegra_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma; tegra_ehci_hc_driver.hub_control = tegra_ehci_hub_control;
+ fwnode_add_dependency_parser(tegra_ehci_get_dependencies); + return platform_driver_register(&tegra_ehci_driver); } module_init(ehci_tegra_init); @@ -618,6 +632,8 @@ module_init(ehci_tegra_init); static void __exit ehci_tegra_cleanup(void) { platform_driver_unregister(&tegra_ehci_driver); + + fwnode_remove_dependency_parser(tegra_ehci_get_dependencies); } module_exit(ehci_tegra_cleanup);
So others can find out what depends on regulators, as specified in bindings/regulator/regulator.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
drivers/regulator/core.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index c9f7201..535cad0 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -4112,6 +4112,31 @@ static const struct file_operations regulator_summary_fops = { #endif };
+static void regulator_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + struct device_node *np; + struct property *pp; + struct device_node *dep; + + np = to_of_node(fwnode); + if (!np) + return; + + for_each_property_of_node(np, pp) { + if (!strends(pp->name, "-supply")) + continue; + + dep = of_parse_phandle(np, pp->name, 0); + if (!dep) + continue; + + fwnode_add_dependency(&dep->fwnode, deps); + + of_node_put(dep); + } +} + static int __init regulator_init(void) { int ret; @@ -4130,6 +4155,8 @@ static int __init regulator_init(void)
regulator_dummy_init();
+ fwnode_add_dependency_parser(regulator_get_dependencies); + return ret; }
On Wed, Jul 01, 2015 at 11:41:04AM +0200, Tomeu Vizoso wrote:
So others can find out what depends on regulators, as specified in bindings/regulator/regulator.txt.
Reviewed-by: Mark Brown broonie@kernel.org
from a regulator point of view.
So others can find out what depends on pwm controllers, as specified in bindings/pwm/pwm.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
drivers/pwm/core.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 3a7769f..81b4fc0 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -917,11 +917,39 @@ static const struct file_operations pwm_debugfs_ops = { .release = seq_release, };
+static void pwm_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + struct device_node *np; + struct of_phandle_args pspec; + int count, i, ret; + + np = to_of_node(fwnode); + if (!np) + return; + + count = of_count_phandle_with_args(np, "pwms", + "#pwm-cells"); + for (i = 0; i < count; i++) { + ret = of_parse_phandle_with_args(np, "pwms", + "#pwm-cells", i, + &pspec); + if (ret || !pspec.np) + continue; + + fwnode_add_dependency(&pspec.np->fwnode, deps); + + of_node_put(pspec.np); + } +} + static int __init pwm_debugfs_init(void) { debugfs_create_file("pwm", S_IFREG | S_IRUGO, NULL, NULL, &pwm_debugfs_ops);
+ fwnode_add_dependency_parser(pwm_get_dependencies); + return 0; }
So others can find out what dependencies a nvidia,tegra-audio-max98090 device has, as specified in bindings/sound/nvidia,tegra-audio-max98090.txt.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com ---
Changes in v2: None
sound/soc/tegra/tegra_max98090.c | 42 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c index 902da36..0f7cbf3 100644 --- a/sound/soc/tegra/tegra_max98090.c +++ b/sound/soc/tegra/tegra_max98090.c @@ -316,7 +316,47 @@ static struct platform_driver tegra_max98090_driver = { .probe = tegra_max98090_probe, .remove = tegra_max98090_remove, }; -module_platform_driver(tegra_max98090_driver); + +static void add_dependency(struct fwnode_handle *fwnode, + const char *property, + struct list_head *deps) +{ + struct device_node *np; + + np = of_parse_phandle(to_of_node(fwnode), property, 0); + if (!np) + return; + + fwnode_add_dependency(&np->fwnode, deps); +} + +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode, + struct list_head *deps) +{ + add_dependency(fwnode, "nvidia,i2s-controller", deps); + add_dependency(fwnode, "nvidia,audio-codec", deps); +} + +static int __init tegra_max98090_init(void) +{ + int err; + + err = platform_driver_register(&tegra_max98090_driver); + if (err < 0) + return err; + + fwnode_add_dependency_parser(tegra_max98090_get_dependencies); + + return 0; +} +module_init(tegra_max98090_init); + +static void __exit tegra_max98090_exit(void) +{ + fwnode_remove_dependency_parser(tegra_max98090_get_dependencies); + platform_driver_unregister(&tegra_max98090_driver); +} +module_exit(tegra_max98090_exit);
MODULE_AUTHOR("Stephen Warren swarren@nvidia.com"); MODULE_DESCRIPTION("Tegra max98090 machine ASoC driver");
On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
+static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
struct list_head *deps)+{
- add_dependency(fwnode, "nvidia,i2s-controller", deps);
- add_dependency(fwnode, "nvidia,audio-codec", deps);
+}
Why is this all being open coded in an individual driver (we already know about and manage all these dependencies in the core...)? If we're going to do this I'd expect the interface for specifying DT nodes to the core to be changed to support this.
On 1 July 2015 at 19:38, Mark Brown broonie@kernel.org wrote:
On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
+static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
struct list_head *deps)+{
add_dependency(fwnode, "nvidia,i2s-controller", deps);add_dependency(fwnode, "nvidia,audio-codec", deps);+}
Why is this all being open coded in an individual driver (we already know about and manage all these dependencies in the core...)? If we're going to do this I'd expect the interface for specifying DT nodes to the core to be changed to support this.
Are you thinking of changing drivers to acquire their resources through Arnd's devm_probe (only that the resource table would have to be in struct device_driver)?
https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel
Sounds like lots of fun, but that means that any given machine will get ordered probe only after all the drivers it uses have been moved to the new declarative API.
TBH, that seems really far away.
Regards,
Tomeu
On Mon, Jul 13, 2015 at 02:10:45PM +0200, Tomeu Vizoso wrote:
On 1 July 2015 at 19:38, Mark Brown broonie@kernel.org wrote:
On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
+static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
struct list_head *deps)+{
add_dependency(fwnode, "nvidia,i2s-controller", deps);add_dependency(fwnode, "nvidia,audio-codec", deps);+}
Why is this all being open coded in an individual driver (we already know about and manage all these dependencies in the core...)? If we're going to do this I'd expect the interface for specifying DT nodes to the core to be changed to support this.
Are you thinking of changing drivers to acquire their resources through Arnd's devm_probe (only that the resource table would have to be in struct device_driver)?
No, I'm looking at how we already have all the "did all my dependencies appear" logic in the core based on data provided by the drivers.
On 13 July 2015 at 17:42, Mark Brown broonie@kernel.org wrote:
On Mon, Jul 13, 2015 at 02:10:45PM +0200, Tomeu Vizoso wrote:
On 1 July 2015 at 19:38, Mark Brown broonie@kernel.org wrote:
On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
+static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
struct list_head *deps)+{
add_dependency(fwnode, "nvidia,i2s-controller", deps);add_dependency(fwnode, "nvidia,audio-codec", deps);+}
Why is this all being open coded in an individual driver (we already know about and manage all these dependencies in the core...)? If we're going to do this I'd expect the interface for specifying DT nodes to the core to be changed to support this.
Are you thinking of changing drivers to acquire their resources through Arnd's devm_probe (only that the resource table would have to be in struct device_driver)?
No, I'm looking at how we already have all the "did all my dependencies appear" logic in the core based on data provided by the drivers.
Sorry, but I still don't get what you mean.
Information about dependencies is currently available only after probe() starts executing, and used to decide whether we want to defer the probe.
The goal of this series is to eliminate most or all of the deferred probes by checking that all dependencies are available before probe() is called.
Because currently we only have dependency information after probe() starts executing, we have to make it available earlier. In this particular version, in callbacks that are registered from the initcalls that register subsystems, classes, drivers, etc. Whatever knows how these dependencies are expressed in the firmware data.
I thought you were pointing out that the property names would be duplicated, once in the probe() implementation and also in the implementation of the get_dependencies callback.
A way to consolidate the code and remove that duplication would be having a declarative API for expressing dependencies, which could be used for both fetching dependencies and for preventing deferred probes. That's why I mentioned devm_probe.
Thanks,
Tomeu
On Tue, Jul 14, 2015 at 09:34:22AM +0200, Tomeu Vizoso wrote:
On 13 July 2015 at 17:42, Mark Brown broonie@kernel.org wrote:
No, I'm looking at how we already have all the "did all my dependencies appear" logic in the core based on data provided by the drivers.
Sorry, but I still don't get what you mean.
I'm not sure how I can be clearer here... you're replacing something that is currently pure data with open coding in each device. That seems like a step back in terms of ease of use.
Information about dependencies is currently available only after probe() starts executing, and used to decide whether we want to defer the probe.
The goal of this series is to eliminate most or all of the deferred probes by checking that all dependencies are available before probe() is called.
Right, but the way it does this is by moving code out of the core into the drivers - currently drivers just tell the core what resources to look up and the core then makes sure that they're all present.
I thought you were pointing out that the property names would be duplicated, once in the probe() implementation and also in the implementation of the get_dependencies callback.
Yes, that is another part of issue with this approach - drivers now have to specify things twice, once for this new interface and once for actually looking things up. That doesn't seem awesome and adding the code into the individual drivers and then having to pull it out again when the redundancy is removed is going to be an enormous amount of churn.
A way to consolidate the code and remove that duplication would be having a declarative API for expressing dependencies, which could be used for both fetching dependencies and for preventing deferred probes. That's why I mentioned devm_probe.
Part of what I'm saying here is that in ASoC we already have (at least as far as the individual drivers are concerned) a declarative way of specifying dependencies. This new code should be able to make use of that, if it can't and especially if none of the code can be shared between drivers then that seems like the interface needs another spin.
I've not seen this devm_probe() code but the name sounds worryingly like it might be fixing the wrong problem :/
On 14 July 2015 at 13:07, Mark Brown broonie@kernel.org wrote:
On Tue, Jul 14, 2015 at 09:34:22AM +0200, Tomeu Vizoso wrote:
On 13 July 2015 at 17:42, Mark Brown broonie@kernel.org wrote:
No, I'm looking at how we already have all the "did all my dependencies appear" logic in the core based on data provided by the drivers.
Sorry, but I still don't get what you mean.
I'm not sure how I can be clearer here... you're replacing something that is currently pure data with open coding in each device. That seems like a step back in terms of ease of use.
I could understand that if snd_soc_dai_link had a field with the property name, and the core called of_parse_phandle on it, but currently what I'm duplicating is:
tegra_max98090_dai.cpu_of_node = of_parse_phandle(np, "nvidia,i2s-controller", 0);
with:
add_dependency(fwnode, "nvidia,i2s-controller", deps);
Admittedly, we could add a cpu_fw_property field to snd_soc_dai_link and have the core call of_parse_phandle itself.
But even then, the core doesn't know about a device's snd_soc_dai_link until probe() is called and then it's too late for the purposes of this series.
That's why I mentioned devm_probe, as it would add a common way to specify the data needed to acquire resources in each driver, which could be made available before probe() is called.
From the proof of concept that Arnd sent in
https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel :
struct foo_priv { spinlock_t lock; void __iomem *regs; int irq; struct gpio_desc *gpio; struct dma_chan *rxdma; struct dma_chan *txdma; bool oldstyle_dma; };
/* * this lists all properties we access from the driver. The list * is interpreted by devm_probe() and can be programmatically * verified to match the binding. */ static const struct devm_probe foo_probe_list[] = { DEVM_ALLOC(foo_priv), DEVM_IOMAP(foo_priv, regs, 0, 0), DEVM_PROP_BOOL(foo_priv, oldstyle_dma, "foo,oldstyle-dma"), DEVM_DMA_SLAVE(foo_priv, rxdma, "rx"); DEVM_DMA_SLAVE(foo_priv, txdma, "tx"); DEVM_GPIO(foo_priv, gpio, 0); DEVM_IRQ_NAMED(foo_priv, irq, foo_irq_handler, "fifo", IRQF_SHARED), {}, };
Thanks,
Tomeu
Information about dependencies is currently available only after probe() starts executing, and used to decide whether we want to defer the probe.
The goal of this series is to eliminate most or all of the deferred probes by checking that all dependencies are available before probe() is called.
Right, but the way it does this is by moving code out of the core into the drivers - currently drivers just tell the core what resources to look up and the core then makes sure that they're all present.
I thought you were pointing out that the property names would be duplicated, once in the probe() implementation and also in the implementation of the get_dependencies callback.
Yes, that is another part of issue with this approach - drivers now have to specify things twice, once for this new interface and once for actually looking things up. That doesn't seem awesome and adding the code into the individual drivers and then having to pull it out again when the redundancy is removed is going to be an enormous amount of churn.
A way to consolidate the code and remove that duplication would be having a declarative API for expressing dependencies, which could be used for both fetching dependencies and for preventing deferred probes. That's why I mentioned devm_probe.
Part of what I'm saying here is that in ASoC we already have (at least as far as the individual drivers are concerned) a declarative way of specifying dependencies. This new code should be able to make use of that, if it can't and especially if none of the code can be shared between drivers then that seems like the interface needs another spin.
I've not seen this devm_probe() code but the name sounds worryingly like it might be fixing the wrong problem :/
On Tue, Jul 14, 2015 at 02:47:04PM +0200, Tomeu Vizoso wrote:
On 14 July 2015 at 13:07, Mark Brown broonie@kernel.org wrote:
I'm not sure how I can be clearer here... you're replacing something that is currently pure data with open coding in each device. That seems like a step back in terms of ease of use.
I could understand that if snd_soc_dai_link had a field with the property name, and the core called of_parse_phandle on it, but currently what I'm duplicating is:
tegra_max98090_dai.cpu_of_node = of_parse_phandle(np, "nvidia,i2s-controller", 0);
with:
add_dependency(fwnode, "nvidia,i2s-controller", deps);
Admittedly, we could add a cpu_fw_property field to snd_soc_dai_link and have the core call of_parse_phandle itself.
Yes, we could - that's really what should be happening here. The other bit of this is that we're doing it twice which isn't success.
But even then, the core doesn't know about a device's snd_soc_dai_link until probe() is called and then it's too late for the purposes of this series.
That's not a good reason to encourage bad patterns in drivers. At the very least the drivers should be able to pass the same struct into both places, having to open code the same thing in two places is going to be error prone.
That's why I mentioned devm_probe, as it would add a common way to specify the data needed to acquire resources in each driver, which could be made available before probe() is called.
That does avoid the duplication. However there are issues with the interface for enumerable buses, it doesn't solve the problem where embedded systems need you to power up the device manually prior to the device actually enumerating. If we're doing early resource acquisition we probably want to solve that too.
Before actually probing a device, find out what dependencies it has and do our best to ensure that they are available at this point.
This is accomplished by finding out what platform devices need to be probed and probing them. Non-platform devices will be probed when the closest ancestor that is a platform device is probed.
If any dependencies are still unavailable after that (most probably a missing driver or an error in the HW description from the firmware), we print a nice error message so that people don't have to add a zillion of printks to find out why a device asked for its probe to be deferred.
Dependencies are discovered with the help of the code that is already implementing the specification of the firmware bindings, via the callbacks registered with fwnode_add_dependency_parser().
Currently the dependencies list is discarded but it could be stored for later usage.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com tegra, kernel, usb ---
Changes in v2: - Allocate the list of dependencies and pass it to the function that fills it.
drivers/base/dd.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index a638bbb..c8a1aff 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -25,6 +25,9 @@ #include <linux/async.h> #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> +#include <linux/property.h> +#include <linux/slab.h> +#include <linux/platform_device.h>
#include "base.h" #include "power/power.h" @@ -54,6 +57,140 @@ static LIST_HEAD(deferred_probe_active_list); static struct workqueue_struct *deferred_wq; static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static bool device_is_bound(struct device *dev) +{ + return klist_node_attached(&dev->p->knode_driver); +} + +static int fwnode_match(struct device *dev, void *data) +{ + return dev->fwnode == data; +} + +static bool fwnode_is_bound(struct fwnode_handle *fwnode) +{ + struct device *dev; + + dev = bus_find_device(&platform_bus_type, NULL, fwnode, fwnode_match); + + /* Check whether device is bound or is being probed right now */ + return dev ? dev->driver : false; +} + +static bool fwnode_is_platform(struct fwnode_handle *fwnode) +{ + struct fwnode_handle *parent; + const char *compatible; + int count; + + count = fwnode_property_read_string_array(fwnode, "compatible", NULL, + 0); + + /* The node has to have a compatible string */ + if (!count) + return false; + + /* But it cannot be only simple-bus */ + if ((count == 1) && + !fwnode_property_read_string(fwnode, "compatible", &compatible) && + !strcmp(compatible, "simple-bus")) + return false; + + parent = fwnode_get_parent(fwnode); + + /* Node is immediately below root */ + if (!fwnode_get_parent(parent)) + return true; + + /* If its parent is a simple-bus */ + if (fwnode_is_compatible(parent, "simple-bus")) + return true; + + return false; +} + +static struct fwnode_handle *get_enclosing_platform_dev( + struct fwnode_handle *fwnode) +{ + struct fwnode_handle *iter, *node = NULL; + + for (iter = fwnode; + iter && fwnode_get_parent(iter); + iter = fwnode_get_parent(iter)) { + + /* + * If we already have a platform device and an ancestor is + * already bound, the first is the one we want to probe. + */ + if (node && fwnode_is_bound(iter)) + break; + + if (fwnode_is_platform(iter)) + node = iter; + } + + return node; +} + +static bool check_dependency(struct fwnode_handle *fwnode) +{ + struct fwnode_handle *target; + struct device *dev; + + if (!fwnode) + return true; + + target = get_enclosing_platform_dev(fwnode); + if (!target) + return true; + + dev = bus_find_device(&platform_bus_type, NULL, target, fwnode_match); + if (!dev) { + pr_debug("Couldn't find device for %s\n", + fwnode_get_name(fwnode)); + return false; + } + + /* + * Device is bound or is being probed right now. If we have bad luck + * and the dependency isn't ready when it's needed, deferred probe + * will save us. + */ + if (dev->driver) + return true; + + bus_probe_device(dev); + + /* If the dependency hasn't finished probing, we'll want a warning */ + return device_is_bound(dev); +} + +static void check_dependencies(struct device *dev) +{ + struct fwnode_dependency *dep, *tmp; + LIST_HEAD(deps); + + if (dev->parent && !check_dependency(dev->parent->fwnode)) + pr_debug("Parent '%s' of device '%s' not available\n", + dev_name(dev->parent), dev_name(dev)); + + if (!dev->fwnode) { + pr_debug("Device '%s' doesn't have a fwnode\n", dev_name(dev)); + return; + } + + fwnode_get_dependencies(dev->fwnode, &deps); + + list_for_each_entry_safe(dep, tmp, &deps, dependency) { + if (!check_dependency(dep->fwnode)) + pr_debug("Dependency '%s' not available\n", + fwnode_get_name(dep->fwnode)); + + list_del(&dep->dependency); + kfree(dep); + } +} + /* * deferred_probe_work_func() - Retry probing devices in the active list. */ @@ -287,6 +424,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
dev->driver = drv;
+ check_dependencies(dev); + /* If using pinctrl, bind pins now before probing */ ret = pinctrl_bind_pins(dev); if (ret)
participants (4)
-
Mark Brown -
Rafael J. Wysocki -
Rafael J. Wysocki -
Tomeu Vizoso