Imre Kaloz <ka...@openwrt.org> writes:
Hi,
I can't test it but it looks good. Just small nitpicks. See below.
> This patch adds gpiolib support for the IXP4xx platform
>
> Signed-off-by: Imre Kaloz <ka...@openwrt.org>
> ---
> arch/arm/Kconfig | 2 +-
> arch/arm/mach-ixp4xx/common.c | 39 +++++++++++++++++++++++++
> arch/arm/mach-ixp4xx/include/mach/gpio.h | 46
> ++++++++++--------------------
> 3 files changed, 55 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 3269576..e793a75 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -481,7 +481,7 @@ config ARCH_IXP4XX
> depends on MMU
> select CLKSRC_MMIO
> select CPU_XSCALE
> - select GENERIC_GPIO
> + select ARCH_REQUIRE_GPIOLIB
> select GENERIC_CLOCKEVENTS
> select HAVE_SCHED_CLOCK
> select MIGHT_HAVE_PCI
> diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
> index 0777257..7603456 100644
> --- a/arch/arm/mach-ixp4xx/common.c
> +++ b/arch/arm/mach-ixp4xx/common.c
> @@ -36,6 +36,7 @@
> #include <asm/page.h>
> #include <asm/irq.h>
> #include <asm/sched_clock.h>
> +#include <asm/gpio.h>
>
> #include <asm/mach/map.h>
> #include <asm/mach/irq.h>
> @@ -375,12 +376,50 @@ static struct platform_device *ixp46x_devices[]
> __initdata = {
> unsigned long ixp4xx_exp_bus_size;
> EXPORT_SYMBOL(ixp4xx_exp_bus_size);
>
> +static int ixp4xx_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> +{
> + gpio_line_config(gpio, IXP4XX_GPIO_IN);
> + return 0;
> +}
> +
> +static int ixp4xx_gpio_direction_output(struct gpio_chip *chip, unsigned
> gpio, int level)
> +{
> + gpio_line_set(gpio, level);
> + gpio_line_config(gpio, IXP4XX_GPIO_OUT);
> + return 0;
> +}
> +
> +static int ixp4xx_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
> +{
> + int value;
> +
> + gpio_line_get(gpio, &value);
> + return value;
> +}
> +
> +static void ixp4xx_gpio_set_value(struct gpio_chip *chip, unsigned gpio, int
> value)
> +{
> + gpio_line_set(gpio, value);
> +}
> +
> +static struct gpio_chip ixp4xx_gpio_chip = {
> + .label = "IXP4XX_GPIO_CHIP",
> + .direction_input = ixp4xx_gpio_direction_input,
> + .direction_output = ixp4xx_gpio_direction_output,
> + .get = ixp4xx_gpio_get_value,
> + .set = ixp4xx_gpio_set_value,
> + .base = 0,
> + .ngpio = 16,
Use NR_BUILTIN_GPIO instead of 16 ?
> +};
> +
> void __init ixp4xx_sys_init(void)
> {
> ixp4xx_exp_bus_size = SZ_16M;
>
> platform_add_devices(ixp4xx_devices, ARRAY_SIZE(ixp4xx_devices));
>
> + gpiochip_add(&ixp4xx_gpio_chip);
> +
> if (cpu_is_ixp46x()) {
> int region;
>
> diff --git a/arch/arm/mach-ixp4xx/include/mach/gpio.h
> b/arch/arm/mach-ixp4xx/include/mach/gpio.h
> index a5f87de..86f3596 100644
> --- a/arch/arm/mach-ixp4xx/include/mach/gpio.h
> +++ b/arch/arm/mach-ixp4xx/include/mach/gpio.h
> @@ -27,47 +27,31 @@
>
> #include <linux/kernel.h>
> #include <mach/hardware.h>
> +#include <asm-generic/gpio.h> /* cansleep wrappers */
>
> -static inline int gpio_request(unsigned gpio, const char *label)
> -{
> - return 0;
> -}
> -
> -static inline void gpio_free(unsigned gpio)
> -{
> - might_sleep();
> -
> - return;
> -}
> -
> -static inline int gpio_direction_input(unsigned gpio)
> -{
> - gpio_line_config(gpio, IXP4XX_GPIO_IN);
> - return 0;
> -}
> -
> -static inline int gpio_direction_output(unsigned gpio, int level)
> -{
> - gpio_line_set(gpio, level);
> - gpio_line_config(gpio, IXP4XX_GPIO_OUT);
> - return 0;
> -}
> +#define NR_BUILTIN_GPIO 16
>
> static inline int gpio_get_value(unsigned gpio)
> {
> - int value;
> -
> - gpio_line_get(gpio, &value);
> -
> - return value;
> + if (gpio < NR_BUILTIN_GPIO)
> + {
> + int value;
> + gpio_line_get(gpio, &value);
> + return value;
> + }
> + else
> + return __gpio_get_value(gpio);
Please use
if () {
} else
I would also put the 'int value' declaration outside the if ().
Thanks,
Arnaud
--
To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/871uvu6u89....@lebrac.rtp-net.org
