[PATCH v1 0/5] convert to use basic mmio gpio library

[PATCH v1 0/5] convert to use basic mmio gpio library

[kamlakant.patel]

From: Kamlakant Patel <kamlakant.patel@linaro.org>

This patch converts GPIO drivers to use BASIC MMIO GPIO library
(i.e GPIO GENERIC library) which makes drivers smaller and simpler.

V1:
Following are the changes compare to previous patch 
* gpio-moxart.c: Removed unnecessary private structure moxart_gpio_chip and use
  bgpio_chip.
* gpio-timberdale.c: Register correct gpio_chip and update code to use
  bgpio_chip elements.
* gpio-iop.c: Pass correct parameters to bgpio_init.

Kamlakant Patel (5):
  gpio: moxart: convert to use basic mmio gpio library
  gpio: timberdale: convert to use basic mmio gpio library
  gpio: iop: convert to use basic mmio gpio library
  gpio: ge: convert to use basic mmio gpio library
  gpio: document basic mmio gpio library

 Documentation/gpio/driver.txt  |  50 ++++++++++++++++++++
 drivers/gpio/Kconfig           |   4 ++
 drivers/gpio/gpio-ge.c         |  96 ++++++++++++++++-----------------------
 drivers/gpio/gpio-iop.c        |  96 +++++++--------------------------------
 drivers/gpio/gpio-moxart.c     | 101 +++++++++++++----------------------------
 drivers/gpio/gpio-timberdale.c |  96 +++++++++++----------------------------
 6 files changed, 168 insertions(+), 275 deletions(-)

-- 
1.9.1

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

[kamlakant.patel]

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;
+	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[] = {
-- 
1.9.1

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

[Alexandre Courbot]

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?

If so,

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

Since this patch greatly simplifies the code and has been properly tested.

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

[Kamlakant Patel]

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

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

[Alexandre Courbot]

On Tue, Dec 16, 2014 at 2:17 PM, Kamlakant Patel
<kamlakant.patel@linaro.org> wrote:
> 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;

My problem is that drivers should not decide what their base GPIO
number is. If all drivers claimed 0, you just could not use two
different drivers on the same system.

But I assume that the driver was already doing that previously, so I
reluctantly maintain my ack. The GPIO base number is a different issue
than what this patch series addresses. With the Tested-by from Jonas I
guess we can at least apply this patch, if Linus agrees.

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

[Linus Walleij]

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

[PATCH v1 2/5] gpio: timberdale: convert to use basic mmio gpio library

[kamlakant.patel]

From: Kamlakant Patel <kamlakant.patel@linaro.org>

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

Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
---
 drivers/gpio/Kconfig           |  1 +
 drivers/gpio/gpio-timberdale.c | 96 ++++++++++++------------------------------
 2 files changed, 28 insertions(+), 69 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3bd4d63..875d312 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -783,6 +783,7 @@ config GPIO_SODAVILLE
 config GPIO_TIMBERDALE
 	bool "Support for timberdale GPIO IP"
 	depends on MFD_TIMBERDALE
+	select GPIO_GENERIC
 	---help---
 	Add support for the GPIO IP in the timberdale FPGA.
 
diff --git a/drivers/gpio/gpio-timberdale.c b/drivers/gpio/gpio-timberdale.c
index a685a3c..8849851 100644
--- a/drivers/gpio/gpio-timberdale.c
+++ b/drivers/gpio/gpio-timberdale.c
@@ -28,6 +28,7 @@
 #include <linux/timb_gpio.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/basic_mmio_gpio.h>
 
 #define DRIVER_NAME "timb-gpio"
 
@@ -45,60 +46,15 @@
 struct timbgpio {
 	void __iomem		*membase;
 	spinlock_t		lock; /* mutual exclusion */
-	struct gpio_chip	gpio;
+	struct bgpio_chip	bgc;
 	int			irq_base;
 	unsigned long		last_ier;
 };
 
-static int timbgpio_update_bit(struct gpio_chip *gpio, unsigned index,
-	unsigned offset, bool enabled)
-{
-	struct timbgpio *tgpio = container_of(gpio, struct timbgpio, gpio);
-	u32 reg;
-
-	spin_lock(&tgpio->lock);
-	reg = ioread32(tgpio->membase + offset);
-
-	if (enabled)
-		reg |= (1 << index);
-	else
-		reg &= ~(1 << index);
-
-	iowrite32(reg, tgpio->membase + offset);
-	spin_unlock(&tgpio->lock);
-
-	return 0;
-}
-
-static int timbgpio_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
-{
-	return timbgpio_update_bit(gpio, nr, TGPIODIR, true);
-}
-
-static int timbgpio_gpio_get(struct gpio_chip *gpio, unsigned nr)
-{
-	struct timbgpio *tgpio = container_of(gpio, struct timbgpio, gpio);
-	u32 value;
-
-	value = ioread32(tgpio->membase + TGPIOVAL);
-	return (value & (1 << nr)) ? 1 : 0;
-}
-
-static int timbgpio_gpio_direction_output(struct gpio_chip *gpio,
-						unsigned nr, int val)
-{
-	return timbgpio_update_bit(gpio, nr, TGPIODIR, false);
-}
-
-static void timbgpio_gpio_set(struct gpio_chip *gpio,
-				unsigned nr, int val)
-{
-	timbgpio_update_bit(gpio, nr, TGPIOVAL, val != 0);
-}
-
 static int timbgpio_to_irq(struct gpio_chip *gpio, unsigned offset)
 {
-	struct timbgpio *tgpio = container_of(gpio, struct timbgpio, gpio);
+	struct bgpio_chip *bgc = container_of(gpio, struct bgpio_chip, gc);
+	struct timbgpio *tgpio = container_of(bgc, struct timbgpio, bgc);
 
 	if (tgpio->irq_base <= 0)
 		return -EINVAL;
@@ -142,7 +98,7 @@ static int timbgpio_irq_type(struct irq_data *d, unsigned trigger)
 	u32 ver;
 	int ret = 0;
 
-	if (offset < 0 || offset > tgpio->gpio.ngpio)
+	if (offset < 0 || offset > tgpio->bgc.gc.ngpio)
 		return -EINVAL;
 
 	ver = ioread32(tgpio->membase + TGPIO_VER);
@@ -208,8 +164,8 @@ static void timbgpio_irq(unsigned int irq, struct irq_desc *desc)
 	 */
 	iowrite32(0, tgpio->membase + TGPIO_IER);
 
-	for_each_set_bit(offset, &ipr, tgpio->gpio.ngpio)
-		generic_handle_irq(timbgpio_to_irq(&tgpio->gpio, offset));
+	for_each_set_bit(offset, &ipr, tgpio->bgc.gc.ngpio)
+		generic_handle_irq(timbgpio_to_irq(&tgpio->bgc.gc, offset));
 
 	iowrite32(tgpio->last_ier, tgpio->membase + TGPIO_IER);
 }
@@ -225,7 +181,6 @@ static int timbgpio_probe(struct platform_device *pdev)
 {
 	int err, i;
 	struct device *dev = &pdev->dev;
-	struct gpio_chip *gc;
 	struct timbgpio *tgpio;
 	struct resource *iomem;
 	struct timbgpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -247,6 +202,7 @@ static int timbgpio_probe(struct platform_device *pdev)
 		dev_err(dev, "Memory alloc failed\n");
 		return -EINVAL;
 	}
+
 	tgpio->irq_base = pdata->irq_base;
 
 	spin_lock_init(&tgpio->lock);
@@ -263,22 +219,24 @@ static int timbgpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	gc = &tgpio->gpio;
-
-	gc->label = dev_name(&pdev->dev);
-	gc->owner = THIS_MODULE;
-	gc->dev = &pdev->dev;
-	gc->direction_input = timbgpio_gpio_direction_input;
-	gc->get = timbgpio_gpio_get;
-	gc->direction_output = timbgpio_gpio_direction_output;
-	gc->set = timbgpio_gpio_set;
-	gc->to_irq = (irq >= 0 && tgpio->irq_base > 0) ? timbgpio_to_irq : NULL;
-	gc->dbg_show = NULL;
-	gc->base = pdata->gpio_base;
-	gc->ngpio = pdata->nr_pins;
-	gc->can_sleep = false;
-
-	err = gpiochip_add(gc);
+	err = bgpio_init(&tgpio->bgc, dev, 4, tgpio->membase + TGPIOVAL,
+			 NULL, NULL, NULL, tgpio->membase + TGPIODIR, 0);
+
+	if (err) {
+		dev_err(&pdev->dev, "bgpio_init failed\n");
+		return err;
+	}
+	tgpio->bgc.gc.label = dev_name(&pdev->dev);
+	tgpio->bgc.gc.owner = THIS_MODULE;
+	tgpio->bgc.gc.dev = &pdev->dev;
+	tgpio->bgc.gc.to_irq = (irq >= 0 && tgpio->irq_base > 0) ?
+						timbgpio_to_irq : NULL;
+	tgpio->bgc.gc.dbg_show = NULL;
+	tgpio->bgc.gc.base = pdata->gpio_base;
+	tgpio->bgc.gc.ngpio = pdata->nr_pins;
+	tgpio->bgc.gc.can_sleep = false;
+
+	err = gpiochip_add(&tgpio->bgc.gc);
 	if (err)
 		return err;
 
@@ -322,7 +280,7 @@ static int timbgpio_remove(struct platform_device *pdev)
 		irq_set_handler_data(irq, NULL);
 	}
 
-	gpiochip_remove(&tgpio->gpio);
+	gpiochip_remove(&tgpio->bgc.gc);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v1 2/5] gpio: timberdale: convert to use basic mmio gpio library

[Linus Walleij]

Richard can you look at this patch and even test it maybe?

Do you still work with the Timberdale hardware or do you
know anyone who does?

Yours,
Linus Walleij

On Mon, Dec 1, 2014 at 1:09 PM,  <kamlakant.patel@linaro.org> wrote:
> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>
> This patch converts TIMBERDALE GPIO driver to use basic_mmio_gpio
> generic library.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> ---
>  drivers/gpio/Kconfig           |  1 +
>  drivers/gpio/gpio-timberdale.c | 96 ++++++++++++------------------------------
>  2 files changed, 28 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 3bd4d63..875d312 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -783,6 +783,7 @@ config GPIO_SODAVILLE
>  config GPIO_TIMBERDALE
>         bool "Support for timberdale GPIO IP"
>         depends on MFD_TIMBERDALE
> +       select GPIO_GENERIC
>         ---help---
>         Add support for the GPIO IP in the timberdale FPGA.
>
> diff --git a/drivers/gpio/gpio-timberdale.c b/drivers/gpio/gpio-timberdale.c
> index a685a3c..8849851 100644
> --- a/drivers/gpio/gpio-timberdale.c
> +++ b/drivers/gpio/gpio-timberdale.c
> @@ -28,6 +28,7 @@
>  #include <linux/timb_gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/slab.h>
> +#include <linux/basic_mmio_gpio.h>
>
>  #define DRIVER_NAME "timb-gpio"
>
> @@ -45,60 +46,15 @@
>  struct timbgpio {
>         void __iomem            *membase;
>         spinlock_t              lock; /* mutual exclusion */
> -       struct gpio_chip        gpio;
> +       struct bgpio_chip       bgc;
>         int                     irq_base;
>         unsigned long           last_ier;
>  };
>
> -static int timbgpio_update_bit(struct gpio_chip *gpio, unsigned index,
> -       unsigned offset, bool enabled)
> -{
> -       struct timbgpio *tgpio = container_of(gpio, struct timbgpio, gpio);
> -       u32 reg;
> -
> -       spin_lock(&tgpio->lock);
> -       reg = ioread32(tgpio->membase + offset);
> -
> -       if (enabled)
> -               reg |= (1 << index);
> -       else
> -               reg &= ~(1 << index);
> -
> -       iowrite32(reg, tgpio->membase + offset);
> -       spin_unlock(&tgpio->lock);
> -
> -       return 0;
> -}
> -
> -static int timbgpio_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
> -{
> -       return timbgpio_update_bit(gpio, nr, TGPIODIR, true);
> -}
> -
> -static int timbgpio_gpio_get(struct gpio_chip *gpio, unsigned nr)
> -{
> -       struct timbgpio *tgpio = container_of(gpio, struct timbgpio, gpio);
> -       u32 value;
> -
> -       value = ioread32(tgpio->membase + TGPIOVAL);
> -       return (value & (1 << nr)) ? 1 : 0;
> -}
> -
> -static int timbgpio_gpio_direction_output(struct gpio_chip *gpio,
> -                                               unsigned nr, int val)
> -{
> -       return timbgpio_update_bit(gpio, nr, TGPIODIR, false);
> -}
> -
> -static void timbgpio_gpio_set(struct gpio_chip *gpio,
> -                               unsigned nr, int val)
> -{
> -       timbgpio_update_bit(gpio, nr, TGPIOVAL, val != 0);
> -}
> -
>  static int timbgpio_to_irq(struct gpio_chip *gpio, unsigned offset)
>  {
> -       struct timbgpio *tgpio = container_of(gpio, struct timbgpio, gpio);
> +       struct bgpio_chip *bgc = container_of(gpio, struct bgpio_chip, gc);
> +       struct timbgpio *tgpio = container_of(bgc, struct timbgpio, bgc);
>
>         if (tgpio->irq_base <= 0)
>                 return -EINVAL;
> @@ -142,7 +98,7 @@ static int timbgpio_irq_type(struct irq_data *d, unsigned trigger)
>         u32 ver;
>         int ret = 0;
>
> -       if (offset < 0 || offset > tgpio->gpio.ngpio)
> +       if (offset < 0 || offset > tgpio->bgc.gc.ngpio)
>                 return -EINVAL;
>
>         ver = ioread32(tgpio->membase + TGPIO_VER);
> @@ -208,8 +164,8 @@ static void timbgpio_irq(unsigned int irq, struct irq_desc *desc)
>          */
>         iowrite32(0, tgpio->membase + TGPIO_IER);
>
> -       for_each_set_bit(offset, &ipr, tgpio->gpio.ngpio)
> -               generic_handle_irq(timbgpio_to_irq(&tgpio->gpio, offset));
> +       for_each_set_bit(offset, &ipr, tgpio->bgc.gc.ngpio)
> +               generic_handle_irq(timbgpio_to_irq(&tgpio->bgc.gc, offset));
>
>         iowrite32(tgpio->last_ier, tgpio->membase + TGPIO_IER);
>  }
> @@ -225,7 +181,6 @@ static int timbgpio_probe(struct platform_device *pdev)
>  {
>         int err, i;
>         struct device *dev = &pdev->dev;
> -       struct gpio_chip *gc;
>         struct timbgpio *tgpio;
>         struct resource *iomem;
>         struct timbgpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -247,6 +202,7 @@ static int timbgpio_probe(struct platform_device *pdev)
>                 dev_err(dev, "Memory alloc failed\n");
>                 return -EINVAL;
>         }
> +
>         tgpio->irq_base = pdata->irq_base;
>
>         spin_lock_init(&tgpio->lock);
> @@ -263,22 +219,24 @@ static int timbgpio_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>         }
>
> -       gc = &tgpio->gpio;
> -
> -       gc->label = dev_name(&pdev->dev);
> -       gc->owner = THIS_MODULE;
> -       gc->dev = &pdev->dev;
> -       gc->direction_input = timbgpio_gpio_direction_input;
> -       gc->get = timbgpio_gpio_get;
> -       gc->direction_output = timbgpio_gpio_direction_output;
> -       gc->set = timbgpio_gpio_set;
> -       gc->to_irq = (irq >= 0 && tgpio->irq_base > 0) ? timbgpio_to_irq : NULL;
> -       gc->dbg_show = NULL;
> -       gc->base = pdata->gpio_base;
> -       gc->ngpio = pdata->nr_pins;
> -       gc->can_sleep = false;
> -
> -       err = gpiochip_add(gc);
> +       err = bgpio_init(&tgpio->bgc, dev, 4, tgpio->membase + TGPIOVAL,
> +                        NULL, NULL, NULL, tgpio->membase + TGPIODIR, 0);
> +
> +       if (err) {
> +               dev_err(&pdev->dev, "bgpio_init failed\n");
> +               return err;
> +       }
> +       tgpio->bgc.gc.label = dev_name(&pdev->dev);
> +       tgpio->bgc.gc.owner = THIS_MODULE;
> +       tgpio->bgc.gc.dev = &pdev->dev;
> +       tgpio->bgc.gc.to_irq = (irq >= 0 && tgpio->irq_base > 0) ?
> +                                               timbgpio_to_irq : NULL;
> +       tgpio->bgc.gc.dbg_show = NULL;
> +       tgpio->bgc.gc.base = pdata->gpio_base;
> +       tgpio->bgc.gc.ngpio = pdata->nr_pins;
> +       tgpio->bgc.gc.can_sleep = false;
> +
> +       err = gpiochip_add(&tgpio->bgc.gc);
>         if (err)
>                 return err;
>
> @@ -322,7 +280,7 @@ static int timbgpio_remove(struct platform_device *pdev)
>                 irq_set_handler_data(irq, NULL);
>         }
>
> -       gpiochip_remove(&tgpio->gpio);
> +       gpiochip_remove(&tgpio->bgc.gc);
>
>         return 0;
>  }
> --
> 1.9.1
>

[PATCH v1 3/5] gpio: iop: convert to use basic mmio gpio library

[kamlakant.patel]

From: Kamlakant Patel <kamlakant.patel@linaro.org>

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

Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
---
 drivers/gpio/Kconfig    |  1 +
 drivers/gpio/gpio-iop.c | 96 +++++++++----------------------------------------
 2 files changed, 18 insertions(+), 79 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 875d312..8cbc3ab 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -407,6 +407,7 @@ config GPIO_ICH
 config GPIO_IOP
 	tristate "Intel IOP GPIO"
 	depends on ARM && (ARCH_IOP32X || ARCH_IOP33X)
+	select GPIO_GENERIC
 	help
 	  Say yes here to support the GPIO functionality of a number of Intel
 	  IOP32X or IOP33X.
diff --git a/drivers/gpio/gpio-iop.c b/drivers/gpio/gpio-iop.c
index 0a5e9d3..d70cf37 100644
--- a/drivers/gpio/gpio-iop.c
+++ b/drivers/gpio/gpio-iop.c
@@ -14,19 +14,12 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/errno.h>
-#include <linux/gpio.h>
 #include <linux/export.h>
 #include <linux/platform_device.h>
-#include <linux/bitops.h>
-#include <linux/io.h>
+#include <linux/basic_mmio_gpio.h>
 
 #define IOP3XX_N_GPIOS	8
 
-#define GPIO_IN			0
-#define GPIO_OUT		1
-#define GPIO_LOW		0
-#define GPIO_HIGH		1
-
 /* Memory base offset */
 static void __iomem *base;
 
@@ -35,86 +28,31 @@ static void __iomem *base;
 #define IOP3XX_GPID		IOP3XX_GPIO_REG(0x0004)
 #define IOP3XX_GPOD		IOP3XX_GPIO_REG(0x0008)
 
-static void gpio_line_config(int line, int direction)
-{
-	unsigned long flags;
-	u32 val;
-
-	local_irq_save(flags);
-	val = readl(IOP3XX_GPOE);
-	if (direction == GPIO_IN) {
-		val |= BIT(line);
-	} else if (direction == GPIO_OUT) {
-		val &= ~BIT(line);
-	}
-	writel(val, IOP3XX_GPOE);
-	local_irq_restore(flags);
-}
-
-static int gpio_line_get(int line)
-{
-	return !!(readl(IOP3XX_GPID) & BIT(line));
-}
-
-static void gpio_line_set(int line, int value)
-{
-	unsigned long flags;
-	u32 val;
-
-	local_irq_save(flags);
-	val = readl(IOP3XX_GPOD);
-	if (value == GPIO_LOW) {
-		val &= ~BIT(line);
-	} else if (value == GPIO_HIGH) {
-		val |= BIT(line);
-	}
-	writel(val, IOP3XX_GPOD);
-	local_irq_restore(flags);
-}
-
-static int iop3xx_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
-{
-	gpio_line_config(gpio, GPIO_IN);
-	return 0;
-}
-
-static int iop3xx_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int level)
-{
-	gpio_line_set(gpio, level);
-	gpio_line_config(gpio, GPIO_OUT);
-	return 0;
-}
-
-static int iop3xx_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
-{
-	return gpio_line_get(gpio);
-}
-
-static void iop3xx_gpio_set_value(struct gpio_chip *chip, unsigned gpio, int value)
-{
-	gpio_line_set(gpio, value);
-}
-
-static struct gpio_chip iop3xx_chip = {
-	.label			= "iop3xx",
-	.direction_input	= iop3xx_gpio_direction_input,
-	.get			= iop3xx_gpio_get_value,
-	.direction_output	= iop3xx_gpio_direction_output,
-	.set			= iop3xx_gpio_set_value,
-	.base			= 0,
-	.ngpio			= IOP3XX_N_GPIOS,
-};
-
 static int iop3xx_gpio_probe(struct platform_device *pdev)
 {
 	struct resource *res;
+	struct bgpio_chip *bgc;
+	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
+	bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL);
+	if (!bgc)
+		return -EINVAL;
+
+	ret = bgpio_init(bgc, &pdev->dev, 4, IOP3XX_GPID,
+			 IOP3XX_GPOD, NULL, NULL, IOP3XX_GPOE, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init failed\n");
+		return ret;
+	}
+	bgc->gc.label			= "iop3xx";
+	bgc->gc.base			= 0;
+	bgc->gc.ngpio			= IOP3XX_N_GPIOS;
 
-	return gpiochip_add(&iop3xx_chip);
+	return gpiochip_add(&bgc->gc);
 }
 
 static struct platform_driver iop3xx_gpio_driver = {
-- 
1.9.1

Re: [PATCH v1 3/5] gpio: iop: convert to use basic mmio gpio library

[Linus Walleij]

Lennert/Arnaud: is any one of you guys still maintaining and testing
IOP stuff so you can say something about this patch?

Or should I just apply it?

Yours,
Linus Walleij


On Mon, Dec 1, 2014 at 1:09 PM,  <kamlakant.patel@linaro.org> wrote:
> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>
> This patch converts IOP GPIO driver to use basic_mmio_gpio
> generic library.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> ---
>  drivers/gpio/Kconfig    |  1 +
>  drivers/gpio/gpio-iop.c | 96 +++++++++----------------------------------------
>  2 files changed, 18 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 875d312..8cbc3ab 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -407,6 +407,7 @@ config GPIO_ICH
>  config GPIO_IOP
>         tristate "Intel IOP GPIO"
>         depends on ARM && (ARCH_IOP32X || ARCH_IOP33X)
> +       select GPIO_GENERIC
>         help
>           Say yes here to support the GPIO functionality of a number of Intel
>           IOP32X or IOP33X.
> diff --git a/drivers/gpio/gpio-iop.c b/drivers/gpio/gpio-iop.c
> index 0a5e9d3..d70cf37 100644
> --- a/drivers/gpio/gpio-iop.c
> +++ b/drivers/gpio/gpio-iop.c
> @@ -14,19 +14,12 @@
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/errno.h>
> -#include <linux/gpio.h>
>  #include <linux/export.h>
>  #include <linux/platform_device.h>
> -#include <linux/bitops.h>
> -#include <linux/io.h>
> +#include <linux/basic_mmio_gpio.h>
>
>  #define IOP3XX_N_GPIOS 8
>
> -#define GPIO_IN                        0
> -#define GPIO_OUT               1
> -#define GPIO_LOW               0
> -#define GPIO_HIGH              1
> -
>  /* Memory base offset */
>  static void __iomem *base;
>
> @@ -35,86 +28,31 @@ static void __iomem *base;
>  #define IOP3XX_GPID            IOP3XX_GPIO_REG(0x0004)
>  #define IOP3XX_GPOD            IOP3XX_GPIO_REG(0x0008)
>
> -static void gpio_line_config(int line, int direction)
> -{
> -       unsigned long flags;
> -       u32 val;
> -
> -       local_irq_save(flags);
> -       val = readl(IOP3XX_GPOE);
> -       if (direction == GPIO_IN) {
> -               val |= BIT(line);
> -       } else if (direction == GPIO_OUT) {
> -               val &= ~BIT(line);
> -       }
> -       writel(val, IOP3XX_GPOE);
> -       local_irq_restore(flags);
> -}
> -
> -static int gpio_line_get(int line)
> -{
> -       return !!(readl(IOP3XX_GPID) & BIT(line));
> -}
> -
> -static void gpio_line_set(int line, int value)
> -{
> -       unsigned long flags;
> -       u32 val;
> -
> -       local_irq_save(flags);
> -       val = readl(IOP3XX_GPOD);
> -       if (value == GPIO_LOW) {
> -               val &= ~BIT(line);
> -       } else if (value == GPIO_HIGH) {
> -               val |= BIT(line);
> -       }
> -       writel(val, IOP3XX_GPOD);
> -       local_irq_restore(flags);
> -}
> -
> -static int iop3xx_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> -{
> -       gpio_line_config(gpio, GPIO_IN);
> -       return 0;
> -}
> -
> -static int iop3xx_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int level)
> -{
> -       gpio_line_set(gpio, level);
> -       gpio_line_config(gpio, GPIO_OUT);
> -       return 0;
> -}
> -
> -static int iop3xx_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
> -{
> -       return gpio_line_get(gpio);
> -}
> -
> -static void iop3xx_gpio_set_value(struct gpio_chip *chip, unsigned gpio, int value)
> -{
> -       gpio_line_set(gpio, value);
> -}
> -
> -static struct gpio_chip iop3xx_chip = {
> -       .label                  = "iop3xx",
> -       .direction_input        = iop3xx_gpio_direction_input,
> -       .get                    = iop3xx_gpio_get_value,
> -       .direction_output       = iop3xx_gpio_direction_output,
> -       .set                    = iop3xx_gpio_set_value,
> -       .base                   = 0,
> -       .ngpio                  = IOP3XX_N_GPIOS,
> -};
> -
>  static int iop3xx_gpio_probe(struct platform_device *pdev)
>  {
>         struct resource *res;
> +       struct bgpio_chip *bgc;
> +       int ret;
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         base = devm_ioremap_resource(&pdev->dev, res);
>         if (IS_ERR(base))
>                 return PTR_ERR(base);
> +       bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL);
> +       if (!bgc)
> +               return -EINVAL;
> +
> +       ret = bgpio_init(bgc, &pdev->dev, 4, IOP3XX_GPID,
> +                        IOP3XX_GPOD, NULL, NULL, IOP3XX_GPOE, 0);
> +       if (ret) {
> +               dev_err(&pdev->dev, "bgpio_init failed\n");
> +               return ret;
> +       }
> +       bgc->gc.label                   = "iop3xx";
> +       bgc->gc.base                    = 0;
> +       bgc->gc.ngpio                   = IOP3XX_N_GPIOS;
>
> -       return gpiochip_add(&iop3xx_chip);
> +       return gpiochip_add(&bgc->gc);
>  }
>
>  static struct platform_driver iop3xx_gpio_driver = {
> --
> 1.9.1
>

Re: [PATCH v1 3/5] gpio: iop: convert to use basic mmio gpio library

[Arnaud Patard]

Linus Walleij <linus.walleij@linaro.org> writes:


Hi,

> Lennert/Arnaud: is any one of you guys still maintaining and testing
> IOP stuff so you can say something about this patch?

I'm still using my iop(80219) system to do backups. So, on the good side, I
still care about it, but on the bad side, it's not easy for me to test
kernels on it.

>
> Or should I just apply it?

I would prefer that some testing is done before applying. I just don't know when
I'll be able to do. Maybe it's easier for Lennert to test it ?

Arnaud

[PATCH v1 4/5] gpio: ge: convert to use basic mmio gpio library

[kamlakant.patel]

From: Kamlakant Patel <kamlakant.patel@linaro.org>

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

Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
---
 drivers/gpio/Kconfig   |  1 +
 drivers/gpio/gpio-ge.c | 96 ++++++++++++++++++++------------------------------
 2 files changed, 40 insertions(+), 57 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8cbc3ab..35a315d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -429,6 +429,7 @@ config GPIO_VX855
 config GPIO_GE_FPGA
 	bool "GE FPGA based GPIO"
 	depends on GE_FPGA
+	select GPIO_GENERIC
 	help
 	  Support for common GPIO functionality provided on some GE Single Board
 	  Computers.
diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
index 1237a73..7579d2e 100644
--- a/drivers/gpio/gpio-ge.c
+++ b/drivers/gpio/gpio-ge.c
@@ -21,7 +21,9 @@
 #include <linux/io.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
+#include <linux/of_address.h>
 #include <linux/module.h>
+#include <linux/basic_mmio_gpio.h>
 
 #define GEF_GPIO_DIRECT		0x00
 #define GEF_GPIO_IN		0x04
@@ -33,53 +35,6 @@
 #define GEF_GPIO_OVERRUN	0x1C
 #define GEF_GPIO_MODE		0x20
 
-static void gef_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
-{
-	struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
-	unsigned int data;
-
-	data = ioread32be(mmchip->regs + GEF_GPIO_OUT);
-	if (value)
-		data = data | BIT(offset);
-	else
-		data = data & ~BIT(offset);
-	iowrite32be(data, mmchip->regs + GEF_GPIO_OUT);
-}
-
-static int gef_gpio_dir_in(struct gpio_chip *chip, unsigned offset)
-{
-	unsigned int data;
-	struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
-
-	data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT);
-	data = data | BIT(offset);
-	iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT);
-
-	return 0;
-}
-
-static int gef_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int value)
-{
-	unsigned int data;
-	struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
-
-	/* Set value before switching to output */
-	gef_gpio_set(mmchip->regs + GEF_GPIO_OUT, offset, value);
-
-	data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT);
-	data = data & ~BIT(offset);
-	iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT);
-
-	return 0;
-}
-
-static int gef_gpio_get(struct gpio_chip *chip, unsigned offset)
-{
-	struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
-
-	return !!(ioread32be(mmchip->regs + GEF_GPIO_IN) & BIT(offset));
-}
-
 static const struct of_device_id gef_gpio_ids[] = {
 	{
 		.compatible	= "gef,sbc610-gpio",
@@ -99,22 +54,49 @@ static int __init gef_gpio_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id =
 		of_match_device(gef_gpio_ids, &pdev->dev);
-	struct of_mm_gpio_chip *mmchip;
+	struct bgpio_chip *bgc;
+	void __iomem *regs;
+	int ret;
 
-	mmchip = devm_kzalloc(&pdev->dev, sizeof(*mmchip), GFP_KERNEL);
-	if (!mmchip)
+	bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL);
+	if (!bgc)
 		return -ENOMEM;
 
+	regs = of_iomap(pdev->dev.of_node, 0);
+	if (!regs)
+		return -ENOMEM;
+
+	ret = bgpio_init(bgc, &pdev->dev, 4, regs + GEF_GPIO_IN,
+			 regs + GEF_GPIO_OUT, NULL, NULL,
+			 regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+	if (ret) {
+		dev_err(&pdev->dev, "bgpio_init failed\n");
+		goto err0;
+	}
+
 	/* Setup pointers to chip functions */
-	mmchip->gc.ngpio = (u16)(uintptr_t)of_id->data;
-	mmchip->gc.of_gpio_n_cells = 2;
-	mmchip->gc.direction_input = gef_gpio_dir_in;
-	mmchip->gc.direction_output = gef_gpio_dir_out;
-	mmchip->gc.get = gef_gpio_get;
-	mmchip->gc.set = gef_gpio_set;
+	bgc->gc.label = kstrdup(pdev->dev.of_node->full_name, GFP_KERNEL);
+	if (!bgc->gc.label)
+		goto err0;
+
+	bgc->gc.base = -1;
+	bgc->gc.ngpio = (u16)(uintptr_t)of_id->data;
+	bgc->gc.of_gpio_n_cells = 2;
+	bgc->gc.of_node = pdev->dev.of_node;
 
 	/* This function adds a memory mapped GPIO chip */
-	return of_mm_gpiochip_add(pdev->dev.of_node, mmchip);
+	ret = gpiochip_add(&bgc->gc);
+	if (ret)
+		goto err1;
+
+	return 0;
+err1:
+	kfree(bgc->gc.label);
+err0:
+	iounmap(regs);
+	pr_err("%s: GPIO chip registration failed\n",
+			pdev->dev.of_node->full_name);
+	return ret;
 };
 
 static struct platform_driver gef_gpio_driver = {
-- 
1.9.1

Re: [PATCH v1 4/5] gpio: ge: convert to use basic mmio gpio library

[Linus Walleij]

Martyn,

do you have some feedback on this patch? Can you test it?
Shall I just apply it?

Yours,
Linus Walleij

On Mon, Dec 1, 2014 at 1:09 PM,  <kamlakant.patel@linaro.org> wrote:
> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>
> This patch converts GE GPIO driver to use basic_mmio_gpio
> generic library.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> ---
>  drivers/gpio/Kconfig   |  1 +
>  drivers/gpio/gpio-ge.c | 96 ++++++++++++++++++++------------------------------
>  2 files changed, 40 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 8cbc3ab..35a315d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -429,6 +429,7 @@ config GPIO_VX855
>  config GPIO_GE_FPGA
>         bool "GE FPGA based GPIO"
>         depends on GE_FPGA
> +       select GPIO_GENERIC
>         help
>           Support for common GPIO functionality provided on some GE Single Board
>           Computers.
> diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
> index 1237a73..7579d2e 100644
> --- a/drivers/gpio/gpio-ge.c
> +++ b/drivers/gpio/gpio-ge.c
> @@ -21,7 +21,9 @@
>  #include <linux/io.h>
>  #include <linux/of_device.h>
>  #include <linux/of_gpio.h>
> +#include <linux/of_address.h>
>  #include <linux/module.h>
> +#include <linux/basic_mmio_gpio.h>
>
>  #define GEF_GPIO_DIRECT                0x00
>  #define GEF_GPIO_IN            0x04
> @@ -33,53 +35,6 @@
>  #define GEF_GPIO_OVERRUN       0x1C
>  #define GEF_GPIO_MODE          0x20
>
> -static void gef_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> -{
> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
> -       unsigned int data;
> -
> -       data = ioread32be(mmchip->regs + GEF_GPIO_OUT);
> -       if (value)
> -               data = data | BIT(offset);
> -       else
> -               data = data & ~BIT(offset);
> -       iowrite32be(data, mmchip->regs + GEF_GPIO_OUT);
> -}
> -
> -static int gef_gpio_dir_in(struct gpio_chip *chip, unsigned offset)
> -{
> -       unsigned int data;
> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
> -
> -       data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT);
> -       data = data | BIT(offset);
> -       iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT);
> -
> -       return 0;
> -}
> -
> -static int gef_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int value)
> -{
> -       unsigned int data;
> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
> -
> -       /* Set value before switching to output */
> -       gef_gpio_set(mmchip->regs + GEF_GPIO_OUT, offset, value);
> -
> -       data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT);
> -       data = data & ~BIT(offset);
> -       iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT);
> -
> -       return 0;
> -}
> -
> -static int gef_gpio_get(struct gpio_chip *chip, unsigned offset)
> -{
> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
> -
> -       return !!(ioread32be(mmchip->regs + GEF_GPIO_IN) & BIT(offset));
> -}
> -
>  static const struct of_device_id gef_gpio_ids[] = {
>         {
>                 .compatible     = "gef,sbc610-gpio",
> @@ -99,22 +54,49 @@ static int __init gef_gpio_probe(struct platform_device *pdev)
>  {
>         const struct of_device_id *of_id =
>                 of_match_device(gef_gpio_ids, &pdev->dev);
> -       struct of_mm_gpio_chip *mmchip;
> +       struct bgpio_chip *bgc;
> +       void __iomem *regs;
> +       int ret;
>
> -       mmchip = devm_kzalloc(&pdev->dev, sizeof(*mmchip), GFP_KERNEL);
> -       if (!mmchip)
> +       bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL);
> +       if (!bgc)
>                 return -ENOMEM;
>
> +       regs = of_iomap(pdev->dev.of_node, 0);
> +       if (!regs)
> +               return -ENOMEM;
> +
> +       ret = bgpio_init(bgc, &pdev->dev, 4, regs + GEF_GPIO_IN,
> +                        regs + GEF_GPIO_OUT, NULL, NULL,
> +                        regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> +       if (ret) {
> +               dev_err(&pdev->dev, "bgpio_init failed\n");
> +               goto err0;
> +       }
> +
>         /* Setup pointers to chip functions */
> -       mmchip->gc.ngpio = (u16)(uintptr_t)of_id->data;
> -       mmchip->gc.of_gpio_n_cells = 2;
> -       mmchip->gc.direction_input = gef_gpio_dir_in;
> -       mmchip->gc.direction_output = gef_gpio_dir_out;
> -       mmchip->gc.get = gef_gpio_get;
> -       mmchip->gc.set = gef_gpio_set;
> +       bgc->gc.label = kstrdup(pdev->dev.of_node->full_name, GFP_KERNEL);
> +       if (!bgc->gc.label)
> +               goto err0;
> +
> +       bgc->gc.base = -1;
> +       bgc->gc.ngpio = (u16)(uintptr_t)of_id->data;
> +       bgc->gc.of_gpio_n_cells = 2;
> +       bgc->gc.of_node = pdev->dev.of_node;
>
>         /* This function adds a memory mapped GPIO chip */
> -       return of_mm_gpiochip_add(pdev->dev.of_node, mmchip);
> +       ret = gpiochip_add(&bgc->gc);
> +       if (ret)
> +               goto err1;
> +
> +       return 0;
> +err1:
> +       kfree(bgc->gc.label);
> +err0:
> +       iounmap(regs);
> +       pr_err("%s: GPIO chip registration failed\n",
> +                       pdev->dev.of_node->full_name);
> +       return ret;
>  };
>
>  static struct platform_driver gef_gpio_driver = {
> --
> 1.9.1
>

Re: [PATCH v1 4/5] gpio: ge: convert to use basic mmio gpio library

[Martyn Welch]

Hi Linus,

Sorry for the delayed response, little bit snowed under.

I'm not going to get a chance to test it, but it looks sane to me. I'm 
happy for this to be applied.

Thanks for your patience,

Martyn

On 09/01/15 09:21, Linus Walleij wrote:
> Martyn,
>
> do you have some feedback on this patch? Can you test it?
> Shall I just apply it?
>
> Yours,
> Linus Walleij
>
> On Mon, Dec 1, 2014 at 1:09 PM,  <kamlakant.patel@linaro.org> wrote:
>> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>>
>> This patch converts GE GPIO driver to use basic_mmio_gpio
>> generic library.
>>
>> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
>> ---
>>   drivers/gpio/Kconfig   |  1 +
>>   drivers/gpio/gpio-ge.c | 96 ++++++++++++++++++++------------------------------
>>   2 files changed, 40 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 8cbc3ab..35a315d 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -429,6 +429,7 @@ config GPIO_VX855
>>   config GPIO_GE_FPGA
>>          bool "GE FPGA based GPIO"
>>          depends on GE_FPGA
>> +       select GPIO_GENERIC
>>          help
>>            Support for common GPIO functionality provided on some GE Single Board
>>            Computers.
>> diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
>> index 1237a73..7579d2e 100644
>> --- a/drivers/gpio/gpio-ge.c
>> +++ b/drivers/gpio/gpio-ge.c
>> @@ -21,7 +21,9 @@
>>   #include <linux/io.h>
>>   #include <linux/of_device.h>
>>   #include <linux/of_gpio.h>
>> +#include <linux/of_address.h>
>>   #include <linux/module.h>
>> +#include <linux/basic_mmio_gpio.h>
>>
>>   #define GEF_GPIO_DIRECT                0x00
>>   #define GEF_GPIO_IN            0x04
>> @@ -33,53 +35,6 @@
>>   #define GEF_GPIO_OVERRUN       0x1C
>>   #define GEF_GPIO_MODE          0x20
>>
>> -static void gef_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> -{
>> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
>> -       unsigned int data;
>> -
>> -       data = ioread32be(mmchip->regs + GEF_GPIO_OUT);
>> -       if (value)
>> -               data = data | BIT(offset);
>> -       else
>> -               data = data & ~BIT(offset);
>> -       iowrite32be(data, mmchip->regs + GEF_GPIO_OUT);
>> -}
>> -
>> -static int gef_gpio_dir_in(struct gpio_chip *chip, unsigned offset)
>> -{
>> -       unsigned int data;
>> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
>> -
>> -       data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT);
>> -       data = data | BIT(offset);
>> -       iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT);
>> -
>> -       return 0;
>> -}
>> -
>> -static int gef_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int value)
>> -{
>> -       unsigned int data;
>> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
>> -
>> -       /* Set value before switching to output */
>> -       gef_gpio_set(mmchip->regs + GEF_GPIO_OUT, offset, value);
>> -
>> -       data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT);
>> -       data = data & ~BIT(offset);
>> -       iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT);
>> -
>> -       return 0;
>> -}
>> -
>> -static int gef_gpio_get(struct gpio_chip *chip, unsigned offset)
>> -{
>> -       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
>> -
>> -       return !!(ioread32be(mmchip->regs + GEF_GPIO_IN) & BIT(offset));
>> -}
>> -
>>   static const struct of_device_id gef_gpio_ids[] = {
>>          {
>>                  .compatible     = "gef,sbc610-gpio",
>> @@ -99,22 +54,49 @@ static int __init gef_gpio_probe(struct platform_device *pdev)
>>   {
>>          const struct of_device_id *of_id =
>>                  of_match_device(gef_gpio_ids, &pdev->dev);
>> -       struct of_mm_gpio_chip *mmchip;
>> +       struct bgpio_chip *bgc;
>> +       void __iomem *regs;
>> +       int ret;
>>
>> -       mmchip = devm_kzalloc(&pdev->dev, sizeof(*mmchip), GFP_KERNEL);
>> -       if (!mmchip)
>> +       bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL);
>> +       if (!bgc)
>>                  return -ENOMEM;
>>
>> +       regs = of_iomap(pdev->dev.of_node, 0);
>> +       if (!regs)
>> +               return -ENOMEM;
>> +
>> +       ret = bgpio_init(bgc, &pdev->dev, 4, regs + GEF_GPIO_IN,
>> +                        regs + GEF_GPIO_OUT, NULL, NULL,
>> +                        regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "bgpio_init failed\n");
>> +               goto err0;
>> +       }
>> +
>>          /* Setup pointers to chip functions */
>> -       mmchip->gc.ngpio = (u16)(uintptr_t)of_id->data;
>> -       mmchip->gc.of_gpio_n_cells = 2;
>> -       mmchip->gc.direction_input = gef_gpio_dir_in;
>> -       mmchip->gc.direction_output = gef_gpio_dir_out;
>> -       mmchip->gc.get = gef_gpio_get;
>> -       mmchip->gc.set = gef_gpio_set;
>> +       bgc->gc.label = kstrdup(pdev->dev.of_node->full_name, GFP_KERNEL);
>> +       if (!bgc->gc.label)
>> +               goto err0;
>> +
>> +       bgc->gc.base = -1;
>> +       bgc->gc.ngpio = (u16)(uintptr_t)of_id->data;
>> +       bgc->gc.of_gpio_n_cells = 2;
>> +       bgc->gc.of_node = pdev->dev.of_node;
>>
>>          /* This function adds a memory mapped GPIO chip */
>> -       return of_mm_gpiochip_add(pdev->dev.of_node, mmchip);
>> +       ret = gpiochip_add(&bgc->gc);
>> +       if (ret)
>> +               goto err1;
>> +
>> +       return 0;
>> +err1:
>> +       kfree(bgc->gc.label);
>> +err0:
>> +       iounmap(regs);
>> +       pr_err("%s: GPIO chip registration failed\n",
>> +                       pdev->dev.of_node->full_name);
>> +       return ret;
>>   };
>>
>>   static struct platform_driver gef_gpio_driver = {
>> --
>> 1.9.1
>>

-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@ge.com                  | VAT:GB 927559189

Re: [PATCH v1 4/5] gpio: ge: convert to use basic mmio gpio library

[Linus Walleij]

On Mon, Dec 1, 2014 at 1:09 PM,  <kamlakant.patel@linaro.org> wrote:

> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>
> This patch converts GE GPIO driver to use basic_mmio_gpio
> generic library.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>

Patch applied with Martyn's Acked-by.

Yours,
Linus Walleij

[PATCH v1 5/5] gpio: document basic mmio gpio library

[kamlakant.patel]

From: Kamlakant Patel <kamlakant.patel@linaro.org>

This is a brief documentation on how to use GPIO Generic
library for memory-mapped GPIO controllers.

Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
---
 Documentation/gpio/driver.txt | 50 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index 31e0b5d..563abea 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -190,3 +190,53 @@ gpiochip_free_own_desc().
 These functions must be used with care since they do not affect module use
 count. Do not use the functions to request gpio descriptors not owned by the
 calling driver.
+
+
+Generic driver for memory-mapped GPIO controllers
+-------------------------------------------------
+The GPIO generic library provides support for basic platform_device
+memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
+symbol GPIO_GENERIC and using library functions provided by GPIO generic
+driver (see drivers/gpio/gpio-generic.c).
+The simplest form of a GPIO controller that the driver support is just a
+single "data" register, where GPIO state can be read and/or written.
+
+The driver can be registered using "basic-mmio-gpio" or for big-endian
+notation support use "basic-mmio-gpio-be". The code will configure gpio_chip
+and issue gpiochip_add().
+
+The driver supports:
+- 8/16/32/64 bits registers. The number of GPIOs is determined by the width of
+  the registers.
+- GPIO controllers with clear/set registers.
+- GPIO controllers with a single "data" register.
+- Big endian bits/GPIOs ordering.
+
+For setting GPIO's there are three supported configurations:
+- single input/output register resource (named "dat").
+- set/clear pair (named "set" and "clr").
+- single output register resource and single input resource ("set" and dat").
+
+For setting the GPIO direction, there are three supported configurations:
+- simple bidirection GPIO that requires no configuration.
+- an output direction register (named "dirout") where a 1 bit indicates the
+  GPIO is an output.
+- an input direction register (named "dirin") where a 1 bit indicates the GPIO
+  is an input.
+
+It is possible to use only parts of GPIO generic library. Each GPIO controller
+using GPIO generic library needs to include the following header.
+
+        #include <linux/basic_mmio_gpio.h>
+
+Use bgpio_init to configure gpio_chip and bgpio_remove to remove the controller.
+int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
+               unsigned long sz, void __iomem *dat, void __iomem *set,
+               void __iomem *clr, void __iomem *dirout, void __iomem *dirin,
+               unsigned long flags);
+
+The "flag" parameter can be following depending on controller configuration:
+BGPIOF_BIG_ENDIAN               BIT(0)
+BGPIOF_UNREADABLE_REG_SET       BIT(1) /* reg_set is unreadable */
+BGPIOF_UNREADABLE_REG_DIR       BIT(2) /* reg_dir is unreadable */
+BGPIOF_BIG_ENDIAN_BYTE_ORDER    BIT(3)
-- 
1.9.1

Re: [PATCH v1 5/5] gpio: document basic mmio gpio library

[Alexandre Courbot]

On Mon, Dec 1, 2014 at 9:09 PM,  <kamlakant.patel@linaro.org> wrote:
> From: Kamlakant Patel <kamlakant.patel@linaro.org>
>
> This is a brief documentation on how to use GPIO Generic
> library for memory-mapped GPIO controllers.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> ---
>  Documentation/gpio/driver.txt | 50 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)

Yum, more doc!

>
> diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
> index 31e0b5d..563abea 100644
> --- a/Documentation/gpio/driver.txt
> +++ b/Documentation/gpio/driver.txt
> @@ -190,3 +190,53 @@ gpiochip_free_own_desc().
>  These functions must be used with care since they do not affect module use
>  count. Do not use the functions to request gpio descriptors not owned by the
>  calling driver.
> +
> +
> +Generic driver for memory-mapped GPIO controllers
> +-------------------------------------------------
> +The GPIO generic library provides support for basic platform_device
> +memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
> +symbol GPIO_GENERIC and using library functions provided by GPIO generic
> +driver (see drivers/gpio/gpio-generic.c).
> +The simplest form of a GPIO controller that the driver support is just a

s/support/supports

> +single "data" register, where GPIO state can be read and/or written.
> +
> +The driver can be registered using "basic-mmio-gpio" or for big-endian
> +notation support use "basic-mmio-gpio-be". The code will configure gpio_chip

Using where? You should say that this is for the platform device name.

> +and issue gpiochip_add().

> +
> +The driver supports:
> +- 8/16/32/64 bits registers. The number of GPIOs is determined by the width of
> +  the registers.
> +- GPIO controllers with clear/set registers.
> +- GPIO controllers with a single "data" register.
> +- Big endian bits/GPIOs ordering.

Maybe add a sentence indicating that these settings are defined in the
drivers using named memory resources.

> +
> +For setting GPIO's there are three supported configurations:
> +- single input/output register resource (named "dat").

This resource seems to be mandatory - please make sure you mention this fact.

> +- set/clear pair (named "set" and "clr").
> +- single output register resource and single input resource ("set" and dat").
> +
> +For setting the GPIO direction, there are three supported configurations:
> +- simple bidirection GPIO that requires no configuration.

s/bidirection/bidirectional maybe?

> +- an output direction register (named "dirout") where a 1 bit indicates the
> +  GPIO is an output.
> +- an input direction register (named "dirin") where a 1 bit indicates the GPIO
> +  is an input.
> +
> +It is possible to use only parts of GPIO generic library. Each GPIO controller
> +using GPIO generic library needs to include the following header.
> +
> +        #include <linux/basic_mmio_gpio.h>
> +
> +Use bgpio_init to configure gpio_chip and bgpio_remove to remove the controller.
> +int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
> +               unsigned long sz, void __iomem *dat, void __iomem *set,
> +               void __iomem *clr, void __iomem *dirout, void __iomem *dirin,
> +               unsigned long flags);

If you put the prototype for bgpio_init(), please also put the one of
bgpio_remove()...

> +
> +The "flag" parameter can be following depending on controller configuration:
> +BGPIOF_BIG_ENDIAN               BIT(0)
> +BGPIOF_UNREADABLE_REG_SET       BIT(1) /* reg_set is unreadable */
> +BGPIOF_UNREADABLE_REG_DIR       BIT(2) /* reg_dir is unreadable */
> +BGPIOF_BIG_ENDIAN_BYTE_ORDER    BIT(3)

Right now this documentation is a little bit confusing. Basically
there are two ways to use this driver:

1) Name your platform device ""basic-mmio-gpio" or
"basic-mmio-gpio-be", set the right named memory resources to specify
the desired configuration, and let bgpio_pdev_probe() do all the work.

2) Allocate a bgpio_chip yourself, call bgpio_init() on it and its
resources, and finally invoke gpiochip_add() yourself.

These two different ways of doing kind of seem to be mixed together.
Can you try to highlight the fact that these are alternatives?

Re: [PATCH v1 5/5] gpio: document basic mmio gpio library

[Kamlakant Patel]

On 4 December 2014 at 14:42, 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 is a brief documentation on how to use GPIO Generic
>> library for memory-mapped GPIO controllers.
>>
>> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
>> ---
>>  Documentation/gpio/driver.txt | 50 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>
> Yum, more doc!
>
>>
>> diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
>> index 31e0b5d..563abea 100644
>> --- a/Documentation/gpio/driver.txt
>> +++ b/Documentation/gpio/driver.txt
>> @@ -190,3 +190,53 @@ gpiochip_free_own_desc().
>>  These functions must be used with care since they do not affect module use
>>  count. Do not use the functions to request gpio descriptors not owned by the
>>  calling driver.
>> +
>> +
>> +Generic driver for memory-mapped GPIO controllers
>> +-------------------------------------------------
>> +The GPIO generic library provides support for basic platform_device
>> +memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
>> +symbol GPIO_GENERIC and using library functions provided by GPIO generic
>> +driver (see drivers/gpio/gpio-generic.c).
>> +The simplest form of a GPIO controller that the driver support is just a
>
> s/support/supports
>
>> +single "data" register, where GPIO state can be read and/or written.
>> +
>> +The driver can be registered using "basic-mmio-gpio" or for big-endian
>> +notation support use "basic-mmio-gpio-be". The code will configure gpio_chip
>
> Using where? You should say that this is for the platform device name.
>
>> +and issue gpiochip_add().
>
>> +
>> +The driver supports:
>> +- 8/16/32/64 bits registers. The number of GPIOs is determined by the width of
>> +  the registers.
>> +- GPIO controllers with clear/set registers.
>> +- GPIO controllers with a single "data" register.
>> +- Big endian bits/GPIOs ordering.
>
> Maybe add a sentence indicating that these settings are defined in the
> drivers using named memory resources.
>
>> +
>> +For setting GPIO's there are three supported configurations:
>> +- single input/output register resource (named "dat").
>
> This resource seems to be mandatory - please make sure you mention this fact.
>
>> +- set/clear pair (named "set" and "clr").
>> +- single output register resource and single input resource ("set" and dat").
>> +
>> +For setting the GPIO direction, there are three supported configurations:
>> +- simple bidirection GPIO that requires no configuration.
>
> s/bidirection/bidirectional maybe?
>
>> +- an output direction register (named "dirout") where a 1 bit indicates the
>> +  GPIO is an output.
>> +- an input direction register (named "dirin") where a 1 bit indicates the GPIO
>> +  is an input.
>> +
>> +It is possible to use only parts of GPIO generic library. Each GPIO controller
>> +using GPIO generic library needs to include the following header.
>> +
>> +        #include <linux/basic_mmio_gpio.h>
>> +
>> +Use bgpio_init to configure gpio_chip and bgpio_remove to remove the controller.
>> +int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
>> +               unsigned long sz, void __iomem *dat, void __iomem *set,
>> +               void __iomem *clr, void __iomem *dirout, void __iomem *dirin,
>> +               unsigned long flags);
>
> If you put the prototype for bgpio_init(), please also put the one of
> bgpio_remove()...
>
>> +
>> +The "flag" parameter can be following depending on controller configuration:
>> +BGPIOF_BIG_ENDIAN               BIT(0)
>> +BGPIOF_UNREADABLE_REG_SET       BIT(1) /* reg_set is unreadable */
>> +BGPIOF_UNREADABLE_REG_DIR       BIT(2) /* reg_dir is unreadable */
>> +BGPIOF_BIG_ENDIAN_BYTE_ORDER    BIT(3)
>
> Right now this documentation is a little bit confusing. Basically
> there are two ways to use this driver:
>
> 1) Name your platform device ""basic-mmio-gpio" or
> "basic-mmio-gpio-be", set the right named memory resources to specify
> the desired configuration, and let bgpio_pdev_probe() do all the work.
>
> 2) Allocate a bgpio_chip yourself, call bgpio_init() on it and its
> resources, and finally invoke gpiochip_add() yourself.
>
> These two different ways of doing kind of seem to be mixed together.
> Can you try to highlight the fact that these are alternatives?

Thanks for the review comments and suggestions. I will update and send
the patch.

Thanks,
Kamlakant Patel

Re: [PATCH v1 5/5] gpio: document basic mmio gpio library

[Kamlakant Patel]

On Thu, Dec 04, 2014 at 05:52:24PM +0530, Kamlakant Patel wrote:
> On 4 December 2014 at 14:42, 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 is a brief documentation on how to use GPIO Generic
> >> library for memory-mapped GPIO controllers.
> >>
> >> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> >> ---
> >>  Documentation/gpio/driver.txt | 50 +++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 50 insertions(+)
> >
> > Yum, more doc!
> >
> >>
> >> diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
> >> index 31e0b5d..563abea 100644
> >> --- a/Documentation/gpio/driver.txt
> >> +++ b/Documentation/gpio/driver.txt
> >> @@ -190,3 +190,53 @@ gpiochip_free_own_desc().
> >>  These functions must be used with care since they do not affect module use
> >>  count. Do not use the functions to request gpio descriptors not owned by the
> >>  calling driver.
> >> +
> >> +
> >> +Generic driver for memory-mapped GPIO controllers
> >> +-------------------------------------------------
> >> +The GPIO generic library provides support for basic platform_device
> >> +memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
> >> +symbol GPIO_GENERIC and using library functions provided by GPIO generic
> >> +driver (see drivers/gpio/gpio-generic.c).
> >> +The simplest form of a GPIO controller that the driver support is just a
> >
> > s/support/supports
> >
> >> +single "data" register, where GPIO state can be read and/or written.
> >> +
> >> +The driver can be registered using "basic-mmio-gpio" or for big-endian
> >> +notation support use "basic-mmio-gpio-be". The code will configure gpio_chip
> >
> > Using where? You should say that this is for the platform device name.
> >
> >> +and issue gpiochip_add().
> >
> >> +
> >> +The driver supports:
> >> +- 8/16/32/64 bits registers. The number of GPIOs is determined by the width of
> >> +  the registers.
> >> +- GPIO controllers with clear/set registers.
> >> +- GPIO controllers with a single "data" register.
> >> +- Big endian bits/GPIOs ordering.
> >
> > Maybe add a sentence indicating that these settings are defined in the
> > drivers using named memory resources.
> >
> >> +
> >> +For setting GPIO's there are three supported configurations:
> >> +- single input/output register resource (named "dat").
> >
> > This resource seems to be mandatory - please make sure you mention this fact.
> >
> >> +- set/clear pair (named "set" and "clr").
> >> +- single output register resource and single input resource ("set" and dat").
> >> +
> >> +For setting the GPIO direction, there are three supported configurations:
> >> +- simple bidirection GPIO that requires no configuration.
> >
> > s/bidirection/bidirectional maybe?
> >
> >> +- an output direction register (named "dirout") where a 1 bit indicates the
> >> +  GPIO is an output.
> >> +- an input direction register (named "dirin") where a 1 bit indicates the GPIO
> >> +  is an input.
> >> +
> >> +It is possible to use only parts of GPIO generic library. Each GPIO controller
> >> +using GPIO generic library needs to include the following header.
> >> +
> >> +        #include <linux/basic_mmio_gpio.h>
> >> +
> >> +Use bgpio_init to configure gpio_chip and bgpio_remove to remove the controller.
> >> +int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
> >> +               unsigned long sz, void __iomem *dat, void __iomem *set,
> >> +               void __iomem *clr, void __iomem *dirout, void __iomem *dirin,
> >> +               unsigned long flags);
> >
> > If you put the prototype for bgpio_init(), please also put the one of
> > bgpio_remove()...
> >
> >> +
> >> +The "flag" parameter can be following depending on controller configuration:
> >> +BGPIOF_BIG_ENDIAN               BIT(0)
> >> +BGPIOF_UNREADABLE_REG_SET       BIT(1) /* reg_set is unreadable */
> >> +BGPIOF_UNREADABLE_REG_DIR       BIT(2) /* reg_dir is unreadable */
> >> +BGPIOF_BIG_ENDIAN_BYTE_ORDER    BIT(3)
> >
> > Right now this documentation is a little bit confusing. Basically
> > there are two ways to use this driver:
> >
> > 1) Name your platform device ""basic-mmio-gpio" or
> > "basic-mmio-gpio-be", set the right named memory resources to specify
> > the desired configuration, and let bgpio_pdev_probe() do all the work.
> >
> > 2) Allocate a bgpio_chip yourself, call bgpio_init() on it and its
> > resources, and finally invoke gpiochip_add() yourself.
> >
> > These two different ways of doing kind of seem to be mixed together.
> > Can you try to highlight the fact that these are alternatives?
> 

This is an updated version of previous patch. Please review.

diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index 31e0b5d..f6b617a 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -190,3 +190,45 @@ gpiochip_free_own_desc().
 These functions must be used with care since they do not affect module use
 count. Do not use the functions to request gpio descriptors not owned by the
 calling driver.
+
+
+Generic driver for memory-mapped GPIO controllers
+-------------------------------------------------
+The GPIO generic library provides support for basic platform_device
+memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
+symbol GPIO_GENERIC and using library functions provided by GPIO generic
+driver (see drivers/gpio/gpio-generic.c).
+
+The driver supports registers of the sizes of 8/16/32/64 bits and the number
+of GPIOs are determined by the width of the registers. A set of named memory
+resources should be defined in the drivers (e.g "dat", "set", "clr", "dirout"
+and "dirin"), where "dat" is a mandatory resource.
+
+Each GPIO controller using GPIO generic library needs to include the following
+header.
+        #include <linux/basic_mmio_gpio.h>
+
+There are two ways to use this driver:
+1. Using basic GPIO MMIO Generic driver directly:
+   Name your platform device "basic-mmio-gpio" or "basic-mmio-gpio-be", set the
+   right named memory resources to specify the desired configuration, and let
+   bgpio_pdev_probe do all the work.
+
+2. Using basic GPIO MMIO Generic library in your driver:
+   Allocate a bgpio_chip yourself in your GPIO driver, call bgpio_init() on it
+   and its resources, and finally invoke gpiochip_add yourself. It is possible
+   to use only parts of the driver, you can overwrite functions and variables
+   in your driver, if necessary.
+   You can call bgpio_remove() to unregister a gpio_chip.
+
+For setting up GPIO's there are three supported configurations:
+- single input/output register resource (named "dat").
+- set/clear pair (named "set" and "clr").
+- single output register resource and single input resource ("set" and "dat").
+
+For setting the GPIO direction, there are three supported configurations:
+- simple bidirectional GPIO that requires no configuration.
+- an output direction register (named "dirout") where a 1 bit indicates the
+  GPIO is an output.
+- an input direction register (named "dirin") where a 1 bit indicates the GPIO
+  is an input.
-- 
1.9.1

Re: [PATCH v1 5/5] gpio: document basic mmio gpio library

[Alexandre Courbot]

On Thu, Dec 18, 2014 at 10:44 PM, Kamlakant Patel
<kamlakant.patel@linaro.org> wrote:
> On Thu, Dec 04, 2014 at 05:52:24PM +0530, Kamlakant Patel wrote:
>> On 4 December 2014 at 14:42, 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 is a brief documentation on how to use GPIO Generic
>> >> library for memory-mapped GPIO controllers.
>> >>
>> >> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
>> >> ---
>> >>  Documentation/gpio/driver.txt | 50 +++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 50 insertions(+)
>> >
>> > Yum, more doc!
>> >
>> >>
>> >> diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
>> >> index 31e0b5d..563abea 100644
>> >> --- a/Documentation/gpio/driver.txt
>> >> +++ b/Documentation/gpio/driver.txt
>> >> @@ -190,3 +190,53 @@ gpiochip_free_own_desc().
>> >>  These functions must be used with care since they do not affect module use
>> >>  count. Do not use the functions to request gpio descriptors not owned by the
>> >>  calling driver.
>> >> +
>> >> +
>> >> +Generic driver for memory-mapped GPIO controllers
>> >> +-------------------------------------------------
>> >> +The GPIO generic library provides support for basic platform_device
>> >> +memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
>> >> +symbol GPIO_GENERIC and using library functions provided by GPIO generic
>> >> +driver (see drivers/gpio/gpio-generic.c).
>> >> +The simplest form of a GPIO controller that the driver support is just a
>> >
>> > s/support/supports
>> >
>> >> +single "data" register, where GPIO state can be read and/or written.
>> >> +
>> >> +The driver can be registered using "basic-mmio-gpio" or for big-endian
>> >> +notation support use "basic-mmio-gpio-be". The code will configure gpio_chip
>> >
>> > Using where? You should say that this is for the platform device name.
>> >
>> >> +and issue gpiochip_add().
>> >
>> >> +
>> >> +The driver supports:
>> >> +- 8/16/32/64 bits registers. The number of GPIOs is determined by the width of
>> >> +  the registers.
>> >> +- GPIO controllers with clear/set registers.
>> >> +- GPIO controllers with a single "data" register.
>> >> +- Big endian bits/GPIOs ordering.
>> >
>> > Maybe add a sentence indicating that these settings are defined in the
>> > drivers using named memory resources.
>> >
>> >> +
>> >> +For setting GPIO's there are three supported configurations:
>> >> +- single input/output register resource (named "dat").
>> >
>> > This resource seems to be mandatory - please make sure you mention this fact.
>> >
>> >> +- set/clear pair (named "set" and "clr").
>> >> +- single output register resource and single input resource ("set" and dat").
>> >> +
>> >> +For setting the GPIO direction, there are three supported configurations:
>> >> +- simple bidirection GPIO that requires no configuration.
>> >
>> > s/bidirection/bidirectional maybe?
>> >
>> >> +- an output direction register (named "dirout") where a 1 bit indicates the
>> >> +  GPIO is an output.
>> >> +- an input direction register (named "dirin") where a 1 bit indicates the GPIO
>> >> +  is an input.
>> >> +
>> >> +It is possible to use only parts of GPIO generic library. Each GPIO controller
>> >> +using GPIO generic library needs to include the following header.
>> >> +
>> >> +        #include <linux/basic_mmio_gpio.h>
>> >> +
>> >> +Use bgpio_init to configure gpio_chip and bgpio_remove to remove the controller.
>> >> +int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
>> >> +               unsigned long sz, void __iomem *dat, void __iomem *set,
>> >> +               void __iomem *clr, void __iomem *dirout, void __iomem *dirin,
>> >> +               unsigned long flags);
>> >
>> > If you put the prototype for bgpio_init(), please also put the one of
>> > bgpio_remove()...
>> >
>> >> +
>> >> +The "flag" parameter can be following depending on controller configuration:
>> >> +BGPIOF_BIG_ENDIAN               BIT(0)
>> >> +BGPIOF_UNREADABLE_REG_SET       BIT(1) /* reg_set is unreadable */
>> >> +BGPIOF_UNREADABLE_REG_DIR       BIT(2) /* reg_dir is unreadable */
>> >> +BGPIOF_BIG_ENDIAN_BYTE_ORDER    BIT(3)
>> >
>> > Right now this documentation is a little bit confusing. Basically
>> > there are two ways to use this driver:
>> >
>> > 1) Name your platform device ""basic-mmio-gpio" or
>> > "basic-mmio-gpio-be", set the right named memory resources to specify
>> > the desired configuration, and let bgpio_pdev_probe() do all the work.
>> >
>> > 2) Allocate a bgpio_chip yourself, call bgpio_init() on it and its
>> > resources, and finally invoke gpiochip_add() yourself.
>> >
>> > These two different ways of doing kind of seem to be mixed together.
>> > Can you try to highlight the fact that these are alternatives?
>>
>
> This is an updated version of previous patch. Please review.
>
> diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
> index 31e0b5d..f6b617a 100644
> --- a/Documentation/gpio/driver.txt
> +++ b/Documentation/gpio/driver.txt
> @@ -190,3 +190,45 @@ gpiochip_free_own_desc().
>  These functions must be used with care since they do not affect module use
>  count. Do not use the functions to request gpio descriptors not owned by the
>  calling driver.
> +
> +
> +Generic driver for memory-mapped GPIO controllers
> +-------------------------------------------------
> +The GPIO generic library provides support for basic platform_device
> +memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
> +symbol GPIO_GENERIC and using library functions provided by GPIO generic
> +driver (see drivers/gpio/gpio-generic.c).
> +
> +The driver supports registers of the sizes of 8/16/32/64 bits and the number
> +of GPIOs are determined by the width of the registers. A set of named memory
> +resources should be defined in the drivers (e.g "dat", "set", "clr", "dirout"
> +and "dirin"), where "dat" is a mandatory resource.
> +
> +Each GPIO controller using GPIO generic library needs to include the following
> +header.
> +        #include <linux/basic_mmio_gpio.h>

Need a blank line to separate the paragraph from the #include.

> +
> +There are two ways to use this driver:
> +1. Using basic GPIO MMIO Generic driver directly:
> +   Name your platform device "basic-mmio-gpio" or "basic-mmio-gpio-be", set the
> +   right named memory resources to specify the desired configuration, and let
> +   bgpio_pdev_probe do all the work.
> +
> +2. Using basic GPIO MMIO Generic library in your driver:
> +   Allocate a bgpio_chip yourself in your GPIO driver, call bgpio_init() on it
> +   and its resources, and finally invoke gpiochip_add yourself. It is possible
> +   to use only parts of the driver, you can overwrite functions and variables
> +   in your driver, if necessary.
> +   You can call bgpio_remove() to unregister a gpio_chip.

It would be great if you could also point to very basic drivers that
use these configurations. Expanations on how to use this are by nature
limited, and having an actual example would be greatly helpful to
users.

> +
> +For setting up GPIO's there are three supported configurations:

s/GPIO's/GPIOs

Also "setting up GPIOs" is quite vague here. Maybe this should become
"To specify the mechanism used to set the GPIOs", or something like
that.

> +- single input/output register resource (named "dat").
> +- set/clear pair (named "set" and "clr").
> +- single output register resource and single input resource ("set" and "dat").
> +
> +For setting the GPIO direction, there are three supported configurations:
> +- simple bidirectional GPIO that requires no configuration.
> +- an output direction register (named "dirout") where a 1 bit indicates the
> +  GPIO is an output.
> +- an input direction register (named "dirin") where a 1 bit indicates the GPIO
> +  is an input.

It is not clear how one can set these configurations from these
paragraphs alone.