gpio: Add generic driver for simple memory mapped controllers
PATCH 1/1 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).
--- 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
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/simple_mm_gpio.c new file +243 -0
243 lines not on this page · raw message
Signature · 1 line
1.7.0.5
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · David Brownell
From: Anton Vorontsov <[email protected]>
Subject: [PATCH] gpio: Add generic driver for simple memory mapped controllersNAK. 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).
Quoted 3 lines
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!
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 2 lines
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,
Quoted 2 lines
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
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 2 lines
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
Quoted 3 lines · Alan Cox
Quoted 1 line
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! :)
Quoted 2 lines
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...
Quoted 7 lines
Quoted 3 lines
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.)
Quoted 2 lines
Quoted 2 lines
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).
Quoted 2 lines
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.)
Quoted 10 lines · David Brownell
Quoted 3 lines · Alan Cox
Quoted 1 line
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
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.
O> If there are "zillions" that suggests the HW
Quoted 3 lines
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.
Quoted 5 lines
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
--- On Thu, 8/26/10, Alan Cox <[email protected]> wrote:
(Which calls
Quoted 3 lines
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).
Quoted 10 lines · On Thu, 26 Aug 2010 15:58:51 -0700 (PDT) David Brownell <[email protected]>…
--- On Thu, 8/26/10, Alan Cox <[email protected]> wrote:
(Which callsQuoted 3 lines
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.
PATCH v2 1/1 gpio: Add driver for Anton GPIO controllers
In reply to David BrownellThe 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).
---
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
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/anton_gpio.c new file +244 -0
244 lines not on this page · raw message
Signature · 1 line
1.7.0.5
Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers
Quoted 3 lines · Anton Vorontsov
- 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?
Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers
On Thu, Aug 26, 2010 at 10:57:33AM -0700, David Brownell wrote:
[...]
Quoted 5 lines
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!
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Re: [PATCH v2] gpio: Add driver for Anton GPIO controllers
Quoted 11 lines · Anton Vorontsov
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
ordw_abcnames, but
NOT meaningless ... when there's actual IP behind the controller! Which, at first, is what I thought you were talking about.
Quoted 1 line
Just kiding, of course.
basic_mmio.cworks 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?).
Quoted 2 lines
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.
PATCH v3 1/1 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).
---
On Thu, Aug 26, 2010 at 03:48:31PM -0700, David Brownell wrote:
[...]
Quoted 3 lines
Quoted 1 line
Just kiding, of course.
basic_mmio.cworks for me.OK, how about resubmitting? With that name?
Sure thing.
Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 5 lines
Quoted 2 lines
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
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/basic_mmio_gpio.c new file +278 -0
278 lines not on this page · raw message
Signature · 1 line
1.7.0.5
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
PATCH v4 1/1 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).
---
Quoted 4 lines · David Brownell
But do it without
BUG()... assume that all uses ofBUG()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
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/basic_mmio_gpio.c new file +280 -0
280 lines not on this page · raw message
Signature · 1 line
1.7.0.5
Re: [PATCH v4] gpio: Add driver for basic memory-mapped GPIO controllers
--- On Sun, 8/29/10, Anton Vorontsov <[email protected]> wrote:
Brownell
Quoted 4 lines
Quoted 1 line · wrote:
But do it without
BUG()...Yeah, good idea. Let's turn these
BUG()intoWARN_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.
PATCH v5 1/1 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).
---
Quoted 10 lines · David Brownell
Quoted 3 lines · Anton Vorontsov
Quoted 1 line
But do it without
BUG()...Yeah, good idea. Let's turn these
BUG()intoWARN_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
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/basic_mmio_gpio.c new file +278 -0
278 lines not on this page · raw message
Signature · 1 line
1.7.0.5
Re: [PATCH v5] gpio: Add driver for basic memory-mapped GPIO controllers
Quoted 7 lines · Anton Vorontsov
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).
Quoted 7 lines
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]>
Re: [PATCH v5] gpio: Add driver for basic memory-mapped GPIO controllers
In reply to Anton VorontsovQuoted 6 lines
+static int
bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
Here -EINVAL is more correct there than 0 ...
PATCH v6 1/1 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.
These controllers may not provide any means of pin setup (in/out/open drain).
The driver supports:
- 8/16/32/64 bits registers;
- GPIO controllers with clear/set registers;
- GPIO controllers with a single "data" register;
- Big endian bits/GPIOs ordering (mostly used on PowerPC).
---
Quoted 6 lines · David Brownell
Quoted 4 lines
+static int
bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;Here
-EINVALis more correct there than 0 ...
IMHO, it doesn't really matter as we don't (and should not) check that value. But OK, done. + some changelog improvements as you suggested.
Thanks.
p.s.
On the next resend the cartoon guy will land and [most probably] will break an arm (or a leg).
Either you give an ack, or have nightmares because of that poor fellow.
drivers/gpio/Kconfig | 5 + drivers/gpio/Makefile | 1 + drivers/gpio/basic_mmio_gpio.c | 279 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 285 insertions(+), 0 deletions(-) create mode 100644 drivers/gpio/basic_mmio_gpio.c
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/basic_mmio_gpio.c new file +279 -0
279 lines not on this page · raw message
Signature · 1 line
1.7.0.5
PATCH v7 1/1 gpio: Add driver for basic memory-mapped GPIO controllers
In reply to David BrownellThe 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.
These controllers may not provide any means of pin setup (in/out/open drain).
The driver supports:
- 8/16/32/64 bits registers;
- GPIO controllers with clear/set registers;
- GPIO controllers with a single "data" register;
- Big endian bits/GPIOs ordering (mostly used on PowerPC).
---
A lucky v7.
In v7 I only changed platform data handling. It appears that when used with MFD core, we can't just pass integers via platform_data poiner, as platform_device_add_data() would try to copy the data pointed by mfd_cell.platform_data.
This is now fixed by introducing a proper platform data struct.
drivers/gpio/Kconfig | 5 + drivers/gpio/Makefile | 1 + drivers/gpio/basic_mmio_gpio.c | 281 +++++++++++++++++++++++++++++++++++++++ include/linux/basic_mmio_gpio.h | 20 +++ 4 files changed, 307 insertions(+), 0 deletions(-) create mode 100644 drivers/gpio/basic_mmio_gpio.c create mode 100644 include/linux/basic_mmio_gpio.h
drivers/gpio/Kconfig +5 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/basic_mmio_gpio.c new file +281 -0
281 lines not on this page · raw message
include/linux/basic_mmio_gpio.h new file +20 -0
@@ -0,0 +1,20 @@ +/* +* 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. +*/ + +#ifndef __BASIC_MMIO_GPIO_H +#define __BASIC_MMIO_GPIO_H + +struct bgpio_pdata { +int base; +}; + +#endif /* __BASIC_MMIO_GPIO_H */
Signature · 1 line
1.7.0.5
Re: [PATCH v7] gpio: Add driver for basic memory-mapped GPIO controllers
Quoted 19 lines · Anton Vorontsov
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.
These controllers may not provide any means of pin setup (in/out/open drain).
The driver supports:
- 8/16/32/64 bits registers;
- GPIO controllers with clear/set registers;
- GPIO controllers with a single "data" register;
- Big endian bits/GPIOs ordering (mostly used on PowerPC).Signed-off-by Anton Vorontsov <[email protected]>Reviewed-by Mark Brown <[email protected]>---
A lucky v7.
Quoted 7 lines
In v7 I only changed platform data handling. It appears that when used with MFD core, we can't just pass integers via
platform_datapoiner, asplatform_device_add_data()would try to copy the data pointed by mfd_cell.platform_data.This is now fixed by introducing a proper platform data struct.
Ping? Anyone?
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Re: [PATCH v7] gpio: Add driver for basic memory-mapped GPIO controllers
In reply to Anton VorontsovQuoted 54 lines · On Tue, 7 Sep 2010 18:01:32 +0400 Anton Vorontsov <[email protected]>…
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.
These controllers may not provide any means of pin setup (in/out/open drain).
The driver supports:
- 8/16/32/64 bits registers;
- GPIO controllers with clear/set registers;
- GPIO controllers with a single "data" register;
- Big endian bits/GPIOs ordering (mostly used on PowerPC)....
drivers/gpio/basic_mmio_gpio.c new file +35
-0@@ -0,0 +1,281 @@ +/* +* 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. ~~~`````````..`````~~` \ /
drivers/gpio/basic_mmio_gpio.c line 45
Quoted 10 lines
drivers/gpio/basic_mmio_gpio.c new file +10
-0+* ..````````......``````````` \o_ +* | +* ^^ / \ +* +* ...`````~~`.....``.`..........``````.`.``.```........``. +* ` 8, 16, 32 and 64 bits registers are supported, and``. +* . the number of GPIOs is determined by the width of ~ +* .. the registers. ,............```.`.`..`.`.~~~.`.`.`~ +* `.......````.``` +*/
ooh, sparkly!
Quoted 6 lines
...
drivers/gpio/basic_mmio_gpio.c new file +3
-0+struct bgpio_chip { +struct gpio_chip gc; +void __iomem *reg_dat;
drivers/gpio/basic_mmio_gpio.c line 76
Quoted 10 lines
drivers/gpio/basic_mmio_gpio.c new file +10
-0+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; +};
It would be good to document bits' and big_endian_bits', and to describe what `lock' locks.
drivers/gpio/basic_mmio_gpio.c line 165
Quoted 7 lines
...
drivers/gpio/basic_mmio_gpio.c new file +4
-0+static int bgpio_dir_in (struct gpio_chip *gc,unsigned int gpio) +{ +return 0 ; +}
hm, what does this mean. The hardware cannot set pin directions to "in"? If so, shouldn't we tell someone that their attempt to do this failed?
Quoted 63 lines
...
drivers/gpio/basic_mmio_gpio.c new file +60
-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_pdata *pdata =dev_get_platdata (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);
drivers/gpio/basic_mmio_gpio.c line 242
Quoted 10 lines
drivers/gpio/basic_mmio_gpio.c new file +10
-0+ +if (pdata) + bgc->gc.base = pdata->base; +else + bgc->gc.base =-1 ; + +dev_set_drvdata (dev, bgc); + +return gpiochip_add (&bgc->gc); +}
If this function returns -EINVAL then much head-scratching will ensue. It might make your life easier to emit a diagnostic just before the failure so you can work out why it failed.
PATCH 1/1 gpio: Add driver for basic memory-mapped GPIO controllers (fix)
Quoted 2 lines · Andrew Morton
It would be good to document
bits' andbig_endian_bits', and to describe what `lock' locks.
Done.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) +{ + return 0; +}hm, what does this mean. The hardware cannot set pin directions to "in"?
Nope, 0 is the success. The hardware cannot set pin directions at all (well, some hw can, but we don't support these yet).
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return gpiochip_add(&bgc->gc); +}If this function returns
-EINVALthen much head-scratching will ensue. It might make your life easier to emit a diagnostic just before the failure so you can work out why it failed.
OK.
---
Thanks!
drivers/gpio/basic_mmio_gpio.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
drivers/gpio/basic_mmio_gpio.c +19 -3
@@ -66,12 +66,23 @@ struct bgpio_chip {void __iomem *reg_dat;void __iomem *reg_set;void __iomem *reg_clr; -spinlock_t lock; +/* Number of bits (GPIOs): <register width> * 8. */ int bits; + +/* +* Some GPIO controllers work with the big-endian bits notation, +* e.g. in a 8-bits register, GPIO7 is the least significant bit. +*/ int big_endian_bits; -/* shadowed data register to clear/set bits safely */ +/* +* Used to lock bgpio_chip->data. Also, this is needed to keep +* shadowed and real data registers writes together. +*/ +spinlock_t lock; + +/* Shadowed data register to clear/set bits safely. */ unsigned long data; };
@@ -181,6 +192,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)struct resource *res_clr;resource_size_t dat_sz;int bits; +int ret; res_dat =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"dat" );if (!res_dat)
@@ -238,7 +250,11 @@ static int __devinit bgpio_probe(struct platform_device *pdev)dev_set_drvdata (dev, bgc); -return gpiochip_add (&bgc->gc); + ret =gpiochip_add (&bgc->gc); +if (ret) +dev_err (dev,"gpiochip_add() failed: %d\n" , ret); + +return ret; } static int __devexit bgpio_remove(struct platform_device *pdev)
Signature · 1 line
1.7.0.5
drivers/gpio/simple_mm_gpio.c line 151
Quoted 3 lines · Anton Vorontsov
drivers/gpio/simple_mm_gpio.c new file +3
-0+ 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.