[PATCH 3/6] ARM: at91/gpio: add DT support

On 12/16/2011 11:11 AM, Jamie Iles :

Hi Nicolas, Jean-Christophe,

This looks pretty good to me, but a couple of minor comments inline.

Jamie

On Thu, Dec 15, 2011 at 08:16:05PM +0100, Nicolas Ferre wrote:

quoted

From: Jean-Christophe PLAGNIOL-VILLARD <redacted>

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <redacted>
Signed-off-by: Nicolas Ferre <redacted>
---

[...]

quoted

diff --git a/arch/arm/mach-at91/gpio.c b/arch/arm/mach-at91/gpio.c
index 7ffb893..edb453a 100644
--- a/arch/arm/mach-at91/gpio.c
+++ b/arch/arm/mach-at91/gpio.c

@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/irqdomain.h>
+#include <linux/of_address.h>
 
 #include <mach/hardware.h>
 #include <mach/at91_pio.h>

@@ -624,40 +625,122 @@ static void at91_gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	}
 }
 
+#ifdef CONFIG_OF_GPIO
+static void __init of_at91_gpio_init_one(struct device_node *np)
+{
+	int alias_id;
+	struct at91_gpio_chip *at91_gpio;
+	const unsigned int *intspec;
+
+	if (!np)
+		return;
+
+	alias_id = of_alias_get_id(np, "gpio");
+	if (alias_id >= MAX_GPIO_BANKS) {
+		pr_err("at91_gpio, failed alias id(%d) > MAX_GPIO_BANKS(%d), ignoring.\n",
+								alias_id, MAX_GPIO_BANKS);
+		return;
+	}
+
+	at91_gpio = &gpio_chip[alias_id];
+	at91_gpio->chip.base = alias_id * at91_gpio->chip.ngpio;
+
+	at91_gpio->regbase = of_iomap(np, 0);
+	if (!at91_gpio->regbase) {
+		pr_err("at91_gpio.%d, failed to map registers, ignoring.\n",
+								alias_id);
+		return;
+	}
+
+	/* Get the interrupts property */
+	intspec = of_get_property(np, "interrupts", NULL);
+	BUG_ON(!intspec);
+	at91_gpio->id = be32_to_cpup(intspec);

Use of_property_read_u32()?  Also, BUG_ON() seems a bit harsh, better to 
print a warning and return an error.

quoted

+
+	at91_gpio->clock = clk_get_sys(NULL, at91_gpio->chip.label);
+	if (!at91_gpio->clock) {
+		pr_err("at91_gpio.%d, failed to get clock, ignoring.\n",
+								alias_id);
+		return;

Perhaps	unmap the regs here?

quoted

+	}
+
+	/* enable PIO controller's clock */
+	clk_enable(at91_gpio->clock);

Missing return value check.

quoted

+
+	at91_gpio->chip.of_node = np;
+	gpio_banks = max(gpio_banks, alias_id + 1);
+}
+
+static int __init of_at91_gpio_init(void)
+{
+	struct device_node *np = NULL;
+
+	/*
+	 * This isn't ideal, but it gets things hooked up until this
+	 * driver is converted into a platform_device
+	 */
+	do {
+		np = of_find_compatible_node(np, NULL, "atmel,at91rm9200-gpio");
+
+		of_at91_gpio_init_one(np);
+	} while (np);

for_each_compatible_node9)?

quoted

+
+	return gpio_banks > 0 ? 0 : -EINVAL;
+}
+#else
+static int __init of_at91_gpio_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
+static void __init at91_gpio_init_one(int i, u32 regbase, int id)
+{
+	struct at91_gpio_chip *at91_gpio = &gpio_chip[i];
+
+	at91_gpio->chip.base = i * at91_gpio->chip.ngpio;
+	at91_gpio->id = id;
+
+	at91_gpio->regbase = ioremap(regbase, 512);
+	if (!at91_gpio->regbase) {
+		pr_err("at91_gpio.%d, failed to map registers, ignoring.\n", i);
+		return;
+	}
+
+	at91_gpio->clock = clk_get_sys(NULL, at91_gpio->chip.label);
+	if (!at91_gpio->clock) {
+		pr_err("at91_gpio.%d, failed to get clock, ignoring.\n", i);
+		return;
+	}
+
+	/* enable PIO controller's clock */
+	clk_enable(at91_gpio->clock);

Missing return value check?

quoted

+	gpio_banks = max(gpio_banks, i + 1);
+}
+
 /*
  * Called from the processor-specific init to enable GPIO pin support.
  */
 void __init at91_gpio_init(struct at91_gpio_bank *data, int nr_banks)
 {
-	unsigned		i;
+	unsigned i;
 	struct at91_gpio_chip *at91_gpio, *last = NULL;
 
 	BUG_ON(nr_banks > MAX_GPIO_BANKS);
 
-	gpio_banks = nr_banks;
+	if (of_at91_gpio_init() < 0) {
+		/* no GPIO controller found in device tree */
+		for (i = 0; i < nr_banks; i++)
+			at91_gpio_init_one(i, data[i].regbase, data[i].id);
+	}
 
-	for (i = 0; i < nr_banks; i++) {
+	for (i = 0; i < gpio_banks; i++) {
 		at91_gpio = &gpio_chip[i];
 
-		at91_gpio->id = data[i].id;
-		at91_gpio->chip.base = i * 32;
-
-		at91_gpio->regbase = ioremap(data[i].regbase, 512);
-		if (!at91_gpio->regbase) {
-			pr_err("at91_gpio.%d, failed to map registers, ignoring.\n", i);
-			continue;
-		}
-
-		at91_gpio->clock = clk_get_sys(NULL, at91_gpio->chip.label);
-		if (!at91_gpio->clock) {
-			pr_err("at91_gpio.%d, failed to get clock, ignoring.\n", i);
-			continue;
-		}
-
-		/* enable PIO controller's clock */
-		clk_enable(at91_gpio->clock);
-
-		/* AT91SAM9263_ID_PIOCDE groups PIOC, PIOD, PIOE */
+		/*
+		 * GPIO controller are grouped on some SoC:
+		 * PIOC, PIOD and PIOE can share the same IRQ line
+		 */
 		if (last && last->id == at91_gpio->id)
 			last->next = at91_gpio;
 		last = at91_gpio;
-- 

Ok Jamie, Thanks for your comments: I will address all of them in a
second revision of this patch that I will post real-soon-now...

Best regards,
-- 
Nicolas Ferre