[v1,1/5] gpio: moxart: convert to use basic mmio gpio library

Message ID 1417435778-21879-2-git-send-email-kamlakant.patel@linaro.org
State Accepted
Commit 3c01b9a896c9c592d9edc7439d5e5cf6c411d014
Headers show

Commit Message

kamlakant.patel@linaro.org Dec. 1, 2014, 12:09 p.m.
From: Kamlakant Patel <kamlakant.patel@linaro.org>

This patch converts MOXART GPIO driver to use basic_mmio_gpio
generic library.

Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
Tested-by: Jonas Jensen <jonas.jensen@gmail.com>
---
 drivers/gpio/Kconfig       |   1 +
 drivers/gpio/gpio-moxart.c | 101 ++++++++++++++-------------------------------
 2 files changed, 32 insertions(+), 70 deletions(-)

Comments

kamlakant.patel@linaro.org Dec. 16, 2014, 5:17 a.m. | #1
Hi Alexandre,

On 4 December 2014 at 14:47, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Mon, Dec 1, 2014 at 9:09 PM,  <kamlakant.patel@linaro.org> wrote:
>> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>>
>> This patch converts MOXART GPIO driver to use basic_mmio_gpio
>> generic library.
>>
>> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
>> Tested-by: Jonas Jensen <jonas.jensen@gmail.com>
>> ---
>>  drivers/gpio/Kconfig       |   1 +
>>  drivers/gpio/gpio-moxart.c | 101 ++++++++++++++-------------------------------
>>  2 files changed, 32 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 0959ca9..3bd4d63 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -184,6 +184,7 @@ config GPIO_F7188X
>>  config GPIO_MOXART
>>         bool "MOXART GPIO support"
>>         depends on ARCH_MOXART
>> +       select GPIO_GENERIC
>>         help
>>           Select this option to enable GPIO driver for
>>           MOXA ART SoC devices.
>> diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
>> index 4661e18..122958f 100644
>> --- a/drivers/gpio/gpio-moxart.c
>> +++ b/drivers/gpio/gpio-moxart.c
>> @@ -23,21 +23,12 @@
>>  #include <linux/delay.h>
>>  #include <linux/timer.h>
>>  #include <linux/bitops.h>
>> +#include <linux/basic_mmio_gpio.h>
>>
>>  #define GPIO_DATA_OUT          0x00
>>  #define GPIO_DATA_IN           0x04
>>  #define GPIO_PIN_DIRECTION     0x08
>>
>> -struct moxart_gpio_chip {
>> -       struct gpio_chip gpio;
>> -       void __iomem *base;
>> -};
>> -
>> -static inline struct moxart_gpio_chip *to_moxart_gpio(struct gpio_chip *chip)
>> -{
>> -       return container_of(chip, struct moxart_gpio_chip, gpio);
>> -}
>> -
>>  static int moxart_gpio_request(struct gpio_chip *chip, unsigned offset)
>>  {
>>         return pinctrl_request_gpio(offset);
>> @@ -48,90 +39,60 @@ static void moxart_gpio_free(struct gpio_chip *chip, unsigned offset)
>>         pinctrl_free_gpio(offset);
>>  }
>>
>> -static void moxart_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> -{
>> -       struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
>> -       void __iomem *ioaddr = gc->base + GPIO_DATA_OUT;
>> -       u32 reg = readl(ioaddr);
>> -
>> -       if (value)
>> -               reg = reg | BIT(offset);
>> -       else
>> -               reg = reg & ~BIT(offset);
>> -
>> -       writel(reg, ioaddr);
>> -}
>> -
>>  static int moxart_gpio_get(struct gpio_chip *chip, unsigned offset)
>>  {
>> -       struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
>> -       u32 ret = readl(gc->base + GPIO_PIN_DIRECTION);
>> +       struct bgpio_chip *bgc = to_bgpio_chip(chip);
>> +       u32 ret = bgc->read_reg(bgc->reg_dir);
>>
>>         if (ret & BIT(offset))
>> -               return !!(readl(gc->base + GPIO_DATA_OUT) & BIT(offset));
>> +               return !!(bgc->read_reg(bgc->reg_set) & BIT(offset));
>>         else
>> -               return !!(readl(gc->base + GPIO_DATA_IN) & BIT(offset));
>> -}
>> -
>> -static int moxart_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>> -{
>> -       struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
>> -       void __iomem *ioaddr = gc->base + GPIO_PIN_DIRECTION;
>> -
>> -       writel(readl(ioaddr) & ~BIT(offset), ioaddr);
>> -       return 0;
>> -}
>> -
>> -static int moxart_gpio_direction_output(struct gpio_chip *chip,
>> -                                       unsigned offset, int value)
>> -{
>> -       struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
>> -       void __iomem *ioaddr = gc->base + GPIO_PIN_DIRECTION;
>> -
>> -       moxart_gpio_set(chip, offset, value);
>> -       writel(readl(ioaddr) | BIT(offset), ioaddr);
>> -       return 0;
>> +               return !!(bgc->read_reg(bgc->reg_dat) & BIT(offset));
>>  }
>>
>> -static struct gpio_chip moxart_template_chip = {
>> -       .label                  = "moxart-gpio",
>> -       .request                = moxart_gpio_request,
>> -       .free                   = moxart_gpio_free,
>> -       .direction_input        = moxart_gpio_direction_input,
>> -       .direction_output       = moxart_gpio_direction_output,
>> -       .set                    = moxart_gpio_set,
>> -       .get                    = moxart_gpio_get,
>> -       .ngpio                  = 32,
>> -       .owner                  = THIS_MODULE,
>> -};
>> -
>>  static int moxart_gpio_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>>         struct resource *res;
>> -       struct moxart_gpio_chip *mgc;
>> +       struct bgpio_chip *bgc;
>> +       void __iomem *base;
>>         int ret;
>>
>> -       mgc = devm_kzalloc(dev, sizeof(*mgc), GFP_KERNEL);
>> -       if (!mgc)
>> +       bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
>> +       if (!bgc)
>>                 return -ENOMEM;
>> -       mgc->gpio = moxart_template_chip;
>>
>>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -       mgc->base = devm_ioremap_resource(dev, res);
>> -       if (IS_ERR(mgc->base))
>> -               return PTR_ERR(mgc->base);
>> +       base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>>
>> -       mgc->gpio.dev = dev;
>> +       ret = bgpio_init(bgc, dev, 4, base + GPIO_DATA_IN,
>> +                   base + GPIO_DATA_OUT, NULL,
>> +                   base + GPIO_PIN_DIRECTION, NULL, 0);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "bgpio_init failed\n");
>> +               return ret;
>> +       }
>>
>> -       ret = gpiochip_add(&mgc->gpio);
>> +       bgc->gc.label = "moxart-gpio";
>> +       bgc->gc.request = moxart_gpio_request;
>> +       bgc->gc.free = moxart_gpio_free;
>> +       bgc->gc.get = moxart_gpio_get;
>> +       bgc->data = bgc->read_reg(bgc->reg_set);
>> +       bgc->gc.base = 0;
>
> Do we actually want all instances of this driver to clain GPIOs 0..31?
>

Here is what I got from Jonas in previous discussion:
...
Thanks, it works, tested on UC-7112-LX hardware.

I have one additional nit..

The GPIO base number is implicitly changed from 0 to 224
(ARCH_NR_GPIOS (256) - ngpio (32)) which happen because of
bgpio_init() (it sets base -1 / gpiochip_find_base() on
gpiochip_add()). Which is confusing since the valid range (from user
space) used to be 0-31. So on export we now get:

[root@zurkon ~]# echo 24 > /sys/class/gpio/export
    [   61.640000] gpio-24 (?): gpiod_request: status -517
    [   61.650000] export_store: status -19

I see other drivers explicitly set base after bgpio_init(), my
suggestion is that we do the same here e.g. :

> +       bgc->gc.label = "moxart-gpio";
> +       bgc->gc.request = moxart_gpio_request;
> +       bgc->gc.free = moxart_gpio_free;
> +       bgc->gc.get = moxart_gpio_get;
> +       bgc->data = bgc->read_reg(bgc->reg_set);
> +       bgc->gc.ngpio = 32;
> +       bgc->gc.dev = dev;
> +       bgc->gc.owner = THIS_MODULE;

bgc->gc.base = 0;

Tested-by: Jonas Jensen <jonas.jensen@gmail.com>
...
> If so,
>
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Since this patch greatly simplifies the code and has been properly tested.

Thanks,
Kamlakant Patel
--
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
Linus Walleij Jan. 9, 2015, 9:25 a.m. | #2
On Mon, Dec 1, 2014 at 1:09 PM,  <kamlakant.patel@linaro.org> wrote:

> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>
> This patch converts MOXART GPIO driver to use basic_mmio_gpio
> generic library.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> Tested-by: Jonas Jensen <jonas.jensen@gmail.com>

Patch applied for v3.20. Sorry for eternal delays :(

Yours,
Linus Walleij
--
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

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0959ca9..3bd4d63 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -184,6 +184,7 @@  config GPIO_F7188X
 config GPIO_MOXART
 	bool "MOXART GPIO support"
 	depends on ARCH_MOXART
+	select GPIO_GENERIC
 	help
 	  Select this option to enable GPIO driver for
 	  MOXA ART SoC devices.
diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
index 4661e18..122958f 100644
--- a/drivers/gpio/gpio-moxart.c
+++ b/drivers/gpio/gpio-moxart.c
@@ -23,21 +23,12 @@ 
 #include <linux/delay.h>
 #include <linux/timer.h>
 #include <linux/bitops.h>
+#include <linux/basic_mmio_gpio.h>
 
 #define GPIO_DATA_OUT		0x00
 #define GPIO_DATA_IN		0x04
 #define GPIO_PIN_DIRECTION	0x08
 
-struct moxart_gpio_chip {
-	struct gpio_chip gpio;
-	void __iomem *base;
-};
-
-static inline struct moxart_gpio_chip *to_moxart_gpio(struct gpio_chip *chip)
-{
-	return container_of(chip, struct moxart_gpio_chip, gpio);
-}
-
 static int moxart_gpio_request(struct gpio_chip *chip, unsigned offset)
 {
 	return pinctrl_request_gpio(offset);
@@ -48,90 +39,60 @@  static void moxart_gpio_free(struct gpio_chip *chip, unsigned offset)
 	pinctrl_free_gpio(offset);
 }
 
-static void moxart_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
-{
-	struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
-	void __iomem *ioaddr = gc->base + GPIO_DATA_OUT;
-	u32 reg = readl(ioaddr);
-
-	if (value)
-		reg = reg | BIT(offset);
-	else
-		reg = reg & ~BIT(offset);
-
-	writel(reg, ioaddr);
-}
-
 static int moxart_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
-	u32 ret = readl(gc->base + GPIO_PIN_DIRECTION);
+	struct bgpio_chip *bgc = to_bgpio_chip(chip);
+	u32 ret = bgc->read_reg(bgc->reg_dir);
 
 	if (ret & BIT(offset))
-		return !!(readl(gc->base + GPIO_DATA_OUT) & BIT(offset));
+		return !!(bgc->read_reg(bgc->reg_set) & BIT(offset));
 	else
-		return !!(readl(gc->base + GPIO_DATA_IN) & BIT(offset));
-}
-
-static int moxart_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
-{
-	struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
-	void __iomem *ioaddr = gc->base + GPIO_PIN_DIRECTION;
-
-	writel(readl(ioaddr) & ~BIT(offset), ioaddr);
-	return 0;
-}
-
-static int moxart_gpio_direction_output(struct gpio_chip *chip,
-					unsigned offset, int value)
-{
-	struct moxart_gpio_chip *gc = to_moxart_gpio(chip);
-	void __iomem *ioaddr = gc->base + GPIO_PIN_DIRECTION;
-
-	moxart_gpio_set(chip, offset, value);
-	writel(readl(ioaddr) | BIT(offset), ioaddr);
-	return 0;
+		return !!(bgc->read_reg(bgc->reg_dat) & BIT(offset));
 }
 
-static struct gpio_chip moxart_template_chip = {
-	.label			= "moxart-gpio",
-	.request		= moxart_gpio_request,
-	.free			= moxart_gpio_free,
-	.direction_input	= moxart_gpio_direction_input,
-	.direction_output	= moxart_gpio_direction_output,
-	.set			= moxart_gpio_set,
-	.get			= moxart_gpio_get,
-	.ngpio			= 32,
-	.owner			= THIS_MODULE,
-};
-
 static int moxart_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct resource *res;
-	struct moxart_gpio_chip *mgc;
+	struct bgpio_chip *bgc;
+	void __iomem *base;
 	int ret;
 
-	mgc = devm_kzalloc(dev, sizeof(*mgc), GFP_KERNEL);
-	if (!mgc)
+	bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
+	if (!bgc)
 		return -ENOMEM;
-	mgc->gpio = moxart_template_chip;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mgc->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(mgc->base))
-		return PTR_ERR(mgc->base);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
 
-	mgc->gpio.dev = dev;
+	ret = bgpio_init(bgc, dev, 4, base + GPIO_DATA_IN,
+		    base + GPIO_DATA_OUT, NULL,
+		    base + GPIO_PIN_DIRECTION, NULL, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init failed\n");
+		return ret;
+	}
 
-	ret = gpiochip_add(&mgc->gpio);
+	bgc->gc.label = "moxart-gpio";
+	bgc->gc.request = moxart_gpio_request;
+	bgc->gc.free = moxart_gpio_free;
+	bgc->gc.get = moxart_gpio_get;
+	bgc->data = bgc->read_reg(bgc->reg_set);
+	bgc->gc.base = 0;
+	bgc->gc.ngpio = 32;
+	bgc->gc.dev = dev;
+	bgc->gc.owner = THIS_MODULE;
+
+	ret = gpiochip_add(&bgc->gc);
 	if (ret) {
 		dev_err(dev, "%s: gpiochip_add failed\n",
 			dev->of_node->full_name);
 		return ret;
 	}
 
-	return 0;
+	return ret;
 }
 
 static const struct of_device_id moxart_gpio_match[] = {