Hi Florian,
On Thu, Jan 15, 2009 at 04:46:48PM +0100, Florian Fainelli wrote:
> This patch converts the GPIO board code to use gpiolib
Very nice; a few minor things though:
> --- a/arch/mips/alchemy/common/gpio.c
> +++ b/arch/mips/alchemy/common/gpio.c
> -static void au1xxx_gpio2_write(unsigned gpio, int value)
> +static void au1000_gpio2_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> {
> - gpio -= AU1XXX_GPIO_BASE;
> + u32 mask = ((GPIO2_OUT_EN_MASK << offset) | (!!value << offset));
> + struct au1000_gpio_chip *gpch;
> + unsigned long flags;
> +
> + gpch = container_of(chip, struct au1000_gpio_chip, chip);
>
> - gpio2->output = (GPIO2_OUTPUT_ENABLE_MASK << gpio) | ((!!value) << gpio);
> + local_irq_save(flags);
> + writel(mask, gpch->regbase + AU1000_GPIO2_OUT);
> + local_irq_restore(flags);
Because of the 'enable | value' scheme I believe you don't require any
locking here.
> +struct au1000_gpio_chip au1000_gpio_chip[] = {
> + [0] = {
> + .regbase = (void __iomem *)SYS_BASE,
> + .chip = {
> + .label = "au1000-gpio1",
> + .direction_input = au1000_gpio1_direction_input,
> + .direction_output = au1000_gpio1_direction_output,
> + .get = au1000_gpio1_get,
> + .set = au1000_gpio1_set,
> + .base = 0,
> + .ngpio = 32,
> + },
> + },
> +#if !defined(CONFIG_SOC_AU1000)
> + [1] = {
> + .regbase = (void __iomem *)GPIO2_BASE,
> + .chip = {
> + .label = "au1000-gpio2",
> + .direction_input = au1000_gpio2_direction_input,
> + .direction_output = au1000_gpio2_direction_output,
> + .get = au1000_gpio2_get,
> + .set = au1000_gpio2_set,
> + .base = AU1XXX_GPIO_BASE,
> + .ngpio = 32,
> + },
> + },
> #endif
> - else
> - return au1xxx_gpio1_read(gpio);
> -}
> -EXPORT_SYMBOL(au1xxx_gpio_get_value);
> +};
[...]
> +static int __init au1000_gpio_init(void)
> {
> - if (gpio >= AU1XXX_GPIO_BASE)
> -#if defined(CONFIG_SOC_AU1000)
> - ;
> -#else
> - au1xxx_gpio2_write(gpio, value);
> -#endif
> - else
> - au1xxx_gpio1_write(gpio, value);
> -}
> -EXPORT_SYMBOL(au1xxx_gpio_set_value);
> + gpiochip_add(&au1000_gpio_chip[0].chip);
> +#if !defined(CONFIG_SOC_AU1000)
> + gpiochip_add(&au1000_gpio_chip[1].chip);
>
[...]
> +arch_initcall(au1000_gpio_init);
Can you please make the gpiolib registration dependent on a
CONFIG symbol? I.e. make the au1000_gpio{,2}_direction() and
friends calls globally visible but let the individual boards
decide whether they want to use the gpio numbering imposed by
this patch.
Long explanation: I maintain a number of modules with a common connector
interface, based on different architectures (sh, mips and arm so far).
I also maintain a few baseboards which can carry theese modules. Modules
provide 16 gpios numbered 0-15, which are used by the baseboards. Since
I need to keep the baseboard code free of arch-specific hacks, every module
provides its own gpio-chip which distributes the gpio-lib calls to various
on- and off-chip peripherals. On my alchemy board in particular, those 16
gpios are provided by a mixture of gpio1, gpio2 and FPGA based pins (yes
I repeatedly moanoed to the hw guys about this but pin multiplexing and
required features make a sane implementation difficult; but at least I was
allowed to write the VHDL for the fpga-based gpios).
If this explanation doesn't make sense I'll gladly whip up an addon patch.
> diff --git a/arch/mips/include/asm/mach-au1x00/gpio.h b/arch/mips/include/asm/mach-au1x00/gpio.h
> index 2dc61e0..34d9b72 100644
> --- a/arch/mips/include/asm/mach-au1x00/gpio.h
> +++ b/arch/mips/include/asm/mach-au1x00/gpio.h
> @@ -5,65 +5,29 @@
>
> #define AU1XXX_GPIO_BASE 200
please change this to AU1XXX_GPIO2_BASE (it's the base number
of the GPIO2 block pins after all)
Thanks!
Manuel Lauss
