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

[Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>]

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:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
[...]
> 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.

> +
> +	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?

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

Missing return value check.

> +
> +	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)?

> +
> +	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?

> +	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;
> -- 
> 1.7.5.4
>