Message-Id: <200711271915.26096.david-b@pacbell.net>
Date:	Tue, 27 Nov 2007 19:15:25 -0800
From:	David Brownell <david-b@...bell.net>
To:	"Linux Kernel list" <linux-kernel@...r.kernel.org>
Cc:	"eric miao" <eric.y.miao@...il.com>,
	"Felipe Balbi" <felipebalbi@...rs.sourceforge.net>,
	"Bill Gatliff" <bgat@...lgatliff.com>,
	"Haavard Skinnemoen" <hskinnemoen@...el.com>,
	"Andrew Victor" <andrew@...people.com>,
	"Tony Lindgren" <tony@...mide.com>,
	"Jean Delvare" <khali@...ux-fr.org>,
	"Kevin Hilman" <khilman@...sta.com>,
	"Paul Mundt" <lethal@...ux-sh.org>,
	"Ben Dooks" <ben@...nity.fluff.org>, Nicolas Pitre <nico@....org>
Subject: [patch/rfc 2.6.24-rc3-mm] gpiolib grows a gpio_desc

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.000000000 -0800
+++ at91/arch/arm/mach-at91/gpio.c	2007-11-27 14:32:01.000000000 -0800
@@ -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");
--- at91.orig/arch/avr32/mach-at32ap/pio.c	2007-11-27 14:30:03.000000000 -0800
+++ at91/arch/avr32/mach-at32ap/pio.c	2007-11-27 14:30:04.000000000 -0800
@@ -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");
--- at91.orig/drivers/spi/mcp23s08.c	2007-11-27 14:29:20.000000000 -0800
+++ at91/drivers/spi/mcp23s08.c	2007-11-27 14:30:04.000000000 -0800
@@ -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");
--- at91.orig/include/asm-avr32/arch-at32ap/gpio.h	2007-11-27 14:30:03.000000000 -0800
+++ at91/include/asm-avr32/arch-at32ap/gpio.h	2007-11-27 14:30:04.000000000 -0800
@@ -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. */
--- at91.orig/include/asm-generic/gpio.h	2007-11-27 14:29:19.000000000 -0800
+++ at91/include/asm-generic/gpio.h	2007-11-27 14:30:04.000000000 -0800
@@ -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);
--- at91.orig/lib/gpiolib.c	2007-11-27 14:29:20.000000000 -0800
+++ at91/lib/gpiolib.c	2007-11-27 14:30:04.000000000 -0800
@@ -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
  * ill-advised when setting direction, and otherwise illegal.
  */
-static void gpio_ensure_requested(struct gpio_chip *chip, unsigned offset)
+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);
 #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 ... */
 
-	for (id = 0; id < ARRAY_SIZE(chips); id++) {
-		chip = chips[id];
+	for (gpio = 0; gpio < ARCH_NR_GPIOS; gpio++) {
+		if (chip == gpio_desc[gpio].chip)
+			continue;
+		chip = gpio_desc[gpio].chip;
 		if (!chip)
 			continue;
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists