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 <[email protected]> >> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <[email protected]> >> Signed-off-by: Nicolas Ferre <[email protected]> >> --- > [...] >> 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 _______________________________________________ devicetree-discuss mailing list [email protected] https://lists.ozlabs.org/listinfo/devicetree-discuss
