[PATCH v2 0/12] Discover and probe dependencies

[PATCH v2 0/12] Discover and probe dependencies

[Tomeu Vizoso]

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

-- 
2.4.1

[PATCH v2 01/12] device: property: delay device-driver matches

[Tomeu Vizoso]

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);
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/12] device: property: delay device-driver matches

[Rafael J. Wysocki]

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);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 01/12] device: property: delay device-driver matches

[Tomeu Vizoso]

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/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/12] device: property: delay device-driver matches

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 01/12] device: property: delay device-driver matches

[Rafael J. Wysocki]

Hi Mark,

On Thu, Jul 16, 2015 at 10:23 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/12] device: property: delay device-driver matches

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2 02/12] device: property: find dependencies of a firmware node

[Tomeu Vizoso]

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,
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node

[Rafael J. Wysocki]

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,
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [alsa-devel] [PATCH v2 02/12] device: property: find dependencies of a firmware node

[Tomeu Vizoso]

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-UUID.pdf

>> +                     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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node

[Rafael J. Wysocki]

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-UUID.pdf

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

[PATCH v2 03/12] string: Introduce strends()

[Tomeu Vizoso]

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);
 
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 04/12] gpio: register dependency parser for firmware nodes

[Tomeu Vizoso]

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 */
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 05/12] gpu: host1x: register dependency parser for firmware nodes

[Tomeu Vizoso]

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);
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 06/12] backlight: Document consumers of backlight nodes

[Tomeu Vizoso]

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>;
+	};
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 07/12] backlight: register dependency parser for firmware nodes

[Tomeu Vizoso]

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;
 }
 
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 08/12] USB: EHCI: register dependency parser for firmware nodes

[Tomeu Vizoso]

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);
 
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 09/12] regulator: register dependency parser for firmware nodes

[Tomeu Vizoso]

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;
 }
 
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/12] regulator: register dependency parser for firmware nodes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2 10/12] pwm: register dependency parser for firmware nodes

[Tomeu Vizoso]

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;
 }
 
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

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");
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Mon, Jul 13, 2015 at 02:10:45PM +0200, Tomeu Vizoso wrote:
> On 1 July 2015 at 19:38, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

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)?
>
>> https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Tue, Jul 14, 2015 at 09:34:22AM +0200, Tomeu Vizoso wrote:
> On 13 July 2015 at 17:42, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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 :/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

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 :/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2 12/12] driver-core: probe dependencies before probing

[Tomeu Vizoso]

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)
-- 
2.4.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel