[PATCH 0/9] of/irq: Defer interrupt reference resolution

[PATCH 0/9] of/irq: Defer interrupt reference resolution

[Thierry Reding]

Hi,

This small series allows interrupt references from the device tree to be
resolved at driver probe time, rather than at device creation time. The
current implementation resolves such references while devices are added
during the call to of_platform_populate(), which happens very early in
the boot process. This causes probe ordering issues, because all devices
that are an interrupt parent for other devices need to have been probed
by that time. This is worked around for primary interrupt controllers by
initializing them using a device tree specific way (of_irq_init()), but
it doesn't work for something like a GPIO controller that is itself a
platform device and an interrupt parent for other devices at the same
time.

Currently such drivers use explicit initcall ordering to force these
chips to be probed earlier than other devices, but that only fixes a
subset of the problematic cases. It doesn't work if the interrupt user
is itself a platform device on the same bus. There are possibly other
cases where it doesn't work either.

This patch series attempts to fix this by not resolving the interrupt
references at device creation time. Instead, some functionality is added
to the driver core to resolve them for each device immediately before it
is probed. Often this is a lot later than the point at which the device
was created, which gives interrupt parents more time and therefore a
better chance of being probed. More importantly, however, it allows the
driver core to detect when an interrupt parent isn't there yet and cause
the device to be queued for deferred probing. After all, resolving probe
ordering issues is one of the primary reason for the introduction of
deferred probing.

Unfortunately the interrupt core code isn't prepared to handle this very
well, so some preparatory work is required.

Patch 1 is a bit of a cleanup. It modifies of_irq_count() to not use the
heavyweight of_irq_to_resource(), which will actually try to create a
mapping. While not usually harmful, it causes a warning during boot if
the interrupt parent hasn't registered an IRQ domain yet. Furthermore it
is much more than the stated intention of the function, which is to
return the number of interrupts that a device node uses.

Patches 2 and 3 introduce two new functions: __irq_create_mapping() and
__irq_create_of_mapping(), which are both equivalent to the non-__ parts
except that they return a negative error code on failure and therefore
allow propagation of a precise error code instead of 0 for all errors.
This is an important prerequisite for subsequent patches. I think that
it would've been nice to not introduced underscore-prefixed variants but
but there are about 130 callers and updating them all would've been
rather messy.

Patch 4 adds an of_irq_get() function, which is irq_of_parse_and_map()
but returns a negative error code on failure instead of 0.

Similarly, __of_irq_to_resource() as introduced in patch 5 is equivalent
to of_irq_to_resource() but returns a negative error code on failure
instead of 0.

Patch 6 uses __of_irq_to_resource() to propagate error code to callers
of the of_irq_to_resource_table() function.

Patch 7 adds functionality to the platform driver code to resolve
interrupt references at probe time. It uses the negative error code of
the of_irq_to_resource_table() function to trigger deferred probing.

Patch 8 implements similar functionality for I2C devices.

Patch 9 serves as an example of the kind of cleanup that can be done
after this series. Obviously this will require quite a bit of retesting
of working setups, but I think that in the long run we're better off
without the kind of explicit probe ordering employed by the gpio-tegra
driver and many others.

Note that I've only implemented this for platform and I2C devices, but
the same can be done for SPI and possibly other subsystems as well.

There is another use-case that I'm aware of for which a similar solution
could be implemented. IOMMUs on SoCs generally need to hook themselves
up to new platform devices. This causes a similar issues as interrupt
resolution and should be fixable by extending the of_platform_probe()
function introduced in patch 7 of this series.

Thierry

Thierry Reding (9):
  of/irq: Rework of_irq_count()
  irqdomain: Introduce __irq_create_mapping()
  irqdomain: Introduce __irq_create_of_mapping()
  of/irq: Introduce of_irq_get()
  of/irq: Introduce __of_irq_to_resource()
  of/irq: Propagate errors in of_irq_to_resource_table()
  of/platform: Resolve interrupt references at probe time
  of/i2c: Resolve interrupt references at probe time
  gpio: tegra: Use module_platform_driver()

 drivers/base/platform.c     |  4 ++
 drivers/gpio/gpio-tegra.c   |  7 +---
 drivers/i2c/i2c-core.c      | 23 ++++++++++-
 drivers/of/irq.c            | 69 +++++++++++++++++++++++--------
 drivers/of/platform.c       | 43 +++++++++++++++++---
 include/linux/of_irq.h      |  6 +++
 include/linux/of_platform.h |  7 ++++
 kernel/irq/irqdomain.c      | 98 +++++++++++++++++++++++++++++++--------------
 8 files changed, 197 insertions(+), 60 deletions(-)

-- 
1.8.4

[PATCH 1/9] of/irq: Rework of_irq_count()

[Thierry Reding]

The of_irq_to_resource() helper that is used to implement of_irq_count()
tries to resolve interrupts and in fact creates a mapping for resolved
interrupts. That's pretty heavy lifting for something that claims to
just return the number of interrupts requested by a given device node.

Instead, use the more lightweight of_irq_map_one(), which, despite the
name, doesn't create an actual mapping. Perhaps a better name would be
of_irq_translate_one().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1752988..5f44388 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -368,9 +368,10 @@ EXPORT_SYMBOL_GPL(of_irq_to_resource);
  */
 int of_irq_count(struct device_node *dev)
 {
+	struct of_irq irq;
 	int nr = 0;
 
-	while (of_irq_to_resource(dev, nr, NULL))
+	while (of_irq_map_one(dev, nr, &irq) == 0)
 		nr++;
 
 	return nr;
-- 
1.8.4

[PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Thierry Reding]

This is a version of irq_create_mapping() that propagates the precise
error code instead of returning 0 for all errors. It will be used in
subsequent patches to allow further propagation of error codes.

To avoid code duplication, implement irq_create_mapping() as a wrapper
around the new __irq_create_mapping().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 kernel/irq/irqdomain.c | 59 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 706724e..d2a3b01 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -374,30 +374,21 @@ unsigned int irq_create_direct_mapping(struct irq_domain *domain)
 }
 EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
 
-/**
- * irq_create_mapping() - Map a hardware interrupt into linux irq space
- * @domain: domain owning this hardware interrupt or NULL for default domain
- * @hwirq: hardware irq number in that domain space
- *
- * Only one mapping per hardware interrupt is permitted. Returns a linux
- * irq number.
- * If the sense/trigger is to be specified, set_irq_type() should be called
- * on the number returned from that call.
- */
-unsigned int irq_create_mapping(struct irq_domain *domain,
-				irq_hw_number_t hwirq)
+static int __irq_create_mapping(struct irq_domain *domain,
+				irq_hw_number_t hwirq, unsigned int *virqp)
 {
-	unsigned int hint;
-	int virq;
+	unsigned int hint, virq;
+	int ret;
 
-	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
+	pr_debug("__irq_create_mapping(0x%p, 0x%lx, %p)\n", domain, hwirq,
+		 virqp);
 
 	/* Look for default domain if nececssary */
 	if (domain == NULL)
 		domain = irq_default_domain;
 	if (domain == NULL) {
 		WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
-		return 0;
+		return -ENODEV;
 	}
 	pr_debug("-> using domain @%p\n", domain);
 
@@ -405,7 +396,11 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 	virq = irq_find_mapping(domain, hwirq);
 	if (virq) {
 		pr_debug("-> existing mapping on virq %d\n", virq);
-		return virq;
+
+		if (virqp)
+			*virqp = virq;
+
+		return 0;
 	}
 
 	/* Allocate a virtual interrupt number */
@@ -417,17 +412,41 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 		virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node));
 	if (virq <= 0) {
 		pr_debug("-> virq allocation failed\n");
-		return 0;
+		return virq ? : -ENOSPC;
 	}
 
-	if (irq_domain_associate(domain, virq, hwirq)) {
+	ret = irq_domain_associate(domain, virq, hwirq);
+	if (ret) {
 		irq_free_desc(virq);
-		return 0;
+		return ret;
 	}
 
 	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
 		hwirq, of_node_full_name(domain->of_node), virq);
 
+	if (virqp)
+		*virqp = virq;
+
+	return 0;
+}
+/**
+ * irq_create_mapping() - Map a hardware interrupt into linux irq space
+ * @domain: domain owning this hardware interrupt or NULL for default domain
+ * @hwirq: hardware irq number in that domain space
+ *
+ * Only one mapping per hardware interrupt is permitted. Returns a linux
+ * irq number.
+ * If the sense/trigger is to be specified, set_irq_type() should be called
+ * on the number returned from that call.
+ */
+unsigned int irq_create_mapping(struct irq_domain *domain,
+				irq_hw_number_t hwirq)
+{
+	unsigned int virq;
+
+	if (__irq_create_mapping(domain, hwirq, &virq))
+		return 0;
+
 	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_mapping);
-- 
1.8.4

[PATCH 3/9] irqdomain: Introduce __irq_create_of_mapping()

[Thierry Reding]

This is a version of irq_create_of_mapping() that propagates the precise
error code instead of returning 0 for all errors. It will be used in
subsequent patches to allow further propagation of error codes.

To avoid code duplication, implement irq_create_of_mapping() as a
wrapper around the new __irq_create_of_mapping().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/linux/of_irq.h |  3 +++
 kernel/irq/irqdomain.c | 39 +++++++++++++++++++++++++++++----------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 535cecf..c383dd1 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -63,6 +63,9 @@ extern int of_irq_map_raw(struct device_node *parent, const __be32 *intspec,
 			  struct of_irq *out_irq);
 extern int of_irq_map_one(struct device_node *device, int index,
 			  struct of_irq *out_irq);
+extern int __irq_create_of_mapping(struct device_node *controller,
+				   const u32 *intspec, unsigned int intsize,
+				   unsigned int *virqp);
 extern unsigned int irq_create_of_mapping(struct device_node *controller,
 					  const u32 *intspec,
 					  unsigned int intsize);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index d2a3b01..9867616 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -484,39 +484,58 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
 }
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
 
-unsigned int irq_create_of_mapping(struct device_node *controller,
-				   const u32 *intspec, unsigned int intsize)
+int __irq_create_of_mapping(struct device_node *controller, const u32 *intspec,
+			    unsigned int intsize, unsigned int *virqp)
 {
+	unsigned int type = IRQ_TYPE_NONE;
 	struct irq_domain *domain;
 	irq_hw_number_t hwirq;
-	unsigned int type = IRQ_TYPE_NONE;
 	unsigned int virq;
+	int ret;
 
 	domain = controller ? irq_find_host(controller) : irq_default_domain;
 	if (!domain) {
 		pr_warn("no irq domain found for %s !\n",
 			of_node_full_name(controller));
-		return 0;
+		return -EPROBE_DEFER;
 	}
 
 	/* If domain has no translation, then we assume interrupt line */
 	if (domain->ops->xlate == NULL)
 		hwirq = intspec[0];
 	else {
-		if (domain->ops->xlate(domain, controller, intspec, intsize,
-				     &hwirq, &type))
-			return 0;
+		ret = domain->ops->xlate(domain, controller, intspec, intsize,
+					 &hwirq, &type);
+		if (ret)
+			return ret;
 	}
 
 	/* Create mapping */
-	virq = irq_create_mapping(domain, hwirq);
-	if (!virq)
-		return virq;
+	ret = __irq_create_mapping(domain, hwirq, &virq);
+	if (ret)
+		return ret;
 
 	/* Set type if specified and different than the current one */
 	if (type != IRQ_TYPE_NONE &&
 	    type != irq_get_trigger_type(virq))
 		irq_set_irq_type(virq, type);
+
+	if (virqp)
+		*virqp = virq;
+
+	return 0;
+}
+
+unsigned int irq_create_of_mapping(struct device_node *controller,
+				   const u32 *intspec, unsigned int intsize)
+{
+	unsigned int virq;
+	int ret;
+
+	ret = __irq_create_of_mapping(controller, intspec, intsize, &virq);
+	if (ret)
+		return 0;
+
 	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_of_mapping);
-- 
1.8.4

[PATCH 4/9] of/irq: Introduce of_irq_get()

[Thierry Reding]

This is a version of irq_of_parse_and_map() that propagates the precise
error code instead of returning 0 for all errors. It will be used in
subsequent patches to allow further propagation of error codes.

To avoid code duplication, implement irq_of_parse_and_map() as a wrapper
around the new of_irq_get().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/irq.c       | 21 +++++++++++++++++----
 include/linux/of_irq.h |  3 +++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 5f44388..8225289 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -26,6 +26,20 @@
 #include <linux/string.h>
 #include <linux/slab.h>
 
+int of_irq_get(struct device_node *dev, unsigned int index, unsigned int *virqp)
+{
+	struct of_irq oirq;
+	int ret;
+
+	ret = of_irq_map_one(dev, index, &oirq);
+	if (ret)
+		return ret;
+
+	return __irq_create_of_mapping(oirq.controller, oirq.specifier,
+				       oirq.size, virqp);
+}
+EXPORT_SYMBOL_GPL(of_irq_get);
+
 /**
  * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
  * @dev: Device node of the device whose interrupt is to be mapped
@@ -36,13 +50,12 @@
  */
 unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 {
-	struct of_irq oirq;
+	unsigned int virq;
 
-	if (of_irq_map_one(dev, index, &oirq))
+	if (of_irq_get(dev, index, &virq))
 		return 0;
 
-	return irq_create_of_mapping(oirq.controller, oirq.specifier,
-				     oirq.size);
+	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index c383dd1..cac15d7 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -17,6 +17,9 @@ struct of_irq;
  */
 extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
 
+extern int of_irq_get(struct device_node *node, unsigned int index,
+		      unsigned int *virqp);
+
 #if defined(CONFIG_OF_IRQ)
 /**
  * of_irq - container for device_node/irq_specifier pair for an irq controller
-- 
1.8.4

[PATCH 5/9] of/irq: Introduce __of_irq_to_resource()

[Thierry Reding]

This is a version of of_irq_to_resource() that propagates the precise
error code instead of returning 0 for all errors. It will be used in
subsequent patches to allow further propagation of error codes.

To avoid code duplication, implement of_irq_to_resource() as a wrapper
around the new __of_irq_to_resource().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/irq.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 8225289..57396fd 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -343,15 +343,15 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq
 }
 EXPORT_SYMBOL_GPL(of_irq_map_one);
 
-/**
- * of_irq_to_resource - Decode a node's IRQ and return it as a resource
- * @dev: pointer to device tree node
- * @index: zero-based index of the irq
- * @r: pointer to resource structure to return result into.
- */
-int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
+static int __of_irq_to_resource(struct device_node *dev, unsigned int index,
+				struct resource *r)
 {
-	int irq = irq_of_parse_and_map(dev, index);
+	unsigned int irq;
+	int ret;
+
+	ret = of_irq_get(dev, index, &irq);
+	if (ret)
+		return ret;
 
 	/* Only dereference the resource if both the
 	 * resource and the irq are valid. */
@@ -373,6 +373,23 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 
 	return irq;
 }
+
+/**
+ * of_irq_to_resource - Decode a node's IRQ and return it as a resource
+ * @dev: pointer to device tree node
+ * @index: zero-based index of the irq
+ * @r: pointer to resource structure to return result into.
+ */
+int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
+{
+	int ret;
+
+	ret = __of_irq_to_resource(dev, index, r);
+	if (ret < 0)
+		return 0;
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(of_irq_to_resource);
 
 /**
-- 
1.8.4

[PATCH 6/9] of/irq: Propagate errors in of_irq_to_resource_table()

[Thierry Reding]

Now that all helpers return precise error codes, this function can
propagate these errors to the caller properly.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/irq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 57396fd..c33a7fd 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -418,11 +418,17 @@ int of_irq_count(struct device_node *dev)
 int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
 		int nr_irqs)
 {
-	int i;
+	int i, ret;
+
+	for (i = 0; i < nr_irqs; i++, res++) {
+		ret = __of_irq_to_resource(dev, i, res);
+		if (ret <= 0) {
+			if (ret < 0)
+				return ret;
 
-	for (i = 0; i < nr_irqs; i++, res++)
-		if (!of_irq_to_resource(dev, i, res))
 			break;
+		}
+	}
 
 	return i;
 }
-- 
1.8.4

[PATCH 7/9] of/platform: Resolve interrupt references at probe time

[Thierry Reding]

Interrupt references are currently resolved very early (when a device is
created). This has the disadvantage that it will fail in cases where the
interrupt parent hasn't been probed and no IRQ domain for it has been
registered yet. To work around that various drivers use explicit
initcall ordering to force interrupt parents to be probed before devices
that need them are created. That's error prone and doesn't always work.
If a platform device uses an interrupt line connected to a different
platform device (such as a GPIO controller), both will be created in the
same batch, and the GPIO controller won't have been probed by its driver
when the depending platform device is created. Interrupt resolution will
fail in that case.

Another common workaround is for drivers to explicitly resolve interrupt
references at probe time. This is suboptimal, however, because it will
require every driver to duplicate the code.

This patch adds support for late interrupt resolution to the platform
driver core, by resolving the references right before a device driver's
.probe() function will be called. This not only delays the resolution
until a much later time (giving interrupt parents a better chance of
being probed in the meantime), but it also allows the platform driver
core to queue the device for deferred probing if the interrupt parent
hasn't registered its IRQ domain yet.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/base/platform.c     |  4 ++++
 drivers/of/platform.c       | 43 +++++++++++++++++++++++++++++++++++++------
 include/linux/of_platform.h |  7 +++++++
 3 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4f8bef3..8dcf835 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
 	struct platform_device *dev = to_platform_device(_dev);
 	int ret;
 
+	ret = of_platform_probe(dev);
+	if (ret)
+		return ret;
+
 	if (ACPI_HANDLE(_dev))
 		acpi_dev_pm_attach(_dev, true);
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b439ac..edaef70 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -142,7 +142,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
 				  struct device *parent)
 {
 	struct platform_device *dev;
-	int rc, i, num_reg = 0, num_irq;
+	int rc, i, num_reg = 0;
 	struct resource *res, temp_res;
 
 	dev = platform_device_alloc("", -1);
@@ -153,23 +153,21 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	if (of_can_translate_address(np))
 		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
 			num_reg++;
-	num_irq = of_irq_count(np);
 
 	/* Populate the resource table */
-	if (num_irq || num_reg) {
-		res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
+	if (num_reg) {
+		res = kzalloc(sizeof(*res) * num_reg, GFP_KERNEL);
 		if (!res) {
 			platform_device_put(dev);
 			return NULL;
 		}
 
-		dev->num_resources = num_reg + num_irq;
+		dev->num_resources = num_reg;
 		dev->resource = res;
 		for (i = 0; i < num_reg; i++, res++) {
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
-		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
 	}
 
 	dev->dev.of_node = of_node_get(np);
@@ -490,4 +488,37 @@ int of_platform_populate(struct device_node *root,
 	return rc;
 }
 EXPORT_SYMBOL_GPL(of_platform_populate);
+
+int of_platform_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int num_irq, ret = 0;
+
+	if (!pdev->dev.of_node)
+		return 0;
+
+	num_irq = of_irq_count(pdev->dev.of_node);
+	if (num_irq > 0) {
+		struct resource *res = pdev->resource;
+		int num_reg = pdev->num_resources;
+		int num = num_reg + num_irq;
+
+		res = krealloc(res, num * sizeof(*res), GFP_KERNEL);
+		if (!res)
+			return -ENOMEM;
+
+		pdev->num_resources = num;
+		pdev->resource = res;
+		res += num_reg;
+
+		ret = of_irq_to_resource_table(np, res, num_irq);
+		if (ret < 0)
+			return ret;
+
+		WARN_ON(ret != num_irq);
+		ret = 0;
+	}
+
+	return ret;
+}
 #endif /* CONFIG_OF_ADDRESS */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 05cb4a9..92fc4f6 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				const struct of_dev_auxdata *lookup,
 				struct device *parent);
+
+extern int of_platform_probe(struct platform_device *pdev);
 #else
 static inline int of_platform_populate(struct device_node *root,
 					const struct of_device_id *matches,
@@ -80,6 +82,11 @@ static inline int of_platform_populate(struct device_node *root,
 {
 	return -ENODEV;
 }
+
+static inline int of_platform_probe(struct platform_device *pdev)
+{
+	return 0;
+}
 #endif
 
 #endif	/* _LINUX_OF_PLATFORM_H */
-- 
1.8.4

[PATCH 8/9] of/i2c: Resolve interrupt references at probe time

[Thierry Reding]

Instead of resolving interrupt references at device creation time, delay
resolution until probe time. At device creation time, there is nothing
that can be done if an interrupt parent isn't ready yet, and the device
will end up with an invalid interrupt number (0).

If the interrupt reference is resolved at probe time, the device's probe
can be deferred, so that it's interrupt resolution can be retried after
more devices (possibly including its interrupt parent) have been probed.

However, individual drivers shouldn't be required to do that themselves,
over and over again, so this commit implements this functionality within
the I2C core.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/i2c-core.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..163a1e8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -236,6 +236,21 @@ int i2c_recover_bus(struct i2c_adapter *adap)
 	return adap->bus_recovery_info->recover_bus(adap);
 }
 
+static int of_i2c_probe(struct i2c_client *client)
+{
+	struct device_node *np = client->dev.of_node;
+	int ret;
+
+	/* skip if the device node specifies no interrupts */
+	if (of_get_property(np, "interrupts", NULL)) {
+		ret = of_irq_get(client->dev.of_node, 0, &client->irq);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -254,6 +269,12 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+		status = of_i2c_probe(client);
+		if (status)
+			return status;
+	}
+
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
 	if (status) {
 		client->driver = NULL;
@@ -1002,7 +1023,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 			continue;
 		}
 
-		info.irq = irq_of_parse_and_map(node, 0);
 		info.of_node = of_node_get(node);
 		info.archdata = &dev_ad;
 
@@ -1016,7 +1036,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 			dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
 				node->full_name);
 			of_node_put(node);
-			irq_dispose_mapping(info.irq);
 			continue;
 		}
 	}
-- 
1.8.4

[PATCH 9/9] gpio: tegra: Use module_platform_driver()

[Thierry Reding]

With the driver core now resolving interrupt references at probe time,
it is no longer necessary to force explicit probe ordering using
initcalls.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Note that there are potentially many more drivers that can be switched
to the generic module_*_driver() interfaces now that interrupts can be
resolved later and deferred probe should be able to handle all the
ordering issues.
---
 drivers/gpio/gpio-tegra.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 9a62672..766e6ef 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -513,12 +513,7 @@ static struct platform_driver tegra_gpio_driver = {
 	},
 	.probe		= tegra_gpio_probe,
 };
-
-static int __init tegra_gpio_init(void)
-{
-	return platform_driver_register(&tegra_gpio_driver);
-}
-postcore_initcall(tegra_gpio_init);
+module_platform_driver(tegra_gpio_driver);
 
 #ifdef	CONFIG_DEBUG_FS
 
-- 
1.8.4

Re: [PATCH 3/9] irqdomain: Introduce __irq_create_of_mapping()

[Rob Herring]

On 09/16/2013 03:32 AM, Thierry Reding wrote:
> This is a version of irq_create_of_mapping() that propagates the precise
> error code instead of returning 0 for all errors. It will be used in
> subsequent patches to allow further propagation of error codes.
> 
> To avoid code duplication, implement irq_create_of_mapping() as a
> wrapper around the new __irq_create_of_mapping().

This function is a manageable number of callers that the callers should
just be updated and avoid the wrapper.

Rob

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/linux/of_irq.h |  3 +++
>  kernel/irq/irqdomain.c | 39 +++++++++++++++++++++++++++++----------
>  2 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 535cecf..c383dd1 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -63,6 +63,9 @@ extern int of_irq_map_raw(struct device_node *parent, const __be32 *intspec,
>  			  struct of_irq *out_irq);
>  extern int of_irq_map_one(struct device_node *device, int index,
>  			  struct of_irq *out_irq);
> +extern int __irq_create_of_mapping(struct device_node *controller,
> +				   const u32 *intspec, unsigned int intsize,
> +				   unsigned int *virqp);
>  extern unsigned int irq_create_of_mapping(struct device_node *controller,
>  					  const u32 *intspec,
>  					  unsigned int intsize);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index d2a3b01..9867616 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -484,39 +484,58 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
>  }
>  EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
>  
> -unsigned int irq_create_of_mapping(struct device_node *controller,
> -				   const u32 *intspec, unsigned int intsize)
> +int __irq_create_of_mapping(struct device_node *controller, const u32 *intspec,
> +			    unsigned int intsize, unsigned int *virqp)
>  {
> +	unsigned int type = IRQ_TYPE_NONE;
>  	struct irq_domain *domain;
>  	irq_hw_number_t hwirq;
> -	unsigned int type = IRQ_TYPE_NONE;
>  	unsigned int virq;
> +	int ret;
>  
>  	domain = controller ? irq_find_host(controller) : irq_default_domain;
>  	if (!domain) {
>  		pr_warn("no irq domain found for %s !\n",
>  			of_node_full_name(controller));
> -		return 0;
> +		return -EPROBE_DEFER;
>  	}
>  
>  	/* If domain has no translation, then we assume interrupt line */
>  	if (domain->ops->xlate == NULL)
>  		hwirq = intspec[0];
>  	else {
> -		if (domain->ops->xlate(domain, controller, intspec, intsize,
> -				     &hwirq, &type))
> -			return 0;
> +		ret = domain->ops->xlate(domain, controller, intspec, intsize,
> +					 &hwirq, &type);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	/* Create mapping */
> -	virq = irq_create_mapping(domain, hwirq);
> -	if (!virq)
> -		return virq;
> +	ret = __irq_create_mapping(domain, hwirq, &virq);
> +	if (ret)
> +		return ret;
>  
>  	/* Set type if specified and different than the current one */
>  	if (type != IRQ_TYPE_NONE &&
>  	    type != irq_get_trigger_type(virq))
>  		irq_set_irq_type(virq, type);
> +
> +	if (virqp)
> +		*virqp = virq;
> +
> +	return 0;
> +}
> +
> +unsigned int irq_create_of_mapping(struct device_node *controller,
> +				   const u32 *intspec, unsigned int intsize)
> +{
> +	unsigned int virq;
> +	int ret;
> +
> +	ret = __irq_create_of_mapping(controller, intspec, intsize, &virq);
> +	if (ret)
> +		return 0;
> +
>  	return virq;
>  }
>  EXPORT_SYMBOL_GPL(irq_create_of_mapping);
> 

Re: [PATCH 4/9] of/irq: Introduce of_irq_get()

[Rob Herring]

On 09/16/2013 03:32 AM, Thierry Reding wrote:
> This is a version of irq_of_parse_and_map() that propagates the precise
> error code instead of returning 0 for all errors. It will be used in
> subsequent patches to allow further propagation of error codes.
> 
> To avoid code duplication, implement irq_of_parse_and_map() as a wrapper
> around the new of_irq_get().

*_get typically also implies some reference counting which I don't
believe this does. I don't think having 2 functions with completely
different names doing the same thing with only a different calling
convention is good either. So I would keep the old name and the names
aligned.

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/of/irq.c       | 21 +++++++++++++++++----
>  include/linux/of_irq.h |  3 +++
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 5f44388..8225289 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -26,6 +26,20 @@
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  
> +int of_irq_get(struct device_node *dev, unsigned int index, unsigned int *virqp)
> +{
> +	struct of_irq oirq;
> +	int ret;
> +
> +	ret = of_irq_map_one(dev, index, &oirq);
> +	if (ret)
> +		return ret;
> +
> +	return __irq_create_of_mapping(oirq.controller, oirq.specifier,
> +				       oirq.size, virqp);
> +}
> +EXPORT_SYMBOL_GPL(of_irq_get);
> +
>  /**
>   * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
>   * @dev: Device node of the device whose interrupt is to be mapped
> @@ -36,13 +50,12 @@
>   */
>  unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
>  {
> -	struct of_irq oirq;
> +	unsigned int virq;
>  
> -	if (of_irq_map_one(dev, index, &oirq))
> +	if (of_irq_get(dev, index, &virq))
>  		return 0;
>  
> -	return irq_create_of_mapping(oirq.controller, oirq.specifier,
> -				     oirq.size);
> +	return virq;

This can be an inline and written more concisely:

{
	unsigned int virq;
	return (of_irq_get(dev, index, &virq) < 0) ? 0 : virq;
}

Rob

Re: [PATCH 5/9] of/irq: Introduce __of_irq_to_resource()

[Rob Herring]

On 09/16/2013 03:32 AM, Thierry Reding wrote:
> This is a version of of_irq_to_resource() that propagates the precise
> error code instead of returning 0 for all errors. It will be used in
> subsequent patches to allow further propagation of error codes.
> 
> To avoid code duplication, implement of_irq_to_resource() as a wrapper
> around the new __of_irq_to_resource().

I think the callers in this case are manageable to update as well.
Several cases could simply use irq_of_parse_and_map instead as they just
pass in a NULL resource.

Rob

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/of/irq.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 8225289..57396fd 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -343,15 +343,15 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq
>  }
>  EXPORT_SYMBOL_GPL(of_irq_map_one);
>  
> -/**
> - * of_irq_to_resource - Decode a node's IRQ and return it as a resource
> - * @dev: pointer to device tree node
> - * @index: zero-based index of the irq
> - * @r: pointer to resource structure to return result into.
> - */
> -int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
> +static int __of_irq_to_resource(struct device_node *dev, unsigned int index,
> +				struct resource *r)
>  {
> -	int irq = irq_of_parse_and_map(dev, index);
> +	unsigned int irq;
> +	int ret;
> +
> +	ret = of_irq_get(dev, index, &irq);
> +	if (ret)
> +		return ret;
>  
>  	/* Only dereference the resource if both the
>  	 * resource and the irq are valid. */
> @@ -373,6 +373,23 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>  
>  	return irq;
>  }
> +
> +/**
> + * of_irq_to_resource - Decode a node's IRQ and return it as a resource
> + * @dev: pointer to device tree node
> + * @index: zero-based index of the irq
> + * @r: pointer to resource structure to return result into.
> + */
> +int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
> +{
> +	int ret;
> +
> +	ret = __of_irq_to_resource(dev, index, r);
> +	if (ret < 0)
> +		return 0;
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(of_irq_to_resource);
>  
>  /**
> 

Re: [PATCH 3/9] irqdomain: Introduce __irq_create_of_mapping()

[Thierry Reding]

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

On Mon, Sep 16, 2013 at 04:17:28PM -0500, Rob Herring wrote:
> On 09/16/2013 03:32 AM, Thierry Reding wrote:
> > This is a version of irq_create_of_mapping() that propagates the precise
> > error code instead of returning 0 for all errors. It will be used in
> > subsequent patches to allow further propagation of error codes.
> > 
> > To avoid code duplication, implement irq_create_of_mapping() as a
> > wrapper around the new __irq_create_of_mapping().
> 
> This function is a manageable number of callers that the callers should
> just be updated and avoid the wrapper.

Okay, I can do that. Can I take this as a sign that you're generally
okay with the idea behind the series?

Thierry

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

Re: [PATCH 0/9] of/irq: Defer interrupt reference resolution

[Alexandre Belloni]

Hi Thierry,

On 16/09/2013 10:31, Thierry Reding wrote:
> Hi,
>
> This small series allows interrupt references from the device tree to be
> resolved at driver probe time, rather than at device creation time. The
> current implementation resolves such references while devices are added
> during the call to of_platform_populate(), which happens very early in
> the boot process. This causes probe ordering issues, because all devices
> that are an interrupt parent for other devices need to have been probed
> by that time. This is worked around for primary interrupt controllers by
> initializing them using a device tree specific way (of_irq_init()), but
> it doesn't work for something like a GPIO controller that is itself a
> platform device and an interrupt parent for other devices at the same
> time.
>
> Currently such drivers use explicit initcall ordering to force these
> chips to be probed earlier than other devices, but that only fixes a
> subset of the problematic cases. It doesn't work if the interrupt user
> is itself a platform device on the same bus. There are possibly other
> cases where it doesn't work either.
>
> This patch series attempts to fix this by not resolving the interrupt
> references at device creation time. Instead, some functionality is added
> to the driver core to resolve them for each device immediately before it
> is probed. Often this is a lot later than the point at which the device
> was created, which gives interrupt parents more time and therefore a
> better chance of being probed. More importantly, however, it allows the
> driver core to detect when an interrupt parent isn't there yet and cause
> the device to be queued for deferred probing. After all, resolving probe
> ordering issues is one of the primary reason for the introduction of
> deferred probing.
>
> Unfortunately the interrupt core code isn't prepared to handle this very
> well, so some preparatory work is required.
>
> Patch 1 is a bit of a cleanup. It modifies of_irq_count() to not use the
> heavyweight of_irq_to_resource(), which will actually try to create a
> mapping. While not usually harmful, it causes a warning during boot if
> the interrupt parent hasn't registered an IRQ domain yet. Furthermore it
> is much more than the stated intention of the function, which is to
> return the number of interrupts that a device node uses.
>
> Patches 2 and 3 introduce two new functions: __irq_create_mapping() and
> __irq_create_of_mapping(), which are both equivalent to the non-__ parts
> except that they return a negative error code on failure and therefore
> allow propagation of a precise error code instead of 0 for all errors.
> This is an important prerequisite for subsequent patches. I think that
> it would've been nice to not introduced underscore-prefixed variants but
> but there are about 130 callers and updating them all would've been
> rather messy.
>
> Patch 4 adds an of_irq_get() function, which is irq_of_parse_and_map()
> but returns a negative error code on failure instead of 0.
>
> Similarly, __of_irq_to_resource() as introduced in patch 5 is equivalent
> to of_irq_to_resource() but returns a negative error code on failure
> instead of 0.
>
> Patch 6 uses __of_irq_to_resource() to propagate error code to callers
> of the of_irq_to_resource_table() function.
>
> Patch 7 adds functionality to the platform driver code to resolve
> interrupt references at probe time. It uses the negative error code of
> the of_irq_to_resource_table() function to trigger deferred probing.
>
> Patch 8 implements similar functionality for I2C devices.
>
> Patch 9 serves as an example of the kind of cleanup that can be done
> after this series. Obviously this will require quite a bit of retesting
> of working setups, but I think that in the long run we're better off
> without the kind of explicit probe ordering employed by the gpio-tegra
> driver and many others.
>
> Note that I've only implemented this for platform and I2C devices, but
> the same can be done for SPI and possibly other subsystems as well.
>
> There is another use-case that I'm aware of for which a similar solution
> could be implemented. IOMMUs on SoCs generally need to hook themselves
> up to new platform devices. This causes a similar issues as interrupt
> resolution and should be fixable by extending the of_platform_probe()
> function introduced in patch 7 of this series.

I believe this will solve the issue I was hitting back in June where
of_i2c_register_devices() failed to get the IRQ while doing it at probe
time was working fine:

http://www.spinics.net/lists/linux-i2c/msg12523.html

I couldn't test your patches yet though. I'll try to test as soon as I
get some free time.



-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/9] of/irq: Defer interrupt reference resolution

[Thierry Reding]

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

On Tue, Sep 17, 2013 at 01:20:39PM +0200, Alexandre Belloni wrote:
> Hi Thierry,
> 
> On 16/09/2013 10:31, Thierry Reding wrote:
> > Hi,
> >
> > This small series allows interrupt references from the device tree to be
> > resolved at driver probe time, rather than at device creation time. The
> > current implementation resolves such references while devices are added
> > during the call to of_platform_populate(), which happens very early in
> > the boot process. This causes probe ordering issues, because all devices
> > that are an interrupt parent for other devices need to have been probed
> > by that time. This is worked around for primary interrupt controllers by
> > initializing them using a device tree specific way (of_irq_init()), but
> > it doesn't work for something like a GPIO controller that is itself a
> > platform device and an interrupt parent for other devices at the same
> > time.
[...]
> I believe this will solve the issue I was hitting back in June where
> of_i2c_register_devices() failed to get the IRQ while doing it at probe
> time was working fine:
> 
> http://www.spinics.net/lists/linux-i2c/msg12523.html
> 
> I couldn't test your patches yet though. I'll try to test as soon as I
> get some free time.

Yes, this series precisely aims at fixing situations as those. It'd be
great if you could give the series a spin to verify that it really works
for your case.

Thierry

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

RE: [PATCH 7/9] of/platform: Resolve interrupt references at probe time

[Strashko, Grygorii]

Hi Thierry,

On 09/16/2013 11:32 AM, Thierry Reding wrote:> Interrupt references are currently resolved very early (when a device is
> created). This has the disadvantage that it will fail in cases where the
> interrupt parent hasn't been probed and no IRQ domain for it has been
> registered yet. To work around that various drivers use explicit
> initcall ordering to force interrupt parents to be probed before devices
> that need them are created. That's error prone and doesn't always work.
> If a platform device uses an interrupt line connected to a different
> platform device (such as a GPIO controller), both will be created in the
> same batch, and the GPIO controller won't have been probed by its driver
> when the depending platform device is created. Interrupt resolution will
> fail in that case.
> 
> Another common workaround is for drivers to explicitly resolve interrupt
> references at probe time. This is suboptimal, however, because it will
> require every driver to duplicate the code.
> 
> This patch adds support for late interrupt resolution to the platform
> driver core, by resolving the references right before a device driver's
> .probe() function will be called. This not only delays the resolution
> until a much later time (giving interrupt parents a better chance of
> being probed in the meantime), but it also allows the platform driver
> core to queue the device for deferred probing if the interrupt parent
> hasn't registered its IRQ domain yet.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/base/platform.c     |  4 ++++
>   drivers/of/platform.c       | 43 +++++++++++++++++++++++++++++++++++++------
>   include/linux/of_platform.h |  7 +++++++
>   3 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4f8bef3..8dcf835 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)

Should it be the part of really_probe()? Isn't it?

>   	struct platform_device *dev = to_platform_device(_dev);
>   	int ret;
>   
> +	ret = of_platform_probe(dev);
> +	if (ret)
> +		return ret;
> +
>   	if (ACPI_HANDLE(_dev))
>   		acpi_dev_pm_attach(_dev, true);
>   
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 9b439ac..edaef70 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -142,7 +142,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
>   				  struct device *parent)
>   {
>   	struct platform_device *dev;
> -	int rc, i, num_reg = 0, num_irq;
> +	int rc, i, num_reg = 0;
>   	struct resource *res, temp_res;
>   
>   	dev = platform_device_alloc("", -1);
> @@ -153,23 +153,21 @@ struct platform_device *of_device_alloc(struct device_node *np,
>   	if (of_can_translate_address(np))
>   		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>   			num_reg++;
> -	num_irq = of_irq_count(np);
>   
>   	/* Populate the resource table */
> -	if (num_irq || num_reg) {
> -		res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> +	if (num_reg) {
> +		res = kzalloc(sizeof(*res) * num_reg, GFP_KERNEL);
>   		if (!res) {
>   			platform_device_put(dev);
>   			return NULL;
>   		}
>   
> -		dev->num_resources = num_reg + num_irq;
> +		dev->num_resources = num_reg;
>   		dev->resource = res;
>   		for (i = 0; i < num_reg; i++, res++) {
>   			rc = of_address_to_resource(np, i, res);
>   			WARN_ON(rc);
>   		}
> -		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>   	}
>   
>   	dev->dev.of_node = of_node_get(np);
> @@ -490,4 +488,37 @@ int of_platform_populate(struct device_node *root,
>   	return rc;
>   }
>   EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +int of_platform_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int num_irq, ret = 0;
> +
> +	if (!pdev->dev.of_node)
> +		return 0;
> +
> +	num_irq = of_irq_count(pdev->dev.of_node);
> +	if (num_irq > 0) {
> +		struct resource *res = pdev->resource;
> +		int num_reg = pdev->num_resources;
> +		int num = num_reg + num_irq;
> +
> +		res = krealloc(res, num * sizeof(*res), GFP_KERNEL);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		pdev->num_resources = num;
> +		pdev->resource = res;
> +		res += num_reg;

What will happen if Driver probe is failed or deferred?
Seems resource table size will grow each time the Driver probe is deferred or failed.

> +
> +		ret = of_irq_to_resource_table(np, res, num_irq);
> +		if (ret < 0)
> +			return ret;
> +
> +		WARN_ON(ret != num_irq);
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
>   #endif /* CONFIG_OF_ADDRESS */
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..92fc4f6 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
>   				const struct of_device_id *matches,
>   				const struct of_dev_auxdata *lookup,
>   				struct device *parent);
> +
> +extern int of_platform_probe(struct platform_device *pdev);
>   #else
>   static inline int of_platform_populate(struct device_node *root,
>   					const struct of_device_id *matches,
> @@ -80,6 +82,11 @@ static inline int of_platform_populate(struct device_node *root,
>   {
>   	return -ENODEV;
>   }
> +
> +static inline int of_platform_probe(struct platform_device *pdev)
> +{
> +	return 0;
> +}
>   #endif
>   
>   #endif	/* _LINUX_OF_PLATFORM_H */
> 
Regards,
- grygorii

Re: [PATCH 4/9] of/irq: Introduce of_irq_get()

[Thierry Reding]

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

On Mon, Sep 16, 2013 at 04:24:47PM -0500, Rob Herring wrote:
> On 09/16/2013 03:32 AM, Thierry Reding wrote:
> > This is a version of irq_of_parse_and_map() that propagates the precise
> > error code instead of returning 0 for all errors. It will be used in
> > subsequent patches to allow further propagation of error codes.
> > 
> > To avoid code duplication, implement irq_of_parse_and_map() as a wrapper
> > around the new of_irq_get().
> 
> *_get typically also implies some reference counting which I don't
> believe this does. I don't think having 2 functions with completely
> different names doing the same thing with only a different calling
> convention is good either. So I would keep the old name and the names
> aligned.

Okay, I'll make the new function __irq_of_parse_and_map().

> >  unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
> >  {
> > -	struct of_irq oirq;
> > +	unsigned int virq;
> >  
> > -	if (of_irq_map_one(dev, index, &oirq))
> > +	if (of_irq_get(dev, index, &virq))
> >  		return 0;
> >  
> > -	return irq_create_of_mapping(oirq.controller, oirq.specifier,
> > -				     oirq.size);
> > +	return virq;
> 
> This can be an inline and written more concisely:
> 
> {
> 	unsigned int virq;
> 	return (of_irq_get(dev, index, &virq) < 0) ? 0 : virq;
> }

I find code such as the above very hard to read. But if you insist I can
move the function into include/of/of_irq.h and make it static inline
with the implementation above.

Thierry

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

Re: [PATCH 7/9] of/platform: Resolve interrupt references at probe time

[Thierry Reding]

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

On Tue, Sep 17, 2013 at 01:04:06PM +0000, Strashko, Grygorii wrote:
> Hi Thierry,
> 
> On 09/16/2013 11:32 AM, Thierry Reding wrote:> Interrupt references are currently resolved very early (when a device is
> > created). This has the disadvantage that it will fail in cases where the
> > interrupt parent hasn't been probed and no IRQ domain for it has been
> > registered yet. To work around that various drivers use explicit
> > initcall ordering to force interrupt parents to be probed before devices
> > that need them are created. That's error prone and doesn't always work.
> > If a platform device uses an interrupt line connected to a different
> > platform device (such as a GPIO controller), both will be created in the
> > same batch, and the GPIO controller won't have been probed by its driver
> > when the depending platform device is created. Interrupt resolution will
> > fail in that case.
> > 
> > Another common workaround is for drivers to explicitly resolve interrupt
> > references at probe time. This is suboptimal, however, because it will
> > require every driver to duplicate the code.
> > 
> > This patch adds support for late interrupt resolution to the platform
> > driver core, by resolving the references right before a device driver's
> > .probe() function will be called. This not only delays the resolution
> > until a much later time (giving interrupt parents a better chance of
> > being probed in the meantime), but it also allows the platform driver
> > core to queue the device for deferred probing if the interrupt parent
> > hasn't registered its IRQ domain yet.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   drivers/base/platform.c     |  4 ++++
> >   drivers/of/platform.c       | 43 +++++++++++++++++++++++++++++++++++++------
> >   include/linux/of_platform.h |  7 +++++++
> >   3 files changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 4f8bef3..8dcf835 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
> 
> Should it be the part of really_probe()? Isn't it?

really_probe() takes a struct device and is in fact called by all types
of devices. This code, however, is highly platform_device specific, so I
don't think we can do it in really_probe().

Unfortunately every device type has its own way of storing interrupts.
Platform devices store them as resources, I2C clients store them as a
separate field in struct i2c_client, etc.

> > +int of_platform_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	int num_irq, ret = 0;
> > +
> > +	if (!pdev->dev.of_node)
> > +		return 0;
> > +
> > +	num_irq = of_irq_count(pdev->dev.of_node);
> > +	if (num_irq > 0) {
> > +		struct resource *res = pdev->resource;
> > +		int num_reg = pdev->num_resources;
> > +		int num = num_reg + num_irq;
> > +
> > +		res = krealloc(res, num * sizeof(*res), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		pdev->num_resources = num;
> > +		pdev->resource = res;
> > +		res += num_reg;
> 
> What will happen if Driver probe is failed or deferred?
> Seems resource table size will grow each time the Driver probe is
> deferred or failed.

That's a very good point. I think what we can do is check whether the
total number of resources that the device has (pdev->num_resources)
corresponds to num_reg + num_irq and skip in that case. That and...

> > +		ret = of_irq_to_resource_table(np, res, num_irq);
> > +		if (ret < 0)
> > +			return ret;

... updating pdev->num_resources after this point should cover all
cases. Do you see any other potential problems?

Thierry

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

Re: [PATCH 8/9] of/i2c: Resolve interrupt references at probe time

[Wolfram Sang]

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

On Mon, Sep 16, 2013 at 10:32:05AM +0200, Thierry Reding wrote:
> Instead of resolving interrupt references at device creation time, delay
> resolution until probe time. At device creation time, there is nothing
> that can be done if an interrupt parent isn't ready yet, and the device
> will end up with an invalid interrupt number (0).
> 
> If the interrupt reference is resolved at probe time, the device's probe
> can be deferred, so that it's interrupt resolution can be retried after
> more devices (possibly including its interrupt parent) have been probed.
> 
> However, individual drivers shouldn't be required to do that themselves,
> over and over again, so this commit implements this functionality within
> the I2C core.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/i2c/i2c-core.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 29d3f04..163a1e8 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -236,6 +236,21 @@ int i2c_recover_bus(struct i2c_adapter *adap)
>  	return adap->bus_recovery_info->recover_bus(adap);
>  }
>  
> +static int of_i2c_probe(struct i2c_client *client)

This function should be in the CONFIG_OF block.

> +{
> +	struct device_node *np = client->dev.of_node;
> +	int ret;
> +
> +	/* skip if the device node specifies no interrupts */
> +	if (of_get_property(np, "interrupts", NULL)) {
> +		ret = of_irq_get(client->dev.of_node, 0, &client->irq);

of_irq_get really sounds as it does refcounting. +1 to change the name.

Thanks,

   Wolfram


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

Re: [PATCH 8/9] of/i2c: Resolve interrupt references at probe time

[Thierry Reding]

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

On Mon, Sep 23, 2013 at 09:34:51AM +0200, Wolfram Sang wrote:
> On Mon, Sep 16, 2013 at 10:32:05AM +0200, Thierry Reding wrote:
> > Instead of resolving interrupt references at device creation time, delay
> > resolution until probe time. At device creation time, there is nothing
> > that can be done if an interrupt parent isn't ready yet, and the device
> > will end up with an invalid interrupt number (0).
> > 
> > If the interrupt reference is resolved at probe time, the device's probe
> > can be deferred, so that it's interrupt resolution can be retried after
> > more devices (possibly including its interrupt parent) have been probed.
> > 
> > However, individual drivers shouldn't be required to do that themselves,
> > over and over again, so this commit implements this functionality within
> > the I2C core.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/i2c/i2c-core.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 29d3f04..163a1e8 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -236,6 +236,21 @@ int i2c_recover_bus(struct i2c_adapter *adap)
> >  	return adap->bus_recovery_info->recover_bus(adap);
> >  }
> >  
> > +static int of_i2c_probe(struct i2c_client *client)
> 
> This function should be in the CONFIG_OF block.

I'll argue the other way. I think that the whole CONFIG_OF block should
be remove, or at least be confined to public functions. The reason that
I've chosen to do it this way is that the i2c_device_probe() uses an
IS_ENABLED(CONFIG_OF), which will evaluate to 0 if OF isn't selected
which in turn will cause the compiler to throw away of_i2c_probe()
because it is static and never used.

The big advantage of this is that we get compile coverage for these
functions independent of what configuration is built. The same could
probably be done for the whole CONFIG_ACPI block. In fact I volunteer to
provide that patch if I've convinced you that this is actually better
than sprinking the code with the #ifdef blocks that make it harder than
necessary to verify that the code builds in all configurations.

> > +{
> > +	struct device_node *np = client->dev.of_node;
> > +	int ret;
> > +
> > +	/* skip if the device node specifies no interrupts */
> > +	if (of_get_property(np, "interrupts", NULL)) {
> > +		ret = of_irq_get(client->dev.of_node, 0, &client->irq);
> 
> of_irq_get really sounds as it does refcounting. +1 to change the name.

Done in v2.

Thanks,
Thierry

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

Re: [PATCH 8/9] of/i2c: Resolve interrupt references at probe time

[Wolfram Sang]

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


> > This function should be in the CONFIG_OF block.
> 
> I'll argue the other way. I think that the whole CONFIG_OF block should
> be remove, or at least be confined to public functions. The reason that
> I've chosen to do it this way is that the i2c_device_probe() uses an
> IS_ENABLED(CONFIG_OF), which will evaluate to 0 if OF isn't selected
> which in turn will cause the compiler to throw away of_i2c_probe()
> because it is static and never used.
> 
> The big advantage of this is that we get compile coverage for these
> functions independent of what configuration is built. The same could
> probably be done for the whole CONFIG_ACPI block. In fact I volunteer to
> provide that patch if I've convinced you that this is actually better
> than sprinking the code with the #ifdef blocks that make it harder than
> necessary to verify that the code builds in all configurations.

Yes, you (easily) did :) Still, I'd like the of_i2c_probe function
near the other of_* functions.

Thanks,

   Wolfram


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

Re: [PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Linus Walleij]

On Mon, Sep 16, 2013 at 10:31 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> This is a version of irq_create_mapping() that propagates the precise
> error code instead of returning 0 for all errors. It will be used in
> subsequent patches to allow further propagation of error codes.
>
> To avoid code duplication, implement irq_create_mapping() as a wrapper
> around the new __irq_create_mapping().
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Surprise! I don't like this.

I think it is better to first go over the call sites and make them
all handle negative return numbers rather than pushing the
obscure __interface.

I know from patch 0 that you think it's too much to change these
127 call sites but I don't think so, and I'm happy to merge one
big patch changing all the 20 users in drivers/gpio.

Likewise with the 11 consumers in drivers/pinctrl.

It's just a a few archs+subsystems and it's just plain work.

So do that first.

Yours,
Linus Walleij

Re: [PATCH 3/9] irqdomain: Introduce __irq_create_of_mapping()

[Linus Walleij]

On Mon, Sep 16, 2013 at 11:17 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 09/16/2013 03:32 AM, Thierry Reding wrote:
>> This is a version of irq_create_of_mapping() that propagates the precise
>> error code instead of returning 0 for all errors. It will be used in
>> subsequent patches to allow further propagation of error codes.
>>
>> To avoid code duplication, implement irq_create_of_mapping() as a
>> wrapper around the new __irq_create_of_mapping().
>
> This function is a manageable number of callers that the callers should
> just be updated and avoid the wrapper.

I second this and also don't want the first patch to use a wrapper.

Yours,
Linus Walleij

Re: [PATCH 4/9] of/irq: Introduce of_irq_get()

[Linus Walleij]

On Tue, Sep 17, 2013 at 3:28 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Sep 16, 2013 at 04:24:47PM -0500, Rob Herring wrote:

>> *_get typically also implies some reference counting which I don't
>> believe this does. I don't think having 2 functions with completely
>> different names doing the same thing with only a different calling
>> convention is good either. So I would keep the old name and the names
>> aligned.
>
> Okay, I'll make the new function __irq_of_parse_and_map().

I don't know why i detest __prefixing so much but I think it's
really nasty.

Usually this is reserved for compiler- and linker related things,
like __packed;  or __init.

I would prefer irq_of_parse_and_map_strict() or something
like that.

Yours,
Linus Walleij

Re: [PATCH 5/9] of/irq: Introduce __of_irq_to_resource()

[Linus Walleij]

On Mon, Sep 16, 2013 at 11:29 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 09/16/2013 03:32 AM, Thierry Reding wrote:
>> This is a version of of_irq_to_resource() that propagates the precise
>> error code instead of returning 0 for all errors. It will be used in
>> subsequent patches to allow further propagation of error codes.
>>
>> To avoid code duplication, implement of_irq_to_resource() as a wrapper
>> around the new __of_irq_to_resource().
>
> I think the callers in this case are manageable to update as well.
> Several cases could simply use irq_of_parse_and_map instead as they just
> pass in a NULL resource.

I second this comment.

Yours,
Linus Walleij

Re: [PATCH 9/9] gpio: tegra: Use module_platform_driver()

[Linus Walleij]

On Mon, Sep 16, 2013 at 10:32 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> With the driver core now resolving interrupt references at probe time,
> it is no longer necessary to force explicit probe ordering using
> initcalls.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Note that there are potentially many more drivers that can be switched
> to the generic module_*_driver() interfaces now that interrupts can be
> resolved later and deferred probe should be able to handle all the
> ordering issues.

Let me see if I get this right: so this would be all GPIO/pinctrl
drivers which are probed exclusively from the device tree
so anything that depends explicitly on CONFIG_OF would
be a candidate?

I think we have a bit of a problem that some drivers depend
only on a certain arch or compiles directly for a certain arch
without any specific Kconfig option so that this may be a
bit hard to spot, so we should keep an eye out for this
once this probing scheme is in place.

Yours,
Linus Walleij

Re: [PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Thierry Reding]

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

On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 10:31 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > This is a version of irq_create_mapping() that propagates the precise
> > error code instead of returning 0 for all errors. It will be used in
> > subsequent patches to allow further propagation of error codes.
> >
> > To avoid code duplication, implement irq_create_mapping() as a wrapper
> > around the new __irq_create_mapping().
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Surprise! I don't like this.
> 
> I think it is better to first go over the call sites and make them
> all handle negative return numbers rather than pushing the
> obscure __interface.
> 
> I know from patch 0 that you think it's too much to change these
> 127 call sites but I don't think so, and I'm happy to merge one
> big patch changing all the 20 users in drivers/gpio.
> 
> Likewise with the 11 consumers in drivers/pinctrl.
> 
> It's just a a few archs+subsystems and it's just plain work.

Well, the problem is that the current patch changes the signature of the
function as well, therefore the call sites will have to be updated all
at once in a single patch to avoid build breakage. And that's excluding
any potential fallout from new callsites added between the creation of
the patch and its application.

Another alternative could be to change the signature in a way that does
not break compatibility. For instance I think it could work out if we
change this function to return int instead of unsigned int but keep the
same semantics to begin with (return 0 on failure). Then update all call
sites to handle potential negative errors and after that return negative
error codes. That still wouldn't catch any callers introduced between
the patch creation and application.

Thierry

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

Re: [PATCH 9/9] gpio: tegra: Use module_platform_driver()

[Thierry Reding]

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

On Mon, Sep 23, 2013 at 09:25:53PM +0200, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 10:32 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > With the driver core now resolving interrupt references at probe time,
> > it is no longer necessary to force explicit probe ordering using
> > initcalls.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Note that there are potentially many more drivers that can be switched
> > to the generic module_*_driver() interfaces now that interrupts can be
> > resolved later and deferred probe should be able to handle all the
> > ordering issues.
> 
> Let me see if I get this right: so this would be all GPIO/pinctrl
> drivers which are probed exclusively from the device tree
> so anything that depends explicitly on CONFIG_OF would
> be a candidate?

It includes all interrupt providers that are probed from the device
tree. I'm not sure exactly what the situation is regarding DT vs non-DT,
but if my memory serves me well, with non-DT setups interrupts need to
be hard-coded in the board support code. Therefore I don't think the
usefulness is limited to drivers that are exclusively probed from device
tree, but rather any interrupt providing driver that can be probed from
device tree.

> I think we have a bit of a problem that some drivers depend
> only on a certain arch or compiles directly for a certain arch
> without any specific Kconfig option so that this may be a
> bit hard to spot, so we should keep an eye out for this
> once this probing scheme is in place.

Yes, it certainly needs to be decided on a case by case basis. There
might be other factors that prevent a driver from being a proper module
driver.

Thierry

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

Re: [PATCH 4/9] of/irq: Introduce of_irq_get()

[Thierry Reding]

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

On Mon, Sep 23, 2013 at 09:18:52PM +0200, Linus Walleij wrote:
> On Tue, Sep 17, 2013 at 3:28 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 16, 2013 at 04:24:47PM -0500, Rob Herring wrote:
> 
> >> *_get typically also implies some reference counting which I don't
> >> believe this does. I don't think having 2 functions with completely
> >> different names doing the same thing with only a different calling
> >> convention is good either. So I would keep the old name and the names
> >> aligned.
> >
> > Okay, I'll make the new function __irq_of_parse_and_map().
> 
> I don't know why i detest __prefixing so much but I think it's
> really nasty.
> 
> Usually this is reserved for compiler- and linker related things,
> like __packed;  or __init.
> 
> I would prefer irq_of_parse_and_map_strict() or something
> like that.

Following from the discussion on one of the other patches, perhaps this
could also be converted to return int (instead of unsigned int), convert
all callers to cope with negative or 0 (instead of only 0) as errors and
then modify it to actually start returning negative error codes once all
users have been updated. Eventually I think we should get rid of using 0
as an error indicator altogether, but as I already mentioned it might be
difficult to do because new users can always come along and use the then
deprecated 0 return on error.

I'm somewhat worried about the amount of work that this is turning into.

Thierry

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

Re: [PATCH 5/9] of/irq: Introduce __of_irq_to_resource()

[Thierry Reding]

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

On Mon, Sep 23, 2013 at 09:20:37PM +0200, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 11:29 PM, Rob Herring <robherring2@gmail.com> wrote:
> > On 09/16/2013 03:32 AM, Thierry Reding wrote:
> >> This is a version of of_irq_to_resource() that propagates the precise
> >> error code instead of returning 0 for all errors. It will be used in
> >> subsequent patches to allow further propagation of error codes.
> >>
> >> To avoid code duplication, implement of_irq_to_resource() as a wrapper
> >> around the new __of_irq_to_resource().
> >
> > I think the callers in this case are manageable to update as well.
> > Several cases could simply use irq_of_parse_and_map instead as they just
> > pass in a NULL resource.
> 
> I second this comment.

That should be fixed in v2 of the series that I posted a few days ago.

Thierry

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

Re: [PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:29 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote:

>> I think it is better to first go over the call sites and make them
>> all handle negative return numbers rather than pushing the
>> obscure __interface.
(...)
>
> Well, the problem is that the current patch changes the signature of the
> function as well, therefore the call sites will have to be updated all
> at once in a single patch to avoid build breakage.

Hm yeah OK I see the problem, but can we atleast avoid the
__thing? Like calling the new function irq_create_mapping_strict()
or whatever.

> Another alternative could be to change the signature in a way that does
> not break compatibility. For instance I think it could work out if we
> change this function to return int instead of unsigned int but keep the
> same semantics to begin with (return 0 on failure). Then update all call
> sites to handle potential negative errors and after that return negative
> error codes.

Hm that sounds like an attractive solution to me actually.

> That still wouldn't catch any callers introduced between
> the patch creation and application.

Such things happen all the time, just have to be attentive in
what goes into linux-next...

Another minor thing:

+static int __irq_create_mapping(struct irq_domain *domain,
+                               irq_hw_number_t hwirq, unsigned int *virqp)

Unless you can make a very good case for why there should
be a "v" in the beginning of virqp, then remove it and call it
"irqp" simply.

All Linux IRQs are virtual and we're already clearly separating
out those that are not by calling them "hwirq".

Yours,
Linus Walleij

Re: [PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Thierry Reding]

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

On Tue, Sep 24, 2013 at 02:20:44PM +0200, Linus Walleij wrote:
> On Mon, Sep 23, 2013 at 10:29 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote:
> 
> >> I think it is better to first go over the call sites and make them
> >> all handle negative return numbers rather than pushing the
> >> obscure __interface.
> (...)
> >
> > Well, the problem is that the current patch changes the signature of the
> > function as well, therefore the call sites will have to be updated all
> > at once in a single patch to avoid build breakage.
> 
> Hm yeah OK I see the problem, but can we atleast avoid the
> __thing? Like calling the new function irq_create_mapping_strict()
> or whatever.

_strict sort of implies that it does something more than the non-strict
irq_create_mapping() while it really doesn't. Perhaps the alternative
proposed below would indeed be a better solution.

> > Another alternative could be to change the signature in a way that does
> > not break compatibility. For instance I think it could work out if we
> > change this function to return int instead of unsigned int but keep the
> > same semantics to begin with (return 0 on failure). Then update all call
> > sites to handle potential negative errors and after that return negative
> > error codes.
> 
> Hm that sounds like an attractive solution to me actually.

The only thing we'd loose is the additional bit, but given that most (if
not all) platforms that use DT are 32-bit (do we actually support any
platforms that don't have 32-bit integers?) that should not matter at
all. We're not very likely to get anywhere near that number of
interrupts in the system.

> > That still wouldn't catch any callers introduced between
> > the patch creation and application.
> 
> Such things happen all the time, just have to be attentive in
> what goes into linux-next...

Given that linux-next might not be with us for much longer before the
3.12 release, I'm thinking of deferring the series until then. Or at
least trying to get it merged. Otherwise we'll probably have to deal
with a lot of fall out during the merge window.

In any case, it'd be nice to get some feedback on the general idea of
the patch series from other people involved. I'd hate to do all the
conversions just to have it NAKed at the last minute.

> Another minor thing:
> 
> +static int __irq_create_mapping(struct irq_domain *domain,
> +                               irq_hw_number_t hwirq, unsigned int *virqp)
> 
> Unless you can make a very good case for why there should
> be a "v" in the beginning of virqp, then remove it and call it
> "irqp" simply.
> 
> All Linux IRQs are virtual and we're already clearly separating
> out those that are not by calling them "hwirq".

Yeah, I was just trying to adapt to what was already there. But with the
alternative proposal that'll go away anyway.

Thierry

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

Re: [PATCH 2/9] irqdomain: Introduce __irq_create_mapping()

[Linus Walleij]

On Tue, Sep 24, 2013 at 8:28 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> Given that linux-next might not be with us for much longer before the
> 3.12 release, I'm thinking of deferring the series until then. Or at
> least trying to get it merged. Otherwise we'll probably have to deal
> with a lot of fall out during the merge window.

Hm, how unfortunate. Typically this is the kind of topic branch
that should go in separately to linux-next.

> In any case, it'd be nice to get some feedback on the general idea of
> the patch series from other people involved. I'd hate to do all the
> conversions just to have it NAKed at the last minute.

With the direction we've discussed here atleast I won't be
doing any NACKing if it's of any help...

Yours,
Linus Walleij