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@xxxxxxxxx> wrote:
> On Sun, Dec 28, 2014 at 1:57 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
>>
>> /* 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.
>
