[PATCH 0/9] gpiolib: remove gpio_desc[] static array

[PATCH 0/9] gpiolib: remove gpio_desc[] static array

[Alexandre Courbot]

Since Linus kindly merged the gpiochip_reserve() removal patches, here comes the
main course.

This series removes the ARCH_NR_GPIOS sized static array of GPIO descriptors
that stood in gpiolib and replaces it with a more flexible mechanism that
dynamically allocates GPIO descriptors as chips are added.

ARCH_NR_GPIOS is still here, but now only controls the upper limit of the GPIO
integer space, i.e. which GPIO numbers are valid and where GPIO chips without a
base GPIO are placed in the GPIO space. Technically it would be possible to get
rid of it completely - but this might change GPIO numbers on some architectures
and make people unhappy. At least it can now be arbitrarily high without
consuming more memory.

As a result of this series, gpiolib has a slightly lower memory footprint (~-9KB
on my Tegra2 board which has a GPIO space of 1024 but only uses 232 of them), is
very slightly slower (because of the gpio to descriptor conversion which is
linear instead of being constant), and (most importantly) is prepared to receive
the new descriptor-based public GPIO interface.

The linear-time conversion between GPIO numbers and descriptors might make some
teeth grind, but please consider the following. First, even though the
conversion is O(n), this is a very small n (the number of GPIO chips), and I
doubt the overhead would be even perceptible. Second, the gpio descriptor
interface that will follow this series will not require this conversion since it
works with descriptors directly. Finally, the GPIO framework allow platforms
that are concerned about performance to implement gpio_get_value() and
gpio_set_value() with a fast path shortcutting gpiolib for those GPIO numbers
for which performance matters, and this is not affected at all by this series.

The patches are split as much as possible to make the switch as easy to follow
as possible - they may actually be oversplit, if this is the case please let me
know and I will resubmit the series.

This has been tested on a Tegra 2 Ventana board. I made sure that debugfs and
sysfs were working as before, and that GPIOs had the expected value. Being a
rather deep change, it should certainly undergo some more testing.

Alexandre Courbot (9):
  gpiolib: link all gpio_chips using a list
  gpiolib: use gpio_chips list in gpiolib_sysfs_init
  gpiolib: use gpio_chips list in gpiochip_find
  gpiolib: use gpio_chips list in sysfs ops
  gpiolib: use gpio_chips list in gpiochip_find_base
  gpiolib: use descriptors internally
  gpiolib: let gpio_chip reference its descriptors
  gpiolib: use gpio_chips list in gpio_to_desc
  gpiolib: dynamically allocate descriptors array

 drivers/gpio/gpiolib.c     | 694 ++++++++++++++++++++++++++++-----------------
 include/asm-generic/gpio.h |   5 +
 2 files changed, 440 insertions(+), 259 deletions(-)

-- 
1.8.1.1

[PATCH 0/9] gpiolib: remove gpio_desc[] static array

[Alexandre Courbot]

Since Linus kindly merged the gpiochip_reserve() removal patches, here comes the
main course.

This series removes the ARCH_NR_GPIOS sized static array of GPIO descriptors
that stood in gpiolib and replaces it with a more flexible mechanism that
dynamically allocates GPIO descriptors as chips are added.

ARCH_NR_GPIOS is still here, but now only controls the upper limit of the GPIO
integer space, i.e. which GPIO numbers are valid and where GPIO chips without a
base GPIO are placed in the GPIO space. Technically it would be possible to get
rid of it completely - but this might change GPIO numbers on some architectures
and make people unhappy. At least it can now be arbitrarily high without
consuming more memory.

As a result of this series, gpiolib has a slightly lower memory footprint (~-9KB
on my Tegra2 board which has a GPIO space of 1024 but only uses 232 of them), is
very slightly slower (because of the gpio to descriptor conversion which is
linear instead of being constant), and (most importantly) is prepared to receive
the new descriptor-based public GPIO interface.

The linear-time conversion between GPIO numbers and descriptors might make some
teeth grind, but please consider the following. First, even though the
conversion is O(n), this is a very small n (the number of GPIO chips), and I
doubt the overhead would be even perceptible. Second, the gpio descriptor
interface that will follow this series will not require this conversion since it
works with descriptors directly. Finally, the GPIO framework allow platforms
that are concerned about performance to implement gpio_get_value() and
gpio_set_value() with a fast path shortcutting gpiolib for those GPIO numbers
for which performance matters, and this is not affected at all by this series.

The patches are split as much as possible to make the switch as easy to follow
as possible - they may actually be oversplit, if this is the case please let me
know and I will resubmit the series.

This has been tested on a Tegra 2 Ventana board. I made sure that debugfs and
sysfs were working as before, and that GPIOs had the expected value. Being a
rather deep change, it should certainly undergo some more testing.

Alexandre Courbot (9):
  gpiolib: link all gpio_chips using a list
  gpiolib: use gpio_chips list in gpiolib_sysfs_init
  gpiolib: use gpio_chips list in gpiochip_find
  gpiolib: use gpio_chips list in sysfs ops
  gpiolib: use gpio_chips list in gpiochip_find_base
  gpiolib: use descriptors internally
  gpiolib: let gpio_chip reference its descriptors
  gpiolib: use gpio_chips list in gpio_to_desc
  gpiolib: dynamically allocate descriptors array

 drivers/gpio/gpiolib.c     | 694 ++++++++++++++++++++++++++++-----------------
 include/asm-generic/gpio.h |   5 +
 2 files changed, 440 insertions(+), 259 deletions(-)

-- 
1.8.1.1

[PATCH 1/9] gpiolib: link all gpio_chips using a list

[Alexandre Courbot]

Add a list member to gpio_chip that allows all chips to be parsed
quickly. The current method requires parsing the entire GPIO integer
space, which is painfully slow. Using a list makes many chip operations
that involve lookup or parsing faster, and also simplifies the code. It
is also necessary to eventually get rid of the global gpio_desc[] array.

The list of gpio_chips is always ordered by base GPIO number to ensure
chips traversal is done in the right order.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c     | 51 +++++++++++++++++++++++++++++++++++++++-------
 include/asm-generic/gpio.h |  2 ++
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e27877a..0050766 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3,6 +3,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/spinlock.h>
+#include <linux/list.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/debugfs.h>
@@ -71,6 +72,8 @@ struct gpio_desc {
 };
 static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
+static LIST_HEAD(gpio_chips);
+
 #ifdef CONFIG_GPIO_SYSFS
 static DEFINE_IDR(dirent_idr);
 #endif
@@ -1013,6 +1016,43 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)
 
 #endif /* CONFIG_GPIO_SYSFS */
 
+/*
+ * Add a new chip to the global chips list, keeping the list of chips sorted
+ * by base order.
+ *
+ * Return -EBUSY if the new chip overlaps with some other chip's integer
+ * space.
+ */
+static int gpiochip_add_to_list(struct gpio_chip *chip)
+{
+	struct list_head *pos = &gpio_chips;
+	struct gpio_chip *_chip;
+	int err = 0;
+
+	/* find where to insert our chip */
+	list_for_each(pos, &gpio_chips) {
+		_chip = list_entry(pos, struct gpio_chip, list);
+		/* shall we insert before _chip? */
+		if (_chip->base >= chip->base + chip->ngpio)
+			break;
+	}
+
+	/* are we stepping on the chip right before? */
+	if (pos != &gpio_chips && pos->prev != &gpio_chips) {
+		_chip = list_entry(pos->prev, struct gpio_chip, list);
+		if (_chip->base + _chip->ngpio > chip->base) {
+			dev_err(chip->dev,
+			       "GPIO integer space overlap, cannot add chip\n");
+			err = -EBUSY;
+		}
+	}
+
+	if (!err)
+		list_add_tail(&chip->list, pos);
+
+	return err;
+}
+
 /**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
@@ -1054,13 +1094,8 @@ int gpiochip_add(struct gpio_chip *chip)
 		chip->base = base;
 	}
 
-	/* these GPIO numbers must not be managed by another gpio_chip */
-	for (id = base; id < base + chip->ngpio; id++) {
-		if (gpio_desc[id].chip != NULL) {
-			status = -EBUSY;
-			break;
-		}
-	}
+	status = gpiochip_add_to_list(chip);
+
 	if (status == 0) {
 		for (id = base; id < base + chip->ngpio; id++) {
 			gpio_desc[id].chip = chip;
@@ -1134,6 +1169,8 @@ int gpiochip_remove(struct gpio_chip *chip)
 	if (status == 0) {
 		for (id = chip->base; id < chip->base + chip->ngpio; id++)
 			gpio_desc[id].chip = NULL;
+
+		list_del(&chip->list);
 	}
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 2034e69..b562f95 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -53,6 +53,7 @@ struct device_node;
  * @label: for diagnostics
  * @dev: optional device providing the GPIOs
  * @owner: helps prevent removal of modules exporting active GPIOs
+ * @list: links gpio_chips together for traversal
  * @request: optional hook for chip-specific activation, such as
  *	enabling module power and clock; may sleep
  * @free: optional hook for chip-specific deactivation, such as
@@ -98,6 +99,7 @@ struct gpio_chip {
 	const char		*label;
 	struct device		*dev;
 	struct module		*owner;
+	struct list_head        list;
 
 	int			(*request)(struct gpio_chip *chip,
 						unsigned offset);
-- 
1.8.1.1

[PATCH 2/9] gpiolib: use gpio_chips list in gpiolib_sysfs_init

[Alexandre Courbot]

Use the small list of GPIO chips instead of parsing the whole GPIO
number space.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0050766..4ceb4a7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -973,7 +973,7 @@ static int __init gpiolib_sysfs_init(void)
 {
 	int		status;
 	unsigned long	flags;
-	unsigned	gpio;
+	struct gpio_chip *chip;
 
 	status = class_register(&gpio_class);
 	if (status < 0)
@@ -986,10 +986,7 @@ static int __init gpiolib_sysfs_init(void)
 	 * registered, and so arch_initcall() can always gpio_export().
 	 */
 	spin_lock_irqsave(&gpio_lock, flags);
-	for (gpio = 0; gpio < ARCH_NR_GPIOS; gpio++) {
-		struct gpio_chip	*chip;
-
-		chip = gpio_desc[gpio].chip;
+	list_for_each_entry(chip, &gpio_chips, list) {
 		if (!chip || chip->exported)
 			continue;
 
-- 
1.8.1.1

[PATCH 3/9] gpiolib: use gpio_chips list in gpiochip_find

[Alexandre Courbot]

Using the GPIO chips list is much faster than parsing the entire GPIO
number space.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4ceb4a7..92f9ee4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1194,20 +1194,17 @@ struct gpio_chip *gpiochip_find(void *data,
 				int (*match)(struct gpio_chip *chip,
 					     void *data))
 {
-	struct gpio_chip *chip = NULL;
+	struct gpio_chip *chip;
 	unsigned long flags;
-	int i;
 
 	spin_lock_irqsave(&gpio_lock, flags);
-	for (i = 0; i < ARCH_NR_GPIOS; i++) {
-		if (!gpio_desc[i].chip)
-			continue;
-
-		if (match(gpio_desc[i].chip, data)) {
-			chip = gpio_desc[i].chip;
+	list_for_each_entry(chip, &gpio_chips, list)
+		if (match(chip, data))
 			break;
-		}
-	}
+
+	/* No match? */
+	if (&chip->list == &gpio_chips)
+		chip = NULL;
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	return chip;
-- 
1.8.1.1

[PATCH 4/9] gpiolib: use gpio_chips list in sysfs ops

[Alexandre Courbot]

This makes the code both simpler and faster compared to parsing the GPIO
number space.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 92f9ee4..e473ded 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1889,45 +1889,28 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
 {
 	struct gpio_chip *chip = NULL;
-	unsigned int gpio;
-	void *ret = NULL;
-	loff_t index = 0;
+	loff_t index = *pos;
 
 	/* REVISIT this isn't locked against gpio_chip removal ... */
 
-	for (gpio = 0; gpio_is_valid(gpio); gpio++) {
-		if (gpio_desc[gpio].chip == chip)
-			continue;
-
-		chip = gpio_desc[gpio].chip;
-		if (!chip)
-			continue;
-
-		if (index++ >= *pos) {
-			ret = chip;
-			break;
-		}
-	}
-
 	s->private = "";
 
-	return ret;
+	list_for_each_entry(chip, &gpio_chips, list)
+		if (index-- == 0)
+			return chip;
+
+	return NULL;
 }
 
 static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
 	struct gpio_chip *chip = v;
-	unsigned int gpio;
 	void *ret = NULL;
 
-	/* skip GPIOs provided by the current chip */
-	for (gpio = chip->base + chip->ngpio; gpio_is_valid(gpio); gpio++) {
-		chip = gpio_desc[gpio].chip;
-		if (chip) {
-			ret = chip;
-			break;
-		}
-	}
+	if (list_is_last(&chip->list, &gpio_chips))
+		ret = NULL;
+	else
+		ret = list_entry(chip->list.next, struct gpio_chip, list);
 
 	s->private = "\n";
 	++*pos;
-- 
1.8.1.1

[PATCH 5/9] gpiolib: use gpio_chips list in gpiochip_find_base

[Alexandre Courbot]

Re-implement gpiochip_find_base using the list of chips instead of the
global gpio_desc[] array. This makes it both simpler and more efficient,
and is needed to remove the global descriptors array.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e473ded..af4c350 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -126,30 +126,25 @@ struct gpio_chip *gpio_to_chip(unsigned gpio)
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
 static int gpiochip_find_base(int ngpio)
 {
-	int i;
-	int spare = 0;
-	int base = -ENOSPC;
-
-	for (i = ARCH_NR_GPIOS - 1; i >= 0 ; i--) {
-		struct gpio_desc *desc = &gpio_desc[i];
-		struct gpio_chip *chip = desc->chip;
-
-		if (!chip) {
-			spare++;
-			if (spare == ngpio) {
-				base = i;
-				break;
-			}
-		} else {
-			spare = 0;
-			if (chip)
-				i -= chip->ngpio - 1;
-		}
+	struct gpio_chip *chip;
+	int base = ARCH_NR_GPIOS - ngpio;
+
+	list_for_each_entry_reverse(chip, &gpio_chips, list) {
+		/* found a free space? */
+		if (chip->base + chip->ngpio <= base)
+			break;
+		else
+			/* nope, check the space right before the chip */
+			base = chip->base - ngpio;
 	}
 
-	if (gpio_is_valid(base))
+	if (gpio_is_valid(base)) {
 		pr_debug("%s: found new base at %d\n", __func__, base);
-	return base;
+		return base;
+	} else {
+		pr_err("%s: cannot find free range\n", __func__);
+		return -ENOSPC;
+	}
 }
 
 /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
-- 
1.8.1.1

[PATCH 6/9] gpiolib: use descriptors internally

[Alexandre Courbot]

Make sure gpiolib works internally with descriptors and (chip, offset)
pairs instead of using the global integer namespace. This prepares the
ground for the removal of the global gpio_desc[] array and the
introduction of the descriptor-based GPIO API.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 493 +++++++++++++++++++++++++++++++------------------
 1 file changed, 317 insertions(+), 176 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index af4c350..82c40bd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -78,6 +78,28 @@ static LIST_HEAD(gpio_chips);
 static DEFINE_IDR(dirent_idr);
 #endif
 
+/*
+ * Internal gpiod_* API using descriptors instead of the integer namespace.
+ * Most of this should eventually go public.
+ */
+static int gpiod_request(struct gpio_desc *desc, const char *label);
+static void gpiod_free(struct gpio_desc *desc);
+static int gpiod_direction_input(struct gpio_desc *desc);
+static int gpiod_direction_output(struct gpio_desc *desc, int value);
+static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
+static int gpiod_get_value_cansleep(struct gpio_desc *desc);
+static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
+static int gpiod_get_value(struct gpio_desc *desc);
+static void gpiod_set_value(struct gpio_desc *desc, int value);
+static int gpiod_cansleep(struct gpio_desc *desc);
+static int gpiod_to_irq(struct gpio_desc *desc);
+static int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
+static int gpiod_export_link(struct device *dev, const char *name,
+			     struct gpio_desc *desc);
+static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
+static void gpiod_unexport(struct gpio_desc *desc);
+
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 #ifdef CONFIG_DEBUG_FS
@@ -85,6 +107,36 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
 #endif
 }
 
+/*
+ * Return the GPIO number of the passed descriptor relative to its chip
+ */
+static int gpio_chip_hwgpio(const struct gpio_desc *desc)
+{
+	return (desc - &gpio_desc[0]) - desc->chip->base;
+}
+
+/**
+ * Convert a GPIO number to its descriptor
+ */
+static struct gpio_desc *gpio_to_desc(unsigned gpio)
+{
+	if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
+		return NULL;
+	else
+		return &gpio_desc[gpio];
+}
+
+/**
+ * Convert a GPIO descriptor to the integer namespace.
+ * This should disappear in the future but is needed since we still
+ * use GPIO numbers for error messages and sysfs nodes
+ */
+static int desc_to_gpio(const struct gpio_desc *desc)
+{
+	return desc - &gpio_desc[0];
+}
+
+
 /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
  * when setting direction, and otherwise illegal.  Until board setup code
  * and drivers use explicit requests everywhere (which won't happen when
@@ -96,10 +148,10 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
  * only "legal" in the sense that (old) code using it won't break yet,
  * but instead only triggers a WARN() stack dump.
  */
-static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset)
+static int gpio_ensure_requested(struct gpio_desc *desc)
 {
 	const struct gpio_chip *chip = desc->chip;
-	const int gpio = chip->base + offset;
+	const int gpio = desc_to_gpio(desc);
 
 	if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
 			"autorequest GPIO-%d\n", gpio)) {
@@ -118,9 +170,14 @@ static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset)
 }
 
 /* caller holds gpio_lock *OR* gpio is marked as requested */
+static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc)
+{
+	return desc->chip;
+}
+
 struct gpio_chip *gpio_to_chip(unsigned gpio)
 {
-	return gpio_desc[gpio].chip;
+	return gpiod_to_chip(gpio_to_desc(gpio));
 }
 
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
@@ -148,19 +205,19 @@ static int gpiochip_find_base(int ngpio)
 }
 
 /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
-static int gpio_get_direction(unsigned gpio)
+static int gpiod_get_direction(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
-	struct gpio_desc	*desc = &gpio_desc[gpio];
+	unsigned		offset;
 	int			status = -EINVAL;
 
-	chip = gpio_to_chip(gpio);
-	gpio -= chip->base;
+	chip = gpiod_to_chip(desc);
+	offset = gpio_chip_hwgpio(desc);
 
 	if (!chip->get_direction)
 		return status;
 
-	status = chip->get_direction(chip, gpio);
+	status = chip->get_direction(chip, offset);
 	if (status > 0) {
 		/* GPIOF_DIR_IN, or other positive */
 		status = 1;
@@ -204,8 +261,7 @@ static DEFINE_MUTEX(sysfs_lock);
 static ssize_t gpio_direction_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	const struct gpio_desc	*desc = dev_get_drvdata(dev);
-	unsigned		gpio = desc - gpio_desc;
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -213,7 +269,7 @@ static ssize_t gpio_direction_show(struct device *dev,
 	if (!test_bit(FLAG_EXPORT, &desc->flags))
 		status = -EIO;
 	else
-		gpio_get_direction(gpio);
+		gpiod_get_direction(desc);
 		status = sprintf(buf, "%s\n",
 			test_bit(FLAG_IS_OUT, &desc->flags)
 				? "out" : "in");
@@ -225,8 +281,7 @@ static ssize_t gpio_direction_show(struct device *dev,
 static ssize_t gpio_direction_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	const struct gpio_desc	*desc = dev_get_drvdata(dev);
-	unsigned		gpio = desc - gpio_desc;
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -234,11 +289,11 @@ static ssize_t gpio_direction_store(struct device *dev,
 	if (!test_bit(FLAG_EXPORT, &desc->flags))
 		status = -EIO;
 	else if (sysfs_streq(buf, "high"))
-		status = gpio_direction_output(gpio, 1);
+		status = gpiod_direction_output(desc, 1);
 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
-		status = gpio_direction_output(gpio, 0);
+		status = gpiod_direction_output(desc, 0);
 	else if (sysfs_streq(buf, "in"))
-		status = gpio_direction_input(gpio);
+		status = gpiod_direction_input(desc);
 	else
 		status = -EINVAL;
 
@@ -252,8 +307,7 @@ static /* const */ DEVICE_ATTR(direction, 0644,
 static ssize_t gpio_value_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	const struct gpio_desc	*desc = dev_get_drvdata(dev);
-	unsigned		gpio = desc - gpio_desc;
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -263,7 +317,7 @@ static ssize_t gpio_value_show(struct device *dev,
 	} else {
 		int value;
 
-		value = !!gpio_get_value_cansleep(gpio);
+		value = !!gpiod_get_value_cansleep(desc);
 		if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
 			value = !value;
 
@@ -277,8 +331,7 @@ static ssize_t gpio_value_show(struct device *dev,
 static ssize_t gpio_value_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	const struct gpio_desc	*desc = dev_get_drvdata(dev);
-	unsigned		gpio = desc - gpio_desc;
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -294,7 +347,7 @@ static ssize_t gpio_value_store(struct device *dev,
 		if (status == 0) {
 			if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
 				value = !value;
-			gpio_set_value_cansleep(gpio, value != 0);
+			gpiod_set_value_cansleep(desc, value != 0);
 			status = size;
 		}
 	}
@@ -324,7 +377,7 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 	if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
 		return 0;
 
-	irq = gpio_to_irq(desc - gpio_desc);
+	irq = gpiod_to_irq(desc);
 	if (irq < 0)
 		return -EIO;
 
@@ -594,29 +647,32 @@ static ssize_t export_store(struct class *class,
 				struct class_attribute *attr,
 				const char *buf, size_t len)
 {
-	long	gpio;
-	int	status;
+	long			gpio;
+	struct gpio_desc	*desc;
+	int			status;
 
 	status = strict_strtol(buf, 0, &gpio);
 	if (status < 0)
 		goto done;
 
+	desc = gpio_to_desc(gpio);
+
 	/* No extra locking here; FLAG_SYSFS just signifies that the
 	 * request and export were done by on behalf of userspace, so
 	 * they may be undone on its behalf too.
 	 */
 
-	status = gpio_request(gpio, "sysfs");
+	status = gpiod_request(desc, "sysfs");
 	if (status < 0) {
 		if (status == -EPROBE_DEFER)
 			status = -ENODEV;
 		goto done;
 	}
-	status = gpio_export(gpio, true);
+	status = gpiod_export(desc, true);
 	if (status < 0)
-		gpio_free(gpio);
+		gpiod_free(desc);
 	else
-		set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
+		set_bit(FLAG_SYSFS, &desc->flags);
 
 done:
 	if (status)
@@ -628,8 +684,9 @@ static ssize_t unexport_store(struct class *class,
 				struct class_attribute *attr,
 				const char *buf, size_t len)
 {
-	long	gpio;
-	int	status;
+	long			gpio;
+	struct gpio_desc	*desc;
+	int			status;
 
 	status = strict_strtol(buf, 0, &gpio);
 	if (status < 0)
@@ -637,17 +694,18 @@ static ssize_t unexport_store(struct class *class,
 
 	status = -EINVAL;
 
+	desc = gpio_to_desc(gpio);
 	/* reject bogus commands (gpio_unexport ignores them) */
-	if (!gpio_is_valid(gpio))
+	if (!desc)
 		goto done;
 
 	/* No extra locking here; FLAG_SYSFS just signifies that the
 	 * request and export were done by on behalf of userspace, so
 	 * they may be undone on its behalf too.
 	 */
-	if (test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags)) {
+	if (test_and_clear_bit(FLAG_SYSFS, &desc->flags)) {
 		status = 0;
-		gpio_free(gpio);
+		gpiod_free(desc);
 	}
 done:
 	if (status)
@@ -684,13 +742,13 @@ static struct class gpio_class = {
  *
  * Returns zero on success, else an error.
  */
-int gpio_export(unsigned gpio, bool direction_may_change)
+static int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
 	unsigned long		flags;
-	struct gpio_desc	*desc;
 	int			status;
 	const char		*ioname = NULL;
 	struct device		*dev;
+	int			offset;
 
 	/* can't export until sysfs is available ... */
 	if (!gpio_class.p) {
@@ -698,20 +756,19 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 		return -ENOENT;
 	}
 
-	if (!gpio_is_valid(gpio)) {
-		pr_debug("%s: gpio %d is not valid\n", __func__, gpio);
+	if (!desc) {
+		pr_debug("%s: invalid gpio descriptor\n", __func__);
 		return -EINVAL;
 	}
 
 	mutex_lock(&sysfs_lock);
 
 	spin_lock_irqsave(&gpio_lock, flags);
-	desc = &gpio_desc[gpio];
 	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
 	     test_bit(FLAG_EXPORT, &desc->flags)) {
 		spin_unlock_irqrestore(&gpio_lock, flags);
 		pr_debug("%s: gpio %d unavailable (requested=%d, exported=%d)\n",
-				__func__, gpio,
+				__func__, desc_to_gpio(desc),
 				test_bit(FLAG_REQUESTED, &desc->flags),
 				test_bit(FLAG_EXPORT, &desc->flags));
 		status = -EPERM;
@@ -722,11 +779,13 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 		direction_may_change = false;
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
-		ioname = desc->chip->names[gpio - desc->chip->base];
+	offset = gpio_chip_hwgpio(desc);
+	if (desc->chip->names && desc->chip->names[offset])
+		ioname = desc->chip->names[offset];
 
 	dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
-			    desc, ioname ? ioname : "gpio%u", gpio);
+			    desc, ioname ? ioname : "gpio%u",
+			    desc_to_gpio(desc));
 	if (IS_ERR(dev)) {
 		status = PTR_ERR(dev);
 		goto fail_unlock;
@@ -742,7 +801,7 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 			goto fail_unregister_device;
 	}
 
-	if (gpio_to_irq(gpio) >= 0 && (direction_may_change ||
+	if (gpiod_to_irq(desc) >= 0 && (direction_may_change ||
 				       !test_bit(FLAG_IS_OUT, &desc->flags))) {
 		status = device_create_file(dev, &dev_attr_edge);
 		if (status)
@@ -757,9 +816,15 @@ fail_unregister_device:
 	device_unregister(dev);
 fail_unlock:
 	mutex_unlock(&sysfs_lock);
-	pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+	pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
+		 status);
 	return status;
 }
+
+int gpio_export(unsigned gpio, bool direction_may_change)
+{
+	return gpiod_export(gpio_to_desc(gpio), direction_may_change);
+}
 EXPORT_SYMBOL_GPL(gpio_export);
 
 static int match_export(struct device *dev, void *data)
@@ -778,18 +843,16 @@ static int match_export(struct device *dev, void *data)
  *
  * Returns zero on success, else an error.
  */
-int gpio_export_link(struct device *dev, const char *name, unsigned gpio)
+static int gpiod_export_link(struct device *dev, const char *name,
+			     struct gpio_desc *desc)
 {
-	struct gpio_desc	*desc;
 	int			status = -EINVAL;
 
-	if (!gpio_is_valid(gpio))
+	if (!desc)
 		goto done;
 
 	mutex_lock(&sysfs_lock);
 
-	desc = &gpio_desc[gpio];
-
 	if (test_bit(FLAG_EXPORT, &desc->flags)) {
 		struct device *tdev;
 
@@ -806,12 +869,17 @@ int gpio_export_link(struct device *dev, const char *name, unsigned gpio)
 
 done:
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
+			 status);
 
 	return status;
 }
-EXPORT_SYMBOL_GPL(gpio_export_link);
 
+int gpio_export_link(struct device *dev, const char *name, unsigned gpio)
+{
+	return gpiod_export_link(dev, name, gpio_to_desc(gpio));
+}
+EXPORT_SYMBOL_GPL(gpio_export_link);
 
 /**
  * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
@@ -825,19 +893,16 @@ EXPORT_SYMBOL_GPL(gpio_export_link);
  *
  * Returns zero on success, else an error.
  */
-int gpio_sysfs_set_active_low(unsigned gpio, int value)
+static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
 {
-	struct gpio_desc	*desc;
 	struct device		*dev = NULL;
 	int			status = -EINVAL;
 
-	if (!gpio_is_valid(gpio))
+	if (!desc)
 		goto done;
 
 	mutex_lock(&sysfs_lock);
 
-	desc = &gpio_desc[gpio];
-
 	if (test_bit(FLAG_EXPORT, &desc->flags)) {
 		dev = class_find_device(&gpio_class, NULL, desc, match_export);
 		if (dev == NULL) {
@@ -853,10 +918,16 @@ unlock:
 
 done:
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
+			 status);
 
 	return status;
 }
+
+int gpio_sysfs_set_active_low(unsigned gpio, int value)
+{
+	return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value);
+}
 EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low);
 
 /**
@@ -865,21 +936,18 @@ EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low);
  *
  * This is implicit on gpio_free().
  */
-void gpio_unexport(unsigned gpio)
+static void gpiod_unexport(struct gpio_desc *desc)
 {
-	struct gpio_desc	*desc;
 	int			status = 0;
 	struct device		*dev = NULL;
 
-	if (!gpio_is_valid(gpio)) {
+	if (!desc) {
 		status = -EINVAL;
 		goto done;
 	}
 
 	mutex_lock(&sysfs_lock);
 
-	desc = &gpio_desc[gpio];
-
 	if (test_bit(FLAG_EXPORT, &desc->flags)) {
 
 		dev = class_find_device(&gpio_class, NULL, desc, match_export);
@@ -891,13 +959,20 @@ void gpio_unexport(unsigned gpio)
 	}
 
 	mutex_unlock(&sysfs_lock);
+
 	if (dev) {
 		device_unregister(dev);
 		put_device(dev);
 	}
 done:
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
+			 status);
+}
+
+void gpio_unexport(unsigned gpio)
+{
+	gpiod_unexport(gpio_to_desc(gpio));
 }
 EXPORT_SYMBOL_GPL(gpio_unexport);
 
@@ -1281,20 +1356,18 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
  * 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)
+static int gpiod_request(struct gpio_desc *desc, const char *label)
 {
-	struct gpio_desc	*desc;
 	struct gpio_chip	*chip;
 	int			status = -EPROBE_DEFER;
 	unsigned long		flags;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!gpio_is_valid(gpio)) {
+	if (!desc) {
 		status = -EINVAL;
 		goto done;
 	}
-	desc = &gpio_desc[gpio];
 	chip = desc->chip;
 	if (chip == NULL)
 		goto done;
@@ -1318,7 +1391,7 @@ int gpio_request(unsigned gpio, const char *label)
 	if (chip->request) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = chip->request(chip, gpio - chip->base);
+		status = chip->request(chip, gpio_chip_hwgpio(desc));
 		spin_lock_irqsave(&gpio_lock, flags);
 
 		if (status < 0) {
@@ -1331,42 +1404,46 @@ int gpio_request(unsigned gpio, const char *label)
 	if (chip->get_direction) {
 		/* chip->get_direction may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		gpio_get_direction(gpio);
+		gpiod_get_direction(desc);
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 done:
 	if (status)
-		pr_debug("gpio_request: gpio-%d (%s) status %d\n",
-			gpio, label ? : "?", status);
+		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
+			 desc ? desc_to_gpio(desc) : -1,
+			 label ? : "?", status);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
+
+int gpio_request(unsigned gpio, const char *label)
+{
+	return gpiod_request(gpio_to_desc(gpio), label);
+}
 EXPORT_SYMBOL_GPL(gpio_request);
 
-void gpio_free(unsigned gpio)
+static void gpiod_free(struct gpio_desc *desc)
 {
 	unsigned long		flags;
-	struct gpio_desc	*desc;
 	struct gpio_chip	*chip;
 
 	might_sleep();
 
-	if (!gpio_is_valid(gpio)) {
+	if (!desc) {
 		WARN_ON(extra_checks);
 		return;
 	}
 
-	gpio_unexport(gpio);
+	gpiod_unexport(desc);
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	desc = &gpio_desc[gpio];
 	chip = desc->chip;
 	if (chip && test_bit(FLAG_REQUESTED, &desc->flags)) {
 		if (chip->free) {
 			spin_unlock_irqrestore(&gpio_lock, flags);
 			might_sleep_if(chip->can_sleep);
-			chip->free(chip, gpio - chip->base);
+			chip->free(chip, gpio_chip_hwgpio(desc));
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
 		desc_set_label(desc, NULL);
@@ -1380,6 +1457,11 @@ void gpio_free(unsigned gpio)
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 }
+
+void gpio_free(unsigned gpio)
+{
+	gpiod_free(gpio_to_desc(gpio));
+}
 EXPORT_SYMBOL_GPL(gpio_free);
 
 /**
@@ -1390,29 +1472,32 @@ EXPORT_SYMBOL_GPL(gpio_free);
  */
 int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 {
+	struct gpio_desc *desc;
 	int err;
 
-	err = gpio_request(gpio, label);
+	desc = gpio_to_desc(gpio);
+
+	err = gpiod_request(desc, label);
 	if (err)
 		return err;
 
 	if (flags & GPIOF_OPEN_DRAIN)
-		set_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags);
+		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 
 	if (flags & GPIOF_OPEN_SOURCE)
-		set_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags);
+		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
 	if (flags & GPIOF_DIR_IN)
-		err = gpio_direction_input(gpio);
+		err = gpiod_direction_input(desc);
 	else
-		err = gpio_direction_output(gpio,
+		err = gpiod_direction_output(desc,
 				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
 
 	if (err)
 		goto free_gpio;
 
 	if (flags & GPIOF_EXPORT) {
-		err = gpio_export(gpio, flags & GPIOF_EXPORT_CHANGEABLE);
+		err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
 		if (err)
 			goto free_gpio;
 	}
@@ -1420,7 +1505,7 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	return 0;
 
  free_gpio:
-	gpio_free(gpio);
+	gpiod_free(desc);
 	return err;
 }
 EXPORT_SYMBOL_GPL(gpio_request_one);
@@ -1476,13 +1561,14 @@ EXPORT_SYMBOL_GPL(gpio_free_array);
 const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
 {
 	unsigned gpio = chip->base + offset;
+	struct gpio_desc *desc = &gpio_desc[gpio];
 
-	if (!gpio_is_valid(gpio) || gpio_desc[gpio].chip != chip)
+	if (!gpio_is_valid(gpio) || desc->chip != chip)
 		return NULL;
-	if (test_bit(FLAG_REQUESTED, &gpio_desc[gpio].flags) == 0)
+	if (test_bit(FLAG_REQUESTED, &desc->flags) == 0)
 		return NULL;
 #ifdef CONFIG_DEBUG_FS
-	return gpio_desc[gpio].label;
+	return desc->label;
 #else
 	return "?";
 #endif
@@ -1499,24 +1585,21 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  * rely on gpio_request() having been called beforehand.
  */
 
-int gpio_direction_input(unsigned gpio)
+static int gpiod_direction_input(struct gpio_desc *desc)
 {
 	unsigned long		flags;
 	struct gpio_chip	*chip;
-	struct gpio_desc	*desc = &gpio_desc[gpio];
 	int			status = -EINVAL;
+	int			offset;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!gpio_is_valid(gpio))
+	if (!desc)
 		goto fail;
 	chip = desc->chip;
 	if (!chip || !chip->get || !chip->direction_input)
 		goto fail;
-	gpio -= chip->base;
-	if (gpio >= chip->ngpio)
-		goto fail;
-	status = gpio_ensure_requested(desc, gpio);
+	status = gpio_ensure_requested(desc);
 	if (status < 0)
 		goto fail;
 
@@ -1526,11 +1609,12 @@ int gpio_direction_input(unsigned gpio)
 
 	might_sleep_if(chip->can_sleep);
 
+	offset = gpio_chip_hwgpio(desc);
 	if (status) {
-		status = chip->request(chip, gpio);
+		status = chip->request(chip, offset);
 		if (status < 0) {
 			pr_debug("GPIO-%d: chip request fail, %d\n",
-				chip->base + gpio, status);
+				desc_to_gpio(desc), status);
 			/* and it's not available to anyone else ...
 			 * gpio_request() is the fully clean solution.
 			 */
@@ -1538,48 +1622,54 @@ int gpio_direction_input(unsigned gpio)
 		}
 	}
 
-	status = chip->direction_input(chip, gpio);
+	status = chip->direction_input(chip, offset);
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
-	trace_gpio_direction(chip->base + gpio, 1, status);
+	trace_gpio_direction(desc_to_gpio(desc), 1, status);
 lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status)
+	if (status) {
+		int gpio = -1;
+		if (desc)
+			gpio = desc_to_gpio(desc);
 		pr_debug("%s: gpio-%d status %d\n",
 			__func__, gpio, status);
+	}
 	return status;
 }
+
+int gpio_direction_input(unsigned gpio)
+{
+	return gpiod_direction_input(gpio_to_desc(gpio));
+}
 EXPORT_SYMBOL_GPL(gpio_direction_input);
 
-int gpio_direction_output(unsigned gpio, int value)
+static int gpiod_direction_output(struct gpio_desc *desc, int value)
 {
 	unsigned long		flags;
 	struct gpio_chip	*chip;
-	struct gpio_desc	*desc = &gpio_desc[gpio];
 	int			status = -EINVAL;
+	int offset;
 
 	/* Open drain pin should not be driven to 1 */
 	if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
-		return gpio_direction_input(gpio);
+		return gpiod_direction_input(desc);
 
 	/* Open source pin should not be driven to 0 */
 	if (!value && test_bit(FLAG_OPEN_SOURCE,  &desc->flags))
-		return gpio_direction_input(gpio);
+		return gpiod_direction_input(desc);
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!gpio_is_valid(gpio))
+	if (!desc)
 		goto fail;
 	chip = desc->chip;
 	if (!chip || !chip->set || !chip->direction_output)
 		goto fail;
-	gpio -= chip->base;
-	if (gpio >= chip->ngpio)
-		goto fail;
-	status = gpio_ensure_requested(desc, gpio);
+	status = gpio_ensure_requested(desc);
 	if (status < 0)
 		goto fail;
 
@@ -1589,11 +1679,12 @@ int gpio_direction_output(unsigned gpio, int value)
 
 	might_sleep_if(chip->can_sleep);
 
+	offset = gpio_chip_hwgpio(desc);
 	if (status) {
-		status = chip->request(chip, gpio);
+		status = chip->request(chip, offset);
 		if (status < 0) {
 			pr_debug("GPIO-%d: chip request fail, %d\n",
-				chip->base + gpio, status);
+				desc_to_gpio(desc), status);
 			/* and it's not available to anyone else ...
 			 * gpio_request() is the fully clean solution.
 			 */
@@ -1601,20 +1692,29 @@ int gpio_direction_output(unsigned gpio, int value)
 		}
 	}
 
-	status = chip->direction_output(chip, gpio, value);
+	status = chip->direction_output(chip, offset, value);
 	if (status == 0)
 		set_bit(FLAG_IS_OUT, &desc->flags);
-	trace_gpio_value(chip->base + gpio, 0, value);
-	trace_gpio_direction(chip->base + gpio, 0, status);
+	trace_gpio_value(desc_to_gpio(desc), 0, value);
+	trace_gpio_direction(desc_to_gpio(desc), 0, status);
 lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status)
+	if (status) {
+		int gpio = -1;
+		if (desc)
+			gpio = desc_to_gpio(desc);
 		pr_debug("%s: gpio-%d status %d\n",
 			__func__, gpio, status);
+	}
 	return status;
 }
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+	return gpiod_direction_output(gpio_to_desc(gpio), value);
+}
 EXPORT_SYMBOL_GPL(gpio_direction_output);
 
 /**
@@ -1622,24 +1722,22 @@ EXPORT_SYMBOL_GPL(gpio_direction_output);
  * @gpio: the gpio to set debounce time
  * @debounce: debounce time is microseconds
  */
-int gpio_set_debounce(unsigned gpio, unsigned debounce)
+static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 {
 	unsigned long		flags;
 	struct gpio_chip	*chip;
-	struct gpio_desc	*desc = &gpio_desc[gpio];
 	int			status = -EINVAL;
+	int			offset;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!gpio_is_valid(gpio))
+	if (!desc)
 		goto fail;
 	chip = desc->chip;
 	if (!chip || !chip->set || !chip->set_debounce)
 		goto fail;
-	gpio -= chip->base;
-	if (gpio >= chip->ngpio)
-		goto fail;
-	status = gpio_ensure_requested(desc, gpio);
+
+	status = gpio_ensure_requested(desc);
 	if (status < 0)
 		goto fail;
 
@@ -1649,16 +1747,26 @@ int gpio_set_debounce(unsigned gpio, unsigned debounce)
 
 	might_sleep_if(chip->can_sleep);
 
-	return chip->set_debounce(chip, gpio, debounce);
+	offset = gpio_chip_hwgpio(desc);
+	return chip->set_debounce(chip, offset, debounce);
 
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status)
+	if (status) {
+		int gpio = -1;
+		if (desc)
+			gpio = desc_to_gpio(desc);
 		pr_debug("%s: gpio-%d status %d\n",
 			__func__, gpio, status);
+	}
 
 	return status;
 }
+
+int gpio_set_debounce(unsigned gpio, unsigned debounce)
+{
+	return gpiod_set_debounce(gpio_to_desc(gpio), debounce);
+}
 EXPORT_SYMBOL_GPL(gpio_set_debounce);
 
 /* I/O calls are only valid after configuration completed; the relevant
@@ -1692,18 +1800,25 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce);
  * It returns the zero or nonzero value provided by the associated
  * gpio_chip.get() method; or zero if no such method is provided.
  */
-int __gpio_get_value(unsigned gpio)
+static int gpiod_get_value(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
+	int offset;
 
-	chip = gpio_to_chip(gpio);
+	chip = desc->chip;
+	offset = gpio_chip_hwgpio(desc);
 	/* Should be using gpio_get_value_cansleep() */
 	WARN_ON(chip->can_sleep);
-	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
-	trace_gpio_value(gpio, 1, value);
+	value = chip->get ? chip->get(chip, offset) : 0;
+	trace_gpio_value(desc_to_gpio(desc), 1, value);
 	return value;
 }
+
+int __gpio_get_value(unsigned gpio)
+{
+	return gpiod_get_value(gpio_to_desc(gpio));
+}
 EXPORT_SYMBOL_GPL(__gpio_get_value);
 
 /*
@@ -1712,23 +1827,25 @@ EXPORT_SYMBOL_GPL(__gpio_get_value);
  * @chip: Gpio chip.
  * @value: Non-zero for setting it HIGH otherise it will set to LOW.
  */
-static void _gpio_set_open_drain_value(unsigned gpio,
-			struct gpio_chip *chip, int value)
+static void _gpio_set_open_drain_value(struct gpio_desc *desc, int value)
 {
 	int err = 0;
+	struct gpio_chip *chip = desc->chip;
+	int offset = gpio_chip_hwgpio(desc);
+
 	if (value) {
-		err = chip->direction_input(chip, gpio - chip->base);
+		err = chip->direction_input(chip, offset);
 		if (!err)
-			clear_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags);
+			clear_bit(FLAG_IS_OUT, &desc->flags);
 	} else {
-		err = chip->direction_output(chip, gpio - chip->base, 0);
+		err = chip->direction_output(chip, offset, 0);
 		if (!err)
-			set_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags);
+			set_bit(FLAG_IS_OUT, &desc->flags);
 	}
-	trace_gpio_direction(gpio, value, err);
+	trace_gpio_direction(desc_to_gpio(desc), value, err);
 	if (err < 0)
 		pr_err("%s: Error in set_value for open drain gpio%d err %d\n",
-					__func__, gpio, err);
+					__func__, desc_to_gpio(desc), err);
 }
 
 /*
@@ -1737,26 +1854,27 @@ static void _gpio_set_open_drain_value(unsigned gpio,
  * @chip: Gpio chip.
  * @value: Non-zero for setting it HIGH otherise it will set to LOW.
  */
-static void _gpio_set_open_source_value(unsigned gpio,
-			struct gpio_chip *chip, int value)
+static void _gpio_set_open_source_value(struct gpio_desc *desc, int value)
 {
 	int err = 0;
+	struct gpio_chip *chip = desc->chip;
+	int offset = gpio_chip_hwgpio(desc);
+
 	if (value) {
-		err = chip->direction_output(chip, gpio - chip->base, 1);
+		err = chip->direction_output(chip, offset, 1);
 		if (!err)
-			set_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags);
+			set_bit(FLAG_IS_OUT, &desc->flags);
 	} else {
-		err = chip->direction_input(chip, gpio - chip->base);
+		err = chip->direction_input(chip, offset);
 		if (!err)
-			clear_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags);
+			clear_bit(FLAG_IS_OUT, &desc->flags);
 	}
-	trace_gpio_direction(gpio, !value, err);
+	trace_gpio_direction(desc_to_gpio(desc), !value, err);
 	if (err < 0)
 		pr_err("%s: Error in set_value for open source gpio%d err %d\n",
-					__func__, gpio, err);
+					__func__, desc_to_gpio(desc), err);
 }
 
-
 /**
  * __gpio_set_value() - assign a gpio's value
  * @gpio: gpio whose value will be assigned
@@ -1766,20 +1884,25 @@ static void _gpio_set_open_source_value(unsigned gpio,
  * This is used directly or indirectly to implement gpio_set_value().
  * It invokes the associated gpio_chip.set() method.
  */
-void __gpio_set_value(unsigned gpio, int value)
+static void gpiod_set_value(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip	*chip;
 
-	chip = gpio_to_chip(gpio);
+	chip = desc->chip;
 	/* Should be using gpio_set_value_cansleep() */
 	WARN_ON(chip->can_sleep);
-	trace_gpio_value(gpio, 0, value);
-	if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
-		_gpio_set_open_drain_value(gpio, chip, value);
-	else if (test_bit(FLAG_OPEN_SOURCE,  &gpio_desc[gpio].flags))
-		_gpio_set_open_source_value(gpio, chip, value);
+	trace_gpio_value(desc_to_gpio(desc), 0, value);
+	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
+		_gpio_set_open_drain_value(desc, value);
+	else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
+		_gpio_set_open_source_value(desc, value);
 	else
-		chip->set(chip, gpio - chip->base, value);
+		chip->set(chip, gpio_chip_hwgpio(desc), value);
+}
+
+void __gpio_set_value(unsigned gpio, int value)
+{
+	return gpiod_set_value(gpio_to_desc(gpio), value);
 }
 EXPORT_SYMBOL_GPL(__gpio_set_value);
 
@@ -1791,14 +1914,15 @@ EXPORT_SYMBOL_GPL(__gpio_set_value);
  * This is used directly or indirectly to implement gpio_cansleep().  It
  * returns nonzero if access reading or writing the GPIO value can sleep.
  */
-int __gpio_cansleep(unsigned gpio)
+static int gpiod_cansleep(struct gpio_desc *desc)
 {
-	struct gpio_chip	*chip;
-
 	/* only call this on GPIOs that are valid! */
-	chip = gpio_to_chip(gpio);
+	return desc->chip->can_sleep;
+}
 
-	return chip->can_sleep;
+int __gpio_cansleep(unsigned gpio)
+{
+	return gpiod_cansleep(gpio_to_desc(gpio));
 }
 EXPORT_SYMBOL_GPL(__gpio_cansleep);
 
@@ -1811,50 +1935,67 @@ EXPORT_SYMBOL_GPL(__gpio_cansleep);
  * It returns the number of the IRQ signaled by this (input) GPIO,
  * or a negative errno.
  */
-int __gpio_to_irq(unsigned gpio)
+static int gpiod_to_irq(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
+	int			offset;
 
-	chip = gpio_to_chip(gpio);
-	return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) : -ENXIO;
+	chip = desc->chip;
+	offset = gpio_chip_hwgpio(desc);
+	return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO;
 }
-EXPORT_SYMBOL_GPL(__gpio_to_irq);
 
+int __gpio_to_irq(unsigned gpio)
+{
+	return gpiod_to_irq(gpio_to_desc(gpio));
+}
+EXPORT_SYMBOL_GPL(__gpio_to_irq);
 
 
 /* There's no value in making it easy to inline GPIO calls that may sleep.
  * Common examples include ones connected to I2C or SPI chips.
  */
 
-int gpio_get_value_cansleep(unsigned gpio)
+static int gpiod_get_value_cansleep(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
+	int offset;
 
 	might_sleep_if(extra_checks);
-	chip = gpio_to_chip(gpio);
-	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
-	trace_gpio_value(gpio, 1, value);
+	chip = desc->chip;
+	offset = gpio_chip_hwgpio(desc);
+	value = chip->get ? chip->get(chip, offset) : 0;
+	trace_gpio_value(desc_to_gpio(desc), 1, value);
 	return value;
 }
+
+int gpio_get_value_cansleep(unsigned gpio)
+{
+	return gpiod_get_value_cansleep(gpio_to_desc(gpio));
+}
 EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
 
-void gpio_set_value_cansleep(unsigned gpio, int value)
+static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip	*chip;
 
 	might_sleep_if(extra_checks);
-	chip = gpio_to_chip(gpio);
-	trace_gpio_value(gpio, 0, value);
-	if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
-		_gpio_set_open_drain_value(gpio, chip, value);
-	else if (test_bit(FLAG_OPEN_SOURCE,  &gpio_desc[gpio].flags))
-		_gpio_set_open_source_value(gpio, chip, value);
+	chip = desc->chip;
+	trace_gpio_value(desc_to_gpio(desc), 0, value);
+	if (test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
+		_gpio_set_open_drain_value(desc, value);
+	else if (test_bit(FLAG_OPEN_SOURCE,  &desc->flags))
+		_gpio_set_open_source_value(desc, value);
 	else
-		chip->set(chip, gpio - chip->base, value);
+		chip->set(chip, gpio_chip_hwgpio(desc), value);
 }
-EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
 
+void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+	return gpiod_set_value_cansleep(gpio_to_desc(gpio), value);
+}
+EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
 
 #ifdef CONFIG_DEBUG_FS
 
@@ -1869,7 +2010,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 		if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
 			continue;
 
-		gpio_get_direction(gpio);
+		gpiod_get_direction(gdesc);
 		is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
 		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
 			gpio, gdesc->label,
-- 
1.8.1.1

[PATCH 7/9] gpiolib: let gpio_chip reference its descriptors

[Alexandre Courbot]

Add a pointer to the gpio_chip structure that references the array of
GPIO descriptors belonging to the chip, and update gpiolib code to use
this pointer instead of the global gpio_desc[] array. This is another
step towards the removal of the gpio_desc[] global array.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c     | 39 +++++++++++++++++++++++----------------
 include/asm-generic/gpio.h |  3 +++
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 82c40bd..9599b9a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -72,6 +72,8 @@ struct gpio_desc {
 };
 static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
+#define GPIO_OFFSET_VALID(chip, offset) (offset >= 0 && offset < chip->ngpio)
+
 static LIST_HEAD(gpio_chips);
 
 #ifdef CONFIG_GPIO_SYSFS
@@ -112,7 +114,7 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
  */
 static int gpio_chip_hwgpio(const struct gpio_desc *desc)
 {
-	return (desc - &gpio_desc[0]) - desc->chip->base;
+	return desc - &desc->chip->desc[0];
 }
 
 /**
@@ -133,7 +135,7 @@ static struct gpio_desc *gpio_to_desc(unsigned gpio)
  */
 static int desc_to_gpio(const struct gpio_desc *desc)
 {
-	return desc - &gpio_desc[0];
+	return desc->chip->base + gpio_chip_hwgpio(desc);
 }
 
 
@@ -1006,9 +1008,9 @@ static int gpiochip_export(struct gpio_chip *chip)
 		unsigned	gpio;
 
 		spin_lock_irqsave(&gpio_lock, flags);
-		gpio = chip->base;
-		while (gpio_desc[gpio].chip == chip)
-			gpio_desc[gpio++].chip = NULL;
+		gpio = 0;
+		while (gpio < chip->ngpio)
+			chip->desc[gpio++].chip = NULL;
 		spin_unlock_irqrestore(&gpio_lock, flags);
 
 		pr_debug("%s: chip %s status %d\n", __func__,
@@ -1164,8 +1166,11 @@ int gpiochip_add(struct gpio_chip *chip)
 	status = gpiochip_add_to_list(chip);
 
 	if (status == 0) {
-		for (id = base; id < base + chip->ngpio; id++) {
-			gpio_desc[id].chip = chip;
+		chip->desc = &gpio_desc[chip->base];
+
+		for (id = 0; id < chip->ngpio; id++) {
+			struct gpio_desc *desc = &chip->desc[id];
+			desc->chip = chip;
 
 			/* REVISIT:  most hardware initializes GPIOs as
 			 * inputs (often with pullups enabled) so power
@@ -1174,7 +1179,7 @@ int gpiochip_add(struct gpio_chip *chip)
 			 * and in case chip->get_direction is not set,
 			 * we may expose the wrong direction in sysfs.
 			 */
-			gpio_desc[id].flags = !chip->direction_input
+			desc->flags = !chip->direction_input
 				? (1 << FLAG_IS_OUT)
 				: 0;
 		}
@@ -1227,15 +1232,15 @@ int gpiochip_remove(struct gpio_chip *chip)
 	gpiochip_remove_pin_ranges(chip);
 	of_gpiochip_remove(chip);
 
-	for (id = chip->base; id < chip->base + chip->ngpio; id++) {
-		if (test_bit(FLAG_REQUESTED, &gpio_desc[id].flags)) {
+	for (id = 0; id < chip->ngpio; id++) {
+		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
 			status = -EBUSY;
 			break;
 		}
 	}
 	if (status == 0) {
-		for (id = chip->base; id < chip->base + chip->ngpio; id++)
-			gpio_desc[id].chip = NULL;
+		for (id = 0; id < chip->ngpio; id++)
+			chip->desc[id].chip = NULL;
 
 		list_del(&chip->list);
 	}
@@ -1560,11 +1565,13 @@ EXPORT_SYMBOL_GPL(gpio_free_array);
  */
 const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
 {
-	unsigned gpio = chip->base + offset;
-	struct gpio_desc *desc = &gpio_desc[gpio];
+	struct gpio_desc *desc;
 
-	if (!gpio_is_valid(gpio) || desc->chip != chip)
+	if (!GPIO_OFFSET_VALID(chip, offset))
 		return NULL;
+
+	desc = &chip->desc[offset];
+
 	if (test_bit(FLAG_REQUESTED, &desc->flags) == 0)
 		return NULL;
 #ifdef CONFIG_DEBUG_FS
@@ -2003,7 +2010,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 {
 	unsigned		i;
 	unsigned		gpio = chip->base;
-	struct gpio_desc	*gdesc = &gpio_desc[gpio];
+	struct gpio_desc	*gdesc = &chip->desc[0];
 	int			is_out;
 
 	for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index b562f95..bde6469 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -47,6 +47,7 @@ struct gpio;
 struct seq_file;
 struct module;
 struct device_node;
+struct gpio_desc;
 
 /**
  * struct gpio_chip - abstract a GPIO controller
@@ -76,6 +77,7 @@ struct device_node;
  *	negative during registration, requests dynamic ID allocation.
  * @ngpio: the number of GPIOs handled by this controller; the last GPIO
  *	handled is (base + ngpio - 1).
+ * @desc: array of ngpio descriptors. Private.
  * @can_sleep: flag must be set iff get()/set() methods sleep, as they
  *	must while accessing GPIO expander chips over I2C or SPI
  * @names: if set, must be an array of strings to use as alternative
@@ -126,6 +128,7 @@ struct gpio_chip {
 						struct gpio_chip *chip);
 	int			base;
 	u16			ngpio;
+	struct gpio_desc	*desc;
 	const char		*const *names;
 	unsigned		can_sleep:1;
 	unsigned		exported:1;
-- 
1.8.1.1

[PATCH 8/9] gpiolib: use gpio_chips list in gpio_to_desc

[Alexandre Courbot]

Parse the list of chips to find the descriptor corresponding to a GPIO
number instead of directly picking the entry of the global gpio_desc[]
array, which is due to be removed.

This turns the complexity of converting a GPIO number into a descriptor
from O(1) to O(n) where n is the number of GPIO chips in the system.
Since n is ought to be small anyway, there should be no noticeable
performance impact. Moreover, GPIO users who care for speed already have
implemented their own gpio_get_value() and gpio_set_value() with a
fast path for the GPIO numbers that matter and this change does not
affect such use cases.

The descriptor-based GPIO API, due to be introduced soon, will make this
lookup unnecessary.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9599b9a..0247c48 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -122,10 +122,21 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
  */
 static struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
-	if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
-		return NULL;
-	else
-		return &gpio_desc[gpio];
+	struct gpio_chip *chip;
+
+	list_for_each_entry(chip, &gpio_chips, list) {
+		int gpio_min = chip->base;
+		int gpio_max = gpio_min + chip->ngpio;
+		if (gpio >= gpio_min && gpio < gpio_max)
+			return &chip->desc[gpio - gpio_min];
+		else if (gpio < gpio_min)
+			/* gpio_chips are ordered by base, so we won't get any
+			 * hit if we arrive here... */
+			break;
+	}
+
+	pr_warn("%s: no registered chip to handle GPIO %d\n", __func__, gpio);
+	return NULL;
 }
 
 /**
-- 
1.8.1.1

[PATCH 9/9] gpiolib: dynamically allocate descriptors array

[Alexandre Courbot]

Allocate the GPIO descriptor's memory dynamically when GPIO chips are
added instead of using the static gpio_desc[] array. gpio_desc[] is now
totally unused and therefore removed.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0247c48..893cf94 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -41,7 +41,7 @@
 #define	extra_checks	0
 #endif
 
-/* gpio_lock prevents conflicts during gpio_desc[] table updates.
+/* gpio_lock prevents conflicts during gpio descriptors updates.
  * While any GPIO is requested, its gpio_chip is not removable;
  * each GPIO's "requested" flag serves as a lock and refcount.
  */
@@ -70,7 +70,6 @@ struct gpio_desc {
 	const char		*label;
 #endif
 };
-static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
 #define GPIO_OFFSET_VALID(chip, offset) (offset >= 0 && offset < chip->ngpio)
 
@@ -1163,6 +1162,13 @@ int gpiochip_add(struct gpio_chip *chip)
 		goto fail;
 	}
 
+	chip->desc = kzalloc(sizeof(struct gpio_desc) * chip->ngpio,
+			     GFP_KERNEL);
+	if (!chip->desc) {
+		status = -ENOMEM;
+		goto fail;
+	}
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	if (base < 0) {
@@ -1177,8 +1183,6 @@ int gpiochip_add(struct gpio_chip *chip)
 	status = gpiochip_add_to_list(chip);
 
 	if (status == 0) {
-		chip->desc = &gpio_desc[chip->base];
-
 		for (id = 0; id < chip->ngpio; id++) {
 			struct gpio_desc *desc = &chip->desc[id];
 			desc->chip = chip;
@@ -1218,6 +1222,8 @@ unlock:
 
 	return 0;
 fail:
+	kfree(chip->desc);
+
 	/* failures here can mean systems won't boot... */
 	pr_err("gpiochip_add: gpios %d..%d (%s) failed to register\n",
 		chip->base, chip->base + chip->ngpio - 1,
@@ -1261,6 +1267,8 @@ int gpiochip_remove(struct gpio_chip *chip)
 	if (status == 0)
 		gpiochip_unexport(chip);
 
+	kfree(chip->desc);
+
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
-- 
1.8.1.1

Re: [PATCH 1/9] gpiolib: link all gpio_chips using a list

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Add a list member to gpio_chip that allows all chips to be parsed
> quickly. The current method requires parsing the entire GPIO integer
> space, which is painfully slow. Using a list makes many chip operations
> that involve lookup or parsing faster, and also simplifies the code. It
> is also necessary to eventually get rid of the global gpio_desc[] array.
>
> The list of gpio_chips is always ordered by base GPIO number to ensure
> chips traversal is done in the right order.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

OK!

> + * Return -EBUSY if the new chip overlaps with some other chip's integer
> + * space.
(...)
> +       /* are we stepping on the chip right before? */
> +       if (pos != &gpio_chips && pos->prev != &gpio_chips) {
> +               _chip = list_entry(pos->prev, struct gpio_chip, list);
> +               if (_chip->base + _chip->ngpio > chip->base) {
> +                       dev_err(chip->dev,
> +                              "GPIO integer space overlap, cannot add chip\n");
> +                       err = -EBUSY;
> +               }
> +       }

This robustness change on its own is a merit for merging the patch.
And keeping a list of gpiochips available is great for stuff like
debugfs.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/9] gpiolib: use gpio_chips list in gpiolib_sysfs_init

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Use the small list of GPIO chips instead of parsing the whole GPIO
> number space.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/9] gpiolib: use gpio_chips list in gpiochip_find

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Using the GPIO chips list is much faster than parsing the entire GPIO
> number space.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

You don't say :-)
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 4/9] gpiolib: use gpio_chips list in sysfs ops

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> This makes the code both simpler and faster compared to parsing the GPIO
> number space.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 5/9] gpiolib: use gpio_chips list in gpiochip_find_base

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Re-implement gpiochip_find_base using the list of chips instead of the
> global gpio_desc[] array. This makes it both simpler and more efficient,
> and is needed to remove the global descriptors array.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

So Haojian just submitted a different patch to the same piece of
code staring to search for GPIO number in ascending order instead,
but I NACKed it.

This looks like it is preserving this userspace-sensitive semantic
so that dynamically added chips will still get the same assigned
numbers.

But I want some guarantees, so state clearly in the commit
that any dynamically assigned chip will get the same base
address after this change as it got before it, and please take
this opportunity to add a comment stating that this search
method is vital for userspace ABIs, and must be preserved.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Make sure gpiolib works internally with descriptors and (chip, offset)
> pairs instead of using the global integer namespace. This prepares the

Its a numberspace not a namespace right?

> ground for the removal of the global gpio_desc[] array and the
> introduction of the descriptor-based GPIO API.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

> +/*
> + * Internal gpiod_* API using descriptors instead of the integer namespace.
> + * Most of this should eventually go public.
> + */
> +static int gpiod_request(struct gpio_desc *desc, const char *label);
> +static void gpiod_free(struct gpio_desc *desc);
> +static int gpiod_direction_input(struct gpio_desc *desc);
> +static int gpiod_direction_output(struct gpio_desc *desc, int value);
> +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
> +static int gpiod_get_value_cansleep(struct gpio_desc *desc);
> +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
> +static int gpiod_get_value(struct gpio_desc *desc);
> +static void gpiod_set_value(struct gpio_desc *desc, int value);
> +static int gpiod_cansleep(struct gpio_desc *desc);
> +static int gpiod_to_irq(struct gpio_desc *desc);
> +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
> +static int gpiod_export_link(struct device *dev, const char *name,
> +                            struct gpio_desc *desc);
> +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
> +static void gpiod_unexport(struct gpio_desc *desc);

Usually I don't like these upfrons forward-declarations, but in this
case I *do* because they are here for a reason, to later become
extern. So I like it!

> +/*
> + * Return the GPIO number of the passed descriptor relative to its chip
> + */
> +static int gpio_chip_hwgpio(const struct gpio_desc *desc)
> +{
> +       return (desc - &gpio_desc[0]) - desc->chip->base;
> +}

That was a scary method. But it works as long as the
descriptors are in a static array I guess...

> +/**
> + * Convert a GPIO number to its descriptor
> + */
> +static struct gpio_desc *gpio_to_desc(unsigned gpio)
> +{
> +       if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
> +               return NULL;

Don't we want to return ERR_PTR(-EINVAL); here?

Then you can use IS_ERR() on the pointers later.

This is the approach taken by the external API for clk
and pins.

> +/**
> + * Convert a GPIO descriptor to the integer namespace.
> + * This should disappear in the future but is needed since we still
> + * use GPIO numbers for error messages and sysfs nodes
> + */
> +static int desc_to_gpio(const struct gpio_desc *desc)
> +{
> +       return desc - &gpio_desc[0];
> +}

Aha OK the scary stuff goes away. Good...

> +
> +

You can never get enough whitespace ;-)

>  /* caller holds gpio_lock *OR* gpio is marked as requested */

That comment should be above the *next* function right?

Strictly speaking it does not apply to gpiod_to_chip() if
I read it right.

> +static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc)
> +{
> +       return desc->chip;
> +}
> +
>  struct gpio_chip *gpio_to_chip(unsigned gpio)
>  {
> -       return gpio_desc[gpio].chip;
> +       return gpiod_to_chip(gpio_to_desc(gpio));
>  }

...Then follows lots of nice stuff...

(...)
>  static ssize_t gpio_direction_show(struct device *dev,
>                 struct device_attribute *attr, char *buf)
>  {
> -       const struct gpio_desc  *desc = dev_get_drvdata(dev);
> -       unsigned                gpio = desc - gpio_desc;
> +       struct gpio_desc        *desc = dev_get_drvdata(dev);

Why not const anymore?

(Applies to all these similar cases in the patch.)

consting is nice. Especially in subsystem code.
I know it is hard to get compilation right without warnings
at times. But it pays off.

In the pinctrl subsystem I spend endless hours reading this
wiki page:

http://en.wikipedia.org/wiki/Const-correctness

I still don't quite get it. And it also wears off. But I like to use it.

> @@ -594,29 +647,32 @@ static ssize_t export_store(struct class *class,
> +       desc = gpio_to_desc(gpio);

I hope you have tested this hunk of the patch from userspace?

> @@ -637,17 +694,18 @@ static ssize_t unexport_store(struct class *class,

This too?

(etc for the userspace interfaces)

> @@ -1538,48 +1622,54 @@ int gpio_direction_input(unsigned gpio)
>                 }
>         }
>
> -       status = chip->direction_input(chip, gpio);
> +       status = chip->direction_input(chip, offset);
>         if (status == 0)
>                 clear_bit(FLAG_IS_OUT, &desc->flags);
>
> -       trace_gpio_direction(chip->base + gpio, 1, status);
> +       trace_gpio_direction(desc_to_gpio(desc), 1, status);
>  lose:
>         return status;
>  fail:
>         spin_unlock_irqrestore(&gpio_lock, flags);
> -       if (status)
> +       if (status) {
> +               int gpio = -1;
> +               if (desc)
> +                       gpio = desc_to_gpio(desc);
>                 pr_debug("%s: gpio-%d status %d\n",
>                         __func__, gpio, status);
> +       }
>         return status;
>  }

So using ERR_PTR/PTR_ERR helps you propagate
errors in situations like these.

Just:

if (IS_ERR(desc))
    status = PTR_ERR(desc);

> -int gpio_direction_output(unsigned gpio, int value)
> +static int gpiod_direction_output(struct gpio_desc *desc, int value)
>  {
>         unsigned long           flags;
>         struct gpio_chip        *chip;
> -       struct gpio_desc        *desc = &gpio_desc[gpio];
>         int                     status = -EINVAL;
> +       int offset;

Use hwgpio?


>         /* Open drain pin should not be driven to 1 */
>         if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
> -               return gpio_direction_input(gpio);
> +               return gpiod_direction_input(desc);
>
>         /* Open source pin should not be driven to 0 */
>         if (!value && test_bit(FLAG_OPEN_SOURCE,  &desc->flags))
> -               return gpio_direction_input(gpio);
> +               return gpiod_direction_input(desc);
>
>         spin_lock_irqsave(&gpio_lock, flags);
>
> -       if (!gpio_is_valid(gpio))
> +       if (!desc)

if (IS_ERR(desc)) ?

> @@ -1589,11 +1679,12 @@ int gpio_direction_output(unsigned gpio, int value)
>
>         might_sleep_if(chip->can_sleep);
>
> +       offset = gpio_chip_hwgpio(desc);

Maybe rename to hwgpio? Or is that done later?

We should stick with either hwgpio or offset everywhere or
it will be a mess.

(...)
>  fail:
>         spin_unlock_irqrestore(&gpio_lock, flags);
> -       if (status)
> +       if (status) {
> +               int gpio = -1;
> +               if (desc)
> +                       gpio = desc_to_gpio(desc);
>                 pr_debug("%s: gpio-%d status %d\n",
>                         __func__, gpio, status);
> +       }
>         return status;

Again IS_ERR()/ERR_PTR(). -1 is not nice.

>  /**
> @@ -1622,24 +1722,22 @@ EXPORT_SYMBOL_GPL(gpio_direction_output);
>   * @gpio: the gpio to set debounce time
>   * @debounce: debounce time is microseconds
>   */
> -int gpio_set_debounce(unsigned gpio, unsigned debounce)
> +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
>  {
>         unsigned long           flags;
>         struct gpio_chip        *chip;
> -       struct gpio_desc        *desc = &gpio_desc[gpio];
>         int                     status = -EINVAL;
> +       int                     offset;

hwgpio?

(then follows some repating cases)

(then some nice code)

> -int gpio_get_value_cansleep(unsigned gpio)
> +static int gpiod_get_value_cansleep(struct gpio_desc *desc)
>  {
>         struct gpio_chip        *chip;
>         int value;
> +       int offset;

hwgpio?

Basically the patch is very nice but I'd like you to iron out and proofread
as per above so we have strong consistency, strong const:ing,
and stringent naming (offset vs hwgpio come to mind).

Yours,
Linus Walleij

Re: [PATCH 7/9] gpiolib: let gpio_chip reference its descriptors

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Add a pointer to the gpio_chip structure that references the array of
> GPIO descriptors belonging to the chip, and update gpiolib code to use
> this pointer instead of the global gpio_desc[] array. This is another
> step towards the removal of the gpio_desc[] global array.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.orh>

Yours,
Linus Walleij

Re: [PATCH 8/9] gpiolib: use gpio_chips list in gpio_to_desc

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Parse the list of chips to find the descriptor corresponding to a GPIO
> number instead of directly picking the entry of the global gpio_desc[]
> array, which is due to be removed.
>
> This turns the complexity of converting a GPIO number into a descriptor
> from O(1) to O(n) where n is the number of GPIO chips in the system.
> Since n is ought to be small anyway, there should be no noticeable
> performance impact. Moreover, GPIO users who care for speed already have
> implemented their own gpio_get_value() and gpio_set_value() with a
> fast path for the GPIO numbers that matter and this change does not
> affect such use cases.
>
> The descriptor-based GPIO API, due to be introduced soon, will make this
> lookup unnecessary.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

OK it's a nice stepping stone.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 9/9] gpiolib: dynamically allocate descriptors array

[Linus Walleij]

On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Allocate the GPIO descriptor's memory dynamically when GPIO chips are
> added instead of using the static gpio_desc[] array. gpio_desc[] is now
> totally unused and therefore removed.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 5/9] gpiolib: use gpio_chips list in gpiochip_find_base

[Alex Courbot]

On 02/06/2013 02:21 AM, Linus Walleij wrote:
> This looks like it is preserving this userspace-sensitive semantic
> so that dynamically added chips will still get the same assigned
> numbers.

It does (it should, at least), the assigned ranges should be strictly 
identical to the previous version. While testing I also made sure all 
chips had the same GPIO range.

> But I want some guarantees, so state clearly in the commit
> that any dynamically assigned chip will get the same base
> address after this change as it got before it, and please take
> this opportunity to add a comment stating that this search
> method is vital for userspace ABIs, and must be preserved.

Done. I will take the blame if something goes wrong. :)

Thanks,
Alex.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Alexandre Courbot]

On Wed, Feb 6, 2013 at 2:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
>> Make sure gpiolib works internally with descriptors and (chip, offset)
>> pairs instead of using the global integer namespace. This prepares the
>
> Its a numberspace not a namespace right?

Yes, absolutely.

>> +/*
>> + * Internal gpiod_* API using descriptors instead of the integer namespace.
>> + * Most of this should eventually go public.
>> + */
>> +static int gpiod_request(struct gpio_desc *desc, const char *label);
>> +static void gpiod_free(struct gpio_desc *desc);
>> +static int gpiod_direction_input(struct gpio_desc *desc);
>> +static int gpiod_direction_output(struct gpio_desc *desc, int value);
>> +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
>> +static int gpiod_get_value_cansleep(struct gpio_desc *desc);
>> +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
>> +static int gpiod_get_value(struct gpio_desc *desc);
>> +static void gpiod_set_value(struct gpio_desc *desc, int value);
>> +static int gpiod_cansleep(struct gpio_desc *desc);
>> +static int gpiod_to_irq(struct gpio_desc *desc);
>> +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>> +static int gpiod_export_link(struct device *dev, const char *name,
>> +                            struct gpio_desc *desc);
>> +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>> +static void gpiod_unexport(struct gpio_desc *desc);
>
> Usually I don't like these upfrons forward-declarations, but in this
> case I *do* because they are here for a reason, to later become
> extern. So I like it!

Yeah, you know how the movie is going to end already. ;) Maybe it
would make sense to append the gpiod patches to this series and not
end up with these declarations?

>> +/*
>> + * Return the GPIO number of the passed descriptor relative to its chip
>> + */
>> +static int gpio_chip_hwgpio(const struct gpio_desc *desc)
>> +{
>> +       return (desc - &gpio_desc[0]) - desc->chip->base;
>> +}
>
> That was a scary method. But it works as long as the
> descriptors are in a static array I guess...

Yes, and it's becomes less scary when the static array goes away.

>> +/**
>> + * Convert a GPIO number to its descriptor
>> + */
>> +static struct gpio_desc *gpio_to_desc(unsigned gpio)
>> +{
>> +       if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
>> +               return NULL;
>
> Don't we want to return ERR_PTR(-EINVAL); here?
>
> Then you can use IS_ERR() on the pointers later.
>
> This is the approach taken by the external API for clk
> and pins.

Yes, that completely makes sense.

>>  /* caller holds gpio_lock *OR* gpio is marked as requested */
>
> That comment should be above the *next* function right?
>
> Strictly speaking it does not apply to gpiod_to_chip() if
> I read it right.

That's right. On a related topic there are actually a whole set of
comments that are not in the right place (because the function that
follows them has been switched to the gpiod prefix). I left them here
because once the gpiod interface becomes public they will be updated
to apply to them, and moving comments back and forth in such a short
time would only make the patches confusing.

>>  static ssize_t gpio_direction_show(struct device *dev,
>>                 struct device_attribute *attr, char *buf)
>>  {
>> -       const struct gpio_desc  *desc = dev_get_drvdata(dev);
>> -       unsigned                gpio = desc - gpio_desc;
>> +       struct gpio_desc        *desc = dev_get_drvdata(dev);
>
> Why not const anymore?
>
> (Applies to all these similar cases in the patch.)
>
> consting is nice. Especially in subsystem code.
> I know it is hard to get compilation right without warnings
> at times. But it pays off.
>
> In the pinctrl subsystem I spend endless hours reading this
> wiki page:
>
> http://en.wikipedia.org/wiki/Const-correctness
>
> I still don't quite get it. And it also wears off. But I like to use it.

Oh I do share your love for const-correctness (look at the parameters
for gpio_chip_hwgpio() and desc_to_gpio()). Only here I could not
preserve it AFAICT.

The previous version of the function was only using desc locally and
relied on GPIO numbers to do the dirty job. Here however, we pass desc
to gpiod_get_direction(). So you will tell me, that as it only returns
the direction its parameter could also be const and that's true -
excepted when it tries to clear some bits in desc->flags, there we
definitely cannot claim it is const anymore. Maybe we could cast the
pointer given to clear_bit/set_bit, but I'm not sure that's more
elegant. Ideally gpiod_get_direction() should not modify its parameter
at all.

>> @@ -594,29 +647,32 @@ static ssize_t export_store(struct class *class,
>> +       desc = gpio_to_desc(gpio);
>
> I hope you have tested this hunk of the patch from userspace?
>
>> @@ -637,17 +694,18 @@ static ssize_t unexport_store(struct class *class,
>
> This too?
>
> (etc for the userspace interfaces)

Yes. Side-question: should I explicitly add my "Tested-by" in this
cases? I thought that the author's Signed-off implied that the patch
has been properly tested, but your question tend to suggest it's not
*that* obvious... ;)

>> +       if (status) {
>> +               int gpio = -1;
>> +               if (desc)
>> +                       gpio = desc_to_gpio(desc);
>>                 pr_debug("%s: gpio-%d status %d\n",
>>                         __func__, gpio, status);
>> +       }
>>         return status;
>>  }
>
> So using ERR_PTR/PTR_ERR helps you propagate
> errors in situations like these.
>
> Just:
>
> if (IS_ERR(desc))
>     status = PTR_ERR(desc);

Yes, that would make these blocks look much better.

>>         /* Open drain pin should not be driven to 1 */
>>         if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
>> -               return gpio_direction_input(gpio);
>> +               return gpiod_direction_input(desc);
>>
>>         /* Open source pin should not be driven to 0 */
>>         if (!value && test_bit(FLAG_OPEN_SOURCE,  &desc->flags))
>> -               return gpio_direction_input(gpio);
>> +               return gpiod_direction_input(desc);
>>
>>         spin_lock_irqsave(&gpio_lock, flags);
>>
>> -       if (!gpio_is_valid(gpio))
>> +       if (!desc)
>
> if (IS_ERR(desc)) ?

It's worse actually, we potentially access desc right before checking
it it's not NULL... Makes no sense at all. Thanks for pointing this.

>> +       offset = gpio_chip_hwgpio(desc);
>
> Maybe rename to hwgpio? Or is that done later?
>
> We should stick with either hwgpio or offset everywhere or
> it will be a mess.

Will replace all these "offset" by "hwgpio" for consistency.

> Basically the patch is very nice but I'd like you to iron out and proofread
> as per above so we have strong consistency, strong const:ing,
> and stringent naming (offset vs hwgpio come to mind).

Yes, there are some inconsistencies (and "inconstistencies") that
remains, e.g. automatically converting calls to gpio_is_valid() into a
check that the descriptor is not NULL was not the best idea. I will
fix the patch according to your comments and then check the whole file
to make sure I have not missed anything. Might do some random
unrelated catches in the process.

Thanks for the review!
Alex.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Grant Likely]

On Thu, 7 Feb 2013 15:57:32 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> On Wed, Feb 6, 2013 at 2:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> >> +/**
> >> + * Convert a GPIO number to its descriptor
> >> + */
> >> +static struct gpio_desc *gpio_to_desc(unsigned gpio)
> >> +{
> >> +       if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
> >> +               return NULL;
> >
> > Don't we want to return ERR_PTR(-EINVAL); here?
> >
> > Then you can use IS_ERR() on the pointers later.
> >
> > This is the approach taken by the external API for clk
> > and pins.
> 
> Yes, that completely makes sense.
> 

No, it does not. The ERR_PTR()/IS_ERR() is a horrible pattern for code
readability because it breaks the expectations that programmers have for
what is and is not a bad pointer. There are decades of history where the
test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make
that test not work, but the compiler won't tell you when you get it
wrong.

There are places where ERR_PTR makes sense. Particularly when
communicating with userspace where error codes have very specific
meanings, but I don't want it in the GPIO subsystem.

g.

Re: [PATCH 1/9] gpiolib: link all gpio_chips using a list

[Grant Likely]

On Tue, 5 Feb 2013 18:00:56 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > Add a list member to gpio_chip that allows all chips to be parsed
> > quickly. The current method requires parsing the entire GPIO integer
> > space, which is painfully slow. Using a list makes many chip operations
> > that involve lookup or parsing faster, and also simplifies the code. It
> > is also necessary to eventually get rid of the global gpio_desc[] array.
> >
> > The list of gpio_chips is always ordered by base GPIO number to ensure
> > chips traversal is done in the right order.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> OK!
> 
> > + * Return -EBUSY if the new chip overlaps with some other chip's integer
> > + * space.
> (...)
> > +       /* are we stepping on the chip right before? */
> > +       if (pos != &gpio_chips && pos->prev != &gpio_chips) {
> > +               _chip = list_entry(pos->prev, struct gpio_chip, list);
> > +               if (_chip->base + _chip->ngpio > chip->base) {
> > +                       dev_err(chip->dev,
> > +                              "GPIO integer space overlap, cannot add chip\n");
> > +                       err = -EBUSY;
> > +               }
> > +       }
> 
> This robustness change on its own is a merit for merging the patch.
> And keeping a list of gpiochips available is great for stuff like
> debugfs.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied, thanks.

g.

Re: [PATCH 2/9] gpiolib: use gpio_chips list in gpiolib_sysfs_init

[Grant Likely]

On Tue, 5 Feb 2013 18:04:43 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > Use the small list of GPIO chips instead of parsing the whole GPIO
> > number space.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 

Applied, thanks.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH 3/9] gpiolib: use gpio_chips list in gpiochip_find

[Grant Likely]

On Tue, 5 Feb 2013 18:05:32 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > Using the GPIO chips list is much faster than parsing the entire GPIO
> > number space.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> You don't say :-)
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied, thanks.

g.

Re: [PATCH 4/9] gpiolib: use gpio_chips list in sysfs ops

[Grant Likely]

On Tue, 5 Feb 2013 18:15:54 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > This makes the code both simpler and faster compared to parsing the GPIO
> > number space.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied, thanks.

However, this code is incorrect (it was incorrect before you touched it,
so not your fault). Moving it to a list makes it a lot worse though
because it introduces the possibility of dereferencing an invalid
pointer. The hooks need to grab the spinlock before parsing the list.

I've got a patch which I'll post. I'd appreciate it if you can test
also.

In actual fact, I'd like to replace the spinlock with a mutex, but that
requires some auditing to make sure there aren't any users from an
atomic context.

g.

Re: [PATCH 5/9] gpiolib: use gpio_chips list in gpiochip_find_base

[Grant Likely]

On Wed, 6 Feb 2013 13:48:19 +0900, Alex Courbot <acourbot@nvidia.com> wrote:
> On 02/06/2013 02:21 AM, Linus Walleij wrote:
> > This looks like it is preserving this userspace-sensitive semantic
> > so that dynamically added chips will still get the same assigned
> > numbers.
> 
> It does (it should, at least), the assigned ranges should be strictly 
> identical to the previous version. While testing I also made sure all 
> chips had the same GPIO range.
> 
> > But I want some guarantees, so state clearly in the commit
> > that any dynamically assigned chip will get the same base
> > address after this change as it got before it, and please take
> > this opportunity to add a comment stating that this search
> > method is vital for userspace ABIs, and must be preserved.
> 
> Done. I will take the blame if something goes wrong. :)

Applied, thanks

g.

Re: [PATCH 8/9] gpiolib: use gpio_chips list in gpio_to_desc

[Grant Likely]

On Sun,  3 Feb 2013 01:29:31 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Parse the list of chips to find the descriptor corresponding to a GPIO
> number instead of directly picking the entry of the global gpio_desc[]
> array, which is due to be removed.
> 
> This turns the complexity of converting a GPIO number into a descriptor
> from O(1) to O(n) where n is the number of GPIO chips in the system.
> Since n is ought to be small anyway, there should be no noticeable
> performance impact. Moreover, GPIO users who care for speed already have
> implemented their own gpio_get_value() and gpio_set_value() with a
> fast path for the GPIO numbers that matter and this change does not
> affect such use cases.
> 
> The descriptor-based GPIO API, due to be introduced soon, will make this
> lookup unnecessary.

I'm actually going to NAK this one. This is a hot path. Having a O(1)
lookup from gpio number to gpio desc is important. I know you want to be
rid of the gpio_desc table entirely, but in this case I think it is
warranted. However, you can change the gpio_desc table to be a table of
pointers to gpio_descs instead of a table of gpio_descs. That would save
a lot of memory since unused GPIOs wouldn't have gpio_descs associated
with them. It is also the mechanism used by the IRQ subsystem.

So, it makes sense for the primary gpio_chip lookup to be the list, but
the table needs to stick around as a secondary lookup.

g.

> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9599b9a..0247c48 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -122,10 +122,21 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
>   */
>  static struct gpio_desc *gpio_to_desc(unsigned gpio)
>  {
> -	if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
> -		return NULL;
> -	else
> -		return &gpio_desc[gpio];
> +	struct gpio_chip *chip;
> +
> +	list_for_each_entry(chip, &gpio_chips, list) {
> +		int gpio_min = chip->base;
> +		int gpio_max = gpio_min + chip->ngpio;
> +		if (gpio >= gpio_min && gpio < gpio_max)
> +			return &chip->desc[gpio - gpio_min];
> +		else if (gpio < gpio_min)
> +			/* gpio_chips are ordered by base, so we won't get any
> +			 * hit if we arrive here... */
> +			break;
> +	}
> +
> +	pr_warn("%s: no registered chip to handle GPIO %d\n", __func__, gpio);
> +	return NULL;
>  }
>  
>  /**
> -- 
> 1.8.1.1
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Grant Likely]

On Sun,  3 Feb 2013 01:29:29 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Make sure gpiolib works internally with descriptors and (chip, offset)
> pairs instead of using the global integer namespace. This prepares the
> ground for the removal of the global gpio_desc[] array and the
> introduction of the descriptor-based GPIO API.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 493 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 317 insertions(+), 176 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index af4c350..82c40bd 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -78,6 +78,28 @@ static LIST_HEAD(gpio_chips);
>  static DEFINE_IDR(dirent_idr);
>  #endif
>  
> +/*
> + * Internal gpiod_* API using descriptors instead of the integer namespace.
> + * Most of this should eventually go public.
> + */
> +static int gpiod_request(struct gpio_desc *desc, const char *label);
> +static void gpiod_free(struct gpio_desc *desc);
> +static int gpiod_direction_input(struct gpio_desc *desc);
> +static int gpiod_direction_output(struct gpio_desc *desc, int value);
> +static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
> +static int gpiod_get_value_cansleep(struct gpio_desc *desc);
> +static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
> +static int gpiod_get_value(struct gpio_desc *desc);
> +static void gpiod_set_value(struct gpio_desc *desc, int value);
> +static int gpiod_cansleep(struct gpio_desc *desc);
> +static int gpiod_to_irq(struct gpio_desc *desc);
> +static int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
> +static int gpiod_export_link(struct device *dev, const char *name,
> +			     struct gpio_desc *desc);
> +static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
> +static void gpiod_unexport(struct gpio_desc *desc);

I've been thinking about the adding of a new API to supplant the old,
and I'm wondering if we're going about this the wrong way around; at
least for the public api. We've moved to a model where device drivers
are really supposed to tread GPIO numbers as opaque handles. Most device
drivers are well behaved on this point. (Others of course are not so
well behaved and either hard code or interpret what a number means, but
those users already need to be fixed before they will work with the
device tree)

Rather than creating a new API with a long-term-plan to move all old
users to the new API - touching a lot of code all over the kernel in the
process - what if we repurposed the existing API in a backwards
compatible way.

I think the maximum GPIO number I've seen in use is 65535, and that was
based on dynamic allocation. What if we did the following:

typedef unsigned long gpio_handle_t;

struct gpio_desc *gpio_handle_to_desc(gpio_handle_t gpio)
{
	if (gpio <= NR_GPIOS)
		return &gpio_desc[gpio];
	return (struct gpio_desc *)gpio;
}

int gpio_get_direction(gpio_handle_t gpio)
{
	struct gpio_desc *desc = gpio_handle_to_desc(gpio);
	if (!desc)
		return -EEXIST;
	return gpiod_get_direction(desc);
}

And do the same for all the other hooks. That will make the gpio changes
much lower impact across the kernel, allow legacy gpio users to keep
doing what they are doing, and encourage driver authors to keep out of
the gpio_desc internals.

Thoughts?

I'm probably going to apply this patch anyway because it only affects
the gpiolib internals, I need to take one more look over it before I
make a decision. I want to get as much of this as possible queued up for
v3.9 to make future work easier.

g.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Grant Likely]

On Sun,  3 Feb 2013 01:29:29 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Make sure gpiolib works internally with descriptors and (chip, offset)
> pairs instead of using the global integer namespace. This prepares the
> ground for the removal of the global gpio_desc[] array and the
> introduction of the descriptor-based GPIO API.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Despite Linus' comments, I've gone ahead and applied this. I think it is
a worthwhile change on its own and it feeds into the future work than
needs to be done. Any of the stuff he brought up can be addressed in
follow-on patches.

g.

Re: [PATCH 7/9] gpiolib: let gpio_chip reference its descriptors

[Grant Likely]

On Tue, 5 Feb 2013 19:00:09 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > Add a pointer to the gpio_chip structure that references the array of
> > GPIO descriptors belonging to the chip, and update gpiolib code to use
> > this pointer instead of the global gpio_desc[] array. This is another
> > step towards the removal of the gpio_desc[] global array.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.orh>

Applied, thanks.

g.

Re: [PATCH 4/9] gpiolib: use gpio_chips list in sysfs ops

[Alexandre Courbot]

On Sat, Feb 9, 2013 at 6:37 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> However, this code is incorrect (it was incorrect before you touched it,
> so not your fault). Moving it to a list makes it a lot worse though
> because it introduces the possibility of dereferencing an invalid
> pointer. The hooks need to grab the spinlock before parsing the list.
>
> I've got a patch which I'll post. I'd appreciate it if you can test
> also.
>
> In actual fact, I'd like to replace the spinlock with a mutex, but that
> requires some auditing to make sure there aren't any users from an
> atomic context.

I thought about doing that as well actually, but left it for later. It
looks safe at first sight, but as you mention we cannot exclude a few
users will break as a consequence of such a switch.

In any case, I plan to give it a try once that big chunk is merged.

Alex.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Alexandre Courbot]

On Sat, Feb 9, 2013 at 10:11 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> I've been thinking about the adding of a new API to supplant the old,
> and I'm wondering if we're going about this the wrong way around; at
> least for the public api. We've moved to a model where device drivers
> are really supposed to tread GPIO numbers as opaque handles. Most device
> drivers are well behaved on this point. (Others of course are not so
> well behaved and either hard code or interpret what a number means, but
> those users already need to be fixed before they will work with the
> device tree)
>
> Rather than creating a new API with a long-term-plan to move all old
> users to the new API - touching a lot of code all over the kernel in the
> process - what if we repurposed the existing API in a backwards
> compatible way.
>
> I think the maximum GPIO number I've seen in use is 65535, and that was
> based on dynamic allocation. What if we did the following:
>
> typedef unsigned long gpio_handle_t;
>
> struct gpio_desc *gpio_handle_to_desc(gpio_handle_t gpio)
> {
>         if (gpio <= NR_GPIOS)
>                 return &gpio_desc[gpio];
>         return (struct gpio_desc *)gpio;
> }
>
> int gpio_get_direction(gpio_handle_t gpio)
> {
>         struct gpio_desc *desc = gpio_handle_to_desc(gpio);
>         if (!desc)
>                 return -EEXIST;
>         return gpiod_get_direction(desc);
> }
>
> And do the same for all the other hooks. That will make the gpio changes
> much lower impact across the kernel, allow legacy gpio users to keep
> doing what they are doing, and encourage driver authors to keep out of
> the gpio_desc internals.

So if I understand correctly, this means reserving the range
[0..65535] for integer GPIOs, assuming that anything bigger is a
descriptor (that have been acquired properly using some gpio_get()
function), and keep using the same interface for both cases? I'm not
sure what to think yet, in a way I find the idea elegant and less
traumatic to current GPIO users. On the other hand, I wonder if we
will not run into some walls.

Here is one I can think of right now: what should gpio_is_valid()
return? If the parameter is in the integer range, no problem, it's
valid. If it's bigger, we could check the validity of the pointer if
we use the gpio_desc[] static array that currently exist, but if we
eventually manage to get rid of it things would become harder. Also,
let's say we do gpio_is_valid(0). Is it the GPIO number 0, which is
valid, or a NULL pointer which is invalid? It might also bring some
confusion that the nature of GPIOs depends on the way they are
acquired (integers for gpio_request(), pointers for gpio_get()). Some
users might use gpio_get(), expect to get a number, and be puzzled.

I am not particularly fond of the idea of introducing a new API that
is nearly identical to one that already exists, but it has the merit
of keeping things clear. I also think we should consider this state as
a transition towards the elimination of the GPIO integer API. I might
be misguided to wish for that, but right now I can only see two things
that would prevent us from achieving this:

1) The fast-path that some platforms implemented in their definition
of gpio_get_value() and gpio_set_value() where they directly invoke
their controller's function inline for GPIOs which values are known at
compile time (can't do that with pointers AFAICT)
2) The sysfs interface. But we could still keep the integer space for
that sole purpose and not use it for our in-kernel APIs.

Actually only 1) is a real blocker. Also in the case we cannot/do not
want to get rid of it, keeping the legacy gpio API as a wrapper around
gpiod as I proposed in an earlier RFC should probably not be too
painful (only a single .h file to maintain).

Alex.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Alexandre Courbot]

On Sat, Feb 9, 2013 at 10:24 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sun,  3 Feb 2013 01:29:29 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> Make sure gpiolib works internally with descriptors and (chip, offset)
>> pairs instead of using the global integer namespace. This prepares the
>> ground for the removal of the global gpio_desc[] array and the
>> introduction of the descriptor-based GPIO API.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Despite Linus' comments, I've gone ahead and applied this. I think it is
> a worthwhile change on its own and it feeds into the future work than
> needs to be done. Any of the stuff he brought up can be addressed in
> follow-on patches.

Ah, I actually wanted to ask you for a couple of days to submit a new
version of this. On top of Linus' comments, there are a few more
sanity checks I'd like to add. If you don't want to rebase on the new
version, I can also send a new patch on top of this one, whichever way
you prefer - but I think rebasing would be cleaner, if you don't mind.

Alex.

Re: [PATCH 8/9] gpiolib: use gpio_chips list in gpio_to_desc

[Alexandre Courbot]

On Sat, Feb 9, 2013 at 6:58 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> I'm actually going to NAK this one. This is a hot path. Having a O(1)
> lookup from gpio number to gpio desc is important. I know you want to be
> rid of the gpio_desc table entirely, but in this case I think it is
> warranted. However, you can change the gpio_desc table to be a table of
> pointers to gpio_descs instead of a table of gpio_descs. That would save
> a lot of memory since unused GPIOs wouldn't have gpio_descs associated
> with them. It is also the mechanism used by the IRQ subsystem.

That would work - what I don't like is that it still ends being a
fixed-size static array that is not necessarily tailored to the
platform's needs. But I understand you don't want to punish the users
of the integer-based API, even though the penalty should really be
neglectable here.

Well, maybe I can try to come again with this once everybody uses GPIO
descriptors instead of integers. ;)

Alex.

Re: [PATCH 8/9] gpiolib: use gpio_chips list in gpio_to_desc

[Grant Likely]

On Sat, Feb 9, 2013 at 2:21 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> On Sat, Feb 9, 2013 at 6:58 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> I'm actually going to NAK this one. This is a hot path. Having a O(1)
>> lookup from gpio number to gpio desc is important. I know you want to be
>> rid of the gpio_desc table entirely, but in this case I think it is
>> warranted. However, you can change the gpio_desc table to be a table of
>> pointers to gpio_descs instead of a table of gpio_descs. That would save
>> a lot of memory since unused GPIOs wouldn't have gpio_descs associated
>> with them. It is also the mechanism used by the IRQ subsystem.
>
> That would work - what I don't like is that it still ends being a
> fixed-size static array that is not necessarily tailored to the
> platform's needs. But I understand you don't want to punish the users
> of the integer-based API, even though the penalty should really be
> neglectable here.
>
> Well, maybe I can try to come again with this once everybody uses GPIO
> descriptors instead of integers. ;)

Yes, when everyone uses descriptor handles the problem goes away.
Until then we can just make the table fairly large. If it is pointers
to descs instead of the descs themselves then the memory cost really
isn't that significant.

We could also make the table itself grow dynamically, but I don't
think there is evidence suggesting that is needed. Thomas tried to do
the same thing with the irq desc table and found the resulting code
was far more convoluted than he wanted to support.

g.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Linus Walleij]

On Sat, Feb 9, 2013 at 10:17 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

> The ERR_PTR()/IS_ERR() is a horrible pattern for code
> readability because it breaks the expectations that programmers have for
> what is and is not a bad pointer. There are decades of history where the
> test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make
> that test not work, but the compiler won't tell you when you get it
> wrong.
>
> There are places where ERR_PTR makes sense. Particularly when
> communicating with userspace where error codes have very specific
> meanings, but I don't want it in the GPIO subsystem.

OK I disagree but you get to decide.

However if you take this all the way to the descriptor API
it will make the consumer (driver) API for GPIO descriptors deviate
from what is today used for clocks, regulators and pins.

With all the resulting confusion for users.
I've seen worse subsystem deviations though.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Paul Mundt]

On Mon, Feb 11, 2013 at 03:09:21PM +0100, Linus Walleij wrote:
> On Sat, Feb 9, 2013 at 10:17 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> > The ERR_PTR()/IS_ERR() is a horrible pattern for code
> > readability because it breaks the expectations that programmers have for
> > what is and is not a bad pointer. There are decades of history where the
> > test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make
> > that test not work, but the compiler won't tell you when you get it
> > wrong.
> >
> > There are places where ERR_PTR makes sense. Particularly when
> > communicating with userspace where error codes have very specific
> > meanings, but I don't want it in the GPIO subsystem.
> 
> OK I disagree but you get to decide.
> 
> However if you take this all the way to the descriptor API
> it will make the consumer (driver) API for GPIO descriptors deviate
> from what is today used for clocks, regulators and pins.
> 
> With all the resulting confusion for users.
> I've seen worse subsystem deviations though.
> 
I've always considered it to be more of a complexity issue. If an
interface can fail for half a dozen different reasons, it's useful to be
able to encapsulate this and pass it down the line to the consumer
(particularly in cases where no useful debug output is provided, and the
first thing someone is going to do is go and instrument the registration
path with printks to figure out where things went wrong). In the case
where the work to do is relatively straightforward and there's not much
complexity, NULL is clearly the way to go.

There are already quite a few cases today converting NULLs over to
arbitrary ERR_PTR values or IS_ERR cases wrapping back to NULL, so it's
clear that both have to be supported interacting with one another
regardless. Then there's that whole IS_ERR_OR_NULL case which everyone
seems to get wrong, but that's another issue entirely.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Stephen Warren]

On 02/11/2013 07:09 AM, Linus Walleij wrote:
> On Sat, Feb 9, 2013 at 10:17 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> 
>> The ERR_PTR()/IS_ERR() is a horrible pattern for code
>> readability because it breaks the expectations that programmers have for
>> what is and is not a bad pointer. There are decades of history where the
>> test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make
>> that test not work, but the compiler won't tell you when you get it
>> wrong.
>>
>> There are places where ERR_PTR makes sense. Particularly when
>> communicating with userspace where error codes have very specific
>> meanings, but I don't want it in the GPIO subsystem.
> 
> OK I disagree but you get to decide.
> 
> However if you take this all the way to the descriptor API
> it will make the consumer (driver) API for GPIO descriptors deviate
> from what is today used for clocks, regulators and pins.
> 
> With all the resulting confusion for users.
> I've seen worse subsystem deviations though.

Sorry I haven't looked at the specific APIs this discussion refers to,
but clients of the GPIO descriptor API are going to need to distinguish
"fail" from "deferred probe", so at least some initial get-like API will
need to pass back some error detail...

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Linus Walleij]

On Mon, Feb 11, 2013 at 6:39 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/11/2013 07:09 AM, Linus Walleij wrote:

>> However if you take this all the way to the descriptor API
>> it will make the consumer (driver) API for GPIO descriptors deviate
>> from what is today used for clocks, regulators and pins.
>>
>> With all the resulting confusion for users.
>> I've seen worse subsystem deviations though.
>
> Sorry I haven't looked at the specific APIs this discussion refers to,
> but clients of the GPIO descriptor API are going to need to distinguish
> "fail" from "deferred probe", so at least some initial get-like API will
> need to pass back some error detail...

Right, so in some other patch I stated that this would lead
to a GPIO descriptor fetch interface such as this:

int gpiod_get(struct gpiod_desc **gpiod, struct device *dev, const char *name);

Rather than the more established:

struct gpio_desc *gpiod_get(struct device *dev, const char *name);

And I'm worried about the lack of consistency.

While I do get the point... I chatted with Grant about it and
I want to talk to some toolchain people about this to see if
pointers containing potential error codes can somehow be
"flagged" by the compiler so we can enforce error checking on
them. (It may sound a bit utopic...)

Yours,
Linus Walleij

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Paul Mundt]

On Tue, Feb 12, 2013 at 01:29:10PM +0100, Linus Walleij wrote:
> On Mon, Feb 11, 2013 at 6:39 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > On 02/11/2013 07:09 AM, Linus Walleij wrote:
> 
> >> However if you take this all the way to the descriptor API
> >> it will make the consumer (driver) API for GPIO descriptors deviate
> >> from what is today used for clocks, regulators and pins.
> >>
> >> With all the resulting confusion for users.
> >> I've seen worse subsystem deviations though.
> >
> > Sorry I haven't looked at the specific APIs this discussion refers to,
> > but clients of the GPIO descriptor API are going to need to distinguish
> > "fail" from "deferred probe", so at least some initial get-like API will
> > need to pass back some error detail...
> 
> Right, so in some other patch I stated that this would lead
> to a GPIO descriptor fetch interface such as this:
> 
> int gpiod_get(struct gpiod_desc **gpiod, struct device *dev, const char *name);
> 
> Rather than the more established:
> 
> struct gpio_desc *gpiod_get(struct device *dev, const char *name);
> 
> And I'm worried about the lack of consistency.
> 
> While I do get the point... I chatted with Grant about it and
> I want to talk to some toolchain people about this to see if
> pointers containing potential error codes can somehow be
> "flagged" by the compiler so we can enforce error checking on
> them. (It may sound a bit utopic...)
> 
At the very least you can __must_check annotate, although that's probably
still a bit coarser grained than what you're after.

Re: [PATCH 6/9] gpiolib: use descriptors internally

[Arnd Bergmann]

On Tuesday 12 February 2013, Paul Mundt wrote:
> > While I do get the point... I chatted with Grant about it and
> > I want to talk to some toolchain people about this to see if
> > pointers containing potential error codes can somehow be
> > "flagged" by the compiler so we can enforce error checking on
> > them. (It may sound a bit utopic...)
> > 
> At the very least you can __must_check annotate, although that's probably
> still a bit coarser grained than what you're after.

There is an idea: we could have variants of __must_check for different
classes of returns: NULL error, ERR_PTR, negative integer on error, ...

	Arnd