[PATCH 0/8] Changes surrounding IRQs and IRQ domains

Here we apply some fixes to the way the AB8500 handles IRQs and
provide the DB8500 with an IRQ domain. We also handle hwirq to
virq conversion slightly differently when registering MFD devices.
The other changes pertain to the way IRQs are requested and dealt
with by AB8500 child devices.

 arch/arm/boot/dts/dbx5x0.dtsi      |    3 ++
 drivers/input/misc/ab8500-ponkey.c |    4 +--
 drivers/mfd/ab8500-core.c          |    3 +-
 drivers/mfd/db8500-prcmu.c         |   54 +++++++++++++++++++++++++++---------
 drivers/mfd/mfd-core.c             |    2 +-
 include/linux/mfd/db8500-prcmu.h   |    2 ++
 include/linux/of_irq.h             |    5 ++++
 kernel/irq/irqdomain.c             |    7 +++++
 8 files changed, 63 insertions(+), 17 deletions(-)

of_irq_find_parent is a handy function to use outside the confines of
the Open Firmware subsystem. One such use-case is when the IRQ Domain
wishes to find an IRQ domain for a given device node. Currently it can
not take any notice of the 'interrupt-parent' property. Instead it
just uses the first IRQ controller as it climbs the Device Tree. If
we were to use this as a precursor the resultant controller is more
likely to be correct.

CC: Rob Herring <redacted>
CC: devicetree-discuss at lists.ozlabs.org
Signed-off-by: Lee Jones <redacted>
---
 include/linux/of_irq.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 1717cd9..b8e2411 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h

@@ -83,6 +83,11 @@ static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
 {
 	return 0;
 }
+
+static inline void *of_irq_find_parent(struct device_node *child)
+{
+	return NULL;
+}
 #endif /* !CONFIG_OF */
 
 #endif /* __OF_IRQ_H */

-- 
1.7.9.5

Without this patch the default behaviour is to climb the Device
Tree and use the first encountered interrupt controller. This
does not take into account if a device node has specified to use
a particular IRQ controller using the interrupt-parent property.
This patch ensures that property is adhered to.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>
---
 drivers/mfd/mfd-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0c3a01c..bb5e753 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c

@@ -97,7 +97,7 @@ static int mfd_add_device(struct device *parent, int id,
 		for_each_child_of_node(parent->of_node, np) {
 			if (of_device_is_compatible(np, cell->of_compatible)) {
 				pdev->dev.of_node = np;
-				domain = irq_find_host(parent->of_node);
+				domain = irq_find_host(np);
 				break;
 			}
 		}

-- 
1.7.9.5

The PRCMU has its own USB, Thermal, GPIO, Modem, HSI and RTC drivers,
amongst other things. This patch allows those subordinate devices to
use it as an interrupt controller as and when they are DT enabled.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>
---
 drivers/mfd/db8500-prcmu.c       |   54 +++++++++++++++++++++++++++++---------
 include/linux/mfd/db8500-prcmu.h |    2 ++
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 7040a00..9899b3f 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c

@@ -270,6 +270,8 @@ static struct {
 	struct prcmu_fw_version version;
 } fw_info;
 
+static struct irq_domain *db8500_irq_domain;
+
 /*
  * This vector maps irq numbers to the bits in the bit field used in
  * communication with the PRCMU firmware.

@@ -2583,7 +2585,7 @@ static void prcmu_irq_mask(struct irq_data *d)
 
 	spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags);
 
-	mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE];
+	mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->hwirq];
 
 	spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags);
 

@@ -2597,7 +2599,7 @@ static void prcmu_irq_unmask(struct irq_data *d)
 
 	spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags);
 
-	mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE];
+	mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->hwirq];
 
 	spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags);
 

@@ -2637,9 +2639,43 @@ static char *fw_project_name(u8 project)
 	}
 }
 
+int db8500_irq_get_virq(int irq)
+{
+	return irq_create_mapping(db8500_irq_domain, irq);
+}
+EXPORT_SYMBOL_GPL(db8500_irq_get_virq);
+
+static int db8500_irq_map(struct irq_domain *d, unsigned int virq,
+				irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(virq, &prcmu_irq_chip,
+				handle_simple_irq);
+	set_irq_flags(virq, IRQF_VALID);
+
+	return 0;
+}
+
+static struct irq_domain_ops db8500_irq_ops = {
+        .map    = db8500_irq_map,
+        .xlate  = irq_domain_xlate_twocell,
+};
+
+static int db8500_irq_init(struct device_node *np)
+{
+	db8500_irq_domain = irq_domain_add_legacy(
+		np, NUM_PRCMU_WAKEUPS, IRQ_PRCMU_BASE,
+		0, &db8500_irq_ops, NULL);
+
+	if (!db8500_irq_domain) {
+		pr_err("Failed to create irqdomain\n");
+		return -ENOSYS;
+	}
+
+	return 0;
+}
+
 void __init db8500_prcmu_early_init(void)
 {
-	unsigned int i;
 	if (cpu_is_u8500v2()) {
 		void *tcpm_base = ioremap_nocache(U8500_PRCMU_TCPM_BASE, SZ_4K);
 

@@ -2683,16 +2719,6 @@ void __init db8500_prcmu_early_init(void)
 	init_completion(&mb5_transfer.work);
 
 	INIT_WORK(&mb0_transfer.mask_work, prcmu_mask_work);
-
-	/* Initalize irqs. */
-	for (i = 0; i < NUM_PRCMU_WAKEUPS; i++) {
-		unsigned int irq;
-
-		irq = IRQ_PRCMU_BASE + i;
-		irq_set_chip_and_handler(irq, &prcmu_irq_chip,
-					 handle_simple_irq);
-		set_irq_flags(irq, IRQF_VALID);
-	}
 }
 
 static void __init init_prcm_registers(void)

@@ -2999,6 +3025,8 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 		goto no_irq_return;
 	}
 
+	db8500_irq_init(np);
+
 	for (i = 0; i < ARRAY_SIZE(db8500_prcmu_devs); i++) {
 		if (!strcmp(db8500_prcmu_devs[i].name, "ab8500-core")) {
 			db8500_prcmu_devs[i].platform_data = ab8500_platdata;

diff --git a/include/linux/mfd/db8500-prcmu.h b/include/linux/mfd/db8500-prcmu.h
index b82f6ee..38494d9 100644
--- a/include/linux/mfd/db8500-prcmu.h
+++ b/include/linux/mfd/db8500-prcmu.h

@@ -571,6 +571,8 @@ u32 db8500_prcmu_read(unsigned int reg);
 void db8500_prcmu_write(unsigned int reg, u32 value);
 void db8500_prcmu_write_masked(unsigned int reg, u32 mask, u32 value);
 
+int db8500_irq_get_virq(int irq);
+
 #else /* !CONFIG_MFD_DB8500_PRCMU */
 
 static inline void db8500_prcmu_early_init(void) {}

-- 
1.7.9.5

There was a plan to place ab8500_irq_get_virq() calls in each AB8500
child device prior to requesting an IRQ, but as we're no longer using
Device Tree to collect our IRQ numbers, it's actually better to allow
the core to do this during device registration time. So the IRQ number
we pull from its resource has already been converted to a virtual IRQ.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: linux-input at vger.kernel.org
Signed-off-by: Lee Jones <redacted>
---
 drivers/input/misc/ab8500-ponkey.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/ab8500-ponkey.c b/drivers/input/misc/ab8500-ponkey.c
index f06231b..84ec691 100644
--- a/drivers/input/misc/ab8500-ponkey.c
+++ b/drivers/input/misc/ab8500-ponkey.c

@@ -74,8 +74,8 @@ static int __devinit ab8500_ponkey_probe(struct platform_device *pdev)
 
 	ponkey->idev = input;
 	ponkey->ab8500 = ab8500;
-	ponkey->irq_dbf = ab8500_irq_get_virq(ab8500, irq_dbf);
-	ponkey->irq_dbr = ab8500_irq_get_virq(ab8500, irq_dbr);
+	ponkey->irq_dbf = irq_dbf;
+	ponkey->irq_dbr = irq_dbr;
 
 	input->name = "AB8500 POn(PowerOn) Key";
 	input->dev.parent = &pdev->dev;

-- 
1.7.9.5

Before the AB8500 had its own IRQ domain, the IRQ handler would take
the fired local IRQ (hwirq) and add it to the irq_base to convert it
to an IRQ number which Linux would understand (virq). However, the
IRQ base is not always used anymore since we can make use of Linear
domains. It's better to use the AB8500 hwirq -> virq mapping helper
function to convert them instead. That's what we do here.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>
---
 drivers/mfd/ab8500-core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 0c5b70f..71a7757 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c

@@ -501,8 +501,9 @@ static irqreturn_t ab8500_irq(int irq, void *dev)
 		do {
 			int bit = __ffs(value);
 			int line = i * 8 + bit;
+			int virq = ab8500_irq_get_virq(ab8500, line);
 
-			handle_nested_irq(ab8500->irq_base + line);
+			handle_nested_irq(virq);
 			value &= ~(1 << bit);
 
 		} while (value);

-- 
1.7.9.5

It's understood that the AB8500 should be subordinate to the DB8500;
however, the AB8500 uses the GIC as it's interrupt controller. If
we do not specify which IRQ controller to use the default is to use
the next encountered IRQ controller as we climb the tree. This would
be the DB8500. This patch ensures the AB8500 makes use of the correct
interrupt controller.

Signed-off-by: Lee Jones <redacted>
---
 arch/arm/boot/dts/dbx5x0.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/dbx5x0.dtsi b/arch/arm/boot/dts/dbx5x0.dtsi
index 6da7ccb..5106662 100644
--- a/arch/arm/boot/dts/dbx5x0.dtsi
+++ b/arch/arm/boot/dts/dbx5x0.dtsi

@@ -332,6 +332,7 @@
 			ab8500 at 5 {
 				compatible = "stericsson,ab8500";
 				reg = <5>; /* mailbox 5 is i2c */
+				interrupt-parent = <&intc>;
 				interrupts = <0 40 0x4>;
 				interrupt-controller;
 				#interrupt-cells = <2>;

-- 
1.7.9.5

We're just about to provide the DB8500-PRCMU with its own IRQ domain,
so that its subordinate drivers can use it as an interrupt controller.
It's obligatory for all IRQ controllers to reference themselves as
such from its own node in Device Tree. This patch does just that.

Signed-off-by: Lee Jones <redacted>
---
 arch/arm/boot/dts/dbx5x0.dtsi |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/dbx5x0.dtsi b/arch/arm/boot/dts/dbx5x0.dtsi
index 32e063d..6da7ccb 100644
--- a/arch/arm/boot/dts/dbx5x0.dtsi
+++ b/arch/arm/boot/dts/dbx5x0.dtsi

@@ -194,6 +194,8 @@
 			interrupts = <0 47 0x4>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			ranges;
 
 			prcmu-timer-4 at 80157450 {

-- 
1.7.9.5

irq_find_host() currently ignores the 'interrupt-parent' property
even if it's specified in the Device Tree. Meaning that a node can
match to a domain in its hierarchy even if it doesn't belong to it.
By searching for the parent first using of_irq_find_parent() we
insist that the 'interrupt-parent' property is taken into account
ensuring a greater chance of returning the correct domain.

CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Grant Likely <redacted>
Signed-off-by: Lee Jones <redacted>
---
 kernel/irq/irqdomain.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 49a7772..db63b9b 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/of_address.h>
 #include <linux/topology.h>
 #include <linux/seq_file.h>

@@ -323,8 +324,14 @@ EXPORT_SYMBOL_GPL(irq_domain_add_tree);
 struct irq_domain *irq_find_host(struct device_node *node)
 {
 	struct irq_domain *h, *found = NULL;
+	struct device_node *parent_node;
 	int rc;
 
+	/* Take heed if an 'interrupt-parent' was specified. */
+	parent_node = of_irq_find_parent(node);
+	if (parent_node)
+		node = parent_node;
+
 	/* We might want to match the legacy controller last since
 	 * it might potentially be set to match all interrupts in
 	 * the absence of a device node. This isn't a problem so far

-- 
1.7.9.5

On 08/09/2012 10:53 AM, Lee Jones wrote:

of_irq_find_parent is a handy function to use outside the confines of
the Open Firmware subsystem. One such use-case is when the IRQ Domain
wishes to find an IRQ domain for a given device node. Currently it can
not take any notice of the 'interrupt-parent' property. Instead it
just uses the first IRQ controller as it climbs the Device Tree. If
we were to use this as a precursor the resultant controller is more
likely to be correct.

Where are you using this? I don't have all the patches in the series.

Rob

On Thu, Aug 09, 2012 at 11:20:54AM -0500, Rob Herring wrote:

On 08/09/2012 10:53 AM, Lee Jones wrote:

quoted

of_irq_find_parent is a handy function to use outside the confines of
the Open Firmware subsystem. One such use-case is when the IRQ Domain
wishes to find an IRQ domain for a given device node. Currently it can
not take any notice of the 'interrupt-parent' property. Instead it
just uses the first IRQ controller as it climbs the Device Tree. If
we were to use this as a precursor the resultant controller is more
likely to be correct.

Where are you using this? I don't have all the patches in the series.

Sorry Rob. Here:

https://lkml.org/lkml/2012/8/9/373

quoted

CC: Rob Herring <redacted>
CC: devicetree-discuss at lists.ozlabs.org
Signed-off-by: Lee Jones <redacted>
---
 include/linux/of_irq.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 1717cd9..b8e2411 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h

@@ -83,6 +83,11 @@ static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
 {
 	return 0;
 }
+
+static inline void *of_irq_find_parent(struct device_node *child)
+{
+	return NULL;
+}
 #endif /* !CONFIG_OF */
 
 #endif /* __OF_IRQ_H */

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On 08/09/2012 10:53 AM, Lee Jones wrote:

of_irq_find_parent is a handy function to use outside the confines of
the Open Firmware subsystem. One such use-case is when the IRQ Domain
wishes to find an IRQ domain for a given device node. Currently it can
not take any notice of the 'interrupt-parent' property. Instead it
just uses the first IRQ controller as it climbs the Device Tree. If
we were to use this as a precursor the resultant controller is more
likely to be correct.

CC: Rob Herring <redacted>
CC: devicetree-discuss at lists.ozlabs.org
Signed-off-by: Lee Jones <redacted>

Acked-by: Rob Herring <redacted>

Rob

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

of_irq_find_parent is a handy function to use outside the confines of
the Open Firmware subsystem. One such use-case is when the IRQ Domain
wishes to find an IRQ domain for a given device node. Currently it can
not take any notice of the 'interrupt-parent' property. Instead it
just uses the first IRQ controller as it climbs the Device Tree. If
we were to use this as a precursor the resultant controller is more
likely to be correct.

CC: Rob Herring <redacted>
CC: devicetree-discuss at lists.ozlabs.org
Signed-off-by: Lee Jones <redacted>

This is clever.

Acked-by: Linus Walleij <redacted>

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

irq_find_host() currently ignores the 'interrupt-parent' property
even if it's specified in the Device Tree. Meaning that a node can
match to a domain in its hierarchy even if it doesn't belong to it.
By searching for the parent first using of_irq_find_parent() we
insist that the 'interrupt-parent' property is taken into account
ensuring a greater chance of returning the correct domain.

CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Grant Likely <redacted>
Signed-off-by: Lee Jones <redacted>

This (with 1/8) is the right solution. Thanks, and I think
Mark may want to look at this too since I recognize he asked
you to work in this direction.

Acked-by: Linus Walleij <redacted>

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

We're just about to provide the DB8500-PRCMU with its own IRQ domain,
so that its subordinate drivers can use it as an interrupt controller.
It's obligatory for all IRQ controllers to reference themselves as
such from its own node in Device Tree. This patch does just that.

Signed-off-by: Lee Jones <redacted>

Acked-by: Linus Walleij <redacted>

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

It's understood that the AB8500 should be subordinate to the DB8500;
however, the AB8500 uses the GIC as it's interrupt controller. If
we do not specify which IRQ controller to use the default is to use
the next encountered IRQ controller as we climb the tree. This would
be the DB8500. This patch ensures the AB8500 makes use of the correct
interrupt controller.

Signed-off-by: Lee Jones <redacted>

Acked-by: Linus Walleij <redacted>

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

Without this patch the default behaviour is to climb the Device
Tree and use the first encountered interrupt controller. This
does not take into account if a device node has specified to use
a particular IRQ controller using the interrupt-parent property.
This patch ensures that property is adhered to.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>

Acked-by: Linus Walleij <redacted>

Mark & Rob may want to check this out too.

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

Before the AB8500 had its own IRQ domain, the IRQ handler would take
the fired local IRQ (hwirq) and add it to the irq_base to convert it
to an IRQ number which Linux would understand (virq). However, the
IRQ base is not always used anymore since we can make use of Linear
domains. It's better to use the AB8500 hwirq -> virq mapping helper
function to convert them instead. That's what we do here.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>

This looks better that what was in there before so
Acked-by: Linus Walleij <redacted>

However:

Still this ab8500_irq_get_virq() business. But is this a static local function
in the ab8500-core.c now? Then it's fine, it's the kernel-wide interface
that is the problem.

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

+static struct irq_domain *db8500_irq_domain;

So this is a good idea.

+int db8500_irq_get_virq(int irq);

And I'm sceptic about this business. Why isn't this physical-to virtual
mapping business confined to the core MFD driver? But enlighten me.

Yours,
Linus Walleij

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

There was a plan to place ab8500_irq_get_virq() calls in each AB8500
child device prior to requesting an IRQ, but as we're no longer using
Device Tree to collect our IRQ numbers, it's actually better to allow
the core to do this during device registration time. So the IRQ number
we pull from its resource has already been converted to a virtual IRQ.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: linux-input at vger.kernel.org
Signed-off-by: Lee Jones <redacted>

This is looking good, I guess you need all patches to go in at the
same time so Dmitry's ACK is required.

FWIW:
Acked-by: Linus Walleij <redacted>

BTW: this makes me suspect that the public ab8500_irq_get_virq()
interface can be *deleted* and the function made static in the
AB8500 driver, right?

Yours,
Linus Walleij

On Tuesday 14 August 2012, Linus Walleij wrote:

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

+static struct irq_domain *db8500_irq_domain;

So this is a good idea.

quoted

+int db8500_irq_get_virq(int irq);

And I'm sceptic about this business. Why isn't this physical-to virtual
mapping business confined to the core MFD driver? But enlighten me.

I believe MFD does not (yet) know about IRQ domains. The wm8994
and 88pm80x drivers do this slightly different by exporting
a foo_request_irq() function that performs the mapping.

Traditionally, an MFD would add an offset to the local IRQ number
to put the VIRQ into the IRQ resource, but this doesn't work when
you have domains other than legacy.

	Arnd

On Tue, Aug 14, 2012 at 11:42 AM, Arnd Bergmann [off-list ref] wrote:

On Tuesday 14 August 2012, Linus Walleij wrote:

quoted

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

+int db8500_irq_get_virq(int irq);

And I'm sceptic about this business. Why isn't this physical-to virtual
mapping business confined to the core MFD driver? But enlighten me.

Traditionally, an MFD would add an offset to the local IRQ number
to put the VIRQ into the IRQ resource, but this doesn't work when
you have domains other than legacy.

Yes but I think I saw this other patch set from Lee, hitting
irqdomain, OF and MFD to actually fix this ... or did I get
it wrong?

Yours,
Linus Walleij

On Tue, Aug 14, 2012 at 12:44:37PM +0200, Linus Walleij wrote:

On Tue, Aug 14, 2012 at 11:42 AM, Arnd Bergmann [off-list ref] wrote:

quoted

On Tuesday 14 August 2012, Linus Walleij wrote:

quoted

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

+int db8500_irq_get_virq(int irq);

And I'm sceptic about this business. Why isn't this physical-to virtual
mapping business confined to the core MFD driver? But enlighten me.

Traditionally, an MFD would add an offset to the local IRQ number
to put the VIRQ into the IRQ resource, but this doesn't work when
you have domains other than legacy.

Yes but I think I saw this other patch set from Lee, hitting
irqdomain, OF and MFD to actually fix this ... or did I get
it wrong?

No, you're not wrong.

Historically (in my patches) xb8500_irq_get_virq() was used by drivers
to obtain a VIRQ when not using Device Tree. Now the MFD core handles
conversion there is little requirement for it. In fact there are no
more users for db8500_irq_get_virq() and only one user for 
ab8500_irq_get_virq() and that's itself. I guess we can rid them and
call irq_get_mapping() directly instead.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Aug 14, 2012 at 10:29:14AM +0200, Linus Walleij wrote:

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

+static struct irq_domain *db8500_irq_domain;

So this is a good idea.

Did you mean this, or did you mean that it's _not_ a good idea?

If the latter is true, where would you prefer to see it?

quoted

+int db8500_irq_get_virq(int irq);

And I'm sceptic about this business. Why isn't this physical-to virtual
mapping business confined to the core MFD driver? But enlighten me.

Yours,
Linus Walleij

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

From: Lee Jones <redacted>
Date: Fri, 3 Aug 2012 15:45:50 +0100
Subject: [PATCH 1/1] mfd: Provide the PRCMU with its own IRQ domain

The PRCMU has its own USB, Thermal, GPIO, Modem, HSI and RTC drivers,
amongst other things. This patch allows those subordinate devices to
use it as an interrupt controller as and when they are DT enabled.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>
---
 drivers/mfd/db8500-prcmu.c |   48 ++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 7040a00..de09113 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c

@@ -270,6 +270,8 @@ static struct {
 	struct prcmu_fw_version version;
 } fw_info;
 
+static struct irq_domain *db8500_irq_domain;
+
 /*
  * This vector maps irq numbers to the bits in the bit field used in
  * communication with the PRCMU firmware.

@@ -2583,7 +2585,7 @@ static void prcmu_irq_mask(struct irq_data *d)
 
 	spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags);
 
-	mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE];
+	mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->hwirq];
 
 	spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags);
 

@@ -2597,7 +2599,7 @@ static void prcmu_irq_unmask(struct irq_data *d)
 
 	spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags);
 
-	mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE];
+	mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->hwirq];
 
 	spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags);
 

@@ -2637,9 +2639,37 @@ static char *fw_project_name(u8 project)
 	}
 }
 
+static int db8500_irq_map(struct irq_domain *d, unsigned int virq,
+				irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(virq, &prcmu_irq_chip,
+				handle_simple_irq);
+	set_irq_flags(virq, IRQF_VALID);
+
+	return 0;
+}
+
+static struct irq_domain_ops db8500_irq_ops = {
+        .map    = db8500_irq_map,
+        .xlate  = irq_domain_xlate_twocell,
+};
+
+static int db8500_irq_init(struct device_node *np)
+{
+	db8500_irq_domain = irq_domain_add_legacy(
+		np, NUM_PRCMU_WAKEUPS, IRQ_PRCMU_BASE,
+		0, &db8500_irq_ops, NULL);
+
+	if (!db8500_irq_domain) {
+		pr_err("Failed to create irqdomain\n");
+		return -ENOSYS;
+	}
+
+	return 0;
+}
+
 void __init db8500_prcmu_early_init(void)
 {
-	unsigned int i;
 	if (cpu_is_u8500v2()) {
 		void *tcpm_base = ioremap_nocache(U8500_PRCMU_TCPM_BASE, SZ_4K);
 

@@ -2683,16 +2713,6 @@ void __init db8500_prcmu_early_init(void)
 	init_completion(&mb5_transfer.work);
 
 	INIT_WORK(&mb0_transfer.mask_work, prcmu_mask_work);
-
-	/* Initalize irqs. */
-	for (i = 0; i < NUM_PRCMU_WAKEUPS; i++) {
-		unsigned int irq;
-
-		irq = IRQ_PRCMU_BASE + i;
-		irq_set_chip_and_handler(irq, &prcmu_irq_chip,
-					 handle_simple_irq);
-		set_irq_flags(irq, IRQF_VALID);
-	}
 }
 
 static void __init init_prcm_registers(void)

@@ -2999,6 +3019,8 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 		goto no_irq_return;
 	}
 
+	db8500_irq_init(np);
+
 	for (i = 0; i < ARRAY_SIZE(db8500_prcmu_devs); i++) {
 		if (!strcmp(db8500_prcmu_devs[i].name, "ab8500-core")) {
 			db8500_prcmu_devs[i].platform_data = ab8500_platdata;

-- 
1.7.9.5

On Mon, Aug 20, 2012 at 09:36:43AM +0100, Lee Jones wrote:

On Tue, Aug 14, 2012 at 12:44:37PM +0200, Linus Walleij wrote:

quoted

Yes but I think I saw this other patch set from Lee, hitting
irqdomain, OF and MFD to actually fix this ... or did I get
it wrong?

No, you're not wrong.

Historically (in my patches) xb8500_irq_get_virq() was used by drivers
to obtain a VIRQ when not using Device Tree. Now the MFD core handles
conversion there is little requirement for it. In fact there are no
more users for db8500_irq_get_virq() and only one user for 
ab8500_irq_get_virq() and that's itself. I guess we can rid them and
call irq_get_mapping() directly instead.

Oh dear.  Unfortunately whoever added this support to the MFD core did
so in such a manner that it's only supported for device tree systems
and only for devices which express the MFD cells as device tree nodes
which means that most devices can't it - db8500 has got a reasonably
unusual combination there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120820/51e1c98f/attachment.sig>

On Mon, Aug 20, 2012 at 01:10:55PM +0100, Mark Brown wrote:

On Mon, Aug 20, 2012 at 09:36:43AM +0100, Lee Jones wrote:

quoted

On Tue, Aug 14, 2012 at 12:44:37PM +0200, Linus Walleij wrote:

quoted

quoted

Yes but I think I saw this other patch set from Lee, hitting
irqdomain, OF and MFD to actually fix this ... or did I get
it wrong?

quoted

No, you're not wrong.

quoted

Historically (in my patches) xb8500_irq_get_virq() was used by drivers
to obtain a VIRQ when not using Device Tree. Now the MFD core handles
conversion there is little requirement for it. In fact there are no
more users for db8500_irq_get_virq() and only one user for 
ab8500_irq_get_virq() and that's itself. I guess we can rid them and
call irq_get_mapping() directly instead.

Oh dear.  Unfortunately whoever added this support to the MFD core did
so in such a manner that it's only supported for device tree systems
and only for devices which express the MFD cells as device tree nodes
which means that most devices can't it - db8500 has got a reasonably
unusual combination there.

Right, that was the initial intention. It would be a trivial semantic
change if drivers without DT support wished to use the functionality
though. However, the only examples I found of a non-DT enabled driver
that could make good use of it in order to strip out some cruft would
be the Arizona and one of the Samsung drivers, and they each have
their own hand-rolled methods of hwirq -> virq conversion now, so any
change to support them would result in multiple invocations of
irq_create_mapping which would likely cause breakage.
 
-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Mon, Aug 20, 2012 at 01:55:32PM +0100, Lee Jones wrote:

Right, that was the initial intention. It would be a trivial semantic
change if drivers without DT support wished to use the functionality
though. However, the only examples I found of a non-DT enabled driver
that could make good use of it in order to strip out some cruft would
be the Arizona and one of the Samsung drivers, and they each have

All of the regmap devices could use this.

their own hand-rolled methods of hwirq -> virq conversion now, so any
change to support them would result in multiple invocations of
irq_create_mapping which would likely cause breakage.

Multiple calls to irq_create_mapping() are totally fine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120820/c6a761ac/attachment.sig>

On Mon, Aug 20, 2012 at 05:29:23PM +0100, Mark Brown wrote:

On Mon, Aug 20, 2012 at 01:55:32PM +0100, Lee Jones wrote:

quoted

Right, that was the initial intention. It would be a trivial semantic
change if drivers without DT support wished to use the functionality
though. However, the only examples I found of a non-DT enabled driver
that could make good use of it in order to strip out some cruft would
be the Arizona and one of the Samsung drivers, and they each have

All of the regmap devices could use this.

Only if they have linear domains and don't support DT.

If they don't have linear domains there's no point, if they support DT
then they can use it as it is.

quoted

their own hand-rolled methods of hwirq -> virq conversion now, so any
change to support them would result in multiple invocations of
irq_create_mapping which would likely cause breakage.

Multiple calls to irq_create_mapping() are totally fine.

Sorry, perhaps I wasn't clear.

<mfd_parent_device>_probe()
|
+-> mfd_add_device()
    |
    +-> res->start = res->end = irq_create_mapping(res->start); // Now a VIRQ


<mfd_child_device>_probe()
|
+-> <mfd_parent_device>_irq_get_virq(res->start) // Ahhh, double VIRQ conversion

To stop being DT dependent we'd need to remove all hand-rolled stuff 
that these drivers are currently using, or else they will be attempting
to convert and already converted VIRQ value. 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Mon, Aug 20, 2012 at 05:49:50PM +0100, Lee Jones wrote:

On Mon, Aug 20, 2012 at 05:29:23PM +0100, Mark Brown wrote:

quoted

All of the regmap devices could use this.

Only if they have linear domains and don't support DT.

Neither of those restrictions really apply...

If they don't have linear domains there's no point, if they support DT
then they can use it as it is.

All this stuff just works for any IRQ domain type, there's no
requirement for a particular one.  It's not urgently exciting for legacy
domains but it's not harmful either and pushes all the handling of this
stuff out of the MFD core and into the irqdomain code which is
definitely an abstraction win.

As far as DT goes supporting DT isn't enough, the current code will only
work on systems that actively use DT and do so using a particular style
of binding.  Since the majority of Linux architectures don't support DT
at all and it's not universally deployed on those that do this means
that generic drivers can't rely on it, and even drivers that use DT
might not be using the binding pattern which the framework code now
uses.

Indeed now that I think about the above isn't this going to be actively
harmful for generic drivers if they use this pattern for their bindings?
On DT systems the framework will unconditionally do the mapping but on
others no mapping will be done.  The function drivers don't know if the
mapping has been performed or not.

Unless I'm missing something here we need to fix this before the merge
window...

To stop being DT dependent we'd need to remove all hand-rolled stuff 
that these drivers are currently using, or else they will be attempting
to convert and already converted VIRQ value. 

Well, yes - using framework code would be the goal here...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120820/febc6aa3/attachment.sig>

quoted

If they don't have linear domains there's no point, if they support DT
then they can use it as it is.

All this stuff just works for any IRQ domain type, there's no
requirement for a particular one.  It's not urgently exciting for legacy
domains but it's not harmful either and pushes all the handling of this
stuff out of the MFD core and into the irqdomain code which is
definitely an abstraction win.

Wherever we do this from to be able to obtain the IRQ domain pointer, 
which is where I'm currently struggling. Our options are:

- Call into a helper function based in the IRQ controller from each 
child device. In turn the IRQ controller will be responsible for creating
the mapping necessary to obtain a virq. Using this method the child
device doesn't need to know if we're using an IRQ domain or not, or
whether we're using Device Tree or not. The drawback is that each child
device will be required to call the helper function prior to requesting
an IRQ. 

- If we're only talking MFD here, we can handle this stuff in the MFD
core, but we need more information. The IRQ domain subsystem only allows
domain look-up via a Device Tree node, so we need to get our hands on
the domain another way in the case of non-DT enabled devices. Either we
add another parameter to mfd_add_device(irq_domain, ...), or we
standardise the 'irq_domain' variable name and use:
        irq_domain = container_of(parent, struct irq_domain, irq_domain);

- I know that you have interest in pushing the functionality into the
IRQ domain subsystem, but I'm struggling to see how. It's calling into
the IRQ domain where we're seeing issues in the first place, specifically
irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
when requesting the IRQ? That way we can pass the correct IRQ without
worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
can do the necessary conversions. If 'irq_domain' is NULL it continues to
use the requested IRQ as a virq.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Aug 14, 2012 at 10:31:08AM +0200, Linus Walleij wrote:

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

There was a plan to place ab8500_irq_get_virq() calls in each AB8500
child device prior to requesting an IRQ, but as we're no longer using
Device Tree to collect our IRQ numbers, it's actually better to allow
the core to do this during device registration time. So the IRQ number
we pull from its resource has already been converted to a virtual IRQ.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: linux-input at vger.kernel.org
Signed-off-by: Lee Jones <redacted>

This is looking good, I guess you need all patches to go in at the
same time so Dmitry's ACK is required.

Yep, just waiting for that now.
 

FWIW:
Acked-by: Linus Walleij <redacted>

BTW: this makes me suspect that the public ab8500_irq_get_virq()
interface can be *deleted* and the function made static in the
AB8500 driver, right?

Right. Already taken care of.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Aug 21, 2012 at 09:56:19AM +0100, Lee Jones wrote:

Wherever we do this from to be able to obtain the IRQ domain pointer, 
which is where I'm currently struggling. Our options are:

- If we're only talking MFD here, we can handle this stuff in the MFD
core, but we need more information. The IRQ domain subsystem only allows
domain look-up via a Device Tree node, so we need to get our hands on

What makes you say this?  This is just a convenience for finding a
domain, irqdomains are *completely* indepentant of device tree.

the domain another way in the case of non-DT enabled devices. Either we
add another parameter to mfd_add_device(irq_domain, ...), or we
standardise the 'irq_domain' variable name and use:
        irq_domain = container_of(parent, struct irq_domain, irq_domain);

Passing the domain into mfd seems like the obvious solution here - it's
exactly what we currently do for legacy stuff (where we pass in the irq
base), ideally what we'd end up with over time is that we could just
remove the irq_base parameter entirely as everything would in time be
moved over to using irqdomains.

- I know that you have interest in pushing the functionality into the
IRQ domain subsystem, but I'm struggling to see how. It's calling into
the IRQ domain where we're seeing issues in the first place, specifically
irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
when requesting the IRQ? That way we can pass the correct IRQ without
worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
can do the necessary conversions. If 'irq_domain' is NULL it continues to
use the requested IRQ as a virq.

This is totally orthogonal to doing the mapping in the MFD subsystem
which is the issue here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120821/c4d1369d/attachment.sig>

On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote:

On Tue, Aug 21, 2012 at 09:56:19AM +0100, Lee Jones wrote:

quoted

Wherever we do this from to be able to obtain the IRQ domain pointer, 
which is where I'm currently struggling. Our options are:

quoted

- If we're only talking MFD here, we can handle this stuff in the MFD
core, but we need more information. The IRQ domain subsystem only allows
domain look-up via a Device Tree node, so we need to get our hands on

What makes you say this?  This is just a convenience for finding a
domain, irqdomains are *completely* indepentant of device tree.

How can you say that? I think you mean _can_ be independent of DT. If
that's what you mean then yes, that's true. All I'm saying is we need
another way to get hold of the domain, because the only way to obtain
it without having direct access is via a device node.
 

quoted

the domain another way in the case of non-DT enabled devices. Either we
add another parameter to mfd_add_device(irq_domain, ...), or we
standardise the 'irq_domain' variable name and use:
        irq_domain = container_of(parent, struct irq_domain, irq_domain);

Passing the domain into mfd seems like the obvious solution here - it's
exactly what we currently do for legacy stuff (where we pass in the irq
base), ideally what we'd end up with over time is that we could just
remove the irq_base parameter entirely as everything would in time be
moved over to using irqdomains.

Right. That's fine (and easy) then. You threw me when you said you wanted
it handled higher-up in the framework, as I didn't see how we could do 
this in the irqdomain itself.
 

quoted

- I know that you have interest in pushing the functionality into the
IRQ domain subsystem, but I'm struggling to see how. It's calling into
the IRQ domain where we're seeing issues in the first place, specifically
irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
when requesting the IRQ? That way we can pass the correct IRQ without
worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
can do the necessary conversions. If 'irq_domain' is NULL it continues to
use the requested IRQ as a virq.

This is totally orthogonal to doing the mapping in the MFD subsystem
which is the issue here.

Again, I only mentioned this because you said you wanted it to be handled
by the irqdomain.

I'll code up the second suggestion now.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Aug 21, 2012 at 11:54:14AM +0100, Lee Jones wrote:

On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote:

quoted

What makes you say this?  This is just a convenience for finding a
domain, irqdomains are *completely* indepentant of device tree.

How can you say that? I think you mean _can_ be independent of DT. If
that's what you mean then yes, that's true. All I'm saying is we need

No, I really mean what I'm saying.  Device tree builds on irqdomains,
not the other way around.

another way to get hold of the domain, because the only way to obtain
it without having direct access is via a device node.

This doesn't actually hold.

quoted

quoted

- I know that you have interest in pushing the functionality into the
IRQ domain subsystem, but I'm struggling to see how. It's calling into
the IRQ domain where we're seeing issues in the first place, specifically
irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
when requesting the IRQ? That way we can pass the correct IRQ without
worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
can do the necessary conversions. If 'irq_domain' is NULL it continues to
use the requested IRQ as a virq.

quoted

This is totally orthogonal to doing the mapping in the MFD subsystem
which is the issue here.

Again, I only mentioned this because you said you wanted it to be handled
by the irqdomain.

The *mapping* should be being handled in irqdomain.

I'll code up the second suggestion now.

I've already done this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120821/ba3c2e95/attachment.sig>

On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote:

On Tue, Aug 21, 2012 at 11:54:14AM +0100, Lee Jones wrote:

quoted

On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote:

quoted

quoted

What makes you say this?  This is just a convenience for finding a
domain, irqdomains are *completely* indepentant of device tree.

quoted

How can you say that? I think you mean _can_ be independent of DT. If
that's what you mean then yes, that's true. All I'm saying is we need

No, I really mean what I'm saying.  Device tree builds on irqdomains,
not the other way around.

This is just semantics.
 

quoted

another way to get hold of the domain, because the only way to obtain
it without having direct access is via a device node.

This doesn't actually hold.

Okay, besides irq_find_host(struct device_node *np), how else can you fetch
a domain from the irqdomain?

quoted

quoted

quoted

- I know that you have interest in pushing the functionality into the
IRQ domain subsystem, but I'm struggling to see how. It's calling into
the IRQ domain where we're seeing issues in the first place, specifically
irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
when requesting the IRQ? That way we can pass the correct IRQ without
worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
can do the necessary conversions. If 'irq_domain' is NULL it continues to
use the requested IRQ as a virq.

quoted

quoted

This is totally orthogonal to doing the mapping in the MFD subsystem
which is the issue here.

quoted

Again, I only mentioned this because you said you wanted it to be handled
by the irqdomain.

The *mapping* should be being handled in irqdomain.

quoted

I'll code up the second suggestion now.

I've already done this.

What have you done already? 

Why make suggestions if you're just going to do the work yourself?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Aug 21, 2012 at 10:23:29AM +0100, Lee Jones wrote:

On Tue, Aug 14, 2012 at 10:31:08AM +0200, Linus Walleij wrote:

quoted

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

There was a plan to place ab8500_irq_get_virq() calls in each AB8500
child device prior to requesting an IRQ, but as we're no longer using
Device Tree to collect our IRQ numbers, it's actually better to allow
the core to do this during device registration time. So the IRQ number
we pull from its resource has already been converted to a virtual IRQ.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: linux-input at vger.kernel.org
Signed-off-by: Lee Jones <redacted>

This is looking good, I guess you need all patches to go in at the
same time so Dmitry's ACK is required.

Yep, just waiting for that now.

Sorry for the delay. Yes, this shoudl be fine, but since it is
essentially a revert of the original patch it should be pushed in as
such.

Thanks.

-- 
Dmitry

On Tue, Aug 21, 2012 at 01:02:46PM +0100, Lee Jones wrote:

On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote:

quoted

quoted

another way to get hold of the domain, because the only way to obtain
it without having direct access is via a device node.

quoted

This doesn't actually hold.

Okay, besides irq_find_host(struct device_node *np), how else can you fetch
a domain from the irqdomain?

I'm not sure I can parse the above, sorry - I'm not sure I can
distinguish "domain" and "irqdomain".

What have you done already? 

Implemented a patch for this which I've now tested a bit and will
probably post in the next hour or so.

Why make suggestions if you're just going to do the work yourself?

I made the suggestion then later on realised that this was actively
going to break things I care about so I actually need it fixing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120821/7a73e216/attachment.sig>

On Tue, Aug 21, 2012 at 05:52:07PM +0100, Mark Brown wrote:

On Tue, Aug 21, 2012 at 01:02:46PM +0100, Lee Jones wrote:

quoted

On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote:

quoted

quoted

quoted

another way to get hold of the domain, because the only way to obtain
it without having direct access is via a device node.

quoted

quoted

This doesn't actually hold.

quoted

Okay, besides irq_find_host(struct device_node *np), how else can you fetch
a domain from the irqdomain?

I'm not sure I can parse the above, sorry - I'm not sure I can
distinguish "domain" and "irqdomain".

Are you being intentionally awkward on this? Picking holes when you know
exactly what I'm trying to say? Not that it matters now, but just out of
principle, let me try to put it in a very clear way so there can be no
misinterpretation.

I was saying that in order for the MFD core to carry out the hwirq->virq
conversion, it needed to obtain the irqdomain pointer pertaining to the
provided hwirq. The only helper function the irqdomain subsystem provides
requires a device node pointer to be passed as an argument, hence the
mention of 'irq_find_host(struct device_node *np)'. Then the Device Tree
is traversed until a specified 'interrupt-controller' is stumbled upon
or is pointed to by the 'interrupt-parent' property. Hence, we have to 
find another way to find the irqdomain pointers for non-DT based MFDs. To
which we now have a solution.

quoted

What have you done already? 

Implemented a patch for this which I've now tested a bit and will
probably post in the next hour or so.

quoted

Why make suggestions if you're just going to do the work yourself?

I made the suggestion then later on realised that this was actively
going to break things I care about so I actually need it fixing.

I'm a little taken aback and annoyed by this. In a previous email thread
you categorically requested that I discuss some of the important changes
with maintainers and people in-the-know prior to actually writing any
code. I was obviously actively working on, had put time into, and was in
the mist of discussing this with you. Then you just go ahead and code it
(the easy part) yourself, essentially wasting my time. Surely there's
some kind of etiquette surrounding such things?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Wed, Aug 22, 2012 at 09:17:50AM +0100, Lee Jones wrote:

I was saying that in order for the MFD core to carry out the hwirq->virq
conversion, it needed to obtain the irqdomain pointer pertaining to the
provided hwirq. The only helper function the irqdomain subsystem provides
requires a device node pointer to be passed as an argument, hence the
mention of 'irq_find_host(struct device_node *np)'. Then the Device Tree
is traversed until a specified 'interrupt-controller' is stumbled upon
or is pointed to by the 'interrupt-parent' property. Hence, we have to 
find another way to find the irqdomain pointers for non-DT based MFDs. To
which we now have a solution.

Oh, right.  Yes, there's no way to get an irqdomain if you don't already
have it or something which has a direct mapping to one like an irqdomain.

quoted

I made the suggestion then later on realised that this was actively
going to break things I care about so I actually need it fixing.

I'm a little taken aback and annoyed by this. In a previous email thread
you categorically requested that I discuss some of the important changes
with maintainers and people in-the-know prior to actually writing any
code.

No, that's not something I've ever said to do.

I *have* asked you to communicate more clearly about what you're doing
but that doesn't mean to stop sending code, it means to have clearer
words around what you're sending.  The really bad pattern here is that
you're frequently working around issues in your drivers with changes in
the subsystem without mentioning that the driver issues even exist -
this makes it much harder understand what you are trying to achieve,
especially when there is a problem with your subsystem changes and/or
the urgency you're attaching to them.

      I was obviously actively working on, had put time into, and was in
the mist of discussing this with you. Then you just go ahead and code it
(the easy part) yourself, essentially wasting my time. Surely there's
some kind of etiquette surrounding such things?

To be honest in this case I had expected to send the patch out much
sooner than I did - several priority interrupts stopped me testing it.
Like I say I realised that I really needed a fix and it seemed like the
quickest way to accomplish that was to just send the code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120822/7e96b246/attachment.sig>

quoted

quoted

I made the suggestion then later on realised that this was actively
going to break things I care about so I actually need it fixing.

quoted

I'm a little taken aback and annoyed by this. In a previous email thread
you categorically requested that I discuss some of the important changes
with maintainers and people in-the-know prior to actually writing any
code.

No, that's not something I've ever said to do.

I *have* asked you to communicate more clearly about what you're doing
but that doesn't mean to stop sending code, it means to have clearer
words around what you're sending. 

That's not how I interpreted your words:

"What you can do here is to commmunicate about what you're doing more.
Don't just think about the code, think about the communication
surrounding the code - this is the core of the issue."

The really bad pattern here is that
you're frequently working around issues in your drivers with changes in
the subsystem without mentioning that the driver issues even exist -
this makes it much harder understand what you are trying to achieve,
especially when there is a problem with your subsystem changes and/or
the urgency you're attaching to them.

Frequently? I've done this once, and yes I did push hard because I
thought the subsystem was incorrect (I still think folding an entire
driver because of a minor failure is wrong, but we digress).

This is completely different to that. This was a subsystem change
designed to aid DT enabled MFDs which 'chose' to register themselves
in a specific way, by passing their compatible string through the 
mfd_cell only. It doesn't affect any other MFD unless they wrongly
assume the conversion would be automatically done for them. However,
the author would know because they would have tested it. All of this
was discussed at length with Arnd and this is what we came up with. 

I think it's great that you like the idea and want to extend that
functionality to other MFDs which perhaps don't support DT, or the
ones that do but don't want to provide compatible strings or device
nodes for all the MFD's child devices. But that is all we're doing
here. There was no breakage. It served a purpose and it worked well.
So well in fact that you've now provided the intended functionality
to other devices.

quoted

      I was obviously actively working on, had put time into, and was in
the mist of discussing this with you. Then you just go ahead and code it
(the easy part) yourself, essentially wasting my time. Surely there's
some kind of etiquette surrounding such things?

To be honest in this case I had expected to send the patch out much
sooner than I did - several priority interrupts stopped me testing it.
Like I say I realised that I really needed a fix and it seemed like the
quickest way to accomplish that was to just send the code.

You only noticed it 2 days ago and I had a patch ready to go yesterday.
I'm confused because I don't understand why would you even complain about
it if you intended to work on it yourself? Surely, "Ah, I see an issue with
xyz, patch to follow." Would have been more appropriate, instead of 
complaining about it, then I go and waste my time trying to fix something
you intend on fixing yourself.

I guess what's done is done now.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Wed, Aug 22, 2012 at 12:55:25PM +0100, Lee Jones wrote:

quoted

I *have* asked you to communicate more clearly about what you're doing
but that doesn't mean to stop sending code, it means to have clearer
words around what you're sending. 

That's not how I interpreted your words:

"What you can do here is to commmunicate about what you're doing more.
Don't just think about the code, think about the communication
surrounding the code - this is the core of the issue."

Just to be clear I'd include things like commit messages, cover letters
and so on in the general area of communication.

I think it's great that you like the idea and want to extend that
functionality to other MFDs which perhaps don't support DT, or the
ones that do but don't want to provide compatible strings or device
nodes for all the MFD's child devices. But that is all we're doing
here. There was no breakage. It served a purpose and it worked well.
So well in fact that you've now provided the intended functionality
to other devices.

I'm looking at this from the point of view of adding the compatible
strings and then finding that the core starts remapping things in the
case where you're probing from DT - and this behaviour will vary
depending on the device tree that the user is using so the driver can't
even make a decision based on if device tree is being used by the
system.

You only noticed it 2 days ago and I had a patch ready to go yesterday.
I'm confused because I don't understand why would you even complain about
it if you intended to work on it yourself? Surely, "Ah, I see an issue with
xyz, patch to follow." Would have been more appropriate, instead of 
complaining about it, then I go and waste my time trying to fix something
you intend on fixing yourself.

I didn't originally intend to do anything, if I had done I'd just have
sent a patch as you say.  Originally I'd just noticed it as being an
awkwardly designed interface at the wrong abstraction layer, it was only
later on that I realised how it could blow up.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120822/d3e74d63/attachment-0001.sig>

Sorry for the delay. Yes, this shoudl be fine, but since it is
essentially a revert of the original patch it should be pushed in as
such.

How's this?

Author: Lee Jones [off-list ref]
Date:   Thu Aug 30 14:08:19 2012 +0100

    Revert "input: ab8500-ponkey: Create AB8500 domain IRQ mapping"
    
    This reverts commit ca3b3faf9bee4dc5df4f10eae2d1e48f7de0a8ad.
    
    There was a plan to place ab8500_irq_get_virq() calls in each AB8500
    child device prior to requesting an IRQ, but as we're no longer using
    Device Tree to collect our IRQ numbers, it's actually better to allow
    the core to do this during device registration time. So the IRQ number
    we pull from its resource has already been converted to a virtual IRQ.
    
    CC: Dmitry Torokhov [off-list ref]
    CC: linux-input at vger.kernel.org
    Acked-by: Linus Walleij [off-list ref]
    Signed-off-by: Lee Jones [off-list ref]

diff --git a/drivers/input/misc/ab8500-ponkey.c b/drivers/input/misc/ab8500-ponkey.c
index f06231b..84ec691 100644
--- a/drivers/input/misc/ab8500-ponkey.c
+++ b/drivers/input/misc/ab8500-ponkey.c

@@ -74,8 +74,8 @@ static int __devinit ab8500_ponkey_probe(struct platform_device *pdev)
 
        ponkey->idev = input;
        ponkey->ab8500 = ab8500;
-       ponkey->irq_dbf = ab8500_irq_get_virq(ab8500, irq_dbf);
-       ponkey->irq_dbr = ab8500_irq_get_virq(ab8500, irq_dbr);
+       ponkey->irq_dbf = irq_dbf;
+       ponkey->irq_dbr = irq_dbr;
 
        input->name = "AB8500 POn(PowerOn) Key";
        input->dev.parent = &pdev->dev;

On Thu, Aug 30, 2012 at 02:12:04PM +0100, Lee Jones wrote:

quoted

Sorry for the delay. Yes, this shoudl be fine, but since it is
essentially a revert of the original patch it should be pushed in as
such.

How's this?

Excellent.

Author: Lee Jones [off-list ref]
Date:   Thu Aug 30 14:08:19 2012 +0100

    Revert "input: ab8500-ponkey: Create AB8500 domain IRQ mapping"
    
    This reverts commit ca3b3faf9bee4dc5df4f10eae2d1e48f7de0a8ad.
    
    There was a plan to place ab8500_irq_get_virq() calls in each AB8500
    child device prior to requesting an IRQ, but as we're no longer using
    Device Tree to collect our IRQ numbers, it's actually better to allow
    the core to do this during device registration time. So the IRQ number
    we pull from its resource has already been converted to a virtual IRQ.
    
    CC: Dmitry Torokhov [off-list ref]
    CC: linux-input at vger.kernel.org
    Acked-by: Linus Walleij [off-list ref]

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

-- 
Dmitry

On Thu, Aug 30, 2012 at 04:02:21PM -0700, Dmitry Torokhov wrote:

On Thu, Aug 30, 2012 at 02:12:04PM +0100, Lee Jones wrote:

quoted

quoted

Sorry for the delay. Yes, this shoudl be fine, but since it is
essentially a revert of the original patch it should be pushed in as
such.

How's this?

Excellent.

I assume you will be merging it with the rest of AB8500 patches, right?

-- 
Dmitry

On Thu, Aug 30, 2012 at 04:03:23PM -0700, Dmitry Torokhov wrote:

On Thu, Aug 30, 2012 at 04:02:21PM -0700, Dmitry Torokhov wrote:

quoted

On Thu, Aug 30, 2012 at 02:12:04PM +0100, Lee Jones wrote:

quoted

quoted

Sorry for the delay. Yes, this shoudl be fine, but since it is
essentially a revert of the original patch it should be pushed in as
such.

How's this?

Excellent.

I assume you will be merging it with the rest of AB8500 patches, right?

Yes, if that's okay with you of course?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Aug 14, 2012 at 10:19:08AM +0200, Linus Walleij wrote:

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

irq_find_host() currently ignores the 'interrupt-parent' property
even if it's specified in the Device Tree. Meaning that a node can
match to a domain in its hierarchy even if it doesn't belong to it.
By searching for the parent first using of_irq_find_parent() we
insist that the 'interrupt-parent' property is taken into account
ensuring a greater chance of returning the correct domain.

CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Grant Likely <redacted>
Signed-off-by: Lee Jones <redacted>

This (with 1/8) is the right solution. Thanks, and I think
Mark may want to look at this too since I recognize he asked
you to work in this direction.

Acked-by: Linus Walleij <redacted>

Rob, did this get your Ack too?

Ben, Grant, Mark, would you like to take a look also?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On 08/31/2012 04:44 AM, Lee Jones wrote:

On Tue, Aug 14, 2012 at 10:19:08AM +0200, Linus Walleij wrote:

quoted

On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones [off-list ref] wrote:

quoted

irq_find_host() currently ignores the 'interrupt-parent' property
even if it's specified in the Device Tree. Meaning that a node can
match to a domain in its hierarchy even if it doesn't belong to it.
By searching for the parent first using of_irq_find_parent() we
insist that the 'interrupt-parent' property is taken into account
ensuring a greater chance of returning the correct domain.

CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Grant Likely <redacted>
Signed-off-by: Lee Jones <redacted>

This (with 1/8) is the right solution. Thanks, and I think
Mark may want to look at this too since I recognize he asked
you to work in this direction.

Acked-by: Linus Walleij <redacted>

Rob, did this get your Ack too?

Acked-by: Rob Herring <redacted>

Rob

Ben, Grant, Mark, would you like to take a look also?

On Fri, Aug 31, 2012 at 08:31:33AM +0100, Lee Jones wrote:

On Thu, Aug 30, 2012 at 04:03:23PM -0700, Dmitry Torokhov wrote:

quoted

On Thu, Aug 30, 2012 at 04:02:21PM -0700, Dmitry Torokhov wrote:

quoted

On Thu, Aug 30, 2012 at 02:12:04PM +0100, Lee Jones wrote:

quoted

quoted

Sorry for the delay. Yes, this shoudl be fine, but since it is
essentially a revert of the original patch it should be pushed in as
such.

How's this?

Excellent.

I assume you will be merging it with the rest of AB8500 patches, right?

Yes, if that's okay with you of course?

Sure, please go ahead,

Thanks.

-- 
Dmitry

Hi Lee,

On Thu, Aug 09, 2012 at 04:53:54PM +0100, Lee Jones wrote:

Before the AB8500 had its own IRQ domain, the IRQ handler would take
the fired local IRQ (hwirq) and add it to the irq_base to convert it
to an IRQ number which Linux would understand (virq). However, the
IRQ base is not always used anymore since we can make use of Linear
domains. It's better to use the AB8500 hwirq -> virq mapping helper
function to convert them instead. That's what we do here.

CC: Samuel Ortiz <redacted>
Signed-off-by: Lee Jones <redacted>
---
 drivers/mfd/ab8500-core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch applied now, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/