[PATCH 1/2] arm/tegra: Remove use of TEGRA_GPIO_TO_IRQ

[PATCH 1/2] arm/tegra: Remove use of TEGRA_GPIO_TO_IRQ

[Stephen Warren]

Replace compile-time usage of TEGRA_GPIO_TO_IRQ with run-time calls to
gpio_to_irq(). This will allow the base IRQ number for the Tegra GPIO
driver to be dynamically allocated in a later patch.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/board-harmony.c  |    2 +-
 arch/arm/mach-tegra/board-seaboard.c |    5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
index 70ee674..60afd08 100644
--- a/arch/arm/mach-tegra/board-harmony.c
+++ b/arch/arm/mach-tegra/board-harmony.c
@@ -101,7 +101,6 @@ static struct wm8903_platform_data harmony_wm8903_pdata = {
 static struct i2c_board_info __initdata wm8903_board_info = {
 	I2C_BOARD_INFO("wm8903", 0x1a),
 	.platform_data = &harmony_wm8903_pdata,
-	.irq = TEGRA_GPIO_TO_IRQ(TEGRA_GPIO_CDC_IRQ),
 };
 
 static void __init harmony_i2c_init(void)
@@ -111,6 +110,7 @@ static void __init harmony_i2c_init(void)
 	platform_device_register(&tegra_i2c_device3);
 	platform_device_register(&tegra_i2c_device4);
 
+	wm8903_board_info.irq = gpio_to_irq(TEGRA_GPIO_CDC_IRQ);
 	i2c_register_board_info(0, &wm8903_board_info, 1);
 }
 
diff --git a/arch/arm/mach-tegra/board-seaboard.c b/arch/arm/mach-tegra/board-seaboard.c
index c1599eb..ce3c9a2 100644
--- a/arch/arm/mach-tegra/board-seaboard.c
+++ b/arch/arm/mach-tegra/board-seaboard.c
@@ -159,7 +159,6 @@ static struct platform_device *seaboard_devices[] __initdata = {
 
 static struct i2c_board_info __initdata isl29018_device = {
 	I2C_BOARD_INFO("isl29018", 0x44),
-	.irq = TEGRA_GPIO_TO_IRQ(TEGRA_GPIO_ISL29018_IRQ),
 };
 
 static struct i2c_board_info __initdata adt7461_device = {
@@ -183,7 +182,6 @@ static struct wm8903_platform_data wm8903_pdata = {
 static struct i2c_board_info __initdata wm8903_device = {
 	I2C_BOARD_INFO("wm8903", 0x1a),
 	.platform_data = &wm8903_pdata,
-	.irq = TEGRA_GPIO_TO_IRQ(TEGRA_GPIO_CDC_IRQ),
 };
 
 static int seaboard_ehci_init(void)
@@ -214,7 +212,10 @@ static void __init seaboard_i2c_init(void)
 	gpio_request(TEGRA_GPIO_ISL29018_IRQ, "isl29018");
 	gpio_direction_input(TEGRA_GPIO_ISL29018_IRQ);
 
+	isl29018_device.irq = gpio_to_irq(TEGRA_GPIO_ISL29018_IRQ);
 	i2c_register_board_info(0, &isl29018_device, 1);
+
+	wm8903_device.irq = gpio_to_irq(TEGRA_GPIO_CDC_IRQ);
 	i2c_register_board_info(0, &wm8903_device, 1);
 
 	i2c_register_board_info(3, &adt7461_device, 1);
-- 
1.7.0.4

[PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Stephen Warren]

Enhance  the driver to dynamically allocate the base IRQ number, and
create an IRQ domain for itself. The use of an IRQ domain ensures that
any device tree node interrupts properties are correctly parsed.

Fix the DT binding documentation to describe interrupt-related properties,
and the contents of "child" node interrupts property.

Update tegra20.dtsi to specify the required interrupt-related properties.

Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer
gives correct results since the IRQ numbers for GPIOs are dynamically
allocated.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/gpio/gpio_nvidia.txt       |   10 ++++++
 arch/arm/boot/dts/tegra20.dtsi                     |    2 +
 arch/arm/mach-tegra/include/mach/gpio-tegra.h      |    2 -
 drivers/gpio/gpio-tegra.c                          |   32 +++++++++++++++----
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
index eb4b530..20b991d 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
@@ -6,3 +6,13 @@ Required properties:
   second cell is used to specify optional parameters:
   - bit 0 specifies polarity (0 for normal, 1 for inverted)
 - gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 2.
+  The first cell is the GPIO number.
+  The second cell is used to specify flags:
+    bits[3:0] trigger type and level flags:
+      1 = low-to-high edge triggered.
+      2 = high-to-low edge triggered.
+      4 = active high level-sensitive.
+      8 = active low level-sensitive.
+      Valid combinations are 1, 2, 3, 4, 8.
+- interrupt-controller : Marks the device node as an interrupt controller.
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index db6f562..e25f4a6 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -79,6 +79,8 @@
 			       0 55 0x04
 			       0 87 0x04
 			       0 89 0x04 >;
+		interrupt-controller;
+		#interrupt-cells = <2>;
 		#gpio-cells = <2>;
 		gpio-controller;
 	};
diff --git a/arch/arm/mach-tegra/include/mach/gpio-tegra.h b/arch/arm/mach-tegra/include/mach/gpio-tegra.h
index 87d37fd..6140820 100644
--- a/arch/arm/mach-tegra/include/mach/gpio-tegra.h
+++ b/arch/arm/mach-tegra/include/mach/gpio-tegra.h
@@ -25,8 +25,6 @@
 
 #define TEGRA_NR_GPIOS		INT_GPIO_NR
 
-#define TEGRA_GPIO_TO_IRQ(gpio) (INT_GPIO_BASE + (gpio))
-
 struct tegra_gpio_table {
 	int	gpio;	/* GPIO number */
 	bool	enable;	/* Enable for GPIO at init? */
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 61044c8..9fa5783 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -25,6 +25,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
+#include <linux/irqdomain.h>
 
 #include <asm/mach/irq.h>
 
@@ -74,7 +75,8 @@ struct tegra_gpio_bank {
 #endif
 };
 
-
+static struct irq_domain irq_domain;
+static struct irq_domain_ops irq_domain_ops;
 static void __iomem *regs;
 static struct tegra_gpio_bank tegra_gpio_banks[7];
 
@@ -139,7 +141,7 @@ static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
 
 static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	return TEGRA_GPIO_TO_IRQ(offset);
+	return irq_domain_to_irq(&irq_domain, offset);
 }
 
 static struct gpio_chip tegra_gpio_chip = {
@@ -155,28 +157,28 @@ static struct gpio_chip tegra_gpio_chip = {
 
 static void tegra_gpio_irq_ack(struct irq_data *d)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 
 	tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
 }
 
 static void tegra_gpio_irq_mask(struct irq_data *d)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 
 	tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 0);
 }
 
 static void tegra_gpio_irq_unmask(struct irq_data *d)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 
 	tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 1);
 }
 
 static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	int port = GPIO_PORT(gpio);
 	int lvl_type;
@@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 	int i;
 	int j;
 
+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops = &irq_domain_ops;
+#ifdef CONFIG_OF
+	if (pdev->dev.of_node) {
+		irq_domain.of_node = pdev->dev.of_node;
+		irq_domain_ops.dt_translate =
+			irq_domain_simple_ops.dt_translate;
+	}
+#endif
+	irq_domain_add(&irq_domain);
+
 	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
 		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 		if (!res) {
@@ -388,7 +406,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 	gpiochip_add(&tegra_gpio_chip);
 
 	for (gpio = 0; gpio < TEGRA_NR_GPIOS; gpio++) {
-		int irq = TEGRA_GPIO_TO_IRQ(gpio);
+		int irq = irq_domain_to_irq(&irq_domain, gpio);
 		/* No validity check; all Tegra GPIOs are valid IRQs */
 
 		bank = &tegra_gpio_banks[GPIO_BANK(gpio)];
-- 
1.7.0.4

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Rob Herring]

On 11/30/2011 06:45 PM, Stephen Warren wrote:
> Enhance  the driver to dynamically allocate the base IRQ number, and
> create an IRQ domain for itself. The use of an IRQ domain ensures that
> any device tree node interrupts properties are correctly parsed.
> 
> Fix the DT binding documentation to describe interrupt-related properties,
> and the contents of "child" node interrupts property.
> 
> Update tegra20.dtsi to specify the required interrupt-related properties.
> 
> Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer
> gives correct results since the IRQ numbers for GPIOs are dynamically
> allocated.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Looks good. A few minor things.
> ---
>  .../devicetree/bindings/gpio/gpio_nvidia.txt       |   10 ++++++
>  arch/arm/boot/dts/tegra20.dtsi                     |    2 +
>  arch/arm/mach-tegra/include/mach/gpio-tegra.h      |    2 -
>  drivers/gpio/gpio-tegra.c                          |   32 +++++++++++++++----
>  4 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> index eb4b530..20b991d 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> @@ -6,3 +6,13 @@ Required properties:
>    second cell is used to specify optional parameters:
>    - bit 0 specifies polarity (0 for normal, 1 for inverted)
>  - gpio-controller : Marks the device node as a GPIO controller.
> +- #interrupt-cells : Should be 2.
> +  The first cell is the GPIO number.
> +  The second cell is used to specify flags:
> +    bits[3:0] trigger type and level flags:
> +      1 = low-to-high edge triggered.
> +      2 = high-to-low edge triggered.
> +      4 = active high level-sensitive.
> +      8 = active low level-sensitive.
> +      Valid combinations are 1, 2, 3, 4, 8.
> +- interrupt-controller : Marks the device node as an interrupt controller.
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index db6f562..e25f4a6 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -79,6 +79,8 @@
>  			       0 55 0x04
>  			       0 87 0x04
>  			       0 89 0x04 >;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
>  		#gpio-cells = <2>;
>  		gpio-controller;
>  	};
> diff --git a/arch/arm/mach-tegra/include/mach/gpio-tegra.h b/arch/arm/mach-tegra/include/mach/gpio-tegra.h
> index 87d37fd..6140820 100644
> --- a/arch/arm/mach-tegra/include/mach/gpio-tegra.h
> +++ b/arch/arm/mach-tegra/include/mach/gpio-tegra.h
> @@ -25,8 +25,6 @@
>  
>  #define TEGRA_NR_GPIOS		INT_GPIO_NR

Can INT_GPIO_NR be killed?

>  
> -#define TEGRA_GPIO_TO_IRQ(gpio) (INT_GPIO_BASE + (gpio))
> -
>  struct tegra_gpio_table {
>  	int	gpio;	/* GPIO number */
>  	bool	enable;	/* Enable for GPIO at init? */
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 61044c8..9fa5783 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -25,6 +25,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
> +#include <linux/irqdomain.h>
>  
>  #include <asm/mach/irq.h>
>  
> @@ -74,7 +75,8 @@ struct tegra_gpio_bank {
>  #endif
>  };
>  
> -
> +static struct irq_domain irq_domain;
> +static struct irq_domain_ops irq_domain_ops;
>  static void __iomem *regs;
>  static struct tegra_gpio_bank tegra_gpio_banks[7];
>  
> @@ -139,7 +141,7 @@ static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
>  
>  static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
> -	return TEGRA_GPIO_TO_IRQ(offset);
> +	return irq_domain_to_irq(&irq_domain, offset);
>  }
>  
>  static struct gpio_chip tegra_gpio_chip = {
> @@ -155,28 +157,28 @@ static struct gpio_chip tegra_gpio_chip = {
>  
>  static void tegra_gpio_irq_ack(struct irq_data *d)
>  {
> -	int gpio = d->irq - INT_GPIO_BASE;
> +	int gpio = d->hwirq;
>  
>  	tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
>  }
>  
>  static void tegra_gpio_irq_mask(struct irq_data *d)
>  {
> -	int gpio = d->irq - INT_GPIO_BASE;
> +	int gpio = d->hwirq;
>  
>  	tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 0);
>  }
>  
>  static void tegra_gpio_irq_unmask(struct irq_data *d)
>  {
> -	int gpio = d->irq - INT_GPIO_BASE;
> +	int gpio = d->hwirq;
>  
>  	tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 1);
>  }
>  
>  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
> -	int gpio = d->irq - INT_GPIO_BASE;
> +	int gpio = d->hwirq;
>  	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
>  	int port = GPIO_PORT(gpio);
>  	int lvl_type;
> @@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
>  	int i;
>  	int j;
>  
> +	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> +	if (irq_domain.irq_base < 0) {
> +		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +		return -ENODEV;
> +	}
> +	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> +	irq_domain.ops = &irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

> +#ifdef CONFIG_OF
> +	if (pdev->dev.of_node) {
> +		irq_domain.of_node = pdev->dev.of_node;
> +		irq_domain_ops.dt_translate =
> +			irq_domain_simple_ops.dt_translate;
> +	}
> +#endif

Then you don't need the ifdef (or the if statement).

> +	irq_domain_add(&irq_domain);
> +
>  	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
>  		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>  		if (!res) {
> @@ -388,7 +406,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
>  	gpiochip_add(&tegra_gpio_chip);
>  
>  	for (gpio = 0; gpio < TEGRA_NR_GPIOS; gpio++) {
> -		int irq = TEGRA_GPIO_TO_IRQ(gpio);
> +		int irq = irq_domain_to_irq(&irq_domain, gpio);
>  		/* No validity check; all Tegra GPIOs are valid IRQs */
>  
>  		bank = &tegra_gpio_banks[GPIO_BANK(gpio)];

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Jamie Iles]

On Thu, Dec 01, 2011 at 07:42:57AM -0600, Rob Herring wrote:
> On 11/30/2011 06:45 PM, Stephen Warren wrote:
> > Enhance  the driver to dynamically allocate the base IRQ number, and
> > create an IRQ domain for itself. The use of an IRQ domain ensures that
> > any device tree node interrupts properties are correctly parsed.
> > 
> > Fix the DT binding documentation to describe interrupt-related properties,
> > and the contents of "child" node interrupts property.
> > 
> > Update tegra20.dtsi to specify the required interrupt-related properties.
> > 
> > Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer
> > gives correct results since the IRQ numbers for GPIOs are dynamically
> > allocated.
> > 
> > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[...]
> >  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> >  {
> > -	int gpio = d->irq - INT_GPIO_BASE;
> > +	int gpio = d->hwirq;
> >  	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> >  	int port = GPIO_PORT(gpio);
> >  	int lvl_type;
> > @@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
> >  	int i;
> >  	int j;
> >  
> > +	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> > +	if (irq_domain.irq_base < 0) {
> > +		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> > +		return -ENODEV;
> > +	}
> > +	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> > +	irq_domain.ops = &irq_domain_ops;
> 
> Why don't you just use irq_domain_simple_ops?

This would need the patch I posted earlier 
(https://lkml.org/lkml/2011/12/1/109) so they can work for the 
!CONFIG_OF case ;-)

Jamie

RE: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Stephen Warren]

Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:
> On Thu, Dec 01, 2011 at 07:42:57AM -0600, Rob Herring wrote:
> > On 11/30/2011 06:45 PM, Stephen Warren wrote:
> > > Enhance  the driver to dynamically allocate the base IRQ number, and
> > > create an IRQ domain for itself. The use of an IRQ domain ensures that
> > > any device tree node interrupts properties are correctly parsed.
> > >
> > > Fix the DT binding documentation to describe interrupt-related properties,
> > > and the contents of "child" node interrupts property.
> > >
> > > Update tegra20.dtsi to specify the required interrupt-related properties.
> > >
> > > Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer
> > > gives correct results since the IRQ numbers for GPIOs are dynamically
> > > allocated.
> > >
> > > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> [...]
> > >  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > >  {
> > > -	int gpio = d->irq - INT_GPIO_BASE;
> > > +	int gpio = d->hwirq;
> > >  	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> > >  	int port = GPIO_PORT(gpio);
> > >  	int lvl_type;
> > > @@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
> > >  	int i;
> > >  	int j;
> > >
> > > +	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> > > +	if (irq_domain.irq_base < 0) {
> > > +		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> > > +		return -ENODEV;
> > > +	}
> > > +	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> > > +	irq_domain.ops = &irq_domain_ops;
> >
> > Why don't you just use irq_domain_simple_ops?
> 
> This would need the patch I posted earlier
> (https://lkml.org/lkml/2011/12/1/109) so they can work for the
> !CONFIG_OF case ;-)

Part of my reasoning was that simple_ops appeared to be intended for
DT-based controllers; is it safe to use those ops for a controller that
wasn't instantiated from DT? I know that right now, the only op in that
structure is dt_translate, and that won't ever be called for the non-DT
case, but is there a guarantee that more functions won't be added to
the simple ops, and that they won't assume DT is in use, and fail if
not?

If these are safe to use in the non-DT case, then yet I could build off
Jamie's patch, although managing the dependencies might be awkward.

-- 
nvpublic

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Jamie Iles]

On Thu, Dec 01, 2011 at 08:52:49AM -0800, Stephen Warren wrote:
> Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:
> > On Thu, Dec 01, 2011 at 07:42:57AM -0600, Rob Herring wrote:
> > > On 11/30/2011 06:45 PM, Stephen Warren wrote:
> > > > Enhance  the driver to dynamically allocate the base IRQ number, and
> > > > create an IRQ domain for itself. The use of an IRQ domain ensures that
> > > > any device tree node interrupts properties are correctly parsed.
> > > >
> > > > Fix the DT binding documentation to describe interrupt-related properties,
> > > > and the contents of "child" node interrupts property.
> > > >
> > > > Update tegra20.dtsi to specify the required interrupt-related properties.
> > > >
> > > > Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer
> > > > gives correct results since the IRQ numbers for GPIOs are dynamically
> > > > allocated.
> > > >
> > > > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > [...]
> > > >  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > > >  {
> > > > -	int gpio = d->irq - INT_GPIO_BASE;
> > > > +	int gpio = d->hwirq;
> > > >  	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> > > >  	int port = GPIO_PORT(gpio);
> > > >  	int lvl_type;
> > > > @@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
> > > >  	int i;
> > > >  	int j;
> > > >
> > > > +	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> > > > +	if (irq_domain.irq_base < 0) {
> > > > +		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> > > > +	irq_domain.ops = &irq_domain_ops;
> > >
> > > Why don't you just use irq_domain_simple_ops?
> > 
> > This would need the patch I posted earlier
> > (https://lkml.org/lkml/2011/12/1/109) so they can work for the
> > !CONFIG_OF case ;-)
> 
> Part of my reasoning was that simple_ops appeared to be intended for
> DT-based controllers; is it safe to use those ops for a controller that
> wasn't instantiated from DT? I know that right now, the only op in that
> structure is dt_translate, and that won't ever be called for the non-DT
> case, but is there a guarantee that more functions won't be added to
> the simple ops, and that they won't assume DT is in use, and fail if
> not?
> 
> If these are safe to use in the non-DT case, then yet I could build off
> Jamie's patch, although managing the dependencies might be awkward.

Yes, it's absolutely fine to use it just that irq_simple_ops isn't 
currently exported unless you have CONFIG_OF_IRQ set so you'd get an 
undefined reference for !CONFIG_OF at the moment.

Jamie

RE: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Stephen Warren]

Jamie Iles wrote at Thursday, December 01, 2011 9:55 AM:
> On Thu, Dec 01, 2011 at 08:52:49AM -0800, Stephen Warren wrote:
> > Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:
> > > On Thu, Dec 01, 2011 at 07:42:57AM -0600, Rob Herring wrote:
> > > > On 11/30/2011 06:45 PM, Stephen Warren wrote:
...
> > > > > +	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> > > > > +	if (irq_domain.irq_base < 0) {
> > > > > +		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> > > > > +	irq_domain.ops = &irq_domain_ops;
> > > >
> > > > Why don't you just use irq_domain_simple_ops?
> > >
> > > This would need the patch I posted earlier
> > > (https://lkml.org/lkml/2011/12/1/109) so they can work for the
> > > !CONFIG_OF case ;-)
> >
> > Part of my reasoning was that simple_ops appeared to be intended for
> > DT-based controllers; is it safe to use those ops for a controller that
> > wasn't instantiated from DT? I know that right now, the only op in that
> > structure is dt_translate, and that won't ever be called for the non-DT
> > case, but is there a guarantee that more functions won't be added to
> > the simple ops, and that they won't assume DT is in use, and fail if
> > not?
> >
> > If these are safe to use in the non-DT case, then yet I could build off
> > Jamie's patch, although managing the dependencies might be awkward.
> 
> Yes, it's absolutely fine to use it just that irq_simple_ops isn't
> currently exported unless you have CONFIG_OF_IRQ set so you'd get an
> undefined reference for !CONFIG_OF at the moment.

OK, sounds good.

So, I think we have a few options:
1) Merge my change as-is, and simplify it later once your patch is in.
2) Put your change in a branch, and merge it into both its usual place,
and the Tegra/ARM branches, so I can rebase my patch on top of it.
3) Have the usual maintainer ack it (I see Rob already did, but I think
Thomas is the maintainer here right?) and just put both patches into the
Tegra/ARM tree. This of course means non-Tegra branches have to wait for
it rather than the other way around.

(1) seems simplest, but (2) is probably doable. Thomas?

-- 
nvpublic

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Nicolas Ferre]

On 12/01/2011 09:57 PM, Stephen Warren :
> Jamie Iles wrote at Thursday, December 01, 2011 9:55 AM:
>> On Thu, Dec 01, 2011 at 08:52:49AM -0800, Stephen Warren wrote:
>>> Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:
>>>> On Thu, Dec 01, 2011 at 07:42:57AM -0600, Rob Herring wrote:
>>>>> On 11/30/2011 06:45 PM, Stephen Warren wrote:
> ...
>>>>>> +	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
>>>>>> +	if (irq_domain.irq_base<  0) {
>>>>>> +		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
>>>>>> +		return -ENODEV;
>>>>>> +	}
>>>>>> +	irq_domain.nr_irq = TEGRA_NR_GPIOS;
>>>>>> +	irq_domain.ops =&irq_domain_ops;
>>>>>
>>>>> Why don't you just use irq_domain_simple_ops?
>>>>
>>>> This would need the patch I posted earlier
>>>> (https://lkml.org/lkml/2011/12/1/109) so they can work for the
>>>> !CONFIG_OF case ;-)
>>>
>>> Part of my reasoning was that simple_ops appeared to be intended for
>>> DT-based controllers; is it safe to use those ops for a controller that
>>> wasn't instantiated from DT? I know that right now, the only op in that
>>> structure is dt_translate, and that won't ever be called for the non-DT
>>> case, but is there a guarantee that more functions won't be added to
>>> the simple ops, and that they won't assume DT is in use, and fail if
>>> not?
>>>
>>> If these are safe to use in the non-DT case, then yet I could build off
>>> Jamie's patch, although managing the dependencies might be awkward.
>>
>> Yes, it's absolutely fine to use it just that irq_simple_ops isn't
>> currently exported unless you have CONFIG_OF_IRQ set so you'd get an
>> undefined reference for !CONFIG_OF at the moment.
>
> OK, sounds good.
>
> So, I think we have a few options:
> 1) Merge my change as-is, and simplify it later once your patch is in.
> 2) Put your change in a branch, and merge it into both its usual place,
> and the Tegra/ARM branches, so I can rebase my patch on top of it.
> 3) Have the usual maintainer ack it (I see Rob already did, but I think
> Thomas is the maintainer here right?) and just put both patches into the
> Tegra/ARM tree. This of course means non-Tegra branches have to wait for
> it rather than the other way around.
>
> (1) seems simplest, but (2) is probably doable. Thomas?

I jump into the discussion to say that I am also interested by Jamie's 
patch. I am following the same path as Stephen at the moment with Atmel 
AT91... A chance I can read all your comments that are so valuable for 
me as well :-)

So, for me (2) will ease things...

Best regards,
-- 
Nicolas Ferre

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Shawn Guo]

Hi Thomas,

On Thu, Dec 01, 2011 at 11:05:15PM +0100, Nicolas Ferre wrote:
> On 12/01/2011 09:57 PM, Stephen Warren :
> >Jamie Iles wrote at Thursday, December 01, 2011 9:55 AM:
> >>On Thu, Dec 01, 2011 at 08:52:49AM -0800, Stephen Warren wrote:
> >>>Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:
> >>>>On Thu, Dec 01, 2011 at 07:42:57AM -0600, Rob Herring wrote:
> >>>>>On 11/30/2011 06:45 PM, Stephen Warren wrote:
> >...
> >>>>>>+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> >>>>>>+	if (irq_domain.irq_base<  0) {
> >>>>>>+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> >>>>>>+		return -ENODEV;
> >>>>>>+	}
> >>>>>>+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> >>>>>>+	irq_domain.ops =&irq_domain_ops;
> >>>>>
> >>>>>Why don't you just use irq_domain_simple_ops?
> >>>>
> >>>>This would need the patch I posted earlier
> >>>>(https://lkml.org/lkml/2011/12/1/109) so they can work for the
> >>>>!CONFIG_OF case ;-)
> >>>
> >>>Part of my reasoning was that simple_ops appeared to be intended for
> >>>DT-based controllers; is it safe to use those ops for a controller that
> >>>wasn't instantiated from DT? I know that right now, the only op in that
> >>>structure is dt_translate, and that won't ever be called for the non-DT
> >>>case, but is there a guarantee that more functions won't be added to
> >>>the simple ops, and that they won't assume DT is in use, and fail if
> >>>not?
> >>>
> >>>If these are safe to use in the non-DT case, then yet I could build off
> >>>Jamie's patch, although managing the dependencies might be awkward.
> >>
> >>Yes, it's absolutely fine to use it just that irq_simple_ops isn't
> >>currently exported unless you have CONFIG_OF_IRQ set so you'd get an
> >>undefined reference for !CONFIG_OF at the moment.
> >
> >OK, sounds good.
> >
> >So, I think we have a few options:
> >1) Merge my change as-is, and simplify it later once your patch is in.
> >2) Put your change in a branch, and merge it into both its usual place,
> >and the Tegra/ARM branches, so I can rebase my patch on top of it.
> >3) Have the usual maintainer ack it (I see Rob already did, but I think
> >Thomas is the maintainer here right?) and just put both patches into the
> >Tegra/ARM tree. This of course means non-Tegra branches have to wait for
> >it rather than the other way around.
> >
> >(1) seems simplest, but (2) is probably doable. Thomas?
> 
> I jump into the discussion to say that I am also interested by
> Jamie's patch. I am following the same path as Stephen at the moment
> with Atmel AT91... A chance I can read all your comments that are so
> valuable for me as well :-)
> 
> So, for me (2) will ease things...
> 
I'm sending Jamie's patch as prerequisite of my gpio interrupt
controller cleanup series to Arnd and Olof (arm-soc tree), so that
similar series for tegra and at91 from Stephen and Nicolas can base
on Jamie's patch too.

Please let us know if you do not want this way.

Regards,
Shawn

[1] [PATCH] irqdomain: export irq_domain_simple_ops for !CONFIG_OF
    https://lkml.org/lkml/2011/12/1/109

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Shawn Guo]

On Wed, Nov 30, 2011 at 05:45:36PM -0700, Stephen Warren wrote:
[...]
>  static void tegra_gpio_irq_ack(struct irq_data *d)
>  {
> -	int gpio = d->irq - INT_GPIO_BASE;
> +	int gpio = d->hwirq;
>  
Though it's working right now, I'm not sure it's safe enough.  This
only works when d->hwirq_base is 0, which is true for now.  But I doubt
it will be always true.  I guess hwirq_base was introduced there for
some reason.  When some day irqdomain starts using this field, the
above code starts being broken.  IMO, the way that generic-chip.c is
using to calculate the number, d->irq - gc->irq_base, is much more
safer.

-- 
Regards,
Shawn

>  	tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
>  }

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Rob Herring]

Shawn,

On 12/05/2011 12:55 AM, Shawn Guo wrote:
> On Wed, Nov 30, 2011 at 05:45:36PM -0700, Stephen Warren wrote:
> [...]
>>  static void tegra_gpio_irq_ack(struct irq_data *d)
>>  {
>> -	int gpio = d->irq - INT_GPIO_BASE;
>> +	int gpio = d->hwirq;
>>  
> Though it's working right now, I'm not sure it's safe enough.  This
> only works when d->hwirq_base is 0, which is true for now.  But I doubt
> it will be always true.  I guess hwirq_base was introduced there for
> some reason.  When some day irqdomain starts using this field, the
> above code starts being broken.  IMO, the way that generic-chip.c is
> using to calculate the number, d->irq - gc->irq_base, is much more
> safer.

It does work as the GIC hwirq_base is non-zero. It was introduced
exactly so that no conversion of hwirq is needed for functions like
this. hwirq_base is the starting point local to the controller
numbering. Say you have gpio controller with 16 lines, but only the
upper 8 lines have interrupt capability. Then you would set hwirq_base
to 8 and nr_irq to 8. Then hwirq will always be set to 8-15.

Rob

Re: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Shawn Guo]

On Mon, Dec 05, 2011 at 07:35:16AM -0600, Rob Herring wrote:
> Shawn,
> 
> On 12/05/2011 12:55 AM, Shawn Guo wrote:
> > On Wed, Nov 30, 2011 at 05:45:36PM -0700, Stephen Warren wrote:
> > [...]
> >>  static void tegra_gpio_irq_ack(struct irq_data *d)
> >>  {
> >> -	int gpio = d->irq - INT_GPIO_BASE;
> >> +	int gpio = d->hwirq;
> >>  
> > Though it's working right now, I'm not sure it's safe enough.  This
> > only works when d->hwirq_base is 0, which is true for now.  But I doubt
> > it will be always true.  I guess hwirq_base was introduced there for
> > some reason.  When some day irqdomain starts using this field, the
> > above code starts being broken.  IMO, the way that generic-chip.c is
> > using to calculate the number, d->irq - gc->irq_base, is much more
> > safer.
> 
> It does work as the GIC hwirq_base is non-zero. It was introduced
> exactly so that no conversion of hwirq is needed for functions like
> this. hwirq_base is the starting point local to the controller
> numbering. Say you have gpio controller with 16 lines, but only the
> upper 8 lines have interrupt capability. Then you would set hwirq_base
> to 8 and nr_irq to 8. Then hwirq will always be set to 8-15.
> 
Ah, ok.  In that case, it's safe to use.  Thanks for help understand
it.  I will use hwirq for my gpio-mxc patch then.

-- 
Regards,
Shawn

RE: [PATCH 2/2] gpio/tegra: Dynamically allocate IRQ base, and support DT

[Stephen Warren]

Shawn Guo wrote at Sunday, December 04, 2011 11:55 PM:
> On Wed, Nov 30, 2011 at 05:45:36PM -0700, Stephen Warren wrote:
> [...]
> >  static void tegra_gpio_irq_ack(struct irq_data *d)
> >  {
> > -	int gpio = d->irq - INT_GPIO_BASE;
> > +	int gpio = d->hwirq;
> >
> Though it's working right now, I'm not sure it's safe enough.  This
> only works when d->hwirq_base is 0, which is true for now.  But I doubt
> it will be always true.  I guess hwirq_base was introduced there for
> some reason.  When some day irqdomain starts using this field, the
> above code starts being broken.  IMO, the way that generic-chip.c is
> using to calculate the number, d->irq - gc->irq_base, is much more
> safer.

If I'm understanding IRQ domains correctly, the hwirq_base value is
picked by the driver when registering the IRQ domain. Hence, there's
no way for hwirq_base to not be zero without someone editing gpio-tegra.c,
and as part of doing that, the quoted code could be adjusted if required.
Does that seem reasonable?

Rob, you suggested the code above - what's your take?

-- 
nvpublic