GPIO implementation framework

Provides new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface.  Platforms can update their GPIO support to use this.  The downside is slower access to non-inlined GPIOs; rarely a problem except when bitbanging some protocol.

The upside is:

  * Providing two features which were "want to have (but OK to defer)" when GPIO interfaces were first discussed in November 2006:

    -   A "struct gpio_chip" to plug in GPIOs that aren't directly supported by SOC platforms, but come from FPGAs or other multifunction devices (like UCB-1x00 GPIOs).

    -   Full support for message-based GPIO expanders, needing a gpio_chip hookup; previous support for this part of the programming interface was just stubs.  (One example: the widely used pcf8574 I2C chips, with 8 GPIOs each.)

  * Including a non-stub implementation of the gpio_{request,free}() calls, which makes those calls much more useful.  The diagnostic labels are also recorded given DEBUG_FS, so /sys/kernel/debug/gpio can show a snapshot of all GPIOs known to this infrastructure.

The driver programming interfaces introduced in 2.6.21 do not change at all; this new infrastructure is entirely below the covers.

Quoted 5 lines · David Brownell

Provides new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface. Platforms can update their GPIO support to use this. The downside is slower access to non-inlined GPIOs; rarely a problem except when bitbanging some protocol.

I was asked just what that overhead *is* ... and it surprised me. A summary of the results is appended to this note.

Fortuntely it turns out those problems all go away if the gpiolib code uses a *raw* spinlock to guard its table lookups. With a raw spinlock, any performance impact of gpiolib seems to be well under a microsecond in this bitbang context (and not objectionable). Preempt became free; enabling debug options had only a minor cost.

That's as it should be, since the only substantive changes were to grab and release a lock, do one table lookup a bit differently, and add one indirection function call ... changes which should not have any visible performance impact on per-bit codepaths, and one might expect to cost on the order of one dozen instructions.

So the next version of this code will include a few minor bugfixes, and will also use a raw spinlock to protect that table. A raw lock seems appropriate there in any case, since non-sleeping GPIOs should be accessible from hardirq contexts even on RT kernels.

If anyone has any strong arguments against using a raw spinlock to protect that table, it'd be nice to know them sooner rather than later.

- Dave

SUMMARY:

Using the i2c-gpio driver on a preempt kernel with all the usual kernel debug options enabled, the per-bit times (*) went up in a bad way: from about 6.4 usec/bit (original GPIO code on this board) up to about 11.2 usec/bit (just switching to gpiolib), which is well into "objectionable overhead" territory for bit access.

Just enabling preempt shot the time up to 7.4 usec/bit ... which is also objectionable (it's all-the-time overhead that is clearly needless), but much less so.

Converting the table lock to be a raw spinlock essentially removed all non-debug overheads. It took enabling all those debug options plus internal gpiolib debugging overhead to get those times up to the 7.4 usec/bit that previously applied even with just preempt.

(*) Those times being eyeballed medians; I didn't make time to find
a way to export a few thousand measurements from the tool and do the math. The typical range was +/- one usec.

The numbers include udelay() calls, so the relevant point is the time *delta* attributable only to increased gpiolib costs, not the base time (with udelays). The delta probably reflects on the order of four GPIO calls: set two different bits, clear one of them, and read it to make sure it cleared.

Quoted 21 lines

The upside is:

* Providing two features which were "want to have (but OK to defer)" when GPIO interfaces were first discussed in November 2006:

-A "struct gpio_chip" to plug in GPIOs that aren't directly supported by SOC platforms, but come from FPGAs or other multifunction devices (like UCB-1x00 GPIOs).

-Full support for message-based GPIO expanders, needing a gpio_chip hookup; previous support for this part of the programming interface was just stubs. (One example: the widely used pcf8574 I2C chips, with 8 GPIOs each.)

* Including a non-stub implementation of the gpio_{request,free}() calls, which makes those calls much more useful. The diagnostic labels are also recorded given DEBUG_FS, so /sys/kernel/debug/gpio can show a snapshot of all GPIOs known to this infrastructure.

The driver programming interfaces introduced in 2.6.21 do not change at all; this new infrastructure is entirely below the covers.

Hi David,

I hope I was not late giving my humble feedback on this framework :-)

Can we use "per gpio based" structure instead of "per gpio_chip" based one, just like what the generic IRQ layer is doing nowadays?

So that

a. you don't have to declare per gpio_chip "can_sleep", "is_out" and "requested".
Those will be just bits of properties of a single GPIO.

b. and furthur more, one can avoid the use of ARCH_GPIOS_PER_CHIP, which leads to many holes

c. gpio_to_chip() will be made easy and straight forward

d. granularity of spin_lock()/_unlock() can be made small (per GPIO instead of per gpio_chip)

What do you think?

- eric

Quoted 34 lines · David Brownell

Quoted 5 lines · David Brownell

Provides new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface. Platforms can update their GPIO support to use this. The downside is slower access to non-inlined GPIOs; rarely a problem except when bitbanging some protocol.

I was asked just what that overhead *is* ... and it surprised me. A summary of the results is appended to this note.

Fortuntely it turns out those problems all go away if the gpiolib code uses a *raw* spinlock to guard its table lookups. With a raw spinlock, any performance impact of gpiolib seems to be well under a microsecond in this bitbang context (and not objectionable). Preempt became free; enabling debug options had only a minor cost.

That's as it should be, since the only substantive changes were to grab and release a lock, do one table lookup a bit differently, and add one indirection function call ... changes which should not have any visible performance impact on per-bit codepaths, and one might expect to cost on the order of one dozen instructions.

So the next version of this code will include a few minor bugfixes, and will also use a raw spinlock to protect that table. A raw lock seems appropriate there in any case, since non-sleeping GPIOs should be accessible from hardirq contexts even on RT kernels.

If anyone has any strong arguments against using a raw spinlock to protect that table, it'd be nice to know them sooner rather than later.

- Dave

Quoted 6 lines · eric miao

Hi David,

I hope I was not late giving my humble feedback on this framework :-)

Can we use "per gpio based" structure instead of "per gpio_chip" based one, just like what the generic IRQ layer is doing nowadays?

We "can" do most anything. What would that improve though?

Each irq_chip handles multiple IRQs ... just like this patch has each gpio_chip handling multiple GPIOs. (Not that I think GPIO code should closely model IRQ code; they need to address very different problems.)

I can't tell what you intend to suggest as a "per-GPIO" data structure; since I can think of at least three different ways to do such a thing, you should be more concrete. I'd think it should be in *addition* to a gpio_chip structure though.

Quoted 6 lines

So that

a. you don't have to declare per gpio_chip "can_sleep", "is_out" and "requested".
Those will be just bits of properties of a single GPIO.

The can_sleep value is a per-controller thing. The other bits are indeed per-GPIO.

So do you mean a structure with two bits, plus a pointer to a gpio_chip, plus likely other stuff (what?) to make it work? What would the hot-path costs be (for getting/setting values of an on-chip GPIO)?

Quoted 3 lines

b. and furthur more, one can avoid the use of ARCH_GPIOS_PER_CHIP, which leads to many holes

Why should holes (in the GPIO number sequence) be a problem? In this code, they don't cost much space at all. They'd cost more if there were a per-GPIO structure though...

The only downside of GPIOS_PER_CHIP that I know of right now is that it complicates mixing gpio bank sizes; it's a ceiling, some controllers would allocate more than they need. The upside of that is efficiency, and a closer match to how underlying hardware works.

Of course, GPIOS_PER_CHIP *could* be decoupled from how the table of gpio_chip pointers is managed. If the table were to group GPIOs in units of 8, a gpio_chip with 32 GPIOs could take four adjacent entries while an 8-bit GPIO expander could take just one. That'd be a very easy patch, supporting a more dense allocation of GPIO numbers... although it would increase static memory consumption by typically NR_GPIOS/4 pointers.

Quoted 1 line

c. gpio_to_chip() will be made easy and straight forward

I'd say "return chips[gpio / ARCH_GPIOS_PER_CHIP]" already meets both criteria!

There's also "efficient" to consider; this way doesn't cost much memory or add levels of indirection (compared to most platforms, which already use a similar array).

Quoted 2 lines

d. granularity of spin_lock()/_unlock() can be made small (per GPIO instead of per gpio_chip)

Why would per-GPIO locking be needed though? Look again...

The locking is there fundamentally because gpio_chip structures may need to be unregistered; that's not a per-gpio issue. Even when a gpio is marked in chip->requested under that lock, that's part of ensuring that the unregistration is prevented so long as the GPIO is in active use.

Plus, fine grained locking is rarely a good idea; it normally increases locking overhead by involving multiple locks. Only add extra locks if a single lock sees too much contention; and even then, only if that contention can't be removed by using a smarter design.

- Dave

Quoted 40 lines

What do you think?

- eric

Quoted 34 lines · David Brownell

Quoted 5 lines · David Brownell

Provides new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface. Platforms can update their GPIO support to use this. The downside is slower access to non-inlined GPIOs; rarely a problem except when bitbanging some protocol.

I was asked just what that overhead *is* ... and it surprised me. A summary of the results is appended to this note.

Fortuntely it turns out those problems all go away if the gpiolib code uses a *raw* spinlock to guard its table lookups. With a raw spinlock, any performance impact of gpiolib seems to be well under a microsecond in this bitbang context (and not objectionable). Preempt became free; enabling debug options had only a minor cost.

That's as it should be, since the only substantive changes were to grab and release a lock, do one table lookup a bit differently, and add one indirection function call ... changes which should not have any visible performance impact on per-bit codepaths, and one might expect to cost on the order of one dozen instructions.

So the next version of this code will include a few minor bugfixes, and will also use a raw spinlock to protect that table. A raw lock seems appropriate there in any case, since non-sleeping GPIOs should be accessible from hardirq contexts even on RT kernels.

If anyone has any strong arguments against using a raw spinlock to protect that table, it'd be nice to know them sooner rather than later.

- Dave

Quoted 2 lines · eric miao

Subject: [PATCH 5/5] move per GPIO "requested" to "struct gpio_desc"

include/asm-generic/gpio.h struct gpio_chip; · line 34

Quoted 7 lines

 struct gpio_desc {
      struct gpio_chip *chip;
      unsigned is_out:1;

include/asm-generic/gpio.h +4 -0

+     unsigned requested:1;
+#ifdef CONFIG_DEBUG_FS
+     const char *requested_str;
+#endif

A better name for this would be "label", matching what's passed from gpio_request(). Ndls abrviatns r bd.

Note that this means (on typical 32-bit embedded hardware) twelve bytes per GPIO, which if you assume 256 GPIOs means an extra 3 KB static memory compared to the patch I sent.

Review static inline int gpio_is_onchip(unsigned gpio, · line 47

Quoted 8 lines

(quoted diff) +1 -1

@@ -43,20 +43,19 @@ static inline int gpio_is_onchip(unsigned gpio,
 struct gpio_chip *chip)
 /* Warn when drivers omit gpio_request() calls -- legal but
  * ill-advised when setting direction, and otherwise illegal.
  */
-static void gpio_ensure_requested(struct gpio_chip *chip, unsigned offset)
+static void gpio_ensure_requested(unsigned gpio)

Simpler to pass a gpio_desc pointer ...

Review static inline int gpio_is_onchip(unsigned gpio, · line 49

Quoted 4 lines

(quoted diff) +1 -2

      if (!requested)
-             printk(KERN_DEBUG "GPIO-%d autorequested\n",
-                             chip->base + offset);
+             pr_debug("GPIO-%d autorequested\n", gpio);

Leave the printk in ... this is the sort of thing we want to see fixed, which becomes unlikely once you hide such diagnostics. And for that matter, what would be enabling the "-DDEBUG" that would trigger a pr_debug() message?

... overall the main downside of these patches seems to be that it consumes more static memory.

Quoted 12 lines

include/asm-generic/gpio.h struct gpio_chip; · line 34

Quoted 8 lines

 struct gpio_desc {
      struct gpio_chip *chip;
      unsigned is_out:1;

include/asm-generic/gpio.h +4 -0

+     unsigned requested:1;
+#ifdef CONFIG_DEBUG_FS
+     const char *requested_str;
+#endif

Note that this means (on typical 32-bit embedded hardware) twelve bytes per GPIO, which if you assume 256 GPIOs means an extra 3 KB static memory compared to the patch I sent.

Note this reduces the memory in gpio_chip, so it consumes almost same memory as the patch you sent.

Quoted 12 lines

Quoted 12 lines

include/asm-generic/gpio.h struct gpio_chip; · line 34

Quoted 8 lines

 struct gpio_desc {
      struct gpio_chip *chip;
      unsigned is_out:1;

include/asm-generic/gpio.h +4 -0

+     unsigned requested:1;
+#ifdef CONFIG_DEBUG_FS
+     const char *requested_str;
+#endif

Note that this means (on typical 32-bit embedded hardware) twelve bytes per GPIO, which if you assume 256 GPIOs means an extra 3 KB static memory compared to the patch I sent.

Actually, 2K is a more accurate number -- ignore DEBUG_FS.

Quoted 2 lines

Note this reduces the memory in gpio_chip, so it consumes almost same memory as the patch you sent.

No; the amount of space shaved from a typical (32-bit banks) gpio_chip is *exactly* the cost of one gpio_desc: two words. In one case, two bitmaps. In the other, a pointer, two bits, and internal struct padding.

So unless each bank has only a single GPIO, this approach does cost more memory. Both for the extra memory associated with each gpio_chip that's used, and for unused gpio_desc.

That's not necessarily a bad thing, though it's always worth avoiding bloat.

- Dave

Review

Quoted 4 lines · eric miao

(quoted diff) +1 -2

 	if (!requested)
-		printk(KERN_DEBUG "GPIO-%d autorequested\n",
-				chip->base + offset);
+		pr_debug("GPIO-%d autorequested\n", gpio);

Leave the printk in ... this is the sort of thing we want to see fixed, which becomes unlikely once you hide such diagnostics. And for that matter, what would be enabling the "-DDEBUG" that would trigger a pr_debug() message?

Quoted 10 lines · David Brownell

Review

Quoted 4 lines · eric miao

(quoted diff) +1 -2

 	if (!requested)
-		printk(KERN_DEBUG "GPIO-%d autorequested\n",
-				chip->base + offset);
+		pr_debug("GPIO-%d autorequested\n", gpio);

Leave the printk in ... this is the sort of thing we want to see fixed, which becomes unlikely once you hide such diagnostics. And for that matter, what would be enabling the "-DDEBUG" that would trigger a pr_debug() message?

The original code isn't correct either.

Either this is a debug message
and indeed pr_debug() should be used, or it's not and KERN_DEBUG should be replaced by a lower log level.

Quoted 12 lines · Jean Delvare

Quoted 10 lines · David Brownell

Review

Quoted 4 lines · eric miao

(quoted diff) +1 -2

 	if (!requested)
-		printk(KERN_DEBUG "GPIO-%d autorequested\n",
-				chip->base + offset);
+		pr_debug("GPIO-%d autorequested\n", gpio);

Leave the printk in ... this is the sort of thing we want to see fixed, which becomes unlikely once you hide such diagnostics. And for that matter, what would be enabling the "-DDEBUG" that would trigger a pr_debug() message?

Quoted 1 line

The original code isn't correct either.

It's perfectly correct. That it's an idiom you don't seem to *like* but is distinct from correctness.

Quoted 3 lines

Either this is a debug message
and indeed pr_debug() should be used, or it's not and KERN_DEBUG should be replaced by a lower log level.

KERN_DEBUG is what says the message level is "debug". Both styles log messages at that priority level.

Which is distinct from saying that the message should vanish from non-debug builds ... that's what pr_debug and friends do, by relying implicitly on "-DDEBUG".

In this case, the original code was saying that the message should NOT just vanish. One reason the patch was incorrect was that even on its own terms, it was wrong ... since it used the "-DDEBUG" mechanism wrong, and prevented the message from *EVER* appearing.

Here comes the point of "struct gpio_desc"

Subject: [PATCH 3/5] use a per GPIO "struct gpio_desc" and chain "gpio_chip" to a list

Quoted 4 lines · eric miao

Here comes the point of "struct gpio_desc"

Subject: [PATCH 3/5] use a per GPIO "struct gpio_desc" and chain "gpio_chip" to a list

I see what it does, but don't see the "why" ... surely you can come up with a one sentence description of why this would be better?

And I'd been so glad to *get rid of* that list, too.

lib/gpiolib.c line 31

Quoted 11 lines

lib/gpiolib.c +4 -4

+struct gpio_desc {
+     struct gpio_chip *chip;
+};
+
 
-/* gpio_lock protects modification to the table of chips and to
- * gpio_chip->requested.  If a gpio is requested, its gpio_chip
- * is not removable.
- */

But it still protects data. Don't remove documentation for what locks protect ... update it! Otherwise someonels going to come by and make a change which breaks the locking model. Usually in some subtle (hard-to-debug) way.

Review

Quoted 11 lines

(quoted diff) +1 -5

-     for (id = 0; id < ARRAY_SIZE(chips); id++) {
-             chip = chips[id];
-             if (!chip)
-                     continue;
-
+     list_for_each_entry(chip, &gpio_chip_list, node) {
              seq_printf(s, "%sGPIOs %d-%d, %s%s:\n",
                              started ? "\n" : "",
                              chip->base, chip->base + chip->ngpio - 1,

Note that this now produces the debug info in a relatively random order ... ordered by registration rather than anything useful, and hence awkward to read.

It'd be better if you just scanned your new gpio_desc[] table in numeric order, and start a new section whenever you find a new gpio_chip.

That'd get rid of that otherwise-useless list, too.

- Dave

so that requested will always be used, only *requested_str will be used for DEBUG_FS tracking assistance

Subject: [PATCH 2/5] define gpio_chip.requested_str as a debugfs tracking string

Here comes a bunch of patches to illustrate my idea, some are not related to the point I mentioned, and they are not mature for now :-)

Subject: [PATCH 1/5] add gpio_is_onchip() for commonly used gpio range checking

  http://marc.info/?l=linux-kernel&m=119463810905330&w=2
  http://marc.info/?l=linux-kernel&m=119463811005344&w=2
  http://marc.info/?l=linux-kernel&m=119463811105352&w=2

Plus the appended tweak.

Quoted 5 lines · eric miao

Here comes a bunch of patches to illustrate my idea, some are not related to the point I mentioned, and they are not mature for now :-)

Subject: [PATCH 1/5] add gpio_is_onchip() for commonly used gpio range checking

I'll send substantive comments later, but meanwhile note that the *CURRENT* version was posted last Friday:

http://marc.info/?l=linux-kernel&m=119463810905330&w=2
http://marc.info/?l=linux-kernel&m=119463811005344&w=2
http://marc.info/?l=linux-kernel&m=119463811105352&w=2

Plus the appended tweak. It's more useful to send patches against current code, so applying them doesn't provide a small avalanche of rejects. :)

With respect to this patch adding gpio_is_onchip(), I don't see a point. The "gpio >= chip->base" check is basically "are the data structures corrupted?" and so it should only be done if "extra_checks" is defined. (And IMO, not then ...) And combining the other two tests that way doesn't make anything more clear to me. That's somewhat of a style issue, I guess, unless you're like me and don't much trust GCC to avoid extra instructions.

- Dave

Quoted 4 lines

Quoted 2 lines

Can we use "per gpio based" structure instead of "per gpio_chip" based one, just like what the generic IRQ layer is doing nowadays?

We "can" do most anything.  What would that improve though?

Quoted 4 lines · eric miao

Quoted 4 lines

Quoted 2 lines

Can we use "per gpio based" structure instead of "per gpio_chip" based one, just like what the generic IRQ layer is doing nowadays?

We "can" do most anything. What would that improve though?

... What would that improve, though? Your followup posts still don't answer that question for me. I see the code, but don't have an answer to that question.

Quoted 6 lines

Quoted 1 line

Quoted 1 line

We "can" do most anything. What would that improve though?

... What would that improve, though?  Your followup posts still don't answer that question for me.  I see the code, but don't have an answer to that question.

to be honest, I don't feel like the holes. Put restrictions on the numbering of GPIOs might not be a good idea either.

So the point of these is to make it easier for platforms (or even just boards) to make sure the GPIO number space is densely packed, rather than loosely so?  Paying about 2KBytes for that privilege.  (Assuming a 32 bit system with 256 GPIOs.)

I could see that being a reasonable tradeoff.  I wouldn't have started there myself, but you know how that goes!

Does anyone else have any comments on that issue?

Quoted 10 lines · David Brownell

So the point of these is to make it easier for platforms (or even just boards) to make sure the GPIO number space is densely packed, rather than loosely so? Paying about 2KBytes for that privilege. (Assuming a 32 bit system with 256 GPIOs.)

I could see that being a reasonable tradeoff. I wouldn't have started there myself, but you know how that goes!

Does anyone else have any comments on that issue?

Nobody else seems to have any comments on Eric's series of patches to add a gpio_desc layer ... whereas, I was looking at updating one platform, and got annoyed at some stuff that would have been non-issues with them in place!

Eric, would you feel like rolling an all-in-one patch against the gpiolib support from 2.6.24-rc3-mm? Including updated versions of your patches:

- [PATCH 2/5] define gpio_chip.requested_str (renaming it as "label" to match its usage)
- [PATCH 3/5] use a per GPIO "struct gpio_desc" (but without that needless list; for debug, just scan the gpio_desc list for the next non-null chip)
- [PATCH] move per GPIO "is_out" to "struct gpio_desc"

        (i.e. patch 4/5)

- [PATCH 5/5] move per GPIO "requested" to "struct gpio_desc" (and "label" too)

along with removing the ARCH_GPIOS_PER_CHIP symbol, and reducing ARCH_NR_GPIOS to a value which will waste less space by default? (Like maybe 256.)

I think an all-in-one patch will be easier to review and agree on including (or not).

- Dave

4. use a loop for "gpio_desc[]" instead of a loop for "gpio_chips[]" in gpiolib_show(), change is straight forward; since it is now per gpio based, the gpio_chip.dbg_show() is removed as well

c) removal of "gpiochip_is_requested()", and use "gpio_desc->requested" instead

d) make gpio_ensure_requested() use gpio_desc, and simplify the code a bit

        status = chip->direction_input(chip, gpio); -       if (status == 0)
-               clear_bit(gpio, chip->is_out); +       if (status)
+               desc->is_out = 0;

Review

Quoted 5 lines · eric miao

status = chip->direction_input(chip, gpio);

(quoted diff) +2 -2

-if (status == 0)
-clear_bit(gpio, chip->is_out);
+if (status)
+desc->is_out = 0;

You added that same bug in two places (direction_output too). Only zero status means success; otherwise it's negative errno.

Clearly this patch wasn't tested at all.

Update gpiolib to use a table of per-GPIO "struct gpio_desc" instead of a table of "struct gpio_chip".

- Change "is_out" and "requested" from arrays in "struct gpio_chip" to bit fields in "struct gpio_desc", eliminating ARCH_GPIOS_PER_CHIP.

- Stop overloading "requested" flag with "label" tracked for debugfs.

- Change gpiochip_is_requested() into a regular function, since it now accesses data that's not exported from the gpiolib code. Also change its signature, for the same reason.

- Reduce default ARCH_NR_GPIOS to 256 to shrink gpio_desc table cost. On 32-bit platforms without debugfs, that table size is 2KB.

This makes it easier to work with chips with different numbers of GPIOs, and to avoid holes in GPIOs number sequences. Those holes can cost a lot of unusable irq_desc space for GPIOs that act as IRQs.

Based on a patch from Eric Miao.

# NOT signed-off yet ... purely for comment. It's been sanity tested.
---
The question I'm most interested in is whether it's worth paying the extra data memory. I'm currently leaning towards "yes", mostly since it'll let me be lazy about some development boards with four different kinds of GPIO controller, each with different numbers of GPIOs.

Note that the ARM AT91 updates are against a patch which has been circulated for comment but not yet submitted. The at32ap and mcp23s08 parts are currently in the MM tree. The PXA platform support didn't need changes; DaVinci won't either. The OMAP support (not recently updated) will need slightly more updates than those shown here.

 arch/arm/mach-at91/gpio.c            |    7 -
 arch/avr32/mach-at32ap/pio.c         |    7 -
 drivers/spi/mcp23s08.c               |    8 -
 include/asm-avr32/arch-at32ap/gpio.h |    2
 include/asm-generic/gpio.h           |   45 +-------
 lib/gpiolib.c                        |  194 ++++++++++++++++++-----------------
 6 files changed, 129 insertions(+), 134 deletions(-)

arch/arm/mach-at91/gpio.c 2007-11-27 14:32:01.000000000 -0800 renamed from arch/arm/mach-at91/gpio.c 2007-11-27 14:31:05.000000000 -0800 +5 -2

@@ -484,7 +484,10 @@ at91_bank_show(struct seq_file *s, struc
        mdsr = __raw_readl(pio + PIO_MDSR);
 
        for (i = 0, mask = 1; i < chip->ngpio; i++, mask <<= 1) {
-               if (!gpiochip_is_requested(chip, i)) {
+               const char      *label;
+
+               label = gpiochip_is_requested(chip, i);
+               if (!label) {
                        /* peripheral-A or peripheral B?
                         * else unused/unrequested GPIO; ignore.
                         */

@@ -501,7 +504,7 @@ at91_bank_show(struct seq_file *s, struc
                 */
                seq_printf(s, " gpio-%-3d P%c%-2d (%-12s) %s %s %s",
                        chip->base + i, 'A' + bank_num, i,
-                       chip->requested[i],
+                       label,
                        (osr & mask) ? "out" : "in ",
                        (mask & pdsr) ? "hi" : "lo",
                        (mask & pusr) ? "  " : "up");

arch/avr32/mach-at32ap/pio.c 2007-11-27 14:30:04.000000000 -0800 renamed from arch/avr32/mach-at32ap/pio.c 2007-11-27 14:30:03.000000000 -0800 +5 -2

@@ -315,12 +315,15 @@ static void pio_bank_show(struct seq_fil
        bank = 'A' + pio->pdev->id;
 
        for (i = 0, mask = 1; i < 32; i++, mask <<= 1) {
-               if (!gpiochip_is_requested(chip, i))
+               const char *label;
+
+               label = gpiochip_is_requested(chip, i);
+               if (!label)
                        continue;
 
                seq_printf(s, " gpio-%-3d P%c%-2d (%-12s) %s %s %s",
                        chip->base + i, bank, i,
-                       chip->requested[i],
+                       label,
                        (osr & mask) ? "out" : "in ",
                        (mask & pdsr) ? "hi" : "lo",
                        (mask & pusr) ? "  " : "up");

drivers/spi/mcp23s08.c 2007-11-27 14:30:04.000000000 -0800 renamed from drivers/spi/mcp23s08.c 2007-11-27 14:29:20.000000000 -0800 +5 -3

@@ -184,12 +184,14 @@ static void mcp23s08_dbg_show(struct seq
        }
 
        for (t = 0, mask = 1; t < 8; t++, mask <<= 1) {
-               if (!gpiochip_is_requested(chip, t))
+               const char      *label;
+
+               label = gpiochip_is_requested(chip, t);
+               if (!label)
                        continue;
 
                seq_printf(s, " gpio-%-3d P%c.%d (%-12s) %s %s %s",
-                       chip->base + t, bank, t,
-                       chip->requested[t],
+                       chip->base + t, bank, t, label,
                        (mcp->cache[MCP_IODIR] & mask) ? "in " : "out",
                        (mcp->cache[MCP_GPIO] & mask) ? "hi" : "lo",
                        (mcp->cache[MCP_GPPU] & mask) ? "  " : "up");

include/asm-avr32/arch-at32ap/gpio.h 2007-11-27 14:30:04.000000000 -0800 renamed from include/asm-avr32/arch-at32ap/gpio.h 2007-11-27 14:30:03.000000000 -0800 +1 -1

@@ -8,7 +8,7 @@
 /* Some GPIO chips can manage IRQs; some can't.  The exact numbers can
  * be changed if needed, but for the moment they're not configurable.
  */
-#define ARCH_NR_GPIOS  (NR_GPIO_IRQS + 2 * ARCH_GPIOS_PER_CHIP)
+#define ARCH_NR_GPIOS  (NR_GPIO_IRQS + 2 * 32)
 
 
 /* Arch-neutral GPIO API, supporting both "native" and external GPIOs. */

include/asm-generic/gpio.h 2007-11-27 14:30:04.000000000 -0800 renamed from include/asm-generic/gpio.h 2007-11-27 14:29:19.000000000 -0800 +8 -37

@@ -4,21 +4,16 @@
 #ifdef CONFIG_GPIO_LIB
 
 /* Platforms may implement their GPIO interface with library code,
- * at a small performance cost for non-inlined operations.
+ * at a small performance cost for non-inlined operations and some
+ * extra memory (for code and per-GPIO table entries).
  *
  * While the GPIO programming interface defines valid GPIO numbers
  * to be in the range 0..MAX_INT, this library restricts them to the
- * smaller range 0..ARCH_NR_GPIOS and allocates them in groups of
- * ARCH_GPIOS_PER_CHIP (which will usually be the word size used for
- * each bank of a SOC processor's integrated GPIO modules).
+ * smaller range 0..ARCH_NR_GPIOS.
  */
 
 #ifndef ARCH_NR_GPIOS
-#define ARCH_NR_GPIOS          512
-#endif
-
-#ifndef ARCH_GPIOS_PER_CHIP
-#define ARCH_GPIOS_PER_CHIP    BITS_PER_LONG
+#define ARCH_NR_GPIOS          256
 #endif
 
 struct seq_file;

@@ -36,13 +31,10 @@ struct seq_file;
  *     state (such as pullup/pulldown configuration).
  * @base: identifies the first GPIO number handled by this chip; or, if
  *     negative during registration, requests dynamic ID allocation.
- * @ngpio: the number of GPIOs handled by this controller; the value must
- *     be at most ARCH_GPIOS_PER_CHIP, so the last GPIO handled is
- *     (base + ngpio - 1).
+ * @ngpio: the number of GPIOs handled by this controller; the last GPIO
+ *     handled is (base + ngpio - 1).
  * @can_sleep: flag must be set iff get()/set() methods sleep, as they
  *     must while accessing GPIO expander chips over I2C or SPI
- * @is_out: bit array where bit N is true iff GPIO with offset N has been
- *      called successfully to configure this as an output
  *
  * A gpio_chip can help platforms abstract various sources of GPIOs so
  * they can all be accessed through a common programing interface.

@@ -70,31 +62,10 @@ struct gpio_chip {
        int                     base;
        u16                     ngpio;
        unsigned                can_sleep:1;
-
-       /* other fields are modified by the gpio library only */
-       DECLARE_BITMAP(is_out, ARCH_GPIOS_PER_CHIP);
-
-#ifdef CONFIG_DEBUG_FS
-       /* fat bits */
-       const char              *requested[ARCH_GPIOS_PER_CHIP];
-#else
-       /* thin bits */
-       DECLARE_BITMAP(requested, ARCH_GPIOS_PER_CHIP);
-#endif
 };
 
-/* returns true iff a given gpio signal has been requested;
- * primarily for code dumping gpio_chip state.
- */
-static inline int
-gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
-{
-#ifdef CONFIG_DEBUG_FS
-       return chip->requested[offset] != NULL;
-#else
-       return test_bit(offset, chip->requested);
-#endif
-}
+extern const char *gpiochip_is_requested(struct gpio_chip *chip,
+                       unsigned offset);
 
 /* add/remove chips */
 extern int gpiochip_add(struct gpio_chip *chip);

lib/gpiolib.c 2007-11-27 14:30:04.000000000 -0800 renamed from lib/gpiolib.c 2007-11-27 14:29:20.000000000 -0800 +11 -3

@@ -28,39 +28,41 @@
 #define        extra_checks    0
 #endif
 
-/* gpio_lock protects the table of chips and gpio_chip->requested.
+/* gpio_lock protects the gpio_desc[] table and desc->requested.
  * While any GPIO is requested, its gpio_chip is not removable;
  * each GPIO's "requested" flag serves as a lock and refcount.
  */
 static DEFINE_SPINLOCK(gpio_lock);
-static struct gpio_chip *chips[DIV_ROUND_UP(ARCH_NR_GPIOS,
-                                       ARCH_GPIOS_PER_CHIP)];
+
+struct gpio_desc {
+       struct gpio_chip        *chip;
+       unsigned                is_out:1;
+       unsigned                requested:1;
+#ifdef CONFIG_DEBUG_FS
+       const char              *label;
+#endif
+};
+static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
 
 /* Warn when drivers omit gpio_request() calls -- legal but

lib/gpiolib.c 2007-11-27 14:30:04.000000000 -0800 renamed from lib/gpiolib.c 2007-11-27 14:29:20.000000000 -0800 +86 -81

 #ifdef CONFIG_DEBUG_FS
-       requested = (int) chip->requested[offset];
-       if (!requested)
-               chip->requested[offset] = "[auto]";
-#else
-       requested = test_and_set_bit(offset, chip->requested);
+               desc->label = "[auto]";
 #endif
-
-       if (!requested)
-               printk(KERN_DEBUG "GPIO-%d autorequested\n",
-                               chip->base + offset);
+       }
 }
 
 /* caller holds gpio_lock *OR* gpio is marked as requested */
 static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
 {
-       return chips[gpio / ARCH_GPIOS_PER_CHIP];
+       return gpio_desc[gpio].chip;
 }
 
 /**

@@ -78,20 +80,26 @@ int gpiochip_add(struct gpio_chip *chip)
        int             status = 0;
        unsigned        id;
 
-       if (chip->base < 0 || (chip->base % ARCH_GPIOS_PER_CHIP) != 0)
-               return -EINVAL;
-       if ((chip->base + chip->ngpio) >= ARCH_NR_GPIOS)
-               return -EINVAL;
-       if (chip->ngpio > ARCH_GPIOS_PER_CHIP)
+       /* NOTE chip->base negative is reserved to mean a request for
+        * dynamic allocation.  We probably won't ever use that ...
+        */
+
+       if (chip->base < 0 || (chip->base  + chip->ngpio) >= ARCH_NR_GPIOS)
                return -EINVAL;
 
        spin_lock_irqsave(&gpio_lock, flags);
 
-       id = chip->base / ARCH_GPIOS_PER_CHIP;
-       if (chips[id] == NULL)
-               chips[id] = chip;
-       else
-               status = -EBUSY;
+       /* make sure the GPIOs are not claimed by any gpio_chip */
+       for (id = chip->base; id < chip->base + chip->ngpio; id++)
+               if (gpio_desc[id].chip != NULL) {
+                       status = -EBUSY;
+                       break;
+               }
+
+       if (status == 0) {
+               for (id = chip->base; id < chip->base + chip->ngpio; id++)
+                       gpio_desc[id].chip = chip;
+       }
 
        spin_unlock_irqrestore(&gpio_lock, flags);
        return status;

@@ -108,23 +116,19 @@ int gpiochip_remove(struct gpio_chip *ch
 {
        unsigned long   flags;
        int             status = 0;
-       int             offset;
-       unsigned        id = chip->base / ARCH_GPIOS_PER_CHIP;
+       unsigned        id;
 
        spin_lock_irqsave(&gpio_lock, flags);
 
-       for (offset = 0; offset < chip->ngpio; offset++) {
-               if (gpiochip_is_requested(chip, offset)) {
+       for (id = chip->base; id < chip->base + chip->ngpio; id++)
+               if (gpio_desc[id].requested) {
                        status = -EBUSY;
                        break;
                }
-       }
 
        if (status == 0) {
-               if (chips[id] == chip)
-                       chips[id] = NULL;
-               else
-                       status = -EINVAL;
+               for (id = chip->base; id < chip->base + chip->ngpio; id++)
+                       gpio_desc[id].chip = NULL;
        }
 
        spin_unlock_irqrestore(&gpio_lock, flags);

@@ -139,35 +143,28 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
  */
 int gpio_request(unsigned gpio, const char *label)
 {
-       struct gpio_chip        *chip;
+       struct gpio_desc        *desc;
        int                     status = -EINVAL;
        unsigned long           flags;
 
        spin_lock_irqsave(&gpio_lock, flags);
 
-       chip = gpio_to_chip(gpio);
-       if (!chip)
-               goto done;
-       gpio -= chip->base;
-       if (gpio >= chip->ngpio)
+       desc = &gpio_desc[gpio];
+       if (desc->chip == NULL)
                goto done;
 
        /* NOTE:  gpio_request() can be called in early boot,
         * before IRQs are enabled.
         */
 
-       status = 0;
+       if (!desc->requested) {
+               desc->requested = 1;
 #ifdef CONFIG_DEBUG_FS
-       if (!label)
-               label = "?";
-       if (chip->requested[gpio])
-               status = -EBUSY;
-       else
-               chip->requested[gpio] = label;
-#else
-       if (test_and_set_bit(gpio, chip->requested))
-               status = -EBUSY;
+               desc->label = (label == NULL) ? "?" : label;
 #endif
+               status = 0;
+       } else
+               status = -EBUSY;
 
 done:
        spin_unlock_irqrestore(&gpio_lock, flags);

@@ -178,33 +175,51 @@ EXPORT_SYMBOL_GPL(gpio_request);
 void gpio_free(unsigned gpio)
 {
        unsigned long           flags;
-       struct gpio_chip        *chip;
+       struct gpio_desc        *desc;
 
        spin_lock_irqsave(&gpio_lock, flags);
 
-       chip = gpio_to_chip(gpio);
-       if (!chip)
-               goto done;
-       gpio -= chip->base;
-       if (gpio >= chip->ngpio)
-               goto done;
-
+       desc = &gpio_desc[gpio];
+       if (desc->chip && desc->requested) {
+               desc->requested = 0;
 #ifdef CONFIG_DEBUG_FS
-       if (chip->requested[gpio])
-               chip->requested[gpio] = NULL;
-       else
-               chip = NULL;
-#else
-       if (!test_and_clear_bit(gpio, chip->requested))
-               chip = NULL;
+               desc->label = NULL;
 #endif
-       WARN_ON(extra_checks && chip == NULL);
-done:
+       } else
+               WARN_ON(extra_checks);
+
        spin_unlock_irqrestore(&gpio_lock, flags);
 }
 EXPORT_SYMBOL_GPL(gpio_free);
 
 
+/**
+ * gpiochip_is_requested - return string iff signal was requested
+ * @chip: controller managing the signal
+ * @offset: controller's identifer
+ *
+ * If debugfs support is enabled, the string returned is the label passed
+ * to gpio_request(); otherwise it is a meaningless constant.  When NULL
+ * is returned, the GPIO is not currently requested.
+ *
+ * This function is for use by GPIO controller drivers.  The label can
+ * help with diagnostics, and knowing that the signal is used as a GPIO
+ * can help ensure it's not accidentally given to another controller.
+ */
+const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
+{
+       unsigned gpio = chip->base + offset;
+
+       if (gpio >= ARCH_NR_GPIOS || gpio_desc[gpio].chip != chip)
+               return NULL;
+#ifdef CONFIG_DEBUG_FS
+       return gpio_desc[gpio].label;
+#else
+       return gpio_desc[gpio].requested ? "?" : NULL;
+#endif
+}
+EXPORT_SYMBOL_GPL(gpiochip_is_requested);
+
 
 /* Drivers MUST make configuration calls before get/set calls
  *

@@ -218,17 +233,18 @@ int gpio_direction_input(unsigned gpio)
 {
        unsigned long           flags;
        struct gpio_chip        *chip;
+       struct gpio_desc        *desc = &gpio_desc[gpio];
        int                     status = -EINVAL;
 
        spin_lock_irqsave(&gpio_lock, flags);
 
-       chip = gpio_to_chip(gpio);
+       chip = desc->chip;
        if (!chip || !chip->get || !chip->direction_input)
                goto fail;
        gpio -= chip->base;
        if (gpio >= chip->ngpio)
                goto fail;
-       gpio_ensure_requested(chip, gpio);
+       gpio_ensure_requested(desc);
 
        /* now we know the gpio is valid and chip won't vanish */
 

@@ -238,7 +254,7 @@ int gpio_direction_input(unsigned gpio)
 
        status = chip->direction_input(chip, gpio);
        if (status == 0)
-               clear_bit(gpio, chip->is_out);
+               desc->is_out = 0;
        return status;
 fail:
        spin_unlock_irqrestore(&gpio_lock, flags);

@@ -250,17 +266,18 @@ int gpio_direction_output(unsigned gpio,
 {
        unsigned long           flags;
        struct gpio_chip        *chip;
+       struct gpio_desc        *desc = &gpio_desc[gpio];
        int                     status = -EINVAL;
 
        spin_lock_irqsave(&gpio_lock, flags);
 
-       chip = gpio_to_chip(gpio);
+       chip = desc->chip;
        if (!chip || !chip->set || !chip->direction_output)
                goto fail;
        gpio -= chip->base;
        if (gpio >= chip->ngpio)
                goto fail;
-       gpio_ensure_requested(chip, gpio);
+       gpio_ensure_requested(desc);
 
        /* now we know the gpio is valid and chip won't vanish */
 

@@ -270,7 +287,7 @@ int gpio_direction_output(unsigned gpio,
 
        status = chip->direction_output(chip, gpio, value);
        if (status == 0)
-               set_bit(gpio, chip->is_out);
+               desc->is_out = 1;
        return status;
 fail:
        spin_unlock_irqrestore(&gpio_lock, flags);

@@ -366,26 +383,23 @@ EXPORT_SYMBOL_GPL(gpio_set_value_canslee
 
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 {
-       unsigned        i;
+       unsigned                i;
+       unsigned                gpio = chip->base;
+       struct gpio_desc        *gdesc = &gpio_desc[gpio];
 
-       for (i = 0; i < chip->ngpio; i++) {
-               unsigned        gpio;
-               int             is_out;
 
-               if (!chip->requested[i])
+       for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
+               if (!gdesc->requested)
                        continue;
 
-               gpio = chip->base + i;
-               is_out = test_bit(i, chip->is_out);
-
                seq_printf(s, " gpio-%-3d (%-12s) %s %s",
-                       gpio, chip->requested[i],
-                       is_out ? "out" : "in ",
+                       gpio, gdesc->label,
+                       gdesc->is_out ? "out" : "in ",
                        chip->get
                                ? (chip->get(chip, i) ? "hi" : "lo")
                                : "?  ");
 
-               if (!is_out) {
+               if (!gdesc->is_out) {
                        int             irq = gpio_to_irq(gpio);
                        struct irq_desc *desc = irq_desc + irq;
 

@@ -435,14 +449,16 @@ static void gpiolib_dbg_show(struct seq_
 
 static int gpiolib_show(struct seq_file *s, void *unused)
 {
-       struct gpio_chip        *chip;
-       unsigned                id;
+       struct gpio_chip        *chip = NULL;
+       unsigned                gpio;
        int                     started = 0;
 
        /* REVISIT this isn't locked against gpio_chip removal ... */

Quoted 7 lines

+static void gpio_ensure_requested(struct gpio_desc *desc)
 {
-       int             requested;
-
+       if (!desc->requested) {
+               desc->requested = 1;
+               pr_warning("GPIO-%ld autorequested\n", gpio_desc - desc);

produces a warning here about %ld,

and maybe you mean desc - gpio_desc??

Except for the above, I feel OK overall.

This is a new-style I2C driver for some common 8 and 16 bit I2C based GPIO expanders: pcf8574 and pcf8575. Since it's a new-style driver, it's configured as part of board-specific init ... eliminating the need for error-prone manual configuration of module parameters.

The driver exposes the GPIO signals using the platform-neutral GPIO programming interface, so they are easily accessed by other kernel code. The lack of such a flexible kernel API is what has ensured the proliferation of board-specific hacks for these chips... stuff that rarely makes it upstream since it's so ugly. This driver will let such board-specific code use standard GPIO calls.

Signed-off-by David Brownell <[email protected]>

---
Note that there's currently a drivers/i2c/chips/pcf8574.c driver.

Key differences include: this one talks to other kernel code so it can use the GPIOs "normally", but that one talks to userspace through sysfs. Also, this one is a "new style" I2C driver, so it's smaller and doesn't need all those error-prone module parameters. Plus, this one handles both 8 and 16 bit chip versions.

 drivers/i2c/chips/Kconfig   |   18 ++
 drivers/i2c/chips/Makefile  |    1 
 drivers/i2c/chips/pcf857x.c |  309 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pcf857x.h     |   43 ++++++
 4 files changed, 371 insertions(+)

drivers/i2c/chips/Kconfig +7 -0

@@ -51,6 +51,24 @@ config SENSORS_EEPROM
 	  This driver can also be built as a module.  If so, the module
 	  will be called eeprom.
 
+config GPIO_PCF857X
+	tristate "PCF875x GPIO expanders"
+	depends on GPIO_LIB
+	help
+	  Say yes here to provide access to some I2C GPIO expanders which
+	  may be used for additional digital outputs or inputs:
+

drivers/i2c/chips/Kconfig +1 -0

+
 config SENSORS_PCF8574
 	tristate "Philips PCF8574 and PCF8574A"
 	depends on EXPERIMENTAL

include/linux/pcf857x.h new file +5 -0

+ *
+ * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
+ * the i2c_board_info used with the pcf875x driver must provide the
+ * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
+ * platform_data (pointer to one of these structures) with at least

include/linux/pcf857x.h new file +15 -0

+ */
+struct pcf857x_platform_data {
+	unsigned	gpio_base;
+	unsigned	n_latch;
+
+	void		*context;
+	int		(*setup)(struct i2c_client *client,
+					int gpio, unsigned ngpio,
+					void *context);
+	int		(*teardown)(struct i2c_client *client,
+					int gpio, unsigned ngpio,
+					void *context);
+};
+
+#endif /* __LINUX_PCF857X_H */

drivers/i2c/chips/pcf857x.c new file +11 -0

+ *
+ * Copyright (C) 2007 David Brownell
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

drivers/i2c/chips/pcf857x.c new file +13 -0

+ * Writing a "one" bit (to match the reset state) lets that pin be used as
+ * an input; it's not an open-drain model, but it acts a bit like that.
+ *
+ * Some other I2C GPIO expander chips (like the pca9534, pca9535, pca9555,
+ * pca9539, mcp23008, and mc23017) have a more complex register model with
+ * more conventional input circuitry, often using 0x20..0x27 addresses.
+ */
+struct pcf857x {
+	struct gpio_chip	chip;
+	struct i2c_client	*client;
+	unsigned		out;		/* software latch */
+};
+

drivers/i2c/chips/pcf857x.c new file +48 -0

+	int status;
+
+	status = i2c_master_send(client, buf, 2);
+	return (status < 0) ? status : 0;
+}
+
+static int i2c_read_le16(struct i2c_client *client)
+{
+	u8 buf[2];
+	int status;
+
+	status = i2c_master_recv(client, buf, 2);
+	if (status < 0)
+		return status;
+	return (buf[1] << 8) | buf[0];
+}
+
+static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
+{
+	struct pcf857x	*gpio = container_of(chip, struct pcf857x, chip);
+
+	gpio->out |= (1 << offset);
+	return i2c_write_le16(gpio->client, (u16) gpio->out);
+}
+
+static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
+{
+	struct pcf857x	*gpio = container_of(chip, struct pcf857x, chip);
+	int		value;
+
+	value = i2c_read_le16(gpio->client);
+	return (value < 0) ? 0 : !!(value & (1 << offset));
+}
+
+static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct pcf857x	*gpio = container_of(chip, struct pcf857x, chip);
+	unsigned	bit = 1 << offset;
+
+	if (value)
+		gpio->out |= bit;
+	else
+		gpio->out &= ~bit;
+	return i2c_write_le16(gpio->client, (u16) gpio->out);
+}
+
+static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
+{

drivers/i2c/chips/pcf857x.c new file +14 -0

+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->chip.base = pdata->gpio_base;
+	gpio->chip.can_sleep = 1;
+
+	/* NOTE:  the OnSemi jlc1562b is also largely compatible with
+	 * these parts, notably for output.  It has a low-resolution
+	 * DAC instead of pin change IRQs; and its inputs can be the
+	 * result of comparators.
+	 */
+
+	/* '74a addresses are 0x38..0x3f; '74 uses 0x20..0x27 */
+	if (strcmp(client->name, "pcf8574a") == 0

drivers/i2c/chips/pcf857x.c new file +7 -0

+
+	/* '75/'75c addresses are 0x20..0x27, just like the '74;
+	 * the '75c doesn't have a current source pulling high.
+	 */
+	} else if (strcmp(client->name, "pcf8575c") == 0
+			|| strcmp(client->name, "pcf8575") == 0) {
+		gpio->chip.ngpio = 16;

drivers/i2c/chips/pcf857x.c new file +59 -0

+	}
+
+	gpio->chip.label = client->name;
+
+	gpio->client = client;
+	i2c_set_clientdata(client, gpio);
+
+	/* NOTE:  these chips have strange "pseudo-bidirectional" I/O pins.
+	 * We can't actually know whether a pin is configured (a) as output
+	 * and driving the signal low, or (b) as input and reporting a low
+	 * value ... without knowing the last value written since the chip
+	 * came out of reset (if any).  We can't read the latched output.
+	 *
+	 * In short, the only reliable solution for setting up pin direction
+	 * is to do it explicitly.  The setup() method can do that.
+	 *
+	 * We use pdata->n_latch to avoid trouble.  In the typical case it's
+	 * left initialized to zero; our software copy of the "latch" then
+	 * matches the chip's reset state.  But there may be cases where a
+	 * system must drive some pins low, without transient glitching.
+	 * Handle those cases by assigning n_latch to a nonzero value.
+	 */
+	gpio->out = ~pdata->n_latch;
+
+	status = gpiochip_add(&gpio->chip);
+	if (status < 0) {
+		kfree(gpio);
+		return status;
+	}
+
+	/* NOTE: these chips can issue "some pin-changed" IRQs, which we
+	 * don't yet even try to use.  Among other issues, the relevant
+	 * genirq state isn't available to modular drivers; and most irq
+	 * methods can't be called from sleeping contexts.
+	 */
+
+	dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
+			gpio->chip.base,
+			gpio->chip.base + gpio->chip.ngpio - 1,
+			client->name,
+			client->irq ? " (irq ignored)" : "");
+
+	/* Let platform code set up the GPIOs and their users.
+	 * Now is the first time anyone can use them.
+	 */
+	if (pdata->setup) {
+		status = pdata->setup(client,
+				gpio->chip.base, gpio->chip.ngpio,
+				pdata->context);
+		if (status < 0)
+			dev_dbg(&client->dev, "setup --> %d\n", status);
+	}
+
+	return 0;
+}
+
+static int pcf857x_remove(struct i2c_client *client)
+{
+	struct pcf857x_platform_data	*pdata = client->dev.platform_data;

drivers/i2c/chips/pcf857x.c new file +25 -0

+			return status;
+		}
+	}
+
+	status = gpiochip_remove(&gpio->chip);
+	if (status == 0)
+		kfree(gpio);
+	else
+		dev_err(&client->dev, "%s --> %d\n", "remove", status);
+	return status;
+}
+
+static struct i2c_driver pcf857x_driver = {
+	.driver = {
+		.name	= "pcf857x",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= pcf857x_probe,
+	.remove	= pcf857x_remove,
+};
+
+static int __init pcf857x_init(void)
+{
+	return i2c_add_driver(&pcf857x_driver);
+}

!!(value & (1 << offset))
is more efficiently written
(value >> offset) & 1

Quoted 3 lines · Jean Delvare

!!(value & (1 << offset))
is more efficiently written
(value >> offset) & 1

... but not more efficiently implemented.

Your version requires code to do the shift on live data at runtime. David's version lets the compiler create the mask once, at compile-time.

Hi Bill,

Quoted 6 lines · Bill Gatliff

Quoted 4 lines · Jean Delvare

!!(value & (1 << offset))
is more efficiently written
(value >> offset) & 1

Quoted 4 lines

... but not more efficiently implemented.

Your version requires code to do the shift on live data at runtime. David's version lets the compiler create the mask once, at compile-time.

I don't understand. How could the compiler create the mask at compile-time when "offset" is only known at runtime?

Hi David,

Sorry for the late review.

Quoted 6 lines

drivers/i2c/chips/Kconfig +0 -0

@@ -51,6 +51,24 @@ config SENSORS_EEPROM
 	  This driver can also be built as a module.  If so, the module
 	  will be called eeprom.
 

drivers/i2c/chips/Kconfig config SENSORS_EEPROM · line 70

Quoted 8 lines

drivers/i2c/chips/Kconfig +8 -0

+config GPIO_PCF857X
+	tristate "PCF875x GPIO expanders"
+	depends on GPIO_LIB
+	help
+	  ...
+
+	  This driver provides only an in-kernel interface to those GPIOs.
+	  Any sysfs interface to userspace would be provided separately.

How?

drivers/i2c/chips/Kconfig config SENSORS_EEPROM · line 65

Quoted 4 lines

drivers/i2c/chips/Kconfig +4 -0

+/**
+ * struct pcf857x_platform_data - data to set up pcf857x driver
+ * @gpio_base: number of the chip's first GPIO
+ * @n_latch: optional bit-inverse of initial output state

Strange name, and I can't make much sense of the description either.

After reading this paragraph I still have no idea what n_latch does. But maybe that's just me.

drivers/i2c/chips/Makefile obj-$(CONFIG_SENSORS_M41T00) += m41t00.o · line 14

Quoted 7 lines

drivers/i2c/chips/Makefile +1 -0

@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o

For alphabetical order, it would go one line above.

drivers/i2c/chips/pcf857x.c line 2

Quoted 8 lines

drivers/i2c/chips/Makefile +0 -0

 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_SENSORS_TLV320AIC23) += tlv320aic23.o

drivers/i2c/chips/pcf857x.c new file +2 -0

@@ -0,0 +1,309 @@
+/*
+ * pcf857x - driver for pcf857{4,4a,5,5c} I2C GPIO expanders

I recommend spelling out chip names completely, as it lets people grep the kernel tree for chip names when they look for support.

drivers/i2c/chips/pcf857x.c line 23

Quoted 1 line

drivers/i2c/chips/pcf857x.c new file +1 -0

+#include <linux/pcf857x.h>

I suspect that there will be many more such header files in the future. Would it make sense to move them to include/linux/gpio?

drivers/i2c/chips/pcf857x.c line 79

Quoted 7 lines

drivers/i2c/chips/pcf857x.c new file +7 -0

+static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
+	...
+
+static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
+{
+	pcf857x_output8(chip, offset, value);
+}

It would be more efficient to drop pcf857x_set8 altogether and do gpio->chip.set = pcf857x_output8.

drivers/i2c/chips/pcf857x.c line 274

Quoted 2 lines

drivers/i2c/chips/pcf857x.c new file +2 -0

+                     dev_err(&client->dev, "%s --> %d\n",
+                                     "teardown", status);

Why %s instead of hard-coding "teardown"?

Quoted 6 lines · Jean Delvare

Quoted 6 lines

drivers/i2c/chips/Kconfig +0 -0

@@ -51,6 +51,24 @@ config SENSORS_EEPROM
 	  This driver can also be built as a module.  If so, the module
 	  will be called eeprom.
 

Quoted 10 lines

drivers/i2c/chips/Kconfig config SENSORS_EEPROM · line 70

Quoted 8 lines

drivers/i2c/chips/Kconfig +8 -0

+config GPIO_PCF857X
+	tristate "PCF875x GPIO expanders"
+	depends on GPIO_LIB
+	help
+	  ...
+
+	  This driver provides only an in-kernel interface to those GPIOs.
+	  Any sysfs interface to userspace would be provided separately.

How?

I'll take that out, to avoid the question. The answer is still mostly TBD, but the gpiolib infrastructure provides a number of the hooks that such a userspace interface would need.

Quoted 6 lines

drivers/i2c/chips/Kconfig config SENSORS_EEPROM · line 65

Quoted 4 lines

drivers/i2c/chips/Kconfig +4 -0

+/**
+ * struct pcf857x_platform_data - data to set up pcf857x driver
+ * @gpio_base: number of the chip's first GPIO
+ * @n_latch: optional bit-inverse of initial output state

Strange name, and I can't make much sense of the description either.

Updated description:

* @n_latch: optional bit-inverse of initial register value; if
* you leave this initialized to zero, the driver will treat
* all bits as inputs as if the chip was just reset

This chip is documented as being "pseudo-bidirectional", which is a sign that there are some confusing mechanisms lurking...

Conventions for naming negative-true signals include a "#" suffix (illegal for C), a overbar (not expressible in ASCII), and prefixes including "/" (illegal for C) and "n" (aha!). I morphed the latter into "n_" since it's often paired with all-caps signal names, as in "nRESET", which are bad kernel coding style.

Latches hold values; http://en.wikipedia.org/wiki/Latch_%28electronics%29
talks about bit-level latching, but GPIO controllers use register-wide latches to record the value that should be driven on output pins. (As opposed to input pins, whose values are read without latching.)

Quoted 2 lines

After reading this paragraph I still have no idea what n_latch does. But maybe that's just me.

It's a wierd little arrangement, maybe you have a better explanation. I tried hitting the confusing points more directly:

* These GPIO chips are only "pseudo-bidirectional"; read the chip specs
* to understand the behavior. They don't have separate registers to
* record which pins are used for input or output, record which output
* values are driven, or provide access to input values. That must all
* be inferred by reading the chip's value and knowing the last value
* written to it. If you don't initialize n_latch, that last written
* value is presumed to be all ones (as if the chip were just reset).

Quoted 9 lines

drivers/i2c/chips/Makefile obj-$(CONFIG_SENSORS_M41T00) += m41t00.o · line 14

Quoted 7 lines

drivers/i2c/chips/Makefile +1 -0

@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o

For alphabetical order, it would go one line above.

For alphabetical order it would go much sooner.
GPIO precedes SENSOR. ;)

Quoted 4 lines

drivers/i2c/chips/Makefile obj-$(CONFIG_SENSORS_M41T00) += m41t00.o · line 15

Quoted 1 line

drivers/i2c/chips/Makefile +1 -0

+#include <linux/pcf857x.h>

I suspect that there will be many more such header files in the future. Would it make sense to move them to include/linux/gpio?

I was thinking more like <linux/i2c/...> myself. There are many more I2C chips than GPIO expanders.

Quoted 10 lines

drivers/i2c/chips/Makefile obj-$(CONFIG_SENSORS_M41T00) += m41t00.o

Quoted 7 lines

drivers/i2c/chips/Makefile +7 -0

+static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
+	...
+
+static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
+{
+	pcf857x_output8(chip, offset, value);
+}

It would be more efficient to drop pcf857x_set8 altogether and do gpio->chip.set = pcf857x_output8.

No can do; return types differ, which means that on some platforms the calling conventions have significant differences.

Quoted 4 lines

Quoted 2 lines

+                     dev_err(&client->dev, "%s --> %d\n", +                                     "teardown", status);

Why %s instead of hard-coding "teardown"?

To share (current code) three copies of the "<3>%s %s: %s --> %d\n" string. Every little bit of kernel bloat prevention helps. ;)

So the user-space interface would be part of the generic GPIO infrastructure? I like the idea.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

+#include <linux/pcf857x.h>

I suspect that there will be many more such header files in the future. Would it make sense to move them to include/linux/gpio?

I was thinking more like <linux/i2c/...> myself. There are many more I2C chips than GPIO expanders.

But most i2c chip drivers don't need a header file. Or is this going to change with the new-style i2c drivers?

Along the same line, I am wondering if it would make sense to put the various GPIO drivers in drivers/gpio.

Quoted 3 lines · Jean Delvare

So the user-space interface would be part of the generic GPIO infrastructure? I like the idea.

I thought that would make sense too! :) Someone would need to write the code though. Having such a mechanism would provide another "carrot" to migrate folk towards the gpiolib core.

From c65d0fb239b79de7f595e47edb2fb641217e7309 Mon Sep 17 00:00:00 2001

Quoted 5 lines · David Brownell

Quoted 3 lines · Jean Delvare

So the user-space interface would be part of the generic GPIO infrastructure? I like the idea.

Quoted 3 lines

I thought that would make sense too! :) Someone would need to write the code though. Having such a mechanism would provide another "carrot" to migrate folk towards the gpiolib core.

Here's some code to do this. It's not entirely perfect yet, but it is usable.

Quoted 10 lines · Trent Piepho

Quoted 8 lines · David Brownell

Quoted 3 lines · Jean Delvare

So the user-space interface would be part of the generic GPIO infrastructure? I like the idea.

I thought that would make sense too! :) Someone would need to write the code though. Having such a mechanism would provide another "carrot" to migrate folk towards the gpiolib core.

Quoted 2 lines

Here's some code to do this. It's not entirely perfect yet, but it is usable.

I quite like the fact that this easily tracks labels but I like the interface of simple_gpio posted a few days back:
http://lkml.org/lkml/2008/3/26/87

Either way, anything unified is good.

Always too slow posting my patches. I wrote this two months ago when there wasn't anything else.

A char device allows better permissions and could be more efficient, if one really wants to do extensive control of gpio lines from userspace. I can see how it might be preferrable in some instances.

The nice thing about sysfs is that you don't need any extra software to interact with it. It's very convienent when you're just trying to debug the gpio driver you're writing or verify that the gpio lines you just connected are doing things. It's also nice to be able to say something like:

# run these commands to un-write protect flash
echo out > /sys/class/gpio/MPC85XX:5/direction
echo 1 > /sys/class/gpio/MPC85XX:5/value

Instead of a complicated process that includes directions for creating the correct device file, compiling a program that will set gpio lines, downloading said program's source, and so on.

David, you're kinda the gatekeeper here; any input from you on which approach is to be preferred, essential features etc?

- Trent's gpio_class exposes all GPIOs, even ones
that are claimed by kernel drivers ... and thus
makes it easy to clobber kernel driver state.

(Plus it won't work on most built-in GPIOs, since they by and large don't have parent devices.)

From my understanding of sysfs, it seems any sysfs based approach has to

What I'd like to see is userspace config commands to cause the gpio_request() ... *maybe* something like

Plus, there should be some way to cause gpio_free() too.

Quoted 8 lines · David Brownell

Quoted 3 lines · Jean Delvare

So the user-space interface would be part of the generic GPIO infrastructure? I like the idea.

I thought that would make sense too! :) Someone would need to write the code though. Having such a mechanism would provide another "carrot" to migrate folk towards the gpiolib core.

Here's some code to do this. It's not entirely perfect yet, but it is usable. I create a directory in sysfs (class/gpio/<name>:<num>) for each gpio line. The are files in this directory to allow reading/setting the gpio value and direction. One can also see the label associated with the gpio if gpiolib is keeping track of labels (i.e. debugfs is on). sysfs also creates a gpio directory under the device associated with the gpio lines (this is a new thing one needs to add) with that device's gpios. Actually, think these are "real" sysfs directories, and the ones in /sys/class are copies made by sysfs.

----------------------------------------------------------------------------
From c65d0fb239b79de7f595e47edb2fb641217e7309 Mon Sep 17 00:00:00 2001
From: Trent Piepho <[email protected]>
Date: Thu, 3 Apr 2008 18:37:23 -0700
Subject: GPIO: Create a sysfs gpio class for messing with GPIOs from userspace

struct gpio_chip gets a new field, dev, which points to the struct device of whatever the gpio lines are part of. If this is non-NULL, gpio class entries are created for this device when gpiochip_add() is called, by a new function gpiochip_classdev_register().

It creates a sysfs directory for each gpio the chip defines. Each device has three attributes so far, direction, value, and label. label is read-only, and will only be present if DEBUG_FS is on, as without DEBUG_FS the gpio layer doesn't keep track of any labels.

Maybe the value file should be changed to RO or WO depending on direction?

Setting the direction auto-allocates the gpio, which is not really what I wanted. Maybe I should call the chip set method directly?

There are almost certainly a bunch of races with gpio_desc access.

No code has been written yet to remove the devices from the class when the class is removed.

The GPIO_CLASS define/ifdef code should either be removed, or turned into a Kconfig variable that can be used turn this feature on and off.

Signed-off-by Trent Piepho <[email protected]>

---
  drivers/gpio/gpiolib.c     |  188 ++++++++++++++++++++++++++++++++++++++++++++
  include/asm-generic/gpio.h |    2 +
  2 files changed, 190 insertions(+), 0 deletions(-)

drivers/gpio/gpiolib.c +10 -0

index d8db2f8..db0677d 100644
@@ -18,6 +18,7 @@
   * only an instruction or two per bit.
   */
 
+#define GPIO_CLASS	1
 
  /* When debugging, extend minimal trust to callers and platform code.
   * Also emit diagnostic messages that may help initial bringup, when

@@ -47,6 +48,9 @@ struct gpio_desc {
  #ifdef CONFIG_DEBUG_FS
  	const char		*label;
  #endif
+#if GPIO_CLASS
+	struct device		*dev;
+#endif
  };
  static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 

@@ -57,6 +61,174 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
  #endif
  }
 
+#if GPIO_CLASS
+#include <linux/device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/err.h>
+

drivers/gpio/gpiolib.c +11 -0

+	}
+
+	if (d)
+		gpio_direction_output(n, 0);
+	else
+		gpio_direction_input(n);
+
+	return size;
+}
+
+static DEVICE_ATTR(direction, 0644, gpio_direction_show, gpio_direction_store);

drivers/gpio/gpiolib.c +6 -0

+	gpio_set_value(n, v);
+
+	return size;
+}
+
+static DEVICE_ATTR(value, 0644, gpio_value_show, gpio_value_store);

drivers/gpio/gpiolib.c +63 -0

+	} else
+		return sprintf(buf, "%s\n", gdesc->label) + 1;
+}
+
+static DEVICE_ATTR(label, 0444, gpio_label_show, NULL);
+#endif
+
+static int gpiochip_classdev_register(const struct gpio_chip *chip)
+{
+	int ret, i;
+	struct gpio_desc *gdesc;
+
+	BUG_ON(!chip->dev);
+
+	for (i = chip->base; i < chip->base + chip->ngpio; i++) {
+		gdesc = gpio_desc + i;
+		gdesc->dev = device_create(gpio_class, chip->dev, 0, "%s:%d",
+					   chip->label, i);
+		if (IS_ERR(gdesc->dev)) {
+			ret = PTR_ERR(gdesc->dev);
+			i--;
+			goto fail;
+		}
+
+		dev_set_drvdata(gdesc->dev, gdesc);
+
+		ret = device_create_file(gdesc->dev, &dev_attr_direction);
+		if (ret)
+			goto fail_dev;
+		ret = device_create_file(gdesc->dev, &dev_attr_value);
+		if (ret)
+			goto fail_dir;
+#ifdef CONFIG_DEBUG_FS
+		ret = device_create_file(gdesc->dev, &dev_attr_label);
+		if (ret)
+			goto fail_value;
+#endif
+	}
+	return 0;
+
+fail:
+	for (; i >= chip->base; i--) {
+		gdesc = gpio_desc + i;
+
+#ifdef CONFIG_DEBUG_FS
+		device_remove_file(gdesc->dev, &dev_attr_label);
+
+fail_value:
+#endif
+		device_remove_file(gdesc->dev, &dev_attr_value);
+
+fail_dir:
+		device_remove_file(gdesc->dev, &dev_attr_direction);
+
+fail_dev:
+		device_unregister(gdesc->dev);
+		gdesc->dev = NULL;
+	}
+	return ret;
+}
+
+static int __init gpio_class_init(void)
+{

drivers/gpio/gpiolib.c +14 -0

+	class_destroy(gpio_class);
+}
+subsys_initcall(gpio_class_init);
+module_exit(gpio_class_exit);
+
+#else /* no class */
+
+/* I coulda been a contender, I coulda had class... */
+static inline int gpiochip_classdev_register(const struct gpio_chip *chip)
+{ return 0; }
+
+#endif
+
+
  /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
   * when setting direction, and otherwise illegal.  Until board setup code
   * and drivers use explicit requests everywhere (which won't happen when

drivers/gpio/gpiolib.c +10 -0

+		status = gpiochip_classdev_register(chip);
+		if (status) {
+			/* De-allocate GPIOs */
+			spin_lock_irqsave(&gpio_lock, flags);
+			for (id = chip->base; id < chip->base + chip->ngpio; id++)
+				gpio_desc[id].chip = NULL;
+			spin_unlock_irqrestore(&gpio_lock, flags);
+		}
+	}
+
  fail:
  	/* failures here can mean systems won't boot... */
  	if (status)

include/asm-generic/gpio.h +0 -0

index f29a502..b2a5262 100644

include/asm-generic/gpio.h +1 -0

   * @base: identifies the first GPIO number handled by this chip; or, if
   *	negative during registration, requests dynamic ID allocation.
   * @ngpio: the number of GPIOs handled by this controller; the last GPIO

@@ -59,6 +60,7 @@ struct gpio_chip {
  						unsigned offset, int value);
  	void			(*dbg_show)(struct seq_file *s,
  						struct gpio_chip *chip);
+	struct device		*dev;
  	int			base;
  	u16			ngpio;
  	unsigned		can_sleep:1;