[PATCH 1/6] ARM: tegra: Remove use of TEGRA_GPIO_TO_IRQ

[PATCH 1/6] 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>
---
This series is 3.4 material (obviously I guess!)

 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 789bdc9..c00aadb 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 ebac65f..d669847 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/6] dt: tegra gpio: Flesh out binding documentation

[Stephen Warren]

Document the required reg and interrupts properties.
Add a complete example.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/gpio/gpio_nvidia.txt       |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
index eb4b530..50b363c 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
@@ -2,7 +2,25 @@ NVIDIA Tegra 2 GPIO controller
 
 Required properties:
 - compatible : "nvidia,tegra20-gpio"
+- reg : Physical base address and length of the controller's registers.
+- interrupts : The interrupt outputs from the controller.
 - #gpio-cells : Should be two. The first cell is the pin number and the
   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.
+
+Example:
+
+gpio: gpio@6000d000 {
+	compatible = "nvidia,tegra20-gpio";
+	reg = < 0x6000d000 0x1000 >;
+	interrupts = < 0 32 0x04
+		       0 33 0x04
+		       0 34 0x04
+		       0 35 0x04
+		       0 55 0x04
+		       0 87 0x04
+		       0 89 0x04 >;
+	#gpio-cells = <2>;
+	gpio-controller;
+};
-- 
1.7.0.4

[PATCH 3/6] ARM: dt: tegra30.dtsi: Reformat gpio's interrupts property

[Stephen Warren]

The new content matches tegra20.dtsi, and is < 80 columns.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30.dtsi |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index ee7db98..e5d1406 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -55,7 +55,13 @@
 	gpio: gpio@6000d000 {
 		compatible = "nvidia,tegra30-gpio", "nvidia,tegra20-gpio";
 		reg = < 0x6000d000 0x1000 >;
-		interrupts = < 0 32 0x04 0 33 0x04 0 34 0x04 0 35 0x04 0 55 0x04 0 87 0x04 0 89 0x04 >;
+		interrupts = < 0 32 0x04
+			       0 33 0x04
+			       0 34 0x04
+			       0 35 0x04
+			       0 55 0x04
+			       0 87 0x04
+			       0 89 0x04 >;
 		#gpio-cells = <2>;
 		gpio-controller;
 	};
-- 
1.7.0.4

[PATCH 4/6] ARM: dt: tegra30.dtsi: Add extra GPIO interrupt

[Stephen Warren]

The Tegra30 GPIO controller has one more bank than Tegra20, and hence
has one more interrupt.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30.dtsi |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index e5d1406..2b3f6cd 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -61,7 +61,8 @@
 			       0 35 0x04
 			       0 55 0x04
 			       0 87 0x04
-			       0 89 0x04 >;
+			       0 89 0x04
+			       0 125 0x04 >;
 		#gpio-cells = <2>;
 		gpio-controller;
 	};
-- 
1.7.0.4

[PATCH 5/6] 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.

Describe interrupt-related properties in the device tree binding docs,
and the contents of "child" node interrupts property.

Update tegra*.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>
---
This patch depends on:
http://ftp.arm.linux.org.uk/pub/armlinux/kernel/git-cur/linux-2.6-arm.git
devel-stable c87fb57346fc7653ace98769f148e0dcd88ac1ee
---
 .../devicetree/bindings/gpio/gpio_nvidia.txt       |   12 +++++++++
 arch/arm/boot/dts/tegra20.dtsi                     |    2 +
 arch/arm/boot/dts/tegra30.dtsi                     |    2 +
 arch/arm/mach-tegra/include/mach/gpio-tegra.h      |    2 -
 drivers/gpio/gpio-tegra.c                          |   25 ++++++++++++++-----
 5 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
index 50b363c..d114e19 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
@@ -8,6 +8,16 @@ 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.
 
 Example:
 
@@ -23,4 +33,6 @@ gpio: gpio@6000d000 {
 		       0 89 0x04 >;
 	#gpio-cells = <2>;
 	gpio-controller;
+	#interrupt-cells = <2>;
+	interrupt-controller;
 };
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 3da7afd..0cdc4a6 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -75,6 +75,8 @@
 			       0 89 0x04 >;
 		#gpio-cells = <2>;
 		gpio-controller;
+		#interrupt-cells = <2>;
+		interrupt-controller;
 	};
 
 	pinmux: pinmux@70000000 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 2b3f6cd..83024c0 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -65,6 +65,8 @@
 			       0 125 0x04 >;
 		#gpio-cells = <2>;
 		gpio-controller;
+		#interrupt-cells = <2>;
+		interrupt-controller;
 	};
 
 	serial@70006000 {
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..c877a33 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,7 @@ struct tegra_gpio_bank {
 #endif
 };
 
-
+static struct irq_domain irq_domain;
 static void __iomem *regs;
 static struct tegra_gpio_bank tegra_gpio_banks[7];
 
@@ -139,7 +140,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 +156,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 +344,16 @@ 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_simple_ops;
+	irq_domain.of_node = pdev->dev.of_node;
+	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 +399,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

[PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Stephen Warren]

Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
banks. Allow the number of banks to be configured at run-time by the
device tree.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
This patch depends on:
http://ftp.arm.linux.org.uk/pub/armlinux/kernel/git-cur/linux-2.6-arm.git
devel-stable 44986ab056076e9dc9fb9f8b4729afef7fa72616
---
 .../devicetree/bindings/gpio/gpio_nvidia.txt       |    4 ++
 arch/arm/boot/dts/tegra20.dtsi                     |    1 +
 arch/arm/boot/dts/tegra30.dtsi                     |    1 +
 drivers/gpio/gpio-tegra.c                          |   41 +++++++++++++++----
 4 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
index d114e19..f9c2cc2 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
@@ -3,6 +3,9 @@ NVIDIA Tegra 2 GPIO controller
 Required properties:
 - compatible : "nvidia,tegra20-gpio"
 - reg : Physical base address and length of the controller's registers.
+- nvidia,num-banks : The number of GPIO banks. This should be 7 for
+  Tegra20 and 8 for Tegra30. This must match the number of interrupt
+  specifiers in the interrupts property.
 - interrupts : The interrupt outputs from the controller.
 - #gpio-cells : Should be two. The first cell is the pin number and the
   second cell is used to specify optional parameters:
@@ -24,6 +27,7 @@ Example:
 gpio: gpio@6000d000 {
 	compatible = "nvidia,tegra20-gpio";
 	reg = < 0x6000d000 0x1000 >;
+	nvidia,num-banks = <7>;
 	interrupts = < 0 32 0x04
 		       0 33 0x04
 		       0 34 0x04
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 0cdc4a6..853a5c6 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -66,6 +66,7 @@
 	gpio: gpio@6000d000 {
 		compatible = "nvidia,tegra20-gpio";
 		reg = < 0x6000d000 0x1000 >;
+		nvidia,num-banks = <7>;
 		interrupts = < 0 32 0x04
 			       0 33 0x04
 			       0 34 0x04
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 83024c0..368cbb3 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -55,6 +55,7 @@
 	gpio: gpio@6000d000 {
 		compatible = "nvidia,tegra30-gpio", "nvidia,tegra20-gpio";
 		reg = < 0x6000d000 0x1000 >;
+		nvidia,num-banks = <8>;
 		interrupts = < 0 32 0x04
 			       0 33 0x04
 			       0 34 0x04
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index c877a33..d27ca13 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -77,7 +77,8 @@ struct tegra_gpio_bank {
 
 static struct irq_domain irq_domain;
 static void __iomem *regs;
-static struct tegra_gpio_bank tegra_gpio_banks[7];
+static u32 tegra_gpio_bank_count;
+static struct tegra_gpio_bank *tegra_gpio_banks;
 
 static inline void tegra_gpio_writel(u32 val, u32 reg)
 {
@@ -274,7 +275,7 @@ void tegra_gpio_resume(void)
 
 	local_irq_save(flags);
 
-	for (b = 0; b < ARRAY_SIZE(tegra_gpio_banks); b++) {
+	for (b = 0; b < tegra_gpio_bank_count; b++) {
 		struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
 
 		for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
@@ -297,7 +298,7 @@ void tegra_gpio_suspend(void)
 	int p;
 
 	local_irq_save(flags);
-	for (b = 0; b < ARRAY_SIZE(tegra_gpio_banks); b++) {
+	for (b = 0; b < tegra_gpio_bank_count; b++) {
 		struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
 
 		for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
@@ -338,23 +339,45 @@ static struct lock_class_key gpio_lock_class;
 
 static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 {
+	int irq_base;
 	struct resource *res;
 	struct tegra_gpio_bank *bank;
 	int gpio;
 	int i;
 	int j;
 
-	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
-	if (irq_domain.irq_base < 0) {
+	if (pdev->dev.of_node) {
+		if (of_property_read_u32(pdev->dev.of_node, "nvidia,num-banks",
+					 &tegra_gpio_bank_count) < 0) {
+			dev_err(&pdev->dev,
+				"Missing property 'nvidia,num-banks'\n");
+			return -ENODEV;
+		}
+	}
+	if (!tegra_gpio_bank_count)
+		tegra_gpio_bank_count = 7;
+	tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
+
+	tegra_gpio_banks = devm_kzalloc(&pdev->dev,
+			tegra_gpio_bank_count * sizeof(*tegra_gpio_banks),
+			GFP_KERNEL);
+	if (!tegra_gpio_banks) {
+		dev_err(&pdev->dev, "Couldn't allocate bank structure\n");
+		return -ENODEV;
+	}
+
+	irq_base = irq_alloc_descs(-1, 0, tegra_gpio_chip.ngpio, 0);
+	if (irq_base < 0) {
 		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
 		return -ENODEV;
 	}
-	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.irq_base = irq_base;
+	irq_domain.nr_irq = tegra_gpio_chip.ngpio;
 	irq_domain.ops = &irq_domain_simple_ops;
 	irq_domain.of_node = pdev->dev.of_node;
 	irq_domain_add(&irq_domain);
 
-	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
+	for (i = 0; i < tegra_gpio_bank_count; i++) {
 		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 		if (!res) {
 			dev_err(&pdev->dev, "Missing IRQ resource\n");
@@ -398,7 +421,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 
 	gpiochip_add(&tegra_gpio_chip);
 
-	for (gpio = 0; gpio < TEGRA_NR_GPIOS; gpio++) {
+	for (gpio = 0; gpio < tegra_gpio_chip.ngpio; gpio++) {
 		int irq = irq_domain_to_irq(&irq_domain, gpio);
 		/* No validity check; all Tegra GPIOs are valid IRQs */
 
@@ -411,7 +434,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 		set_irq_flags(irq, IRQF_VALID);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
+	for (i = 0; i < tegra_gpio_bank_count; i++) {
 		bank = &tegra_gpio_banks[i];
 
 		irq_set_chained_handler(bank->irq, tegra_gpio_irq_handler);
-- 
1.7.0.4

Re: [PATCH 3/6] ARM: dt: tegra30.dtsi: Reformat gpio's interrupts property

[Grant Likely]

On Wed, Jan 04, 2012 at 11:39:35AM -0700, Stephen Warren wrote:
> The new content matches tegra20.dtsi, and is < 80 columns.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra30.dtsi |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index ee7db98..e5d1406 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -55,7 +55,13 @@
>  	gpio: gpio@6000d000 {
>  		compatible = "nvidia,tegra30-gpio", "nvidia,tegra20-gpio";
>  		reg = < 0x6000d000 0x1000 >;
> -		interrupts = < 0 32 0x04 0 33 0x04 0 34 0x04 0 35 0x04 0 55 0x04 0 87 0x04 0 89 0x04 >;
> +		interrupts = < 0 32 0x04
> +			       0 33 0x04
> +			       0 34 0x04
> +			       0 35 0x04
> +			       0 55 0x04
> +			       0 87 0x04
> +			       0 89 0x04 >;

Nit: I often find the following form convenient (even if it doesn't change the data generation):

		interrupts = < 0 32 0x04 >,
			     < 0 33 0x04 >,
			     < 0 34 0x04 >,
			     < 0 35 0x04 >,
			     < 0 55 0x04 >,
			     < 0 87 0x04 >,
			     < 0 89 0x04 >;

Otherwise:

Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

>  		#gpio-cells = <2>;
>  		gpio-controller;
>  	};
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] ARM: tegra: Remove use of TEGRA_GPIO_TO_IRQ

[Grant Likely]

On Wed, Jan 04, 2012 at 11:39:33AM -0700, Stephen Warren wrote:
> 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>

For the whole series:
Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

> ---
> This series is 3.4 material (obviously I guess!)
> 
>  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 789bdc9..c00aadb 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 ebac65f..d669847 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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Rob Herring]

On 01/04/2012 12:39 PM, Stephen Warren wrote:
> Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
> banks. Allow the number of banks to be configured at run-time by the
> device tree.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> This patch depends on:
> http://ftp.arm.linux.org.uk/pub/armlinux/kernel/git-cur/linux-2.6-arm.git
> devel-stable 44986ab056076e9dc9fb9f8b4729afef7fa72616
> ---
>  .../devicetree/bindings/gpio/gpio_nvidia.txt       |    4 ++
>  arch/arm/boot/dts/tegra20.dtsi                     |    1 +
>  arch/arm/boot/dts/tegra30.dtsi                     |    1 +
>  drivers/gpio/gpio-tegra.c                          |   41 +++++++++++++++----
>  4 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> index d114e19..f9c2cc2 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> @@ -3,6 +3,9 @@ NVIDIA Tegra 2 GPIO controller
>  Required properties:
>  - compatible : "nvidia,tegra20-gpio"
>  - reg : Physical base address and length of the controller's registers.
> +- nvidia,num-banks : The number of GPIO banks. This should be 7 for
> +  Tegra20 and 8 for Tegra30. This must match the number of interrupt
> +  specifiers in the interrupts property.

You can determine the number of banks based on the compatible property
rather than needing an additional property.

Otherwise, for the series:

Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Rob

>  - interrupts : The interrupt outputs from the controller.
>  - #gpio-cells : Should be two. The first cell is the pin number and the
>    second cell is used to specify optional parameters:
> @@ -24,6 +27,7 @@ Example:
>  gpio: gpio@6000d000 {
>  	compatible = "nvidia,tegra20-gpio";
>  	reg = < 0x6000d000 0x1000 >;
> +	nvidia,num-banks = <7>;
>  	interrupts = < 0 32 0x04
>  		       0 33 0x04
>  		       0 34 0x04
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 0cdc4a6..853a5c6 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -66,6 +66,7 @@
>  	gpio: gpio@6000d000 {
>  		compatible = "nvidia,tegra20-gpio";
>  		reg = < 0x6000d000 0x1000 >;
> +		nvidia,num-banks = <7>;
>  		interrupts = < 0 32 0x04
>  			       0 33 0x04
>  			       0 34 0x04
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index 83024c0..368cbb3 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -55,6 +55,7 @@
>  	gpio: gpio@6000d000 {
>  		compatible = "nvidia,tegra30-gpio", "nvidia,tegra20-gpio";
>  		reg = < 0x6000d000 0x1000 >;
> +		nvidia,num-banks = <8>;
>  		interrupts = < 0 32 0x04
>  			       0 33 0x04
>  			       0 34 0x04
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index c877a33..d27ca13 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -77,7 +77,8 @@ struct tegra_gpio_bank {
>  
>  static struct irq_domain irq_domain;
>  static void __iomem *regs;
> -static struct tegra_gpio_bank tegra_gpio_banks[7];
> +static u32 tegra_gpio_bank_count;
> +static struct tegra_gpio_bank *tegra_gpio_banks;
>  
>  static inline void tegra_gpio_writel(u32 val, u32 reg)
>  {
> @@ -274,7 +275,7 @@ void tegra_gpio_resume(void)
>  
>  	local_irq_save(flags);
>  
> -	for (b = 0; b < ARRAY_SIZE(tegra_gpio_banks); b++) {
> +	for (b = 0; b < tegra_gpio_bank_count; b++) {
>  		struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
>  
>  		for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
> @@ -297,7 +298,7 @@ void tegra_gpio_suspend(void)
>  	int p;
>  
>  	local_irq_save(flags);
> -	for (b = 0; b < ARRAY_SIZE(tegra_gpio_banks); b++) {
> +	for (b = 0; b < tegra_gpio_bank_count; b++) {
>  		struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
>  
>  		for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
> @@ -338,23 +339,45 @@ static struct lock_class_key gpio_lock_class;
>  
>  static int __devinit tegra_gpio_probe(struct platform_device *pdev)
>  {
> +	int irq_base;
>  	struct resource *res;
>  	struct tegra_gpio_bank *bank;
>  	int gpio;
>  	int i;
>  	int j;
>  
> -	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
> -	if (irq_domain.irq_base < 0) {
> +	if (pdev->dev.of_node) {
> +		if (of_property_read_u32(pdev->dev.of_node, "nvidia,num-banks",
> +					 &tegra_gpio_bank_count) < 0) {
> +			dev_err(&pdev->dev,
> +				"Missing property 'nvidia,num-banks'\n");
> +			return -ENODEV;
> +		}
> +	}
> +	if (!tegra_gpio_bank_count)
> +		tegra_gpio_bank_count = 7;
> +	tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
> +
> +	tegra_gpio_banks = devm_kzalloc(&pdev->dev,
> +			tegra_gpio_bank_count * sizeof(*tegra_gpio_banks),
> +			GFP_KERNEL);
> +	if (!tegra_gpio_banks) {
> +		dev_err(&pdev->dev, "Couldn't allocate bank structure\n");
> +		return -ENODEV;
> +	}
> +
> +	irq_base = irq_alloc_descs(-1, 0, tegra_gpio_chip.ngpio, 0);
> +	if (irq_base < 0) {
>  		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
>  		return -ENODEV;
>  	}
> -	irq_domain.nr_irq = TEGRA_NR_GPIOS;
> +	irq_domain.irq_base = irq_base;
> +	irq_domain.nr_irq = tegra_gpio_chip.ngpio;
>  	irq_domain.ops = &irq_domain_simple_ops;
>  	irq_domain.of_node = pdev->dev.of_node;
>  	irq_domain_add(&irq_domain);
>  
> -	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
> +	for (i = 0; i < tegra_gpio_bank_count; i++) {
>  		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>  		if (!res) {
>  			dev_err(&pdev->dev, "Missing IRQ resource\n");
> @@ -398,7 +421,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
>  
>  	gpiochip_add(&tegra_gpio_chip);
>  
> -	for (gpio = 0; gpio < TEGRA_NR_GPIOS; gpio++) {
> +	for (gpio = 0; gpio < tegra_gpio_chip.ngpio; gpio++) {
>  		int irq = irq_domain_to_irq(&irq_domain, gpio);
>  		/* No validity check; all Tegra GPIOs are valid IRQs */
>  
> @@ -411,7 +434,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
> +	for (i = 0; i < tegra_gpio_bank_count; i++) {
>  		bank = &tegra_gpio_banks[i];
>  
>  		irq_set_chained_handler(bank->irq, tegra_gpio_irq_handler);

RE: [PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Stephen Warren]

Rob Herring wrote at Wednesday, January 04, 2012 12:54 PM:
> On 01/04/2012 12:39 PM, Stephen Warren wrote:
> > Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
> > banks. Allow the number of banks to be configured at run-time by the
> > device tree.
...
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
...
> >  Required properties:
> >  - compatible : "nvidia,tegra20-gpio"
> >  - reg : Physical base address and length of the controller's registers.
> > +- nvidia,num-banks : The number of GPIO banks. This should be 7 for
> > +  Tegra20 and 8 for Tegra30. This must match the number of interrupt
> > +  specifiers in the interrupts property.
> 
> You can determine the number of banks based on the compatible property
> rather than needing an additional property.

That's certainly possible.

However, if say nvidia,tegraNNN-gpio has 9 banks, we then have to
explicitly edit the driver to know that, whereas by using a property,
we wouldn't have to change the driver at all to support a future GPIO
controller. So, isn't it better to explicitly represent this in DT?

Note that I have no idea how many GPIO banks our future chips will have,
so this might not turn out to save any work at all, but perhaps.

-- 
nvpublic

Re: [PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Grant Likely]

On Wed, Jan 4, 2012 at 1:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Rob Herring wrote at Wednesday, January 04, 2012 12:54 PM:
>> On 01/04/2012 12:39 PM, Stephen Warren wrote:
>> > Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
>> > banks. Allow the number of banks to be configured at run-time by the
>> > device tree.
> ...
>> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> ...
>> >  Required properties:
>> >  - compatible : "nvidia,tegra20-gpio"
>> >  - reg : Physical base address and length of the controller's registers.
>> > +- nvidia,num-banks : The number of GPIO banks. This should be 7 for
>> > +  Tegra20 and 8 for Tegra30. This must match the number of interrupt
>> > +  specifiers in the interrupts property.
>>
>> You can determine the number of banks based on the compatible property
>> rather than needing an additional property.
>
> That's certainly possible.
>
> However, if say nvidia,tegraNNN-gpio has 9 banks, we then have to
> explicitly edit the driver to know that, whereas by using a property,
> we wouldn't have to change the driver at all to support a future GPIO
> controller. So, isn't it better to explicitly represent this in DT?
>
> Note that I have no idea how many GPIO banks our future chips will have,
> so this might not turn out to save any work at all, but perhaps.

It's an engineering/design decision that requires taste and instinct.
Either approach is fine, you decide which one will be the best in the
long term.

g.

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

[Thierry Reding]

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

* Stephen Warren wrote:
> @@ -343,6 +344,16 @@ 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_simple_ops;
> +	irq_domain.of_node = pdev->dev.of_node;
> +	irq_domain_add(&irq_domain);

I was wondering: except for setting the nr_irq field this is pretty much what
irq_domain_add_simple() does. While I get that you need access to the domain
later on and cannot use the helper, I'm still wondering why the nr_irq field
is not initialized by irq_domain_add_simple().

I have a driver for a GPIO/IRQ expander that does exactly the same and was
always wondering why the irq_data.hwirq field isn't properly setup, and
irq_domain.nr_irq being 0 seems to be exactly the reason. So am I supposed to
not use irq_domain_add_simple() in this case or should we rather update the
helper to take a nr_irq parameter that can be used to initialize the nr_irq
field?

Thierry

P.S.: sorry for hijacking

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

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

[Jamie Iles]

Hi Stephen,

On Wed, Jan 04, 2012 at 11:39:37AM -0700, 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.
> 
> Describe interrupt-related properties in the device tree binding docs,
> and the contents of "child" node interrupts property.
> 
> Update tegra*.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>
> ---
> This patch depends on:
> http://ftp.arm.linux.org.uk/pub/armlinux/kernel/git-cur/linux-2.6-arm.git
> devel-stable c87fb57346fc7653ace98769f148e0dcd88ac1ee
> ---
>  .../devicetree/bindings/gpio/gpio_nvidia.txt       |   12 +++++++++
>  arch/arm/boot/dts/tegra20.dtsi                     |    2 +
>  arch/arm/boot/dts/tegra30.dtsi                     |    2 +
>  arch/arm/mach-tegra/include/mach/gpio-tegra.h      |    2 -
>  drivers/gpio/gpio-tegra.c                          |   25 ++++++++++++++-----
>  5 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> index 50b363c..d114e19 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> @@ -8,6 +8,16 @@ 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.

It looks to me like the tegra gpio driver can do IRQ_TYPE_EDGE_BOTH so I 
would expect 12 to be a valid combination too no?

Jamie

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

[Stephen Warren]

Jamie Iles wrote at Thursday, January 05, 2012 6:18 AM:
> On Wed, Jan 04, 2012 at 11:39:37AM -0700, 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.
...
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
...
> > +- #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.
> 
> It looks to me like the tegra gpio driver can do IRQ_TYPE_EDGE_BOTH so I
> would expect 12 to be a valid combination too no?

I believe EDGE_BOTH is 3 (OR of edge low and edge high)

-- 
nvpbulic

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

[Jamie Iles]

On Thu, Jan 05, 2012 at 08:47:15AM -0800, Stephen Warren wrote:
> Jamie Iles wrote at Thursday, January 05, 2012 6:18 AM:
> > On Wed, Jan 04, 2012 at 11:39:37AM -0700, 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.
> ...
> > > diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> ...
> > > +- #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.
> > 
> > It looks to me like the tegra gpio driver can do IRQ_TYPE_EDGE_BOTH so I
> > would expect 12 to be a valid combination too no?
> 
> I believe EDGE_BOTH is 3 (OR of edge low and edge high)

Doh!  Yes, you're quite right.  Sorry for the noise!

Jamie

Re: [PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Rob Herring]

On 01/04/2012 04:00 PM, Grant Likely wrote:
> On Wed, Jan 4, 2012 at 1:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Rob Herring wrote at Wednesday, January 04, 2012 12:54 PM:
>>> On 01/04/2012 12:39 PM, Stephen Warren wrote:
>>>> Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
>>>> banks. Allow the number of banks to be configured at run-time by the
>>>> device tree.
>> ...
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
>> ...
>>>>  Required properties:
>>>>  - compatible : "nvidia,tegra20-gpio"
>>>>  - reg : Physical base address and length of the controller's registers.
>>>> +- nvidia,num-banks : The number of GPIO banks. This should be 7 for
>>>> +  Tegra20 and 8 for Tegra30. This must match the number of interrupt
>>>> +  specifiers in the interrupts property.
>>>
>>> You can determine the number of banks based on the compatible property
>>> rather than needing an additional property.
>>
>> That's certainly possible.
>>
>> However, if say nvidia,tegraNNN-gpio has 9 banks, we then have to
>> explicitly edit the driver to know that, whereas by using a property,
>> we wouldn't have to change the driver at all to support a future GPIO
>> controller. So, isn't it better to explicitly represent this in DT?
>>
>> Note that I have no idea how many GPIO banks our future chips will have,
>> so this might not turn out to save any work at all, but perhaps.
> 
> It's an engineering/design decision that requires taste and instinct.
> Either approach is fine, you decide which one will be the best in the
> long term.

Agreed. I'm really fine with it either way.

Trying to predict future h/w is a bit pointless IMO. H/w designers
always find new ways to do things differently. i.MX family has gpio
interrupts hooked up 3 different ways for example. How would you handle
the case that the banks are sparsely implemented?

Is adding support for a different number of banks every couple of years
really an issue? It's much more important to have properties for which
change with every board.

Rob

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

[Stephen Warren]

Thierry Reding wrote at Thursday, January 05, 2012 12:23 AM:
> * Stephen Warren wrote:
> > @@ -343,6 +344,16 @@ 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_simple_ops;
> > +	irq_domain.of_node = pdev->dev.of_node;
> > +	irq_domain_add(&irq_domain);
> 
> I was wondering: except for setting the nr_irq field this is pretty much what
> irq_domain_add_simple() does. While I get that you need access to the domain
> later on and cannot use the helper, I'm still wondering why the nr_irq field
> is not initialized by irq_domain_add_simple().

I'm not completely sure, but I think irq_domain_add_simple() was initially
added as a transition measure; it looks like all the current users are for
a single top-level interrupt controller where the Linux IRQ numbers are
used directly in the .dts files. Once you add other interrupt controllers
into the mix, the API as-is starts to make less sense.

> I have a driver for a GPIO/IRQ expander that does exactly the same and was
> always wondering why the irq_data.hwirq field isn't properly setup, and
> irq_domain.nr_irq being 0 seems to be exactly the reason. So am I supposed to
> not use irq_domain_add_simple() in this case or should we rather update the
> helper to take a nr_irq parameter that can be used to initialize the nr_irq
> field?

I think updating the helper like that makes sense, and also have it return
the IRQ domain object. Grant, do you agree?

-- 
nvpublic

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

[Grant Likely]

On Thu, Jan 05, 2012 at 09:47:44AM -0800, Stephen Warren wrote:
> Thierry Reding wrote at Thursday, January 05, 2012 12:23 AM:
> > * Stephen Warren wrote:
> > > @@ -343,6 +344,16 @@ 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_simple_ops;
> > > +	irq_domain.of_node = pdev->dev.of_node;
> > > +	irq_domain_add(&irq_domain);
> > 
> > I was wondering: except for setting the nr_irq field this is pretty much what
> > irq_domain_add_simple() does. While I get that you need access to the domain
> > later on and cannot use the helper, I'm still wondering why the nr_irq field
> > is not initialized by irq_domain_add_simple().
> 
> I'm not completely sure, but I think irq_domain_add_simple() was initially
> added as a transition measure; it looks like all the current users are for
> a single top-level interrupt controller where the Linux IRQ numbers are
> used directly in the .dts files. Once you add other interrupt controllers
> into the mix, the API as-is starts to make less sense.
> 
> > I have a driver for a GPIO/IRQ expander that does exactly the same and was
> > always wondering why the irq_data.hwirq field isn't properly setup, and
> > irq_domain.nr_irq being 0 seems to be exactly the reason. So am I supposed to
> > not use irq_domain_add_simple() in this case or should we rather update the
> > helper to take a nr_irq parameter that can be used to initialize the nr_irq
> > field?
> 
> I think updating the helper like that makes sense, and also have it return
> the IRQ domain object. Grant, do you agree?

Update the helper.

g.

RE: [PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Stephen Warren]

Rob Herring wrote at Thursday, January 05, 2012 10:25 AM:
> On 01/04/2012 04:00 PM, Grant Likely wrote:
> > On Wed, Jan 4, 2012 at 1:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> >> Rob Herring wrote at Wednesday, January 04, 2012 12:54 PM:
> >>> On 01/04/2012 12:39 PM, Stephen Warren wrote:
> >>>> Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
> >>>> banks. Allow the number of banks to be configured at run-time by the
> >>>> device tree.
> >> ...
> >>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
> >> ...
> >>>>  Required properties:
> >>>>  - compatible : "nvidia,tegra20-gpio"
> >>>>  - reg : Physical base address and length of the controller's registers.
> >>>> +- nvidia,num-banks : The number of GPIO banks. This should be 7 for
> >>>> +  Tegra20 and 8 for Tegra30. This must match the number of interrupt
> >>>> +  specifiers in the interrupts property.
> >>>
> >>> You can determine the number of banks based on the compatible property
> >>> rather than needing an additional property.
> >>
> >> That's certainly possible.
> >>
> >> However, if say nvidia,tegraNNN-gpio has 9 banks, we then have to
> >> explicitly edit the driver to know that, whereas by using a property,
> >> we wouldn't have to change the driver at all to support a future GPIO
> >> controller. So, isn't it better to explicitly represent this in DT?
> >>
> >> Note that I have no idea how many GPIO banks our future chips will have,
> >> so this might not turn out to save any work at all, but perhaps.
> >
> > It's an engineering/design decision that requires taste and instinct.
> > Either approach is fine, you decide which one will be the best in the
> > long term.
> 
> Agreed. I'm really fine with it either way.
> 
> Trying to predict future h/w is a bit pointless IMO. H/w designers
> always find new ways to do things differently. i.MX family has gpio
> interrupts hooked up 3 different ways for example. How would you handle
> the case that the banks are sparsely implemented?
> 
> Is adding support for a different number of banks every couple of years
> really an issue? It's much more important to have properties for which
> change with every board.

Thinking about this some more, I'm tempted to rework this patch to remove
the extra DT property and just "detect" the number of banks based on the
length of the interrupts property, (well, actually the number of IRQ
resources that the platform device has) since each bank has its own
interrupt. Does anyone disagree with doing that?

-- 
nvpublic

Re: [PATCH 6/6] gpio: tegra: Parameterize the number of banks

[Rob Herring]

On 01/13/2012 02:55 PM, Stephen Warren wrote:
> Rob Herring wrote at Thursday, January 05, 2012 10:25 AM:
>> On 01/04/2012 04:00 PM, Grant Likely wrote:
>>> On Wed, Jan 4, 2012 at 1:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Rob Herring wrote at Wednesday, January 04, 2012 12:54 PM:
>>>>> On 01/04/2012 12:39 PM, Stephen Warren wrote:
>>>>>> Tegra20's GPIO controller has 7 banks, and Tegra30's controller has 8
>>>>>> banks. Allow the number of banks to be configured at run-time by the
>>>>>> device tree.
>>>> ...
>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio_nvidia.txt
>>>> ...
>>>>>>  Required properties:
>>>>>>  - compatible : "nvidia,tegra20-gpio"
>>>>>>  - reg : Physical base address and length of the controller's registers.
>>>>>> +- nvidia,num-banks : The number of GPIO banks. This should be 7 for
>>>>>> +  Tegra20 and 8 for Tegra30. This must match the number of interrupt
>>>>>> +  specifiers in the interrupts property.
>>>>>
>>>>> You can determine the number of banks based on the compatible property
>>>>> rather than needing an additional property.
>>>>
>>>> That's certainly possible.
>>>>
>>>> However, if say nvidia,tegraNNN-gpio has 9 banks, we then have to
>>>> explicitly edit the driver to know that, whereas by using a property,
>>>> we wouldn't have to change the driver at all to support a future GPIO
>>>> controller. So, isn't it better to explicitly represent this in DT?
>>>>
>>>> Note that I have no idea how many GPIO banks our future chips will have,
>>>> so this might not turn out to save any work at all, but perhaps.
>>>
>>> It's an engineering/design decision that requires taste and instinct.
>>> Either approach is fine, you decide which one will be the best in the
>>> long term.
>>
>> Agreed. I'm really fine with it either way.
>>
>> Trying to predict future h/w is a bit pointless IMO. H/w designers
>> always find new ways to do things differently. i.MX family has gpio
>> interrupts hooked up 3 different ways for example. How would you handle
>> the case that the banks are sparsely implemented?
>>
>> Is adding support for a different number of banks every couple of years
>> really an issue? It's much more important to have properties for which
>> change with every board.
> 
> Thinking about this some more, I'm tempted to rework this patch to remove
> the extra DT property and just "detect" the number of banks based on the
> length of the interrupts property, (well, actually the number of IRQ
> resources that the platform device has) since each bank has its own
> interrupt. Does anyone disagree with doing that?

Sounds fine too me.

Rob

Re: [PATCH 1/6] ARM: tegra: Remove use of TEGRA_GPIO_TO_IRQ

[Olof Johansson]

Hi,

On Wed, Jan 04, 2012 at 12:52:18PM -0700, Grant Likely wrote:
> On Wed, Jan 04, 2012 at 11:39:33AM -0700, Stephen Warren wrote:
> > 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>
> 
> For the whole series:
> Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>


Thanks, applied 1-6 to tegra for-3.4 (version 2 of patch 6).


-Olof