[PATCH v2] drivers/pinctrl: grab default handles from device core

[PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

This makes the device core auto-grab the pinctrl handle and set
the "default" (PINCTRL_STATE_DEFAULT) state for every device
that is present in the device model right before probe. This will
account for the lion's share of embedded silicon devcies.

A modification of the semantics for pinctrl_get() is also done:
previously if the pinctrl handle for a certain device was already
taken, the pinctrl core would return an error. Now, since the
core may have already default-grabbed the handle and set its
state to "default", if the handle was already taken, this will
be disregarded and the located, previously instanitated handle
will be returned to the caller.

This way all code in drivers explicitly requesting their pinctrl
handlers will still be functional, and drivers that want to
explicitly retrieve and switch their handles can still do that.
But if the desired functionality is just boilerplate of this
type in the probe() function:

struct pinctrl  *p;

p = devm_pinctrl_get_select_default(&dev);
if (IS_ERR(p)) {
   if (PTR_ERR(p) == -EPROBE_DEFER)
        return -EPROBE_DEFER;
        dev_warn(&dev, "no pinctrl handle\n");
}

The discussion began with the addition of such boilerplate
to the omap4 keypad driver:
http://marc.info/?l=linux-input&m=135091157719300&w=2

A previous approach using notifiers was discussed:
http://marc.info/?l=linux-kernel&m=135263661110528&w=2
This failed because it could not handle deferred probes.

This patch alone does not solve the entire dilemma faced:
whether code should be distributed into the drivers or
if it should be centralized to e.g. a PM domain. But it
solves the immediate issue of the addition of boilerplate
to a lot of drivers that just want to grab the default
state. As mentioned, they can later explicitly retrieve
the handle and set different states, and this could as
well be done by e.g. PM domains as it is only related
to a certain struct device * pointer.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Mitch Bradley <wmb@firmworks.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Rickard Andersson <rickard.andersson@stericsson.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Only store a pointer in the device struct, and only allocate
  this if it's really used by the device.
- Seeking ACK on this from Greg (and others who like it) so I
  can merge it through the pinctrl subsystem.
---
 Documentation/pinctrl.txt       | 24 ++++++++++--
 drivers/base/Makefile           |  1 +
 drivers/base/dd.c               |  7 ++++
 drivers/base/pinctrl.c          | 81 +++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/core.c          | 11 +++++-
 include/linux/device.h          |  7 ++++
 include/linux/pinctrl/devinfo.h | 45 +++++++++++++++++++++++
 7 files changed, 171 insertions(+), 5 deletions(-)
 create mode 100644 drivers/base/pinctrl.c
 create mode 100644 include/linux/pinctrl/devinfo.h

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index da40efb..68836e5 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -972,6 +972,18 @@ pinmux core.
 Pin control requests from drivers
 =================================
 
+When a device driver is about to probe the device core will automatically
+attempt to issue pinctrl_get_select_default() on these devices.
+This way driver writers do not need to add any of the boilerplate code
+of the type found below. However when doing fine-grained state selection
+and not using the "default" state, you may have to do some device driver
+handling of the pinctrl handles and states.
+
+So if you just want to put the pins for a certain device into the default
+state and be done with it, there is nothing you need to do besides
+providing the proper mapping table. The device core will take care of
+the rest.
+
 Generally it is discouraged to let individual drivers get and enable pin
 control. So if possible, handle the pin control in platform code or some other
 place where you have access to all the affected struct device * pointers. In
@@ -1097,9 +1109,9 @@ situations that can be electrically unpleasant, you will certainly want to
 mux in and bias pins in a certain way before the GPIO subsystems starts to
 deal with them.
 
-The above can be hidden: using pinctrl hogs, the pin control driver may be
-setting up the config and muxing for the pins when it is probing,
-nevertheless orthogonal to the GPIO subsystem.
+The above can be hidden: using the device core, the pinctrl core may be
+setting up the config and muxing for the pins right before the device is
+probing, nevertheless orthogonal to the GPIO subsystem.
 
 But there are also situations where it makes sense for the GPIO subsystem
 to communicate directly with with the pinctrl subsystem, using the latter
@@ -1144,6 +1156,12 @@ PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-foo", NULL /* group */, "power_func")
 
 This gives the exact same result as the above construction.
 
+This should not be used for any kind of device which is represented in
+the device model, as the pinctrl core will attempt to do the equal of
+pinctrl_get_select_default() for these devices right before their device
+drivers are probed, so hogging these will just make the model look
+strange. Instead put in proper map entries.
+
 
 Runtime pinmuxing
 =================
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 5aa2d70..4e22ce3 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -21,6 +21,7 @@ endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
 obj-$(CONFIG_SOC_BUS) += soc.o
+obj-$(CONFIG_PINCTRL) += pinctrl.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e3bbed8..65631015 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -24,6 +24,7 @@
 #include <linux/wait.h>
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/devinfo.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -269,6 +270,12 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	WARN_ON(!list_empty(&dev->devres_head));
 
 	dev->driver = drv;
+
+	/* If using pinctrl, bind pins now before probing */
+	ret = pinctrl_bind_pins(dev);
+	if (ret)
+		goto probe_failed;
+
 	if (driver_sysfs_add(dev)) {
 		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
 			__func__, dev_name(dev));
diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
new file mode 100644
index 0000000..edbf8e1
--- /dev/null
+++ b/drivers/base/pinctrl.c
@@ -0,0 +1,81 @@
+/*
+ * Driver core interface to the pinctrl subsystem.
+ *
+ * Copyright (C) 2012 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/device.h>
+#include <linux/pinctrl/devinfo.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/slab.h>
+
+/**
+ * pinctrl_bind_pins() - called by the device core before probe
+ * @dev: the device that is just about to probe
+ */
+int pinctrl_bind_pins(struct device *dev)
+{
+	struct dev_pin_info *dpi;
+	int ret;
+
+	/* Allocate a pin state container on-the-fly */
+	if (!dev->pins) {
+		dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
+		if (!dpi)
+			return -ENOMEM;
+	} else
+		dpi = dev->pins;
+
+	/*
+	 * Check if we already have a pinctrl handle, as we may arrive here
+	 * after a deferral in the state selection below
+	 */
+	if (!dpi->p) {
+		dpi->p = devm_pinctrl_get(dev);
+		if (IS_ERR_OR_NULL(dpi->p)) {
+			int ret = PTR_ERR(dpi->p);
+
+			dev_dbg(dev, "no pinctrl handle\n");
+			/* Only return deferrals */
+			if (ret == -EPROBE_DEFER)
+				return ret;
+			return 0;
+		}
+	}
+
+	/*
+	 * For a newly allocated info struct, here is where we keep it,
+	 * since at this point it actually contains something.
+	 */
+	dev->pins = dpi;
+
+	/*
+	 * We may have looked up the state earlier as well.
+	 */
+	if (!dpi->default_state) {
+		dpi->default_state = pinctrl_lookup_state(dpi->p,
+						PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(dpi->default_state)) {
+			dev_dbg(dev, "no default pinctrl state\n");
+			return 0;
+		}
+	}
+
+	ret = pinctrl_select_state(dpi->p, dpi->default_state);
+	if (ret) {
+		dev_dbg(dev, "failed to activate default pinctrl state\n");
+
+		/* Only return deferrals */
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		return 0;
+	}
+
+	return 0;
+}
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 5b4885c..6947bf0 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -734,9 +734,16 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev)
 	if (WARN_ON(!dev))
 		return ERR_PTR(-EINVAL);
 
+	/*
+	 * See if somebody else (such as the device core) has already
+	 * obtained a handle to the pinctrl for this device. In that case,
+	 * return another pointer to it.
+	 */
 	p = find_pinctrl(dev);
-	if (p != NULL)
-		return ERR_PTR(-EBUSY);
+	if (p != NULL) {
+		dev_dbg(dev, "obtain a copy of previously claimed pinctrl\n");
+		return p;
+	}
 
 	return create_pinctrl(dev);
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 86ef6ab..86d11f7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -21,6 +21,7 @@
 #include <linux/compiler.h>
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/pinctrl/devinfo.h>
 #include <linux/pm.h>
 #include <linux/atomic.h>
 #include <linux/ratelimit.h>
@@ -605,6 +606,8 @@ struct device_dma_parameters {
  * @pm_domain:	Provide callbacks that are executed during system suspend,
  * 		hibernation, system resume and during runtime PM transitions
  * 		along with subsystem-level and driver-level callbacks.
+ * @pins:	For device pin management.
+ *		See Documentation/pinctrl.txt for details.
  * @numa_node:	NUMA node this device is close to.
  * @dma_mask:	Dma mask (if dma'ble device).
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all
@@ -656,6 +659,10 @@ struct device {
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 
+#ifdef CONFIG_PINCTRL
+	struct dev_pin_info	*pins;
+#endif
+
 #ifdef CONFIG_NUMA
 	int		numa_node;	/* NUMA node this device is close to */
 #endif
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
new file mode 100644
index 0000000..6e5f8a9
--- /dev/null
+++ b/include/linux/pinctrl/devinfo.h
@@ -0,0 +1,45 @@
+/*
+ * Per-device information from the pin control system.
+ * This is the stuff that get included into the device
+ * core.
+ *
+ * Copyright (C) 2012 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * This interface is used in the core to keep track of pins.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#ifndef PINCTRL_DEVINFO_H
+#define PINCTRL_DEVINFO_H
+
+#ifdef CONFIG_PINCTRL
+
+/* The device core acts as a consumer toward pinctrl */
+#include <linux/pinctrl/consumer.h>
+
+/**
+ * struct dev_pin_info - pin state container for devices
+ * @p: pinctrl handle for the containing device
+ * @default_state: the default state for the handle, if found
+ */
+struct dev_pin_info {
+	struct pinctrl *p;
+	struct pinctrl_state *default_state;
+};
+
+extern int pinctrl_bind_pins(struct device *dev);
+
+#else
+
+/* Stubs if we're not using pinctrl */
+
+static inline int pinctrl_bind_pins(struct device *dev)
+{
+	return 0;
+}
+
+#endif /* CONFIG_PINCTRL */
+#endif /* PINCTRL_DEVINFO_H */
-- 
1.7.11.3

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Greg Kroah-Hartman]

On Wed, Dec 12, 2012 at 09:25:07PM +0100, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This makes the device core auto-grab the pinctrl handle and set
> the "default" (PINCTRL_STATE_DEFAULT) state for every device
> that is present in the device model right before probe. This will
> account for the lion's share of embedded silicon devcies.
> 
> A modification of the semantics for pinctrl_get() is also done:
> previously if the pinctrl handle for a certain device was already
> taken, the pinctrl core would return an error. Now, since the
> core may have already default-grabbed the handle and set its
> state to "default", if the handle was already taken, this will
> be disregarded and the located, previously instanitated handle
> will be returned to the caller.
> 
> This way all code in drivers explicitly requesting their pinctrl
> handlers will still be functional, and drivers that want to
> explicitly retrieve and switch their handles can still do that.
> But if the desired functionality is just boilerplate of this
> type in the probe() function:
> 
> struct pinctrl  *p;
> 
> p = devm_pinctrl_get_select_default(&dev);
> if (IS_ERR(p)) {
>    if (PTR_ERR(p) == -EPROBE_DEFER)
>         return -EPROBE_DEFER;
>         dev_warn(&dev, "no pinctrl handle\n");
> }
> 
> The discussion began with the addition of such boilerplate
> to the omap4 keypad driver:
> http://marc.info/?l=linux-input&m=135091157719300&w=2
> 
> A previous approach using notifiers was discussed:
> http://marc.info/?l=linux-kernel&m=135263661110528&w=2
> This failed because it could not handle deferred probes.
> 
> This patch alone does not solve the entire dilemma faced:
> whether code should be distributed into the drivers or
> if it should be centralized to e.g. a PM domain. But it
> solves the immediate issue of the addition of boilerplate
> to a lot of drivers that just want to grab the default
> state. As mentioned, they can later explicitly retrieve
> the handle and set different states, and this could as
> well be done by e.g. PM domains as it is only related
> to a certain struct device * pointer.
> 
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Mitch Bradley <wmb@firmworks.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Rickard Andersson <rickard.andersson@stericsson.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Only store a pointer in the device struct, and only allocate
>   this if it's really used by the device.
> - Seeking ACK on this from Greg (and others who like it) so I
>   can merge it through the pinctrl subsystem.

It looks ok, but this can't go in for 3.8, sorry, please wait for 3.9.

greg k-h

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Mark Brown]

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

On Wed, Dec 12, 2012 at 09:25:07PM +0100, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This makes the device core auto-grab the pinctrl handle and set
> the "default" (PINCTRL_STATE_DEFAULT) state for every device
> that is present in the device model right before probe. This will
> account for the lion's share of embedded silicon devcies.

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Thanks for doing this - sorry it took me a while to get round to reading
this properly, the end of last year was very hectic.

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

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Wed, Dec 12, 2012 at 10:54 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 12, 2012 at 09:25:07PM +0100, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This makes the device core auto-grab the pinctrl handle and set
>> the "default" (PINCTRL_STATE_DEFAULT) state for every device
>> that is present in the device model right before probe. This will
>> account for the lion's share of embedded silicon devcies.
(...)
>
> It looks ok, but this can't go in for 3.8, sorry, please wait for 3.9.

Greg, now that v3.8-rc1 is out, can you give me an ACK on
this patch so I can put it into linux-next for some rotation
through the pinctrl tree?

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Wed, Jan 2, 2013 at 2:42 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Dec 12, 2012 at 10:54 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Wed, Dec 12, 2012 at 09:25:07PM +0100, Linus Walleij wrote:
>>> From: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> This makes the device core auto-grab the pinctrl handle and set
>>> the "default" (PINCTRL_STATE_DEFAULT) state for every device
>>> that is present in the device model right before probe. This will
>>> account for the lion's share of embedded silicon devcies.
> (...)
>>
>> It looks ok, but this can't go in for 3.8, sorry, please wait for 3.9.
>
> Greg, now that v3.8-rc1 is out, can you give me an ACK on
> this patch so I can put it into linux-next for some rotation
> through the pinctrl tree?

Ping on Greg.

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 12/12/2012 01:25 PM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This makes the device core auto-grab the pinctrl handle and set
> the "default" (PINCTRL_STATE_DEFAULT) state for every device
> that is present in the device model right before probe. This will
> account for the lion's share of embedded silicon devcies.

There are quite a few problems with this patch, and they end up
completely breaking at least Tegra in next-20130110.

> diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c

> +int pinctrl_bind_pins(struct device *dev)
> +{
> +	struct dev_pin_info *dpi;
> +	int ret;
> +
> +	/* Allocate a pin state container on-the-fly */
> +	if (!dev->pins) {
> +		dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);

This is allocated using a devm_ function. If -EPROBE_DEFER is returned
below after the assignment to dev->pins or if the driver's own probe()
returns -EPROBE_DEFER, this allocation will be freed by the driver core.
This can leave dev->pins pointing to something non-NULL, yet invalid.

I haven't fully verified this, but I believe this issue is causing
crashes on Tegra. Certainly if I force this code to follow the path that
always allocates new structs or performs new gets, then the crashes go away.

> +		if (!dpi)
> +			return -ENOMEM;
> +	} else
> +		dpi = dev->pins;
> +
> +	/*
> +	 * Check if we already have a pinctrl handle, as we may arrive here
> +	 * after a deferral in the state selection below
> +	 */
> +	if (!dpi->p) {
> +		dpi->p = devm_pinctrl_get(dev);

That won't succeed for a pinctrl device that has a default state in
order to implement hogs. This will then cause the pin controller device
to always defer probe and never activate. This will leave HW
unconfigured and/or prevent other devices from successfully calling
pinctrl_get().

This issue also happens on Tegra.

> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c

> @@ -734,9 +734,16 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev)
>  	if (WARN_ON(!dev))
>  		return ERR_PTR(-EINVAL);
>  
> +	/*
> +	 * See if somebody else (such as the device core) has already
> +	 * obtained a handle to the pinctrl for this device. In that case,
> +	 * return another pointer to it.
> +	 */
>  	p = find_pinctrl(dev);
> -	if (p != NULL)
> -		return ERR_PTR(-EBUSY);

I deliberately returned an error here, because there's no reference
counting on the struct pinctrl objects. If a driver calls pinctrl_get(),
with the new code below, it will retrieve the same struct. If it later
calls pinctrl_put(), the put will immediately free the structure. This
will invalidate the pointers that reference it in struct device's pins
field.

This issue will probably trigger on Tegra, since we at least have a
pinctrl-based I2C mux that calls pinctrl_get().

> +	if (p != NULL) {
> +		dev_dbg(dev, "obtain a copy of previously claimed pinctrl\n");
> +		return p;
> +	}
>  
>  	return create_pinctrl(dev);
>  }

Perhaps we could remove this patch from linux-next, and have a V3?

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/10/2013 01:42 PM, Stephen Warren wrote:
> On 12/12/2012 01:25 PM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This makes the device core auto-grab the pinctrl handle and set
>> the "default" (PINCTRL_STATE_DEFAULT) state for every device
>> that is present in the device model right before probe. This will
>> account for the lion's share of embedded silicon devcies.
> 
> There are quite a few problems with this patch, and they end up
> completely breaking at least Tegra in next-20130110.
> 
>> diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
> 
>> +int pinctrl_bind_pins(struct device *dev)
>> +{
>> +	struct dev_pin_info *dpi;
>> +	int ret;
>> +
>> +	/* Allocate a pin state container on-the-fly */
>> +	if (!dev->pins) {
>> +		dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
>> +		if (!dpi)
>> +			return -ENOMEM;
>> +	} else
>> +		dpi = dev->pins;
>> +
>> +	/*
>> +	 * Check if we already have a pinctrl handle, as we may arrive here
>> +	 * after a deferral in the state selection below
>> +	 */
>> +	if (!dpi->p) {
>> +		dpi->p = devm_pinctrl_get(dev);
> 
> That won't succeed for a pinctrl device that has a default state in
> order to implement hogs. This will then cause the pin controller device
> to always defer probe and never activate. This will leave HW
> unconfigured and/or prevent other devices from successfully calling
> pinctrl_get().

I see that an attempt was made to solve this problem, in the patch
immediately preceding this one (at least, as applied in the pinctrl
tree). However, that patch only addresses the case where the pin
controller is being looked up in the map, and not the case when
converting device tree to the map in the first place. The patch below
solves this:

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fe2d1af..fd40a11 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -141,6 +141,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
const char *statename,
                pctldev = find_pinctrl_by_of_node(np_pctldev);
                if (pctldev)
                        break;
+               /* Do not defer probing of hogs (circular loop) */
+               if (np_pctldev == p->dev->of_node) {
+                       of_node_put(np_pctldev);
+                       return -ENODEV;
+               }
        }
        of_node_put(np_pctldev);

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Thu, Jan 10, 2013 at 9:42 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 12/12/2012 01:25 PM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This makes the device core auto-grab the pinctrl handle and set
>> the "default" (PINCTRL_STATE_DEFAULT) state for every device
>> that is present in the device model right before probe. This will
>> account for the lion's share of embedded silicon devcies.
>
> There are quite a few problems with this patch, and they end up
> completely breaking at least Tegra in next-20130110.

But I have not put this patch into linux-next.

I was still waiting for Greg to ACK it...

Apparently it was included in a pull request to the SH tree or something,
Simon can you remove these patches for now, this patch needs to
be elaborated on in the pinctrl tree first.

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Thu, Jan 10, 2013 at 11:07 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> I see that an attempt was made to solve this problem, in the patch
> immediately preceding this one (at least, as applied in the pinctrl
> tree). However, that patch only addresses the case where the pin
> controller is being looked up in the map, and not the case when
> converting device tree to the map in the first place. The patch below
> solves this:
>
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> index fe2d1af..fd40a11 100644
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -141,6 +141,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
> const char *statename,
>                 pctldev = find_pinctrl_by_of_node(np_pctldev);
>                 if (pctldev)
>                         break;
> +               /* Do not defer probing of hogs (circular loop) */
> +               if (np_pctldev == p->dev->of_node) {
> +                       of_node_put(np_pctldev);
> +                       return -ENODEV;
> +               }
>         }
>         of_node_put(np_pctldev);

OK I've duplicated this, will send out as patch.

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Laurent Pinchart]

Hi Simon,

On Friday 11 January 2013 21:12:43 Linus Walleij wrote:
> On Thu, Jan 10, 2013 at 9:42 PM, Stephen Warren wrote:
> > On 12/12/2012 01:25 PM, Linus Walleij wrote:
> >> From: Linus Walleij <linus.walleij@linaro.org>
> >> 
> >> This makes the device core auto-grab the pinctrl handle and set
> >> the "default" (PINCTRL_STATE_DEFAULT) state for every device
> >> that is present in the device model right before probe. This will
> >> account for the lion's share of embedded silicon devcies.
> > 
> > There are quite a few problems with this patch, and they end up
> > completely breaking at least Tegra in next-20130110.
> 
> But I have not put this patch into linux-next.
> 
> I was still waiting for Greg to ACK it...
> 
> Apparently it was included in a pull request to the SH tree or something,
> Simon can you remove these patches for now, this patch needs to
> be elaborated on in the pinctrl tree first.

I've sent several patch series for the SH PFC (Pin Function Controller) to the 
linux-sh mailing list. One of the series included pinctrl core patches for 
easier testing, but I made it clear that they should *not* be pushed to 
mainline through the SH tree.

Actually the whole PFC series have been found out today by Guennadi 
Liakhovetski to be buggy. I will fix the problems and send a new version. In 
the meantime the pinmux-pinctrl and pinmux-dt series should not be pushed to 
mainline.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I've sent several patch series for the SH PFC (Pin Function Controller) to the
> linux-sh mailing list. One of the series included pinctrl core patches for
> easier testing, but I made it clear that they should *not* be pushed to
> mainline through the SH tree.

No big deal, what fun is linux-next if we don't break it ;-)

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/11/2013 01:45 PM, Linus Walleij wrote:
> On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> 
>> I've sent several patch series for the SH PFC (Pin Function Controller) to the
>> linux-sh mailing list. One of the series included pinctrl core patches for
>> easier testing, but I made it clear that they should *not* be pushed to
>> mainline through the SH tree.
> 
> No big deal, what fun is linux-next if we don't break it ;-)

Hmm. It's causing a lot of engineers here a lot of trouble, since they
all see linux-next won't boot, and haven't been paying enough attention
to know which commit to revert:-(. Lots of lost productivity:-(

Simon, the offending commit:

6d3ef6b drivers/pinctrl: grab default handles from device core

is still in next-20130116. Can you please remove it?

Or does it make sense for Stephen Rothwell to revert it when building
linux-next?

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Wed, Jan 16, 2013 at 6:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Hmm. It's causing a lot of engineers here a lot of trouble, since they
> all see linux-next won't boot, and haven't been paying enough attention
> to know which commit to revert:-(. Lots of lost productivity:-(
>
> Simon, the offending commit:
>
> 6d3ef6b drivers/pinctrl: grab default handles from device core
>
> is still in next-20130116. Can you please remove it?

Not that this should be in the SH tree, but I have merged your fix
to the pinctrl tree and it should be in -next, does it fix the problem?

Mainly asking because once Greg ACKs that patch I want to
put it into the pinctrl tree...

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Greg KH]

On Thu, Jan 17, 2013 at 01:59:52AM +0100, Linus Walleij wrote:
> On Wed, Jan 16, 2013 at 6:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> > Hmm. It's causing a lot of engineers here a lot of trouble, since they
> > all see linux-next won't boot, and haven't been paying enough attention
> > to know which commit to revert:-(. Lots of lost productivity:-(
> >
> > Simon, the offending commit:
> >
> > 6d3ef6b drivers/pinctrl: grab default handles from device core
> >
> > is still in next-20130116. Can you please remove it?
> 
> Not that this should be in the SH tree, but I have merged your fix
> to the pinctrl tree and it should be in -next, does it fix the problem?
> 
> Mainly asking because once Greg ACKs that patch I want to
> put it into the pinctrl tree...

I want to see the updated one with all of the fixes before I ack
anything.

thanks,

greg k-h

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Simon Horman]

On Wed, Jan 16, 2013 at 10:49:05AM -0700, Stephen Warren wrote:
> On 01/11/2013 01:45 PM, Linus Walleij wrote:
> > On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > 
> >> I've sent several patch series for the SH PFC (Pin Function Controller) to the
> >> linux-sh mailing list. One of the series included pinctrl core patches for
> >> easier testing, but I made it clear that they should *not* be pushed to
> >> mainline through the SH tree.
> > 
> > No big deal, what fun is linux-next if we don't break it ;-)
> 
> Hmm. It's causing a lot of engineers here a lot of trouble, since they
> all see linux-next won't boot, and haven't been paying enough attention
> to know which commit to revert:-(. Lots of lost productivity:-(
> 
> Simon, the offending commit:
> 
> 6d3ef6b drivers/pinctrl: grab default handles from device core
> 
> is still in next-20130116. Can you please remove it?

I removed the commit yesterday.
Please let me know if it seems to be lingering.

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/16/2013 05:59 PM, Linus Walleij wrote:
> On Wed, Jan 16, 2013 at 6:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Hmm. It's causing a lot of engineers here a lot of trouble, since they
>> all see linux-next won't boot, and haven't been paying enough attention
>> to know which commit to revert:-(. Lots of lost productivity:-(
>>
>> Simon, the offending commit:
>>
>> 6d3ef6b drivers/pinctrl: grab default handles from device core
>>
>> is still in next-20130116. Can you please remove it?
> 
> Not that this should be in the SH tree, but I have merged your fix
> to the pinctrl tree and it should be in -next, does it fix the problem?

There were two problems; one was the issues handling hogs during DT->map
conversion, for which you applied a patch. The other was the crashes due
to incorrect devm_* usage and interactions with -EPROBE_DEFER in the
patch which Simon applied, which requires that patch to be revised.

> Mainly asking because once Greg ACKs that patch I want to
> put it into the pinctrl tree...

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/16/2013 11:02 PM, Simon Horman wrote:
> On Wed, Jan 16, 2013 at 10:49:05AM -0700, Stephen Warren wrote:
>> On 01/11/2013 01:45 PM, Linus Walleij wrote:
>>> On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>
>>>> I've sent several patch series for the SH PFC (Pin Function Controller) to the
>>>> linux-sh mailing list. One of the series included pinctrl core patches for
>>>> easier testing, but I made it clear that they should *not* be pushed to
>>>> mainline through the SH tree.
>>>
>>> No big deal, what fun is linux-next if we don't break it ;-)
>>
>> Hmm. It's causing a lot of engineers here a lot of trouble, since they
>> all see linux-next won't boot, and haven't been paying enough attention
>> to know which commit to revert:-(. Lots of lost productivity:-(
>>
>> Simon, the offending commit:
>>
>> 6d3ef6b drivers/pinctrl: grab default handles from device core
>>
>> is still in next-20130116. Can you please remove it?
> 
> I removed the commit yesterday.
> Please let me know if it seems to be lingering.

Thanks. I believe it's gone from next-20130117, judging by a quick
search through "git log" at least.

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Simon Horman]

On Thu, Jan 17, 2013 at 09:31:01AM -0700, Stephen Warren wrote:
> On 01/16/2013 11:02 PM, Simon Horman wrote:
> > On Wed, Jan 16, 2013 at 10:49:05AM -0700, Stephen Warren wrote:
> >> On 01/11/2013 01:45 PM, Linus Walleij wrote:
> >>> On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
> >>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>
> >>>> I've sent several patch series for the SH PFC (Pin Function Controller) to the
> >>>> linux-sh mailing list. One of the series included pinctrl core patches for
> >>>> easier testing, but I made it clear that they should *not* be pushed to
> >>>> mainline through the SH tree.
> >>>
> >>> No big deal, what fun is linux-next if we don't break it ;-)
> >>
> >> Hmm. It's causing a lot of engineers here a lot of trouble, since they
> >> all see linux-next won't boot, and haven't been paying enough attention
> >> to know which commit to revert:-(. Lots of lost productivity:-(
> >>
> >> Simon, the offending commit:
> >>
> >> 6d3ef6b drivers/pinctrl: grab default handles from device core
> >>
> >> is still in next-20130116. Can you please remove it?
> > 
> > I removed the commit yesterday.
> > Please let me know if it seems to be lingering.
> 
> Thanks. I believe it's gone from next-20130117, judging by a quick
> search through "git log" at least.

Thanks and sorry for the mess.

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Thu, Jan 10, 2013 at 9:42 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> +     /* Allocate a pin state container on-the-fly */
>> +     if (!dev->pins) {
>> +             dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
>
> This is allocated using a devm_ function. If -EPROBE_DEFER is returned
> below after the assignment to dev->pins or if the driver's own probe()
> returns -EPROBE_DEFER, this allocation will be freed by the driver core.
> This can leave dev->pins pointing to something non-NULL, yet invalid.

OK I (think I) fixed it like this:

For the second deferral, set dev->pins to NULL.

I did some other fixes here to explicitly devm_kfree() the container
if no pinctrl handle was found. No point in keeping it around.

>> +     if (!dpi->p) {
>> +             dpi->p = devm_pinctrl_get(dev);
>
> That won't succeed for a pinctrl device that has a default state in
> order to implement hogs. This will then cause the pin controller device
> to always defer probe and never activate. This will leave HW
> unconfigured and/or prevent other devices from successfully calling
> pinctrl_get().

Fixed by your suggested patch, thanks.

>> +     /*
>> +      * See if somebody else (such as the device core) has already
>> +      * obtained a handle to the pinctrl for this device. In that case,
>> +      * return another pointer to it.
>> +      */
>>       p = find_pinctrl(dev);
>> -     if (p != NULL)
>> -             return ERR_PTR(-EBUSY);
>
> I deliberately returned an error here, because there's no reference
> counting on the struct pinctrl objects. If a driver calls pinctrl_get(),
> with the new code below, it will retrieve the same struct. If it later
> calls pinctrl_put(), the put will immediately free the structure. This
> will invalidate the pointers that reference it in struct device's pins
> field.
>
> This issue will probably trigger on Tegra, since we at least have a
> pinctrl-based I2C mux that calls pinctrl_get().

OK I just introduced a reference counter using <linux/kref.h>.

I've retested on U300 and U8500.

Yours,
Linus Walleij