[PATCH] irqdomain: Initialize number of IRQs for simple domains

[PATCH] irqdomain: Initialize number of IRQs for simple domains

[Thierry Reding]

The irq_domain_add() function needs the number of interrupts in the
domain to properly initialize them. In addition the allocated domain
is now returned by the irq_domain_{add,generate}_simple() helpers.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 arch/arm/mach-at91/board-dt.c         |    6 +++++-
 arch/arm/mach-imx/imx51-dt.c          |   13 +++++++++++--
 arch/arm/mach-imx/imx53-dt.c          |   13 +++++++++++--
 arch/arm/mach-imx/mach-imx6q.c        |    4 +++-
 arch/arm/mach-msm/board-msm8x60.c     |    7 +++++--
 arch/arm/mach-omap2/board-generic.c   |    7 +++++--
 arch/arm/mach-prima2/irq.c            |    4 +++-
 arch/arm/mach-versatile/core.c        |   10 ++++++++--
 arch/arm/plat-mxc/include/mach/irqs.h |    1 +
 arch/arm/plat-mxc/tzic.c              |    2 --
 include/linux/irqdomain.h             |   27 ++++++++++++++++++++++-----
 kernel/irq/irqdomain.c                |   30 +++++++++++++++++++-----------
 12 files changed, 93 insertions(+), 31 deletions(-)

diff --git a/arch/arm/mach-at91/board-dt.c b/arch/arm/mach-at91/board-dt.c
index bb6b434..12f003d 100644
--- a/arch/arm/mach-at91/board-dt.c
+++ b/arch/arm/mach-at91/board-dt.c
@@ -95,7 +95,11 @@ static const struct of_device_id aic_of_match[] __initconst = {
 
 static void __init at91_dt_init_irq(void)
 {
-	irq_domain_generate_simple(aic_of_match, 0xfffff000, 0);
+	struct irq_domain *domain;
+
+	domain = irq_domain_generate_simple(aic_of_match, 0xfffff000, 0,
+			NR_AIC_IRQS);
+	WARN_ON(IS_ERR(domain));
 	at91_init_irq_default();
 }
 
diff --git a/arch/arm/mach-imx/imx51-dt.c b/arch/arm/mach-imx/imx51-dt.c
index e6bad17..72bf94c 100644
--- a/arch/arm/mach-imx/imx51-dt.c
+++ b/arch/arm/mach-imx/imx51-dt.c
@@ -47,7 +47,12 @@ static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = {
 static int __init imx51_tzic_add_irq_domain(struct device_node *np,
 				struct device_node *interrupt_parent)
 {
-	irq_domain_add_simple(np, 0);
+	struct irq_domain *domain;
+
+	domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS);
+	if (IS_ERR(domain))
+		return PTR_ERR(domain);
+
 	return 0;
 }
 
@@ -55,9 +60,13 @@ static int __init imx51_gpio_add_irq_domain(struct device_node *np,
 				struct device_node *interrupt_parent)
 {
 	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
+	struct irq_domain *domain;
 
 	gpio_irq_base -= 32;
-	irq_domain_add_simple(np, gpio_irq_base);
+
+	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
+	if (IS_ERR(domain))
+		return PTR_ERR(domain);
 
 	return 0;
 }
diff --git a/arch/arm/mach-imx/imx53-dt.c b/arch/arm/mach-imx/imx53-dt.c
index 05ebb3e..ccae620 100644
--- a/arch/arm/mach-imx/imx53-dt.c
+++ b/arch/arm/mach-imx/imx53-dt.c
@@ -51,7 +51,12 @@ static const struct of_dev_auxdata imx53_auxdata_lookup[] __initconst = {
 static int __init imx53_tzic_add_irq_domain(struct device_node *np,
 				struct device_node *interrupt_parent)
 {
-	irq_domain_add_simple(np, 0);
+	struct irq_domain *domain;
+
+	domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS);
+	if (IS_ERR(domain))
+		return PTR_ERR(domain);
+
 	return 0;
 }
 
@@ -59,9 +64,13 @@ static int __init imx53_gpio_add_irq_domain(struct device_node *np,
 				struct device_node *interrupt_parent)
 {
 	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
+	struct irq_domain *domain;
 
 	gpio_irq_base -= 32;
-	irq_domain_add_simple(np, gpio_irq_base);
+
+	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
+	if (IS_ERR(domain))
+		return PTR_ERR(domain);
 
 	return 0;
 }
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index c257281..9ed7812 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -95,9 +95,11 @@ static int __init imx6q_gpio_add_irq_domain(struct device_node *np,
 				struct device_node *interrupt_parent)
 {
 	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
+	struct irq_domain *domain;
 
 	gpio_irq_base -= 32;
-	irq_domain_add_simple(np, gpio_irq_base);
+	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
+	WARN_ON(IS_ERR(domain));
 
 	return 0;
 }
diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c
index 0a11342..a50c7e2 100644
--- a/arch/arm/mach-msm/board-msm8x60.c
+++ b/arch/arm/mach-msm/board-msm8x60.c
@@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void)
 
 	node = of_find_matching_node_by_address(NULL, msm_dt_gic_match,
 			MSM8X60_QGIC_DIST_PHYS);
-	if (node)
-		irq_domain_add_simple(node, GIC_SPI_START);
+	if (node) {
+		struct irq_domain *domain = irq_domain_add_simple(node,
+				GIC_SPI_START, NR_MSM_IRQS);
+		WARN_ON(IS_ERR(domain));
+	}
 
 	if (of_machine_is_compatible("qcom,msm8660-surf")) {
 		printk(KERN_INFO "Init surf UART registers\n");
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index d587560..bf67781 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = {
 static void __init omap_generic_init(void)
 {
 	struct device_node *node = of_find_matching_node(NULL, intc_match);
-	if (node)
-		irq_domain_add_simple(node, 0);
+	if (node) {
+		struct irq_domain *domain;
+		domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS);
+		WARN_ON(IS_ERR(domain));
+	}
 
 	omap_sdrc_init(NULL, NULL);
 
diff --git a/arch/arm/mach-prima2/irq.c b/arch/arm/mach-prima2/irq.c
index d93ceef..d3c5136 100644
--- a/arch/arm/mach-prima2/irq.c
+++ b/arch/arm/mach-prima2/irq.c
@@ -58,6 +58,7 @@ static struct of_device_id intc_ids[]  = {
 
 void __init sirfsoc_of_irq_init(void)
 {
+	struct irq_domain *domain;
 	struct device_node *np;
 
 	np = of_find_matching_node(NULL, intc_ids);
@@ -68,7 +69,8 @@ void __init sirfsoc_of_irq_init(void)
 	if (!sirfsoc_intc_base)
 		panic("unable to map intc cpu registers\n");
 
-	irq_domain_add_simple(np, 0);
+	domain = irq_domain_add_simple(np, 0, NR_IRQS);
+	WARN_ON(IS_ERR(domain));
 
 	of_node_put(np);
 
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index 02b7b93..6c40be4 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -98,13 +98,19 @@ static const struct of_device_id sic_of_match[] __initconst = {
 
 void __init versatile_init_irq(void)
 {
+	struct irq_domain *domain;
+
 	vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0);
-	irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START);
+	domain = irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE,
+			IRQ_VIC_START, IRQ_VIC_END - IRQ_VIC_START + 1);
+	WARN_ON(IS_ERR(domain));
 
 	writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR);
 
 	fpga_irq_init(IRQ_VICSOURCE31, ~PIC_MASK, &sic_irq);
-	irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, IRQ_SIC_START);
+	domain = irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE,
+			IRQ_SIC_START, IRQ_SIC_END - IRQ_SIC_START + 1);
+	WARN_ON(IS_ERR(domain));
 
 	/*
 	 * Interrupts on secondary controller from 0 to 8 are routed to
diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h
index fd9efb0..2fda5aa 100644
--- a/arch/arm/plat-mxc/include/mach/irqs.h
+++ b/arch/arm/plat-mxc/include/mach/irqs.h
@@ -54,6 +54,7 @@
 /* REVISIT: Add IPU irqs on IMX51 */
 
 #define NR_IRQS			(MXC_IPU_IRQ_START + MX3_IPU_IRQS)
+#define TZIC_NUM_IRQS		128
 
 extern int imx_irq_set_priority(unsigned char irq, unsigned char prio);
 
diff --git a/arch/arm/plat-mxc/tzic.c b/arch/arm/plat-mxc/tzic.c
index 98308ec..98b23b8 100644
--- a/arch/arm/plat-mxc/tzic.c
+++ b/arch/arm/plat-mxc/tzic.c
@@ -50,8 +50,6 @@
 
 void __iomem *tzic_base; /* Used as irq controller base in entry-macro.S */
 
-#define TZIC_NUM_IRQS 128
-
 #ifdef CONFIG_FIQ
 static int tzic_set_irq_fiq(unsigned int irq, unsigned int type)
 {
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index bd4272b..1538d4c 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -15,6 +15,7 @@
 #ifndef _LINUX_IRQDOMAIN_H
 #define _LINUX_IRQDOMAIN_H
 
+#include <linux/err.h>
 #include <linux/irq.h>
 #include <linux/mod_devicetable.h>
 
@@ -96,12 +97,28 @@ extern struct irq_domain_ops irq_domain_simple_ops;
 #endif /* CONFIG_IRQ_DOMAIN */
 
 #if defined(CONFIG_IRQ_DOMAIN) && defined(CONFIG_OF_IRQ)
-extern void irq_domain_add_simple(struct device_node *controller, int irq_base);
-extern void irq_domain_generate_simple(const struct of_device_id *match,
-					u64 phys_base, unsigned int irq_start);
+extern struct irq_domain *irq_domain_add_simple(struct device_node *controller,
+						unsigned int irq_base,
+						unsigned int nr_irq);
+extern struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match,
+						     u64 phys_base,
+						     unsigned int irq_start,
+						     unsigned int nr_irq);
 #else /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */
-static inline void irq_domain_generate_simple(const struct of_device_id *match,
-					u64 phys_base, unsigned int irq_start) { }
+static inline struct irq_domain *irq_domain_add_simple(struct device_node *controller,
+						       int irq_base,
+						       unsigned int nr_irq)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match,
+							    u64 phys_base,
+							    unsigned int irq_start,
+							    unsigned int nr_irq)
+{
+	return ERR_PTR(-ENOSYS);
+}
 #endif /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */
 
 #endif /* _LINUX_IRQDOMAIN_H */
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 1f9e265..807c44b 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -147,36 +147,44 @@ int irq_domain_simple_dt_translate(struct irq_domain *d,
 }
 
 /**
- * irq_domain_create_simple() - Set up a 'simple' translation range
+ * irq_domain_add_simple() - Set up a 'simple' translation range
  */
-void irq_domain_add_simple(struct device_node *controller, int irq_base)
+struct irq_domain *irq_domain_add_simple(struct device_node *controller,
+					 unsigned int irq_base,
+					 unsigned int nr_irq)
 {
 	struct irq_domain *domain;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain) {
-		WARN_ON(1);
-		return;
-	}
+	if (!domain)
+		return ERR_PTR(-ENOMEM);
 
 	domain->irq_base = irq_base;
+	domain->nr_irq = nr_irq;
 	domain->of_node = of_node_get(controller);
 	domain->ops = &irq_domain_simple_ops;
 	irq_domain_add(domain);
+
+	return domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_add_simple);
 
-void irq_domain_generate_simple(const struct of_device_id *match,
-				u64 phys_base, unsigned int irq_start)
+struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match,
+					      u64 phys_base,
+					      unsigned int irq_start,
+					      unsigned int nr_irq)
 {
+	struct irq_domain *domain = ERR_PTR(-ENODEV);
 	struct device_node *node;
-	pr_info("looking for phys_base=%llx, irq_start=%i\n",
-		(unsigned long long) phys_base, (int) irq_start);
+	pr_info("looking for phys_base=%llx, irq_start=%u, nr_irq=%u\n",
+		(unsigned long long) phys_base, irq_start, nr_irq);
 	node = of_find_matching_node_by_address(NULL, match, phys_base);
 	if (node)
-		irq_domain_add_simple(node, irq_start);
+		domain = irq_domain_add_simple(node, irq_start, nr_irq);
 	else
 		pr_info("no node found\n");
+
+	return domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_generate_simple);
 #endif /* CONFIG_OF_IRQ */
-- 
1.7.8.1

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Nicolas Ferre]

On 01/06/2012 03:28 PM, Thierry Reding :
> The irq_domain_add() function needs the number of interrupts in the
> domain to properly initialize them. In addition the allocated domain
> is now returned by the irq_domain_{add,generate}_simple() helpers.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>

Hi Thierry,

Good to see this patch ;-)
Thanks for having done this modification.

For AT91 and irqdomain (as far as I can tell)

Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Best regards,

> ---
>  arch/arm/mach-at91/board-dt.c         |    6 +++++-
>  arch/arm/mach-imx/imx51-dt.c          |   13 +++++++++++--
>  arch/arm/mach-imx/imx53-dt.c          |   13 +++++++++++--
>  arch/arm/mach-imx/mach-imx6q.c        |    4 +++-
>  arch/arm/mach-msm/board-msm8x60.c     |    7 +++++--
>  arch/arm/mach-omap2/board-generic.c   |    7 +++++--
>  arch/arm/mach-prima2/irq.c            |    4 +++-
>  arch/arm/mach-versatile/core.c        |   10 ++++++++--
>  arch/arm/plat-mxc/include/mach/irqs.h |    1 +
>  arch/arm/plat-mxc/tzic.c              |    2 --
>  include/linux/irqdomain.h             |   27 ++++++++++++++++++++++-----
>  kernel/irq/irqdomain.c                |   30 +++++++++++++++++++-----------
>  12 files changed, 93 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/board-dt.c b/arch/arm/mach-at91/board-dt.c
> index bb6b434..12f003d 100644
> --- a/arch/arm/mach-at91/board-dt.c
> +++ b/arch/arm/mach-at91/board-dt.c
> @@ -95,7 +95,11 @@ static const struct of_device_id aic_of_match[] __initconst = {
>  
>  static void __init at91_dt_init_irq(void)
>  {
> -	irq_domain_generate_simple(aic_of_match, 0xfffff000, 0);
> +	struct irq_domain *domain;
> +
> +	domain = irq_domain_generate_simple(aic_of_match, 0xfffff000, 0,
> +			NR_AIC_IRQS);
> +	WARN_ON(IS_ERR(domain));
>  	at91_init_irq_default();
>  }
>  
> diff --git a/arch/arm/mach-imx/imx51-dt.c b/arch/arm/mach-imx/imx51-dt.c
> index e6bad17..72bf94c 100644
> --- a/arch/arm/mach-imx/imx51-dt.c
> +++ b/arch/arm/mach-imx/imx51-dt.c
> @@ -47,7 +47,12 @@ static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = {
>  static int __init imx51_tzic_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
> -	irq_domain_add_simple(np, 0);
> +	struct irq_domain *domain;
> +
> +	domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
> +
>  	return 0;
>  }
>  
> @@ -55,9 +60,13 @@ static int __init imx51_gpio_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
>  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +	struct irq_domain *domain;
>  
>  	gpio_irq_base -= 32;
> -	irq_domain_add_simple(np, gpio_irq_base);
> +
> +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
>  
>  	return 0;
>  }
> diff --git a/arch/arm/mach-imx/imx53-dt.c b/arch/arm/mach-imx/imx53-dt.c
> index 05ebb3e..ccae620 100644
> --- a/arch/arm/mach-imx/imx53-dt.c
> +++ b/arch/arm/mach-imx/imx53-dt.c
> @@ -51,7 +51,12 @@ static const struct of_dev_auxdata imx53_auxdata_lookup[] __initconst = {
>  static int __init imx53_tzic_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
> -	irq_domain_add_simple(np, 0);
> +	struct irq_domain *domain;
> +
> +	domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
> +
>  	return 0;
>  }
>  
> @@ -59,9 +64,13 @@ static int __init imx53_gpio_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
>  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +	struct irq_domain *domain;
>  
>  	gpio_irq_base -= 32;
> -	irq_domain_add_simple(np, gpio_irq_base);
> +
> +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
>  
>  	return 0;
>  }
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index c257281..9ed7812 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -95,9 +95,11 @@ static int __init imx6q_gpio_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
>  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +	struct irq_domain *domain;
>  
>  	gpio_irq_base -= 32;
> -	irq_domain_add_simple(np, gpio_irq_base);
> +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> +	WARN_ON(IS_ERR(domain));
>  
>  	return 0;
>  }
> diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c
> index 0a11342..a50c7e2 100644
> --- a/arch/arm/mach-msm/board-msm8x60.c
> +++ b/arch/arm/mach-msm/board-msm8x60.c
> @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void)
>  
>  	node = of_find_matching_node_by_address(NULL, msm_dt_gic_match,
>  			MSM8X60_QGIC_DIST_PHYS);
> -	if (node)
> -		irq_domain_add_simple(node, GIC_SPI_START);
> +	if (node) {
> +		struct irq_domain *domain = irq_domain_add_simple(node,
> +				GIC_SPI_START, NR_MSM_IRQS);
> +		WARN_ON(IS_ERR(domain));
> +	}
>  
>  	if (of_machine_is_compatible("qcom,msm8660-surf")) {
>  		printk(KERN_INFO "Init surf UART registers\n");
> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> index d587560..bf67781 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = {
>  static void __init omap_generic_init(void)
>  {
>  	struct device_node *node = of_find_matching_node(NULL, intc_match);
> -	if (node)
> -		irq_domain_add_simple(node, 0);
> +	if (node) {
> +		struct irq_domain *domain;
> +		domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS);
> +		WARN_ON(IS_ERR(domain));
> +	}
>  
>  	omap_sdrc_init(NULL, NULL);
>  
> diff --git a/arch/arm/mach-prima2/irq.c b/arch/arm/mach-prima2/irq.c
> index d93ceef..d3c5136 100644
> --- a/arch/arm/mach-prima2/irq.c
> +++ b/arch/arm/mach-prima2/irq.c
> @@ -58,6 +58,7 @@ static struct of_device_id intc_ids[]  = {
>  
>  void __init sirfsoc_of_irq_init(void)
>  {
> +	struct irq_domain *domain;
>  	struct device_node *np;
>  
>  	np = of_find_matching_node(NULL, intc_ids);
> @@ -68,7 +69,8 @@ void __init sirfsoc_of_irq_init(void)
>  	if (!sirfsoc_intc_base)
>  		panic("unable to map intc cpu registers\n");
>  
> -	irq_domain_add_simple(np, 0);
> +	domain = irq_domain_add_simple(np, 0, NR_IRQS);
> +	WARN_ON(IS_ERR(domain));
>  
>  	of_node_put(np);
>  
> diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
> index 02b7b93..6c40be4 100644
> --- a/arch/arm/mach-versatile/core.c
> +++ b/arch/arm/mach-versatile/core.c
> @@ -98,13 +98,19 @@ static const struct of_device_id sic_of_match[] __initconst = {
>  
>  void __init versatile_init_irq(void)
>  {
> +	struct irq_domain *domain;
> +
>  	vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0);
> -	irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START);
> +	domain = irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE,
> +			IRQ_VIC_START, IRQ_VIC_END - IRQ_VIC_START + 1);
> +	WARN_ON(IS_ERR(domain));
>  
>  	writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR);
>  
>  	fpga_irq_init(IRQ_VICSOURCE31, ~PIC_MASK, &sic_irq);
> -	irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, IRQ_SIC_START);
> +	domain = irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE,
> +			IRQ_SIC_START, IRQ_SIC_END - IRQ_SIC_START + 1);
> +	WARN_ON(IS_ERR(domain));
>  
>  	/*
>  	 * Interrupts on secondary controller from 0 to 8 are routed to
> diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h
> index fd9efb0..2fda5aa 100644
> --- a/arch/arm/plat-mxc/include/mach/irqs.h
> +++ b/arch/arm/plat-mxc/include/mach/irqs.h
> @@ -54,6 +54,7 @@
>  /* REVISIT: Add IPU irqs on IMX51 */
>  
>  #define NR_IRQS			(MXC_IPU_IRQ_START + MX3_IPU_IRQS)
> +#define TZIC_NUM_IRQS		128
>  
>  extern int imx_irq_set_priority(unsigned char irq, unsigned char prio);
>  
> diff --git a/arch/arm/plat-mxc/tzic.c b/arch/arm/plat-mxc/tzic.c
> index 98308ec..98b23b8 100644
> --- a/arch/arm/plat-mxc/tzic.c
> +++ b/arch/arm/plat-mxc/tzic.c
> @@ -50,8 +50,6 @@
>  
>  void __iomem *tzic_base; /* Used as irq controller base in entry-macro.S */
>  
> -#define TZIC_NUM_IRQS 128
> -
>  #ifdef CONFIG_FIQ
>  static int tzic_set_irq_fiq(unsigned int irq, unsigned int type)
>  {
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index bd4272b..1538d4c 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -15,6 +15,7 @@
>  #ifndef _LINUX_IRQDOMAIN_H
>  #define _LINUX_IRQDOMAIN_H
>  
> +#include <linux/err.h>
>  #include <linux/irq.h>
>  #include <linux/mod_devicetable.h>
>  
> @@ -96,12 +97,28 @@ extern struct irq_domain_ops irq_domain_simple_ops;
>  #endif /* CONFIG_IRQ_DOMAIN */
>  
>  #if defined(CONFIG_IRQ_DOMAIN) && defined(CONFIG_OF_IRQ)
> -extern void irq_domain_add_simple(struct device_node *controller, int irq_base);
> -extern void irq_domain_generate_simple(const struct of_device_id *match,
> -					u64 phys_base, unsigned int irq_start);
> +extern struct irq_domain *irq_domain_add_simple(struct device_node *controller,
> +						unsigned int irq_base,
> +						unsigned int nr_irq);
> +extern struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match,
> +						     u64 phys_base,
> +						     unsigned int irq_start,
> +						     unsigned int nr_irq);
>  #else /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */
> -static inline void irq_domain_generate_simple(const struct of_device_id *match,
> -					u64 phys_base, unsigned int irq_start) { }
> +static inline struct irq_domain *irq_domain_add_simple(struct device_node *controller,
> +						       int irq_base,
> +						       unsigned int nr_irq)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
> +static inline struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match,
> +							    u64 phys_base,
> +							    unsigned int irq_start,
> +							    unsigned int nr_irq)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
>  #endif /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */
>  
>  #endif /* _LINUX_IRQDOMAIN_H */
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 1f9e265..807c44b 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -147,36 +147,44 @@ int irq_domain_simple_dt_translate(struct irq_domain *d,
>  }
>  
>  /**
> - * irq_domain_create_simple() - Set up a 'simple' translation range
> + * irq_domain_add_simple() - Set up a 'simple' translation range
>   */
> -void irq_domain_add_simple(struct device_node *controller, int irq_base)
> +struct irq_domain *irq_domain_add_simple(struct device_node *controller,
> +					 unsigned int irq_base,
> +					 unsigned int nr_irq)
>  {
>  	struct irq_domain *domain;
>  
>  	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> -	if (!domain) {
> -		WARN_ON(1);
> -		return;
> -	}
> +	if (!domain)
> +		return ERR_PTR(-ENOMEM);
>  
>  	domain->irq_base = irq_base;
> +	domain->nr_irq = nr_irq;
>  	domain->of_node = of_node_get(controller);
>  	domain->ops = &irq_domain_simple_ops;
>  	irq_domain_add(domain);
> +
> +	return domain;
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_add_simple);
>  
> -void irq_domain_generate_simple(const struct of_device_id *match,
> -				u64 phys_base, unsigned int irq_start)
> +struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match,
> +					      u64 phys_base,
> +					      unsigned int irq_start,
> +					      unsigned int nr_irq)
>  {
> +	struct irq_domain *domain = ERR_PTR(-ENODEV);
>  	struct device_node *node;
> -	pr_info("looking for phys_base=%llx, irq_start=%i\n",
> -		(unsigned long long) phys_base, (int) irq_start);
> +	pr_info("looking for phys_base=%llx, irq_start=%u, nr_irq=%u\n",
> +		(unsigned long long) phys_base, irq_start, nr_irq);
>  	node = of_find_matching_node_by_address(NULL, match, phys_base);
>  	if (node)
> -		irq_domain_add_simple(node, irq_start);
> +		domain = irq_domain_add_simple(node, irq_start, nr_irq);
>  	else
>  		pr_info("no node found\n");
> +
> +	return domain;
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_generate_simple);
>  #endif /* CONFIG_OF_IRQ */


-- 
Nicolas Ferre

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Grant Likely]

Hi Thierry,

On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> The irq_domain_add() function needs the number of interrupts in the
> domain to properly initialize them. In addition the allocated domain
> is now returned by the irq_domain_{add,generate}_simple() helpers.

The commit text should also include the justification for renaming
irq_domain_create_simple() -> irq_domain_add_simple()

>
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
>  /**
> - * irq_domain_create_simple() - Set up a 'simple' translation range
> + * irq_domain_add_simple() - Set up a 'simple' translation range
>  */
> -void irq_domain_add_simple(struct device_node *controller, int irq_base)
> +struct irq_domain *irq_domain_add_simple(struct device_node *controller,
> +                                        unsigned int irq_base,
> +                                        unsigned int nr_irq)
>  {
>        struct irq_domain *domain;
>
>        domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> -       if (!domain) {
> -               WARN_ON(1);
> -               return;
> -       }
> +       if (!domain)
> +               return ERR_PTR(-ENOMEM);

Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO).
Return NULL on failure and keep the WARN_ON() in this function.
Otherwise looks good.  Thanks for writing this patch.

g.

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[David Brown]

On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote:
> diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c
> index 0a11342..a50c7e2 100644
> --- a/arch/arm/mach-msm/board-msm8x60.c
> +++ b/arch/arm/mach-msm/board-msm8x60.c
> @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void)
>  
>  	node = of_find_matching_node_by_address(NULL, msm_dt_gic_match,
>  			MSM8X60_QGIC_DIST_PHYS);
> -	if (node)
> -		irq_domain_add_simple(node, GIC_SPI_START);
> +	if (node) {
> +		struct irq_domain *domain = irq_domain_add_simple(node,
> +				GIC_SPI_START, NR_MSM_IRQS);
> +		WARN_ON(IS_ERR(domain));
> +	}
>  
>  	if (of_machine_is_compatible("qcom,msm8660-surf")) {
>  		printk(KERN_INFO "Init surf UART registers\n");

This is probably a consequence of MSM not really being "simple", but
just using that.  However, NR_MSM_IRQS is only the number of IRQs on
the MSM core.  There are also GPIO irqs, and potentially board IRQs
(the board has an I2C-based chip with a bunch of IRQ lines on it).

The only define that captures this now is 'NR_IRQS', even though we're
trying to get rid of that.

Ultimately, the correct answer will be to get the various interrupt
controllers using their own domains, but for now, this needs to be a
larger value to avoid missing a bunch of the interrupts.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1499 bytes --]

* David Brown wrote:
> On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote:
> > diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c
> > index 0a11342..a50c7e2 100644
> > --- a/arch/arm/mach-msm/board-msm8x60.c
> > +++ b/arch/arm/mach-msm/board-msm8x60.c
> > @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void)
> >  
> >  	node = of_find_matching_node_by_address(NULL, msm_dt_gic_match,
> >  			MSM8X60_QGIC_DIST_PHYS);
> > -	if (node)
> > -		irq_domain_add_simple(node, GIC_SPI_START);
> > +	if (node) {
> > +		struct irq_domain *domain = irq_domain_add_simple(node,
> > +				GIC_SPI_START, NR_MSM_IRQS);
> > +		WARN_ON(IS_ERR(domain));
> > +	}
> >  
> >  	if (of_machine_is_compatible("qcom,msm8660-surf")) {
> >  		printk(KERN_INFO "Init surf UART registers\n");
> 
> This is probably a consequence of MSM not really being "simple", but
> just using that.  However, NR_MSM_IRQS is only the number of IRQs on
> the MSM core.  There are also GPIO irqs, and potentially board IRQs
> (the board has an I2C-based chip with a bunch of IRQ lines on it).
> 
> The only define that captures this now is 'NR_IRQS', even though we're
> trying to get rid of that.
> 
> Ultimately, the correct answer will be to get the various interrupt
> controllers using their own domains,

Yes.

> but for now, this needs to be a larger value to avoid missing a bunch of
> the interrupts.

Okay, I'll make it NR_IRQS then.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1625 bytes --]

* Grant Likely wrote:
> Hi Thierry,
> 
> On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > The irq_domain_add() function needs the number of interrupts in the
> > domain to properly initialize them. In addition the allocated domain
> > is now returned by the irq_domain_{add,generate}_simple() helpers.
> 
> The commit text should also include the justification for renaming
> irq_domain_create_simple() -> irq_domain_add_simple()

Actually the commit only fixes up the comment. The function has always been
called irq_domain_add_simple().

For reference, this was introduced in commit 7e71330.

> >        domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> > -       if (!domain) {
> > -               WARN_ON(1);
> > -               return;
> > -       }
> > +       if (!domain)
> > +               return ERR_PTR(-ENOMEM);
> 
> Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO).

Returning NULL here is probably okay. Can the ERR_PTR stay in
irq_domain_generate_simple(), though? It has two error conditions and
handling both by returning NULL may not be what we want.

> Return NULL on failure and keep the WARN_ON() in this function.
> Otherwise looks good.  Thanks for writing this patch.

I thought it would be better to pull the WARN_ON out of the function because
we now actually have a way to determine if the call succeeded in the caller.
In most cases I assume the caller will be much better suited to handle the
situation gracefully such that the WARN_ON is not required.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Rob Herring]

On 01/06/2012 10:07 AM, David Brown wrote:
> On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote:
>> diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c
>> index 0a11342..a50c7e2 100644
>> --- a/arch/arm/mach-msm/board-msm8x60.c
>> +++ b/arch/arm/mach-msm/board-msm8x60.c
>> @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void)
>>  
>>  	node = of_find_matching_node_by_address(NULL, msm_dt_gic_match,
>>  			MSM8X60_QGIC_DIST_PHYS);
>> -	if (node)
>> -		irq_domain_add_simple(node, GIC_SPI_START);
>> +	if (node) {
>> +		struct irq_domain *domain = irq_domain_add_simple(node,
>> +				GIC_SPI_START, NR_MSM_IRQS);
>> +		WARN_ON(IS_ERR(domain));
>> +	}
>>  
>>  	if (of_machine_is_compatible("qcom,msm8660-surf")) {
>>  		printk(KERN_INFO "Init surf UART registers\n");
> 
> This is probably a consequence of MSM not really being "simple", but
> just using that.  However, NR_MSM_IRQS is only the number of IRQs on
> the MSM core.  There are also GPIO irqs, and potentially board IRQs
> (the board has an I2C-based chip with a bunch of IRQ lines on it).
> 
> The only define that captures this now is 'NR_IRQS', even though we're
> trying to get rid of that.
> 
> Ultimately, the correct answer will be to get the various interrupt
> controllers using their own domains, but for now, this needs to be a
> larger value to avoid missing a bunch of the interrupts.

No. This should only be the number of interrupts for a controller as the
interrupt numbers in the device tree should be relative to a controller
and not the Linux virq number. The numbering in the dts needs to be
correct. You don't need a domain until you start getting the interrupts
from the dts.

Rob

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Cousson, Benoit]

Hi Thierry,

On 1/6/2012 3:28 PM, Thierry Reding wrote:
> The irq_domain_add() function needs the number of interrupts in the
> domain to properly initialize them. In addition the allocated domain
> is now returned by the irq_domain_{add,generate}_simple() helpers.

[...]

> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> index d587560..bf67781 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = {
>   static void __init omap_generic_init(void)
>   {
>   	struct device_node *node = of_find_matching_node(NULL, intc_match);
> -	if (node)
> -		irq_domain_add_simple(node, 0);
> +	if (node) {
> +		struct irq_domain *domain;
> +		domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS);

The number of interrupts will depend on the OMAP generation. That one is 
just valid for the 3430 INTC controller.
Since the previous code was using zero, I guess that using 0 there 
should be fine.

Moreover, that piece of code should not exist anymore on 3.3 if the 
series I sent last month to leverage Rob's DT interrupt init is merged [1].

I've just ping Rob and Grant on that series to get a status.

Regards,
Benoit


[1] http://www.spinics.net/lists/linux-omap/msg62124.html

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[David Brown]

On Fri, Jan 06, 2012 at 10:26:17AM -0600, Rob Herring wrote:
> On 01/06/2012 10:07 AM, David Brown wrote:
> > On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote:
> >> diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c
> >> index 0a11342..a50c7e2 100644
> >> --- a/arch/arm/mach-msm/board-msm8x60.c
> >> +++ b/arch/arm/mach-msm/board-msm8x60.c
> >> @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void)
> >>  
> >>  	node = of_find_matching_node_by_address(NULL, msm_dt_gic_match,
> >>  			MSM8X60_QGIC_DIST_PHYS);
> >> -	if (node)
> >> -		irq_domain_add_simple(node, GIC_SPI_START);
> >> +	if (node) {
> >> +		struct irq_domain *domain = irq_domain_add_simple(node,
> >> +				GIC_SPI_START, NR_MSM_IRQS);
> >> +		WARN_ON(IS_ERR(domain));
> >> +	}
> >>  
> >>  	if (of_machine_is_compatible("qcom,msm8660-surf")) {
> >>  		printk(KERN_INFO "Init surf UART registers\n");
> > 
> > This is probably a consequence of MSM not really being "simple", but
> > just using that.  However, NR_MSM_IRQS is only the number of IRQs on
> > the MSM core.  There are also GPIO irqs, and potentially board IRQs
> > (the board has an I2C-based chip with a bunch of IRQ lines on it).
> > 
> > The only define that captures this now is 'NR_IRQS', even though we're
> > trying to get rid of that.
> > 
> > Ultimately, the correct answer will be to get the various interrupt
> > controllers using their own domains, but for now, this needs to be a
> > larger value to avoid missing a bunch of the interrupts.
> 
> No. This should only be the number of interrupts for a controller as the
> interrupt numbers in the device tree should be relative to a controller
> and not the Linux virq number. The numbering in the dts needs to be
> correct. You don't need a domain until you start getting the interrupts
> from the dts.

Yes, _should only be_.  And, you're probably right that we shouldn't
change this, and should use the NR_MSM_IRQS.  When a DTS device needs
to use a GPIO IRQ, the GPIO controller should properly register its
own domain.  The only IRQ in the DT is an MSM IRQ, so this doesn't
actually break anything.

So, Thierry, please leave it as NR_MSM_IRQS as in your original
patch.  In that form.

Acked-by: David Brown <davidb-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Grant Likely]

On Fri, Jan 06, 2012 at 05:20:16PM +0100, Thierry Reding wrote:
> * Grant Likely wrote:
> > Hi Thierry,
> > 
> > On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding
> > <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > > The irq_domain_add() function needs the number of interrupts in the
> > > domain to properly initialize them. In addition the allocated domain
> > > is now returned by the irq_domain_{add,generate}_simple() helpers.
> > 
> > The commit text should also include the justification for renaming
> > irq_domain_create_simple() -> irq_domain_add_simple()
> 
> Actually the commit only fixes up the comment. The function has always been
> called irq_domain_add_simple().
> 
> For reference, this was introduced in commit 7e71330.

Hahaha.  Oops, you're right.  :-)

> 
> > >        domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> > > -       if (!domain) {
> > > -               WARN_ON(1);
> > > -               return;
> > > -       }
> > > +       if (!domain)
> > > +               return ERR_PTR(-ENOMEM);
> > 
> > Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO).
> 
> Returning NULL here is probably okay. Can the ERR_PTR stay in
> irq_domain_generate_simple(), though? It has two error conditions and
> handling both by returning NULL may not be what we want.

No.  ERR_PTR is a horrible pattern because you cannot tell by looking
at a prototype that returns a pointer whether or not the correct
failure test is "if (!ptr)" or "if (IS_ERR(ptr))".  Unless it is
absolutely critical for an error code to be returned (which isn't the
case here) I will not accept new code that uses ERR_PTR().

In this case, if irq_domain_add_simple() fails, then something is very
wrong.  I'd much rather the routine complain loudly regardless of the
error condition.

Actually, looking again at irq_domain_generate_simple() it should
probably succeed even if it cannot find a matching node since an
irq_domain does more than just device tree translation.  Although,
irq_domain_generate_simple() is a stop-gap solution that will
eventually be removed.

g.

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Shawn Guo]

Hi Thierry,

On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote:
> The irq_domain_add() function needs the number of interrupts in the
> domain to properly initialize them. In addition the allocated domain
> is now returned by the irq_domain_{add,generate}_simple() helpers.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
...
>  arch/arm/mach-imx/imx51-dt.c          |   13 +++++++++++--
>  arch/arm/mach-imx/imx53-dt.c          |   13 +++++++++++--
>  arch/arm/mach-imx/mach-imx6q.c        |    4 +++-
...
>  arch/arm/plat-mxc/include/mach/irqs.h |    1 +
>  arch/arm/plat-mxc/tzic.c              |    2 --

Thanks for patching imx.  It looks good to me, except a couple minor
comments below.

...

> diff --git a/arch/arm/mach-imx/imx51-dt.c b/arch/arm/mach-imx/imx51-dt.c
> index e6bad17..72bf94c 100644
> --- a/arch/arm/mach-imx/imx51-dt.c
> +++ b/arch/arm/mach-imx/imx51-dt.c
> @@ -47,7 +47,12 @@ static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = {
>  static int __init imx51_tzic_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
> -	irq_domain_add_simple(np, 0);
> +	struct irq_domain *domain;
> +
> +	domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
> +
>  	return 0;
>  }
>  
> @@ -55,9 +60,13 @@ static int __init imx51_gpio_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
>  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +	struct irq_domain *domain;
>  
>  	gpio_irq_base -= 32;
> -	irq_domain_add_simple(np, gpio_irq_base);
> +
> +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
>  
>  	return 0;
>  }
> diff --git a/arch/arm/mach-imx/imx53-dt.c b/arch/arm/mach-imx/imx53-dt.c
> index 05ebb3e..ccae620 100644
> --- a/arch/arm/mach-imx/imx53-dt.c
> +++ b/arch/arm/mach-imx/imx53-dt.c
> @@ -51,7 +51,12 @@ static const struct of_dev_auxdata imx53_auxdata_lookup[] __initconst = {
>  static int __init imx53_tzic_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
> -	irq_domain_add_simple(np, 0);
> +	struct irq_domain *domain;
> +
> +	domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
> +
>  	return 0;
>  }
>  
> @@ -59,9 +64,13 @@ static int __init imx53_gpio_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
>  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +	struct irq_domain *domain;
>  
>  	gpio_irq_base -= 32;
> -	irq_domain_add_simple(np, gpio_irq_base);
> +
> +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
>  
>  	return 0;
>  }
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index c257281..9ed7812 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -95,9 +95,11 @@ static int __init imx6q_gpio_add_irq_domain(struct device_node *np,
>  				struct device_node *interrupt_parent)
>  {
>  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> +	struct irq_domain *domain;
>  
>  	gpio_irq_base -= 32;
> -	irq_domain_add_simple(np, gpio_irq_base);
> +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> +	WARN_ON(IS_ERR(domain));

Why do we handle the error in a different pattern that is used for
all above?

>  
>  	return 0;
>  }

...

> diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h
> index fd9efb0..2fda5aa 100644
> --- a/arch/arm/plat-mxc/include/mach/irqs.h
> +++ b/arch/arm/plat-mxc/include/mach/irqs.h
> @@ -54,6 +54,7 @@
>  /* REVISIT: Add IPU irqs on IMX51 */
>  
>  #define NR_IRQS			(MXC_IPU_IRQ_START + MX3_IPU_IRQS)
> +#define TZIC_NUM_IRQS		128
>  
>  extern int imx_irq_set_priority(unsigned char irq, unsigned char prio);
>  
I just made a small change on top of yours.  Can you please consider
to amend it to your patch if it looks sane to you?

diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h
index 2fda5aa..70376e0 100644
--- a/arch/arm/plat-mxc/include/mach/irqs.h
+++ b/arch/arm/plat-mxc/include/mach/irqs.h
@@ -13,19 +13,20 @@

 #include <asm-generic/gpio.h>

+#define GIC_NUM_IRQS           160
+#define TZIC_NUM_IRQS          128
+#define AVIC_NUM_IRQS          64
+
 /*
- * SoCs with GIC interrupt controller have 160 IRQs, those with TZIC
- * have 128 IRQs, and those with AVIC have 64.
- *
  * To support single image, the biggest number should be defined on
  * top of the list.
  */
 #if defined CONFIG_ARM_GIC
-#define MXC_INTERNAL_IRQS      160
+#define MXC_INTERNAL_IRQS      GIC_NUM_IRQS
 #elif defined CONFIG_MXC_TZIC
-#define MXC_INTERNAL_IRQS      128
+#define MXC_INTERNAL_IRQS      TZIC_NUM_IRQS
 #else
-#define MXC_INTERNAL_IRQS      64
+#define MXC_INTERNAL_IRQS      AVIC_NUM_IRQS
 #endif

 #define MXC_GPIO_IRQ_START     MXC_INTERNAL_IRQS
@@ -54,7 +55,6 @@
 /* REVISIT: Add IPU irqs on IMX51 */

 #define NR_IRQS                        (MXC_IPU_IRQ_START + MX3_IPU_IRQS)
-#define TZIC_NUM_IRQS          128

 extern int imx_irq_set_priority(unsigned char irq, unsigned char prio);

-- 
Regards,
Shawn

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1064 bytes --]

* Grant Likely wrote:
> No.  ERR_PTR is a horrible pattern because you cannot tell by looking
> at a prototype that returns a pointer whether or not the correct
> failure test is "if (!ptr)" or "if (IS_ERR(ptr))".  Unless it is
> absolutely critical for an error code to be returned (which isn't the
> case here) I will not accept new code that uses ERR_PTR().
> 
> In this case, if irq_domain_add_simple() fails, then something is very
> wrong.  I'd much rather the routine complain loudly regardless of the
> error condition.

Okay, I'll keep the WARN_ON(1) and simply return NULL.

> Actually, looking again at irq_domain_generate_simple() it should
> probably succeed even if it cannot find a matching node since an
> irq_domain does more than just device tree translation.  Although,
> irq_domain_generate_simple() is a stop-gap solution that will
> eventually be removed.

So I'll just handle errors in irq_domain_generate_simple() the same way as in
irq_domain_add_simple() and will return NULL if no matching node is found.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2822 bytes --]

* Shawn Guo wrote:
> On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote:
[...]
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> > index c257281..9ed7812 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -95,9 +95,11 @@ static int __init imx6q_gpio_add_irq_domain(struct device_node *np,
> >  				struct device_node *interrupt_parent)
> >  {
> >  	static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS;
> > +	struct irq_domain *domain;
> >  
> >  	gpio_irq_base -= 32;
> > -	irq_domain_add_simple(np, gpio_irq_base);
> > +	domain = irq_domain_add_simple(np, gpio_irq_base, 32);
> > +	WARN_ON(IS_ERR(domain));
> 
> Why do we handle the error in a different pattern that is used for
> all above?

Because I wasn't paying attention =) It should be returning an error of
course. With the changes that Grant requested, domain will be NULL on error
and I guess returning -ENOMEM would be fine here (and in the above hunks).

It is of course different behaviour because the code would previously
continue execution and ignore the error, but I guess that's okay since
without the IRQ domain being registered the board will not work properly
anyway.

> I just made a small change on top of yours.  Can you please consider
> to amend it to your patch if it looks sane to you?
> 
> diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h
> index 2fda5aa..70376e0 100644
> --- a/arch/arm/plat-mxc/include/mach/irqs.h
> +++ b/arch/arm/plat-mxc/include/mach/irqs.h
> @@ -13,19 +13,20 @@
> 
>  #include <asm-generic/gpio.h>
> 
> +#define GIC_NUM_IRQS           160
> +#define TZIC_NUM_IRQS          128
> +#define AVIC_NUM_IRQS          64
> +
>  /*
> - * SoCs with GIC interrupt controller have 160 IRQs, those with TZIC
> - * have 128 IRQs, and those with AVIC have 64.
> - *
>   * To support single image, the biggest number should be defined on
>   * top of the list.
>   */
>  #if defined CONFIG_ARM_GIC
> -#define MXC_INTERNAL_IRQS      160
> +#define MXC_INTERNAL_IRQS      GIC_NUM_IRQS
>  #elif defined CONFIG_MXC_TZIC
> -#define MXC_INTERNAL_IRQS      128
> +#define MXC_INTERNAL_IRQS      TZIC_NUM_IRQS
>  #else
> -#define MXC_INTERNAL_IRQS      64
> +#define MXC_INTERNAL_IRQS      AVIC_NUM_IRQS
>  #endif
> 
>  #define MXC_GPIO_IRQ_START     MXC_INTERNAL_IRQS
> @@ -54,7 +55,6 @@
>  /* REVISIT: Add IPU irqs on IMX51 */
> 
>  #define NR_IRQS                        (MXC_IPU_IRQ_START + MX3_IPU_IRQS)
> -#define TZIC_NUM_IRQS          128
> 
>  extern int imx_irq_set_priority(unsigned char irq, unsigned char prio);
> 

Looks good. I'll add it to the patch along with the other requested changes.

Thanks,
Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] irqdomain: Initialize number of IRQs for simple domains

[Thierry Reding]

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

* Cousson, Benoit wrote:
> Hi Thierry,
> 
> On 1/6/2012 3:28 PM, Thierry Reding wrote:
> >The irq_domain_add() function needs the number of interrupts in the
> >domain to properly initialize them. In addition the allocated domain
> >is now returned by the irq_domain_{add,generate}_simple() helpers.
> 
> [...]
> 
> >diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> >index d587560..bf67781 100644
> >--- a/arch/arm/mach-omap2/board-generic.c
> >+++ b/arch/arm/mach-omap2/board-generic.c
> >@@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = {
> >  static void __init omap_generic_init(void)
> >  {
> >  	struct device_node *node = of_find_matching_node(NULL, intc_match);
> >-	if (node)
> >-		irq_domain_add_simple(node, 0);
> >+	if (node) {
> >+		struct irq_domain *domain;
> >+		domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS);
> 
> The number of interrupts will depend on the OMAP generation. That
> one is just valid for the 3430 INTC controller.
> Since the previous code was using zero, I guess that using 0 there
> should be fine.
> 
> Moreover, that piece of code should not exist anymore on 3.3 if the
> series I sent last month to leverage Rob's DT interrupt init is
> merged [1].
> 
> I've just ping Rob and Grant on that series to get a status.

Okay, so I guess we should wait for you patch to go in first and I'll update
this patch on top of that. I assume we can coordinate things in linux-next?

Thierry

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