| Message ID | 1419742654-15094-1-git-send-email-chenhc@lemote.com |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 28, 2014 at 1:57 PM, Huacai Chen <chenhc@lemote.com> 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@lemote.com> > --- > 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@st.com> > * Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com> > + * Copyright (c) 2014 Huacai Chen <chenhc@lemote.com> > * > * 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 > > /* 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); > + > + mask = 1 << (gpio + STLS2F_GPIO_IN_OFFSET); > + spin_lock(&gpio_lock); > + val = LOONGSON_GPIODATA; > + spin_unlock(&gpio_lock); Careful, you are not anymore dealing with absolute GPIO numbers like your former custom gpio_get_value() function did. This function will be called by the gpiolib core after it has matched the GPIO to your chip. So testing for gpio >= STLS2F_N_GPIO is not needed. Furthermore, the passed GPIO number will be relative to the chip's base index. In your case it seems like the base is 0, so this doesn't change anything, but be aware of this fact. > + > + 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; > + } > + > + mask = 1 << gpio; > + > + spin_lock(&gpio_lock); > + val = LOONGSON_GPIODATA; > + if (value) > + val |= mask; > + else > + val &= (~mask); > + LOONGSON_GPIODATA = val; Same thing here. Since this is a potentially dangerous refactoring of this driver, I'd like a statement that confirms it is still working properly after patches 3, 4, and 5 of this series. IOW, please test your driver after each of these patches to ensure the refactoring is done properly. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Alexandre I think all "(gpio >= STLS2F_N_GPIO)" can be removed in this file, not only ls2f_gpio_get_value() and ls2f_gpio_set_value(), am I right? Huacai On Mon, Jan 19, 2015 at 2:17 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Sun, Dec 28, 2014 at 1:57 PM, Huacai Chen <chenhc@lemote.com> 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@lemote.com> >> --- >> 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@st.com> >> * Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com> >> + * Copyright (c) 2014 Huacai Chen <chenhc@lemote.com> >> * >> * 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 >> >> /* 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); >> + >> + mask = 1 << (gpio + STLS2F_GPIO_IN_OFFSET); >> + spin_lock(&gpio_lock); >> + val = LOONGSON_GPIODATA; >> + spin_unlock(&gpio_lock); > > Careful, you are not anymore dealing with absolute GPIO numbers like > your former custom gpio_get_value() function did. > > This function will be called by the gpiolib core after it has matched > the GPIO to your chip. So testing for gpio >= STLS2F_N_GPIO is not > needed. > > Furthermore, the passed GPIO number will be relative to the chip's > base index. In your case it seems like the base is 0, so this doesn't > change anything, but be aware of this fact. > >> + >> + 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; >> + } >> + >> + mask = 1 << gpio; >> + >> + spin_lock(&gpio_lock); >> + val = LOONGSON_GPIODATA; >> + if (value) >> + val |= mask; >> + else >> + val &= (~mask); >> + LOONGSON_GPIODATA = val; > > Same thing here. > > Since this is a potentially dangerous refactoring of this driver, I'd > like a statement that confirms it is still working properly after > patches 3, 4, and 5 of this series. IOW, please test your driver after > each of these patches to ensure the refactoring is done properly. > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@st.com> * Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com> + * Copyright (c) 2014 Huacai Chen <chenhc@lemote.com> * * 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 /* 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); + + 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; + } + + mask = 1 << gpio; + + spin_lock(&gpio_lock); + val = LOONGSON_GPIODATA; + if (value) + val |= mask; + else + val &= (~mask); + LOONGSON_GPIODATA = val; + spin_unlock(&gpio_lock); } static struct gpio_chip ls2f_chip = { @@ -130,6 +109,7 @@ static struct gpio_chip ls2f_chip = { .set = ls2f_gpio_set_value, .base = 0, .ngpio = STLS2F_N_GPIO, + .can_sleep = false, }; static int __init ls2f_gpio_setup(void)
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@lemote.com> --- arch/mips/include/asm/mach-loongson/gpio.h | 15 +++--- arch/mips/loongson/common/gpio.c | 82 +++++++++++----------------- 2 files changed, 39 insertions(+), 58 deletions(-)