Hi, Alexandre,
I'm afraid that ARCH_HAVE_CUSTOM_GPIO_H cannot be simply removed, because ARCH_HAVE_CUSTOM_GPIO_H is slected by CONFIG_MIPS, other MIPS CPU (not Loongson) may need it.
Huacai
------------------ Original ------------------
From: "Alexandre Courbot"<gnurou@xxxxxxxxx>;
Date: Fri, Nov 28, 2014 03:48 PM
To: "Huacai Chen"<chenhc@xxxxxxxxxx>;
Cc: "Ralf Baechle"<ralf@xxxxxxxxxxxxxx>; "John Crispin"<john@xxxxxxxxxxx>; "Steven J. Hill"<Steven.Hill@xxxxxxxxxx>; "linux-mips"<linux-mips@xxxxxxxxxxxxxx>; "Fuxin Zhang"<zhangfx@xxxxxxxxxx>; "wuzhangjin"<wuzhangjin@xxxxxxxxx>; "linux-gpio@xxxxxxxxxxxxxxx"<linux-gpio@xxxxxxxxxxxxxxx>;
Subject: Re: [PATCH V5 2/7] MIPS: Cleanup Loongson-2F's gpio driver
On Fri, Nov 21, 2014 at 6:16 PM, Huacai Chen <chenhc@xxxxxxxxxx> wrote:
> This cleanup is prepare to move the driver to drivers/gpio. Custom
> definitions of gpio_get_value()/gpio_set_value() are dropped.
>
> Signed-off-by: Huacai Chen <chenhc@xxxxxxxxxx>
> ---
> arch/mips/include/asm/mach-loongson/gpio.h | 15 +++---
> arch/mips/loongson/common/gpio.c | 82 +++++++++++-----------------
> 2 files changed, 39 insertions(+), 58 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-loongson/gpio.h b/arch/mips/include/asm/mach-loongson/gpio.h
> index 211a7b7..b3b2169 100644
> --- a/arch/mips/include/asm/mach-loongson/gpio.h
> +++ b/arch/mips/include/asm/mach-loongson/gpio.h
> @@ -1,8 +1,9 @@
> /*
> - * STLS2F GPIO Support
> + * Loongson GPIO Support
> *
> * Copyright (c) 2008 Richard Liu, STMicroelectronics <richard.liu@xxxxxx>
> * Copyright (c) 2008-2010 Arnaud Patard <apatard@xxxxxxxxxxxx>
> + * Copyright (c) 2014 Huacai Chen <chenhc@xxxxxxxxxx>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -10,14 +11,14 @@
> * (at your option) any later version.
> */
>
> -#ifndef __STLS2F_GPIO_H
> -#define __STLS2F_GPIO_H
> +#ifndef __LOONGSON_GPIO_H
> +#define __LOONGSON_GPIO_H
>
> #include <asm-generic/gpio.h>
>
> -extern void gpio_set_value(unsigned gpio, int value);
> -extern int gpio_get_value(unsigned gpio);
> -extern int gpio_cansleep(unsigned gpio);
> +#define gpio_get_value __gpio_get_value
> +#define gpio_set_value __gpio_set_value
> +#define gpio_cansleep __gpio_cansleep
Since this architecture is using the standard GPIO functions, can't we
simply get rid of this file and the ARCH_HAVE_CUSTOM_GPIO_H option so
the already-defined inline functions in include/linux/gpio.h are used
instead?
>
> /* The chip can do interrupt
> * but it has not been tested and doc not clear
> @@ -32,4 +33,4 @@ static inline int irq_to_gpio(int gpio)
> return -EINVAL;
> }
>
> -#endif /* __STLS2F_GPIO_H */
> +#endif /* __LOONGSON_GPIO_H */
> diff --git a/arch/mips/loongson/common/gpio.c b/arch/mips/loongson/common/gpio.c
> index 29dbaa2..087aac3 100644
> --- a/arch/mips/loongson/common/gpio.c
> +++ b/arch/mips/loongson/common/gpio.c
> @@ -24,55 +24,6 @@
>
> static DEFINE_SPINLOCK(gpio_lock);
>
> -int gpio_get_value(unsigned gpio)
> -{
> - u32 val;
> - u32 mask;
> -
> - if (gpio >= STLS2F_N_GPIO)
> - return __gpio_get_value(gpio);
> -
> - mask = 1 << (gpio + STLS2F_GPIO_IN_OFFSET);
> - spin_lock(&gpio_lock);
> - val = LOONGSON_GPIODATA;
> - spin_unlock(&gpio_lock);
> -
> - return (val & mask) != 0;
> -}
> -EXPORT_SYMBOL(gpio_get_value);
> -
> -void gpio_set_value(unsigned gpio, int state)
> -{
> - u32 val;
> - u32 mask;
> -
> - if (gpio >= STLS2F_N_GPIO) {
> - __gpio_set_value(gpio, state);
> - return ;
> - }
> -
> - mask = 1 << gpio;
> -
> - spin_lock(&gpio_lock);
> - val = LOONGSON_GPIODATA;
> - if (state)
> - val |= mask;
> - else
> - val &= (~mask);
> - LOONGSON_GPIODATA = val;
> - spin_unlock(&gpio_lock);
> -}
> -EXPORT_SYMBOL(gpio_set_value);
> -
> -int gpio_cansleep(unsigned gpio)
> -{
> - if (gpio < STLS2F_N_GPIO)
> - return 0;
> - else
> - return __gpio_cansleep(gpio);
> -}
> -EXPORT_SYMBOL(gpio_cansleep);
> -
> static int ls2f_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> {
> u32 temp;
> @@ -113,13 +64,41 @@ static int ls2f_gpio_direction_output(struct gpio_chip *chip,
>
> static int ls2f_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
> {
> - return gpio_get_value(gpio);
> + u32 val;
> + u32 mask;
> +
> + if (gpio >= STLS2F_N_GPIO)
> + return __gpio_get_value(gpio);
This condition should not be needed. gpiolib will always use the right chip now.
> +
> + mask = 1 << (gpio + STLS2F_GPIO_IN_OFFSET);
> + spin_lock(&gpio_lock);
> + val = LOONGSON_GPIODATA;
> + spin_unlock(&gpio_lock);
> +
> + return (val & mask) != 0;
> }
>
> static void ls2f_gpio_set_value(struct gpio_chip *chip,
> unsigned gpio, int value)
> {
> - gpio_set_value(gpio, value);
> + u32 val;
> + u32 mask;
> +
> + if (gpio >= STLS2F_N_GPIO) {
> + __gpio_set_value(gpio, value);
> + return;
> + }
Same here, this should not be needed at all.
Also, have you tested that this driver still works after being moved
to drivers/gpio? You are including a <loongson.h> that I am not sure
the compiler will find from there. I also suspect the arch_initcall is
not ideal anymore if you compile the driver as a module - you may want
to remove that option if GPIOs are used early in system boot.
