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

[Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>]

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:
>> 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;
>> -- 

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