gpio: introduce descriptor-based interface

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

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.

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

index 1077754..b7a9ad6 100644
@@ -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);

1.8.1

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

Absolutely, thanks.

Alex.

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.

Correct.

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

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.

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

[ examples of broken code skipped ]

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

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 <[email protected]>
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:

Victim-for-firing-sqad: Russell King <[email protected]>

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

index f2edce2..d5a85df 100644
@@ -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);
 }

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.

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.

Guns-supplied-by: NRA (obviously)

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.

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.

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

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

index 11e2a41..251f54f 100644
@@ -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);

index 7322838..3fadadf 100644
@@ -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);
 }

index e065daa..fabb32d 100644
@@ -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);

index e2c291f..548547e 100644
@@ -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);

index dea62a9..36a6918 100644
@@ -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;

index 3cdc1bb..6d29e6a 100644
@@ -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;
 	}
 

index 837c7b9..844c8d2 100644
@@ -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),

index 721e7b4..e31c2b4 100644
@@ -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;
 }

index d51b75b..c654bb8 100644
@@ -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;
 	}

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

index 11e2a41..61225e1 100644
@@ -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);

index 7322838..6016189 100644
@@ -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);
 }

index e065daa..fabb32d 100644
@@ -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);

index e2c291f..548547e 100644
@@ -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);

index dea62a9..36a6918 100644
@@ -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;

index 3cdc1bb..6d29e6a 100644
@@ -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;
 	}
 

index 837c7b9..844c8d2 100644
@@ -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),

index 721e7b4..a480266 100644
@@ -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;
 }

index d51b75b..d0fe4c3a 100644
@@ -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__);

Looks correct to me.

Regards,

Tony

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

Yours,
Linus Walleij

On Wed, Jan 09, 2013 at 11:10:55AM +0000, Russell King - ARM Linux wrote:
[...]

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

Thierry

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

---
 drivers/gpio/gpiolib.c        | 164 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h |   8 +++
 include/linux/gpio/driver.h   |  21 ++++++
 3 files changed, 193 insertions(+)

index d04b90b..06ffadb 100644
@@ -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)

index 0ce96cf..2f30761 100644
@@ -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);

index d42f941..d43d671 100644
@@ -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

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

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

Alex.

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

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

index d542a14..8c9f8c5 100644
@@ -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);

index d43d671..84447a5 100644
@@ -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

1.8.1

---
 Documentation/gpio.txt | 94 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 2 deletions(-)

index 77a1d11..871ccda 100644
@@ -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

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

Great! This was initially suggested by Linus W.

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.

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.

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

Thanks,
Alex.

Right.

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.

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

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.

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.

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

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.

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.

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.

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.

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.

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

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.

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.

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

Right, makes sense.

Arnd

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.

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.

Geert/Greg: interested in looking into this issue?

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

Yours,
Linus Walleij

Hi Linus,

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

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?

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

Hi Steven,

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.

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.

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

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