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 <[email protected]>
---
Documentation/gpio.txt | 12 -
include/asm-generic/gpio.h | 127 +++++++++++
lib/Kconfig | 6
lib/Makefile | 2
lib/gpiolib.c | 500 +++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 644 insertions(+), 3 deletions(-)
--- a/Documentation/gpio.txt 2007-10-29 01:27:23.000000000 -0700
+++ b/Documentation/gpio.txt 2007-10-29 01:30:02.000000000 -0700
@@ -63,7 +63,9 @@ and that can be critical for glue logic.
Plus, this doesn't define an implementation framework, just an interface.
One platform might implement it as simple inline functions accessing chip
registers; another might implement it by delegating through abstractions
-used for several very different kinds of GPIO controller.
+used for several very different kinds of GPIO controller. (There is some
+library code supporting such an implementation strategy, but drivers using
+the interface should not care if that's how the interface is implemented.)
That said, if the convention is supported on their platform, drivers should
use it when possible. Platforms should declare GENERIC_GPIO support in
@@ -223,6 +225,9 @@ Note that requesting a GPIO does NOT cau
way; it just marks that GPIO as in use. Separate code must handle any
pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
+Also note that it's your responsibility to have stopped using a GPIO
+before you free it.
+
GPIOs mapped to IRQs
--------------------
@@ -238,7 +243,7 @@ map between them using calls like:
Those return either the corresponding number in the other namespace, or
else a negative errno code if the mapping can't be done. (For example,
-some GPIOs can't used as IRQs.) It is an unchecked error to use a GPIO
+some GPIOs can't be used as IRQs.) It is an unchecked error to use a GPIO
number that wasn't set up as an input using gpio_direction_input(), or
to use an IRQ number that didn't originally come from gpio_to_irq().
@@ -299,6 +304,7 @@ Related to multiplexing is configuration
pulldowns integrated on some platforms. Not all platforms support them,
or support them in the same way; and any given board might use external
pullups (or pulldowns) so that the on-chip ones should not be used.
+(When a circuit needs 5 kOhm, on-chip 100 kOhm resistors won't do.)
There are other system-specific mechanisms that are not specified here,
like the aforementioned options for input de-glitching and wire-OR output.
@@ -312,4 +318,4 @@ Dynamic definition of GPIOs is not curre
a side effect of configuring an add-on board with some GPIO expanders.
These calls are purely for kernel space, but a userspace API could be built
-on top of it.
+on top of them.
--- a/lib/Kconfig 2007-10-29 01:27:23.000000000 -0700
+++ b/lib/Kconfig 2007-10-29 01:30:02.000000000 -0700
@@ -141,4 +141,10 @@ config HAS_DMA
config CHECK_SIGNATURE
bool
+#
+# gpiolib is selected if needed
+#
+config GPIO_LIB
+ boolean
+
endmenu
--- a/include/asm-generic/gpio.h 2007-10-29 01:27:23.000000000 -0700
+++ b/include/asm-generic/gpio.h 2007-10-29 01:30:02.000000000 -0700
@@ -1,6 +1,131 @@
#ifndef _ASM_GENERIC_GPIO_H
#define _ASM_GENERIC_GPIO_H
+#ifdef CONFIG_GPIO_LIB
+
+/* Platforms may implement their GPIO interface with library code,
+ * at the cost of performance for non-inlined operations.
+ *
+ * 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).
+ */
+
+#ifndef ARCH_NR_GPIOS
+#define ARCH_NR_GPIOS 512
+#endif
+
+#ifndef ARCH_GPIOS_PER_CHIP
+#define ARCH_GPIOS_PER_CHIP BITS_PER_LONG
+#endif
+
+struct seq_file;
+
+/**
+ * struct gpio_chip - abstract a GPIO controller
+ * @label: for diagnostics
+ * @direction_input: configures signal "offset" as input, or returns error
+ * @get: returns value for signal "offset"; for output signals this
+ * returns either the value actually sensed, or zero
+ * @direction_output: configures signal "offset" as output, or returns error
+ * @set: assigns output value for signal "offset"
+ * @dbg_show: optional routine to show contents in debugfs; default code
+ * will be used when this is omitted, but custom code can show extra
+ * 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).
+ * @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.
+ * Example sources would be SOC controllers, FPGAs, multifunction
+ * chips, dedicated GPIO expanders, and so on.
+ *
+ * Each chip controls a number of signals, numbered 0..@ngpio, which are
+ * identified in method calls by an "offset" value. When those signals
+ * are referenced through calls like gpio_get_value(gpio), the offset
+ * is calculated by subtracting @base from the gpio number.
+ */
+struct gpio_chip {
+ char *label;
+
+ int (*direction_input)(struct gpio_chip *chip,
+ unsigned offset);
+ int (*get)(struct gpio_chip *chip,
+ unsigned offset);
+ int (*direction_output)(struct gpio_chip *chip,
+ unsigned offset, int value);
+ void (*set)(struct gpio_chip *chip,
+ unsigned offset, int value);
+ void (*dbg_show)(struct seq_file *s,
+ struct gpio_chip *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
+}
+
+/* add/remove chips */
+extern int gpiochip_add(struct gpio_chip *chip);
+extern int __must_check gpiochip_remove(struct gpio_chip *chip);
+
+
+/* Always use the library code for GPIO management calls,
+ * or when sleeping may be involved.
+ */
+extern int gpio_request(unsigned gpio, const char *label);
+extern void gpio_free(unsigned gpio);
+
+extern int gpio_direction_input(unsigned gpio);
+extern int gpio_direction_output(unsigned gpio, int value);
+
+extern int gpio_get_value_cansleep(unsigned gpio);
+extern void gpio_set_value_cansleep(unsigned gpio, int value);
+
+
+/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
+ * the GPIO is constant and refers to some always-present controller,
+ * giving direct access to chip registers and tight bitbanging loops.
+ */
+extern int __gpio_get_value(unsigned gpio);
+extern void __gpio_set_value(unsigned gpio, int value);
+
+extern int __gpio_cansleep(unsigned gpio);
+
+
+#else
+
/* platforms that don't directly support access to GPIOs through I2C, SPI,
* or other blocking infrastructure can use these wrappers.
*/
@@ -22,4 +147,6 @@ static inline void gpio_set_value_cansle
gpio_set_value(gpio, value);
}
+#endif
+
#endif /* _ASM_GENERIC_GPIO_H */
--- a/lib/Makefile 2007-10-29 01:27:23.000000000 -0700
+++ b/lib/Makefile 2007-10-29 01:30:02.000000000 -0700
@@ -68,6 +68,8 @@ obj-$(CONFIG_FAULT_INJECTION) += fault-i
lib-$(CONFIG_GENERIC_BUG) += bug.o
+lib-$(CONFIG_GPIO_LIB) += gpiolib.o
+
hostprogs-y := gen_crc32table
clean-files := crc32table.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/lib/gpiolib.c 2007-10-29 07:30:04.000000000 -0700
@@ -0,0 +1,500 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/spinlock.h>
+
+#include <asm/gpio.h>
+
+
+/* Optional implementation infrastructure for GPIO interfaces.
+ *
+ * Platforms may want to use this if they tend to use very many GPIOs
+ * that aren't part of a System-On-Chip core; or across I2C/SPI/etc.
+ *
+ * When kernel footprint or instruction count is an issue, simpler
+ * implementations may be preferred.
+ */
+
+
+/* When debugging, extend minimal trust to callers and platform code;
+ * otherwise, minimize overhead in what may be bitbanging codepaths.
+ */
+#ifdef CONFIG_DEBUG_GPIO
+#define extra_checks 1
+#else
+#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)];
+
+/* 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)
+{
+ int 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);
+#endif
+
+ if (!requested)
+ printk(KERN_DEBUG "GPIO-%d autorequested\n",
+ chip->base + offset);
+}
+
+/* caller holds gpio_lock */
+static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
+{
+ return chips[gpio / ARCH_GPIOS_PER_CHIP];
+}
+
+/**
+ * gpiochip_add() - register a gpio_chip
+ * @chip: the chip to register, with chip->base initialized
+ * Context: potentially before irqs or kmalloc will work
+ *
+ * Returns a negative errno if the chip can't be registered, such as
+ * because the chip->base is invalid or already associated with a
+ * different chip. Otherwise it returns zero as a success code.
+ */
+int gpiochip_add(struct gpio_chip *chip)
+{
+ unsigned long flags;
+ 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)
+ 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;
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return status;
+}
+EXPORT_SYMBOL_GPL(gpiochip_add);
+
+/**
+ * gpiochip_remove() - unregister a gpio_chip
+ * @chip: the chip to unregister
+ *
+ * A gpio_chip with any GPIOs still requested may not be removed.
+ */
+int gpiochip_remove(struct gpio_chip *chip)
+{
+ unsigned long flags;
+ int status = 0;
+ int offset;
+ unsigned id = chip->base / ARCH_GPIOS_PER_CHIP;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ for (offset = 0; offset < chip->ngpio; offset++) {
+ if (gpiochip_is_requested(chip, offset)) {
+ status = -EBUSY;
+ break;
+ }
+ }
+
+ if (status == 0) {
+ if (chips[id] == chip)
+ chips[id] = NULL;
+ else
+ status = -EINVAL;
+ }
+
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return status;
+}
+EXPORT_SYMBOL_GPL(gpiochip_remove);
+
+
+/* These "optional" allocation calls help prevent drivers from stomping
+ * on each other, and help provide better diagnostics in debugfs.
+ * They're called even less than the "set direction" calls.
+ */
+int gpio_request(unsigned gpio, const char *label)
+{
+ struct gpio_chip *chip;
+ 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)
+ goto done;
+
+ /* NOTE: gpio_request() can be called in early boot,
+ * before IRQs are enabled.
+ */
+
+ status = 0;
+#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;
+#endif
+
+done:
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return status;
+}
+EXPORT_SYMBOL_GPL(gpio_request);
+
+void gpio_free(unsigned gpio)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ chip = gpio_to_chip(gpio);
+ if (!chip)
+ goto done;
+ gpio -= chip->base;
+ if (gpio >= chip->ngpio)
+ goto done;
+
+#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;
+#endif
+ WARN_ON(extra_checks && chip == NULL);
+done:
+ spin_unlock_irqrestore(&gpio_lock, flags);
+}
+EXPORT_SYMBOL_GPL(gpio_free);
+
+
+
+/* Drivers MUST make configuration calls before get/set calls
+ *
+ * As a rule these aren't called more than once (except for
+ * drivers using the open-drain emulation idiom) so these are
+ * natural places to accumulate extra debugging checks. Note
+ * that we can't rely on gpio_request() having been called.
+ */
+
+int gpio_direction_input(unsigned gpio)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+ int status = -EINVAL;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ chip = gpio_to_chip(gpio);
+ if (!chip || !chip->get || !chip->direction_input)
+ goto fail;
+ gpio -= chip->base;
+ if (gpio >= chip->ngpio)
+ goto fail;
+ gpio_ensure_requested(chip, gpio);
+
+ /* now we know the gpio is valid and chip won't vanish */
+
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ might_sleep_if(extra_checks && chip->can_sleep);
+
+ status = chip->direction_input(chip, gpio);
+ if (status == 0)
+ clear_bit(gpio, chip->is_out);
+ return status;
+fail:
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return status;
+}
+EXPORT_SYMBOL(gpio_direction_input);
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+ int status = -EINVAL;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+
+ chip = gpio_to_chip(gpio);
+ if (!chip || !chip->get || !chip->direction_output)
+ goto fail;
+ gpio -= chip->base;
+ if (gpio >= chip->ngpio)
+ goto fail;
+ gpio_ensure_requested(chip, gpio);
+
+ /* now we know the gpio is valid and chip won't vanish */
+
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ might_sleep_if(extra_checks && chip->can_sleep);
+
+ status = chip->direction_output(chip, gpio, value);
+ if (status == 0)
+ set_bit(gpio, chip->is_out);
+ return status;
+fail:
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return status;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_output);
+
+
+/* I/O calls are only valid after configuration completed; the relevant
+ * "is this a valid GPIO" error checks should already have been done.
+ *
+ * "Get" operations are often inlinable as reading a pin value register,
+ * and masking the relevant bit in that register.
+ *
+ * When "set" operations are inlinable, they involve writing that mask to
+ * one register to set a low value, or a different register to set it high.
+ * Otherwise a spinlock is needed, and there's little value to inlining.
+ */
+int __gpio_get_value(unsigned gpio)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+ chip = gpio_to_chip(gpio);
+ if (extra_checks)
+ gpio_ensure_requested(chip, gpio - chip->base);
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ if (unlikely(chip->can_sleep)) {
+ WARN_ON(extra_checks);
+ return 0;
+ } else
+ return chip->get(chip, gpio - chip->base);
+}
+EXPORT_SYMBOL_GPL(__gpio_get_value);
+
+void __gpio_set_value(unsigned gpio, int value)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+ chip = gpio_to_chip(gpio);
+ if (extra_checks)
+ gpio_ensure_requested(chip, gpio - chip->base);
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ if (unlikely(chip->can_sleep))
+ WARN_ON(extra_checks);
+ else
+ chip->set(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(__gpio_set_value);
+
+int __gpio_cansleep(unsigned gpio)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+
+ spin_lock_irqsave(&gpio_lock, flags);
+ chip = gpio_to_chip(gpio);
+ if (extra_checks)
+ gpio_ensure_requested(chip, gpio - chip->base);
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return chip->can_sleep;
+}
+EXPORT_SYMBOL_GPL(__gpio_cansleep);
+
+
+
+/* There's no value in inlining GPIO calls that may sleep.
+ * Common examples include ones connected to I2C or SPI chips.
+ */
+
+int gpio_get_value_cansleep(unsigned gpio)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+
+ might_sleep_if(extra_checks);
+
+ spin_lock_irqsave(&gpio_lock, flags);
+ chip = gpio_to_chip(gpio);
+ if (extra_checks)
+ gpio_ensure_requested(chip, gpio - chip->base);
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ return chip->get(chip, gpio - chip->base);
+}
+EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
+
+void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+ unsigned long flags;
+ struct gpio_chip *chip;
+
+ might_sleep_if(extra_checks);
+
+ spin_lock_irqsave(&gpio_lock, flags);
+ chip = gpio_to_chip(gpio);
+ if (extra_checks)
+ gpio_ensure_requested(chip, gpio - chip->base);
+ spin_unlock_irqrestore(&gpio_lock, flags);
+
+ chip->set(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
+
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+
+static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+ unsigned i;
+
+ for (i = 0; i < chip->ngpio; i++) {
+ unsigned gpio;
+ int is_out;
+
+ if (!chip->requested[i])
+ 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 ",
+ chip->get
+ ? (chip->get(chip, i) ? "hi" : "lo")
+ : "? ");
+
+ if (!is_out) {
+ int irq = gpio_to_irq(gpio);
+ struct irq_desc *desc = irq_desc + irq;
+
+ /* This races with request_irq(), set_irq_type(),
+ * and set_irq_wake() ... but those are "rare".
+ *
+ * More significantly, trigger type flags aren't
+ * currently maintained by genirq.
+ */
+ if (irq >= 0 && desc->action) {
+ char *trigger;
+
+ switch (desc->status & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_NONE:
+ trigger = "(default)";
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ trigger = "edge-falling";
+ break;
+ case IRQ_TYPE_EDGE_RISING:
+ trigger = "edge-rising";
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ trigger = "edge-both";
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ trigger = "level-high";
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ trigger = "level-low";
+ break;
+ default:
+ trigger = "?trigger?";
+ break;
+ }
+
+ seq_printf(s, " irq-%d %s%s",
+ irq, trigger,
+ (desc->status & IRQ_WAKEUP)
+ ? " wakeup" : "");
+ }
+ }
+
+ seq_printf(s, "\n");
+ }
+}
+
+static int gpiolib_show(struct seq_file *s, void *unused)
+{
+ struct gpio_chip *chip;
+ unsigned id;
+ int started = 0;
+
+ /* REVISIT this isn't locked against gpio_chip removal ... */
+
+ for (id = 0; id < ARRAY_SIZE(chips); id++) {
+ chip = chips[id];
+ if (!chip)
+ continue;
+
+ seq_printf(s, "%sGPIOs %d-%d, %s%s:\n",
+ started ? "\n" : "",
+ chip->base, chip->base + chip->ngpio - 1,
+ chip->label ? : "generic",
+ chip->can_sleep ? ", can sleep" : "");
+ started = 1;
+ if (chip->dbg_show)
+ chip->dbg_show(s, chip);
+ else
+ gpiolib_dbg_show(s, chip);
+ }
+ return 0;
+}
+
+static int gpiolib_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, gpiolib_show, NULL);
+}
+
+static struct file_operations gpiolib_operations = {
+ .open = gpiolib_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int __init gpiolib_debugfs_init(void)
+{
+ /* /sys/kernel/debug/gpio */
+ (void) debugfs_create_file("gpio", S_IFREG | S_IRUGO,
+ NULL, NULL, &gpiolib_operations);
+ return 0;
+}
+postcore_initcall(gpiolib_debugfs_init);
+
+#endif /* DEBUG_FS */
On Monday 29 October 2007, David Brownell wrote:
>
> 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.
> 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
On Nov 6, 2007 5:05 AM, David Brownell <[email protected]> wrote:
> On Monday 29 October 2007, David Brownell wrote:
> >
> > 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.
>
>
>
> > 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.
>
>
>
--
Cheers
- eric
On Monday 12 November 2007, eric miao wrote:
> 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.
> 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)?
> 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.
> 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).
> 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
> What do you think?
>
> - eric
>
> On Nov 6, 2007 5:05 AM, David Brownell <[email protected]> wrote:
> > On Monday 29 October 2007, David Brownell wrote:
> > >
> > > 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
> >
>
On Nov 14, 2007 3:06 AM, David Brownell <[email protected]> wrote:
> On Monday 12 November 2007, eric miao wrote:
> > 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.
>
exactly, I'd say a "struct gpio_desc" as
>
> > 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)?
>
the cost is as trivial as the current one.
>
> > 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...
>
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
later adjust gpio_to_irq() and irq_to_gpio() easily??
> 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.
>
>
> > 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).
>
>
> > 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.
>
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
(RCU maybe), since removing a gpio_chip is really *rare*.
> - Dave
>
>
>
>
> > What do you think?
> >
> > - eric
> >
> > On Nov 6, 2007 5:05 AM, David Brownell <[email protected]> wrote:
> > > On Monday 29 October 2007, David Brownell wrote:
> > > >
> > > > 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
> > >
> >
>
--
Cheers
- eric
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 the gpio is valid and chip won't vanish */
@@ -250,9 +260,11 @@ int gpio_direction_output(unsigned gpio, int value)
chip = gpio_to_chip(gpio);
if (!chip || !chip->get || !chip->direction_output)
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 the gpio is valid and chip won't vanish */
--
1.5.2.5.GIT
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])
- status = -EBUSY;
- else
- chip->requested[gpio] = label;
-#else
+
if (test_and_set_bit(gpio, chip->requested))
status = -EBUSY;
+
+#ifdef CONFIG_DEBUG_FS
+ if (status == 0)
+ chip->requested_str[gpio] = (label == NULL) ? "?" : label;
#endif
done:
@@ -190,14 +186,12 @@ void gpio_free(unsigned gpio)
gpio -= chip->base;
-#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;
+
+#ifdef CONFIG_DEBUG_FS
+ if (chip != NULL)
+ chip->requested_str[gpio] = NULL;
#endif
WARN_ON(extra_checks && chip == NULL);
done:
@@ -400,14 +394,14 @@ static void gpiolib_dbg_show(struct seq_file *s,
struct gpio_chip *chip)
unsigned gpio;
int is_out;
- if (!chip->requested[i])
+ if (!chip->requested_str[i])
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],
+ gpio, chip->requested_str[i],
is_out ? "out" : "in ",
chip->get
? (chip->get(chip, i) ? "hi" : "lo")
--
1.5.2.5.GIT
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->base < 0 || (chip->base % ARCH_GPIOS_PER_CHIP) != 0)
@@ -89,13 +87,21 @@ int gpiochip_add(struct gpio_chip *chip)
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;
+
+ for (id = chip->base; id < chip->base + chip->ngpio; id++)
+ if (gpio_desc[id].chip) {
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return -EBUSY;
+ }
+
+ for (id = chip->base; id < chip->base + chip->ngpio; id++)
+ gpio_desc[id].chip = chip;
+
+#ifdef CONFIG_DEBUG_FS
+ list_add(&chip->node, &gpio_chip_list);
+#endif
spin_unlock_irqrestore(&gpio_lock, flags);
- return status;
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -108,28 +114,26 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
int gpiochip_remove(struct gpio_chip *chip)
{
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++) {
+ for (offset = 0; offset < chip->ngpio; offset++)
if (gpiochip_is_requested(chip, offset)) {
- status = -EBUSY;
- break;
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return -EBUSY;
}
- }
- 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;
+
+#ifdef CONFIG_DEBUG_FS
+ list_del(&chip->node);
+#endif
spin_unlock_irqrestore(&gpio_lock, flags);
- return status;
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_remove);
@@ -463,11 +467,7 @@ static int gpiolib_show(struct seq_file *s, void *unused)
/* REVISIT this isn't locked against gpio_chip removal ... */
- 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,
--
1.5.2.5.GIT
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_irqrestore(&gpio_lock, flags);
@@ -251,11 +253,14 @@ int gpio_direction_output(unsigned gpio, int value)
{
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_output)
goto fail;
@@ -272,8 +277,7 @@ int gpio_direction_output(unsigned gpio, int value)
might_sleep_if(extra_checks && chip->can_sleep);
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);
@@ -402,7 +406,7 @@ static void gpiolib_dbg_show(struct seq_file *s,
struct gpio_chip *chip)
continue;
gpio = chip->base + i;
- is_out = test_bit(i, chip->is_out);
+ is_out = gpio_desc[gpio].is_out;
seq_printf(s, " gpio-%-3d (%-12s) %s %s",
gpio, chip->requested_str[i],
--
1.5.2.5.GIT
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_str[offset] = "(auto)";
+ gpio_desc[gpio].requested_str = "(auto)";
#endif
if (!requested)
- printk(KERN_DEBUG "GPIO-%d autorequested\n",
- chip->base + offset);
+ pr_debug("GPIO-%d autorequested\n", gpio);
}
/* caller holds gpio_lock */
@@ -114,13 +113,12 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
int gpiochip_remove(struct gpio_chip *chip)
{
unsigned long flags;
- int offset;
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) {
spin_unlock_irqrestore(&gpio_lock, flags);
return -EBUSY;
}
@@ -145,11 +143,14 @@ 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);
+
+ desc = &gpio_desc[gpio];
+ chip = desc->chip;
if (!gpio_is_onchip(gpio, chip))
goto done;
@@ -162,12 +163,14 @@ int gpio_request(unsigned gpio, const char *label)
status = 0;
- if (test_and_set_bit(gpio, chip->requested))
+ if (desc->requested == 0)
+ desc->requested = 1;
+ else
status = -EBUSY;
#ifdef CONFIG_DEBUG_FS
if (status == 0)
- chip->requested_str[gpio] = (label == NULL) ? "?" : label;
+ desc->requested_str = (label == NULL) ? "?" : label;
#endif
done:
@@ -180,9 +183,11 @@ void gpio_free(unsigned gpio)
{
unsigned long flags;
struct gpio_chip *chip;
+ struct gpio_desc *desc;
spin_lock_irqsave(&gpio_lock, flags);
+ desc = &gpio_desc[gpio];
chip = gpio_to_chip(gpio);
if (!gpio_is_onchip(gpio, chip))
@@ -190,12 +195,13 @@ void gpio_free(unsigned gpio)
gpio -= chip->base;
- if (!test_and_clear_bit(gpio, chip->requested))
+ if (desc->requested == 0)
+ desc->requested = 1;
+ else
chip = NULL;
#ifdef CONFIG_DEBUG_FS
- if (chip != NULL)
- chip->requested_str[gpio] = NULL;
+ desc->requested_str = NULL;
#endif
WARN_ON(extra_checks && chip == NULL);
done:
@@ -231,8 +237,8 @@ int gpio_direction_input(unsigned gpio)
if (!gpio_is_onchip(gpio, chip))
goto fail;
+ gpio_ensure_requested(gpio);
gpio -= chip->base;
- gpio_ensure_requested(chip, gpio);
/* now we know the gpio is valid and chip won't vanish */
@@ -267,8 +273,8 @@ int gpio_direction_output(unsigned gpio, int value)
if (!gpio_is_onchip(gpio, chip))
goto fail;
+ gpio_ensure_requested(gpio);
gpio -= chip->base;
- gpio_ensure_requested(chip, gpio);
/* now we know the gpio is valid and chip won't vanish */
@@ -304,7 +310,7 @@ int __gpio_get_value(unsigned gpio)
spin_lock_irqsave(&gpio_lock, flags);
chip = gpio_to_chip(gpio);
if (extra_checks)
- gpio_ensure_requested(chip, gpio - chip->base);
+ gpio_ensure_requested(gpio);
spin_unlock_irqrestore(&gpio_lock, flags);
if (unlikely(chip->can_sleep)) {
@@ -323,7 +329,7 @@ void __gpio_set_value(unsigned gpio, int value)
spin_lock_irqsave(&gpio_lock, flags);
chip = gpio_to_chip(gpio);
if (extra_checks)
- gpio_ensure_requested(chip, gpio - chip->base);
+ gpio_ensure_requested(gpio);
spin_unlock_irqrestore(&gpio_lock, flags);
if (unlikely(chip->can_sleep))
@@ -341,7 +347,7 @@ int __gpio_cansleep(unsigned gpio)
spin_lock_irqsave(&gpio_lock, flags);
chip = gpio_to_chip(gpio);
if (extra_checks)
- gpio_ensure_requested(chip, gpio - chip->base);
+ gpio_ensure_requested(gpio);
spin_unlock_irqrestore(&gpio_lock, flags);
return chip->can_sleep;
}
@@ -363,7 +369,7 @@ int gpio_get_value_cansleep(unsigned gpio)
spin_lock_irqsave(&gpio_lock, flags);
chip = gpio_to_chip(gpio);
if (extra_checks)
- gpio_ensure_requested(chip, gpio - chip->base);
+ gpio_ensure_requested(gpio);
spin_unlock_irqrestore(&gpio_lock, flags);
return chip->get(chip, gpio - chip->base);
@@ -380,7 +386,7 @@ void gpio_set_value_cansleep(unsigned gpio, int value)
spin_lock_irqsave(&gpio_lock, flags);
chip = gpio_to_chip(gpio);
if (extra_checks)
- gpio_ensure_requested(chip, gpio - chip->base);
+ gpio_ensure_requested(gpio);
spin_unlock_irqrestore(&gpio_lock, flags);
chip->set(chip, gpio - chip->base, value);
@@ -401,21 +407,23 @@ static void gpiolib_dbg_show(struct seq_file *s,
struct gpio_chip *chip)
for (i = 0; i < chip->ngpio; i++) {
unsigned gpio;
int is_out;
+ struct gpio_desc *desc;
- if (!chip->requested_str[i])
+ if (!desc->requested)
continue;
gpio = chip->base + i;
- is_out = gpio_desc[gpio].is_out;
+ desc = &gpio_desc[gpio];
+ is_out = desc->is_out;
seq_printf(s, " gpio-%-3d (%-12s) %s %s",
- gpio, chip->requested_str[i],
- is_out ? "out" : "in ",
+ gpio, desc->requested_str,
+ desc->is_out ? "out" : "in ",
chip->get
? (chip->get(chip, i) ? "hi" : "lo")
: "? ");
- if (!is_out) {
+ if (!desc->is_out) {
int irq = gpio_to_irq(gpio);
struct irq_desc *desc = irq_desc + irq;
--
1.5.2.5.GIT
On Tuesday 13 November 2007, eric miao wrote:
>
> 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
On Tuesday 13 November 2007, eric miao wrote:
>
> 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
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. :)
On Tuesday 13 November 2007, eric miao wrote:
> > > 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.
On Tuesday 13 November 2007, David Brownell wrote:
> ? 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.
-ENOPATCH ... ;)
==========
Minor fixups to the gpiolib code.
Signed-off-by: David Brownell <[email protected]>
---
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 */
On Tuesday 13 November 2007, eric miao wrote:
> 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.
> +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.
>
> - 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
On Tuesday 13 November 2007, eric miao wrote:
> Subject: [PATCH 5/5] move per GPIO "requested" to "struct gpio_desc"
>
> struct gpio_desc {
> struct gpio_chip *chip;
> unsigned is_out:1;
> + 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.
> @@ -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 ...
> 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.
On Nov 14, 2007 11:25 AM, David Brownell <[email protected]> wrote:
> On Tuesday 13 November 2007, eric miao wrote:
> >
> > 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. :)
>
Ok, I'll update the patches later.
>
> 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.
>
just a style issue, moving something commonly done into
a routine, and extra_checks could be put there instead
everywhere for a clean look :-)
> - Dave
>
>
--
Cheers
- eric
On Nov 14, 2007 11:30 AM, David Brownell <[email protected]> wrote:
> On Tuesday 13 November 2007, eric miao wrote:
> > > > 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.
>
to be honest, I don't feel like the holes. Put restrictions on the numbering
of GPIOs might not be a good idea either.
>
>
>
--
Cheers
- eric
On Nov 14, 2007 12:18 PM, David Brownell <[email protected]> wrote:
> On Tuesday 13 November 2007, eric miao wrote:
> > 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.
I'll be happy too.
>
>
> > +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.
I'd prefer to name it "gpio_desc_lock" instead, which is self-explanatory
and thus requires no comment at all
>
> >
> > - 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.
>
absolutely, you get the same feeling of mine and since this is for illustration
purpose only, I don't want more patches to fix this...
> - Dave
>
--
Cheers
- eric
On Nov 14, 2007 12:36 PM, David Brownell <[email protected]> wrote:
> On Tuesday 13 November 2007, eric miao wrote:
> > Subject: [PATCH 5/5] move per GPIO "requested" to "struct gpio_desc"
> >
>
> > struct gpio_desc {
> > struct gpio_chip *chip;
> > unsigned is_out:1;
> > + 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.
>
Fine.
>
> 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.
>
> > @@ -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 ...
>
>
> > 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?
>
line length issue, just ignore this if you prefer.
>
>
> ... overall the main downside of these patches seems to
> be that it consumes more static memory.
>
Not really, since it reduces the holes. That all depend on your
ARCH_NR_GPIOS.
--
Cheers
- eric
On Tuesday 13 November 2007, eric miao wrote:
> > > > 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?
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
> > > struct gpio_desc {
> > > struct gpio_chip *chip;
> > > unsigned is_out:1;
> > > + 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.
> 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
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.
On Nov 14, 2007 3:19 PM, David Brownell <[email protected]> wrote:
>
> > > > struct gpio_desc {
> > > > struct gpio_chip *chip;
> > > > unsigned is_out:1;
> > > > + 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.
>
>
> > 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.
>
Well, absolutely agree on this.
> - Dave
>
--
Cheers
- eric
On Tue, 13 Nov 2007 20:36:13 -0800, David Brownell wrote:
> On Tuesday 13 November 2007, eric miao wrote:
> > 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.
--
Jean Delvare
On Saturday 17 November 2007, Jean Delvare wrote:
> On Tue, 13 Nov 2007 20:36:13 -0800, David Brownell wrote:
> > On Tuesday 13 November 2007, eric miao wrote:
> > > 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.
It's perfectly correct. That it's an idiom you don't
seem to *like* but is distinct from correctness.
> 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.
- Dave
Hi David,
On Sat, 17 Nov 2007 09:36:24 -0800, David Brownell wrote:
> On Saturday 17 November 2007, Jean Delvare wrote:
> > On Tue, 13 Nov 2007 20:36:13 -0800, David Brownell wrote:
> > > On Tuesday 13 November 2007, eric miao wrote:
> > > > 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.
>
> It's perfectly correct. That it's an idiom you don't
> seem to *like* but is distinct from correctness.
>
> > 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".
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
you?
> 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.
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
On Tuesday 13 November 2007, David Brownell wrote:
> 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
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 *requested_label;
+#endif
+};
/**
* struct gpio_chip - abstract a GPIO controller
@@ -41,8 +47,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.
@@ -65,37 +69,11 @@ struct gpio_chip {
unsigned offset, int value);
void (*set)(struct gpio_chip *chip,
unsigned offset, int value);
- void (*dbg_show)(struct seq_file *s,
- struct gpio_chip *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
-}
-
/* 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 a853715..f24182e 100644
--- a/lib/gpiolib.c
+++ b/lib/gpiolib.c
@@ -28,39 +28,30 @@
#define extra_checks 0
#endif
-/* gpio_lock protects the table of chips and gpio_chip->requested.
+/* gpio_lock protects the table of gpio_desc[] 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 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, unsigned gpio)
{
- int 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);
+ if (!desc->requested)
+ desc->requested_label = "(auto)";
#endif
-
- if (!requested)
- printk(KERN_DEBUG "GPIO-%d autorequested\n",
- chip->base + offset);
+ if (!desc->requested)
+ printk(KERN_DEBUG "GPIO-%d autorequested\n", gpio);
}
/* 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;
}
/**
@@ -75,26 +66,25 @@ 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->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)
+ 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) {
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return -EBUSY;
+ }
+
+ for (id = chip->base; id < chip->base + chip->ngpio; id++)
+ gpio_desc[id].chip = chip;
spin_unlock_irqrestore(&gpio_lock, flags);
- return status;
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -107,28 +97,21 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
int gpiochip_remove(struct gpio_chip *chip)
{
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)) {
- status = -EBUSY;
- break;
+ for (id = chip->base; id < chip->base + chip->ngpio; id++)
+ if (gpio_desc[id].requested) {
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return -EBUSY;
}
- }
- 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);
- return status;
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_remove);
@@ -139,17 +122,15 @@ 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,
@@ -157,18 +138,16 @@ int gpio_request(unsigned gpio, const char *label)
*/
status = 0;
-#ifdef CONFIG_DEBUG_FS
- if (!label)
- label = "?";
- if (chip->requested[gpio])
- status = -EBUSY;
+
+ if (!desc->requested)
+ desc->requested = 1;
else
- chip->requested[gpio] = label;
-#else
- if (test_and_set_bit(gpio, chip->requested))
status = -EBUSY;
-#endif
+#ifdef CONFIG_DEBUG_FS
+ if (status == 0)
+ desc->requested_label = (label == NULL) ? "?" : label;
+#endif
done:
spin_unlock_irqrestore(&gpio_lock, flags);
return status;
@@ -178,27 +157,23 @@ 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)
+ desc = &gpio_desc[gpio];
+
+ if (desc->chip == NULL)
goto done;
-#ifdef CONFIG_DEBUG_FS
- if (chip->requested[gpio])
- chip->requested[gpio] = NULL;
+ if (desc->requested)
+ desc->requested = 0;
else
- chip = NULL;
-#else
- if (!test_and_clear_bit(gpio, chip->requested))
- chip = NULL;
+ WARN_ON(extra_checks);
+
+#ifdef CONFIG_DEBUG_FS
+ desc->requested_label = NULL;
#endif
- WARN_ON(extra_checks && chip == NULL);
done:
spin_unlock_irqrestore(&gpio_lock, flags);
}
@@ -218,17 +193,22 @@ 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;
+
+ gpio_ensure_requested(desc, gpio);
+
if (!chip || !chip->get || !chip->direction_input)
goto fail;
+
gpio -= chip->base;
if (gpio >= chip->ngpio)
goto fail;
- gpio_ensure_requested(chip, gpio);
/* now we know the gpio is valid and chip won't vanish */
@@ -237,8 +217,8 @@ 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);
+ if (status)
+ desc->is_out = 0;
return status;
fail:
spin_unlock_irqrestore(&gpio_lock, flags);
@@ -250,17 +230,22 @@ int gpio_direction_output(unsigned gpio, int value)
{
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;
+
+ gpio_ensure_requested(desc, gpio);
+
if (!chip || !chip->set || !chip->direction_output)
goto fail;
+
gpio -= chip->base;
if (gpio >= chip->ngpio)
goto fail;
- gpio_ensure_requested(chip, gpio);
/* now we know the gpio is valid and chip won't vanish */
@@ -269,8 +254,8 @@ int gpio_direction_output(unsigned gpio, int value)
might_sleep_if(extra_checks && chip->can_sleep);
status = chip->direction_output(chip, gpio, value);
- if (status == 0)
- set_bit(gpio, chip->is_out);
+ if (status)
+ desc->is_out = 1;
return status;
fail:
spin_unlock_irqrestore(&gpio_lock, flags);
@@ -363,27 +348,29 @@ EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
#include <linux/debugfs.h>
#include <linux/seq_file.h>
-
-static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+static int gpiolib_show(struct seq_file *s, void *unused)
{
- unsigned i;
+ struct gpio_chip *chip;
+ unsigned id;
- for (i = 0; i < chip->ngpio; i++) {
- unsigned gpio;
- int is_out;
+ /* REVISIT this isn't locked against gpio_chip removal ... */
- if (!chip->requested[i])
- continue;
+ seq_printf(s, "gpio chip_name requested direction value irq\n");
- gpio = chip->base + i;
- is_out = test_bit(i, chip->is_out);
+ for (id = 0; id < ARCH_NR_GPIO; id++) {
+ struct gpio_desc *desc = &gpio_desc[id];
+ struct gpio_chip *chip = desc->chip;
+
+ if (chip == NULL)
+ continue;
- seq_printf(s, " gpio-%-3d (%-12s) %s %s",
- gpio, chip->requested[i],
- is_out ? "out" : "in ",
- chip->get
- ? (chip->get(chip, i) ? "hi" : "lo")
- : "? ");
+ seq_printf(s, " %3d %12s %9s %9s %5s", gpio,
+ chip->label,
+ desc->requested_label,
+ (desc->is_out) ? "out" : "in",
+ (chip->get) ?
+ (chip->get(chip, i) ? "hi" : "lo")
+ : "?");
if (!is_out) {
int irq = gpio_to_irq(gpio);
@@ -431,32 +418,6 @@ static void gpiolib_dbg_show(struct seq_file *s,
struct gpio_chip *chip)
seq_printf(s, "\n");
}
-}
-
-static int gpiolib_show(struct seq_file *s, void *unused)
-{
- struct gpio_chip *chip;
- unsigned id;
- int started = 0;
-
- /* REVISIT this isn't locked against gpio_chip removal ... */
-
- for (id = 0; id < ARRAY_SIZE(chips); id++) {
- chip = chips[id];
- if (!chip)
- continue;
-
- seq_printf(s, "%sGPIOs %d-%d, %s%s:\n",
- started ? "\n" : "",
- chip->base, chip->base + chip->ngpio - 1,
- chip->label ? : "generic",
- chip->can_sleep ? ", can sleep" : "");
- started = 1;
- if (chip->dbg_show)
- chip->dbg_show(s, chip);
- else
- gpiolib_dbg_show(s, chip);
- }
return 0;
}
------ >8 -------
On Nov 27, 2007 9:46 AM, David Brownell <[email protected]> wrote:
> On Tuesday 13 November 2007, David Brownell wrote:
> > 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
>
--
Cheers
- eric
Thanks, I'll be looking at it ... the one thing I strongly dislike is
this change:
On Tuesday 27 November 2007, eric miao wrote:
> 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
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
On Tuesday 27 November 2007, eric miao wrote:
> c) removal of "gpiochip_is_requested()", and use "gpio_desc->requested"
> instead
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
the avr32 platform conversion) plus others not yet merged...
> d) make gpio_ensure_requested() use gpio_desc, and simplify the code a
> bit
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...
On Tuesday 27 November 2007, eric miao wrote:
> ????????status = chip->direction_input(chip, gpio);
> -???????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(-)
--- 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;
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,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
+ return gpio_desc[chip->base + offset].requested;
}
/* add/remove chips */
diff --git a/lib/gpiolib.c b/lib/gpiolib.c
index a853715..6050af5 100644
--- a/lib/gpiolib.c
+++ b/lib/gpiolib.c
@@ -28,39 +28,30 @@
#define extra_checks 0
#endif
-/* gpio_lock protects the table of chips and gpio_chip->requested.
+/* gpio_lock protects the table of gpio_desc[] 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 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, unsigned gpio)
{
- int 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);
+ if (!desc->requested)
+ desc->requested_label = "(auto)";
#endif
-
- if (!requested)
- printk(KERN_DEBUG "GPIO-%d autorequested\n",
- chip->base + offset);
+ if (!desc->requested)
+ printk(KERN_DEBUG "GPIO-%d autorequested\n", gpio);
}
/* 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;
}
/**
@@ -75,26 +66,25 @@ 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->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)
+ 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) {
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return -EBUSY;
+ }
+
+ for (id = chip->base; id < chip->base + chip->ngpio; id++)
+ gpio_desc[id].chip = chip;
spin_unlock_irqrestore(&gpio_lock, flags);
- return status;
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -107,28 +97,21 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
int gpiochip_remove(struct gpio_chip *chip)
{
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)) {
- status = -EBUSY;
- break;
+ for (id = chip->base; id < chip->base + chip->ngpio; id++)
+ if (gpio_desc[id].requested) {
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ return -EBUSY;
}
- }
- 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);
- return status;
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_remove);
@@ -139,17 +122,15 @@ 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,
@@ -157,18 +138,16 @@ int gpio_request(unsigned gpio, const char *label)
*/
status = 0;
-#ifdef CONFIG_DEBUG_FS
- if (!label)
- label = "?";
- if (chip->requested[gpio])
- status = -EBUSY;
+
+ if (!desc->requested)
+ desc->requested = 1;
else
- chip->requested[gpio] = label;
-#else
- if (test_and_set_bit(gpio, chip->requested))
status = -EBUSY;
-#endif
+#ifdef CONFIG_DEBUG_FS
+ if (status == 0)
+ desc->requested_label = (label == NULL) ? "?" : label;
+#endif
done:
spin_unlock_irqrestore(&gpio_lock, flags);
return status;
@@ -178,27 +157,23 @@ 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)
+ desc = &gpio_desc[gpio];
+
+ if (desc->chip == NULL)
goto done;
-#ifdef CONFIG_DEBUG_FS
- if (chip->requested[gpio])
- chip->requested[gpio] = NULL;
+ if (desc->requested)
+ desc->requested = 0;
else
- chip = NULL;
-#else
- if (!test_and_clear_bit(gpio, chip->requested))
- chip = NULL;
+ WARN_ON(extra_checks);
+
+#ifdef CONFIG_DEBUG_FS
+ desc->requested_label = NULL;
#endif
- WARN_ON(extra_checks && chip == NULL);
done:
spin_unlock_irqrestore(&gpio_lock, flags);
}
@@ -218,17 +193,22 @@ 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;
+
+ gpio_ensure_requested(desc, gpio);
+
if (!chip || !chip->get || !chip->direction_input)
goto fail;
+
gpio -= chip->base;
if (gpio >= chip->ngpio)
goto fail;
- gpio_ensure_requested(chip, gpio);
/* now we know the gpio is valid and chip won't vanish */
@@ -238,7 +218,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 +230,22 @@ int gpio_direction_output(unsigned gpio, int value)
{
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;
+
+ gpio_ensure_requested(desc, gpio);
+
if (!chip || !chip->set || !chip->direction_output)
goto fail;
+
gpio -= chip->base;
if (gpio >= chip->ngpio)
goto fail;
- gpio_ensure_requested(chip, gpio);
/* now we know the gpio is valid and chip won't vanish */
@@ -270,7 +255,7 @@ int gpio_direction_output(unsigned gpio, int value)
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);
@@ -363,29 +348,39 @@ EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
#include <linux/debugfs.h>
#include <linux/seq_file.h>
-
-static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+static int gpiolib_show(struct seq_file *s, void *unused)
{
- unsigned i;
+ unsigned gpio = 0;
+ unsigned offset;
+
+ /* REVISIT this isn't locked against gpio_chip removal ... */
- for (i = 0; i < chip->ngpio; i++) {
- unsigned gpio;
- int is_out;
+ seq_printf(s, "gpio chip_name requested direction value irq\n");
- if (!chip->requested[i])
+ while (gpio < ARCH_NR_GPIOS) {
+ struct gpio_desc *desc = &gpio_desc[gpio];
+ struct gpio_chip *chip = desc->chip;
+
+ if (chip == NULL)
continue;
- gpio = chip->base + i;
- is_out = test_bit(i, chip->is_out);
+ if (chip->dbg_show) {
+ chip->dbg_show(s, chip);
+ gpio += chip->ngpio;
+ continue;
+ }
+
+ offset = gpio - chip->base;
- seq_printf(s, " gpio-%-3d (%-12s) %s %s",
- gpio, chip->requested[i],
- is_out ? "out" : "in ",
- chip->get
- ? (chip->get(chip, i) ? "hi" : "lo")
- : "? ");
+ seq_printf(s, " %3d %11.11s %10.10s %9.9s %5.5s", gpio,
+ chip->label,
+ desc->requested_label,
+ (desc->is_out) ? "out" : "in",
+ (chip->get) ?
+ (chip->get(chip, offset) ? "hi" : "lo")
+ : "?");
- if (!is_out) {
+ if (!desc->is_out) {
int irq = gpio_to_irq(gpio);
struct irq_desc *desc = irq_desc + irq;
@@ -430,32 +425,7 @@ static void gpiolib_dbg_show(struct seq_file *s,
struct gpio_chip *chip)
}
seq_printf(s, "\n");
- }
-}
-
-static int gpiolib_show(struct seq_file *s, void *unused)
-{
- struct gpio_chip *chip;
- unsigned id;
- int started = 0;
-
- /* REVISIT this isn't locked against gpio_chip removal ... */
-
- for (id = 0; id < ARRAY_SIZE(chips); id++) {
- chip = chips[id];
- if (!chip)
- continue;
-
- seq_printf(s, "%sGPIOs %d-%d, %s%s:\n",
- started ? "\n" : "",
- chip->base, chip->base + chip->ngpio - 1,
- chip->label ? : "generic",
- chip->can_sleep ? ", can sleep" : "");
- started = 1;
- if (chip->dbg_show)
- chip->dbg_show(s, chip);
- else
- gpiolib_dbg_show(s, chip);
+ gpio++;
}
return 0;
}
--
1.5.2.5.GIT
On Nov 28, 2007 3:29 AM, David Brownell <[email protected]> wrote:
> On Tuesday 27 November 2007, eric miao wrote:
> > status = chip->direction_input(chip, gpio);
> > -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.
>
--
Cheers
- eric
On Nov 28, 2007 11:15 AM, David Brownell <[email protected]> wrote:
> 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);
produces a warning here about %ld, and maybe you mean
desc - gpio_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;
>
>
Except for the above, I feel OK overall.
--
Cheers
- eric
On Wednesday 28 November 2007, eric miao wrote:
> > +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,
It uses "%ld" since I got a warning with "%d". :(
Evidently a cast will be needed.
> and maybe you mean desc - gpio_desc??
Maybe. Either that or the "GPIO-" should be "GPIO"!
> Except for the above, I feel OK overall.