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.
One open issue is how to handle IRQs reported through GPIO expanders.  For

example, I2C chips may be able to report IRQs, but the genirq framework

won't much like the need to manage them in can-sleep contexts or the way

their irq_chip structures can be removed.
This opens the door to an augmented programming interface, addressing GPIOs

by chip and index.  That could be used as a performance tweak (lookup once

and cache, avoiding locking and lookup overheads) or to support transient

GPIOs which aren't registered in the integer GPIO namespace (a USB-to-GPIO

adapter, GPIOs coupled to some other type of add-on card, etc).
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

---

 Documentation/gpio.txt     |   12 -

 include/asm-generi...
[ message continues ]

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 ma...
[ message continues ]

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
--

Cheers

- eric

-

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
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
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
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,
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 lo...
[ message continues ]

well, I don't think holes are problems, but think about the restriction

ARCH_GPIOS_PER_CHIP enforces the numbering of GPIOs, don't

you think we need a more flexible numbering scheme, so one can
well, I don't see much benefit for now, either. But binding/unbinding

the gpio_chip to the gpio_desc could possibly be made locking free
--

Cheers

- eric

-

... 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
--

Cheers

- eric

-

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?
One point you haven't really brought up in this thread is

your concern about the impact of this on IRQs.  One issue

being that for GPIOs used as IRQs, with linear mappings

resembling
	static inline int gpio_to_irq(unsigned gpio)

	{

		if (gpio >= LAST_IRQ_CAPABLE_GPIO)

			return -EINVAL;

		return irq + FIRST_GPIO_IRQ_NUMBER;

	}
then tightly packed GPIOs mean less space wasted for IRQ

descriptors that would never be used.
And since an irq_desc bigger than your gpio_desc, there's

a tradeoff between wasting space on unused gpio_desc structs

versus unused irq_desc structs.  2 KBytes would cost about

only 35 irq_desc structs, vs 256 gpio_desc structs.
I'm guessing that's why you care about dense packing for

the GPIO numbers...
- Dave
-

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

-

OK, here's the all-in-one patch based on David's most recent gpiolib fix,
Changes include:
1. use per gpio structure "gpio_desc", thus eliminating the restriction of

   ARCH_GPIOS_PER_CHIP, thus making it possible leaving no holes in GPIOs

   numbering
   Note: the number of GPIOs on different GPIO expander chips may vary

   much, from 1-2 GPIOs on some audio codecs to dedicated 16-pin GPIO

   expander chips. The ARCH_GPIOS_PER_CHIP might not be fair enough.
   this change introduces the following small changes:
   a) removal of "gpio_chips[]" and use "gpio_desc[]" instead

   b) move of "is_out" and "requested" from "struct gpio_chip" to "struct

      gpio_desc" and make them bit fields

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

      instead
   Note: I do want a reference count field within gpio_chip to record the

   number of requested GPIOs within the chip, leave this to next patch
   d) make gpio_ensure_requested() use gpio_desc, and simplify the code a

      bit
2. reduce ARCH_NR_GPIOS to 256, which is moderate for most sane platforms,

   and thus saving some memory of the per gpio structures
3. dedicated "gpio_desc.requested_label" field for use with debugfs
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
------ >8 -------
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

index 869b739..e3b2c8f 100644

--- a/include/asm-generic/gpio.h

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

@@ -14,14 +14,20 @@

  */
 #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;

+struct gpio_chip;

+

+struct gpio_desc {

+	struct gpio_chip	*chip;

+	unsigned		is_out:1;

+	unsigned		requested:1;

+#ifdef CONFIG_DEBUG_FS

+	const char		*reques...
[ message continues ]

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(-)
--- at91.orig/arch/arm/mach-at91/gpio.c	2007-11-27 14:31:05.000...
[ message continues ]

produces a warning here about %ld, and maybe you mean
Except for the above, I feel OK overall.
--

Cheers

- eric

-

It uses "%ld" since I got a warning with "%d".  :(
-

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.

-

Sorry, I thought you want a preliminary one, here's the one

tested and including your comments except for one:
if caller holds gpio_lock, gpio_ensure_requested() is actually

safe.
please review:
---

 include/asm-generic/gpio.h |   35 +++-----

 lib/gpiolib.c              |  212 +++++++++++++++++++-------------------------

 2 files changed, 104 insertions(+), 143 deletions(-)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

index 869b739..34e60ba 100644

--- a/include/asm-generic/gpio.h

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

@@ -14,14 +14,22 @@

  */
 #ifndef ARCH_NR_GPIOS

-#define ARCH_NR_GPIOS		512

+#define ARCH_NR_GPIOS		256

 #endif
-#ifndef ARCH_GPIOS_PER_CHIP

-#define ARCH_GPIOS_PER_CHIP	BITS_PER_LONG

+struct seq_file;

+struct gpio_chip;

+

+struct gpio_desc {

+	struct gpio_chip	*chip;

+	unsigned		is_out:1;

+	unsigned		requested:1;

+#ifdef CONFIG_DEBUG_FS

+	const char		*requested_label;

 #endif

+};
-struct seq_file;

+extern struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 /**

  * struct gpio_chip - abstract a GPIO controller

@@ -41,8 +49,6 @@ struct seq_file;

  *	(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,17 +76,6 @@ 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;

@@ -89,11 +84...
[ message continues ]

That's problematic for GPIO code that needs to know if a given

GPIO has been requested ... since you don't export gpio_desc[]

(and shouldn't) that functionality has effectively been removed!
So that would break two patches now in MM (mcp23s08 support, and
That's problematic too ... since it no longer actually ensures

that the chip gets marked as requested.  :(
Since struct gpio_desc isn't exported, it might as well be

private to lib/gpiolib.c instead of public in the header...

-

Thanks, I'll be looking at it ... the one thing I strongly dislike is

this change:
That removes the ability to display all kinds of significant

chip-specific state ... like whether a given signal has active

pullups or pulldowns, uses open-drain signaling, and so forth.
It also makes access a lot slower ... e.g. rather than one

batch of I2C or SPI operations for all N signals on a chip,

it's got to do N batches.
Plus it just needlessly breaks existing code.
I'll put together a version without that problem.
- Dave

-

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
---

 lib/gpiolib.c |   32 ++++++++++++++++++++++----------

 1 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/lib/gpiolib.c b/lib/gpiolib.c

index f2ae689..c627efb 100644

--- a/lib/gpiolib.c

+++ b/lib/gpiolib.c

@@ -33,6 +33,14 @@ static DEFINE_SPINLOCK(gpio_lock);

 static struct gpio_chip *chips[DIV_ROUND_UP(ARCH_NR_GPIOS,

 					ARCH_GPIOS_PER_CHIP)];
+static inline int gpio_is_onchip(unsigned gpio, struct gpio_chip *chip)

+{

+	if (chip && gpio >= chip->base && gpio < chip->base + chip->ngpio)

+		return 1;

+	else

+		return 0;

+}

+

 /* Warn when drivers omit gpio_request() calls -- legal but

  * ill-advised when setting direction, and otherwise illegal.

  */

@@ -139,11 +147,11 @@ int gpio_request(unsigned gpio, const char *label)
 	spin_lock_irqsave(&gpio_lock, flags);

 	chip = gpio_to_chip(gpio);

-	if (!chip)

+

+	if (!gpio_is_onchip(gpio, chip))

 		goto done;

+

 	gpio -= chip->base;

-	if (gpio >= chip->ngpio)

-		goto done;
 	/* NOTE:  gpio_request() can be called in early boot,

 	 * before IRQs are enabled.

@@ -176,11 +184,11 @@ void gpio_free(unsigned gpio)

 	spin_lock_irqsave(&gpio_lock, flags);
 	chip = gpio_to_chip(gpio);

-	if (!chip)

+

+	if (!gpio_is_onchip(gpio, chip))

 		goto done;

+

 	gpio -= chip->base;

-	if (gpio >= chip->ngpio)

-		goto done;
 #ifdef CONFIG_DEBUG_FS

 	if (chip->requested[gpio])

@@ -218,9 +226,11 @@ int gpio_direction_input(unsigned gpio)

 	chip = gpio_to_chip(gpio);

 	if (!chip || !chip->get || !chip->direction_input)

 		goto fail;

-	gpio -= chip->base;

-	if (gpio >= chip->ngpio)

+

+	if (!gpio_is_onchip(gpio, chip))

 		goto fail;

+

+	gpio -= chip->base;

 	gpio_ensure_requested(chip, gpio);
 	/* now we know ...
[ message continues ]

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
-

just a style issue, moving something commonly done into

a routine, and extra_checks could be put there instead
--

Cheers

- eric

-

-ENOPATCH ... ;)
==========

Minor fixups to the gpiolib code.
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

---

 lib/gpiolib.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
--- g26.orig/lib/gpiolib.c	2007-11-12 15:06:45.000000000 -0800

+++ g26/lib/gpiolib.c	2007-11-12 15:07:36.000000000 -0800

@@ -28,7 +28,7 @@

 #define	extra_checks	0

 #endif
-/* gpio_lock protects the table of chips and to gpio_chip->requested.

+/* gpio_lock protects the table of chips and gpio_chip->requested.

  * While any gpio is requested, its gpio_chip is not removable.  It's

  * a raw spinlock to ensure safe access from hardirq contexts, and to

  * shrink bitbang overhead:  per-bit preemption would be very wrong.

@@ -533,6 +533,6 @@ static int __init gpiolib_debugfs_init(v

 				NULL, NULL, &gpiolib_operations);

 	return 0;

 }

-postcore_initcall(gpiolib_debugfs_init);

+subsys_initcall(gpiolib_debugfs_init);
 #endif	/* DEBUG_FS */
-

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
---

 include/asm-generic/gpio.h |   11 ++---------

 lib/gpiolib.c              |   34 ++++++++++++++--------------------

 2 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

index d00a287..ba3e336 100644

--- a/include/asm-generic/gpio.h

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

@@ -73,13 +73,10 @@ struct gpio_chip {
 	/* other fields are modified by the gpio library only */

 	DECLARE_BITMAP(is_out, ARCH_GPIOS_PER_CHIP);

+	DECLARE_BITMAP(requested, 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);

+	const char		*requested_str[ARCH_GPIOS_PER_CHIP];

 #endif

 };
@@ -89,11 +86,7 @@ struct gpio_chip {

 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

 }
 /* add/remove chips */

diff --git a/lib/gpiolib.c b/lib/gpiolib.c

index c627efb..d52c7f1 100644

--- a/lib/gpiolib.c

+++ b/lib/gpiolib.c

@@ -48,12 +48,11 @@ static void gpio_ensure_requested(struct gpio_chip

*chip, unsigned offset)

 {

 	int		requested;
+	requested = test_and_set_bit(offset, chip->requested);

+

 #ifdef CONFIG_DEBUG_FS

-	requested = (int) chip->requested[offset];

 	if (!requested)

-		chip->requested[offset] = "(auto)";

-#else

-	requested = test_and_set_bit(offset, chip->requested);

+		chip->requested_str[offset] = "(auto)";

 #endif
 	if (!requested)

@@ -158,16 +157,13 @@ int gpio_request(unsigned gpio, const char *label)

 	 */
 	status = 0;

-#ifdef CONFIG_DEBUG_FS

-	if (!label)

-		label = "?";

-	if (chip->requested[gpio])

...
[ message continues ]

Doesn't seem unreasonable, since the extra cost is only

one bit per GPIO (and that would be paid only when debugfs

is available).  What can I say ... I'd rather not pay the

extra memory space.  :)
-

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
---

 include/asm-generic/gpio.h |    7 +++++

 lib/gpiolib.c              |   64 ++++++++++++++++++++++----------------------

 2 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

index ba3e336..783adcf 100644

--- a/include/asm-generic/gpio.h

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

@@ -23,6 +23,12 @@
 struct seq_file;
+struct gpio_chip;

+

+struct gpio_desc {

+	struct gpio_chip *chip;

+};

+

 /**

  * struct gpio_chip - abstract a GPIO controller

  * @label: for diagnostics

@@ -76,6 +82,7 @@ struct gpio_chip {

 	DECLARE_BITMAP(requested, ARCH_GPIOS_PER_CHIP);
 #ifdef CONFIG_DEBUG_FS

+	struct list_head	node;

 	const char		*requested_str[ARCH_GPIOS_PER_CHIP];

 #endif

 };

diff --git a/lib/gpiolib.c b/lib/gpiolib.c

index d52c7f1..57e0d10 100644

--- a/lib/gpiolib.c

+++ b/lib/gpiolib.c

@@ -25,13 +25,12 @@

 #define	extra_checks	0

 #endif
-/* 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.

- */

 static DEFINE_SPINLOCK(gpio_lock);

-static struct gpio_chip *chips[DIV_ROUND_UP(ARCH_NR_GPIOS,

-					ARCH_GPIOS_PER_CHIP)];

+struct gpio_desc gpio_desc[ARCH_NR_GPIOS];

+

+#ifdef CONFIG_DEBUG_FS

+static LIST_HEAD(gpio_chip_list);

+#endif
 static inline int gpio_is_onchip(unsigned gpio, struct gpio_chip *chip)

 {

@@ -63,7 +62,7 @@ static void gpio_ensure_requested(struct gpio_chip

*chip, unsigned offset)

 /* caller holds gpio_lock */

 static inline struct gpio_chip *gpio_to_chip(unsigned gpio)

 {

-	return chips[gpio / ARCH_GPIOS_PER_CHIP];

+	return gpio_desc[gpio].chip;

 }
 /**

@@ -78,7 +77,6 @@ static inline struct gpio_chip *gpio_to_chip(unsigned gpio)

 int gpiochip_add(struct gpio_chip *chip)

 {

 	unsigned long	flags;

-	int		status = 0;

 	unsigned	id;
 	if (chip...
[ message continues ]

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?
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.
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

-

I'd prefer to name it "gpio_desc_lock" instead, which is self-explanatory
absolutely, you get the same feeling of mine and since this is for illustration
--

Cheers

- eric

-

Subject: [PATCH] move per GPIO "is_out" to "struct gpio_desc"
---

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

 lib/gpiolib.c              |   18 +++++++++++-------

 2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

index 783adcf..da67038 100644

--- a/include/asm-generic/gpio.h

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

@@ -27,6 +27,7 @@ struct gpio_chip;
 struct gpio_desc {

 	struct gpio_chip *chip;

+	unsigned is_out:1;

 };
 /**

@@ -47,8 +48,6 @@ struct gpio_desc {

  *	(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.

@@ -78,7 +77,6 @@ struct gpio_chip {

 	unsigned		can_sleep:1;
 	/* other fields are modified by the gpio library only */

-	DECLARE_BITMAP(is_out, ARCH_GPIOS_PER_CHIP);

 	DECLARE_BITMAP(requested, ARCH_GPIOS_PER_CHIP);
 #ifdef CONFIG_DEBUG_FS

diff --git a/lib/gpiolib.c b/lib/gpiolib.c

index 57e0d10..a089597 100644

--- a/lib/gpiolib.c

+++ b/lib/gpiolib.c

@@ -217,11 +217,14 @@ int gpio_direction_input(unsigned gpio)

 {

 	unsigned long		flags;

 	struct gpio_chip	*chip;

+	struct gpio_desc	*desc;

 	int			status = -EINVAL;
 	spin_lock_irqsave(&gpio_lock, flags);
-	chip = gpio_to_chip(gpio);

+	desc = &gpio_desc[gpio];

+	chip = desc->chip;

+

 	if (!chip || !chip->get || !chip->direction_input)

 		goto fail;
@@ -238,8 +241,7 @@ int gpio_direction_input(unsigned gpio)

 	might_sleep_if(extra_checks && chip->can_sleep);
 	status = chip->direction_input(chip, gpio);

-	if (status == 0)

-		clear_bit(gpio, chip->is_out);

+	desc->is_out = 0;

 	return status;

 fail:

 	spin_unlock_ir...
[ message continues ]

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

 include/asm-generic/gpio.h |   17 +++---------

 lib/gpiolib.c              |   62 ++++++++++++++++++++++++-------------------

 2 files changed, 39 insertions(+), 40 deletions(-)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

index da67038..7e70c67 100644

--- a/include/asm-generic/gpio.h

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

@@ -28,6 +28,10 @@ struct gpio_chip;

 struct gpio_desc {

 	struct gpio_chip *chip;

 	unsigned is_out:1;

+	unsigned requested:1;

+#ifdef CONFIG_DEBUG_FS

+	const char *requested_str;

+#endif

 };
 /**

@@ -76,24 +80,11 @@ struct gpio_chip {

 	u16			ngpio;

 	unsigned		can_sleep:1;
-	/* other fields are modified by the gpio library only */

-	DECLARE_BITMAP(requested, ARCH_GPIOS_PER_CHIP);

-

 #ifdef CONFIG_DEBUG_FS

 	struct list_head	node;

-	const char		*requested_str[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)

-{

-	return test_bit(offset, chip->requested);

-}

-

 /* add/remove chips */

 extern int gpiochip_add(struct gpio_chip *chip);

 extern int __must_check gpiochip_remove(struct gpio_chip *chip);

diff --git a/lib/gpiolib.c b/lib/gpiolib.c

index a089597..f9e1c88 100644

--- a/lib/gpiolib.c

+++ b/lib/gpiolib.c

@@ -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)

 {

-	int		requested;

+	int requested;
-	requested = test_and_set_bit(offset, chip->requested);

+	requested = gpio_desc[gpio].requested;
 #ifdef CONFIG_DEBUG_FS

 	if (!requested)

-		chip->requested_st...
[ message continues ]

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
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.

-

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.
--

Jean Delvare

-

It's perfectly correct.  That it's an idiom you don't
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.
- Dave

-

Hi David,
Were you trying to be funny or something? You aren't really suggesting

that I don't know what a debug level is, and how pr_debug() works, are
It's perfectly correct: developers can add "#define DEBUG" manually

before they build the code. That it's an idiom you don't seem to *like*

is distinct from correctness.
OK, can we stop now? David, you need to learn how to work with the

community. The way you keep discussing every word of every reply is

very unpleasant and not constructive at all. Whenever someone reviews

your code, he or she is giving you something. You should be thankful

for the help you received. Instead of that, it looks like you try to

defend yourself against him/her. Please stop working against people

reviewing your code, and start working _with_ them.
Thanks,

--

Jean Delvare

-

Note this reduces the memory in gpio_chip, so it consumes almost same
Not really, since it reduces the holes. That all depend on your

ARCH_NR_GPIOS.
--

Cheers

- eric

-

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

-

Y, the IRQ <--> GPIO mapping is another thing I'm concerned about. Other than

that, all the other part of the gpiolib is a great work, actually,

I've been waiting

for this for quite a long time and just don't have time for a hands-on until

recently.
So let's get more feedback on this.
--

Cheers

- eric

-