2010-08-25 19:42:57

by Anton Vorontsov

[permalink] [raw]
Subject: [PATCH] gpio: Add generic driver for simple memory mapped controllers

The simple memory mapped GPIO controllers may be found in various
onboard FPGAs that are used to control board's switches, LEDs,
chip-selects, Ethernet/USB PHY power, etc.

Usually these controllers do not privide any means of pin setup
(in/out/open drain).

The driver provides:
- Support for 8/16/32/64 bits registers;
- Support for GPIO controllers with clear/set registers;
- Support for GPIO controllers with a single "data" register;
- Support for big endian bits/GPIOs ordering (mostly used on PowerPC).

Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/gpio/Kconfig | 5 +
drivers/gpio/Makefile | 1 +
drivers/gpio/simple_mm_gpio.c | 243 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 249 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/simple_mm_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f623953..a145063 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -82,6 +82,11 @@ config GPIO_PL061
help
Say yes here to support the PrimeCell PL061 GPIO device

+config GPIO_SIMPLE_MM
+ tristate "Simple memory mapped GPIO controllers support"
+ help
+ Say yes here to support simple memory mapped GPIO controllers.
+
config GPIO_XILINX
bool "Xilinx GPIO support"
depends on PPC_OF || MICROBLAZE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a69e060..bfd43bb 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o
obj-$(CONFIG_GPIO_PCA953X) += pca953x.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
obj-$(CONFIG_GPIO_PL061) += pl061.o
+obj-$(CONFIG_GPIO_SIMPLE_MM) += simple_mm_gpio.o
obj-$(CONFIG_GPIO_TC35892) += tc35892-gpio.o
obj-$(CONFIG_GPIO_TIMBERDALE) += timbgpio.o
obj-$(CONFIG_GPIO_TWL4030) += twl4030-gpio.o
diff --git a/drivers/gpio/simple_mm_gpio.c b/drivers/gpio/simple_mm_gpio.c
new file mode 100644
index 0000000..028dce8
--- /dev/null
+++ b/drivers/gpio/simple_mm_gpio.c
@@ -0,0 +1,243 @@
+/*
+ * Simple memory mapped GPIOs
+ *
+ * Copyright 2008 MontaVista Software, Inc.
+ * Copyright 2008,2010 Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/bug.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/compiler.h>
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+
+struct sgpio_chip {
+ struct gpio_chip gc;
+ void __iomem *reg_dat;
+ void __iomem *reg_set;
+ void __iomem *reg_clr;
+ spinlock_t lock;
+
+ int bits;
+ int big_endian_bits;
+
+ /* shadowed data register to clear/set bits safely */
+ unsigned long data;
+};
+
+static struct sgpio_chip *to_sgpio_chip(struct gpio_chip *gc)
+{
+ return container_of(gc, struct sgpio_chip, gc);
+}
+
+static unsigned long sgpio_in(struct sgpio_chip *sgc)
+{
+ switch (sgc->bits) {
+ case 8:
+ return __raw_readb(sgc->reg_dat);
+ case 16:
+ return __raw_readw(sgc->reg_dat);
+ case 32:
+ return __raw_readl(sgc->reg_dat);
+#if BITS_PER_LONG >= 64
+ case 64:
+ return __raw_readq(sgc->reg_dat);
+#endif
+ }
+ BUG();
+}
+
+static void sgpio_out(struct sgpio_chip *sgc, void __iomem *reg,
+ unsigned long data)
+{
+ switch (sgc->bits) {
+ case 8:
+ __raw_writeb(data, reg);
+ return;
+ case 16:
+ __raw_writew(data, reg);
+ return;
+ case 32:
+ __raw_writel(data, reg);
+ return;
+#if BITS_PER_LONG >= 64
+ case 64:
+ __raw_writeq(data, reg);
+ return;
+#endif
+ }
+ BUG();
+}
+
+static unsigned long sgpio_pin2mask(struct sgpio_chip *sgc, unsigned int pin)
+{
+ if (sgc->big_endian_bits)
+ return 1 << (sgc->bits - 1 - pin);
+ else
+ return 1 << pin;
+}
+
+static int sgpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct sgpio_chip *sgc = to_sgpio_chip(gc);
+
+ return sgpio_in(sgc) & sgpio_pin2mask(sgc, gpio);
+}
+
+static void sgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct sgpio_chip *sgc = to_sgpio_chip(gc);
+ unsigned long mask = sgpio_pin2mask(sgc, gpio);
+ unsigned long flags;
+
+ if (sgc->reg_set) {
+ if (val)
+ sgpio_out(sgc, sgc->reg_set, mask);
+ else
+ sgpio_out(sgc, sgc->reg_clr, mask);
+ return;
+ }
+
+ spin_lock_irqsave(&sgc->lock, flags);
+
+ if (val)
+ sgc->data |= mask;
+ else
+ sgc->data &= ~mask;
+
+ sgpio_out(sgc, sgc->reg_dat, sgc->data);
+
+ spin_unlock_irqrestore(&sgc->lock, flags);
+}
+
+static int sgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
+}
+
+static int sgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ sgpio_set(gc, gpio, val);
+ return 0;
+}
+
+static int __devinit sgpio_probe(struct platform_device *pdev)
+{
+ const struct platform_device_id *platid = platform_get_device_id(pdev);
+ struct device *dev = &pdev->dev;
+ struct sgpio_chip *sgc;
+ struct resource *res_dat;
+ struct resource *res_set;
+ struct resource *res_clr;
+ resource_size_t dat_sz;
+ int bits;
+
+ res_dat = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res_dat)
+ return -EINVAL;
+
+ dat_sz = resource_size(res_dat);
+ if (!is_power_of_2(dat_sz))
+ return -EINVAL;
+
+ bits = dat_sz * 8;
+ if (bits > BITS_PER_LONG)
+ return -EINVAL;
+
+ sgc = devm_kzalloc(dev, sizeof(*sgc), GFP_KERNEL);
+ if (!sgc)
+ return -ENOMEM;
+
+ sgc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
+ if (!sgc->reg_dat)
+ return -ENOMEM;
+
+ res_set = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ res_clr = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+ if (res_set && res_clr) {
+ if (resource_size(res_set) != resource_size(res_clr) ||
+ resource_size(res_set) != dat_sz)
+ return -EINVAL;
+
+ sgc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
+ sgc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
+ if (!sgc->reg_set || !sgc->reg_clr)
+ return -ENOMEM;
+ } else if (res_set || res_clr) {
+ return -EINVAL;
+ }
+
+ spin_lock_init(&sgc->lock);
+
+ sgc->bits = bits;
+ sgc->big_endian_bits = !strcmp(platid->name, "simple-mm-gpio-be");
+ sgc->data = sgpio_in(sgc);
+
+ sgc->gc.ngpio = bits;
+ sgc->gc.direction_input = sgpio_dir_in;
+ sgc->gc.direction_output = sgpio_dir_out;
+ sgc->gc.get = sgpio_get;
+ sgc->gc.set = sgpio_set;
+
+ if (pdev->dev.platform_data)
+ sgc->gc.base = (unsigned long)pdev->dev.platform_data;
+ else
+ sgc->gc.base = -1;
+
+ dev_set_drvdata(dev, sgc);
+
+ return gpiochip_add(&sgc->gc);
+}
+
+static int __devexit sgpio_remove(struct platform_device *pdev)
+{
+ struct sgpio_chip *sgc = dev_get_drvdata(&pdev->dev);
+
+ return gpiochip_remove(&sgc->gc);
+}
+
+static const struct platform_device_id sgpio_id_table[] = {
+ { "simple-mm-gpio", },
+ { "simple-mm-gpio-be", },
+ {},
+};
+MODULE_DEVICE_TABLE(platform, sgpio_id_table);
+
+static struct platform_driver sgpio_driver = {
+ .driver = {
+ .name = "simple-mm-gpio",
+ },
+ .id_table = sgpio_id_table,
+ .probe = sgpio_probe,
+ .remove = __devexit_p(sgpio_remove),
+};
+
+static int __init sgpio_init(void)
+{
+ return platform_driver_register(&sgpio_driver);
+}
+module_init(sgpio_init);
+
+static void __exit sgpio_exit(void)
+{
+ platform_driver_unregister(&sgpio_driver);
+}
+module_exit(sgpio_exit);
+
+MODULE_DESCRIPTION("Driver for simple memory mapped GPIO controllers");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.0.5


2010-08-26 05:17:13

by Anton Vorontsov

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers

On Wed, Aug 25, 2010 at 05:11:23PM -0700, David Brownell wrote:
> From: Anton Vorontsov <[email protected]>
> Subject: [PATCH] gpio: Add generic driver for simple memory mapped controllers
>
> NAK.  As you said, it matches the programming of
> certain FPGAs ... so it's NOT "generic".  Rename to match
> the Verilog/VHDL module name or somesuch, if there's
> really much reuse of that module.

I know at least two completely different platforms that
might reuse this driver.

One is PowerPC with ~10 different boards, and another is
an ARM board.

On PowerPC we use arch/powerpc/sysdev/simple_gpio.c, but
the new driver is much more generic, and is aimed to
replace the PowerPC specific one. So it is 'generic' in
this exact sense.

I also think that it is impractical to name the driver
after the particular FPGA IP codename, or even depend
on 'FPGA' word at all (as the GPIOs might be in ASIC
someday).

> There are other FPGA/CPLD GPIO modules, and most of the
> GPIOs in SOCs also match your description, but aren't
> even vaguely compatible with this specific interface.

Well, good. They don't have to use this driver. The driver
is for 'simple' GPIOs, just as the driver name states. I.e.
just a 'data' register or data + set + clr. The name
'simple', IMHO, perfectly matches the purpose of that
driver, no?

If you don't like the word 'generic' in the email subject
(the word does not appear anywhere else), I can remove it,
but I don't see why the name 'simple' doesn't work for
this driver.

Thanks!

--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

2010-08-26 16:22:07

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers


> If you don't like the word 'generic' in the
email subject

Or patch description. It's neither
generic (IP-block-specific) nor simple.


I can remove it,
> but I don't see why the name 'simple'
> doesn't work for > this driver.

It's not simple; I've seen far simpler GPIO
controllers.

Just rename it to match the IP block used.
That removes all confusion. Engineers will
be able to know more easily whether this
driver is even relevant to them.

An example might be "amba-9999 (a made-up
number, likely not matching a GPIO controller).
DesignWare drivers tend to use dw_* prefixes.

- Dave

2010-08-26 16:32:05

by Alan

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers

> Just rename it to match the IP block used.

There are zillions of IP blocks that have that interface, how exactly do
you propose to rename it.

> That removes all confusion. Engineers will
> be able to know more easily whether this
> driver is even relevant to them.

Not unless its got a name about 1000 characters long listing all the
devices it might drive - and changes name weekly.

> An example might be "amba-9999 (a made-up
> number, likely not matching a GPIO controller).

What's wrong with a sensible generic name like gpio-mmio-table ?

Alan

2010-08-26 17:26:10

by Anton Vorontsov

[permalink] [raw]
Subject: [PATCH v2] gpio: Add driver for Anton GPIO controllers

The Anton GPIO controllers are not so complex controllers that may
be found in various on-board FPGAs that are used to control board's
switches, LEDs, chip-selects, Ethernet/USB PHY power, etc.

Usually these controllers do not privide any means of pin setup
(in/out/open drain).

The driver provides:
- Support for 8/16/32/64 bits registers;
- Support for GPIO controllers with clear/set registers;
- Support for GPIO controllers with a single "data" register;
- Support for big endian bits/GPIOs ordering (mostly used on PowerPC).

Signed-off-by: Anton Vorontsov <[email protected]>
---

In v2:

- Hopefully addressed David's comments regarding driver name;
- Now the driver fills gpio_chip.label.

drivers/gpio/Kconfig | 5 +
drivers/gpio/Makefile | 1 +
drivers/gpio/anton_gpio.c | 244 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 250 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/anton_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f623953..4658ca6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -70,6 +70,11 @@ config GPIO_MAX730X

comment "Memory mapped GPIO expanders:"

+config GPIO_ANTON
+ tristate "Anton GPIO controllers support"
+ help
+ Say yes here to support Anton GPIO controllers.
+
config GPIO_IT8761E
tristate "IT8761E GPIO support"
depends on GPIOLIB
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a69e060..a31ddae 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o

obj-$(CONFIG_GPIO_ADP5520) += adp5520-gpio.o
obj-$(CONFIG_GPIO_ADP5588) += adp5588-gpio.o
+obj-$(CONFIG_GPIO_ANTON) += anton_gpio.o
obj-$(CONFIG_GPIO_LANGWELL) += langwell_gpio.o
obj-$(CONFIG_GPIO_MAX730X) += max730x.o
obj-$(CONFIG_GPIO_MAX7300) += max7300.o
diff --git a/drivers/gpio/anton_gpio.c b/drivers/gpio/anton_gpio.c
new file mode 100644
index 0000000..d044385
--- /dev/null
+++ b/drivers/gpio/anton_gpio.c
@@ -0,0 +1,244 @@
+/*
+ * Anton GPIO driver
+ *
+ * Copyright 2008 MontaVista Software, Inc.
+ * Copyright 2008,2010 Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/bug.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/compiler.h>
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+
+struct agpio_chip {
+ struct gpio_chip gc;
+ void __iomem *reg_dat;
+ void __iomem *reg_set;
+ void __iomem *reg_clr;
+ spinlock_t lock;
+
+ int bits;
+ int big_endian_bits;
+
+ /* shadowed data register to clear/set bits safely */
+ unsigned long data;
+};
+
+static struct agpio_chip *to_agpio_chip(struct gpio_chip *gc)
+{
+ return container_of(gc, struct agpio_chip, gc);
+}
+
+static unsigned long agpio_in(struct agpio_chip *agc)
+{
+ switch (agc->bits) {
+ case 8:
+ return __raw_readb(agc->reg_dat);
+ case 16:
+ return __raw_readw(agc->reg_dat);
+ case 32:
+ return __raw_readl(agc->reg_dat);
+#if BITS_PER_LONG >= 64
+ case 64:
+ return __raw_readq(agc->reg_dat);
+#endif
+ }
+ BUG();
+}
+
+static void agpio_out(struct agpio_chip *agc, void __iomem *reg,
+ unsigned long data)
+{
+ switch (agc->bits) {
+ case 8:
+ __raw_writeb(data, reg);
+ return;
+ case 16:
+ __raw_writew(data, reg);
+ return;
+ case 32:
+ __raw_writel(data, reg);
+ return;
+#if BITS_PER_LONG >= 64
+ case 64:
+ __raw_writeq(data, reg);
+ return;
+#endif
+ }
+ BUG();
+}
+
+static unsigned long agpio_pin2mask(struct agpio_chip *agc, unsigned int pin)
+{
+ if (agc->big_endian_bits)
+ return 1 << (agc->bits - 1 - pin);
+ else
+ return 1 << pin;
+}
+
+static int agpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct agpio_chip *agc = to_agpio_chip(gc);
+
+ return agpio_in(agc) & agpio_pin2mask(agc, gpio);
+}
+
+static void agpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct agpio_chip *agc = to_agpio_chip(gc);
+ unsigned long mask = agpio_pin2mask(agc, gpio);
+ unsigned long flags;
+
+ if (agc->reg_set) {
+ if (val)
+ agpio_out(agc, agc->reg_set, mask);
+ else
+ agpio_out(agc, agc->reg_clr, mask);
+ return;
+ }
+
+ spin_lock_irqsave(&agc->lock, flags);
+
+ if (val)
+ agc->data |= mask;
+ else
+ agc->data &= ~mask;
+
+ agpio_out(agc, agc->reg_dat, agc->data);
+
+ spin_unlock_irqrestore(&agc->lock, flags);
+}
+
+static int agpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
+}
+
+static int agpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ agpio_set(gc, gpio, val);
+ return 0;
+}
+
+static int __devinit agpio_probe(struct platform_device *pdev)
+{
+ const struct platform_device_id *platid = platform_get_device_id(pdev);
+ struct device *dev = &pdev->dev;
+ struct agpio_chip *agc;
+ struct resource *res_dat;
+ struct resource *res_set;
+ struct resource *res_clr;
+ resource_size_t dat_sz;
+ int bits;
+
+ res_dat = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res_dat)
+ return -EINVAL;
+
+ dat_sz = resource_size(res_dat);
+ if (!is_power_of_2(dat_sz))
+ return -EINVAL;
+
+ bits = dat_sz * 8;
+ if (bits > BITS_PER_LONG)
+ return -EINVAL;
+
+ agc = devm_kzalloc(dev, sizeof(*agc), GFP_KERNEL);
+ if (!agc)
+ return -ENOMEM;
+
+ agc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
+ if (!agc->reg_dat)
+ return -ENOMEM;
+
+ res_set = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ res_clr = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+ if (res_set && res_clr) {
+ if (resource_size(res_set) != resource_size(res_clr) ||
+ resource_size(res_set) != dat_sz)
+ return -EINVAL;
+
+ agc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
+ agc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
+ if (!agc->reg_set || !agc->reg_clr)
+ return -ENOMEM;
+ } else if (res_set || res_clr) {
+ return -EINVAL;
+ }
+
+ spin_lock_init(&agc->lock);
+
+ agc->bits = bits;
+ agc->big_endian_bits = !strcmp(platid->name, "anton-gpio-be");
+ agc->data = agpio_in(agc);
+
+ agc->gc.ngpio = bits;
+ agc->gc.direction_input = agpio_dir_in;
+ agc->gc.direction_output = agpio_dir_out;
+ agc->gc.get = agpio_get;
+ agc->gc.set = agpio_set;
+ agc->gc.label = dev_name(dev);
+
+ if (dev->platform_data)
+ agc->gc.base = (unsigned long)dev->platform_data;
+ else
+ agc->gc.base = -1;
+
+ dev_set_drvdata(dev, agc);
+
+ return gpiochip_add(&agc->gc);
+}
+
+static int __devexit agpio_remove(struct platform_device *pdev)
+{
+ struct agpio_chip *agc = dev_get_drvdata(&pdev->dev);
+
+ return gpiochip_remove(&agc->gc);
+}
+
+static const struct platform_device_id agpio_id_table[] = {
+ { "anton-gpio", },
+ { "anton-gpio-be", },
+ {},
+};
+MODULE_DEVICE_TABLE(platform, agpio_id_table);
+
+static struct platform_driver agpio_driver = {
+ .driver = {
+ .name = "anton-gpio",
+ },
+ .id_table = agpio_id_table,
+ .probe = agpio_probe,
+ .remove = __devexit_p(agpio_remove),
+};
+
+static int __init agpio_init(void)
+{
+ return platform_driver_register(&agpio_driver);
+}
+module_init(agpio_init);
+
+static void __exit agpio_exit(void)
+{
+ platform_driver_unregister(&agpio_driver);
+}
+module_exit(agpio_exit);
+
+MODULE_DESCRIPTION("Driver for Anton GPIO controllers");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.0.5

2010-08-26 17:35:05

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers



--- On Thu, 8/26/10, Alan Cox <[email protected]> wrote:

> > Just rename it to match the IP block used.
>
> There are zillions of IP blocks that have that interface,

If there are "zillions" that suggests the HW
engineers have version/naming issues just like
certain software engineers. Only goes to show
how close Verilog and VHDL are to software! :)


> how exactly do
> you propose to rename it.

My suggestion was to use the name provided/used
by the hardware engineers. (E.g. whatever the
Verilog or VHDL equivalent of a module name is.)

So -- NO RENAME -- except for the driver.

If I understand you correctly, those engineers
are not reusing a named module; they are at best
just copying/pasting some Verilog/VHDL and adding
ASIC/SoC/.../FPGA-specific hacks. (Which calls into
question just how much assurance there can be that
one driver will work reliably for all instances...


> > That removes all confusion.? Engineers will
> >? be able to know more easily whether this
> > driver is even relevant to them.
>
> Not unless its got a name about 1000 characters long
> listing all the
> devices it might drive - and changes name weekly.

Someone should have a few words with those hardware
engineers about module naming and consistency, so
software doesn't need to work around such issues.

Such problems have been solved for decades, and
there's no reason to create confusion higher up
the system stack. (or force workarounds.)


> > An example might be "amba-9999 (a made-up
> > number, likely not matching a GPIO controller).

Note that such names are already used in Linux
with IP blocks from AMBA; Designware blocks, as
I noted, aren't dissimilar (dw_foo.c in at least
a few cases).

> What's wrong with a sensible generic name like
> gpio-mmio-table ?


That's a much better name. I'ts got technical
content, even! (vs "simple" being "marketing",
including easy confusion with other entities).

All I'll say is that it might
be *too* generic a name. I know I can come up
with current examples of GPIO done via MMIO
(tables) that this driver won't fit at all.
That can probably be worked around via docs and
conventions.

There'd need to be good docs on what this
gpio-mmio-table interface expects. Some of that
belongs in Kconfig, not much; mostly I'd think
it belongs in driver header comments, but also
some in the patch comment itself. I hear you
strongly implying that there's no generic hardware
doc to reference. (As theree would be if this
block came from the AMBA or DesignWare families.)



2010-08-26 17:57:39

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers



--- On Thu, 8/26/10, Anton Vorontsov <[email protected]> wrote:
>
> - Hopefully addressed David's comments
> regarding driver name;

I'm not sure about the precedent of putting
developer names in source.c and driver names...
start that, and everyone will want to follow!!

OK, so Linus started that one, arguably... :)

Not that I want to be naming this, but how
about a tweak to Alan's suggestion, keeping
the name technical?

"gpio_basic_mmio.c" ... or similar?

2010-08-26 18:36:28

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers

On Thu, Aug 26, 2010 at 10:34:58AM -0700, David Brownell wrote:
> --- On Thu, 8/26/10, Alan Cox <[email protected]> wrote:

> > > Just rename it to match the IP block used.

> > There are zillions of IP blocks that have that interface,

> If there are "zillions" that suggests the HW
> engineers have version/naming issues just like
> certain software engineers. Only goes to show
> how close Verilog and VHDL are to software! :)

Half the thing here is that it's barely IP - it's the sort of thing
that's so trivial to implement that it'd take more time to locate a
suitable IP than to just hook up the output pins to the register map
directly.

> > how exactly do
> > you propose to rename it.

> My suggestion was to use the name provided/used
> by the hardware engineers. (E.g. whatever the
> Verilog or VHDL equivalent of a module name is.)

That's not going to happen, nobody's sharing any IP for this.

> If I understand you correctly, those engineers
> are not reusing a named module; they are at best
> just copying/pasting some Verilog/VHDL and adding
> ASIC/SoC/.../FPGA-specific hacks. (Which calls into
> question just how much assurance there can be that
> one driver will work reliably for all instances...

Essentially all this sort of GPIO controller is is a straight wire
through of a set of register bits to an output signal (especially if you
don't have a separate clear register). It's the first thing a hardware
engineer would think of for implementing this - it's not even copy and
paste really, my understanding is that normally it's just basic plumbing
to wire the relevant signals together.

2010-08-26 18:38:52

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers

On Wed, Aug 25, 2010 at 11:42:49PM +0400, Anton Vorontsov wrote:

> + res_dat = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res_dat)
> + return -EINVAL;

I'd suggest naming these resources rather than grabbing by number,
that'll make it easier to plumb the machine driver side in since you
don't have to remember which number corresponds to which function.

2010-08-26 20:51:37

by Alan

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers

O> If there are "zillions" that suggests the HW
> engineers have version/naming issues just like
> certain software engineers. Only goes to show
> how close Verilog and VHDL are to software! :)

Verilog ? Its far far more primitive than that. You'll find exactly that
arrangement in prehistoric 8bit micro support chips - eg the 6522. In
fact the only reason I can see for not using it on primitive 8bit micro
devices is that they *already* had more features.

> are not reusing a named module; they are at best
> just copying/pasting some Verilog/VHDL and adding
> ASIC/SoC/.../FPGA-specific hacks. (Which calls into
> question just how much assurance there can be that
> one driver will work reliably for all instances...

No they are implementing trivial common sense logic. It's a bit like
complaining we have multiple drivers that use addition.

I think you need a reality check. Its a VLSI undergraduate project level
device, or was back when they taught undergraduates a sampling of chip
design by drawing the transistors. This is "my first logic design" stuff.

Alan

2010-08-26 21:20:32

by Anton Vorontsov

[permalink] [raw]
Subject: Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers

On Thu, Aug 26, 2010 at 10:57:33AM -0700, David Brownell wrote:
[...]
> Not that I want to be naming this, but how
> about a tweak to Alan's suggestion, keeping
> the name technical?
>
> "gpio_basic_mmio.c" ... or similar?

I would agree yesterday, but now I tend to like
anton_gpio.c. :-D

You seemed to be fine with meaningless amba-9999
or dw_abc names, but now, when I propose such a
marvellous name, you seem to lean to a synonym of
an old one, the one that I used in v1? Wouldn't
it be a step backwards? :-)

Just kiding, of course. basic_mmio.c works for me.

I also think it makes sense to drop gpio_ prefix,
as the driver is already in gpio/ directory.

Thanks!

--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

2010-08-26 22:48:35

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers



--- On Thu, 8/26/10, Anton Vorontsov <[email protected]> wrote:

> From: Anton Vorontsov <[email protected]>
> Subject: Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers
> To: "David Brownell" <[email protected]>
> Cc: "Andrew Morton" <[email protected]>, "Samuel Ortiz" <[email protected]>, "Mark Brown" <[email protected]>, "David Brownell" <[email protected]>, "Alan Cox" <[email protected]>, [email protected]
> Date: Thursday, August 26, 2010, 2:20 PM
> On Thu, Aug 26, 2010 at 10:57:33AM
> -0700, David Brownell wrote:
> [...]
>
> You seemed to be fine with meaningless amba-9999
> or dw_abc names, but

NOT meaningless ... when there's actual IP behind
the controller! Which, at first, is what I thought
you were talking about.


> Just kiding, of course. basic_mmio.c works for me.

OK, how about resubmitting? With that name?

Be sure to add a bit of documentation about
the controller requirements ... basically that
it has a register from which all values can be
read, and that either that register can be written,
or there's a pair of set-bit/clear-bit registers
affecting that register and the output pins.

Might be worth mentioning how trivial it is to do
that in hardware like CPLDs/FPGAS/etc, which is
why this handles different word sizes/endianness.
and the expectation that in at least some cases
this will be used with roll-your-own ASIC/FPGA
logic in Verilog or VHDL (yes?).

> I also think it makes sense to drop gpio_ prefix,
> as the driver is already in gpio/ directory.

No, keep it; files move around sometimes, and it's
best if that loses no information.

2010-08-26 22:58:54

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers



--- On Thu, 8/26/10, Alan Cox <[email protected]> wrote:
(Which calls
> into addition.
>
> I think you need a reality check.

I know you don't say that every time someone makes
different assumptions about context than you have.
So for now I guess I'll just feel special...

And I expect you'd have to admit that if indeed there
were some IP reuse happening, then everything I said
which assumed reuse would be true. Likewise, that
such reuse does happen even for simple stuff like
GPIOs (to facilitate reuse of program code ... even
on the 8-bit micros you mentioned, I've seen it).

2010-08-27 00:00:19

by Alan

[permalink] [raw]
Subject: Re: [PATCH] gpio: Add generic driver for simple memory mapped controllers

On Thu, 26 Aug 2010 15:58:51 -0700 (PDT)
David Brownell <[email protected]> wrote:

>
>
> --- On Thu, 8/26/10, Alan Cox <[email protected]> wrote:
> (Which calls
> > into addition.
> >
> > I think you need a reality check.
>
> I know you don't say that every time someone makes
> different assumptions about context than you have.

I don't think its assumptions. In this case it's a case of actually
looking at what these devices are in the real world. Hence 'reality
check'.

Sure there are plenty of 're-usable' components that appear everywhere -
Designware etch turns up in everything but this isn't one of them.

2010-08-27 15:57:58

by Anton Vorontsov

[permalink] [raw]
Subject: [PATCH v3] gpio: Add driver for basic memory-mapped GPIO controllers

The basic GPIO controllers may be found in various on-board FPGA
and ASIC solutions that are used to control board's switches, LEDs,
chip-selects, Ethernet/USB PHY power, etc.

Usually these controllers do not privide any means of pin setup
(in/out/open drain).

The driver provides:
- Support for 8/16/32/64 bits registers;
- Support for GPIO controllers with clear/set registers;
- Support for GPIO controllers with a single "data" register;
- Support for big endian bits/GPIOs ordering (mostly used on PowerPC).

Signed-off-by: Anton Vorontsov <[email protected]>
---

On Thu, Aug 26, 2010 at 03:48:31PM -0700, David Brownell wrote:
[...]
> > Just kiding, of course. basic_mmio.c works for me.
>
> OK, how about resubmitting? With that name?

Sure thing.

> Be sure to add a bit of documentation about
> the controller requirements ... basically that
> it has a register from which all values can be
> read, and that either that register can be written,
> or there's a pair of set-bit/clear-bit registers
> affecting that register and the output pins.
>
> Might be worth mentioning how trivial it is to do
> that in hardware like CPLDs/FPGAS/etc, which is
> why this handles different word sizes/endianness.
> and the expectation that in at least some cases
> this will be used with roll-your-own ASIC/FPGA
> logic in Verilog or VHDL (yes?).

Yep. Done.

> > I also think it makes sense to drop gpio_ prefix,
> > as the driver is already in gpio/ directory.
>
> No, keep it; files move around sometimes, and it's
> best if that loses no information.

OK.

Also addressed Mark's comments wrt getting resources
by name. Plus, it appears that there's also gc.dev field
in gpio_chip, fill it too.

I hope it's perfect now. :-)

Thanks!

drivers/gpio/Kconfig | 5 +
drivers/gpio/Makefile | 1 +
drivers/gpio/basic_mmio_gpio.c | 278 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 284 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/basic_mmio_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f623953..d1d1c24 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -70,6 +70,11 @@ config GPIO_MAX730X

comment "Memory mapped GPIO expanders:"

+config GPIO_BASIC_MMIO
+ tristate "Basic memory-mapped GPIO controllers support"
+ help
+ Say yes here to support basic memory-mapped GPIO controllers.
+
config GPIO_IT8761E
tristate "IT8761E GPIO support"
depends on GPIOLIB
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a69e060..dfb571b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o

obj-$(CONFIG_GPIO_ADP5520) += adp5520-gpio.o
obj-$(CONFIG_GPIO_ADP5588) += adp5588-gpio.o
+obj-$(CONFIG_GPIO_BASIC_MMIO) += basic_mmio_gpio.o
obj-$(CONFIG_GPIO_LANGWELL) += langwell_gpio.o
obj-$(CONFIG_GPIO_MAX730X) += max730x.o
obj-$(CONFIG_GPIO_MAX7300) += max7300.o
diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
new file mode 100644
index 0000000..6526959
--- /dev/null
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -0,0 +1,278 @@
+/*
+ * Driver for basic memory-mapped GPIO controllers.
+ *
+ * Copyright 2008 MontaVista Software, Inc.
+ * Copyright 2008,2010 Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * ....``.```~~~~````.`.`.`.`.```````'',,,.........`````......`.......
+ * ...`` ```````..
+ * ..The simplest form of a GPIO controller that the driver supports is``
+ * `.just a single "data" register, where GPIO state can be read and/or `
+ * ..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
+ * ```````
+ * __/~~|___/~| . ```~~~~~~ ,~.`.`.`.`````.~~...,,,,...
+ * |___________|~$@~~~ %~~ .. Implementing such a GPIO .
+ * o ` . ` controller in FPGA is,`.
+ * `....trivial. '~`.```.````
+ * ```````
+ * .```````~~~~`..`.``.``.
+ * . The driver supports `... ,..```.`~~~```````````````....````.``,,.
+ * . big-endian notation, just`. ... A bit more sophisticated controllers ,
+ * . register the device with -be`. . with a pair of set/clear-bit registers ,
+ * `.. suffix. ```~~`````....`.` . affecting the data register and the .`
+ * ``.`.``...``` ```.. output pins are also supported.`
+ * ^^ `````.`````````.,``~``~``~~``````
+ * . ^^
+ * ,..`.`.`...````````````......`.`.`.`.`.`..`.`.`..
+ * .. The expectation is that in at least some cases . ,-~~~-,
+ * .this will be used with roll-your-own ASIC/FPGA .` \ /
+ * .logic in Verilog or VHDL. ~~~`````````..`````~~` \ /
+ * ..````````......``````````` \o_
+ * |
+ * ^^ / \
+ *
+ * ...`````~~`.....``.`..........``````.`.``.```........``.
+ * ` 8, 16, 32 and 64 bits registers are supported, and``.
+ * . the number of GPIOs is determined by the width of ~
+ * .. the registers. ,............```.`.`..`.`.~~~.`.`.`~
+ * `.......````.```
+ */
+
+#include <linux/init.h>
+#include <linux/bug.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/compiler.h>
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+
+struct bgpio_chip {
+ struct gpio_chip gc;
+ void __iomem *reg_dat;
+ void __iomem *reg_set;
+ void __iomem *reg_clr;
+ spinlock_t lock;
+
+ int bits;
+ int big_endian_bits;
+
+ /* shadowed data register to clear/set bits safely */
+ unsigned long data;
+};
+
+static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
+{
+ return container_of(gc, struct bgpio_chip, gc);
+}
+
+static unsigned long bgpio_in(struct bgpio_chip *bgc)
+{
+ switch (bgc->bits) {
+ case 8:
+ return __raw_readb(bgc->reg_dat);
+ case 16:
+ return __raw_readw(bgc->reg_dat);
+ case 32:
+ return __raw_readl(bgc->reg_dat);
+#if BITS_PER_LONG >= 64
+ case 64:
+ return __raw_readq(bgc->reg_dat);
+#endif
+ }
+ BUG();
+}
+
+static void bgpio_out(struct bgpio_chip *bgc, void __iomem *reg,
+ unsigned long data)
+{
+ switch (bgc->bits) {
+ case 8:
+ __raw_writeb(data, reg);
+ return;
+ case 16:
+ __raw_writew(data, reg);
+ return;
+ case 32:
+ __raw_writel(data, reg);
+ return;
+#if BITS_PER_LONG >= 64
+ case 64:
+ __raw_writeq(data, reg);
+ return;
+#endif
+ }
+ BUG();
+}
+
+static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
+{
+ if (bgc->big_endian_bits)
+ return 1 << (bgc->bits - 1 - pin);
+ else
+ return 1 << pin;
+}
+
+static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+ return bgpio_in(bgc) & bgpio_pin2mask(bgc, gpio);
+}
+
+static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+ unsigned long mask = bgpio_pin2mask(bgc, gpio);
+ unsigned long flags;
+
+ if (bgc->reg_set) {
+ if (val)
+ bgpio_out(bgc, bgc->reg_set, mask);
+ else
+ bgpio_out(bgc, bgc->reg_clr, mask);
+ return;
+ }
+
+ spin_lock_irqsave(&bgc->lock, flags);
+
+ if (val)
+ bgc->data |= mask;
+ else
+ bgc->data &= ~mask;
+
+ bgpio_out(bgc, bgc->reg_dat, bgc->data);
+
+ spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
+}
+
+static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ bgpio_set(gc, gpio, val);
+ return 0;
+}
+
+static int __devinit bgpio_probe(struct platform_device *pdev)
+{
+ const struct platform_device_id *platid = platform_get_device_id(pdev);
+ struct device *dev = &pdev->dev;
+ struct bgpio_chip *bgc;
+ struct resource *res_dat;
+ struct resource *res_set;
+ struct resource *res_clr;
+ resource_size_t dat_sz;
+ int bits;
+
+ res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
+ if (!res_dat)
+ return -EINVAL;
+
+ dat_sz = resource_size(res_dat);
+ if (!is_power_of_2(dat_sz))
+ return -EINVAL;
+
+ bits = dat_sz * 8;
+ if (bits > BITS_PER_LONG)
+ return -EINVAL;
+
+ bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
+ if (!bgc)
+ return -ENOMEM;
+
+ bgc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
+ if (!bgc->reg_dat)
+ return -ENOMEM;
+
+ res_set = platform_get_resource_byname(pdev, IORESOURCE_MEM, "set");
+ res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
+ if (res_set && res_clr) {
+ if (resource_size(res_set) != resource_size(res_clr) ||
+ resource_size(res_set) != dat_sz)
+ return -EINVAL;
+
+ bgc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
+ bgc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
+ if (!bgc->reg_set || !bgc->reg_clr)
+ return -ENOMEM;
+ } else if (res_set || res_clr) {
+ return -EINVAL;
+ }
+
+ spin_lock_init(&bgc->lock);
+
+ bgc->bits = bits;
+ bgc->big_endian_bits = !strcmp(platid->name, "basic-mmio-gpio-be");
+ bgc->data = bgpio_in(bgc);
+
+ bgc->gc.ngpio = bits;
+ bgc->gc.direction_input = bgpio_dir_in;
+ bgc->gc.direction_output = bgpio_dir_out;
+ bgc->gc.get = bgpio_get;
+ bgc->gc.set = bgpio_set;
+ bgc->gc.dev = dev;
+ bgc->gc.label = dev_name(dev);
+
+ if (dev->platform_data)
+ bgc->gc.base = (unsigned long)dev->platform_data;
+ else
+ bgc->gc.base = -1;
+
+ dev_set_drvdata(dev, bgc);
+
+ return gpiochip_add(&bgc->gc);
+}
+
+static int __devexit bgpio_remove(struct platform_device *pdev)
+{
+ struct bgpio_chip *bgc = dev_get_drvdata(&pdev->dev);
+
+ return gpiochip_remove(&bgc->gc);
+}
+
+static const struct platform_device_id bgpio_id_table[] = {
+ { "basic-mmio-gpio", },
+ { "basic-mmio-gpio-be", },
+ {},
+};
+MODULE_DEVICE_TABLE(platform, bgpio_id_table);
+
+static struct platform_driver bgpio_driver = {
+ .driver = {
+ .name = "basic-mmio-gpio",
+ },
+ .id_table = bgpio_id_table,
+ .probe = bgpio_probe,
+ .remove = __devexit_p(bgpio_remove),
+};
+
+static int __init bgpio_init(void)
+{
+ return platform_driver_register(&bgpio_driver);
+}
+module_init(bgpio_init);
+
+static void __exit bgpio_exit(void)
+{
+ platform_driver_unregister(&bgpio_driver);
+}
+module_exit(bgpio_exit);
+
+MODULE_DESCRIPTION("Driver for basic memory-mapped GPIO controllers");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.0.5

2010-08-28 19:08:21

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v3] gpio: Add driver for basic memory-mapped GPIO controllers

Much better, thanks!

But do it without BUG()... assume that all
uses of BUG() are errors, unless the OS
really has *NO* way to continue operating.
In this case, it can clearly limp along.


- Dave

2010-08-29 21:29:07

by Anton Vorontsov

[permalink] [raw]
Subject: [PATCH v4] gpio: Add driver for basic memory-mapped GPIO controllers

The basic GPIO controllers may be found in various on-board FPGA
and ASIC solutions that are used to control board's switches, LEDs,
chip-selects, Ethernet/USB PHY power, etc.

Usually these controllers do not privide any means of pin setup
(in/out/open drain).

The driver provides:
- Support for 8/16/32/64 bits registers;
- Support for GPIO controllers with clear/set registers;
- Support for GPIO controllers with a single "data" register;
- Support for big endian bits/GPIOs ordering (mostly used on PowerPC).

Signed-off-by: Anton Vorontsov <[email protected]>
---

On Sat, Aug 28, 2010 at 12:08:20PM -0700, David Brownell wrote:
> But do it without BUG()... assume that all
> uses of BUG() are errors, unless the OS
> really has *NO* way to continue operating.
> In this case, it can clearly limp along.

Yeah, good idea. Let's turn these BUG() into WARN_ON_ONCE()?

Thanks!

drivers/gpio/Kconfig | 5 +
drivers/gpio/Makefile | 1 +
drivers/gpio/basic_mmio_gpio.c | 280 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 286 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/basic_mmio_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f623953..d1d1c24 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -70,6 +70,11 @@ config GPIO_MAX730X

comment "Memory mapped GPIO expanders:"

+config GPIO_BASIC_MMIO
+ tristate "Basic memory-mapped GPIO controllers support"
+ help
+ Say yes here to support basic memory-mapped GPIO controllers.
+
config GPIO_IT8761E
tristate "IT8761E GPIO support"
depends on GPIOLIB
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a69e060..dfb571b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o

obj-$(CONFIG_GPIO_ADP5520) += adp5520-gpio.o
obj-$(CONFIG_GPIO_ADP5588) += adp5588-gpio.o
+obj-$(CONFIG_GPIO_BASIC_MMIO) += basic_mmio_gpio.o
obj-$(CONFIG_GPIO_LANGWELL) += langwell_gpio.o
obj-$(CONFIG_GPIO_MAX730X) += max730x.o
obj-$(CONFIG_GPIO_MAX7300) += max7300.o
diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
new file mode 100644
index 0000000..7117f1d
--- /dev/null
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -0,0 +1,280 @@
+/*
+ * Driver for basic memory-mapped GPIO controllers.
+ *
+ * Copyright 2008 MontaVista Software, Inc.
+ * Copyright 2008,2010 Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * ....``.```~~~~````.`.`.`.`.```````'',,,.........`````......`.......
+ * ...`` ```````..
+ * ..The simplest form of a GPIO controller that the driver supports is``
+ * `.just a single "data" register, where GPIO state can be read and/or `
+ * `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
+ * `````````
+ ___
+_/~~|___/~| . ```~~~~~~ ___/___\___ ,~.`.`.`.`````.~~...,,,,...
+__________|~$@~~~ %~ /o*o*o*o*o*o\ .. Implementing such a GPIO .
+o ` ~~~~\___/~~~~ ` controller in FPGA is ,.`
+ `....trivial..'~`.```.```
+ * ```````
+ * .```````~~~~`..`.``.``.
+ * . The driver supports `... ,..```.`~~~```````````````....````.``,,
+ * . big-endian notation, just`. .. A bit more sophisticated controllers ,
+ * . register the device with -be`. .with a pair of set/clear-bit registers ,
+ * `.. suffix. ```~~`````....`.` . affecting the data register and the .`
+ * ``.`.``...``` ```.. output pins are also supported.`
+ * ^^ `````.`````````.,``~``~``~~``````
+ * . ^^
+ * ,..`.`.`...````````````......`.`.`.`.`.`..`.`.`..
+ * .. The expectation is that in at least some cases . ,-~~~-,
+ * .this will be used with roll-your-own ASIC/FPGA .` \ /
+ * .logic in Verilog or VHDL. ~~~`````````..`````~~` \ /
+ * ..````````......``````````` \o_
+ * |
+ * ^^ / \
+ *
+ * ...`````~~`.....``.`..........``````.`.``.```........``.
+ * ` 8, 16, 32 and 64 bits registers are supported, and``.
+ * . the number of GPIOs is determined by the width of ~
+ * .. the registers. ,............```.`.`..`.`.~~~.`.`.`~
+ * `.......````.```
+ */
+
+#include <linux/init.h>
+#include <linux/bug.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/compiler.h>
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+
+struct bgpio_chip {
+ struct gpio_chip gc;
+ void __iomem *reg_dat;
+ void __iomem *reg_set;
+ void __iomem *reg_clr;
+ spinlock_t lock;
+
+ int bits;
+ int big_endian_bits;
+
+ /* shadowed data register to clear/set bits safely */
+ unsigned long data;
+};
+
+static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
+{
+ return container_of(gc, struct bgpio_chip, gc);
+}
+
+static unsigned long bgpio_in(struct bgpio_chip *bgc)
+{
+ switch (bgc->bits) {
+ case 8:
+ return __raw_readb(bgc->reg_dat);
+ case 16:
+ return __raw_readw(bgc->reg_dat);
+ case 32:
+ return __raw_readl(bgc->reg_dat);
+#if BITS_PER_LONG >= 64
+ case 64:
+ return __raw_readq(bgc->reg_dat);
+#endif
+ }
+ WARN_ON_ONCE(1);
+ return 0;
+}
+
+static void bgpio_out(struct bgpio_chip *bgc, void __iomem *reg,
+ unsigned long data)
+{
+ switch (bgc->bits) {
+ case 8:
+ __raw_writeb(data, reg);
+ return;
+ case 16:
+ __raw_writew(data, reg);
+ return;
+ case 32:
+ __raw_writel(data, reg);
+ return;
+#if BITS_PER_LONG >= 64
+ case 64:
+ __raw_writeq(data, reg);
+ return;
+#endif
+ }
+ WARN_ON_ONCE(1);
+}
+
+static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
+{
+ if (bgc->big_endian_bits)
+ return 1 << (bgc->bits - 1 - pin);
+ else
+ return 1 << pin;
+}
+
+static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+ return bgpio_in(bgc) & bgpio_pin2mask(bgc, gpio);
+}
+
+static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+ unsigned long mask = bgpio_pin2mask(bgc, gpio);
+ unsigned long flags;
+
+ if (bgc->reg_set) {
+ if (val)
+ bgpio_out(bgc, bgc->reg_set, mask);
+ else
+ bgpio_out(bgc, bgc->reg_clr, mask);
+ return;
+ }
+
+ spin_lock_irqsave(&bgc->lock, flags);
+
+ if (val)
+ bgc->data |= mask;
+ else
+ bgc->data &= ~mask;
+
+ bgpio_out(bgc, bgc->reg_dat, bgc->data);
+
+ spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
+}
+
+static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ bgpio_set(gc, gpio, val);
+ return 0;
+}
+
+static int __devinit bgpio_probe(struct platform_device *pdev)
+{
+ const struct platform_device_id *platid = platform_get_device_id(pdev);
+ struct device *dev = &pdev->dev;
+ struct bgpio_chip *bgc;
+ struct resource *res_dat;
+ struct resource *res_set;
+ struct resource *res_clr;
+ resource_size_t dat_sz;
+ int bits;
+
+ res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
+ if (!res_dat)
+ return -EINVAL;
+
+ dat_sz = resource_size(res_dat);
+ if (!is_power_of_2(dat_sz))
+ return -EINVAL;
+
+ bits = dat_sz * 8;
+ if (bits > BITS_PER_LONG)
+ return -EINVAL;
+
+ bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
+ if (!bgc)
+ return -ENOMEM;
+
+ bgc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
+ if (!bgc->reg_dat)
+ return -ENOMEM;
+
+ res_set = platform_get_resource_byname(pdev, IORESOURCE_MEM, "set");
+ res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
+ if (res_set && res_clr) {
+ if (resource_size(res_set) != resource_size(res_clr) ||
+ resource_size(res_set) != dat_sz)
+ return -EINVAL;
+
+ bgc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
+ bgc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
+ if (!bgc->reg_set || !bgc->reg_clr)
+ return -ENOMEM;
+ } else if (res_set || res_clr) {
+ return -EINVAL;
+ }
+
+ spin_lock_init(&bgc->lock);
+
+ bgc->bits = bits;
+ bgc->big_endian_bits = !strcmp(platid->name, "basic-mmio-gpio-be");
+ bgc->data = bgpio_in(bgc);
+
+ bgc->gc.ngpio = bits;
+ bgc->gc.direction_input = bgpio_dir_in;
+ bgc->gc.direction_output = bgpio_dir_out;
+ bgc->gc.get = bgpio_get;
+ bgc->gc.set = bgpio_set;
+ bgc->gc.dev = dev;
+ bgc->gc.label = dev_name(dev);
+
+ if (dev->platform_data)
+ bgc->gc.base = (unsigned long)dev->platform_data;
+ else
+ bgc->gc.base = -1;
+
+ dev_set_drvdata(dev, bgc);
+
+ return gpiochip_add(&bgc->gc);
+}
+
+static int __devexit bgpio_remove(struct platform_device *pdev)
+{
+ struct bgpio_chip *bgc = dev_get_drvdata(&pdev->dev);
+
+ return gpiochip_remove(&bgc->gc);
+}
+
+static const struct platform_device_id bgpio_id_table[] = {
+ { "basic-mmio-gpio", },
+ { "basic-mmio-gpio-be", },
+ {},
+};
+MODULE_DEVICE_TABLE(platform, bgpio_id_table);
+
+static struct platform_driver bgpio_driver = {
+ .driver = {
+ .name = "basic-mmio-gpio",
+ },
+ .id_table = bgpio_id_table,
+ .probe = bgpio_probe,
+ .remove = __devexit_p(bgpio_remove),
+};
+
+static int __init bgpio_init(void)
+{
+ return platform_driver_register(&bgpio_driver);
+}
+module_init(bgpio_init);
+
+static void __exit bgpio_exit(void)
+{
+ platform_driver_unregister(&bgpio_driver);
+}
+module_exit(bgpio_exit);
+
+MODULE_DESCRIPTION("Driver for basic memory-mapped GPIO controllers");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.0.5

2010-08-30 20:23:20

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v4] gpio: Add driver for basic memory-mapped GPIO controllers



--- On Sun, 8/29/10, Anton Vorontsov <[email protected]> wrote:
Brownell
> wrote:
> > But? do it without BUG()...
> Yeah, good idea. Let's turn these BUG() into
> WARN_ON_ONCE()?

I'm not a big fan of redundant -- and
noisy! error reports like
that: recall that errors were already reported
(and ignored) on those paths, indicating invalid
GPIOs (requested, or direction set, etc).

The read paths should probably just return zero;
that's required in some cases, if you read the
interface spec.

2010-08-31 17:59:11

by Anton Vorontsov

[permalink] [raw]
Subject: [PATCH v5] gpio: Add driver for basic memory-mapped GPIO controllers

The basic GPIO controllers may be found in various on-board FPGA
and ASIC solutions that are used to control board's switches, LEDs,
chip-selects, Ethernet/USB PHY power, etc.

Usually these controllers do not privide any means of pin setup
(in/out/open drain).

The driver provides:
- Support for 8/16/32/64 bits registers;
- Support for GPIO controllers with clear/set registers;
- Support for GPIO controllers with a single "data" register;
- Support for big endian bits/GPIOs ordering (mostly used on PowerPC).

Signed-off-by: Anton Vorontsov <[email protected]>
---

On Mon, Aug 30, 2010 at 01:23:18PM -0700, David Brownell wrote:
> --- On Sun, 8/29/10, Anton Vorontsov <[email protected]> wrote:
> > > But  do it without BUG()...
> > Yeah, good idea. Let's turn these BUG() into
> > WARN_ON_ONCE()?
>
> I'm not a big fan of redundant -- and
> noisy! error reports like
> that: recall that errors were already reported
> (and ignored) on those paths, indicating invalid
> GPIOs (requested, or direction set, etc).

OK, here's another version. Looks good?

Thanks,

drivers/gpio/Kconfig | 5 +
drivers/gpio/Makefile | 1 +
drivers/gpio/basic_mmio_gpio.c | 278 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 284 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/basic_mmio_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f623953..d1d1c24 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -70,6 +70,11 @@ config GPIO_MAX730X

comment "Memory mapped GPIO expanders:"

+config GPIO_BASIC_MMIO
+ tristate "Basic memory-mapped GPIO controllers support"
+ help
+ Say yes here to support basic memory-mapped GPIO controllers.
+
config GPIO_IT8761E
tristate "IT8761E GPIO support"
depends on GPIOLIB
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a69e060..dfb571b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o

obj-$(CONFIG_GPIO_ADP5520) += adp5520-gpio.o
obj-$(CONFIG_GPIO_ADP5588) += adp5588-gpio.o
+obj-$(CONFIG_GPIO_BASIC_MMIO) += basic_mmio_gpio.o
obj-$(CONFIG_GPIO_LANGWELL) += langwell_gpio.o
obj-$(CONFIG_GPIO_MAX730X) += max730x.o
obj-$(CONFIG_GPIO_MAX7300) += max7300.o
diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
new file mode 100644
index 0000000..ff5bc06
--- /dev/null
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -0,0 +1,278 @@
+/*
+ * Driver for basic memory-mapped GPIO controllers.
+ *
+ * Copyright 2008 MontaVista Software, Inc.
+ * Copyright 2008,2010 Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * ....``.```~~~~````.`.`.`.`.```````'',,,.........`````......`.......
+ * ...`` ```````..
+ * ..The simplest form of a GPIO controller that the driver supports is``
+ * `.just a single "data" register, where GPIO state can be read and/or `
+ * `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
+ * `````````
+ ___
+_/~~|___/~| . ```~~~~~~ ___/___\___ ,~.`.`.`.`````.~~...,,,,...
+__________|~$@~~~ %~ /o*o*o*o*o*o\ .. Implementing such a GPIO .
+o ` ~~~~\___/~~~~ ` controller in FPGA is ,.`
+ `....trivial..'~`.```.```
+ * ```````
+ * .```````~~~~`..`.``.``.
+ * . The driver supports `... ,..```.`~~~```````````````....````.``,,
+ * . big-endian notation, just`. .. A bit more sophisticated controllers ,
+ * . register the device with -be`. .with a pair of set/clear-bit registers ,
+ * `.. suffix. ```~~`````....`.` . affecting the data register and the .`
+ * ``.`.``...``` ```.. output pins are also supported.`
+ * ^^ `````.`````````.,``~``~``~~``````
+ * . ^^
+ * ,..`.`.`...````````````......`.`.`.`.`.`..`.`.`..
+ * .. The expectation is that in at least some cases . ,-~~~-,
+ * .this will be used with roll-your-own ASIC/FPGA .` \ /
+ * .logic in Verilog or VHDL. ~~~`````````..`````~~` \ /
+ * ..````````......``````````` \o_
+ * |
+ * ^^ / \
+ *
+ * ...`````~~`.....``.`..........``````.`.``.```........``.
+ * ` 8, 16, 32 and 64 bits registers are supported, and``.
+ * . the number of GPIOs is determined by the width of ~
+ * .. the registers. ,............```.`.`..`.`.~~~.`.`.`~
+ * `.......````.```
+ */
+
+#include <linux/init.h>
+#include <linux/bug.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/compiler.h>
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+
+struct bgpio_chip {
+ struct gpio_chip gc;
+ void __iomem *reg_dat;
+ void __iomem *reg_set;
+ void __iomem *reg_clr;
+ spinlock_t lock;
+
+ int bits;
+ int big_endian_bits;
+
+ /* shadowed data register to clear/set bits safely */
+ unsigned long data;
+};
+
+static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
+{
+ return container_of(gc, struct bgpio_chip, gc);
+}
+
+static unsigned long bgpio_in(struct bgpio_chip *bgc)
+{
+ switch (bgc->bits) {
+ case 8:
+ return __raw_readb(bgc->reg_dat);
+ case 16:
+ return __raw_readw(bgc->reg_dat);
+ case 32:
+ return __raw_readl(bgc->reg_dat);
+#if BITS_PER_LONG >= 64
+ case 64:
+ return __raw_readq(bgc->reg_dat);
+#endif
+ }
+ return 0;
+}
+
+static void bgpio_out(struct bgpio_chip *bgc, void __iomem *reg,
+ unsigned long data)
+{
+ switch (bgc->bits) {
+ case 8:
+ __raw_writeb(data, reg);
+ return;
+ case 16:
+ __raw_writew(data, reg);
+ return;
+ case 32:
+ __raw_writel(data, reg);
+ return;
+#if BITS_PER_LONG >= 64
+ case 64:
+ __raw_writeq(data, reg);
+ return;
+#endif
+ }
+}
+
+static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
+{
+ if (bgc->big_endian_bits)
+ return 1 << (bgc->bits - 1 - pin);
+ else
+ return 1 << pin;
+}
+
+static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+ return bgpio_in(bgc) & bgpio_pin2mask(bgc, gpio);
+}
+
+static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+ unsigned long mask = bgpio_pin2mask(bgc, gpio);
+ unsigned long flags;
+
+ if (bgc->reg_set) {
+ if (val)
+ bgpio_out(bgc, bgc->reg_set, mask);
+ else
+ bgpio_out(bgc, bgc->reg_clr, mask);
+ return;
+ }
+
+ spin_lock_irqsave(&bgc->lock, flags);
+
+ if (val)
+ bgc->data |= mask;
+ else
+ bgc->data &= ~mask;
+
+ bgpio_out(bgc, bgc->reg_dat, bgc->data);
+
+ spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
+}
+
+static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ bgpio_set(gc, gpio, val);
+ return 0;
+}
+
+static int __devinit bgpio_probe(struct platform_device *pdev)
+{
+ const struct platform_device_id *platid = platform_get_device_id(pdev);
+ struct device *dev = &pdev->dev;
+ struct bgpio_chip *bgc;
+ struct resource *res_dat;
+ struct resource *res_set;
+ struct resource *res_clr;
+ resource_size_t dat_sz;
+ int bits;
+
+ res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
+ if (!res_dat)
+ return -EINVAL;
+
+ dat_sz = resource_size(res_dat);
+ if (!is_power_of_2(dat_sz))
+ return -EINVAL;
+
+ bits = dat_sz * 8;
+ if (bits > BITS_PER_LONG)
+ return -EINVAL;
+
+ bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
+ if (!bgc)
+ return -ENOMEM;
+
+ bgc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
+ if (!bgc->reg_dat)
+ return -ENOMEM;
+
+ res_set = platform_get_resource_byname(pdev, IORESOURCE_MEM, "set");
+ res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
+ if (res_set && res_clr) {
+ if (resource_size(res_set) != resource_size(res_clr) ||
+ resource_size(res_set) != dat_sz)
+ return -EINVAL;
+
+ bgc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
+ bgc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
+ if (!bgc->reg_set || !bgc->reg_clr)
+ return -ENOMEM;
+ } else if (res_set || res_clr) {
+ return -EINVAL;
+ }
+
+ spin_lock_init(&bgc->lock);
+
+ bgc->bits = bits;
+ bgc->big_endian_bits = !strcmp(platid->name, "basic-mmio-gpio-be");
+ bgc->data = bgpio_in(bgc);
+
+ bgc->gc.ngpio = bits;
+ bgc->gc.direction_input = bgpio_dir_in;
+ bgc->gc.direction_output = bgpio_dir_out;
+ bgc->gc.get = bgpio_get;
+ bgc->gc.set = bgpio_set;
+ bgc->gc.dev = dev;
+ bgc->gc.label = dev_name(dev);
+
+ if (dev->platform_data)
+ bgc->gc.base = (unsigned long)dev->platform_data;
+ else
+ bgc->gc.base = -1;
+
+ dev_set_drvdata(dev, bgc);
+
+ return gpiochip_add(&bgc->gc);
+}
+
+static int __devexit bgpio_remove(struct platform_device *pdev)
+{
+ struct bgpio_chip *bgc = dev_get_drvdata(&pdev->dev);
+
+ return gpiochip_remove(&bgc->gc);
+}
+
+static const struct platform_device_id bgpio_id_table[] = {
+ { "basic-mmio-gpio", },
+ { "basic-mmio-gpio-be", },
+ {},
+};
+MODULE_DEVICE_TABLE(platform, bgpio_id_table);
+
+static struct platform_driver bgpio_driver = {
+ .driver = {
+ .name = "basic-mmio-gpio",
+ },
+ .id_table = bgpio_id_table,
+ .probe = bgpio_probe,
+ .remove = __devexit_p(bgpio_remove),
+};
+
+static int __init bgpio_init(void)
+{
+ return platform_driver_register(&bgpio_driver);
+}
+module_init(bgpio_init);
+
+static void __exit bgpio_exit(void)
+{
+ platform_driver_unregister(&bgpio_driver);
+}
+module_exit(bgpio_exit);
+
+MODULE_DESCRIPTION("Driver for basic memory-mapped GPIO controllers");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.0.5

2010-08-31 18:21:54

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v5] gpio: Add driver for basic memory-mapped GPIO controllers

On Tue, Aug 31, 2010 at 09:58:39PM +0400, Anton Vorontsov wrote:
> The basic GPIO controllers may be found in various on-board FPGA
> and ASIC solutions that are used to control board's switches, LEDs,
> chip-selects, Ethernet/USB PHY power, etc.
>
> Usually these controllers do not privide any means of pin setup
> (in/out/open drain).
>
> The driver provides:
> - Support for 8/16/32/64 bits registers;
> - Support for GPIO controllers with clear/set registers;
> - Support for GPIO controllers with a single "data" register;
> - Support for big endian bits/GPIOs ordering (mostly used on PowerPC).
>
> Signed-off-by: Anton Vorontsov <[email protected]>

Reviewed-by: Mark Brown <[email protected]>

2010-08-31 20:32:11

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH v5] gpio: Add driver for basic memory-mapped GPIO controllers


>
> +static int bgpio_dir_in(struct gpio_chip *gc, unsigned int
> gpio)
> +{
> +??? return 0;


Here -EINVAL is more correct there than 0 ...