linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use
@ 2012-01-25  0:18 Grant Likely
  2012-01-25  0:18 ` [RFC 2/2] irqdomain/powerpc: Replace custom xlate functions with library functions Grant Likely
  2012-01-25  1:50 ` [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Rob Herring
  0 siblings, 2 replies; 7+ messages in thread
From: Grant Likely @ 2012-01-25  0:18 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, linuxppc-dev
  Cc: Grant Likely, Rob Herring, Benjamin Herrenschmidt

Rather than having each interrupt controller driver creating its own barely
unique .xlate function for irq_domain, create a library of translators which
any driver can use directly.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

This builds on top of my irq_domain series and will be included with it when
I post v3.  Rob, does this serve your needs for generic irq controller DT
support?

g.

 include/linux/irqdomain.h |   15 ++++++++++
 kernel/irq/irqdomain.c    |   69 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index e7379a3..5e497a0 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -110,6 +110,9 @@ struct irq_domain {
 	void *host_data;
 	irq_hw_number_t inval_irq;
 
+	/* Data for common irq xlate functions */
+	unsigned int xlate_type;
+
 	/* Optional device node pointer */
 	struct device_node *of_node;
 };
@@ -163,6 +166,18 @@ extern unsigned int irq_linear_revmap(struct irq_domain *host,
 				      irq_hw_number_t hwirq);
 
 extern struct irq_domain_ops irq_domain_simple_ops;
+
+/* stock xlate functions */
+int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
+			const u32 *intspec, unsigned int intsize,
+			irq_hw_number_t *out_hwirq, unsigned int *out_type);
+int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
+			const u32 *intspec, unsigned int intsize,
+			irq_hw_number_t *out_hwirq, unsigned int *out_type);
+int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
+			const u32 *intspec, unsigned int intsize,
+			irq_hw_number_t *out_hwirq, unsigned int *out_type);
+
 #if defined(CONFIG_OF_IRQ)
 extern void irq_domain_generate_simple(const struct of_device_id *match,
 					u64 phys_base, unsigned int irq_start);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index def8e7b..df80205 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -53,6 +53,7 @@ static struct irq_domain *irq_domain_alloc(struct device_node *of_node,
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->of_node = of_node_get(of_node);
+	domain->xlate_type = IRQ_TYPE_NONE;
 
 	if (domain->ops->match == NULL)
 		domain->ops->match = default_irq_domain_match;
@@ -690,25 +691,77 @@ int irq_domain_simple_map(struct irq_domain *d, unsigned int irq,
 	return 0;
 }
 
-int irq_domain_simple_xlate(struct irq_domain *d,
-			    struct device_node *controller,
-			    const u32 *intspec, unsigned int intsize,
-			    unsigned long *out_hwirq, unsigned int *out_type)
+/**
+ * irq_domain_xlate_onecell() - Generic xlate for direct one cell bindings
+ *
+ * Device Tree IRQ specifier translation function which works with one cell
+ * bindings where the cell value maps directly to the hwirq number.
+ */
+int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
+			     const u32 *intspec, unsigned int intsize,
+			     unsigned long *out_hwirq, unsigned int *out_type)
+{
+	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
+		 ctrlr->full_name, intsize))
+		return -EINVAL;
+	*out_hwirq = intspec[0];
+	*out_type = d->xlate_type;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
+
+/**
+ * irq_domain_xlate_twocell() - Generic xlate for direct two cell bindings
+ *
+ * Device Tree IRQ specifier translation function which works with two cell
+ * bindings where the cell values map directly to the hwirq number
+ * and linux irq flags.
+ */
+int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
+			const u32 *intspec, unsigned int intsize,
+			irq_hw_number_t *out_hwirq, unsigned int *out_type)
 {
-	if (d->of_node != controller)
+	if (WARN(intsize < 2, "Bad intspec for %s: intsize=%i < 2\n",
+		 ctrlr->full_name, intsize))
+		return -EINVAL;
+	if (intsize < 2)
 		return -EINVAL;
-	if (intsize < 1)
+	*out_hwirq = intspec[0];
+	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
+
+/**
+ * irq_domain_xlate_onetwocell() - Generic xlate for one or two cell bindings
+ *
+ * Device Tree IRQ specifier translation function which works with either one
+ * or two cell bindings where the cell values map directly to the hwirq number
+ * and linux irq flags.
+ *
+ * Note: don't use this function unless your interrupt controller explicitly
+ * supports both one and two cell bindings.  For the majority of controllers
+ * the _onecell() or _twocell() variants above should be used.
+ */
+int irq_domain_xlate_onetwocell(struct irq_domain *d,
+				struct device_node *ctrlr,
+				const u32 *intspec, unsigned int intsize,
+				unsigned long *out_hwirq, unsigned int *out_type)
+{
+	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
+		 ctrlr->full_name, intsize))
 		return -EINVAL;
 	*out_hwirq = intspec[0];
-	*out_type = IRQ_TYPE_NONE;
+	*out_type = d->xlate_type;
 	if (intsize > 1)
 		*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);
 
 struct irq_domain_ops irq_domain_simple_ops = {
 	.map = irq_domain_simple_map,
-	.xlate = irq_domain_simple_xlate,
+	.xlate = irq_domain_xlate_onetwocell,
 };
 EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
 
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC 2/2] irqdomain/powerpc: Replace custom xlate functions with library functions
  2012-01-25  0:18 [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Grant Likely
@ 2012-01-25  0:18 ` Grant Likely
  2012-01-25  1:50 ` [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Rob Herring
  1 sibling, 0 replies; 7+ messages in thread
From: Grant Likely @ 2012-01-25  0:18 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, linuxppc-dev
  Cc: Grant Likely, Rob Herring, Benjamin Herrenschmidt

This patch converts a number of the powerpc drivers to use the common library
of irq_domain xlate functions, dropping a bunch of lines in the process.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

This builds on top of my irq_domain series and will be included with it when
I post v3.  **completely untested**.  I will do some due diligence before I
post it again.  Ben, if you're okay with this approach, then I'll make it
part of the irq_domain series.

g.

 arch/powerpc/platforms/86xx/gef_pic.c  |   17 ++---------------
 arch/powerpc/platforms/powermac/pic.c  |   13 +------------
 arch/powerpc/platforms/wsp/opb_pic.c   |   13 +------------
 arch/powerpc/sysdev/cpm2_pic.c         |   14 +-------------
 arch/powerpc/sysdev/ipic.c             |   18 +-----------------
 arch/powerpc/sysdev/qe_lib/qe_ic.c     |   15 +--------------
 arch/powerpc/sysdev/tsi108_pci.c       |   12 ++----------
 arch/powerpc/sysdev/uic.c              |   14 +-------------
 arch/powerpc/sysdev/xics/xics-common.c |   24 ++++++++----------------
 drivers/gpio/gpio-mpc8xxx.c            |   17 +----------------
 10 files changed, 19 insertions(+), 138 deletions(-)

diff --git a/arch/powerpc/platforms/86xx/gef_pic.c b/arch/powerpc/platforms/86xx/gef_pic.c
index 126a94b..de614b1 100644
--- a/arch/powerpc/platforms/86xx/gef_pic.c
+++ b/arch/powerpc/platforms/86xx/gef_pic.c
@@ -163,23 +163,9 @@ static int gef_pic_host_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int gef_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
-			    const u32 *intspec, unsigned int intsize,
-			    irq_hw_number_t *out_hwirq, unsigned int *out_flags)
-{
-
-	*out_hwirq = intspec[0];
-	if (intsize > 1)
-		*out_flags = intspec[1];
-	else
-		*out_flags = IRQ_TYPE_LEVEL_HIGH;
-
-	return 0;
-}
-
 static struct irq_domain_ops gef_pic_host_ops = {
 	.map	= gef_pic_host_map,
-	.xlate	= gef_pic_host_xlate,
+	.xlate	= irq_domain_xlate_onetwocell,
 };
 
 
@@ -216,6 +202,7 @@ void __init gef_pic_init(struct device_node *np)
 					  &gef_pic_host_ops, NULL);
 	if (gef_pic_irq_host == NULL)
 		return;
+	gef_pic_irq_host->xlate_default_type = IRQ_TYPE_LEVEL_HIGH;
 
 	/* Chain with parent controller */
 	irq_set_chained_handler(gef_pic_cascade_irq, gef_pic_cascade);
diff --git a/arch/powerpc/platforms/powermac/pic.c b/arch/powerpc/platforms/powermac/pic.c
index 646fdf3..576cb32 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -288,21 +288,10 @@ static int pmac_pic_host_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int pmac_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
-			       const u32 *intspec, unsigned int intsize,
-			       irq_hw_number_t *out_hwirq,
-			       unsigned int *out_flags)
-
-{
-	*out_flags = IRQ_TYPE_NONE;
-	*out_hwirq = *intspec;
-	return 0;
-}
-
 static struct irq_domain_ops pmac_pic_host_ops = {
 	.match = pmac_pic_host_match,
 	.map = pmac_pic_host_map,
-	.xlate = pmac_pic_host_xlate,
+	.xlate = irq_domain_xlate_onecell;
 };
 
 static void __init pmac_pic_probe_oldstyle(void)
diff --git a/arch/powerpc/platforms/wsp/opb_pic.c b/arch/powerpc/platforms/wsp/opb_pic.c
index 4837515..8bd4136 100644
--- a/arch/powerpc/platforms/wsp/opb_pic.c
+++ b/arch/powerpc/platforms/wsp/opb_pic.c
@@ -196,20 +196,9 @@ static int opb_host_map(struct irq_domain *host, unsigned int virq,
 	return 0;
 }
 
-static int opb_host_xlate(struct irq_domain *host, struct device_node *dn,
-		const u32 *intspec, unsigned int intsize,
-		irq_hw_number_t *out_hwirq, unsigned int *out_type)
-{
-	/* Interrupt size must == 2 */
-	BUG_ON(intsize != 2);
-	*out_hwirq = intspec[0];
-	*out_type = intspec[1];
-	return 0;
-}
-
 static struct irq_domain_ops opb_host_ops = {
 	.map = opb_host_map,
-	.xlate = opb_host_xlate,
+	.xlate = irq_domain_xlate_twocell,
 };
 
 irqreturn_t opb_irq_handler(int irq, void *private)
diff --git a/arch/powerpc/sysdev/cpm2_pic.c b/arch/powerpc/sysdev/cpm2_pic.c
index b364332..1430201 100644
--- a/arch/powerpc/sysdev/cpm2_pic.c
+++ b/arch/powerpc/sysdev/cpm2_pic.c
@@ -224,21 +224,9 @@ static int cpm2_pic_host_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int cpm2_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
-			    const u32 *intspec, unsigned int intsize,
-			    irq_hw_number_t *out_hwirq, unsigned int *out_flags)
-{
-	*out_hwirq = intspec[0];
-	if (intsize > 1)
-		*out_flags = intspec[1];
-	else
-		*out_flags = IRQ_TYPE_NONE;
-	return 0;
-}
-
 static struct irq_domain_ops cpm2_pic_host_ops = {
 	.map = cpm2_pic_host_map,
-	.xlate = cpm2_pic_host_xlate,
+	.xlate = irq_domain_xlate_onetwocell,
 };
 
 void cpm2_pic_init(struct device_node *node)
diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
index 0eaaa01..b50f978 100644
--- a/arch/powerpc/sysdev/ipic.c
+++ b/arch/powerpc/sysdev/ipic.c
@@ -692,26 +692,10 @@ static int ipic_host_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int ipic_host_xlate(struct irq_domain *h, struct device_node *ct,
-			   const u32 *intspec, unsigned int intsize,
-			   irq_hw_number_t *out_hwirq, unsigned int *out_flags)
-
-{
-	/* interrupt sense values coming from the device tree equal either
-	 * LEVEL_LOW (low assertion) or EDGE_FALLING (high-to-low change)
-	 */
-	*out_hwirq = intspec[0];
-	if (intsize > 1)
-		*out_flags = intspec[1];
-	else
-		*out_flags = IRQ_TYPE_NONE;
-	return 0;
-}
-
 static struct irq_domain_ops ipic_host_ops = {
 	.match	= ipic_host_match,
 	.map	= ipic_host_map,
-	.xlate	= ipic_host_xlate,
+	.xlate	= irq_domain_xlate_onetwocell,
 };
 
 struct ipic * __init ipic_init(struct device_node *node, unsigned int flags)
diff --git a/arch/powerpc/sysdev/qe_lib/qe_ic.c b/arch/powerpc/sysdev/qe_lib/qe_ic.c
index e9b3d5c..2fba6ef 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_ic.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_ic.c
@@ -272,23 +272,10 @@ static int qe_ic_host_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int qe_ic_host_xlate(struct irq_domain *h, struct device_node *ct,
-			    const u32 * intspec, unsigned int intsize,
-			    irq_hw_number_t * out_hwirq,
-			    unsigned int *out_flags)
-{
-	*out_hwirq = intspec[0];
-	if (intsize > 1)
-		*out_flags = intspec[1];
-	else
-		*out_flags = IRQ_TYPE_NONE;
-	return 0;
-}
-
 static struct irq_domain_ops qe_ic_host_ops = {
 	.match = qe_ic_host_match,
 	.map = qe_ic_host_map,
-	.xlate = qe_ic_host_xlate,
+	.xlate = irq_domain_xlate_onetwocell,
 };
 
 /* Return an interrupt vector or NO_IRQ if no interrupt is pending. */
diff --git a/arch/powerpc/sysdev/tsi108_pci.c b/arch/powerpc/sysdev/tsi108_pci.c
index 188012c..4085ada 100644
--- a/arch/powerpc/sysdev/tsi108_pci.c
+++ b/arch/powerpc/sysdev/tsi108_pci.c
@@ -376,15 +376,6 @@ static struct irq_chip tsi108_pci_irq = {
 	.irq_unmask = tsi108_pci_irq_unmask,
 };
 
-static int pci_irq_host_xlate(struct irq_domain *h, struct device_node *ct,
-			    const u32 *intspec, unsigned int intsize,
-			    irq_hw_number_t *out_hwirq, unsigned int *out_flags)
-{
-	*out_hwirq = intspec[0];
-	*out_flags = IRQ_TYPE_LEVEL_HIGH;
-	return 0;
-}
-
 static int pci_irq_host_map(struct irq_domain *h, unsigned int virq,
 			  irq_hw_number_t hw)
 {	unsigned int irq;
@@ -399,7 +390,7 @@ static int pci_irq_host_map(struct irq_domain *h, unsigned int virq,
 
 static struct irq_domain_ops pci_irq_domain_ops = {
 	.map = pci_irq_host_map,
-	.xlate = pci_irq_host_xlate,
+	.xlate = irq_domain_xlate_onecell,
 };
 
 /*
@@ -424,6 +415,7 @@ void __init tsi108_pci_int_init(struct device_node *node)
 		printk(KERN_ERR "pci_irq_host: failed to allocate irq domain!\n");
 		return;
 	}
+	pci_irq_host->xlate_type = IRQ_TYPE_LEVEL_HIGH;
 
 	init_pci_source();
 }
diff --git a/arch/powerpc/sysdev/uic.c b/arch/powerpc/sysdev/uic.c
index 84e59c9..9203393 100644
--- a/arch/powerpc/sysdev/uic.c
+++ b/arch/powerpc/sysdev/uic.c
@@ -190,21 +190,9 @@ static int uic_host_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int uic_host_xlate(struct irq_domain *h, struct device_node *ct,
-			  const u32 *intspec, unsigned int intsize,
-			  irq_hw_number_t *out_hwirq, unsigned int *out_type)
-
-{
-	/* UIC intspecs must have 2 cells */
-	BUG_ON(intsize != 2);
-	*out_hwirq = intspec[0];
-	*out_type = intspec[1];
-	return 0;
-}
-
 static struct irq_domain_ops uic_host_ops = {
 	.map	= uic_host_map,
-	.xlate	= uic_host_xlate,
+	.xlate	= irq_domain_xlate_twocell,
 };
 
 void uic_irq_cascade(unsigned int virq, struct irq_desc *desc)
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
index ea5e204..754d22a 100644
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -351,31 +351,23 @@ static int xics_host_map(struct irq_domain *h, unsigned int virq,
 	return -EINVAL;
 }
 
-static int xics_host_xlate(struct irq_domain *h, struct device_node *ct,
-			   const u32 *intspec, unsigned int intsize,
-			   irq_hw_number_t *out_hwirq, unsigned int *out_flags)
-
-{
-	/* Current xics implementation translates everything
-	 * to level. It is not technically right for MSIs but this
-	 * is irrelevant at this point. We might get smarter in the future
-	 */
-	*out_hwirq = intspec[0];
-	*out_flags = IRQ_TYPE_LEVEL_LOW;
-
-	return 0;
-}
-
 static struct irq_domain_ops xics_host_ops = {
 	.match = xics_host_match,
 	.map = xics_host_map,
-	.xlate = xics_host_xlate,
+	.xlate = irq_domain_xlate_onecell,
 };
 
 static void __init xics_init_host(void)
 {
 	xics_host = irq_domain_add_tree(NULL, &xics_host_ops, NULL);
 	BUG_ON(xics_host == NULL);
+
+	/*
+	 * Current xics implementation translates everything to level. It is
+	 * not technically right for MSIs but this is irrelevant at this point.
+	 * We might get smarter in the future
+	 */
+	xics_host->xlate_type = IRQ_TYPE_LEVEL_LOW;
 	irq_set_default_host(xics_host);
 }
 
diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index 149d987..e6568c1 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -296,24 +296,9 @@ static int mpc8xxx_gpio_irq_map(struct irq_domain *h, unsigned int virq,
 	return 0;
 }
 
-static int mpc8xxx_gpio_irq_xlate(struct irq_domain *h, struct device_node *ct,
-				  const u32 *intspec, unsigned int intsize,
-				  irq_hw_number_t *out_hwirq,
-				  unsigned int *out_flags)
-
-{
-	/* interrupt sense values coming from the device tree equal either
-	 * EDGE_FALLING or EDGE_BOTH
-	 */
-	*out_hwirq = intspec[0];
-	*out_flags = intspec[1];
-
-	return 0;
-}
-
 static struct irq_domain_ops mpc8xxx_gpio_irq_ops = {
 	.map	= mpc8xxx_gpio_irq_map,
-	.xlate	= mpc8xxx_gpio_irq_xlate,
+	.xlate	= irq_domain_xlate_twocell,
 };
 
 static struct of_device_id mpc8xxx_gpio_ids[] __initdata = {
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use
  2012-01-25  0:18 [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Grant Likely
  2012-01-25  0:18 ` [RFC 2/2] irqdomain/powerpc: Replace custom xlate functions with library functions Grant Likely
@ 2012-01-25  1:50 ` Rob Herring
  2012-01-25  5:35   ` Grant Likely
  1 sibling, 1 reply; 7+ messages in thread
From: Rob Herring @ 2012-01-25  1:50 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-arm-kernel, linux-kernel, linuxppc-dev, Benjamin Herrenschmidt



On 01/24/2012 06:18 PM, Grant Likely wrote:
> Rather than having each interrupt controller driver creating its own barely
> unique .xlate function for irq_domain, create a library of translators which
> any driver can use directly.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> This builds on top of my irq_domain series and will be included with it when
> I post v3.  Rob, does this serve your needs for generic irq controller DT
> support?
> 

Yes, but comments below.

> g.
> 
>  include/linux/irqdomain.h |   15 ++++++++++
>  kernel/irq/irqdomain.c    |   69 +++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 76 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index e7379a3..5e497a0 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -110,6 +110,9 @@ struct irq_domain {
>  	void *host_data;
>  	irq_hw_number_t inval_irq;
>  
> +	/* Data for common irq xlate functions */
> +	unsigned int xlate_type;
> +

How does this get set? Do we want interrupt controllers messing with the
domain struct directly long term?

>  	/* Optional device node pointer */
>  	struct device_node *of_node;
>  };
> @@ -163,6 +166,18 @@ extern unsigned int irq_linear_revmap(struct irq_domain *host,
>  				      irq_hw_number_t hwirq);
>  
>  extern struct irq_domain_ops irq_domain_simple_ops;
> +
> +/* stock xlate functions */
> +int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
> +			const u32 *intspec, unsigned int intsize,
> +			irq_hw_number_t *out_hwirq, unsigned int *out_type);
> +int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
> +			const u32 *intspec, unsigned int intsize,
> +			irq_hw_number_t *out_hwirq, unsigned int *out_type);
> +int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
> +			const u32 *intspec, unsigned int intsize,
> +			irq_hw_number_t *out_hwirq, unsigned int *out_type);
> +
>  #if defined(CONFIG_OF_IRQ)
>  extern void irq_domain_generate_simple(const struct of_device_id *match,
>  					u64 phys_base, unsigned int irq_start);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index def8e7b..df80205 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -53,6 +53,7 @@ static struct irq_domain *irq_domain_alloc(struct device_node *of_node,
>  	domain->ops = ops;
>  	domain->host_data = host_data;
>  	domain->of_node = of_node_get(of_node);
> +	domain->xlate_type = IRQ_TYPE_NONE;
>  
>  	if (domain->ops->match == NULL)
>  		domain->ops->match = default_irq_domain_match;
> @@ -690,25 +691,77 @@ int irq_domain_simple_map(struct irq_domain *d, unsigned int irq,
>  	return 0;
>  }
>  
> -int irq_domain_simple_xlate(struct irq_domain *d,
> -			    struct device_node *controller,
> -			    const u32 *intspec, unsigned int intsize,
> -			    unsigned long *out_hwirq, unsigned int *out_type)
> +/**
> + * irq_domain_xlate_onecell() - Generic xlate for direct one cell bindings
> + *
> + * Device Tree IRQ specifier translation function which works with one cell
> + * bindings where the cell value maps directly to the hwirq number.
> + */
> +int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
> +			     const u32 *intspec, unsigned int intsize,
> +			     unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
> +		 ctrlr->full_name, intsize))
> +		return -EINVAL;
> +	*out_hwirq = intspec[0];
> +	*out_type = d->xlate_type;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
> +
> +/**
> + * irq_domain_xlate_twocell() - Generic xlate for direct two cell bindings
> + *
> + * Device Tree IRQ specifier translation function which works with two cell
> + * bindings where the cell values map directly to the hwirq number
> + * and linux irq flags.
> + */
> +int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
> +			const u32 *intspec, unsigned int intsize,
> +			irq_hw_number_t *out_hwirq, unsigned int *out_type)
>  {
> -	if (d->of_node != controller)
> +	if (WARN(intsize < 2, "Bad intspec for %s: intsize=%i < 2\n",
> +		 ctrlr->full_name, intsize))
> +		return -EINVAL;
> +	if (intsize < 2)

You've already checked this...

>  		return -EINVAL;
> -	if (intsize < 1)
> +	*out_hwirq = intspec[0];
> +	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
> +
> +/**
> + * irq_domain_xlate_onetwocell() - Generic xlate for one or two cell bindings
> + *
> + * Device Tree IRQ specifier translation function which works with either one
> + * or two cell bindings where the cell values map directly to the hwirq number
> + * and linux irq flags.
> + *
> + * Note: don't use this function unless your interrupt controller explicitly
> + * supports both one and two cell bindings.  For the majority of controllers
> + * the _onecell() or _twocell() variants above should be used.

This comment is confusing since you turn around and use it for simple
ops below.

> + */
> +int irq_domain_xlate_onetwocell(struct irq_domain *d,
> +				struct device_node *ctrlr,
> +				const u32 *intspec, unsigned int intsize,
> +				unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
> +		 ctrlr->full_name, intsize))
>  		return -EINVAL;

Isn't this check redundant with the check in of_irq_map_raw:

	if (ointsize != intsize)
		return -EINVAL;

The only diff in these functions is really checking the size. So I
really don't think we need 3 versions if that can be factored out where
you know value of #interrupt-cells.

Rob

>  	*out_hwirq = intspec[0];
> -	*out_type = IRQ_TYPE_NONE;
> +	*out_type = d->xlate_type;
>  	if (intsize > 1)
>  		*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);
>  
>  struct irq_domain_ops irq_domain_simple_ops = {
>  	.map = irq_domain_simple_map,
> -	.xlate = irq_domain_simple_xlate,
> +	.xlate = irq_domain_xlate_onetwocell,
>  };
>  EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
>  

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use
  2012-01-25  1:50 ` [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Rob Herring
@ 2012-01-25  5:35   ` Grant Likely
  2012-01-25 17:59     ` Grant Likely
  0 siblings, 1 reply; 7+ messages in thread
From: Grant Likely @ 2012-01-25  5:35 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-arm-kernel, linux-kernel, linuxppc-dev, Benjamin Herrenschmidt

On Tue, Jan 24, 2012 at 6:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>
>
> On 01/24/2012 06:18 PM, Grant Likely wrote:
>> Rather than having each interrupt controller driver creating its own barely
>> unique .xlate function for irq_domain, create a library of translators which
>> any driver can use directly.
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index e7379a3..5e497a0 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -110,6 +110,9 @@ struct irq_domain {
>>       void *host_data;
>>       irq_hw_number_t inval_irq;
>>
>> +     /* Data for common irq xlate functions */
>> +     unsigned int xlate_type;
>> +
>
> How does this get set? Do we want interrupt controllers messing with the
> domain struct directly long term?

It defaults to IRQ_TYPE_NONE in the alloc function and drivers can
override it.  Alternately it could be made part of the
irq_domain_add() arguments, but I'm not thrilled with adding a whole
bunch of arguments to the function prototype.

>
>>       /* Optional device node pointer */
>>       struct device_node *of_node;
>>  };
>> @@ -163,6 +166,18 @@ extern unsigned int irq_linear_revmap(struct irq_domain *host,
>>                                     irq_hw_number_t hwirq);
>>
>>  extern struct irq_domain_ops irq_domain_simple_ops;
>> +
>> +/* stock xlate functions */
>> +int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
>> +                     const u32 *intspec, unsigned int intsize,
>> +                     irq_hw_number_t *out_hwirq, unsigned int *out_type);
>> +int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
>> +                     const u32 *intspec, unsigned int intsize,
>> +                     irq_hw_number_t *out_hwirq, unsigned int *out_type);
>> +int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
>> +                     const u32 *intspec, unsigned int intsize,
>> +                     irq_hw_number_t *out_hwirq, unsigned int *out_type);
>> +
>>  #if defined(CONFIG_OF_IRQ)
>>  extern void irq_domain_generate_simple(const struct of_device_id *match,
>>                                       u64 phys_base, unsigned int irq_start);
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index def8e7b..df80205 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -53,6 +53,7 @@ static struct irq_domain *irq_domain_alloc(struct device_node *of_node,
>>       domain->ops = ops;
>>       domain->host_data = host_data;
>>       domain->of_node = of_node_get(of_node);
>> +     domain->xlate_type = IRQ_TYPE_NONE;
>>
>>       if (domain->ops->match == NULL)
>>               domain->ops->match = default_irq_domain_match;
>> @@ -690,25 +691,77 @@ int irq_domain_simple_map(struct irq_domain *d, unsigned int irq,
>>       return 0;
>>  }
>>
>> -int irq_domain_simple_xlate(struct irq_domain *d,
>> -                         struct device_node *controller,
>> -                         const u32 *intspec, unsigned int intsize,
>> -                         unsigned long *out_hwirq, unsigned int *out_type)
>> +/**
>> + * irq_domain_xlate_onecell() - Generic xlate for direct one cell bindings
>> + *
>> + * Device Tree IRQ specifier translation function which works with one cell
>> + * bindings where the cell value maps directly to the hwirq number.
>> + */
>> +int irq_domain_xlate_onecell(struct irq_domain *d, struct device_node *ctrlr,
>> +                          const u32 *intspec, unsigned int intsize,
>> +                          unsigned long *out_hwirq, unsigned int *out_type)
>> +{
>> +     if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
>> +              ctrlr->full_name, intsize))
>> +             return -EINVAL;
>> +     *out_hwirq = intspec[0];
>> +     *out_type = d->xlate_type;
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
>> +
>> +/**
>> + * irq_domain_xlate_twocell() - Generic xlate for direct two cell bindings
>> + *
>> + * Device Tree IRQ specifier translation function which works with two cell
>> + * bindings where the cell values map directly to the hwirq number
>> + * and linux irq flags.
>> + */
>> +int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
>> +                     const u32 *intspec, unsigned int intsize,
>> +                     irq_hw_number_t *out_hwirq, unsigned int *out_type)
>>  {
>> -     if (d->of_node != controller)
>> +     if (WARN(intsize < 2, "Bad intspec for %s: intsize=%i < 2\n",
>> +              ctrlr->full_name, intsize))
>> +             return -EINVAL;
>> +     if (intsize < 2)
>
> You've already checked this...

cut-and-paste error.  Fixed.

>
>>               return -EINVAL;
>> -     if (intsize < 1)
>> +     *out_hwirq = intspec[0];
>> +     *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
>> +
>> +/**
>> + * irq_domain_xlate_onetwocell() - Generic xlate for one or two cell bindings
>> + *
>> + * Device Tree IRQ specifier translation function which works with either one
>> + * or two cell bindings where the cell values map directly to the hwirq number
>> + * and linux irq flags.
>> + *
>> + * Note: don't use this function unless your interrupt controller explicitly
>> + * supports both one and two cell bindings.  For the majority of controllers
>> + * the _onecell() or _twocell() variants above should be used.
>
> This comment is confusing since you turn around and use it for simple
> ops below.

I intend to faze out the generic simple ops so that most drivers will
explicitly use _onecell or _twocell.

>> + */
>> +int irq_domain_xlate_onetwocell(struct irq_domain *d,
>> +                             struct device_node *ctrlr,
>> +                             const u32 *intspec, unsigned int intsize,
>> +                             unsigned long *out_hwirq, unsigned int *out_type)
>> +{
>> +     if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
>> +              ctrlr->full_name, intsize))
>>               return -EINVAL;
>
> Isn't this check redundant with the check in of_irq_map_raw:
>
>        if (ointsize != intsize)
>                return -EINVAL;

No.  It's a different test.  The core code does indeed ensure that
intsize matches the #interrupt-cells value defined in the device tree,
but it does nothing to check if the #interrupt-cells value is set to
something sane.  This test ensures that the xlate function will not
dereference beyond the end of the intspec array.

Ideally the interrupt controller driver should already verify the
#interrupt-cells value at initialization, but I'm not going to rely on
it when this test is so cheap.  The intent is to be really noisy about
bad data in the device tree.

> The only diff in these functions is really checking the size. So I
> really don't think we need 3 versions if that can be factored out where
> you know value of #interrupt-cells.

There are subtleties here and #interrupt-cells is not always the
correct thing to test.  For example, some powerpc controllers has
#interrupt-cells = <2>, but the second cell must be ignored so the
_onecell version is the correct function.  I originally investigated
parameterizing a single function, but I didn't like how it ended up
looking because it would require adding something like irq_cells_min
and irq_cells_max to the irq_domain structure.  I think using
variations on the function ends up being cleaner.  I may also add
something like a ..._twocell_lookup() version that uses a lookup table
for the type values to handle the other type of xlate common in the
powerpc tree.

Thanks for the quick review.

g.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use
  2012-01-25  5:35   ` Grant Likely
@ 2012-01-25 17:59     ` Grant Likely
  2012-01-26 14:50       ` Rob Herring
  0 siblings, 1 reply; 7+ messages in thread
From: Grant Likely @ 2012-01-25 17:59 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-arm-kernel, linux-kernel, linuxppc-dev, Benjamin Herrenschmidt

On Tue, Jan 24, 2012 at 10:35 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Tue, Jan 24, 2012 at 6:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>>
>>
>> On 01/24/2012 06:18 PM, Grant Likely wrote:
>>> Rather than having each interrupt controller driver creating its own barely
>>> unique .xlate function for irq_domain, create a library of translators which
>>> any driver can use directly.
>>>
>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>> index e7379a3..5e497a0 100644
>>> --- a/include/linux/irqdomain.h
>>> +++ b/include/linux/irqdomain.h
>>> @@ -110,6 +110,9 @@ struct irq_domain {
>>>       void *host_data;
>>>       irq_hw_number_t inval_irq;
>>>
>>> +     /* Data for common irq xlate functions */
>>> +     unsigned int xlate_type;
>>> +
>>
>> How does this get set? Do we want interrupt controllers messing with the
>> domain struct directly long term?
>
> It defaults to IRQ_TYPE_NONE in the alloc function and drivers can
> override it.  Alternately it could be made part of the
> irq_domain_add() arguments, but I'm not thrilled with adding a whole
> bunch of arguments to the function prototype.

Do you like this better (built on top of this patch)?

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index ef2b1fe..7856c04 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -701,12 +701,10 @@ int irq_domain_xlate_onecell(struct irq_domain
*d, struct device_node *ctrlr,
 			     const u32 *intspec, unsigned int intsize,
 			     unsigned long *out_hwirq, unsigned int *out_type)
 {
-	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
-		 ctrlr->full_name, intsize))
+	if (WARN_ON(intsize < 1))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = d->xlate_type;
-	return 0;
+	return irq_domain_xlate_onetwocell(d, ctrlr, intspec, 1,
+					   out_hwirq, out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);

@@ -721,12 +719,10 @@ int irq_domain_xlate_twocell(struct irq_domain
*d, struct device_node *ctrlr,
 			const u32 *intspec, unsigned int intsize,
 			irq_hw_number_t *out_hwirq, unsigned int *out_type)
 {
-	if (WARN(intsize < 2, "Bad intspec for %s: intsize=%i < 2\n",
-		 ctrlr->full_name, intsize))
+	if (WARN_ON(intsize < 2))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
-	return 0;
+	return irq_domain_xlate_onetwocell(d, ctrlr, intspec, intsize,
+					   out_hwirq, out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);

@@ -746,8 +742,7 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 				const u32 *intspec, unsigned int intsize,
 				unsigned long *out_hwirq, unsigned int *out_type)
 {
-	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
-		 ctrlr->full_name, intsize))
+	if (WARN_ON(intsize < 1))
 		return -EINVAL;
 	*out_hwirq = intspec[0];
 	*out_type = d->xlate_type;

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use
  2012-01-25 17:59     ` Grant Likely
@ 2012-01-26 14:50       ` Rob Herring
  2012-01-26 18:15         ` Grant Likely
  0 siblings, 1 reply; 7+ messages in thread
From: Rob Herring @ 2012-01-26 14:50 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-arm-kernel, linux-kernel, linuxppc-dev, Benjamin Herrenschmidt

On 01/25/2012 11:59 AM, Grant Likely wrote:
> On Tue, Jan 24, 2012 at 10:35 PM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> On Tue, Jan 24, 2012 at 6:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>
>>>
>>> On 01/24/2012 06:18 PM, Grant Likely wrote:
>>>> Rather than having each interrupt controller driver creating its own barely
>>>> unique .xlate function for irq_domain, create a library of translators which
>>>> any driver can use directly.
>>>>
>>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>>> index e7379a3..5e497a0 100644
>>>> --- a/include/linux/irqdomain.h
>>>> +++ b/include/linux/irqdomain.h
>>>> @@ -110,6 +110,9 @@ struct irq_domain {
>>>>       void *host_data;
>>>>       irq_hw_number_t inval_irq;
>>>>
>>>> +     /* Data for common irq xlate functions */
>>>> +     unsigned int xlate_type;
>>>> +
>>>
>>> How does this get set? Do we want interrupt controllers messing with the
>>> domain struct directly long term?
>>
>> It defaults to IRQ_TYPE_NONE in the alloc function and drivers can
>> override it.  Alternately it could be made part of the
>> irq_domain_add() arguments, but I'm not thrilled with adding a whole
>> bunch of arguments to the function prototype.
> 
> Do you like this better (built on top of this patch)?

Yes, I think it's better.

> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index ef2b1fe..7856c04 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -701,12 +701,10 @@ int irq_domain_xlate_onecell(struct irq_domain
> *d, struct device_node *ctrlr,
>  			     const u32 *intspec, unsigned int intsize,
>  			     unsigned long *out_hwirq, unsigned int *out_type)
>  {
> -	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
> -		 ctrlr->full_name, intsize))
> +	if (WARN_ON(intsize < 1))
>  		return -EINVAL;
> -	*out_hwirq = intspec[0];
> -	*out_type = d->xlate_type;
> -	return 0;
> +	return irq_domain_xlate_onetwocell(d, ctrlr, intspec, 1,
> +					   out_hwirq, out_type);
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
> 
> @@ -721,12 +719,10 @@ int irq_domain_xlate_twocell(struct irq_domain
> *d, struct device_node *ctrlr,
>  			const u32 *intspec, unsigned int intsize,
>  			irq_hw_number_t *out_hwirq, unsigned int *out_type)
>  {
> -	if (WARN(intsize < 2, "Bad intspec for %s: intsize=%i < 2\n",
> -		 ctrlr->full_name, intsize))
> +	if (WARN_ON(intsize < 2))
>  		return -EINVAL;
> -	*out_hwirq = intspec[0];
> -	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> -	return 0;
> +	return irq_domain_xlate_onetwocell(d, ctrlr, intspec, intsize,
> +					   out_hwirq, out_type);
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
> 
> @@ -746,8 +742,7 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
>  				const u32 *intspec, unsigned int intsize,
>  				unsigned long *out_hwirq, unsigned int *out_type)
>  {
> -	if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
> -		 ctrlr->full_name, intsize))
> +	if (WARN_ON(intsize < 1))
>  		return -EINVAL;
>  	*out_hwirq = intspec[0];
>  	*out_type = d->xlate_type;


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use
  2012-01-26 14:50       ` Rob Herring
@ 2012-01-26 18:15         ` Grant Likely
  0 siblings, 0 replies; 7+ messages in thread
From: Grant Likely @ 2012-01-26 18:15 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-arm-kernel, linux-kernel, linuxppc-dev, Benjamin Herrenschmidt

On Thu, Jan 26, 2012 at 7:50 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 01/25/2012 11:59 AM, Grant Likely wrote:
>> On Tue, Jan 24, 2012 at 10:35 PM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>> On Tue, Jan 24, 2012 at 6:50 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>
>>>>
>>>> On 01/24/2012 06:18 PM, Grant Likely wrote:
>>>>> Rather than having each interrupt controller driver creating its own barely
>>>>> unique .xlate function for irq_domain, create a library of translators which
>>>>> any driver can use directly.
>>>>>
>>>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>>>> index e7379a3..5e497a0 100644
>>>>> --- a/include/linux/irqdomain.h
>>>>> +++ b/include/linux/irqdomain.h
>>>>> @@ -110,6 +110,9 @@ struct irq_domain {
>>>>>       void *host_data;
>>>>>       irq_hw_number_t inval_irq;
>>>>>
>>>>> +     /* Data for common irq xlate functions */
>>>>> +     unsigned int xlate_type;
>>>>> +
>>>>
>>>> How does this get set? Do we want interrupt controllers messing with the
>>>> domain struct directly long term?
>>>
>>> It defaults to IRQ_TYPE_NONE in the alloc function and drivers can
>>> override it.  Alternately it could be made part of the
>>> irq_domain_add() arguments, but I'm not thrilled with adding a whole
>>> bunch of arguments to the function prototype.
>>
>> Do you like this better (built on top of this patch)?
>
> Yes, I think it's better.

I've changed my mind on this after looking at the generated code.
These functions are so simple that it actually is larger (both in
source and object size) to call out to a common function.  I've gone
back to each _xlate_*() function open coding the two assignments.

g.

>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index ef2b1fe..7856c04 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -701,12 +701,10 @@ int irq_domain_xlate_onecell(struct irq_domain
>> *d, struct device_node *ctrlr,
>>                            const u32 *intspec, unsigned int intsize,
>>                            unsigned long *out_hwirq, unsigned int *out_type)
>>  {
>> -     if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
>> -              ctrlr->full_name, intsize))
>> +     if (WARN_ON(intsize < 1))
>>               return -EINVAL;
>> -     *out_hwirq = intspec[0];
>> -     *out_type = d->xlate_type;
>> -     return 0;
>> +     return irq_domain_xlate_onetwocell(d, ctrlr, intspec, 1,
>> +                                        out_hwirq, out_type);
>>  }
>>  EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
>>
>> @@ -721,12 +719,10 @@ int irq_domain_xlate_twocell(struct irq_domain
>> *d, struct device_node *ctrlr,
>>                       const u32 *intspec, unsigned int intsize,
>>                       irq_hw_number_t *out_hwirq, unsigned int *out_type)
>>  {
>> -     if (WARN(intsize < 2, "Bad intspec for %s: intsize=%i < 2\n",
>> -              ctrlr->full_name, intsize))
>> +     if (WARN_ON(intsize < 2))
>>               return -EINVAL;
>> -     *out_hwirq = intspec[0];
>> -     *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
>> -     return 0;
>> +     return irq_domain_xlate_onetwocell(d, ctrlr, intspec, intsize,
>> +                                        out_hwirq, out_type);
>>  }
>>  EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
>>
>> @@ -746,8 +742,7 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
>>                               const u32 *intspec, unsigned int intsize,
>>                               unsigned long *out_hwirq, unsigned int *out_type)
>>  {
>> -     if (WARN(intsize < 1, "Bad intspec for %s: intsize=%i < 1\n",
>> -              ctrlr->full_name, intsize))
>> +     if (WARN_ON(intsize < 1))
>>               return -EINVAL;
>>       *out_hwirq = intspec[0];
>>       *out_type = d->xlate_type;
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-01-26 18:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-25  0:18 [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Grant Likely
2012-01-25  0:18 ` [RFC 2/2] irqdomain/powerpc: Replace custom xlate functions with library functions Grant Likely
2012-01-25  1:50 ` [RFC 1/2] irq_domain: Create common xlate functions that device drivers can use Rob Herring
2012-01-25  5:35   ` Grant Likely
2012-01-25 17:59     ` Grant Likely
2012-01-26 14:50       ` Rob Herring
2012-01-26 18:15         ` Grant Likely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).