[PATCH 0/4] gpio: introduce descriptor-based interface

[PATCH 0/4] gpio: introduce descriptor-based interface

[Alexandre Courbot]

This series introduce a first take at implementing the RFC for the new GPIO API
that I submitted last month. It proposes a new, opaque descriptor-based GPIO API
that becomes available when GPIOlib is compiled, and provides a safer, more
abstract alternative to the current integer-based interface. GPIOlib internals
are also switched to use the descriptor logic, and the former integer API
becomes a lightweight wrapper around the new descriptor-based API.

Functionally speaking the new API is identical to the integer-based API, with
only the prefix changing from gpio_ to gpiod_. However, the second patch
introduces new functions for obtaining GPIOs from a device and a consumer name,
in a fashion similar to what is done with e.g. regulators and PWMs.

GPIOs can then be provided either by board-specific lookup tables, or through
the device tree. Device tree lookup might require some attention as it does not
handle properties with multiple descriptors yet. Also, there is currently no
equivalent of gpio_request_array() and GPIOs can only be allocated one-by-one.
Feedback about the relevancy of batch requesting GPIOs is welcome.

This patch series also prepares GPIOlib for the next step, which is getting rid
of ARCH_NR_GPIOS and of the static array in GPIOlib and replace the latter with
per-chip arrays that are allocated when the chip is added. Some challenge may
arise from the fact that gpiochip_add is potentially called before kmalloc is
available.

Anyway, I expect this patchset to go through several iterations in order to
address the points mentioned above (and of course the ones I missed). As usual,
your valuable feedback is most welcome.

Alexandre Courbot (4):
  gpiolib: introduce descriptor-based GPIO interface
  gpiolib: add gpiod_get and gpiod_put functions
  gpiolib: of: convert OF helpers to descriptor API
  gpiolib: add documentation for new gpiod_ API

 Documentation/gpio.txt        |  94 +++++-
 drivers/gpio/devres.c         |  59 +++-
 drivers/gpio/gpiolib-of.c     |  26 +-
 drivers/gpio/gpiolib.c        | 648 ++++++++++++++++++++++++++++--------------
 include/asm-generic/gpio.h    | 176 ++++--------
 include/linux/gpio/consumer.h | 128 +++++++++
 include/linux/gpio/driver.h   | 171 +++++++++++
 include/linux/of_gpio.h       |  73 +----
 8 files changed, 956 insertions(+), 419 deletions(-)
 create mode 100644 include/linux/gpio/consumer.h
 create mode 100644 include/linux/gpio/driver.h

-- 
1.8.1

[PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Alexandre Courbot]

With the current API, GPIOs are represented by a unique integer. This
causes problems in terms of security (GPIO numbers can be arbitrarily
forged and used without proper allocation) and flexibility (the maximum
number of GPIOs that the system can handle is fixed at compilation time
and a static array of that size is allocated to store the GPIO
descriptors).

This patch introduces an alternative GPIO API that uses opaque handlers
and refactors GPIOlib's internals to work with these handlers instead of
GPIO numbers. The former integer-based API is still available as a light
wrapper around this new API.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/devres.c         |  59 +++--
 drivers/gpio/gpiolib.c        | 486 +++++++++++++++++++++++-------------------
 include/asm-generic/gpio.h    | 176 ++++++---------
 include/linux/gpio/consumer.h |  64 ++++++
 include/linux/gpio/driver.h   | 112 ++++++++++
 5 files changed, 552 insertions(+), 345 deletions(-)
 create mode 100644 include/linux/gpio/consumer.h
 create mode 100644 include/linux/gpio/driver.h

diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
index 1077754..b7a9ad6 100644
--- a/drivers/gpio/devres.c
+++ b/drivers/gpio/devres.c
@@ -15,22 +15,24 @@
  */
 
 #include <linux/module.h>
+#include <linux/err.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/device.h>
 #include <linux/gfp.h>
 
 static void devm_gpio_release(struct device *dev, void *res)
 {
-	unsigned *gpio = res;
+	struct gpio_desc **desc = res;
 
-	gpio_free(*gpio);
+	gpiod_put(*desc);
 }
 
 static int devm_gpio_match(struct device *dev, void *res, void *data)
 {
-	unsigned *this = res, *gpio = data;
+	struct gpio_desc **this = res, **desc = data;
 
-	return *this == *gpio;
+	return *this == *desc;
 }
 
 /**
@@ -50,10 +52,11 @@ static int devm_gpio_match(struct device *dev, void *res, void *data)
 
 int devm_gpio_request(struct device *dev, unsigned gpio, const char *label)
 {
-	unsigned *dr;
+	struct gpio_desc **dr;
 	int rc;
 
-	dr = devres_alloc(devm_gpio_release, sizeof(unsigned), GFP_KERNEL);
+	dr = devres_alloc(devm_gpio_release, sizeof(struct gpio_desc *),
+			  GFP_KERNEL);
 	if (!dr)
 		return -ENOMEM;
 
@@ -63,7 +66,7 @@ int devm_gpio_request(struct device *dev, unsigned gpio, const char *label)
 		return rc;
 	}
 
-	*dr = gpio;
+	*dr = gpio_to_desc(gpio);
 	devres_add(dev, dr);
 
 	return 0;
@@ -80,10 +83,11 @@ EXPORT_SYMBOL(devm_gpio_request);
 int devm_gpio_request_one(struct device *dev, unsigned gpio,
 			  unsigned long flags, const char *label)
 {
-	unsigned *dr;
+	struct gpio_desc **dr;
 	int rc;
 
-	dr = devres_alloc(devm_gpio_release, sizeof(unsigned), GFP_KERNEL);
+	dr = devres_alloc(devm_gpio_release, sizeof(struct gpio_desc *),
+			  GFP_KERNEL);
 	if (!dr)
 		return -ENOMEM;
 
@@ -93,7 +97,7 @@ int devm_gpio_request_one(struct device *dev, unsigned gpio,
 		return rc;
 	}
 
-	*dr = gpio;
+	*dr = gpio_to_desc(gpio);
 	devres_add(dev, dr);
 
 	return 0;
@@ -112,8 +116,39 @@ EXPORT_SYMBOL(devm_gpio_request_one);
  */
 void devm_gpio_free(struct device *dev, unsigned int gpio)
 {
+	struct gpio_desc *desc = gpio_to_desc(gpio);
 
-	WARN_ON(devres_release(dev, devm_gpio_release, devm_gpio_match,
-		&gpio));
+	devm_gpiod_put(dev, desc);
 }
 EXPORT_SYMBOL(devm_gpio_free);
+
+struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
+						     const char *con_id)
+{
+	struct gpio_desc **dr;
+	struct gpio_desc *desc;
+
+	dr = devres_alloc(devm_gpio_release, sizeof(struct gpio_desc *),
+			  GFP_KERNEL);
+	if (!dr)
+		return ERR_PTR(-ENOMEM);
+
+	desc = gpiod_get(dev, con_id);
+	if (IS_ERR_OR_NULL(desc)) {
+		devres_free(dr);
+		return desc;
+	}
+
+	*dr = desc;
+	devres_add(dev, dr);
+
+	return 0;
+}
+EXPORT_SYMBOL(devm_gpiod_get);
+
+void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
+{
+	WARN_ON(devres_release(dev, devm_gpio_release, devm_gpio_match,
+		&desc));
+}
+EXPORT_SYMBOL(devm_gpiod_put);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 199fca1..d04b90b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -11,6 +11,8 @@
 #include <linux/of_gpio.h>
 #include <linux/idr.h>
 #include <linux/slab.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/gpio.h>
@@ -76,6 +78,9 @@ static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 static DEFINE_IDR(dirent_idr);
 #endif
 
+static int gpiod_request(struct gpio_desc *desc, const char *label);
+static void gpiod_free(struct gpio_desc *desc);
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 #ifdef CONFIG_DEBUG_FS
@@ -83,6 +88,38 @@ 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(struct gpio_desc *desc)
+{
+	return (desc - &gpio_desc[0]) - desc->chip->base;
+}
+
+/**
+ * Convert a GPIO number to its descriptor
+ */
+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];
+}
+EXPORT_SYMBOL_GPL(gpio_to_desc);
+
+/**
+ * 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
+ */
+int desc_to_gpio(const struct gpio_desc *desc)
+{
+	return desc - &gpio_desc[0];
+}
+EXPORT_SYMBOL_GPL(desc_to_gpio);
+
+
 /* 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
@@ -94,10 +131,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)) {
@@ -116,9 +153,9 @@ static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset)
 }
 
 /* caller holds gpio_lock *OR* gpio is marked as requested */
-struct gpio_chip *gpio_to_chip(unsigned gpio)
+struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc)
 {
-	return gpio_desc[gpio].chip;
+	return desc->chip;
 }
 
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
@@ -192,19 +229,19 @@ err:
 }
 
 /* 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;
@@ -248,8 +285,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);
@@ -257,7 +293,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");
@@ -269,8 +305,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);
@@ -278,11 +313,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;
 
@@ -296,8 +331,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);
@@ -307,7 +341,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;
 
@@ -321,8 +355,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);
@@ -338,7 +371,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;
 		}
 	}
@@ -368,7 +401,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;
 
@@ -638,29 +671,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)
@@ -672,8 +708,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)
@@ -681,17 +718,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)
@@ -714,8 +752,8 @@ static struct class gpio_class = {
 
 
 /**
- * gpio_export - export a GPIO through sysfs
- * @gpio: gpio to make available, already requested
+ * gpiod_export - export a GPIO through sysfs
+ * @desc: descriptor of the gpio to make available
  * @direction_may_change: true if userspace may change gpio direction
  * Context: arch_initcall or later
  *
@@ -728,13 +766,13 @@ static struct class gpio_class = {
  *
  * Returns zero on success, else an error.
  */
-int gpio_export(unsigned gpio, bool direction_may_change)
+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) {
@@ -742,20 +780,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;
@@ -766,11 +803,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;
@@ -786,7 +825,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)
@@ -801,10 +840,11 @@ 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;
 }
-EXPORT_SYMBOL_GPL(gpio_export);
+EXPORT_SYMBOL_GPL(gpiod_export);
 
 static int match_export(struct device *dev, void *data)
 {
@@ -812,28 +852,26 @@ static int match_export(struct device *dev, void *data)
 }
 
 /**
- * gpio_export_link - create a sysfs link to an exported GPIO node
+ * gpiod_export_link - create a sysfs link to an exported GPIO node
  * @dev: device under which to create symlink
  * @name: name of the symlink
- * @gpio: gpio to create symlink to, already exported
+ * @desc: descriptor of gpio to create symlink to, already exported
  *
  * Set up a symlink from /sys/.../dev/name to /sys/class/gpio/gpioN
  * node. Caller is responsible for unlinking.
  *
  * Returns zero on success, else an error.
  */
-int gpio_export_link(struct device *dev, const char *name, unsigned gpio)
+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;
 
@@ -850,16 +888,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);
+EXPORT_SYMBOL_GPL(gpiod_export_link);
 
 
 /**
- * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
- * @gpio: gpio to change
+ * gpiod_sysfs_set_active_low - set the polarity of gpio sysfs value
+ * @desc: descriptor of gpio to change
  * @value: non-zero to use active low, i.e. inverted values
  *
  * Set the polarity of /sys/class/gpio/gpioN/value sysfs attribute.
@@ -869,19 +908,16 @@ EXPORT_SYMBOL_GPL(gpio_export_link);
  *
  * Returns zero on success, else an error.
  */
-int gpio_sysfs_set_active_low(unsigned gpio, int value)
+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) {
@@ -897,33 +933,31 @@ 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;
 }
-EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low);
+EXPORT_SYMBOL_GPL(gpiod_sysfs_set_active_low);
 
 /**
- * gpio_unexport - reverse effect of gpio_export()
- * @gpio: gpio to make unavailable
+ * gpiod_unexport - reverse effect of gpio_export()
+ * @desc: descriptor of gpio to make unavailable
  *
  * This is implicit on gpio_free().
  */
-void gpio_unexport(unsigned gpio)
+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);
@@ -935,15 +969,17 @@ 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);
 }
-EXPORT_SYMBOL_GPL(gpio_unexport);
+EXPORT_SYMBOL_GPL(gpiod_unexport);
 
 static int gpiochip_export(struct gpio_chip *chip)
 {
@@ -1297,20 +1333,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;
@@ -1334,7 +1368,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) {
@@ -1347,42 +1381,45 @@ 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_to_gpio(desc), 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);
@@ -1396,6 +1433,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);
 
 /**
@@ -1406,29 +1448,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;
 	}
@@ -1436,7 +1481,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);
@@ -1492,13 +1537,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
@@ -1515,24 +1561,21 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  * rely on gpio_request() having been called beforehand.
  */
 
-int gpio_direction_input(unsigned gpio)
+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;
 
@@ -1542,11 +1585,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.
 			 */
@@ -1554,48 +1598,49 @@ 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;
 }
-EXPORT_SYMBOL_GPL(gpio_direction_input);
+EXPORT_SYMBOL_GPL(gpiod_direction_input);
 
-int gpio_direction_output(unsigned gpio, int value)
+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;
 
@@ -1605,11 +1650,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.
 			 */
@@ -1617,45 +1663,47 @@ 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;
 }
-EXPORT_SYMBOL_GPL(gpio_direction_output);
+EXPORT_SYMBOL_GPL(gpiod_direction_output);
 
 /**
- * gpio_set_debounce - sets @debounce time for a @gpio
- * @gpio: the gpio to set debounce time
+ * gpiod_set_debounce - sets @debounce time for a gpio
+ * @desc: descriptor to the gpio to set debounce time
  * @debounce: debounce time is microseconds
  */
-int gpio_set_debounce(unsigned gpio, unsigned debounce)
+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;
 
@@ -1665,17 +1713,22 @@ 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;
 }
-EXPORT_SYMBOL_GPL(gpio_set_debounce);
+EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
 /* I/O calls are only valid after configuration completed; the relevant
  * "is this a valid GPIO" error checks should already have been done.
@@ -1700,27 +1753,28 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce);
  */
 
 /**
- * __gpio_get_value() - return a gpio's value
- * @gpio: gpio whose value will be returned
+ * gpiod_get_value() - return a gpio's value
+ * @desc: descriptor of gpio whose value will be returned
  * Context: any
  *
- * This is used directly or indirectly to implement gpio_get_value().
- * It returns the zero or nonzero value provided by the associated
+ * 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)
+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;
 }
-EXPORT_SYMBOL_GPL(__gpio_get_value);
+EXPORT_SYMBOL_GPL(gpiod_get_value);
 
 /*
  *  _gpio_set_open_drain_value() - Set the open drain gpio's value.
@@ -1728,23 +1782,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);
 }
 
 /*
@@ -1753,124 +1809,120 @@ 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
+ * gpiod_set_value() - assign a gpio's value
+ * @desc: descriptor of gpio whose value will be assigned
  * @value: value to assign
  * Context: any
  *
- * This is used directly or indirectly to implement gpio_set_value().
- * It invokes the associated gpio_chip.set() method.
+ * Invokes the associated gpio_chip.set() method.
  */
-void __gpio_set_value(unsigned gpio, int value)
+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);
 }
-EXPORT_SYMBOL_GPL(__gpio_set_value);
+EXPORT_SYMBOL_GPL(gpiod_set_value);
 
 /**
- * __gpio_cansleep() - report whether gpio value access will sleep
- * @gpio: gpio in question
+ * gpiod_cansleep() - report whether gpio value access will sleep
+ * @desc: descriptor of gpio in question
  * Context: any
  *
- * This is used directly or indirectly to implement gpio_cansleep().  It
- * returns nonzero if access reading or writing the GPIO value can sleep.
+ * Returns nonzero if access reading or writing the GPIO value can sleep.
  */
-int __gpio_cansleep(unsigned gpio)
+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 chip->can_sleep;
+	return desc->chip->can_sleep;
 }
-EXPORT_SYMBOL_GPL(__gpio_cansleep);
+EXPORT_SYMBOL_GPL(gpiod_cansleep);
 
 /**
- * __gpio_to_irq() - return the IRQ corresponding to a GPIO
- * @gpio: gpio whose IRQ will be returned (already requested)
+ * gpiod_to_irq() - return the IRQ corresponding to a GPIO
+ * @desc: descriptor of gpio whose IRQ will be returned (already requested)
  * Context: any
  *
- * This is used directly or indirectly to implement gpio_to_irq().
- * It returns the number of the IRQ signaled by this (input) GPIO,
+ * Returns the number of the IRQ signaled by this (input) GPIO,
  * or a negative errno.
  */
-int __gpio_to_irq(unsigned gpio)
+int gpiod_to_irq(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 
-	chip = gpio_to_chip(gpio);
-	return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) : -ENXIO;
+	chip = desc->chip;
+	if (chip->to_irq)
+		return chip->to_irq(chip, gpio_chip_hwgpio(desc));
+	else
+		return -ENXIO;
 }
-EXPORT_SYMBOL_GPL(__gpio_to_irq);
-
-
+EXPORT_SYMBOL_GPL(gpiod_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)
+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;
 }
-EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
+EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
 
-void gpio_set_value_cansleep(unsigned gpio, int value)
+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);
-
+EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
 
 #ifdef CONFIG_DEBUG_FS
 
@@ -1885,7 +1937,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,
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 20ca766..6fe2a03 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -10,6 +10,8 @@
 #ifdef CONFIG_GPIOLIB
 
 #include <linux/compiler.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
 
 /* Platforms may implement their GPIO interface with library code,
  * at a small performance cost for non-inlined operations and some
@@ -48,110 +50,12 @@ struct seq_file;
 struct module;
 struct device_node;
 
-/**
- * struct gpio_chip - abstract a GPIO controller
- * @label: for diagnostics
- * @dev: optional device providing the GPIOs
- * @owner: helps prevent removal of modules exporting active GPIOs
- * @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
- *	disabling module power and clock; may sleep
- * @get_direction: returns direction for signal "offset", 0=out, 1=in,
- *	(same as GPIOF_DIR_XXX), or negative error
- * @direction_input: configures signal "offset" as input, or returns error
- * @get: returns value for signal "offset"; for output signals this
- *	returns either the value actually sensed, or zero
- * @direction_output: configures signal "offset" as output, or returns error
- * @set_debounce: optional hook for setting debounce time for specified gpio in
- *      interrupt triggered gpio chips
- * @set: assigns output value for signal "offset"
- * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
- *	implementation may not sleep
- * @dbg_show: optional routine to show contents in debugfs; default code
- *	will be used when this is omitted, but custom code can show extra
- *	state (such as pullup/pulldown configuration).
- * @base: identifies the first GPIO number handled by this chip; or, if
- *	negative during registration, requests dynamic ID allocation.
- * @ngpio: the number of GPIOs handled by this controller; the last GPIO
- *	handled is (base + ngpio - 1).
- * @can_sleep: flag must be set iff get()/set() methods sleep, as they
- *	must while accessing GPIO expander chips over I2C or SPI
- * @names: if set, must be an array of strings to use as alternative
- *      names for the GPIOs in this chip. Any entry in the array
- *      may be NULL if there is no alias for the GPIO, however the
- *      array must be @ngpio entries long.  A name can include a single printk
- *      format specifier for an unsigned int.  It is substituted by the actual
- *      number of the gpio.
- *
- * A gpio_chip can help platforms abstract various sources of GPIOs so
- * they can all be accessed through a common programing interface.
- * Example sources would be SOC controllers, FPGAs, multifunction
- * chips, dedicated GPIO expanders, and so on.
- *
- * Each chip controls a number of signals, identified in method calls
- * by "offset" values in the range 0..(@ngpio - 1).  When those signals
- * are referenced through calls like gpio_get_value(gpio), the offset
- * is calculated by subtracting @base from the gpio number.
- */
-struct gpio_chip {
-	const char		*label;
-	struct device		*dev;
-	struct module		*owner;
-
-	int			(*request)(struct gpio_chip *chip,
-						unsigned offset);
-	void			(*free)(struct gpio_chip *chip,
-						unsigned offset);
-	int			(*get_direction)(struct gpio_chip *chip,
-						unsigned offset);
-	int			(*direction_input)(struct gpio_chip *chip,
-						unsigned offset);
-	int			(*get)(struct gpio_chip *chip,
-						unsigned offset);
-	int			(*direction_output)(struct gpio_chip *chip,
-						unsigned offset, int value);
-	int			(*set_debounce)(struct gpio_chip *chip,
-						unsigned offset, unsigned debounce);
-
-	void			(*set)(struct gpio_chip *chip,
-						unsigned offset, int value);
-
-	int			(*to_irq)(struct gpio_chip *chip,
-						unsigned offset);
-
-	void			(*dbg_show)(struct seq_file *s,
-						struct gpio_chip *chip);
-	int			base;
-	u16			ngpio;
-	const char		*const *names;
-	unsigned		can_sleep:1;
-	unsigned		exported:1;
-
-#if defined(CONFIG_OF_GPIO)
-	/*
-	 * If CONFIG_OF is enabled, then all GPIO controllers described in the
-	 * device tree automatically may have an OF translation
-	 */
-	struct device_node *of_node;
-	int of_gpio_n_cells;
-	int (*of_xlate)(struct gpio_chip *gc,
-		        const struct of_phandle_args *gpiospec, u32 *flags);
-#endif
-#ifdef CONFIG_PINCTRL
-	/*
-	 * If CONFIG_PINCTRL is enabled, then gpio controllers can optionally
-	 * describe the actual pin range which they serve in an SoC. This
-	 * information would be used by pinctrl subsystem to configure
-	 * corresponding pins for gpio usage.
-	 */
-	struct list_head pin_ranges;
-#endif
-};
-
 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 			unsigned offset);
-extern struct gpio_chip *gpio_to_chip(unsigned gpio);
+static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
+{
+	return gpiod_to_chip(gpio_to_desc(gpio));
+}
 extern int __must_check gpiochip_reserve(int start, int ngpio);
 
 /* add/remove chips */
@@ -168,25 +72,52 @@ extern struct gpio_chip *gpiochip_find(void *data,
 extern int gpio_request(unsigned gpio, const char *label);
 extern void gpio_free(unsigned gpio);
 
-extern int gpio_direction_input(unsigned gpio);
-extern int gpio_direction_output(unsigned gpio, int value);
+static inline int gpio_direction_input(unsigned gpio)
+{
+	return gpiod_direction_input(gpio_to_desc(gpio));
+}
+static inline int gpio_direction_output(unsigned gpio, int value)
+{
+	return gpiod_direction_output(gpio_to_desc(gpio), value);
+}
 
-extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
+static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
+{
+	return gpiod_set_debounce(gpio_to_desc(gpio), debounce);
+}
 
-extern int gpio_get_value_cansleep(unsigned gpio);
-extern void gpio_set_value_cansleep(unsigned gpio, int value);
+static inline int gpio_get_value_cansleep(unsigned gpio)
+{
+	return gpiod_get_value_cansleep(gpio_to_desc(gpio));
+}
+static inline void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+	return gpiod_set_value_cansleep(gpio_to_desc(gpio), value);
+}
 
 
 /* A platform's <asm/gpio.h> code may want to inline the I/O calls when
  * the GPIO is constant and refers to some always-present controller,
  * giving direct access to chip registers and tight bitbanging loops.
  */
-extern int __gpio_get_value(unsigned gpio);
-extern void __gpio_set_value(unsigned gpio, int value);
+static inline int __gpio_get_value(unsigned gpio)
+{
+	return gpiod_get_value(gpio_to_desc(gpio));
+}
+static inline void __gpio_set_value(unsigned gpio, int value)
+{
+	gpiod_set_value(gpio_to_desc(gpio), value);
+}
 
-extern int __gpio_cansleep(unsigned gpio);
+static inline int __gpio_cansleep(unsigned gpio)
+{
+	return gpiod_cansleep(gpio_to_desc(gpio));
+}
 
-extern int __gpio_to_irq(unsigned gpio);
+static inline int __gpio_to_irq(unsigned gpio)
+{
+	return gpiod_to_irq(gpio_to_desc(gpio));
+}
 
 extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
 extern int gpio_request_array(const struct gpio *array, size_t num);
@@ -204,11 +135,24 @@ void devm_gpio_free(struct device *dev, unsigned int gpio);
  * A sysfs interface can be exported by individual drivers if they want,
  * but more typically is configured entirely from userspace.
  */
-extern int gpio_export(unsigned gpio, bool direction_may_change);
-extern int gpio_export_link(struct device *dev, const char *name,
-			unsigned gpio);
-extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
-extern void gpio_unexport(unsigned gpio);
+
+static inline int gpio_export(unsigned gpio, bool direction_may_change)
+{
+	return gpiod_export(gpio_to_desc(gpio), direction_may_change);
+}
+static inline int gpio_export_link(struct device *dev, const char *name,
+			unsigned gpio)
+{
+	return gpiod_export_link(dev, name, gpio_to_desc(gpio));
+}
+static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
+{
+	return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value);
+}
+static inline void gpio_unexport(unsigned gpio)
+{
+	gpiod_unexport(gpio_to_desc(gpio));
+}
 
 #endif	/* CONFIG_GPIO_SYSFS */
 
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
new file mode 100644
index 0000000..0ce96cf
--- /dev/null
+++ b/include/linux/gpio/consumer.h
@@ -0,0 +1,64 @@
+#ifndef __LINUX_GPIO_CONSUMER_H
+#define __LINUX_GPIO_CONSUMER_H
+
+#ifdef CONFIG_GPIOLIB
+
+struct device;
+struct gpio_chip;
+
+/**
+ * Opaque descriptor for a GPIO. These are obtained and manipulated through
+ * the gpiod_* API and are preferable to the old integer-based handles.
+ *
+ * Contrary to integers, a pointer to a gpio_desc is guaranteed to be valid
+ * until the GPIO is released.
+ */
+struct gpio_desc;
+
+struct gpio_desc *gpio_to_desc(unsigned gpio);
+int desc_to_gpio(const struct gpio_desc *desc);
+struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc);
+
+int gpiod_direction_input(struct gpio_desc *desc);
+int gpiod_direction_output(struct gpio_desc *desc, int value);
+int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
+int gpiod_get_value_cansleep(struct gpio_desc *desc);
+void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
+
+int gpiod_get_value(struct gpio_desc *desc);
+void gpiod_set_value(struct gpio_desc *desc, int value);
+int gpiod_cansleep(struct gpio_desc *desc);
+int gpiod_to_irq(struct gpio_desc *desc);
+
+#ifdef CONFIG_GPIO_SYSFS
+
+int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
+int gpiod_export_link(struct device *dev, const char *name,
+			struct gpio_desc *desc);
+int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
+void gpiod_unexport(struct gpio_desc *desc);
+
+#else  /* CONFIG_GPIO_SYSFS */
+
+static inline int gpiod_export(struct gpio_desc *desc, bool dir_may_change)
+{
+	return -ENOSYS;
+}
+static inline int gpiod_export_link(struct device *dev, const char *name,
+				struct gpio_desc *desc)
+{
+	return -ENOSYS;
+}
+static inline int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
+{
+	return -ENOSYS;
+}
+static inline void gpiod_unexport(struct gpio_desc *desc)
+{
+}
+
+#endif	/* CONFIG_GPIO_SYSFS */
+
+#endif /* CONFIG_GPIOLIB */
+
+#endif
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
new file mode 100644
index 0000000..d42f941
--- /dev/null
+++ b/include/linux/gpio/driver.h
@@ -0,0 +1,112 @@
+#ifndef __LINUX_GPIO_DRIVER_H_
+#define __LINUX_GPIO_DRIVER_H_
+
+#ifdef CONFIG_GPIOLIB
+
+#include <linux/types.h>
+
+/**
+ * struct gpio_chip - abstract a GPIO controller
+ * @label: for diagnostics
+ * @dev: optional device providing the GPIOs
+ * @owner: helps prevent removal of modules exporting active GPIOs
+ * @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
+ *	disabling module power and clock; may sleep
+ * @get_direction: returns direction for signal "offset", 0=out, 1=in,
+ *	(same as GPIOF_DIR_XXX), or negative error
+ * @direction_input: configures signal "offset" as input, or returns error
+ * @get: returns value for signal "offset"; for output signals this
+ *	returns either the value actually sensed, or zero
+ * @direction_output: configures signal "offset" as output, or returns error
+ * @set_debounce: optional hook for setting debounce time for specified gpio in
+ *      interrupt triggered gpio chips
+ * @set: assigns output value for signal "offset"
+ * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
+ *	implementation may not sleep
+ * @dbg_show: optional routine to show contents in debugfs; default code
+ *	will be used when this is omitted, but custom code can show extra
+ *	state (such as pullup/pulldown configuration).
+ * @base: identifies the first GPIO number handled by this chip; or, if
+ *	negative during registration, requests dynamic ID allocation.
+ * @ngpio: the number of GPIOs handled by this controller; the last GPIO
+ *	handled is (base + ngpio - 1).
+ * @can_sleep: flag must be set iff get()/set() methods sleep, as they
+ *	must while accessing GPIO expander chips over I2C or SPI
+ * @names: if set, must be an array of strings to use as alternative
+ *      names for the GPIOs in this chip. Any entry in the array
+ *      may be NULL if there is no alias for the GPIO, however the
+ *      array must be @ngpio entries long.  A name can include a single printk
+ *      format specifier for an unsigned int.  It is substituted by the actual
+ *      number of the gpio.
+ *
+ * A gpio_chip can help platforms abstract various sources of GPIOs so
+ * they can all be accessed through a common programing interface.
+ * Example sources would be SOC controllers, FPGAs, multifunction
+ * chips, dedicated GPIO expanders, and so on.
+ *
+ * Each chip controls a number of signals, identified in method calls
+ * by "offset" values in the range 0..(@ngpio - 1).  When those signals
+ * are referenced through calls like gpio_get_value(gpio), the offset
+ * is calculated by subtracting @base from the gpio number.
+ */
+struct gpio_chip {
+	const char		*label;
+	struct device		*dev;
+	struct module		*owner;
+
+	int			(*request)(struct gpio_chip *chip,
+						unsigned offset);
+	void			(*free)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*get_direction)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*direction_input)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*get)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*direction_output)(struct gpio_chip *chip,
+						unsigned offset, int value);
+	int			(*set_debounce)(struct gpio_chip *chip,
+						unsigned offset,
+						unsigned debounce);
+
+	void			(*set)(struct gpio_chip *chip,
+						unsigned offset, int value);
+
+	int			(*to_irq)(struct gpio_chip *chip,
+						unsigned offset);
+
+	void			(*dbg_show)(struct seq_file *s,
+						struct gpio_chip *chip);
+	int			base;
+	u16			ngpio;
+	const char		*const *names;
+	unsigned		can_sleep:1;
+	unsigned		exported:1;
+
+#if defined(CONFIG_OF_GPIO)
+	/*
+	 * If CONFIG_OF is enabled, then all GPIO controllers described in the
+	 * device tree automatically may have an OF translation
+	 */
+	struct device_node *of_node;
+	int of_gpio_n_cells;
+	int (*of_xlate)(struct gpio_chip *gc,
+			const struct of_phandle_args *gpiospec, u32 *flags);
+#endif
+#ifdef CONFIG_PINCTRL
+	/*
+	 * If CONFIG_PINCTRL is enabled, then gpio controllers can optionally
+	 * describe the actual pin range which they serve in an SoC. This
+	 * information would be used by pinctrl subsystem to configure
+	 * corresponding pins for gpio usage.
+	 */
+	struct list_head pin_ranges;
+#endif
+};
+
+#endif /* CONFIG_GPIOLIB */
+
+#endif
-- 
1.8.1

[PATCH 2/4] gpiolib: add gpiod_get and gpiod_put functions

[Alexandre Courbot]

Adds new GPIO allocation functions that work with the opaque descriptor
interface.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c        | 164 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h |   8 +++
 include/linux/gpio/driver.h   |  21 ++++++
 3 files changed, 193 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d04b90b..06ffadb 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>
@@ -74,6 +75,11 @@ struct gpio_desc {
 };
 static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
+/* protects both gpio_lookup_list and gpio_chips */
+static DEFINE_MUTEX(gpio_lookup_lock);
+static LIST_HEAD(gpio_lookup_list);
+static LIST_HEAD(gpio_chips);
+
 #ifdef CONFIG_GPIO_SYSFS
 static DEFINE_IDR(dirent_idr);
 #endif
@@ -1162,6 +1168,11 @@ int gpiochip_add(struct gpio_chip *chip)
 
 	of_gpiochip_add(chip);
 
+	INIT_LIST_HEAD(&chip->list);
+	mutex_lock(&gpio_lookup_lock);
+	list_add(&chip->list, &gpio_chips);
+	mutex_unlock(&gpio_lookup_lock);
+
 unlock:
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
@@ -1200,6 +1211,10 @@ int gpiochip_remove(struct gpio_chip *chip)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
+	mutex_lock(&gpio_lookup_lock);
+	list_del_init(&chip->list);
+	mutex_unlock(&gpio_lookup_lock);
+
 	gpiochip_remove_pin_ranges(chip);
 	of_gpiochip_remove(chip);
 
@@ -1924,6 +1939,155 @@ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
 }
 EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
 
+
+/**
+ * gpioh_add_table() - register GPIO device consumers
+ * @table: array of consumers to register
+ * @num: number of consumers in table
+ */
+void gpiod_add_table(struct gpiod_lookup *table, size_t size)
+{
+	mutex_lock(&gpio_lookup_lock);
+
+	while (size--) {
+		list_add_tail(&table->list, &gpio_lookup_list);
+		table++;
+	}
+
+	mutex_unlock(&gpio_lookup_lock);
+}
+
+/*
+ * Caller must have a acquired gpio_lookup_lock
+ */
+static struct gpio_chip *find_chip_by_name(const char *name)
+{
+	struct gpio_chip *chip = NULL;
+
+	list_for_each_entry(chip, &gpio_lookup_list, list) {
+		if (chip->label == NULL)
+			continue;
+		if (!strcmp(chip->label, name))
+			break;
+	}
+
+	return chip;
+}
+
+#ifdef CONFIG_OF
+static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id)
+{
+	char prop_name[32]; /* 32 is max size of property name */
+
+	if (con_id)
+		snprintf(prop_name, 32, "%s-gpios", con_id);
+	else
+		snprintf(prop_name, 32, "gpios");
+
+	return of_get_named_gpiod_flags(dev->of_node, prop_name, 0, NULL);
+}
+#else
+static struct device_node *of_find_gpio(struct device *dev, const char *id)
+{
+	return NULL;
+}
+#endif
+
+static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id)
+{
+	const char *dev_id = dev ? dev_name(dev) : NULL;
+	struct gpio_desc *desc = ERR_PTR(-ENODEV);
+	unsigned int match, best = 0;
+	struct gpiod_lookup *p;
+
+	mutex_lock(&gpio_lookup_lock);
+
+	list_for_each_entry(p, &gpio_lookup_list, list) {
+		match = 0;
+
+		if (p->dev_id) {
+			if (!dev_id || strcmp(p->dev_id, dev_id))
+				continue;
+
+			match += 2;
+		}
+
+		if (p->con_id) {
+			if (!con_id || strcmp(p->con_id, con_id))
+				continue;
+
+			match += 1;
+		}
+
+		if (match > best) {
+			struct gpio_chip *chip;
+
+			chip = find_chip_by_name(p->chip_label);
+			if (!chip) {
+				dev_warn(dev, "cannot find GPIO chip %s\n",
+					 p->chip_label);
+				continue;
+			}
+
+			if (chip->ngpio >= p->chip_hwnum) {
+				dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
+					 chip->label, chip->ngpio);
+				continue;
+			}
+
+			desc = gpio_to_desc(chip->base + p->chip_hwnum);
+
+			if (match != 3)
+				best = match;
+			else
+				break;
+		}
+	}
+
+	mutex_unlock(&gpio_lookup_lock);
+
+	return desc;
+}
+
+/**
+ *
+ */
+struct gpio_desc *__must_check gpiod_get(struct device *dev, const char *con_id)
+{
+	struct gpio_desc *desc;
+	int status;
+
+	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
+
+	/* Using device tree? */
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
+		dev_dbg(dev, "using device tree for GPIO lookup\n");
+		desc = of_find_gpio(dev, con_id);
+	} else {
+		dev_dbg(dev, "using lookup tables for GPIO lookup");
+		desc = gpiod_find(dev, con_id);
+	}
+
+	if (IS_ERR(desc)) {
+		dev_warn(dev, "lookup for GPIO %s failed\n", con_id);
+		return desc;
+	}
+
+	status = gpiod_request(desc, con_id);
+
+	if (status < 0)
+		return ERR_PTR(status);
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_get);
+
+void gpiod_put(struct gpio_desc *desc)
+{
+	gpiod_free(desc);
+}
+EXPORT_SYMBOL_GPL(gpiod_put);
+
 #ifdef CONFIG_DEBUG_FS
 
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 0ce96cf..2f30761 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -15,6 +15,14 @@ struct gpio_chip;
  */
 struct gpio_desc;
 
+struct gpio_desc *__must_check gpiod_get(struct device *dev,
+					 const char *con_id);
+void gpiod_put(struct gpio_desc *desc);
+
+struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
+					      const char *con_id);
+void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
+
 struct gpio_desc *gpio_to_desc(unsigned gpio);
 int desc_to_gpio(const struct gpio_desc *desc);
 struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index d42f941..d43d671 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -4,12 +4,14 @@
 #ifdef CONFIG_GPIOLIB
 
 #include <linux/types.h>
+#include <linux/list.h>
 
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @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
@@ -55,6 +57,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);
@@ -107,6 +110,24 @@ struct gpio_chip {
 #endif
 };
 
+struct gpiod_lookup {
+	struct list_head list;
+	const char *chip_label;
+	u16 chip_hwnum;
+	const char *dev_id;
+	const char *con_id;
+};
+
+#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _dev_id, _con_id)	\
+{								\
+	.chip_label = _chip_label,				\
+	.chip_hwnum = _chip_hwnum,				\
+	.dev_id = _dev_id,					\
+	.con_id = _con_id,					\
+}
+
+void gpiod_add_table(struct gpiod_lookup *table, size_t size);
+
 #endif /* CONFIG_GPIOLIB */
 
 #endif
-- 
1.8.1

[PATCH 3/4] gpiolib: of: convert OF helpers to descriptor API

[Alexandre Courbot]

Convert gpiolib-of.c's internals to rely on descriptors instead of
integers and add the gpiod_ counterparts of existing OF functions.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib-of.c     | 26 +++++++++------
 include/linux/gpio/consumer.h | 56 +++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h   | 38 ++++++++++++++++++++++
 include/linux/of_gpio.h       | 73 ++++++-------------------------------------
 4 files changed, 120 insertions(+), 73 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index d542a14..8c9f8c5 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -13,6 +13,7 @@
 
 #include <linux/device.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
@@ -22,12 +23,14 @@
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
 
+struct gpio_desc;
+
 /* Private data structure for of_gpiochip_find_and_xlate */
 struct gg_data {
 	enum of_gpio_flags *flags;
 	struct of_phandle_args gpiospec;
 
-	int out_gpio;
+	struct gpio_desc *out_gpio;
 };
 
 /* Private function for resolving node pointer to gpio_chip */
@@ -45,28 +48,31 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
 	if (ret < 0)
 		return false;
 
-	gg_data->out_gpio = ret + gc->base;
+	gg_data->out_gpio = gpio_to_desc(ret + gc->base);
 	return true;
 }
 
 /**
- * of_get_named_gpio_flags() - Get a GPIO number and flags to use with GPIO API
+ * of_get_named_gpiod_flags() - Get a GPIO descriptor and flags for GPIO API
  * @np:		device node to get GPIO from
  * @propname:	property name containing gpio specifier(s)
  * @index:	index of the GPIO
  * @flags:	a flags pointer to fill in
  *
- * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
+ * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
  * value on the error condition. If @flags is not NULL the function also fills
  * in flags for the GPIO.
  */
-int of_get_named_gpio_flags(struct device_node *np, const char *propname,
-                           int index, enum of_gpio_flags *flags)
+struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
+		const char *propname, int index, enum of_gpio_flags *flags)
 {
 	/* Return -EPROBE_DEFER to support probe() functions to be called
 	 * later when the GPIO actually becomes available
 	 */
-	struct gg_data gg_data = { .flags = flags, .out_gpio = -EPROBE_DEFER };
+	struct gg_data gg_data = {
+		.flags = flags,
+		.out_gpio = ERR_PTR(-EPROBE_DEFER),
+	};
 	int ret;
 
 	/* .of_xlate might decide to not fill in the flags, so clear it. */
@@ -77,13 +83,15 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 					 &gg_data.gpiospec);
 	if (ret) {
 		pr_debug("%s: can't parse gpios property\n", __func__);
-		return ret;
+		return ERR_PTR(ret);
 	}
 
 	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
 
 	of_node_put(gg_data.gpiospec.np);
-	pr_debug("%s exited with status %d\n", __func__, gg_data.out_gpio);
+
+	ret = PTR_RET(gg_data.out_gpio);
+	pr_debug("%s exited with status %d\n", __func__, ret);
 	return gg_data.out_gpio;
 }
 EXPORT_SYMBOL(of_get_named_gpio_flags);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 2f30761..b6a3bc8 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -3,6 +3,8 @@
 
 #ifdef CONFIG_GPIOLIB
 
+#include <linux/kernel.h>
+
 struct device;
 struct gpio_chip;
 
@@ -67,6 +69,60 @@ static inline void gpiod_unexport(struct gpio_desc *desc)
 
 #endif	/* CONFIG_GPIO_SYSFS */
 
+
+struct device_node;
+
+/*
+ * This is Linux-specific flags. By default controllers' and Linux' mapping
+ * match, but GPIO controllers are free to translate their own flags to
+ * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended.
+ */
+enum of_gpio_flags {
+	OF_GPIO_ACTIVE_LOW = 0x1,
+};
+
+#ifdef CONFIG_OF_GPIO
+
+extern unsigned int of_gpio_named_count(struct device_node *np,
+					const char *propname);
+
+extern struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
+		const char *list_name, int index, enum of_gpio_flags *flags);
+
+#else
+
+/* Drivers may not strictly depend on the GPIO support, so let them link. */
+static inline unsigned int of_gpio_named_count(struct device_node *np,
+					       const char *propname)
+{
+	return 0;
+}
+
+static inline struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
+		const char *list_name, int index, enum of_gpio_flags *flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+#endif /* CONFIG_OF_GPIO */
+
+static inline struct gpio_desc *of_get_gpiod_flags(struct device_node *np,
+					int index, enum of_gpio_flags *flags)
+{
+	return of_get_named_gpiod_flags(np, "gpios", index, flags);
+}
+
+static inline struct gpio_desc *of_get_named_gpiod(struct device_node *np,
+					const char *propname, int index)
+{
+	return of_get_named_gpiod_flags(np, propname, index, NULL);
+}
+
+static inline struct gpio_desc *of_get_gpiod(struct device_node *np, int index)
+{
+	return of_get_gpiod_flags(np, index, NULL);
+}
+
 #endif /* CONFIG_GPIOLIB */
 
 #endif
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index d43d671..84447a5 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -128,6 +128,44 @@ struct gpiod_lookup {
 
 void gpiod_add_table(struct gpiod_lookup *table, size_t size);
 
+#ifdef CONFIG_OF_GPIO
+
+/*
+ * OF GPIO chip for memory mapped banks
+ */
+struct of_mm_gpio_chip {
+	struct gpio_chip gc;
+	void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
+	void __iomem *regs;
+};
+
+static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
+{
+	return container_of(gc, struct of_mm_gpio_chip, gc);
+}
+
+extern int of_mm_gpiochip_add(struct device_node *np,
+			      struct of_mm_gpio_chip *mm_gc);
+extern void of_gpiochip_add(struct gpio_chip *gc);
+extern void of_gpiochip_remove(struct gpio_chip *gc);
+extern int of_gpio_simple_xlate(struct gpio_chip *gc,
+				const struct of_phandle_args *gpiospec,
+				u32 *flags);
+
+#else
+
+static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
+				       const struct of_phandle_args *gpiospec,
+				       u32 *flags)
+{
+	return -ENOSYS;
+}
+
+static inline void of_gpiochip_add(struct gpio_chip *gc) { }
+static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
+
+#endif /* CONFIG_OF_GPIO */
+
 #endif /* CONFIG_GPIOLIB */
 
 #endif
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index c454f57..f703707 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -17,78 +17,23 @@
 #include <linux/compiler.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
 
-struct device_node;
-
-/*
- * This is Linux-specific flags. By default controllers' and Linux' mapping
- * match, but GPIO controllers are free to translate their own flags to
- * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended.
- */
-enum of_gpio_flags {
-	OF_GPIO_ACTIVE_LOW = 0x1,
-};
-
-#ifdef CONFIG_OF_GPIO
-
-/*
- * OF GPIO chip for memory mapped banks
- */
-struct of_mm_gpio_chip {
-	struct gpio_chip gc;
-	void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
-	void __iomem *regs;
-};
-
-static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
-{
-	return container_of(gc, struct of_mm_gpio_chip, gc);
-}
-
-extern int of_get_named_gpio_flags(struct device_node *np,
-		const char *list_name, int index, enum of_gpio_flags *flags);
-
-extern unsigned int of_gpio_named_count(struct device_node *np,
-					const char* propname);
-
-extern int of_mm_gpiochip_add(struct device_node *np,
-			      struct of_mm_gpio_chip *mm_gc);
-
-extern void of_gpiochip_add(struct gpio_chip *gc);
-extern void of_gpiochip_remove(struct gpio_chip *gc);
-extern int of_gpio_simple_xlate(struct gpio_chip *gc,
-				const struct of_phandle_args *gpiospec,
-				u32 *flags);
-
-#else /* CONFIG_OF_GPIO */
-
-/* Drivers may not strictly depend on the GPIO support, so let them link. */
 static inline int of_get_named_gpio_flags(struct device_node *np,
 		const char *list_name, int index, enum of_gpio_flags *flags)
 {
-	return -ENOSYS;
-}
-
-static inline unsigned int of_gpio_named_count(struct device_node *np,
-					const char* propname)
-{
-	return 0;
-}
-
-static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
-				       const struct of_phandle_args *gpiospec,
-				       u32 *flags)
-{
-	return -ENOSYS;
+	struct gpio_desc *desc;
+	desc = of_get_named_gpiod_flags(np, list_name, index, flags);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+	else
+		return desc_to_gpio(desc);
 }
 
-static inline void of_gpiochip_add(struct gpio_chip *gc) { }
-static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
-
-#endif /* CONFIG_OF_GPIO */
-
 /**
  * of_gpio_count - Count GPIOs for a device
  * @np:		device node to count GPIOs for
-- 
1.8.1

[PATCH 4/4] gpiolib: add documentation for new gpiod_ API

[Alexandre Courbot]

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/gpio.txt | 94 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 2 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 77a1d11..871ccda 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -120,7 +120,8 @@ example, a number might be valid but temporarily unused on a given board.
 
 Whether a platform supports multiple GPIO controllers is a platform-specific
 implementation issue, as are whether that support can leave "holes" in the space
-of GPIO numbers, and whether new controllers can be added at runtime.  Such issues
+of GPIO numbers, and whether new controllers can be added at runtime.  Such
+issues
 can affect things including whether adjacent GPIO numbers are both valid.
 
 Using GPIOs
@@ -302,7 +303,8 @@ are claimed, three additional calls are defined:
 	 * 'flags', identical to gpio_request() wrt other arguments and
 	 * return value
 	 */
-	int gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
+	int gpio_request_one(unsigned gpio, unsigned long flags, const char
+*label);
 
 	/* request multiple GPIOs in a single call
 	 */
@@ -773,3 +775,91 @@ differences between boards from user space.  This only affects the
 sysfs interface.  Polarity change can be done both before and after
 gpio_export(), and previously enabled poll(2) support for either
 rising or falling edge will be reconfigured to follow this setting.
+
+GPIO descriptor interface
+=========================
+A more secure, alternative GPIO interface is available through GPIOlib. Instead
+of relying on integers (which can easily be forged and used without being
+properly requested) to reference GPIOs, it uses a system of opaque descriptors
+that must be properly obtained and disposed through the common get/put set of
+functions. This ensures that all GPIO descriptors are valid at any time and
+makes it unnecessary to check the validity of a GPIO. Apart from this
+difference, the interface is similar to the integer-based one, excepted that the
+gpio_ prefix is changed to gpiod_.
+
+This interface can be used in conjonction with the integer-based API, however
+new drivers should really try to use the safer descriptor-based interface.
+Drivers using this interface should depend on CONFIG_GPIOLIB being set, as it is
+only available when GPIOlib is compiled in.
+
+Using GPIOs
+-----------
+GPIO consumers should include <linux/gpio/consumer.h> which declares the
+consumer-side GPIO functions. GPIOs are obtained through gpiod_get:
+
+	struct gpio_desc *gpiod_get(struct device *dev,
+				    const char *con_id);
+
+This will return the GPIO descriptor corresponding to the con_id function of
+dev, or an error code in case of error. A devm variant is also available:
+
+	struct gpio_desc *devm_gpiod_get(struct device *dev,
+					 const char *con_id);
+
+GPIO descriptors are disposed using the corresponding put functions:
+
+	void gpiod_put(struct gpio_desc *desc);
+	void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
+
+A valid descriptor can then be used with one of the gpiod_ functions. Their
+interface is identical to the integer-based API, excepted that they take a GPIO
+descriptor instead of an integer:
+
+	int gpiod_direction_input(struct gpio_desc *desc);
+	int gpiod_direction_output(struct gpio_desc *desc, int value);
+	int gpiod_get_value(struct gpio_desc *desc);
+	void gpiod_set_value(struct gpio_desc *desc, int value);
+	...
+
+If you need to convert a descriptor to an integer or vice-versa, you can use
+gpio_to_desc or desc_to_gpio:
+
+	struct gpio_desc *gpio_to_desc(unsigned gpio);
+	int desc_to_gpio(const struct gpio_desc *desc);
+
+The same GPIO can be used by both interfaces as long as it has properly been
+acquired by one of them (i.e. using either gpio_request() or gpiod_get()).
+
+Declaring GPIOs
+---------------
+GPIOs can be made available for devices either through board-specific lookup
+tables, or using the device tree.
+
+Board-specific lookup tables match a device name and consumer ID to a GPIO chip
+and GPIO number relative to that chip. They are declared as follows:
+
+	static struct gpio_lookup board_gpio_lookup[] = {
+		GPIO_LOOKUP("tegra-gpio", 28, "backlight.1", "power"),
+	};
+
+	static void __init board_init(void)
+	{
+		...
+		gpiod_add_table(board_gpio_lookup,
+				ARRAY_SIZE(board_gpio_lookup));
+		...
+	}
+
+In the driver side, the following code:
+
+	gpiod_get(dev, "power");
+
+will return the descriptor for GPIO 28 of the "tegra-gpio" chip provided
+strcmp(dev_name(dev), "backlight.1") == 0.
+
+If the device tree is used, then the same "power" GPIO can be declared into the
+device's node as follows:
+
+	power-gpios = <&gpio 28 0>;
+
+Note that gpiod_get() only allows to access the first GPIO specifier.
\ No newline at end of file
-- 
1.8.1

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Arnd Bergmann]

On Tuesday 08 January 2013, Alexandre Courbot wrote:
> +struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
> +                                                    const char *con_id)
> +{
> +       struct gpio_desc **dr;
> +       struct gpio_desc *desc;
> +
> +       dr = devres_alloc(devm_gpio_release, sizeof(struct gpio_desc *),
> +                         GFP_KERNEL);
> +       if (!dr)
> +               return ERR_PTR(-ENOMEM);
> +
> +       desc = gpiod_get(dev, con_id);
> +       if (IS_ERR_OR_NULL(desc)) {
> +               devres_free(dr);
> +               return desc;
> +       }

Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
should not check for that.

> +       *dr = desc;
> +       devres_add(dev, dr);
> +
> +       return 0;
> +}

I'm pretty sure you meant to write 'return desc;' here.

	Arnd

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Arnd Bergmann]

On Tuesday 08 January 2013, Alexandre Courbot wrote:
> This series introduce a first take at implementing the RFC for the new GPIO API
> that I submitted last month. It proposes a new, opaque descriptor-based GPIO API
> that becomes available when GPIOlib is compiled, and provides a safer, more
> abstract alternative to the current integer-based interface. GPIOlib internals
> are also switched to use the descriptor logic, and the former integer API
> becomes a lightweight wrapper around the new descriptor-based API.
> 
> Functionally speaking the new API is identical to the integer-based API, with
> only the prefix changing from gpio_ to gpiod_. However, the second patch
> introduces new functions for obtaining GPIOs from a device and a consumer name,
> in a fashion similar to what is done with e.g. regulators and PWMs.

I like the interface, good idea!

A few questions:

Is there a plan for migrating all the existing users of the current GPIO
interface?

How do you want to deal with drivers that work on platforms that currently
don't use gpiolib but have their own implementation of asm/gpio.h? Are
we going to phase them out?

If we are adding a new way to deal with GPIOs, would it make sense to
have that more closely integrated into pinctrl in one form or another?
My feeling is that there is already a significant overlap between the
two, and while the addition of the gpiod_* functions doesn't necessarily
make this worse, it could be a chance to improve the current situation
to make the interface more consistent with pinctrl.

	Arnd

Re: [PATCH 2/4] gpiolib: add gpiod_get and gpiod_put functions

[Arnd Bergmann]

On Tuesday 08 January 2013, Alexandre Courbot wrote:
> 
> Adds new GPIO allocation functions that work with the opaque descriptor
> interface.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

I think you need to reorder the patches slightly, since the gpiod_get
function introduced here is already being used in the first patch, which
breaks bisection.

	Arnd

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Alexandre Courbot]

On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> should not check for that.

Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,
may I ask why this is the case?

>> +       *dr = desc;
>> +       devres_add(dev, dr);
>> +
>> +       return 0;
>> +}
>
> I'm pretty sure you meant to write 'return desc;' here.

Absolutely, thanks.

Alex.

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Alexandre Courbot]

On Tue, Jan 8, 2013 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> I like the interface, good idea!

Great! This was initially suggested by Linus W.

> A few questions:
>
> Is there a plan for migrating all the existing users of the current GPIO
> interface?

Nothing specifically planned for now, as we need to make sure the new
interface covers all needs first. There would be a lot of drivers to
change if we decide to deprecate the integer interface, but Coccinelle
could probably help here.

The question is, do we want to totally get rid of the integer
namespace? That would be the ultimate step, but would require another
way to identify GPIOs (controller_device:offset might be such a way),
and also to reorganize sysfs nodes. Wouldn't that be considered
breaking user-space? 'cause we all know what happens to those who
break user-space.

> How do you want to deal with drivers that work on platforms that currently
> don't use gpiolib but have their own implementation of asm/gpio.h? Are
> we going to phase them out?

With the current code, a driver should depend on gpiolib being
compiled if it uses the new interface. It is not even declared if
gpiolib is not used.

Given that both interfaces are quite close, one could imagine having a
gpiod wrapper around the integer namespace (the "opaque descriptors"
would then just be casted integers). This way drivers would only need
to depend on GENERIC_GPIO. It's a little bit weird to have gpiod
wrapping around gpio in one case and the opposite in another though -
I'd rather have these platforms convert to GPIO descriptors internally
(or even better, to gpiolib), but this is probably asking too much.

I do not know all the details of gpiolib's history, but why would
anyone want to implement the generic gpio interface and not use
gpiolib anyways?

Then there are platforms who do not follow generic gpios and implement
their own sauce. I don't think we need to care here, as these are not
supposed to be used with generic drivers anyway.

> If we are adding a new way to deal with GPIOs, would it make sense to
> have that more closely integrated into pinctrl in one form or another?
> My feeling is that there is already a significant overlap between the
> two, and while the addition of the gpiod_* functions doesn't necessarily
> make this worse, it could be a chance to improve the current situation
> to make the interface more consistent with pinctrl.

That may be a chance to introduce deeper changes indeed - what do you
have in mind exactly?

Thanks,
Alex.

Re: [PATCH 2/4] gpiolib: add gpiod_get and gpiod_put functions

[Alexandre Courbot]

On Tue, Jan 8, 2013 at 10:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 08 January 2013, Alexandre Courbot wrote:
>>
>> Adds new GPIO allocation functions that work with the opaque descriptor
>> interface.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> I think you need to reorder the patches slightly, since the gpiod_get
> function introduced here is already being used in the first patch, which
> breaks bisection.

Yes, gpiod_get and gpiod_put should only appear from the second patch
on. Thanks for pointing that out.

Alex.

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 10:06:16AM +0900, Alexandre Courbot wrote:
> On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> > introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> > should not check for that.
> 
> Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,
> may I ask why this is the case?

I think I've explained that in the past; many people just do not think.
They just use whatever macro they feel like is the right one.  We keep
seeing this, and this is a persistent problem. It's getting to be more
of a problem because people are starting to argue back when you point
out that they're wrong.

People are even starting to believe that documentation which specifies
explicitly "values where IS_ERR() is true are considered errors,
everything else is valid" means that the use of IS_ERR_OR_NULL() in such
cases is permissible.  (I've had such an argument with two people
recently.)

So, interfaces which have well defined return values and even interfaces
which specify _how_ errors should be checked end up being checked with
the wrong macros.  People constantly translate IS_ERR() to IS_ERR_OR_NULL()
even when it's inappropriate.

People don't think and people don't read documentation.  People don't
remember this level of detail.  Whatever the excuse, the problem remains.
IS_ERR_OR_NULL() always gets used inappropriately and without any regard
to whether it's correct or not.

So yes, IS_ERR_OR_NULL() _is_ pure evil.  IMHO this macro is doing more
harm than good.

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Arnd Bergmann]

On Wednesday 09 January 2013, Alexandre Courbot wrote:
> On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> > introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> > should not check for that.
> 
> Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,

Correct.

> may I ask why this is the case?

It's very hard to get right: either you are interested in the error code,
and then you don't have one in some cases, or you don't care but have
to check for it anyway. When you define a function, just make it clear
what the expected return values are, either NULL for error or a negative
ERR_PTR value, but not both.

	Arnd

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 10:35:22AM +0000, Arnd Bergmann wrote:
> On Wednesday 09 January 2013, Alexandre Courbot wrote:
> > On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> > > introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> > > should not check for that.
> > 
> > Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,
> 
> Correct.
> 
> > may I ask why this is the case?
> 
> It's very hard to get right: either you are interested in the error code,
> and then you don't have one in some cases, or you don't care but have
> to check for it anyway. When you define a function, just make it clear
> what the expected return values are, either NULL for error or a negative
> ERR_PTR value, but not both.

Indeed, and any code which does this:

	if (IS_ERR_OR_NULL(ptr))
		return PTR_ERR(ptr);

is buggy because on NULL it returns 0, which is generally accepted as being
"success".

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Arnd Bergmann]

On Wednesday 09 January 2013, Alexandre Courbot wrote:
> On Tue, Jan 8, 2013 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > I like the interface, good idea!
> 
> Great! This was initially suggested by Linus W.
> 
> > A few questions:
> >
> > Is there a plan for migrating all the existing users of the current GPIO
> > interface?
> 
> Nothing specifically planned for now, as we need to make sure the new
> interface covers all needs first. There would be a lot of drivers to
> change if we decide to deprecate the integer interface, but Coccinelle
> could probably help here.

Right.

> The question is, do we want to totally get rid of the integer
> namespace? That would be the ultimate step, but would require another
> way to identify GPIOs (controller_device:offset might be such a way),
> and also to reorganize sysfs nodes. Wouldn't that be considered
> breaking user-space? 'cause we all know what happens to those who
> break user-space.

The user interface could eventually be the only part of the kernel that
uses the numbers, but you are right that we cannot change that.

> > How do you want to deal with drivers that work on platforms that currently
> > don't use gpiolib but have their own implementation of asm/gpio.h? Are
> > we going to phase them out?
> 
> With the current code, a driver should depend on gpiolib being
> compiled if it uses the new interface. It is not even declared if
> gpiolib is not used.
> 
> Given that both interfaces are quite close, one could imagine having a
> gpiod wrapper around the integer namespace (the "opaque descriptors"
> would then just be casted integers). This way drivers would only need
> to depend on GENERIC_GPIO. It's a little bit weird to have gpiod
> wrapping around gpio in one case and the opposite in another though -
> I'd rather have these platforms convert to GPIO descriptors internally
> (or even better, to gpiolib), but this is probably asking too much.

I think it would be reasonable to force everybody to use gpiolib,
that's much easier than converting everyone to the descriptor based
interface.

> I do not know all the details of gpiolib's history, but why would
> anyone want to implement the generic gpio interface and not use
> gpiolib anyways?

Only legacy users did this. Initially there was only the header file,
with the API declared but several different implementations of it.
gpiolib was introduced later to reduce code duplication and allow having
multiple implementations in the same kernel.

> Then there are platforms who do not follow generic gpios and implement
> their own sauce. I don't think we need to care here, as these are not
> supposed to be used with generic drivers anyway.

I agree. Ideally we would convert them over as well, but there is no
need to rush that, or force it on anybody. We should just make sure
we don't grow any new platforms doing this.

> > If we are adding a new way to deal with GPIOs, would it make sense to
> > have that more closely integrated into pinctrl in one form or another?
> > My feeling is that there is already a significant overlap between the
> > two, and while the addition of the gpiod_* functions doesn't necessarily
> > make this worse, it could be a chance to improve the current situation
> > to make the interface more consistent with pinctrl.
> 
> That may be a chance to introduce deeper changes indeed - what do you
> have in mind exactly?

I don't know enough about pinctrl to have a specific idea yet, but maybe
someone else has ideas. Regarding the integration of pinctrl with gpio,
I was thinking in the past that we could make pinctrl provide everything
that gpiolib does, and have a generic gpiolib driver on top of pinctrl
so that platforms don't need to implement both interfaces but only need
to provide a pure pinctrl driver. Not sure if this makes any sense.

	Arnd

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 10:44:14AM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 09, 2013 at 10:35:22AM +0000, Arnd Bergmann wrote:
> > On Wednesday 09 January 2013, Alexandre Courbot wrote:
> > > On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > > Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> > > > introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> > > > should not check for that.
> > > 
> > > Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,
> > 
> > Correct.
> > 
> > > may I ask why this is the case?
> > 
> > It's very hard to get right: either you are interested in the error code,
> > and then you don't have one in some cases, or you don't care but have
> > to check for it anyway. When you define a function, just make it clear
> > what the expected return values are, either NULL for error or a negative
> > ERR_PTR value, but not both.
> 
> Indeed, and any code which does this:
> 
> 	if (IS_ERR_OR_NULL(ptr))
> 		return PTR_ERR(ptr);
> 
> is buggy because on NULL it returns 0, which is generally accepted as being
> "success".

                oh = omap_hwmod_lookup(oh_name);
                if (!oh) {

        oh = omap_hwmod_lookup(oh_name);
        if (IS_ERR_OR_NULL(oh)) {

Does this function return NULL on errors or an errno-encoded-pointer on
errors?

        d = debugfs_create_dir("pm_debug", NULL);
        if (IS_ERR_OR_NULL(d))
                return PTR_ERR(d);

Well, covered above.  NULL is success here.

        err = gpio_request(en_vdd_1v05, "EN_VDD_1V05");
        if (err) {
                pr_err("%s: gpio_request failed: %d\n", __func__, err);
                return err;
        }

        gpio_direction_output(en_vdd_1v05, 1);

        regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
        if (IS_ERR_OR_NULL(regulator)) {
                pr_err("%s: regulator_get failed: %d\n", __func__,
                       (int)PTR_ERR(regulator));
                goto err_reg;
        }
...
err_reg:
        gpio_free(en_vdd_1v05);

        return err;
}

So 'err' here is never set.  when IS_ERR_OR_NULL evaluates true.
Setting 'err' to PTR_ERR(regulator) is not correct because a NULL return
sets 'err' to zero.  Same here:

        /* create property id */
        ret = ipp_create_id(&ctx->prop_idr, &ctx->prop_lock, c_node,
                &property->prop_id);
        if (ret) {
                DRM_ERROR("failed to create id.\n");
                goto err_clear;
        }
...
        c_node->start_work = ipp_create_cmd_work();
        if (IS_ERR_OR_NULL(c_node->start_work)) {
                DRM_ERROR("failed to create start work.\n");
                goto err_clear;
        }
        c_node->stop_work = ipp_create_cmd_work();
        if (IS_ERR_OR_NULL(c_node->stop_work)) {
                DRM_ERROR("failed to create stop work.\n");
                goto err_free_start;
        }
        c_node->event_work = ipp_create_event_work();
        if (IS_ERR_OR_NULL(c_node->event_work)) {
                DRM_ERROR("failed to create event work.\n");
                goto err_free_stop;
        }

We also have it cropping up as a way to 'verify' function arguments
are correct:

int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
                          struct gpd_timing_data *td)
{
        if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
                return -EINVAL;

And then we have this beauty in USB code:

        if (!IS_ERR_OR_NULL(udc->transceiver))
                (void) otg_set_peripheral(udc->transceiver->otg, NULL);
        else
                pullup_disable(udc);
...
        seq_printf(s,
                "UDC rev %d.%d, fifo mode %d, gadget %s\n"
                "hmc %d, transceiver %s\n",
                tmp >> 4, tmp & 0xf,
                fifo_mode,
                udc->driver ? udc->driver->driver.name : "(none)",
                HMC,
                udc->transceiver
                        ? udc->transceiver->label
                        : (cpu_is_omap1710()
                                ? "external" : "(none)"));

If udc->transceiver were to be an error code, the above will segfault or
access memory at the top of memory space.

These are just a few of the issues I've picked out at random from grepping
the kernel source for IS_ERR_OR_NULL().  Yes, there's some valid use cases
but the above are all horrid, buggy or down right wrong, and I wouldn't be
at all surprised if that was all too common.

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Arnd Bergmann]

On Wednesday 09 January 2013, Russell King - ARM Linux wrote:
>         d = debugfs_create_dir("pm_debug", NULL);
>         if (IS_ERR_OR_NULL(d))
>                 return PTR_ERR(d);
> 
> Well, covered above.  NULL is success here.

This one is actually worse, because in case of debugfs_create_dir,
a negative error code is documented to mean "success": The debugfs
functions intentionally return ERR_PTR(-ENODEV) when debugfs is
disabled so that any code checking for NULL pretends it is a valid
pointer, but that code is only allowed to pass this pointer into
other debugfs functions that are also stubbed out and never
dereference it.

	Arnd

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Nicolas Pitre]

On Wed, 9 Jan 2013, Russell King - ARM Linux wrote:

> On Wed, Jan 09, 2013 at 10:44:14AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Jan 09, 2013 at 10:35:22AM +0000, Arnd Bergmann wrote:
> > > On Wednesday 09 January 2013, Alexandre Courbot wrote:
> > > > On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> > > > > introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> > > > > should not check for that.
> > > > 
> > > > Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,
> > > 
> > > Correct.
> > > 
> > > > may I ask why this is the case?
> > > 
> > > It's very hard to get right: either you are interested in the error code,
> > > and then you don't have one in some cases, or you don't care but have
> > > to check for it anyway. When you define a function, just make it clear
> > > what the expected return values are, either NULL for error or a negative
> > > ERR_PTR value, but not both.
> > 
> > Indeed, and any code which does this:
> > 
> > 	if (IS_ERR_OR_NULL(ptr))
> > 		return PTR_ERR(ptr);
> > 
> > is buggy because on NULL it returns 0, which is generally accepted as being
> > "success".
>

[ examples of broken code skipped ]

> These are just a few of the issues I've picked out at random from grepping
> the kernel source for IS_ERR_OR_NULL().  Yes, there's some valid use cases
> but the above are all horrid, buggy or down right wrong, and I wouldn't be
> at all surprised if that was all too common.

I do agree with Russell here.  Despite the original intentions behind 
IS_ERR_OR_NULL() which were certainly legitimate, the end result in 
practice is less reliable code with increased maintenance costs.  Unlike 
other convenience macros in the kernel, this one is giving a false sense 
of correctness with too many people falling in the trap of using it just 
because it is available.

I strongly think this macro should simply be removed from the source 
tree entirely and the code reverted to explicit tests against NULL when 
appropriate.


Nicolas

[PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Russell King - ARM Linux]

So, it seems there's some concensus building here, and it seems that
I've become the chosen victi^wvolunteer for this.  So, here's a patch.
It's missing a Guns-supplied-by: tag though.

From: Russell King <rmk+kernel@arm.linux.org.uk>
Subject: Mark IS_ERR_OR_NULL() deprecated

IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much
thought about it's effects.  Common errors include:
 1. checking the returned pointer for functions defined as only
    returning errno-pointer values, rather than using IS_ERR().
    This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return
     PTR_ERR(ptr);
 2. using it to check functions which only ever return NULL on error,
    thereby leading to another zero-error value return.
In the case of debugfs functions, these return errno-pointer values when
debugfs is configured out, which means code which blindly checks using 
IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for
something that's not implemented.

Therefore, let's schedule it for removal in a few releases.

Nicolas Pitre comments:
> I do agree with Russell here.  Despite the original intentions behind
> IS_ERR_OR_NULL() which were certainly legitimate, the end result in
> practice is less reliable code with increased maintenance costs.
> Unlike other convenience macros in the kernel, this one is giving a
> false sense of correctness with too many people falling in the trap
> of using it just because it is available.
> 
> I strongly think this macro should simply be removed from the source
> tree entirely and the code reverted to explicit tests against NULL
> when appropriate.

Suggested-by: David Howells <dhowells@redhat.com>
Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com>
Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
Ok, so I'm in the firing line for suggesting this, but it appears
several people wish this to happen.

I'm not intending to push this patch forwards _just_ yet: we need to
sort out the existing users _first_ to prevent the kernel turning into
one hell of a mess of warnings.

 include/linux/err.h |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index f2edce2..d5a85df 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -34,7 +34,22 @@ static inline long __must_check IS_ERR(const void *ptr)
 	return IS_ERR_VALUE((unsigned long)ptr);
 }
 
-static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
+/*
+ * IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much
+ * thought about it's effects.  Common errors include:
+ *  1. checking the returned pointer for functions defined as only returning
+ *     errno-pointer values, rather than using IS_ERR().
+ *     This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return PTR_ERR(ptr);
+ *  2. using it to check functions which only ever return NULL on error,
+ *     thereby leading to another zero-error value return.
+ * In the case of debugfs functions, these return errno-pointer values when
+ * debugfs is configured out, which means code which blindly checks using
+ * IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for
+ * something that's not implemented.
+ *
+ * Therefore, let's schedule it for removal in a few releases.
+ */
+static inline long __must_check __deprecated IS_ERR_OR_NULL(const void *ptr)
 {
 	return !ptr || IS_ERR_VALUE((unsigned long)ptr);
 }

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Grant Likely]

On Wed, Jan 9, 2013 at 3:04 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> So, it seems there's some concensus building here, and it seems that
> I've become the chosen victi^wvolunteer for this.  So, here's a patch.
> It's missing a Guns-supplied-by: tag though.
>
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Subject: Mark IS_ERR_OR_NULL() deprecated
>
> IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much
> thought about it's effects.  Common errors include:
>  1. checking the returned pointer for functions defined as only
>     returning errno-pointer values, rather than using IS_ERR().
>     This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return
>      PTR_ERR(ptr);
>  2. using it to check functions which only ever return NULL on error,
>     thereby leading to another zero-error value return.
> In the case of debugfs functions, these return errno-pointer values when
> debugfs is configured out, which means code which blindly checks using
> IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for
> something that's not implemented.
>
> Therefore, let's schedule it for removal in a few releases.
>
> Nicolas Pitre comments:
>> I do agree with Russell here.  Despite the original intentions behind
>> IS_ERR_OR_NULL() which were certainly legitimate, the end result in
>> practice is less reliable code with increased maintenance costs.
>> Unlike other convenience macros in the kernel, this one is giving a
>> false sense of correctness with too many people falling in the trap
>> of using it just because it is available.
>>
>> I strongly think this macro should simply be removed from the source
>> tree entirely and the code reverted to explicit tests against NULL
>> when appropriate.
>
> Suggested-by: David Howells <dhowells@redhat.com>
> Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com>
> Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

I fully agree with doing this. While I'm not a fan of the PTR_ERR
pattern, this does (hopefully) make users think just a little bit more
about it.

> ---
> Ok, so I'm in the firing line for suggesting this, but it appears
> several people wish this to happen.
>
> I'm not intending to push this patch forwards _just_ yet: we need to
> sort out the existing users _first_ to prevent the kernel turning into
> one hell of a mess of warnings.

I currently see 355 users. That's a lot, but not inconceivable for an
auditing effort for pulling them out instead of scheduling for future
removal.

g.

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Arnd Bergmann]

On Wednesday 09 January 2013, Grant Likely wrote:
> > Suggested-by: David Howells <dhowells@redhat.com>
> > Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com>
> > Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk>
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Nicolas Pitre]

On Wed, 9 Jan 2013, Russell King - ARM Linux wrote:

> So, it seems there's some concensus building here, and it seems that
> I've become the chosen victi^wvolunteer for this.  So, here's a patch.
> It's missing a Guns-supplied-by: tag though.

Guns-supplied-by: NRA (obviously)

> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Subject: Mark IS_ERR_OR_NULL() deprecated
> 
> IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much
> thought about it's effects.  Common errors include:
>  1. checking the returned pointer for functions defined as only
>     returning errno-pointer values, rather than using IS_ERR().
>     This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return
>      PTR_ERR(ptr);
>  2. using it to check functions which only ever return NULL on error,
>     thereby leading to another zero-error value return.
> In the case of debugfs functions, these return errno-pointer values when
> debugfs is configured out, which means code which blindly checks using 
> IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for
> something that's not implemented.
> 
> Therefore, let's schedule it for removal in a few releases.
> 
> Nicolas Pitre comments:
> > I do agree with Russell here.  Despite the original intentions behind
> > IS_ERR_OR_NULL() which were certainly legitimate, the end result in
> > practice is less reliable code with increased maintenance costs.
> > Unlike other convenience macros in the kernel, this one is giving a
> > false sense of correctness with too many people falling in the trap
> > of using it just because it is available.
> > 
> > I strongly think this macro should simply be removed from the source
> > tree entirely and the code reverted to explicit tests against NULL
> > when appropriate.
> 
> Suggested-by: David Howells <dhowells@redhat.com>
> Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com>
> Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Nicolas Pitre <nico@linaro.org>

Anyone with good coccinelle skills around to deal with the users?


> ---
> Ok, so I'm in the firing line for suggesting this, but it appears
> several people wish this to happen.
> 
> I'm not intending to push this patch forwards _just_ yet: we need to
> sort out the existing users _first_ to prevent the kernel turning into
> one hell of a mess of warnings.
> 
>  include/linux/err.h |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index f2edce2..d5a85df 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -34,7 +34,22 @@ static inline long __must_check IS_ERR(const void *ptr)
>  	return IS_ERR_VALUE((unsigned long)ptr);
>  }
>  
> -static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
> +/*
> + * IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much
> + * thought about it's effects.  Common errors include:
> + *  1. checking the returned pointer for functions defined as only returning
> + *     errno-pointer values, rather than using IS_ERR().
> + *     This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return PTR_ERR(ptr);
> + *  2. using it to check functions which only ever return NULL on error,
> + *     thereby leading to another zero-error value return.
> + * In the case of debugfs functions, these return errno-pointer values when
> + * debugfs is configured out, which means code which blindly checks using
> + * IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for
> + * something that's not implemented.
> + *
> + * Therefore, let's schedule it for removal in a few releases.
> + */
> +static inline long __must_check __deprecated IS_ERR_OR_NULL(const void *ptr)
>  {
>  	return !ptr || IS_ERR_VALUE((unsigned long)ptr);
>  }
> 

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote:
> Anyone with good coccinelle skills around to deal with the users?

I'm not sure that's a solution.

For example:

        err = gpio_request(en_vdd_1v05, "EN_VDD_1V05");
        if (err) {
                pr_err("%s: gpio_request failed: %d\n", __func__, err);   
                return err;   
        }
...
        regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
        if (IS_ERR_OR_NULL(regulator)) {
                pr_err("%s: regulator_get failed: %d\n", __func__,
                       (int)PTR_ERR(regulator));
                goto err_reg;
        }

        regulator_enable(regulator);

        err = tegra_pcie_init(true, true);
...
err_reg:
        gpio_free(en_vdd_1v05);
                
        return err;
}

Now, regulator_get() returns error-pointers for real errors when it's
configured in.  When regulator support is not configured, it returns
NULL.

So, one solution here would be:

	if (IS_ERR(regulator)) {
		err = PTR_ERR(regulator);
		pr_err("%s: regulator_get failed: %d\n", __func__, err);
		goto err_reg;
	}

but leaves us with the question: is it safe to call tegra_pcie_init()
without regulator support?

The problem there is that "fixing" this causes a behavioural change in
the code, and we don't know what effect that change may have.

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Nicolas Pitre]

On Wed, 9 Jan 2013, Russell King - ARM Linux wrote:

> On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote:
> > Anyone with good coccinelle skills around to deal with the users?
> 
> I'm not sure that's a solution.

Well, I was thinking that coccinelle could handle the majority of the 
354 users when the "fix" is obvious enough to be automated.

That said, if we want people to fix their code, it is probably necessary 
to merge your patch right away so the warnings are actually being seen, 
and revert it right before the final v3.8 release if the remaining 
warnings are still too numerous.  Repeat with next cycle.

> For example:
> 
>         err = gpio_request(en_vdd_1v05, "EN_VDD_1V05");
>         if (err) {
>                 pr_err("%s: gpio_request failed: %d\n", __func__, err);   
>                 return err;   
>         }
> ...
>         regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
>         if (IS_ERR_OR_NULL(regulator)) {
>                 pr_err("%s: regulator_get failed: %d\n", __func__,
>                        (int)PTR_ERR(regulator));
>                 goto err_reg;
>         }
> 
>         regulator_enable(regulator);
> 
>         err = tegra_pcie_init(true, true);
> ...
> err_reg:
>         gpio_free(en_vdd_1v05);
>                 
>         return err;
> }
> 
> Now, regulator_get() returns error-pointers for real errors when it's
> configured in.  When regulator support is not configured, it returns
> NULL.
> 
> So, one solution here would be:
> 
> 	if (IS_ERR(regulator)) {
> 		err = PTR_ERR(regulator);
> 		pr_err("%s: regulator_get failed: %d\n", __func__, err);
> 		goto err_reg;
> 	}
> 
> but leaves us with the question: is it safe to call tegra_pcie_init()
> without regulator support?

The best approach is to assume it is not, but unlike the current code, 
this should be fixed by returning an appropriate error code instead of 0.


Nicolas

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 11:09:23AM -0500, Nicolas Pitre wrote:
> On Wed, 9 Jan 2013, Russell King - ARM Linux wrote:
> 
> > On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote:
> > > Anyone with good coccinelle skills around to deal with the users?
> > 
> > I'm not sure that's a solution.
> 
> Well, I was thinking that coccinelle could handle the majority of the 
> 354 users when the "fix" is obvious enough to be automated.
> 
> That said, if we want people to fix their code, it is probably necessary 
> to merge your patch right away so the warnings are actually being seen, 
> and revert it right before the final v3.8 release if the remaining 
> warnings are still too numerous.  Repeat with next cycle.

Well, this is what I currently have for arch/arm thus far, and ooh look,
we save some lines of code too.

 arch/arm/mach-integrator/integrator_ap.c |    4 +---
 arch/arm/mach-integrator/integrator_cp.c |    5 +----
 arch/arm/mach-omap2/omap_device.c        |   11 ++++-------
 arch/arm/mach-omap2/pm-debug.c           |    6 +++---
 arch/arm/mach-omap2/powerdomain.c        |    2 +-
 arch/arm/mach-tegra/board-harmony-pcie.c |    6 +++---
 arch/arm/mach-tegra/tegra2_emc.c         |    2 +-
 arch/arm/mach-ux500/cpu.c                |    3 +--
 arch/arm/plat-omap/dmtimer.c             |    4 ++--
 9 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-integrator/integrator_ap.c b/arch/arm/mach-integrator/integrator_ap.c
index 11e2a41..251f54f 100644
--- a/arch/arm/mach-integrator/integrator_ap.c
+++ b/arch/arm/mach-integrator/integrator_ap.c
@@ -547,9 +547,7 @@ static void __init ap_init_of(void)
 	}
 
 	parent = soc_device_to_device(soc_dev);
-
-	if (!IS_ERR_OR_NULL(parent))
-		integrator_init_sysfs(parent, ap_sc_id);
+	integrator_init_sysfs(parent, ap_sc_id);
 
 	of_platform_populate(root, of_default_bus_match_table,
 			ap_auxdata_lookup, parent);
diff --git a/arch/arm/mach-integrator/integrator_cp.c b/arch/arm/mach-integrator/integrator_cp.c
index 7322838..3fadadf 100644
--- a/arch/arm/mach-integrator/integrator_cp.c
+++ b/arch/arm/mach-integrator/integrator_cp.c
@@ -371,10 +371,7 @@ static void __init intcp_init_of(void)
 	}
 
 	parent = soc_device_to_device(soc_dev);
-
-	if (!IS_ERR_OR_NULL(parent))
-		integrator_init_sysfs(parent, intcp_sc_id);
-
+	integrator_init_sysfs(parent, intcp_sc_id);
 	of_platform_populate(root, of_default_bus_match_table,
 			intcp_auxdata_lookup, parent);
 }
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index e065daa..fabb32d 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -1157,20 +1157,17 @@ struct device *omap_device_get_by_hwmod_name(const char *oh_name)
 	}
 
 	oh = omap_hwmod_lookup(oh_name);
-	if (IS_ERR_OR_NULL(oh)) {
+	if (!oh) {
 		WARN(1, "%s: no hwmod for %s\n", __func__,
 			oh_name);
-		return ERR_PTR(oh ? PTR_ERR(oh) : -ENODEV);
+		return -ENODEV;
 	}
-	if (IS_ERR_OR_NULL(oh->od)) {
+	if (!oh->od) {
 		WARN(1, "%s: no omap_device for %s\n", __func__,
 			oh_name);
-		return ERR_PTR(oh->od ? PTR_ERR(oh->od) : -ENODEV);
+		return -ENODEV;
 	}
 
-	if (IS_ERR_OR_NULL(oh->od->pdev))
-		return ERR_PTR(oh->od->pdev ? PTR_ERR(oh->od->pdev) : -ENODEV);
-
 	return &oh->od->pdev->dev;
 }
 EXPORT_SYMBOL(omap_device_get_by_hwmod_name);
diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index e2c291f..548547e 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -219,7 +219,7 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *dir)
 		return 0;
 
 	d = debugfs_create_dir(pwrdm->name, (struct dentry *)dir);
-	if (!(IS_ERR_OR_NULL(d)))
+	if (d)
 		(void) debugfs_create_file("suspend", S_IRUGO|S_IWUSR, d,
 			(void *)pwrdm, &pwrdm_suspend_fops);
 
@@ -263,8 +263,8 @@ static int __init pm_dbg_init(void)
 		return 0;
 
 	d = debugfs_create_dir("pm_debug", NULL);
-	if (IS_ERR_OR_NULL(d))
-		return PTR_ERR(d);
+	if (!d)
+		return -EINVAL;
 
 	(void) debugfs_create_file("count", S_IRUGO,
 		d, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index dea62a9..36a6918 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1054,7 +1054,7 @@ bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm)
 {
 	int i;
 
-	if (IS_ERR_OR_NULL(pwrdm)) {
+	if (!pwrdm) {
 		pr_debug("powerdomain: %s: invalid powerdomain pointer\n",
 			 __func__);
 		return 1;
diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
index 3cdc1bb..6d29e6a 100644
--- a/arch/arm/mach-tegra/board-harmony-pcie.c
+++ b/arch/arm/mach-tegra/board-harmony-pcie.c
@@ -56,9 +56,9 @@ int __init harmony_pcie_init(void)
 	gpio_direction_output(en_vdd_1v05, 1);
 
 	regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
-	if (IS_ERR_OR_NULL(regulator)) {
-		pr_err("%s: regulator_get failed: %d\n", __func__,
-		       (int)PTR_ERR(regulator));
+	if (IS_ERR(regulator)) {
+		err = PTR_ERR(regulator);
+		pr_err("%s: regulator_get failed: %d\n", __func__, err);
 		goto err_reg;
 	}
 
diff --git a/arch/arm/mach-tegra/tegra2_emc.c b/arch/arm/mach-tegra/tegra2_emc.c
index 837c7b9..844c8d2 100644
--- a/arch/arm/mach-tegra/tegra2_emc.c
+++ b/arch/arm/mach-tegra/tegra2_emc.c
@@ -276,7 +276,7 @@ static struct tegra_emc_pdata __devinit *tegra_emc_fill_pdata(struct platform_de
 	int i;
 
 	WARN_ON(pdev->dev.platform_data);
-	BUG_ON(IS_ERR_OR_NULL(c));
+	BUG_ON(IS_ERR(c));
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	pdata->tables = devm_kzalloc(&pdev->dev, sizeof(*pdata->tables),
diff --git a/arch/arm/mach-ux500/cpu.c b/arch/arm/mach-ux500/cpu.c
index 721e7b4..e31c2b4 100644
--- a/arch/arm/mach-ux500/cpu.c
+++ b/arch/arm/mach-ux500/cpu.c
@@ -151,8 +151,7 @@ struct device * __init ux500_soc_device_init(const char *soc_id)
 	}
 
 	parent = soc_device_to_device(soc_dev);
-	if (!IS_ERR_OR_NULL(parent))
-		device_create_file(parent, &ux500_soc_attr);
+	device_create_file(parent, &ux500_soc_attr);
 
 	return parent;
 }
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..c654bb8 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -140,7 +140,7 @@ static int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 	 */
 	if (!(timer->capability & OMAP_TIMER_NEEDS_RESET)) {
 		timer->fclk = clk_get(&timer->pdev->dev, "fck");
-		if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer->fclk))) {
+		if (WARN_ON_ONCE(IS_ERR(timer->fclk))) {
 			timer->fclk = NULL;
 			dev_err(&timer->pdev->dev, ": No fclk handle.\n");
 			return -EINVAL;
@@ -500,7 +500,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	}
 
 	parent = clk_get(&timer->pdev->dev, parent_name);
-	if (IS_ERR_OR_NULL(parent)) {
+	if (IS_ERR(parent)) {
 		pr_err("%s: %s not found\n", __func__, parent_name);
 		return -EINVAL;
 	}

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 04:21:45PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 09, 2013 at 11:09:23AM -0500, Nicolas Pitre wrote:
> > On Wed, 9 Jan 2013, Russell King - ARM Linux wrote:
> > 
> > > On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote:
> > > > Anyone with good coccinelle skills around to deal with the users?
> > > 
> > > I'm not sure that's a solution.
> > 
> > Well, I was thinking that coccinelle could handle the majority of the 
> > 354 users when the "fix" is obvious enough to be automated.
> > 
> > That said, if we want people to fix their code, it is probably necessary 
> > to merge your patch right away so the warnings are actually being seen, 
> > and revert it right before the final v3.8 release if the remaining 
> > warnings are still too numerous.  Repeat with next cycle.
> 
> Well, this is what I currently have for arch/arm thus far, and ooh look,
> we save some lines of code too.

Actually, the OMAP dmtimer.c code needs a few more fixes while we're
reviewing this stuff...  This latest patch leaves three files in
arch/arm still using IS_ERR_OR_NULL() as they do seem to want to
legitimately check for error pointers _and_ NULL.

I'm restricting the patch below to just those cases where it's wrong
in arch/arm.

The omap_device.c changes probably also deserve some explanation.  As
far as I can see, if we have an 'od' then 'od->pdev' _must_ already
be valid (pdev is passed into omap_device_alloc() which creates the
'od' - and the passed pdev better be checked _before_ it's passed in.)
It also appears that oh->od will _never_ be an error pointer value -
apart from tracing the paths creating that, there is some evidence for
this with tests elsewhere just for NULL here.  And lastly
omap_hwmod_lookup() is documented to _only_ return NULL on error, and
review of it confirms that.

Other stuff.
- soc_device_register() returns error-pointer values only.  Nothing
  allows it to return NULL.
- soc_device_to_device() merely translates the type of the pointer
  and has no failure value - plus code paths make use of this pointer
  outside of the IS_ERR_OR_NULL() check, so it better be valid.
- debugfs interfaces return NULL on error.  Everything else should be
  considered valid.  (Yes, it returns ERR_PTR(-ENODEV) when it's
  unconfigured, but that's to make it _look_ like a valid pointer to
  prevent these paths from erroring out unnecessarily.)
- pwrdm_can_ever_lose_context() is only ever calls from the GPIO code,
  and the source of the passed pointer is defined to only ever be a
  valid pointer or NULL.
- regulator API, errors are valid error pointers.  NULL means regulator
  API not available.  Whether it's right that board-harmony-pcie.c
  should not proceed without the regulator API is debatable, but the
  current code is just fscked.
- clk_get_sys/clk_get - I've been over that many times, I don't need to
  restate it yet again.

 arch/arm/mach-integrator/integrator_ap.c |    6 ++----
 arch/arm/mach-integrator/integrator_cp.c |    7 ++-----
 arch/arm/mach-omap2/omap_device.c        |   11 ++++-------
 arch/arm/mach-omap2/pm-debug.c           |    6 +++---
 arch/arm/mach-omap2/powerdomain.c        |    2 +-
 arch/arm/mach-tegra/board-harmony-pcie.c |    6 +++---
 arch/arm/mach-tegra/tegra2_emc.c         |    2 +-
 arch/arm/mach-ux500/cpu.c                |    5 ++---
 arch/arm/plat-omap/dmtimer.c             |   10 +++++-----
 9 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/arch/arm/mach-integrator/integrator_ap.c b/arch/arm/mach-integrator/integrator_ap.c
index 11e2a41..61225e1 100644
--- a/arch/arm/mach-integrator/integrator_ap.c
+++ b/arch/arm/mach-integrator/integrator_ap.c
@@ -540,16 +540,14 @@ static void __init ap_init_of(void)
 					   'A' + (ap_sc_id & 0x0f));
 
 	soc_dev = soc_device_register(soc_dev_attr);
-	if (IS_ERR_OR_NULL(soc_dev)) {
+	if (IS_ERR(soc_dev)) {
 		kfree(soc_dev_attr->revision);
 		kfree(soc_dev_attr);
 		return;
 	}
 
 	parent = soc_device_to_device(soc_dev);
-
-	if (!IS_ERR_OR_NULL(parent))
-		integrator_init_sysfs(parent, ap_sc_id);
+	integrator_init_sysfs(parent, ap_sc_id);
 
 	of_platform_populate(root, of_default_bus_match_table,
 			ap_auxdata_lookup, parent);
diff --git a/arch/arm/mach-integrator/integrator_cp.c b/arch/arm/mach-integrator/integrator_cp.c
index 7322838..6016189 100644
--- a/arch/arm/mach-integrator/integrator_cp.c
+++ b/arch/arm/mach-integrator/integrator_cp.c
@@ -364,17 +364,14 @@ static void __init intcp_init_of(void)
 					   'A' + (intcp_sc_id & 0x0f));
 
 	soc_dev = soc_device_register(soc_dev_attr);
-	if (IS_ERR_OR_NULL(soc_dev)) {
+	if (IS_ERR(soc_dev)) {
 		kfree(soc_dev_attr->revision);
 		kfree(soc_dev_attr);
 		return;
 	}
 
 	parent = soc_device_to_device(soc_dev);
-
-	if (!IS_ERR_OR_NULL(parent))
-		integrator_init_sysfs(parent, intcp_sc_id);
-
+	integrator_init_sysfs(parent, intcp_sc_id);
 	of_platform_populate(root, of_default_bus_match_table,
 			intcp_auxdata_lookup, parent);
 }
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index e065daa..fabb32d 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -1157,20 +1157,17 @@ struct device *omap_device_get_by_hwmod_name(const char *oh_name)
 	}
 
 	oh = omap_hwmod_lookup(oh_name);
-	if (IS_ERR_OR_NULL(oh)) {
+	if (!oh) {
 		WARN(1, "%s: no hwmod for %s\n", __func__,
 			oh_name);
-		return ERR_PTR(oh ? PTR_ERR(oh) : -ENODEV);
+		return -ENODEV;
 	}
-	if (IS_ERR_OR_NULL(oh->od)) {
+	if (!oh->od) {
 		WARN(1, "%s: no omap_device for %s\n", __func__,
 			oh_name);
-		return ERR_PTR(oh->od ? PTR_ERR(oh->od) : -ENODEV);
+		return -ENODEV;
 	}
 
-	if (IS_ERR_OR_NULL(oh->od->pdev))
-		return ERR_PTR(oh->od->pdev ? PTR_ERR(oh->od->pdev) : -ENODEV);
-
 	return &oh->od->pdev->dev;
 }
 EXPORT_SYMBOL(omap_device_get_by_hwmod_name);
diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index e2c291f..548547e 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -219,7 +219,7 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *dir)
 		return 0;
 
 	d = debugfs_create_dir(pwrdm->name, (struct dentry *)dir);
-	if (!(IS_ERR_OR_NULL(d)))
+	if (d)
 		(void) debugfs_create_file("suspend", S_IRUGO|S_IWUSR, d,
 			(void *)pwrdm, &pwrdm_suspend_fops);
 
@@ -263,8 +263,8 @@ static int __init pm_dbg_init(void)
 		return 0;
 
 	d = debugfs_create_dir("pm_debug", NULL);
-	if (IS_ERR_OR_NULL(d))
-		return PTR_ERR(d);
+	if (!d)
+		return -EINVAL;
 
 	(void) debugfs_create_file("count", S_IRUGO,
 		d, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index dea62a9..36a6918 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1054,7 +1054,7 @@ bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm)
 {
 	int i;
 
-	if (IS_ERR_OR_NULL(pwrdm)) {
+	if (!pwrdm) {
 		pr_debug("powerdomain: %s: invalid powerdomain pointer\n",
 			 __func__);
 		return 1;
diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
index 3cdc1bb..6d29e6a 100644
--- a/arch/arm/mach-tegra/board-harmony-pcie.c
+++ b/arch/arm/mach-tegra/board-harmony-pcie.c
@@ -56,9 +56,9 @@ int __init harmony_pcie_init(void)
 	gpio_direction_output(en_vdd_1v05, 1);
 
 	regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
-	if (IS_ERR_OR_NULL(regulator)) {
-		pr_err("%s: regulator_get failed: %d\n", __func__,
-		       (int)PTR_ERR(regulator));
+	if (IS_ERR(regulator)) {
+		err = PTR_ERR(regulator);
+		pr_err("%s: regulator_get failed: %d\n", __func__, err);
 		goto err_reg;
 	}
 
diff --git a/arch/arm/mach-tegra/tegra2_emc.c b/arch/arm/mach-tegra/tegra2_emc.c
index 837c7b9..844c8d2 100644
--- a/arch/arm/mach-tegra/tegra2_emc.c
+++ b/arch/arm/mach-tegra/tegra2_emc.c
@@ -276,7 +276,7 @@ static struct tegra_emc_pdata __devinit *tegra_emc_fill_pdata(struct platform_de
 	int i;
 
 	WARN_ON(pdev->dev.platform_data);
-	BUG_ON(IS_ERR_OR_NULL(c));
+	BUG_ON(IS_ERR(c));
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	pdata->tables = devm_kzalloc(&pdev->dev, sizeof(*pdata->tables),
diff --git a/arch/arm/mach-ux500/cpu.c b/arch/arm/mach-ux500/cpu.c
index 721e7b4..a480266 100644
--- a/arch/arm/mach-ux500/cpu.c
+++ b/arch/arm/mach-ux500/cpu.c
@@ -145,14 +145,13 @@ struct device * __init ux500_soc_device_init(const char *soc_id)
 	soc_info_populate(soc_dev_attr, soc_id);
 
 	soc_dev = soc_device_register(soc_dev_attr);
-	if (IS_ERR_OR_NULL(soc_dev)) {
+	if (IS_ERR(soc_dev)) {
 	        kfree(soc_dev_attr);
 		return NULL;
 	}
 
 	parent = soc_device_to_device(soc_dev);
-	if (!IS_ERR_OR_NULL(parent))
-		device_create_file(parent, &ux500_soc_attr);
+	device_create_file(parent, &ux500_soc_attr);
 
 	return parent;
 }
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..d0fe4c3a 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -140,8 +140,7 @@ static int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 	 */
 	if (!(timer->capability & OMAP_TIMER_NEEDS_RESET)) {
 		timer->fclk = clk_get(&timer->pdev->dev, "fck");
-		if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer->fclk))) {
-			timer->fclk = NULL;
+		if (WARN_ON_ONCE(IS_ERR(timer->fclk))) {
 			dev_err(&timer->pdev->dev, ": No fclk handle.\n");
 			return -EINVAL;
 		}
@@ -373,7 +372,7 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask);
 
 struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
 {
-	if (timer)
+	if (timer && !IS_ERR(timer->fclk))
 		return timer->fclk;
 	return NULL;
 }
@@ -482,7 +481,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	if (pdata && pdata->set_timer_src)
 		return pdata->set_timer_src(timer->pdev, source);
 
-	if (!timer->fclk)
+	if (IS_ERR(timer->fclk))
 		return -EINVAL;
 
 	switch (source) {
@@ -500,7 +499,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	}
 
 	parent = clk_get(&timer->pdev->dev, parent_name);
-	if (IS_ERR_OR_NULL(parent)) {
+	if (IS_ERR(parent)) {
 		pr_err("%s: %s not found\n", __func__, parent_name);
 		return -EINVAL;
 	}
@@ -808,6 +807,7 @@ static int __devinit omap_dm_timer_probe(struct platform_device *pdev)
 		return  -ENOMEM;
 	}
 
+	timer->fclk = ERR_PTR(-ENODEV);
 	timer->io_base = devm_request_and_ioremap(dev, mem);
 	if (!timer->io_base) {
 		dev_err(dev, "%s: region already claimed.\n", __func__);

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [130109 09:15]:
> On Wed, Jan 09, 2013 at 04:21:45PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Jan 09, 2013 at 11:09:23AM -0500, Nicolas Pitre wrote:
> > > On Wed, 9 Jan 2013, Russell King - ARM Linux wrote:
> > > 
> > > > On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote:
> > > > > Anyone with good coccinelle skills around to deal with the users?
> > > > 
> > > > I'm not sure that's a solution.
> > > 
> > > Well, I was thinking that coccinelle could handle the majority of the 
> > > 354 users when the "fix" is obvious enough to be automated.
> > > 
> > > That said, if we want people to fix their code, it is probably necessary 
> > > to merge your patch right away so the warnings are actually being seen, 
> > > and revert it right before the final v3.8 release if the remaining 
> > > warnings are still too numerous.  Repeat with next cycle.
> > 
> > Well, this is what I currently have for arch/arm thus far, and ooh look,
> > we save some lines of code too.
> 
> Actually, the OMAP dmtimer.c code needs a few more fixes while we're
> reviewing this stuff...  This latest patch leaves three files in
> arch/arm still using IS_ERR_OR_NULL() as they do seem to want to
> legitimately check for error pointers _and_ NULL.
> 
> I'm restricting the patch below to just those cases where it's wrong
> in arch/arm.
> 
> The omap_device.c changes probably also deserve some explanation.  As
> far as I can see, if we have an 'od' then 'od->pdev' _must_ already
> be valid (pdev is passed into omap_device_alloc() which creates the
> 'od' - and the passed pdev better be checked _before_ it's passed in.)
> It also appears that oh->od will _never_ be an error pointer value -
> apart from tracing the paths creating that, there is some evidence for
> this with tests elsewhere just for NULL here.  And lastly
> omap_hwmod_lookup() is documented to _only_ return NULL on error, and
> review of it confirms that.

Looks correct to me.

Regards,

Tony

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Alex Courbot]

On Wednesday 09 January 2013 18:46:12 Arnd Bergmann wrote:
> > The question is, do we want to totally get rid of the integer
> > namespace? That would be the ultimate step, but would require another
> > way to identify GPIOs (controller_device:offset might be such a way),
> > and also to reorganize sysfs nodes. Wouldn't that be considered
> > breaking user-space? 'cause we all know what happens to those who
> > break user-space.
> 
> 
> The user interface could eventually be the only part of the kernel that
> uses the numbers, but you are right that we cannot change that.

That's sad, as it makes it necessary to maintain the global integer namespace 
(assigning a base GPIO to each controller and making sure the controllers 
ranges do no overlap) even if it is not used internally anymore. We could make 
global numbers assignment transparent, but that would potentially change the 
GPIO numbers in user-space and cause another incompatibility.

> > With the current code, a driver should depend on gpiolib being
> > compiled if it uses the new interface. It is not even declared if
> > gpiolib is not used.
> > 
> > Given that both interfaces are quite close, one could imagine having a
> > gpiod wrapper around the integer namespace (the "opaque descriptors"
> > would then just be casted integers). This way drivers would only need
> > to depend on GENERIC_GPIO. It's a little bit weird to have gpiod
> > wrapping around gpio in one case and the opposite in another though -
> > I'd rather have these platforms convert to GPIO descriptors internally
> > (or even better, to gpiolib), but this is probably asking too much.
> 
> 
> I think it would be reasonable to force everybody to use gpiolib,
> that's much easier than converting everyone to the descriptor based
> interface.
> 
> 
> > I do not know all the details of gpiolib's history, but why would
> > anyone want to implement the generic gpio interface and not use
> > gpiolib anyways?
> 
> 
> Only legacy users did this. Initially there was only the header file,
> with the API declared but several different implementations of it.
> gpiolib was introduced later to reduce code duplication and allow having
> multiple implementations in the same kernel.

Does the following sound reasonable?
1) Make sure every target that uses GENERIC_GPIO also implements its drivers 
using gpiolib, convert the (hopefully) few ones that don't to use gpiolib
2) Make GENERIC_GPIO require GPIOLIB or just merge both options into a single 
one
3) Turn gpio into a full subsystem (like pinctrl)

This should make things less blurry and easier to maintain (less header files, 
only one interface, etc.) GPIO controllers would also be better integrated 
into the driver model.

> > > If we are adding a new way to deal with GPIOs, would it make sense to
> > > have that more closely integrated into pinctrl in one form or another?
> > > My feeling is that there is already a significant overlap between the
> > > two, and while the addition of the gpiod_* functions doesn't
> > > necessarily
> > > make this worse, it could be a chance to improve the current situation
> > > to make the interface more consistent with pinctrl.
> > 
> > 
> > That may be a chance to introduce deeper changes indeed - what do you
> > have in mind exactly?
> 
> 
> I don't know enough about pinctrl to have a specific idea yet, but maybe
> someone else has ideas.

I had a deeper look at pinctrl, and indeed I can see the connection between 
the two. There already interfaces to link GPIO ranges to pin ranges and have 
GPIO drivers switch the pin to the correct state when a GPIO is requested 
(this, btw, should also be updated to not use global GPIO numbers at some 
point). Maybe some tighter integration that I just don't see yet can be done 
too.

> Regarding the integration of pinctrl with gpio,
> I was thinking in the past that we could make pinctrl provide everything
> that gpiolib does, and have a generic gpiolib driver on top of pinctrl
> so that platforms don't need to implement both interfaces but only need
> to provide a pure pinctrl driver. Not sure if this makes any sense.

That would work if all GPIOs were connected to a ball, but how about GPIO 
expanders that are external to the chip? They have no use for pinctrl AFAICT. 
On the other hand, maybe we can have one pinctrl-gpio driver for those chips 
where pinctrl alone can emulate all the functionality of a GPIO controller. 
Maybe such a driver exists already?

But in general, I agree pinctrl should be a source of inspiration for how to 
design GPIO. In particular, having a per-chip integer namespace instead of a 
single global one is definitely something to take (and that's how things work 
in the DT already).

Alex.

Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

On Wed, Jan 09, 2013 at 11:10:55AM +0000, Russell King - ARM Linux wrote:
[...]
>         err = gpio_request(en_vdd_1v05, "EN_VDD_1V05");
>         if (err) {
>                 pr_err("%s: gpio_request failed: %d\n", __func__, err);
>                 return err;
>         }
> 
>         gpio_direction_output(en_vdd_1v05, 1);
> 
>         regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
>         if (IS_ERR_OR_NULL(regulator)) {
>                 pr_err("%s: regulator_get failed: %d\n", __func__,
>                        (int)PTR_ERR(regulator));
>                 goto err_reg;
>         }
> ...
> err_reg:
>         gpio_free(en_vdd_1v05);
> 
>         return err;
> }
> 
> So 'err' here is never set.  when IS_ERR_OR_NULL evaluates true.
> Setting 'err' to PTR_ERR(regulator) is not correct because a NULL return
> sets 'err' to zero.

FWIW, this one is fixed in the Tegra PCIe series I posted yesterday.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Arnd Bergmann]

On Thursday 10 January 2013, Alex Courbot wrote:
> On Wednesday 09 January 2013 18:46:12 Arnd Bergmann wrote:
> > 
> > Only legacy users did this. Initially there was only the header file,
> > with the API declared but several different implementations of it.
> > gpiolib was introduced later to reduce code duplication and allow having
> > multiple implementations in the same kernel.
> 
> Does the following sound reasonable?
> 1) Make sure every target that uses GENERIC_GPIO also implements its drivers 
> using gpiolib, convert the (hopefully) few ones that don't to use gpiolib
> 2) Make GENERIC_GPIO require GPIOLIB or just merge both options into a single 
> one
> 3) Turn gpio into a full subsystem (like pinctrl)
> 
> This should make things less blurry and easier to maintain (less header files, 
> only one interface, etc.) GPIO controllers would also be better integrated 
> into the driver model.

Yes, I think that would be good.

I've tried to find platforms that don't yet use GPIOLIB and fortunately
there are very few left:

I found two that provide the generic gpio interfaces when gpiolib
is disabled, but use gpiolib otherwise for the same hardware,
arch/m68k/include/asm/mcfgpio.h and arch/blackfin/include/asm/gpio.h.
I would assume that we can simply remove the non-gpiolib shortcut
here at cost of a small overhead.

Then there are a bunch that use gpiolib but have a nontrivial
implementation of gpio_get_value and other functions. I'm not sure
if these are a problematic with your code.

> > Regarding the integration of pinctrl with gpio,
> > I was thinking in the past that we could make pinctrl provide everything
> > that gpiolib does, and have a generic gpiolib driver on top of pinctrl
> > so that platforms don't need to implement both interfaces but only need
> > to provide a pure pinctrl driver. Not sure if this makes any sense.
> 
> That would work if all GPIOs were connected to a ball, but how about GPIO 
> expanders that are external to the chip? They have no use for pinctrl AFAICT. 
> On the other hand, maybe we can have one pinctrl-gpio driver for those chips 
> where pinctrl alone can emulate all the functionality of a GPIO controller. 
> Maybe such a driver exists already?

I don't think we have that yet, but it would be another option: rather
than putting a generic gpiolib driver on top of pinctrl, we could have
pinctrl support for all gpios that go through gpiolib, and move device
drivers over to use pinctrl as the way to manage gpios rather than the
classic gpio drivers. That would be a larger change though, and require
that we pull in the pinctrl subsystem on a lot of machines that don't
need it today.

	Arnd

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Alex Courbot]

On 01/10/2013 07:08 PM, Arnd Bergmann wrote:
> I've tried to find platforms that don't yet use GPIOLIB and fortunately
> there are very few left:
>
> I found two that provide the generic gpio interfaces when gpiolib
> is disabled, but use gpiolib otherwise for the same hardware,
> arch/m68k/include/asm/mcfgpio.h and arch/blackfin/include/asm/gpio.h.
> I would assume that we can simply remove the non-gpiolib shortcut
> here at cost of a small overhead.

I performed a search on my side too (checking configurations options 
which select GENERIC_GPIO but not ARCH_REQUIRE_GPIOLIB) and found the 
same list. This takes some time btw - many platforms use this combo to 
make GPIO support optional. Can I ask how you figured out these two archs?

I'd assume the overhead induced by forcibly compiling GPIOlib is 
neglectable, but let's make sure this is ok indeed - Mike, Geert, would 
you mind if selecting GENERIC_GPIO enforced GPIOlib to be compiled in, 
effectively making it mandatory to implement the generic GPIO interface 
using GPIOlib? Both your implementations support GPIOlib already, but 
can also work without it, and that would make that possibility obsolete.

> Then there are a bunch that use gpiolib but have a nontrivial
> implementation of gpio_get_value and other functions. I'm not sure
> if these are a problematic with your code.

AFAICT these all implement an inline path that bypasses GPIOlib when the 
GPIO number is known at compile time, for faster bitbanging I presume. 
It should be totally harmless to keep them. Unfortunately, I don't think 
it would be possible to have a similar trick using handlers.

Thanks,
Alex.

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Arnd Bergmann]

On Monday 14 January 2013, Alex Courbot wrote:
> On 01/10/2013 07:08 PM, Arnd Bergmann wrote:
> > I found two that provide the generic gpio interfaces when gpiolib
> > is disabled, but use gpiolib otherwise for the same hardware,
> > arch/m68k/include/asm/mcfgpio.h and arch/blackfin/include/asm/gpio.h.
> > I would assume that we can simply remove the non-gpiolib shortcut
> > here at cost of a small overhead.
> 
> I performed a search on my side too (checking configurations options 
> which select GENERIC_GPIO but not ARCH_REQUIRE_GPIOLIB) and found the 
> same list. This takes some time btw - many platforms use this combo to 
> make GPIO support optional. Can I ask how you figured out these two archs?

I basically grepped for GENERIC_GPIO and looked at
the individual implementations.

> > Then there are a bunch that use gpiolib but have a nontrivial
> > implementation of gpio_get_value and other functions. I'm not sure
> > if these are a problematic with your code.
> 
> AFAICT these all implement an inline path that bypasses GPIOlib when the 
> GPIO number is known at compile time, for faster bitbanging I presume. 
> It should be totally harmless to keep them. Unfortunately, I don't think 
> it would be possible to have a similar trick using handlers.

Right, makes sense.

	Arnd

Re: [PATCH] Proposed removal of IS_ERR_OR_NULL() (was: Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface)

[Linus Walleij]

On Wed, Jan 9, 2013 at 4:04 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> So, it seems there's some concensus building here, and it seems that
> I've become the chosen victi^wvolunteer for this.  So, here's a patch.
> It's missing a Guns-supplied-by: tag though.
>
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Subject: Mark IS_ERR_OR_NULL() deprecated

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

Following the tradition of computer science an alternate title of
the patch could be "IS_ERR_OR_NULL() considered harmful".

Yours,
Linus Walleij

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Linus Walleij]

On Thu, Jan 10, 2013 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> I've tried to find platforms that don't yet use GPIOLIB and fortunately
> there are very few left:
>
> I found two that provide the generic gpio interfaces when gpiolib
> is disabled, but use gpiolib otherwise for the same hardware,
> arch/m68k/include/asm/mcfgpio.h and arch/blackfin/include/asm/gpio.h.
> I would assume that we can simply remove the non-gpiolib shortcut
> here at cost of a small overhead.

Geert/Greg: interested in looking into this issue?

The GPIO maintainers want to get rid of non-gpiolib platforms.

Yours,
Linus Walleij

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Linus Walleij]

On Thu, Jan 10, 2013 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 10 January 2013, Alex Courbot wrote:

>> > Regarding the integration of pinctrl with gpio,
>> > I was thinking in the past that we could make pinctrl provide everything
>> > that gpiolib does, and have a generic gpiolib driver on top of pinctrl
>> > so that platforms don't need to implement both interfaces but only need
>> > to provide a pure pinctrl driver. Not sure if this makes any sense.
>>
>> That would work if all GPIOs were connected to a ball, but how about GPIO
>> expanders that are external to the chip? They have no use for pinctrl AFAICT.
>> On the other hand, maybe we can have one pinctrl-gpio driver for those chips
>> where pinctrl alone can emulate all the functionality of a GPIO controller.
>> Maybe such a driver exists already?
>
> I don't think we have that yet, but it would be another option: rather
> than putting a generic gpiolib driver on top of pinctrl, we could have
> pinctrl support for all gpios that go through gpiolib, and move device
> drivers over to use pinctrl as the way to manage gpios rather than the
> classic gpio drivers. That would be a larger change though, and require
> that we pull in the pinctrl subsystem on a lot of machines that don't
> need it today.

Not quite following but have the following loose idea:

- Add API such as <linux/pinctrl/gpio.h> with struct pin_gpio_ops {}
  or similar. As orthogonal to the mux and config interfaces we already
  have.

- Add ops like .set_input(), .set_output(), .drive_high() and .drive_low()
  (etc) to the ops struct so all functionality currently provided by
  gpiolib can be implemened by a driver.

- Make global pin numbers optional in gpiolib for the next part...

- Register a generic GPIO chip on top of the pinctrl-gpio.c, preferably
  only supporting Alex' descriptors.

- Provide a userspace interface to pinctrl with something like /dev/pinctrlN
  with an ioctl() interface, solving also Roland Stigge's issue with
  driving many pins at the same time in a smooth way.

Sounds like a plan?

I'd like to avoid the either-or-approach where you have to use
pinctrl only or only gpiolib, so a compatibility layer kindof.

I'm prepping a talk at ELC so will try to jot down something more
substantial.

Yours,
Linus Walleij

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Greg Ungerer]

Hi Linus,

(My gerg@snapgear.com email no longer gets to me, gerg@uclinux.org is
the best direct email for me now).

On 01/17/2013 09:15 PM, Linus Walleij wrote:
> On Thu, Jan 10, 2013 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
>> I've tried to find platforms that don't yet use GPIOLIB and fortunately
>> there are very few left:
>>
>> I found two that provide the generic gpio interfaces when gpiolib
>> is disabled, but use gpiolib otherwise for the same hardware,
>> arch/m68k/include/asm/mcfgpio.h and arch/blackfin/include/asm/gpio.h.
>> I would assume that we can simply remove the non-gpiolib shortcut
>> here at cost of a small overhead.
> 
> Geert/Greg: interested in looking into this issue?
> 
> The GPIO maintainers want to get rid of non-gpiolib platforms.

Steven King did most of the ColdFire GPIO work (CC'ed on this).
Steven: are you happy to drop the non-gpiolib support?

Regards
Greg

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Steven King]

On Thursday 17 January 2013 4:02:06 am Greg Ungerer wrote:
> Hi Linus,
>
> (My gerg@snapgear.com email no longer gets to me, gerg@uclinux.org is
> the best direct email for me now).
>
> On 01/17/2013 09:15 PM, Linus Walleij wrote:
> > On Thu, Jan 10, 2013 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> I've tried to find platforms that don't yet use GPIOLIB and fortunately
> >> there are very few left:
> >>
> >> I found two that provide the generic gpio interfaces when gpiolib
> >> is disabled, but use gpiolib otherwise for the same hardware,
> >> arch/m68k/include/asm/mcfgpio.h and arch/blackfin/include/asm/gpio.h.
> >> I would assume that we can simply remove the non-gpiolib shortcut
> >> here at cost of a small overhead.
> >
> > Geert/Greg: interested in looking into this issue?
> >
> > The GPIO maintainers want to get rid of non-gpiolib platforms.
>
> Steven King did most of the ColdFire GPIO work (CC'ed on this).
> Steven: are you happy to drop the non-gpiolib support?
>
> Regards
> Greg

Well, my concern is the small, single chip platforms with limited ram and 
speeds measured in MHz.  My goal was that these platforms that had very basic 
gpio needs, no offboard gpio, just toggling a few pins for spi or whatever, 
could do that without pulling in a bunch of code they dont need.  I realize 
that for x86 or arm people with their giga Hz cpus with gigabytes of ram, its 
no big deal, but my little 60 MHz coldfire v2s with only 16 megs of ram (and 
even more constraining, 2 megs of flash) need all the help they can get.

I haven't been keeping up with the kernel list of late, can someone point me 
to what''s being discussed so I can see what were talking about here?

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Arnd Bergmann]

On Thursday 17 January 2013, Steven King wrote:
> I haven't been keeping up with the kernel list of late, can someone point me 
> to what''s being discussed so I can see what were talking about here?

We are discussion about changes to the GPIO API, in particular about
adding a descriptor (i.e. pointer to struct) based interface to
replace integers as the primary key.

Any kind of change on this level is currently problematic because it
is basically impossible to tell which files are implementing the
current interface and would need to get changed along with changing
gpiolib. If everything goes through gpiolib, it's much easier
to make compatible extensions in one place.

Do you have any numbers about how much of a difference for your
platforms we are talking about at most?

	Arnd

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Alex Courbot]

Hi Steven,

On 01/18/2013 01:50 AM, Steven King wrote:
> Well, my concern is the small, single chip platforms with limited ram and
> speeds measured in MHz.  My goal was that these platforms that had very basic
> gpio needs, no offboard gpio, just toggling a few pins for spi or whatever,
> could do that without pulling in a bunch of code they dont need.  I realize
> that for x86 or arm people with their giga Hz cpus with gigabytes of ram, its
> no big deal, but my little 60 MHz coldfire v2s with only 16 megs of ram (and
> even more constraining, 2 megs of flash) need all the help they can get.

Running readelf on gpiolib.o built for Tegra, here are the footprints:

.text: 9412B
.data: 260B
.bss: 12312B
.rodata: 268B

Total: 22252B or 22KB.

... of which more than half is the BSS section which consists of a 
static array of 1024 GPIO descriptors, an arbitrary number that you can 
tune and is way too large even for Tegra (but we have to use these 
gigabytes somehow). Say you only need to use 256 GPIOs and .bss is down 
to ~3KB, for a total overhead of 15kB or 0.09% of your 16MB which, in 
perspective, suddenly seem gigantic. :)

If you are concerned about the additional indirection that GPIOlib 
introduces and the potential slowdown for bitbanging, you can always 
define custom gpio_set_value() and gpio_get_value() macros to shortcut 
GPIOlib when the GPIO is in the range you want performance for.

> I haven't been keeping up with the kernel list of late, can someone point me
> to what''s being discussed so I can see what were talking about here?

Arnd explained it already, but basically we'd like to consolidate the 
GPIO subsystem around GPIOlib and introduce safer interfaces (while 
preserving backward compatibility). Good progress in that direction 
would be achieved if all users of the GENERIC_GPIO interface relied on 
GPIOlib (making GENERIC_GPIO require GPIOLIB).

Actually the question of switching to GPIOlib is only worth being asked 
if you are making use of drivers that require GENERIC_GPIO. If this is 
not the case and your GPIOs are only used by your own platform code, you 
can always give up using defining GENERIC_GPIO and continue implementing 
them your own way.

Thanks,
Alex.

Re: [PATCH 0/4] gpio: introduce descriptor-based interface

[Linus Walleij]

On Sun, Jan 20, 2013 at 7:07 AM, Alex Courbot <acourbot@nvidia.com> wrote:

> Actually the question of switching to GPIOlib is only worth being asked if
> you are making use of drivers that require GENERIC_GPIO. If this is not the
> case and your GPIOs are only used by your own platform code, you can always
> give up using defining GENERIC_GPIO and continue implementing them your own
> way.

Agreed. The problem can be summed up like this:

#include <linux/gpio.h>

Considered ambiguous.

#include <asm/foo/bar/baz-gpio.h>

OK with me.

Yours,
Linus Walleij