Re: [PATCH 3/3] GPIO: Add Loongson-3A/3B GPIO driver support

[陈华才 <chenhc@lemote.com>]

Hi, Alexandre

Yes, the GPIO is needed very early, I will send new paches to replace 'tristate' with 'bool'. Thanks.
 
Huacai
 
------------------ Original ------------------
From:  "Alexandre Courbot"<gnurou@gmail.com>;
Date:  Tue, Mar 31, 2015 10:45 PM
To:  "Huacai Chen"<chenhc@lemote.com>; 
Cc:  "Linus Walleij"<linus.walleij@linaro.org>; "linux-gpio@vger.kernel.org"<linux-gpio@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; 
Subject:  Re: [PATCH 3/3] GPIO: Add Loongson-3A/3B GPIO driver support

 
On Thu, Mar 26, 2015 at 11:34 AM, Huacai Chen <chenhc@lemote.com> wrote:
> Improve Loongson-2's GPIO driver to support Loongson-3A/3B, and update
> Loongson-3's default config file.
>
> Acked-by: Ralf Baechle <ralf@linux-mips.org>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  arch/mips/Kconfig                     |    1 +
>  arch/mips/configs/loongson3_defconfig |    1 +
>  drivers/gpio/Kconfig                  |    6 ++--
>  drivers/gpio/gpio-loongson.c          |   44 ++++++++++++++++++++------------
>  4 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 915e689..b2a5827 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -1245,6 +1245,7 @@ config CPU_LOONGSON3
>         select CPU_SUPPORTS_HUGEPAGES
>         select WEAK_ORDERING
>         select WEAK_REORDERING_BEYOND_LLSC
> +       select ARCH_REQUIRE_GPIOLIB
>         help
>                 The Loongson 3 processor implements the MIPS64R2 instruction
>                 set with many extensions.
> diff --git a/arch/mips/configs/loongson3_defconfig b/arch/mips/configs/loongson3_defconfig
> index 7eabcd2..c844299 100644
> --- a/arch/mips/configs/loongson3_defconfig
> +++ b/arch/mips/configs/loongson3_defconfig
> @@ -243,6 +243,7 @@ CONFIG_HW_RANDOM=y
>  CONFIG_RAW_DRIVER=m
>  CONFIG_I2C_CHARDEV=y
>  CONFIG_I2C_PIIX4=y
> +CONFIG_GPIO_LOONGSON=y
>  CONFIG_SENSORS_LM75=m
>  CONFIG_SENSORS_LM93=m
>  CONFIG_SENSORS_W83627HF=m
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 6454160..7e8227b 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -500,10 +500,10 @@ config GPIO_GRGPIO
>           VHDL IP core library.
>
>  config GPIO_LOONGSON
> -       tristate "Loongson-2 GPIO support"
> -       depends on CPU_LOONGSON2
> +       tristate "Loongson-2/3 GPIO support"
> +       depends on CPU_LOONGSON2 || CPU_LOONGSON3
>         help
> -         driver for GPIO functionality on Loongson-2F processors.
> +         driver for GPIO functionality on Loongson-2F/3A/3B processors.
>
>  config GPIO_TB10X
>         bool
> diff --git a/drivers/gpio/gpio-loongson.c b/drivers/gpio/gpio-loongson.c
> index b4e69e0..ccc65a1 100644
> --- a/drivers/gpio/gpio-loongson.c
> +++ b/drivers/gpio/gpio-loongson.c
> @@ -1,8 +1,10 @@
>  /*
> - *  STLS2F GPIO Support
> + *  Loongson-2F/3A/3B GPIO Support
>   *
>   *  Copyright (c) 2008 Richard Liu,  STMicroelectronics         <richard.liu@st.com>
>   *  Copyright (c) 2008-2010 Arnaud Patard <apatard@mandriva.com>
> + *  Copyright (c) 2013 Hongbing Hu <huhb@lemote.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
> @@ -20,11 +22,19 @@
>  #include <linux/gpio.h>
>
>  #define STLS2F_N_GPIO          4
> -#define STLS2F_GPIO_IN_OFFSET  16
> +#define STLS3A_N_GPIO          16
> +
> +#ifdef CONFIG_CPU_LOONGSON3
> +#define LOONGSON_N_GPIO        STLS3A_N_GPIO
> +#else
> +#define LOONGSON_N_GPIO        STLS2F_N_GPIO
> +#endif
> +
> +#define LOONGSON_GPIO_IN_OFFSET        16
>
>  static DEFINE_SPINLOCK(gpio_lock);
>
> -static int ls2f_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> +static int loongson_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
>  {
>         u32 temp;
>         u32 mask;
> @@ -39,7 +49,7 @@ static int ls2f_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
>         return 0;
>  }
>
> -static int ls2f_gpio_direction_output(struct gpio_chip *chip,
> +static int loongson_gpio_direction_output(struct gpio_chip *chip,
>                 unsigned gpio, int level)
>  {
>         u32 temp;
> @@ -56,12 +66,12 @@ static int ls2f_gpio_direction_output(struct gpio_chip *chip,
>         return 0;
>  }
>
> -static int ls2f_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
> +static int loongson_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
>  {
>         u32 val;
>         u32 mask;
>
> -       mask = 1 << (gpio + STLS2F_GPIO_IN_OFFSET);
> +       mask = 1 << (gpio + LOONGSON_GPIO_IN_OFFSET);
>         spin_lock(&gpio_lock);
>         val = LOONGSON_GPIODATA;
>         spin_unlock(&gpio_lock);
> @@ -69,7 +79,7 @@ static int ls2f_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
>         return (val & mask) != 0;
>  }
>
> -static void ls2f_gpio_set_value(struct gpio_chip *chip,
> +static void loongson_gpio_set_value(struct gpio_chip *chip,
>                 unsigned gpio, int value)
>  {
>         u32 val;
> @@ -87,19 +97,19 @@ static void ls2f_gpio_set_value(struct gpio_chip *chip,
>         spin_unlock(&gpio_lock);
>  }
>
> -static struct gpio_chip ls2f_chip = {
> -       .label                  = "ls2f",
> -       .direction_input        = ls2f_gpio_direction_input,
> -       .get                    = ls2f_gpio_get_value,
> -       .direction_output       = ls2f_gpio_direction_output,
> -       .set                    = ls2f_gpio_set_value,
> +static struct gpio_chip loongson_chip = {
> +       .label                  = "Loongson-gpio-chip",
> +       .direction_input        = loongson_gpio_direction_input,
> +       .get                    = loongson_gpio_get_value,
> +       .direction_output       = loongson_gpio_direction_output,
> +       .set                    = loongson_gpio_set_value,
>         .base                   = 0,
> -       .ngpio                  = STLS2F_N_GPIO,
> +       .ngpio                  = LOONGSON_N_GPIO,
>         .can_sleep              = false,
>  };
>
> -static int __init ls2f_gpio_setup(void)
> +static int __init loongson_gpio_setup(void)
>  {
> -       return gpiochip_add(&ls2f_chip);
> +       return gpiochip_add(&loongson_chip);
>  }
> -arch_initcall(ls2f_gpio_setup);
> +postcore_initcall(loongson_gpio_setup);

This driver does not have a probe function and its device is added by
this initcall. I am ok with this but in that case, isn't it illegal to
allow the driver to be compiled as a module, like your patch 2/3 does?

As a future work, it would be nice to convert this to a platform
driver, unless the GPIOs are needed too early in the system's
lifecycle.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>