All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 13:44 ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-05 13:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman
  Cc: linux-kernel, linux-arm-kernel, Linus Walleij, Hebbar Gururaja,
	Mark Brown, Dmitry Torokhov, Stephen Warren, Wolfram Sang

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

If a device have sleep and idle states in addition to the
default state, look up these in the core and stash them in
the pinctrl state container.

Add accessor functions for pinctrl consumers to put the pins
into "default", "sleep" and "idle" states passing nothing but
the struct device * affected.

Solution suggested by Kevin Hilman, Mark Brown and Dmitry
Torokhov in response to a patch series from Hebbar
Gururaja.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Gregs ACK on this in the end, so we can take this
in through the pinctrl tree. But first let's review!
---
 drivers/base/pinctrl.c           | 19 +++++++++++++
 drivers/pinctrl/core.c           | 61 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pinctrl/consumer.h | 34 ++++++++++++++++++++++
 include/linux/pinctrl/devinfo.h  |  4 +++
 4 files changed, 118 insertions(+)

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 67a274e..5fb74b4 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -48,6 +48,25 @@ int pinctrl_bind_pins(struct device *dev)
 		goto cleanup_get;
 	}
 
+#ifdef CONFIG_PM
+	/*
+	 * If power management is enabled, we also look for the optional
+	 * sleep and idle pin states, with semantics as defined in
+	 * <linux/pinctrl/pinctrl-state.h>
+	 */
+	dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
+					PINCTRL_STATE_SLEEP);
+	if (IS_ERR(dev->pins->sleep_state))
+		/* Not supplying this state is perfectly legal */
+		dev_dbg(dev, "no sleep pinctrl state\n");
+
+	dev->pins->idle_state = pinctrl_lookup_state(dev->pins->p,
+					PINCTRL_STATE_IDLE);
+	if (IS_ERR(dev->pins->idle_state))
+		/* Not supplying this state is perfectly legal */
+		dev_dbg(dev, "no idle pinctrl state\n");
+#endif
+
 	return 0;
 
 	/*
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 1f9608b..90fa17a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1196,6 +1196,67 @@ int pinctrl_force_default(struct pinctrl_dev *pctldev)
 }
 EXPORT_SYMBOL_GPL(pinctrl_force_default);
 
+#ifdef CONFIG_PM
+
+/**
+ * pinctrl_pm_select_default_state() - select default pinctrl state for PM
+ * @dev: device to select default state for
+ */
+int pinctrl_pm_select_default_state(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+	if (IS_ERR(pins->default_state))
+		return 0; /* No default state */
+	ret = pinctrl_select_state(pins->p, pins->default_state);
+	if (ret)
+		dev_err(dev, "failed to activate default pinctrl state\n");
+	return ret;
+}
+
+/**
+ * pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM
+ * @dev: device to select sleep state for
+ */
+int pinctrl_pm_select_sleep_state(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+	if (IS_ERR(pins->sleep_state))
+		return 0; /* No default state */
+	ret = pinctrl_select_state(pins->p, pins->sleep_state);
+	if (ret)
+		dev_err(dev, "failed to activate sleep pinctrl state\n");
+	return ret;
+}
+
+/**
+ * pinctrl_pm_select_idle_state() - select idle pinctrl state for PM
+ * @dev: device to select idle state for
+ */
+int pinctrl_pm_select_idle_state(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+	if (IS_ERR(pins->idle_state))
+		return 0; /* No default state */
+	ret = pinctrl_select_state(pins->p, pins->idle_state);
+	if (ret)
+		dev_err(dev, "failed to activate idle pinctrl state\n");
+	return ret;
+}
+
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 
 static int pinctrl_pins_show(struct seq_file *s, void *what)
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 4aad3ce..0f32f10 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -40,6 +40,25 @@ extern int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *s);
 extern struct pinctrl * __must_check devm_pinctrl_get(struct device *dev);
 extern void devm_pinctrl_put(struct pinctrl *p);
 
+#ifdef CONFIG_PM
+extern int pinctrl_pm_select_default_state(struct device *dev);
+extern int pinctrl_pm_select_sleep_state(struct device *dev);
+extern int pinctrl_pm_select_idle_state(struct device *dev);
+#else
+static inline int pinctrl_pm_select_default_state(struct device *dev)
+{
+	return 0;
+}
+static inline int pinctrl_pm_select_sleep_state(struct device *dev)
+{
+	return 0;
+}
+static inline int pinctrl_pm_select_idle_state(struct device *dev)
+{
+	return 0;
+}
+#endif
+
 #else /* !CONFIG_PINCTRL */
 
 static inline int pinctrl_request_gpio(unsigned gpio)
@@ -199,6 +218,21 @@ static inline int pin_config_group_set(const char *dev_name,
 	return 0;
 }
 
+static inline int pinctrl_pm_select_default_state(struct device *dev)
+{
+	return 0;
+}
+
+static inline int pinctrl_pm_select_sleep_state(struct device *dev)
+{
+	return 0;
+}
+
+static inline int pinctrl_pm_select_idle_state(struct device *dev)
+{
+	return 0;
+}
+
 #endif
 
 #endif /* __LINUX_PINCTRL_CONSUMER_H */
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index 6e5f8a9..281cb91 100644
--- a/include/linux/pinctrl/devinfo.h
+++ b/include/linux/pinctrl/devinfo.h
@@ -28,6 +28,10 @@
 struct dev_pin_info {
 	struct pinctrl *p;
 	struct pinctrl_state *default_state;
+#ifdef CONFIG_PM
+	struct pinctrl_state *sleep_state;
+	struct pinctrl_state *idle_state;
+#endif
 };
 
 extern int pinctrl_bind_pins(struct device *dev);
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 13:44 ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-05 13:44 UTC (permalink / raw)
  To: linux-arm-kernel

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

If a device have sleep and idle states in addition to the
default state, look up these in the core and stash them in
the pinctrl state container.

Add accessor functions for pinctrl consumers to put the pins
into "default", "sleep" and "idle" states passing nothing but
the struct device * affected.

Solution suggested by Kevin Hilman, Mark Brown and Dmitry
Torokhov in response to a patch series from Hebbar
Gururaja.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Gregs ACK on this in the end, so we can take this
in through the pinctrl tree. But first let's review!
---
 drivers/base/pinctrl.c           | 19 +++++++++++++
 drivers/pinctrl/core.c           | 61 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pinctrl/consumer.h | 34 ++++++++++++++++++++++
 include/linux/pinctrl/devinfo.h  |  4 +++
 4 files changed, 118 insertions(+)

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 67a274e..5fb74b4 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -48,6 +48,25 @@ int pinctrl_bind_pins(struct device *dev)
 		goto cleanup_get;
 	}
 
+#ifdef CONFIG_PM
+	/*
+	 * If power management is enabled, we also look for the optional
+	 * sleep and idle pin states, with semantics as defined in
+	 * <linux/pinctrl/pinctrl-state.h>
+	 */
+	dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
+					PINCTRL_STATE_SLEEP);
+	if (IS_ERR(dev->pins->sleep_state))
+		/* Not supplying this state is perfectly legal */
+		dev_dbg(dev, "no sleep pinctrl state\n");
+
+	dev->pins->idle_state = pinctrl_lookup_state(dev->pins->p,
+					PINCTRL_STATE_IDLE);
+	if (IS_ERR(dev->pins->idle_state))
+		/* Not supplying this state is perfectly legal */
+		dev_dbg(dev, "no idle pinctrl state\n");
+#endif
+
 	return 0;
 
 	/*
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 1f9608b..90fa17a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1196,6 +1196,67 @@ int pinctrl_force_default(struct pinctrl_dev *pctldev)
 }
 EXPORT_SYMBOL_GPL(pinctrl_force_default);
 
+#ifdef CONFIG_PM
+
+/**
+ * pinctrl_pm_select_default_state() - select default pinctrl state for PM
+ * @dev: device to select default state for
+ */
+int pinctrl_pm_select_default_state(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+	if (IS_ERR(pins->default_state))
+		return 0; /* No default state */
+	ret = pinctrl_select_state(pins->p, pins->default_state);
+	if (ret)
+		dev_err(dev, "failed to activate default pinctrl state\n");
+	return ret;
+}
+
+/**
+ * pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM
+ * @dev: device to select sleep state for
+ */
+int pinctrl_pm_select_sleep_state(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+	if (IS_ERR(pins->sleep_state))
+		return 0; /* No default state */
+	ret = pinctrl_select_state(pins->p, pins->sleep_state);
+	if (ret)
+		dev_err(dev, "failed to activate sleep pinctrl state\n");
+	return ret;
+}
+
+/**
+ * pinctrl_pm_select_idle_state() - select idle pinctrl state for PM
+ * @dev: device to select idle state for
+ */
+int pinctrl_pm_select_idle_state(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+	if (IS_ERR(pins->idle_state))
+		return 0; /* No default state */
+	ret = pinctrl_select_state(pins->p, pins->idle_state);
+	if (ret)
+		dev_err(dev, "failed to activate idle pinctrl state\n");
+	return ret;
+}
+
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 
 static int pinctrl_pins_show(struct seq_file *s, void *what)
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 4aad3ce..0f32f10 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -40,6 +40,25 @@ extern int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *s);
 extern struct pinctrl * __must_check devm_pinctrl_get(struct device *dev);
 extern void devm_pinctrl_put(struct pinctrl *p);
 
+#ifdef CONFIG_PM
+extern int pinctrl_pm_select_default_state(struct device *dev);
+extern int pinctrl_pm_select_sleep_state(struct device *dev);
+extern int pinctrl_pm_select_idle_state(struct device *dev);
+#else
+static inline int pinctrl_pm_select_default_state(struct device *dev)
+{
+	return 0;
+}
+static inline int pinctrl_pm_select_sleep_state(struct device *dev)
+{
+	return 0;
+}
+static inline int pinctrl_pm_select_idle_state(struct device *dev)
+{
+	return 0;
+}
+#endif
+
 #else /* !CONFIG_PINCTRL */
 
 static inline int pinctrl_request_gpio(unsigned gpio)
@@ -199,6 +218,21 @@ static inline int pin_config_group_set(const char *dev_name,
 	return 0;
 }
 
+static inline int pinctrl_pm_select_default_state(struct device *dev)
+{
+	return 0;
+}
+
+static inline int pinctrl_pm_select_sleep_state(struct device *dev)
+{
+	return 0;
+}
+
+static inline int pinctrl_pm_select_idle_state(struct device *dev)
+{
+	return 0;
+}
+
 #endif
 
 #endif /* __LINUX_PINCTRL_CONSUMER_H */
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index 6e5f8a9..281cb91 100644
--- a/include/linux/pinctrl/devinfo.h
+++ b/include/linux/pinctrl/devinfo.h
@@ -28,6 +28,10 @@
 struct dev_pin_info {
 	struct pinctrl *p;
 	struct pinctrl_state *default_state;
+#ifdef CONFIG_PM
+	struct pinctrl_state *sleep_state;
+	struct pinctrl_state *idle_state;
+#endif
 };
 
 extern int pinctrl_bind_pins(struct device *dev);
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/3] tty: serial: modify PL011 driver to use pinctrl PM helpers
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 13:44   ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-05 13:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman
  Cc: linux-kernel, linux-arm-kernel, Linus Walleij, Hebbar Gururaja,
	Mark Brown, Dmitry Torokhov, Stephen Warren, Wolfram Sang

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

This augments the PL011 UART driver to utilize the new pinctrl
core PM helpers to transition the driver to default and sleep
states, cutting away some boilerplate code.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Greg's ACK on this as well so we can take the
whole refactoring through the pinctrl tree.
---
 drivers/tty/serial/amba-pl011.c | 42 +++--------------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e2774f9..6ac3254 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -151,10 +151,6 @@ struct pl011_dmatx_data {
 struct uart_amba_port {
 	struct uart_port	port;
 	struct clk		*clk;
-	/* Two optional pin states - default & sleep */
-	struct pinctrl		*pinctrl;
-	struct pinctrl_state	*pins_default;
-	struct pinctrl_state	*pins_sleep;
 	const struct vendor_data *vendor;
 	unsigned int		dmacr;		/* dma control reg */
 	unsigned int		im;		/* interrupt mask */
@@ -1480,12 +1476,7 @@ static int pl011_hwinit(struct uart_port *port)
 	int retval;
 
 	/* Optionaly enable pins to be muxed in and configured */
-	if (!IS_ERR(uap->pins_default)) {
-		retval = pinctrl_select_state(uap->pinctrl, uap->pins_default);
-		if (retval)
-			dev_err(port->dev,
-				"could not set default pins\n");
-	}
+	pinctrl_pm_select_default_state(port->dev);
 
 	/*
 	 * Try to enable the clock producer.
@@ -1611,7 +1602,6 @@ static void pl011_shutdown(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
 	unsigned int cr;
-	int retval;
 
 	/*
 	 * disable all interrupts
@@ -1654,13 +1644,7 @@ static void pl011_shutdown(struct uart_port *port)
 	 */
 	clk_disable_unprepare(uap->clk);
 	/* Optionally let pins go into sleep states */
-	if (!IS_ERR(uap->pins_sleep)) {
-		retval = pinctrl_select_state(uap->pinctrl, uap->pins_sleep);
-		if (retval)
-			dev_err(port->dev,
-				"could not set pins to sleep state\n");
-	}
-
+	pinctrl_pm_select_sleep_state(port->dev);
 
 	if (uap->port.dev->platform_data) {
 		struct amba_pl011_data *plat;
@@ -2013,12 +1997,7 @@ static int __init pl011_console_setup(struct console *co, char *options)
 		return -ENODEV;
 
 	/* Allow pins to be muxed in and configured */
-	if (!IS_ERR(uap->pins_default)) {
-		ret = pinctrl_select_state(uap->pinctrl, uap->pins_default);
-		if (ret)
-			dev_err(uap->port.dev,
-				"could not set default pins\n");
-	}
+	pinctrl_pm_select_default_state(uap->port.dev);
 
 	ret = clk_prepare(uap->clk);
 	if (ret)
@@ -2132,21 +2111,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 		goto out;
 	}
 
-	uap->pinctrl = devm_pinctrl_get(&dev->dev);
-	if (IS_ERR(uap->pinctrl)) {
-		ret = PTR_ERR(uap->pinctrl);
-		goto out;
-	}
-	uap->pins_default = pinctrl_lookup_state(uap->pinctrl,
-						 PINCTRL_STATE_DEFAULT);
-	if (IS_ERR(uap->pins_default))
-		dev_err(&dev->dev, "could not get default pinstate\n");
-
-	uap->pins_sleep = pinctrl_lookup_state(uap->pinctrl,
-					       PINCTRL_STATE_SLEEP);
-	if (IS_ERR(uap->pins_sleep))
-		dev_dbg(&dev->dev, "could not get sleep pinstate\n");
-
 	uap->clk = devm_clk_get(&dev->dev, NULL);
 	if (IS_ERR(uap->clk)) {
 		ret = PTR_ERR(uap->clk);
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/3] tty: serial: modify PL011 driver to use pinctrl PM helpers
@ 2013-06-05 13:44   ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-05 13:44 UTC (permalink / raw)
  To: linux-arm-kernel

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

This augments the PL011 UART driver to utilize the new pinctrl
core PM helpers to transition the driver to default and sleep
states, cutting away some boilerplate code.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Greg's ACK on this as well so we can take the
whole refactoring through the pinctrl tree.
---
 drivers/tty/serial/amba-pl011.c | 42 +++--------------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e2774f9..6ac3254 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -151,10 +151,6 @@ struct pl011_dmatx_data {
 struct uart_amba_port {
 	struct uart_port	port;
 	struct clk		*clk;
-	/* Two optional pin states - default & sleep */
-	struct pinctrl		*pinctrl;
-	struct pinctrl_state	*pins_default;
-	struct pinctrl_state	*pins_sleep;
 	const struct vendor_data *vendor;
 	unsigned int		dmacr;		/* dma control reg */
 	unsigned int		im;		/* interrupt mask */
@@ -1480,12 +1476,7 @@ static int pl011_hwinit(struct uart_port *port)
 	int retval;
 
 	/* Optionaly enable pins to be muxed in and configured */
-	if (!IS_ERR(uap->pins_default)) {
-		retval = pinctrl_select_state(uap->pinctrl, uap->pins_default);
-		if (retval)
-			dev_err(port->dev,
-				"could not set default pins\n");
-	}
+	pinctrl_pm_select_default_state(port->dev);
 
 	/*
 	 * Try to enable the clock producer.
@@ -1611,7 +1602,6 @@ static void pl011_shutdown(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
 	unsigned int cr;
-	int retval;
 
 	/*
 	 * disable all interrupts
@@ -1654,13 +1644,7 @@ static void pl011_shutdown(struct uart_port *port)
 	 */
 	clk_disable_unprepare(uap->clk);
 	/* Optionally let pins go into sleep states */
-	if (!IS_ERR(uap->pins_sleep)) {
-		retval = pinctrl_select_state(uap->pinctrl, uap->pins_sleep);
-		if (retval)
-			dev_err(port->dev,
-				"could not set pins to sleep state\n");
-	}
-
+	pinctrl_pm_select_sleep_state(port->dev);
 
 	if (uap->port.dev->platform_data) {
 		struct amba_pl011_data *plat;
@@ -2013,12 +1997,7 @@ static int __init pl011_console_setup(struct console *co, char *options)
 		return -ENODEV;
 
 	/* Allow pins to be muxed in and configured */
-	if (!IS_ERR(uap->pins_default)) {
-		ret = pinctrl_select_state(uap->pinctrl, uap->pins_default);
-		if (ret)
-			dev_err(uap->port.dev,
-				"could not set default pins\n");
-	}
+	pinctrl_pm_select_default_state(uap->port.dev);
 
 	ret = clk_prepare(uap->clk);
 	if (ret)
@@ -2132,21 +2111,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 		goto out;
 	}
 
-	uap->pinctrl = devm_pinctrl_get(&dev->dev);
-	if (IS_ERR(uap->pinctrl)) {
-		ret = PTR_ERR(uap->pinctrl);
-		goto out;
-	}
-	uap->pins_default = pinctrl_lookup_state(uap->pinctrl,
-						 PINCTRL_STATE_DEFAULT);
-	if (IS_ERR(uap->pins_default))
-		dev_err(&dev->dev, "could not get default pinstate\n");
-
-	uap->pins_sleep = pinctrl_lookup_state(uap->pinctrl,
-					       PINCTRL_STATE_SLEEP);
-	if (IS_ERR(uap->pins_sleep))
-		dev_dbg(&dev->dev, "could not get sleep pinstate\n");
-
 	uap->clk = devm_clk_get(&dev->dev, NULL);
 	if (IS_ERR(uap->clk)) {
 		ret = PTR_ERR(uap->clk);
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 13:44   ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-05 13:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman
  Cc: linux-kernel, linux-arm-kernel, Linus Walleij, Hebbar Gururaja,
	Mark Brown, Dmitry Torokhov, Stephen Warren, Wolfram Sang

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

This utilize the new pinctrl core PM helpers to transition
the driver to "sleep" and "idle" states, cutting away some
boilerplate code.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Wolfram's ACK on this to take it through the
pinctrl tree in the end.
---
 drivers/i2c/busses/i2c-nomadik.c | 90 +++++-----------------------------------
 1 file changed, 10 insertions(+), 80 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 650293f..c7e3b0c 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -148,10 +148,6 @@ struct i2c_nmk_client {
  * @stop: stop condition.
  * @xfer_complete: acknowledge completion for a I2C message.
  * @result: controller propogated result.
- * @pinctrl: pinctrl handle.
- * @pins_default: default state for the pins.
- * @pins_idle: idle state for the pins.
- * @pins_sleep: sleep state for the pins.
  * @busy: Busy doing transfer.
  */
 struct nmk_i2c_dev {
@@ -165,11 +161,6 @@ struct nmk_i2c_dev {
 	int				stop;
 	struct completion		xfer_complete;
 	int				result;
-	/* Three pin states - default, idle & sleep */
-	struct pinctrl			*pinctrl;
-	struct pinctrl_state		*pins_default;
-	struct pinctrl_state		*pins_idle;
-	struct pinctrl_state		*pins_sleep;
 	bool				busy;
 };
 
@@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	}
 
 	/* Optionaly enable pins to be muxed in and configured */
-	if (!IS_ERR(dev->pins_default)) {
-		status = pinctrl_select_state(dev->pinctrl,
-				dev->pins_default);
-		if (status)
-			dev_err(&dev->adev->dev,
-				"could not set default pins\n");
-	}
+	pinctrl_pm_select_default_state(&dev->adev->dev);
 
 	status = init_hw(dev);
 	if (status)
@@ -681,13 +666,7 @@ out:
 	clk_disable_unprepare(dev->clk);
 out_clk:
 	/* Optionally let pins go into idle state */
-	if (!IS_ERR(dev->pins_idle)) {
-		status = pinctrl_select_state(dev->pinctrl,
-				dev->pins_idle);
-		if (status)
-			dev_err(&dev->adev->dev,
-				"could not set pins to idle state\n");
-	}
+	pinctrl_pm_select_idle_state(&dev->adev->dev);
 
 	pm_runtime_put_sync(&dev->adev->dev);
 
@@ -882,41 +861,22 @@ static int nmk_i2c_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
 	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
-	int ret;
 
 	if (nmk_i2c->busy)
 		return -EBUSY;
 
-	if (!IS_ERR(nmk_i2c->pins_sleep)) {
-		ret = pinctrl_select_state(nmk_i2c->pinctrl,
-				nmk_i2c->pins_sleep);
-		if (ret)
-			dev_err(dev, "could not set pins to sleep state\n");
-	}
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
 static int nmk_i2c_resume(struct device *dev)
 {
-	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
-	int ret;
-
 	/* First go to the default state */
-	if (!IS_ERR(nmk_i2c->pins_default)) {
-		ret = pinctrl_select_state(nmk_i2c->pinctrl,
-				nmk_i2c->pins_default);
-		if (ret)
-			dev_err(dev, "could not set pins to default state\n");
-	}
+	pinctrl_pm_select_default_state(dev);
 	/* Then let's idle the pins until the next transfer happens */
-	if (!IS_ERR(nmk_i2c->pins_idle)) {
-		ret = pinctrl_select_state(nmk_i2c->pinctrl,
-				nmk_i2c->pins_idle);
-		if (ret)
-			dev_err(dev, "could not set pins to idle state\n");
-	}
+	pinctrl_pm_select_idle_state(dev);
+
 	return 0;
 }
 #else
@@ -1004,39 +964,10 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	dev->adev = adev;
 	amba_set_drvdata(adev, dev);
 
-	dev->pinctrl = devm_pinctrl_get(&adev->dev);
-	if (IS_ERR(dev->pinctrl)) {
-		ret = PTR_ERR(dev->pinctrl);
-		goto err_pinctrl;
-	}
-
-	dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
-						 PINCTRL_STATE_DEFAULT);
-	if (IS_ERR(dev->pins_default)) {
-		dev_err(&adev->dev, "could not get default pinstate\n");
-	} else {
-		ret = pinctrl_select_state(dev->pinctrl,
-					   dev->pins_default);
-		if (ret)
-			dev_dbg(&adev->dev, "could not set default pinstate\n");
-	}
-
-	dev->pins_idle = pinctrl_lookup_state(dev->pinctrl,
-					      PINCTRL_STATE_IDLE);
-	if (IS_ERR(dev->pins_idle)) {
-		dev_dbg(&adev->dev, "could not get idle pinstate\n");
-	} else {
-		/* If possible, let's go to idle until the first transfer */
-		ret = pinctrl_select_state(dev->pinctrl,
-					   dev->pins_idle);
-		if (ret)
-			dev_dbg(&adev->dev, "could not set idle pinstate\n");
-	}
-
-	dev->pins_sleep = pinctrl_lookup_state(dev->pinctrl,
-					       PINCTRL_STATE_SLEEP);
-	if (IS_ERR(dev->pins_sleep))
-		dev_dbg(&adev->dev, "could not get sleep pinstate\n");
+	/* Select default pin state */
+	pinctrl_pm_select_default_state(&adev->dev);
+	/* If possible, let's go to idle until the first transfer */
+	pinctrl_pm_select_idle_state(&adev->dev);
 
 	dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
 	if (!dev->virtbase) {
@@ -1106,7 +1037,6 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	iounmap(dev->virtbase);
  err_no_ioremap:
 	kfree(dev);
- err_pinctrl:
  err_no_mem:
 
 	return ret;
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
@ 2013-06-05 13:44   ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-05 13:44 UTC (permalink / raw)
  To: linux-arm-kernel

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

This utilize the new pinctrl core PM helpers to transition
the driver to "sleep" and "idle" states, cutting away some
boilerplate code.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Wolfram's ACK on this to take it through the
pinctrl tree in the end.
---
 drivers/i2c/busses/i2c-nomadik.c | 90 +++++-----------------------------------
 1 file changed, 10 insertions(+), 80 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 650293f..c7e3b0c 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -148,10 +148,6 @@ struct i2c_nmk_client {
  * @stop: stop condition.
  * @xfer_complete: acknowledge completion for a I2C message.
  * @result: controller propogated result.
- * @pinctrl: pinctrl handle.
- * @pins_default: default state for the pins.
- * @pins_idle: idle state for the pins.
- * @pins_sleep: sleep state for the pins.
  * @busy: Busy doing transfer.
  */
 struct nmk_i2c_dev {
@@ -165,11 +161,6 @@ struct nmk_i2c_dev {
 	int				stop;
 	struct completion		xfer_complete;
 	int				result;
-	/* Three pin states - default, idle & sleep */
-	struct pinctrl			*pinctrl;
-	struct pinctrl_state		*pins_default;
-	struct pinctrl_state		*pins_idle;
-	struct pinctrl_state		*pins_sleep;
 	bool				busy;
 };
 
@@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	}
 
 	/* Optionaly enable pins to be muxed in and configured */
-	if (!IS_ERR(dev->pins_default)) {
-		status = pinctrl_select_state(dev->pinctrl,
-				dev->pins_default);
-		if (status)
-			dev_err(&dev->adev->dev,
-				"could not set default pins\n");
-	}
+	pinctrl_pm_select_default_state(&dev->adev->dev);
 
 	status = init_hw(dev);
 	if (status)
@@ -681,13 +666,7 @@ out:
 	clk_disable_unprepare(dev->clk);
 out_clk:
 	/* Optionally let pins go into idle state */
-	if (!IS_ERR(dev->pins_idle)) {
-		status = pinctrl_select_state(dev->pinctrl,
-				dev->pins_idle);
-		if (status)
-			dev_err(&dev->adev->dev,
-				"could not set pins to idle state\n");
-	}
+	pinctrl_pm_select_idle_state(&dev->adev->dev);
 
 	pm_runtime_put_sync(&dev->adev->dev);
 
@@ -882,41 +861,22 @@ static int nmk_i2c_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
 	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
-	int ret;
 
 	if (nmk_i2c->busy)
 		return -EBUSY;
 
-	if (!IS_ERR(nmk_i2c->pins_sleep)) {
-		ret = pinctrl_select_state(nmk_i2c->pinctrl,
-				nmk_i2c->pins_sleep);
-		if (ret)
-			dev_err(dev, "could not set pins to sleep state\n");
-	}
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
 static int nmk_i2c_resume(struct device *dev)
 {
-	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
-	int ret;
-
 	/* First go to the default state */
-	if (!IS_ERR(nmk_i2c->pins_default)) {
-		ret = pinctrl_select_state(nmk_i2c->pinctrl,
-				nmk_i2c->pins_default);
-		if (ret)
-			dev_err(dev, "could not set pins to default state\n");
-	}
+	pinctrl_pm_select_default_state(dev);
 	/* Then let's idle the pins until the next transfer happens */
-	if (!IS_ERR(nmk_i2c->pins_idle)) {
-		ret = pinctrl_select_state(nmk_i2c->pinctrl,
-				nmk_i2c->pins_idle);
-		if (ret)
-			dev_err(dev, "could not set pins to idle state\n");
-	}
+	pinctrl_pm_select_idle_state(dev);
+
 	return 0;
 }
 #else
@@ -1004,39 +964,10 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	dev->adev = adev;
 	amba_set_drvdata(adev, dev);
 
-	dev->pinctrl = devm_pinctrl_get(&adev->dev);
-	if (IS_ERR(dev->pinctrl)) {
-		ret = PTR_ERR(dev->pinctrl);
-		goto err_pinctrl;
-	}
-
-	dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
-						 PINCTRL_STATE_DEFAULT);
-	if (IS_ERR(dev->pins_default)) {
-		dev_err(&adev->dev, "could not get default pinstate\n");
-	} else {
-		ret = pinctrl_select_state(dev->pinctrl,
-					   dev->pins_default);
-		if (ret)
-			dev_dbg(&adev->dev, "could not set default pinstate\n");
-	}
-
-	dev->pins_idle = pinctrl_lookup_state(dev->pinctrl,
-					      PINCTRL_STATE_IDLE);
-	if (IS_ERR(dev->pins_idle)) {
-		dev_dbg(&adev->dev, "could not get idle pinstate\n");
-	} else {
-		/* If possible, let's go to idle until the first transfer */
-		ret = pinctrl_select_state(dev->pinctrl,
-					   dev->pins_idle);
-		if (ret)
-			dev_dbg(&adev->dev, "could not set idle pinstate\n");
-	}
-
-	dev->pins_sleep = pinctrl_lookup_state(dev->pinctrl,
-					       PINCTRL_STATE_SLEEP);
-	if (IS_ERR(dev->pins_sleep))
-		dev_dbg(&adev->dev, "could not get sleep pinstate\n");
+	/* Select default pin state */
+	pinctrl_pm_select_default_state(&adev->dev);
+	/* If possible, let's go to idle until the first transfer */
+	pinctrl_pm_select_idle_state(&adev->dev);
 
 	dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
 	if (!dev->virtbase) {
@@ -1106,7 +1037,6 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	iounmap(dev->virtbase);
  err_no_ioremap:
 	kfree(dev);
- err_pinctrl:
  err_no_mem:
 
 	return ret;
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 14:03   ` Wolfram Sang
  -1 siblings, 0 replies; 36+ messages in thread
From: Wolfram Sang @ 2013-06-05 14:03 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman, linux-kernel,
	linux-arm-kernel, Linus Walleij, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Stephen Warren

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

On Wed, Jun 05, 2013 at 03:44:31PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
> 
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.
> 
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Some nits:

> +	if (IS_ERR(pins->sleep_state))
> +		return 0; /* No default state */

Comment wants to say "sleep state"?

> +	ret = pinctrl_select_state(pins->p, pins->sleep_state);
> +	if (ret)
> +		dev_err(dev, "failed to activate sleep pinctrl state\n");

Better say "pinctrl sleep state"?

> +	if (IS_ERR(pins->idle_state))
> +		return 0; /* No default state */
> +	ret = pinctrl_select_state(pins->p, pins->idle_state);
> +	if (ret)
> +		dev_err(dev, "failed to activate idle pinctrl state\n");

Similar issues here...

Other than that, on all 3 patches:

Acked-by: Wolfram Sang <wsa@the-dreams.de>


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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 14:03   ` Wolfram Sang
  0 siblings, 0 replies; 36+ messages in thread
From: Wolfram Sang @ 2013-06-05 14:03 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 05, 2013 at 03:44:31PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
> 
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.
> 
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Some nits:

> +	if (IS_ERR(pins->sleep_state))
> +		return 0; /* No default state */

Comment wants to say "sleep state"?

> +	ret = pinctrl_select_state(pins->p, pins->sleep_state);
> +	if (ret)
> +		dev_err(dev, "failed to activate sleep pinctrl state\n");

Better say "pinctrl sleep state"?

> +	if (IS_ERR(pins->idle_state))
> +		return 0; /* No default state */
> +	ret = pinctrl_select_state(pins->p, pins->idle_state);
> +	if (ret)
> +		dev_err(dev, "failed to activate idle pinctrl state\n");

Similar issues here...

Other than that, on all 3 patches:

Acked-by: Wolfram Sang <wsa@the-dreams.de>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130605/6151fdf8/attachment.sig>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 14:47   ` Mark Brown
  -1 siblings, 0 replies; 36+ messages in thread
From: Mark Brown @ 2013-06-05 14:47 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman, linux-kernel,
	linux-arm-kernel, Linus Walleij, Hebbar Gururaja,
	Dmitry Torokhov, Stephen Warren, Wolfram Sang

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

On Wed, Jun 05, 2013 at 03:44:31PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.

Reviewed-by: Mark Brown <broonie@linaro.org>

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 14:47   ` Mark Brown
  0 siblings, 0 replies; 36+ messages in thread
From: Mark Brown @ 2013-06-05 14:47 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 05, 2013 at 03:44:31PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.

Reviewed-by: Mark Brown <broonie@linaro.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130605/dcfa13aa/attachment.sig>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 15:57   ` Kevin Hilman
  -1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2013-06-05 15:57 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Greg Kroah-Hartman, Stephen Warren, linux-kernel,
	linux-arm-kernel, Linus Walleij, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Stephen Warren, Wolfram Sang

Linus Walleij <linus.walleij@stericsson.com> writes:

> From: Linus Walleij <linus.walleij@linaro.org>
>
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
>
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
>
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.
>
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I'm seeking Gregs ACK on this in the end, so we can take this
> in through the pinctrl tree. But first let's review!

Reviewed-by: Kevin Hilman <khilman@linaro.org>


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 15:57   ` Kevin Hilman
  0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2013-06-05 15:57 UTC (permalink / raw)
  To: linux-arm-kernel

Linus Walleij <linus.walleij@stericsson.com> writes:

> From: Linus Walleij <linus.walleij@linaro.org>
>
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
>
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
>
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.
>
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I'm seeking Gregs ACK on this in the end, so we can take this
> in through the pinctrl tree. But first let's review!

Reviewed-by: Kevin Hilman <khilman@linaro.org>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
  2013-06-05 13:44   ` Linus Walleij
@ 2013-06-05 16:34     ` Kevin Hilman
  -1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2013-06-05 16:34 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Greg Kroah-Hartman, Stephen Warren, linux-kernel,
	linux-arm-kernel, Linus Walleij, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Stephen Warren, Wolfram Sang

Linus Walleij <linus.walleij@stericsson.com> writes:

> From: Linus Walleij <linus.walleij@linaro.org>
>
> This utilize the new pinctrl core PM helpers to transition
> the driver to "sleep" and "idle" states, cutting away some
> boilerplate code.
>
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I have some questions on the interaction with runtime PM here...

> ---
> I'm seeking Wolfram's ACK on this to take it through the
> pinctrl tree in the end.
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 90 +++++-----------------------------------
>  1 file changed, 10 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 650293f..c7e3b0c 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -148,10 +148,6 @@ struct i2c_nmk_client {
>   * @stop: stop condition.
>   * @xfer_complete: acknowledge completion for a I2C message.
>   * @result: controller propogated result.
> - * @pinctrl: pinctrl handle.
> - * @pins_default: default state for the pins.
> - * @pins_idle: idle state for the pins.
> - * @pins_sleep: sleep state for the pins.
>   * @busy: Busy doing transfer.
>   */
>  struct nmk_i2c_dev {
> @@ -165,11 +161,6 @@ struct nmk_i2c_dev {
>  	int				stop;
>  	struct completion		xfer_complete;
>  	int				result;
> -	/* Three pin states - default, idle & sleep */
> -	struct pinctrl			*pinctrl;
> -	struct pinctrl_state		*pins_default;
> -	struct pinctrl_state		*pins_idle;
> -	struct pinctrl_state		*pins_sleep;
>  	bool				busy;
>  };
>  
> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	}
>  
>  	/* Optionaly enable pins to be muxed in and configured */
> -	if (!IS_ERR(dev->pins_default)) {
> -		status = pinctrl_select_state(dev->pinctrl,
> -				dev->pins_default);
> -		if (status)
> -			dev_err(&dev->adev->dev,
> -				"could not set default pins\n");
> -	}
> +	pinctrl_pm_select_default_state(&dev->adev->dev);

Shouldn't this be in the ->runtime_resume() callback of the driver (the
original code should've as well.)

IOW, the pinctrl changes only need to happen when the runtime PM
usecount goes from zero to 1.  For any additional users, the device will
already be active and pins already in default state.

I'm not familiar with this HW, and maybe the driver already prevents
multiple users, but for correctness sake (and because others will copy
this), the (re)muxing should be in the runtime PM callback.

Also, IMO, that's further evidence that the pinctrl stuff could (and
probably should) be handled by the PM core.

>  	status = init_hw(dev);
>  	if (status)
> @@ -681,13 +666,7 @@ out:
>  	clk_disable_unprepare(dev->clk);
>  out_clk:
>  	/* Optionally let pins go into idle state */
> -	if (!IS_ERR(dev->pins_idle)) {
> -		status = pinctrl_select_state(dev->pinctrl,
> -				dev->pins_idle);
> -		if (status)
> -			dev_err(&dev->adev->dev,
> -				"could not set pins to idle state\n");
> -	}
> +	pinctrl_pm_select_idle_state(&dev->adev->dev);

Again, if there are multiple users, you're changing the pin states
without knowing if you're the last user or not (again, the problem
existed in the original code as well.)

Runtime PM is doing the usecouning, so this should be in the
->runtime_suspend() callback.

>  	pm_runtime_put_sync(&dev->adev->dev);
>  

Kevin

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
@ 2013-06-05 16:34     ` Kevin Hilman
  0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2013-06-05 16:34 UTC (permalink / raw)
  To: linux-arm-kernel

Linus Walleij <linus.walleij@stericsson.com> writes:

> From: Linus Walleij <linus.walleij@linaro.org>
>
> This utilize the new pinctrl core PM helpers to transition
> the driver to "sleep" and "idle" states, cutting away some
> boilerplate code.
>
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I have some questions on the interaction with runtime PM here...

> ---
> I'm seeking Wolfram's ACK on this to take it through the
> pinctrl tree in the end.
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 90 +++++-----------------------------------
>  1 file changed, 10 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 650293f..c7e3b0c 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -148,10 +148,6 @@ struct i2c_nmk_client {
>   * @stop: stop condition.
>   * @xfer_complete: acknowledge completion for a I2C message.
>   * @result: controller propogated result.
> - * @pinctrl: pinctrl handle.
> - * @pins_default: default state for the pins.
> - * @pins_idle: idle state for the pins.
> - * @pins_sleep: sleep state for the pins.
>   * @busy: Busy doing transfer.
>   */
>  struct nmk_i2c_dev {
> @@ -165,11 +161,6 @@ struct nmk_i2c_dev {
>  	int				stop;
>  	struct completion		xfer_complete;
>  	int				result;
> -	/* Three pin states - default, idle & sleep */
> -	struct pinctrl			*pinctrl;
> -	struct pinctrl_state		*pins_default;
> -	struct pinctrl_state		*pins_idle;
> -	struct pinctrl_state		*pins_sleep;
>  	bool				busy;
>  };
>  
> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	}
>  
>  	/* Optionaly enable pins to be muxed in and configured */
> -	if (!IS_ERR(dev->pins_default)) {
> -		status = pinctrl_select_state(dev->pinctrl,
> -				dev->pins_default);
> -		if (status)
> -			dev_err(&dev->adev->dev,
> -				"could not set default pins\n");
> -	}
> +	pinctrl_pm_select_default_state(&dev->adev->dev);

Shouldn't this be in the ->runtime_resume() callback of the driver (the
original code should've as well.)

IOW, the pinctrl changes only need to happen when the runtime PM
usecount goes from zero to 1.  For any additional users, the device will
already be active and pins already in default state.

I'm not familiar with this HW, and maybe the driver already prevents
multiple users, but for correctness sake (and because others will copy
this), the (re)muxing should be in the runtime PM callback.

Also, IMO, that's further evidence that the pinctrl stuff could (and
probably should) be handled by the PM core.

>  	status = init_hw(dev);
>  	if (status)
> @@ -681,13 +666,7 @@ out:
>  	clk_disable_unprepare(dev->clk);
>  out_clk:
>  	/* Optionally let pins go into idle state */
> -	if (!IS_ERR(dev->pins_idle)) {
> -		status = pinctrl_select_state(dev->pinctrl,
> -				dev->pins_idle);
> -		if (status)
> -			dev_err(&dev->adev->dev,
> -				"could not set pins to idle state\n");
> -	}
> +	pinctrl_pm_select_idle_state(&dev->adev->dev);

Again, if there are multiple users, you're changing the pin states
without knowing if you're the last user or not (again, the problem
existed in the original code as well.)

Runtime PM is doing the usecouning, so this should be in the
->runtime_suspend() callback.

>  	pm_runtime_put_sync(&dev->adev->dev);
>  

Kevin

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 17:22   ` Stephen Warren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stephen Warren @ 2013-06-05 17:22 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman, linux-kernel,
	linux-arm-kernel, Linus Walleij, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Wolfram Sang

On 06/05/2013 07:44 AM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
> 
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.

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

> +int pinctrl_pm_select_default_state(struct device *dev)

> +int pinctrl_pm_select_sleep_state(struct device *dev)

> +int pinctrl_pm_select_idle_state(struct device *dev)

The implementation of those 3 functions is basically identical. I'd be
inclined to move it to a helper function, and just pass (dev,
pins->xxx_state) to it.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 17:22   ` Stephen Warren
  0 siblings, 0 replies; 36+ messages in thread
From: Stephen Warren @ 2013-06-05 17:22 UTC (permalink / raw)
  To: linux-arm-kernel

On 06/05/2013 07:44 AM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
> 
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.

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

> +int pinctrl_pm_select_default_state(struct device *dev)

> +int pinctrl_pm_select_sleep_state(struct device *dev)

> +int pinctrl_pm_select_idle_state(struct device *dev)

The implementation of those 3 functions is basically identical. I'd be
inclined to move it to a helper function, and just pass (dev,
pins->xxx_state) to it.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-05 18:54   ` Greg Kroah-Hartman
  -1 siblings, 0 replies; 36+ messages in thread
From: Greg Kroah-Hartman @ 2013-06-05 18:54 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Stephen Warren, Kevin Hilman, linux-kernel, linux-arm-kernel,
	Linus Walleij, Hebbar Gururaja, Mark Brown, Dmitry Torokhov,
	Stephen Warren, Wolfram Sang

On Wed, Jun 05, 2013 at 03:44:31PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
> 
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.
> 
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I'm seeking Gregs ACK on this in the end, so we can take this
> in through the pinctrl tree. But first let's review!

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-05 18:54   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 36+ messages in thread
From: Greg Kroah-Hartman @ 2013-06-05 18:54 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 05, 2013 at 03:44:31PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> If a device have sleep and idle states in addition to the
> default state, look up these in the core and stash them in
> the pinctrl state container.
> 
> Add accessor functions for pinctrl consumers to put the pins
> into "default", "sleep" and "idle" states passing nothing but
> the struct device * affected.
> 
> Solution suggested by Kevin Hilman, Mark Brown and Dmitry
> Torokhov in response to a patch series from Hebbar
> Gururaja.
> 
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I'm seeking Gregs ACK on this in the end, so we can take this
> in through the pinctrl tree. But first let's review!

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/3] tty: serial: modify PL011 driver to use pinctrl PM helpers
  2013-06-05 13:44   ` Linus Walleij
@ 2013-06-05 18:54     ` Greg Kroah-Hartman
  -1 siblings, 0 replies; 36+ messages in thread
From: Greg Kroah-Hartman @ 2013-06-05 18:54 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Stephen Warren, Kevin Hilman, linux-kernel, linux-arm-kernel,
	Linus Walleij, Hebbar Gururaja, Mark Brown, Dmitry Torokhov,
	Stephen Warren, Wolfram Sang

On Wed, Jun 05, 2013 at 03:44:32PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This augments the PL011 UART driver to utilize the new pinctrl
> core PM helpers to transition the driver to default and sleep
> states, cutting away some boilerplate code.
> 
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I'm seeking Greg's ACK on this as well so we can take the
> whole refactoring through the pinctrl tree.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 2/3] tty: serial: modify PL011 driver to use pinctrl PM helpers
@ 2013-06-05 18:54     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 36+ messages in thread
From: Greg Kroah-Hartman @ 2013-06-05 18:54 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 05, 2013 at 03:44:32PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This augments the PL011 UART driver to utilize the new pinctrl
> core PM helpers to transition the driver to default and sleep
> states, cutting away some boilerplate code.
> 
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I'm seeking Greg's ACK on this as well so we can take the
> whole refactoring through the pinctrl tree.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 17:22   ` Stephen Warren
@ 2013-06-07  7:53     ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-07  7:53 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Linus Walleij, Greg Kroah-Hartman, Stephen Warren, Kevin Hilman,
	linux-kernel, linux-arm-kernel, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Wolfram Sang

On Wed, Jun 5, 2013 at 7:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>
>> +int pinctrl_pm_select_default_state(struct device *dev)
>
>> +int pinctrl_pm_select_sleep_state(struct device *dev)
>
>> +int pinctrl_pm_select_idle_state(struct device *dev)
>
> The implementation of those 3 functions is basically identical. I'd be
> inclined to move it to a helper function, and just pass (dev,
> pins->xxx_state) to it.

Point taken, but as the comments only affect pinctrl/core.c
and I got so many nice ACKs on the patch, I'll apply this
and think about a refactoring patch only hitting the core
in drivers/pinctrl/* to nice this up. (Need this infrastructure
in place for the OMAP work now I think...)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-07  7:53     ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-07  7:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 5, 2013 at 7:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>
>> +int pinctrl_pm_select_default_state(struct device *dev)
>
>> +int pinctrl_pm_select_sleep_state(struct device *dev)
>
>> +int pinctrl_pm_select_idle_state(struct device *dev)
>
> The implementation of those 3 functions is basically identical. I'd be
> inclined to move it to a helper function, and just pass (dev,
> pins->xxx_state) to it.

Point taken, but as the comments only affect pinctrl/core.c
and I got so many nice ACKs on the patch, I'll apply this
and think about a refactoring patch only hitting the core
in drivers/pinctrl/* to nice this up. (Need this infrastructure
in place for the OMAP work now I think...)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
  2013-06-05 16:34     ` Kevin Hilman
@ 2013-06-07  8:33       ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-07  8:33 UTC (permalink / raw)
  To: Kevin Hilman, Ulf Hansson
  Cc: Linus Walleij, Greg Kroah-Hartman, Stephen Warren, linux-kernel,
	linux-arm-kernel, Hebbar Gururaja, Mark Brown, Dmitry Torokhov,
	Stephen Warren, Wolfram Sang, Russell King - ARM Linux

On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote:

>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This utilize the new pinctrl core PM helpers to transition
>> the driver to "sleep" and "idle" states, cutting away some
>> boilerplate code.
>>
>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> I have some questions on the interaction with runtime PM here...

OK, to get the basic infrastructure in place I have merged these
patches with the I2C maintainers ACK, since it is doing one thing,
i.e. moving the functionality out of the driver and into the pinctrl
core.

I am considering semantic changes related to runtime PM in
addition to this as a separate patch, so let's start talking about
that here.

It would be inappropriate to try to create a patch that is
changing these two things at once, but let's see where we end
up by the merge window.

>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>>       }
>>
>>       /* Optionaly enable pins to be muxed in and configured */
>> -     if (!IS_ERR(dev->pins_default)) {
>> -             status = pinctrl_select_state(dev->pinctrl,
>> -                             dev->pins_default);
>> -             if (status)
>> -                     dev_err(&dev->adev->dev,
>> -                             "could not set default pins\n");
>> -     }
>> +     pinctrl_pm_select_default_state(&dev->adev->dev);
>
> Shouldn't this be in the ->runtime_resume() callback of the driver (the
> original code should've as well.)
>
> IOW, the pinctrl changes only need to happen when the runtime PM
> usecount goes from zero to 1.  For any additional users, the device will
> already be active and pins already in default state.
>
> I'm not familiar with this HW, and maybe the driver already prevents
> multiple users, but for correctness sake (and because others will copy
> this), the (re)muxing should be in the runtime PM callback.

I2C message are serialized/marshalled by nature so it's actually
not causing a concurrency problem: this xfer function will not be
called from two places for the same driver.

What is true however is that we're hammering the pins from
active to idle for every transfer, instead of letting runtime PM
provide some nice hysteresis (autosuspend) around that.

Notice though that:

- This driver has no driver-local runtime PM callbacks, so the
  runtime PM calls are intended to inform the rest of the system,
  such as the bus, that the device is idle.

- The bus used is the AMBA (PrimeCell) bus,
  drivers/amba/bus.c

> Also, IMO, that's further evidence that the pinctrl stuff could (and
> probably should) be handled by the PM core.

So I'm now thinking about how to achieve this.

What happens for this driver when the usecount goes from
1->0 is (the other way is very similar):

drivers/base/power/runtime.c

        if (dev->pm_domain)
                callback = dev->pm_domain->ops.runtime_suspend;
        else if (dev->type && dev->type->pm)
                callback = dev->type->pm->runtime_suspend;
        else if (dev->class && dev->class->pm)
                callback = dev->class->pm->runtime_suspend;
        else if (dev->bus && dev->bus->pm)
                callback = dev->bus->pm->runtime_suspend;
        else
                callback = NULL;

        if (!callback && dev->driver && dev->driver->pm)
                callback = dev->driver->pm->runtime_suspend;

        retval = rpm_callback(callback, dev);

This platform will currently hit dev->bus->pm->runtime_suspend
to drivers/amba/bus.c:

static int amba_pm_runtime_suspend(struct device *dev)
{
        struct amba_device *pcdev = to_amba_device(dev);
        int ret = pm_generic_runtime_suspend(dev);

        if (ret == 0 && dev->driver)
                clk_disable(pcdev->pclk);

        return ret;
}

The pm_generic_runtime_suspend will call the driver callbacks
(none in this case).

Then the bus core proceeds to gate off the block clock and
we're done.

I could make a patch adding runtime PM ops to the
driver itself to set the pinctrl state from there, which would
be a nice improvement in itself.

But we're discussing handling it all in the PM core, so
let's think bigger.

If we're making this all generic, were in this chain do you
suggest that I set the pins to idle?
drivers/base/power/runtime.c?

One thing in particular that worries me here is the ordering
of things, because that has been a severe issue for us.

For example: maybe on a certain platform pins need to
be idled/defaulted *before* calling the PM domain or
bus callbacks are executed, because of transient IRQs
and whatnot. So I put my pinctrl_pm_select_idle_state()
*before* the chain of calls.

But sometimes you may want to execute the
pinctrl_pm_select_idle_state() *after* all other things have
been done, including the calls to the domain/bus/driver.

And this is only for the runtime suspend/resume path.

For the common suspend/resume path things get more
complex still. Users may need to call
pinctrl_pm_select_sleep_state() in the middle of the
code sending the platform done, and will not survive it
being called by the PM core, and we'd need to add a flag
for this etc.

To sum up I am afraid of a can of worms of corner cases
on something that looks simple here. Thus I cannot really
make a patch moving pinctrl state selection to the PM
core, I don't know the business there well enough, I just know
there are tigers in there :-/

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
@ 2013-06-07  8:33       ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-07  8:33 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote:

>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This utilize the new pinctrl core PM helpers to transition
>> the driver to "sleep" and "idle" states, cutting away some
>> boilerplate code.
>>
>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> I have some questions on the interaction with runtime PM here...

OK, to get the basic infrastructure in place I have merged these
patches with the I2C maintainers ACK, since it is doing one thing,
i.e. moving the functionality out of the driver and into the pinctrl
core.

I am considering semantic changes related to runtime PM in
addition to this as a separate patch, so let's start talking about
that here.

It would be inappropriate to try to create a patch that is
changing these two things at once, but let's see where we end
up by the merge window.

>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>>       }
>>
>>       /* Optionaly enable pins to be muxed in and configured */
>> -     if (!IS_ERR(dev->pins_default)) {
>> -             status = pinctrl_select_state(dev->pinctrl,
>> -                             dev->pins_default);
>> -             if (status)
>> -                     dev_err(&dev->adev->dev,
>> -                             "could not set default pins\n");
>> -     }
>> +     pinctrl_pm_select_default_state(&dev->adev->dev);
>
> Shouldn't this be in the ->runtime_resume() callback of the driver (the
> original code should've as well.)
>
> IOW, the pinctrl changes only need to happen when the runtime PM
> usecount goes from zero to 1.  For any additional users, the device will
> already be active and pins already in default state.
>
> I'm not familiar with this HW, and maybe the driver already prevents
> multiple users, but for correctness sake (and because others will copy
> this), the (re)muxing should be in the runtime PM callback.

I2C message are serialized/marshalled by nature so it's actually
not causing a concurrency problem: this xfer function will not be
called from two places for the same driver.

What is true however is that we're hammering the pins from
active to idle for every transfer, instead of letting runtime PM
provide some nice hysteresis (autosuspend) around that.

Notice though that:

- This driver has no driver-local runtime PM callbacks, so the
  runtime PM calls are intended to inform the rest of the system,
  such as the bus, that the device is idle.

- The bus used is the AMBA (PrimeCell) bus,
  drivers/amba/bus.c

> Also, IMO, that's further evidence that the pinctrl stuff could (and
> probably should) be handled by the PM core.

So I'm now thinking about how to achieve this.

What happens for this driver when the usecount goes from
1->0 is (the other way is very similar):

drivers/base/power/runtime.c

        if (dev->pm_domain)
                callback = dev->pm_domain->ops.runtime_suspend;
        else if (dev->type && dev->type->pm)
                callback = dev->type->pm->runtime_suspend;
        else if (dev->class && dev->class->pm)
                callback = dev->class->pm->runtime_suspend;
        else if (dev->bus && dev->bus->pm)
                callback = dev->bus->pm->runtime_suspend;
        else
                callback = NULL;

        if (!callback && dev->driver && dev->driver->pm)
                callback = dev->driver->pm->runtime_suspend;

        retval = rpm_callback(callback, dev);

This platform will currently hit dev->bus->pm->runtime_suspend
to drivers/amba/bus.c:

static int amba_pm_runtime_suspend(struct device *dev)
{
        struct amba_device *pcdev = to_amba_device(dev);
        int ret = pm_generic_runtime_suspend(dev);

        if (ret == 0 && dev->driver)
                clk_disable(pcdev->pclk);

        return ret;
}

The pm_generic_runtime_suspend will call the driver callbacks
(none in this case).

Then the bus core proceeds to gate off the block clock and
we're done.

I could make a patch adding runtime PM ops to the
driver itself to set the pinctrl state from there, which would
be a nice improvement in itself.

But we're discussing handling it all in the PM core, so
let's think bigger.

If we're making this all generic, were in this chain do you
suggest that I set the pins to idle?
drivers/base/power/runtime.c?

One thing in particular that worries me here is the ordering
of things, because that has been a severe issue for us.

For example: maybe on a certain platform pins need to
be idled/defaulted *before* calling the PM domain or
bus callbacks are executed, because of transient IRQs
and whatnot. So I put my pinctrl_pm_select_idle_state()
*before* the chain of calls.

But sometimes you may want to execute the
pinctrl_pm_select_idle_state() *after* all other things have
been done, including the calls to the domain/bus/driver.

And this is only for the runtime suspend/resume path.

For the common suspend/resume path things get more
complex still. Users may need to call
pinctrl_pm_select_sleep_state() in the middle of the
code sending the platform done, and will not survive it
being called by the PM core, and we'd need to add a flag
for this etc.

To sum up I am afraid of a can of worms of corner cases
on something that looks simple here. Thus I cannot really
make a patch moving pinctrl state selection to the PM
core, I don't know the business there well enough, I just know
there are tigers in there :-/

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
  2013-06-07  8:33       ` Linus Walleij
@ 2013-06-07 14:31         ` Kevin Hilman
  -1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2013-06-07 14:31 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Ulf Hansson, Linus Walleij, Greg Kroah-Hartman, Stephen Warren,
	linux-kernel, linux-arm-kernel, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Stephen Warren, Wolfram Sang,
	Russell King - ARM Linux, Grygorii Strashko

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

> On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote:
>
>>> From: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> This utilize the new pinctrl core PM helpers to transition
>>> the driver to "sleep" and "idle" states, cutting away some
>>> boilerplate code.
>>>
>>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
>>> Cc: Mark Brown <broonie@kernel.org>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Cc: Kevin Hilman <khilman@linaro.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> I have some questions on the interaction with runtime PM here...
>
> OK, to get the basic infrastructure in place I have merged these
> patches with the I2C maintainers ACK, since it is doing one thing,
> i.e. moving the functionality out of the driver and into the pinctrl
> core.
>
> I am considering semantic changes related to runtime PM in
> addition to this as a separate patch, so let's start talking about
> that here.
>
> It would be inappropriate to try to create a patch that is
> changing these two things at once, but let's see where we end
> up by the merge window.
>
>>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>       }
>>>
>>>       /* Optionaly enable pins to be muxed in and configured */
>>> -     if (!IS_ERR(dev->pins_default)) {
>>> -             status = pinctrl_select_state(dev->pinctrl,
>>> -                             dev->pins_default);
>>> -             if (status)
>>> -                     dev_err(&dev->adev->dev,
>>> -                             "could not set default pins\n");
>>> -     }
>>> +     pinctrl_pm_select_default_state(&dev->adev->dev);
>>
>> Shouldn't this be in the ->runtime_resume() callback of the driver (the
>> original code should've as well.)
>>
>> IOW, the pinctrl changes only need to happen when the runtime PM
>> usecount goes from zero to 1.  For any additional users, the device will
>> already be active and pins already in default state.
>>
>> I'm not familiar with this HW, and maybe the driver already prevents
>> multiple users, but for correctness sake (and because others will copy
>> this), the (re)muxing should be in the runtime PM callback.
>
> I2C message are serialized/marshalled by nature so it's actually
> not causing a concurrency problem: this xfer function will not be
> called from two places for the same driver.
>
> What is true however is that we're hammering the pins from
> active to idle for every transfer, instead of letting runtime PM
> provide some nice hysteresis (autosuspend) around that.
>
> Notice though that:
>
> - This driver has no driver-local runtime PM callbacks, so the
>   runtime PM calls are intended to inform the rest of the system,
>   such as the bus, that the device is idle.
>
> - The bus used is the AMBA (PrimeCell) bus,
>   drivers/amba/bus.c
>
>> Also, IMO, that's further evidence that the pinctrl stuff could (and
>> probably should) be handled by the PM core.
>
> So I'm now thinking about how to achieve this.
>
> What happens for this driver when the usecount goes from
> 1->0 is (the other way is very similar):
>
> drivers/base/power/runtime.c
>
>         if (dev->pm_domain)
>                 callback = dev->pm_domain->ops.runtime_suspend;
>         else if (dev->type && dev->type->pm)
>                 callback = dev->type->pm->runtime_suspend;
>         else if (dev->class && dev->class->pm)
>                 callback = dev->class->pm->runtime_suspend;
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->runtime_suspend;
>         else
>                 callback = NULL;
>
>         if (!callback && dev->driver && dev->driver->pm)
>                 callback = dev->driver->pm->runtime_suspend;
>
>         retval = rpm_callback(callback, dev);
>
> This platform will currently hit dev->bus->pm->runtime_suspend
> to drivers/amba/bus.c:
>
> static int amba_pm_runtime_suspend(struct device *dev)
> {
>         struct amba_device *pcdev = to_amba_device(dev);
>         int ret = pm_generic_runtime_suspend(dev);
>
>         if (ret == 0 && dev->driver)
>                 clk_disable(pcdev->pclk);
>
>         return ret;
> }
>
> The pm_generic_runtime_suspend will call the driver callbacks
> (none in this case).
>
> Then the bus core proceeds to gate off the block clock and
> we're done.
>
> I could make a patch adding runtime PM ops to the
> driver itself to set the pinctrl state from there, which would
> be a nice improvement in itself.
>
> But we're discussing handling it all in the PM core, so
> let's think bigger.
>
> If we're making this all generic, were in this chain do you
> suggest that I set the pins to idle?
> drivers/base/power/runtime.c?

Yes, that's where I was thinking.

> One thing in particular that worries me here is the ordering
> of things, because that has been a severe issue for us.

In the original series from Hebbar Gururaja, Grygorii Strashko pointed
out that we may have some of the ordering issues on OMAP as well.  I
hadn't thought about that (enough) yet.

> For example: maybe on a certain platform pins need to
> be idled/defaulted *before* calling the PM domain or
> bus callbacks are executed, because of transient IRQs
> and whatnot. So I put my pinctrl_pm_select_idle_state()
> *before* the chain of calls.

Right, that corresponds to the runtime PM core calling the drivers
->runtime_suspend() callback before the subsystem/bus/domain has done
its thing.

> But sometimes you may want to execute the
> pinctrl_pm_select_idle_state() *after* all other things have
> been done, including the calls to the domain/bus/driver.

Whether it's in the PM core or not, with runtime PM today, there is no
easy way to do this from the driver ($SUBJECT patch assumes a single
user, which is not true in general.)

The only way a driver truly knows that the domain/bus/subsystem calls
have been done is when its own callback is called, and for suspend this
only happens *before* the device is actually idled.  This is effectively
a pre-runtime_suspend callback.  We don't currently have a
post-runtime_suspend callback (or a pre-runtime_resume callback.)  

Might be we need something like that to do this in a generic way.

> And this is only for the runtime suspend/resume path.
>
> For the common suspend/resume path things get more
> complex still. Users may need to call
> pinctrl_pm_select_sleep_state() in the middle of the
> code sending the platform done, and will not survive it
> being called by the PM core, and we'd need to add a flag
> for this etc.
>
> To sum up I am afraid of a can of worms of corner cases
> on something that looks simple here. Thus I cannot really
> make a patch moving pinctrl state selection to the PM
> core, I don't know the business there well enough, I just know
> there are tigers in there :-/

Yeah, the static PM case is definitely a can of worms, especially in the
case where many devices are already runtime suspended.  That defnitely
needs some serious thought and testing before handling in the PM core.

So for now, it's probably not a good idea to move things to the PM core
until we see a strong pattern emerging.

That being said, maybe we could handle this in the
subsystem/bus/pm_domain level though (in your case AMBA, in OMAP case
the pm_domain) to still avoid sprinkling this across all the drivers?

Kevin


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 3/3] i2c: nomadik: use pinctrl PM helpers
@ 2013-06-07 14:31         ` Kevin Hilman
  0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2013-06-07 14:31 UTC (permalink / raw)
  To: linux-arm-kernel

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

> On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote:
>
>>> From: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> This utilize the new pinctrl core PM helpers to transition
>>> the driver to "sleep" and "idle" states, cutting away some
>>> boilerplate code.
>>>
>>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
>>> Cc: Mark Brown <broonie@kernel.org>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Cc: Kevin Hilman <khilman@linaro.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> I have some questions on the interaction with runtime PM here...
>
> OK, to get the basic infrastructure in place I have merged these
> patches with the I2C maintainers ACK, since it is doing one thing,
> i.e. moving the functionality out of the driver and into the pinctrl
> core.
>
> I am considering semantic changes related to runtime PM in
> addition to this as a separate patch, so let's start talking about
> that here.
>
> It would be inappropriate to try to create a patch that is
> changing these two things at once, but let's see where we end
> up by the merge window.
>
>>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>       }
>>>
>>>       /* Optionaly enable pins to be muxed in and configured */
>>> -     if (!IS_ERR(dev->pins_default)) {
>>> -             status = pinctrl_select_state(dev->pinctrl,
>>> -                             dev->pins_default);
>>> -             if (status)
>>> -                     dev_err(&dev->adev->dev,
>>> -                             "could not set default pins\n");
>>> -     }
>>> +     pinctrl_pm_select_default_state(&dev->adev->dev);
>>
>> Shouldn't this be in the ->runtime_resume() callback of the driver (the
>> original code should've as well.)
>>
>> IOW, the pinctrl changes only need to happen when the runtime PM
>> usecount goes from zero to 1.  For any additional users, the device will
>> already be active and pins already in default state.
>>
>> I'm not familiar with this HW, and maybe the driver already prevents
>> multiple users, but for correctness sake (and because others will copy
>> this), the (re)muxing should be in the runtime PM callback.
>
> I2C message are serialized/marshalled by nature so it's actually
> not causing a concurrency problem: this xfer function will not be
> called from two places for the same driver.
>
> What is true however is that we're hammering the pins from
> active to idle for every transfer, instead of letting runtime PM
> provide some nice hysteresis (autosuspend) around that.
>
> Notice though that:
>
> - This driver has no driver-local runtime PM callbacks, so the
>   runtime PM calls are intended to inform the rest of the system,
>   such as the bus, that the device is idle.
>
> - The bus used is the AMBA (PrimeCell) bus,
>   drivers/amba/bus.c
>
>> Also, IMO, that's further evidence that the pinctrl stuff could (and
>> probably should) be handled by the PM core.
>
> So I'm now thinking about how to achieve this.
>
> What happens for this driver when the usecount goes from
> 1->0 is (the other way is very similar):
>
> drivers/base/power/runtime.c
>
>         if (dev->pm_domain)
>                 callback = dev->pm_domain->ops.runtime_suspend;
>         else if (dev->type && dev->type->pm)
>                 callback = dev->type->pm->runtime_suspend;
>         else if (dev->class && dev->class->pm)
>                 callback = dev->class->pm->runtime_suspend;
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->runtime_suspend;
>         else
>                 callback = NULL;
>
>         if (!callback && dev->driver && dev->driver->pm)
>                 callback = dev->driver->pm->runtime_suspend;
>
>         retval = rpm_callback(callback, dev);
>
> This platform will currently hit dev->bus->pm->runtime_suspend
> to drivers/amba/bus.c:
>
> static int amba_pm_runtime_suspend(struct device *dev)
> {
>         struct amba_device *pcdev = to_amba_device(dev);
>         int ret = pm_generic_runtime_suspend(dev);
>
>         if (ret == 0 && dev->driver)
>                 clk_disable(pcdev->pclk);
>
>         return ret;
> }
>
> The pm_generic_runtime_suspend will call the driver callbacks
> (none in this case).
>
> Then the bus core proceeds to gate off the block clock and
> we're done.
>
> I could make a patch adding runtime PM ops to the
> driver itself to set the pinctrl state from there, which would
> be a nice improvement in itself.
>
> But we're discussing handling it all in the PM core, so
> let's think bigger.
>
> If we're making this all generic, were in this chain do you
> suggest that I set the pins to idle?
> drivers/base/power/runtime.c?

Yes, that's where I was thinking.

> One thing in particular that worries me here is the ordering
> of things, because that has been a severe issue for us.

In the original series from Hebbar Gururaja, Grygorii Strashko pointed
out that we may have some of the ordering issues on OMAP as well.  I
hadn't thought about that (enough) yet.

> For example: maybe on a certain platform pins need to
> be idled/defaulted *before* calling the PM domain or
> bus callbacks are executed, because of transient IRQs
> and whatnot. So I put my pinctrl_pm_select_idle_state()
> *before* the chain of calls.

Right, that corresponds to the runtime PM core calling the drivers
->runtime_suspend() callback before the subsystem/bus/domain has done
its thing.

> But sometimes you may want to execute the
> pinctrl_pm_select_idle_state() *after* all other things have
> been done, including the calls to the domain/bus/driver.

Whether it's in the PM core or not, with runtime PM today, there is no
easy way to do this from the driver ($SUBJECT patch assumes a single
user, which is not true in general.)

The only way a driver truly knows that the domain/bus/subsystem calls
have been done is when its own callback is called, and for suspend this
only happens *before* the device is actually idled.  This is effectively
a pre-runtime_suspend callback.  We don't currently have a
post-runtime_suspend callback (or a pre-runtime_resume callback.)  

Might be we need something like that to do this in a generic way.

> And this is only for the runtime suspend/resume path.
>
> For the common suspend/resume path things get more
> complex still. Users may need to call
> pinctrl_pm_select_sleep_state() in the middle of the
> code sending the platform done, and will not survive it
> being called by the PM core, and we'd need to add a flag
> for this etc.
>
> To sum up I am afraid of a can of worms of corner cases
> on something that looks simple here. Thus I cannot really
> make a patch moving pinctrl state selection to the PM
> core, I don't know the business there well enough, I just know
> there are tigers in there :-/

Yeah, the static PM case is definitely a can of worms, especially in the
case where many devices are already runtime suspended.  That defnitely
needs some serious thought and testing before handling in the PM core.

So for now, it's probably not a good idea to move things to the PM core
until we see a strong pattern emerging.

That being said, maybe we could handle this in the
subsystem/bus/pm_domain level though (in your case AMBA, in OMAP case
the pm_domain) to still avoid sprinkling this across all the drivers?

Kevin

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-05 17:22   ` Stephen Warren
@ 2013-06-11  8:28     ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-11  8:28 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Linus Walleij, Greg Kroah-Hartman, Stephen Warren, Kevin Hilman,
	linux-kernel, linux-arm-kernel, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Wolfram Sang

On Wed, Jun 5, 2013 at 7:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>
>> +int pinctrl_pm_select_default_state(struct device *dev)
>
>> +int pinctrl_pm_select_sleep_state(struct device *dev)
>
>> +int pinctrl_pm_select_idle_state(struct device *dev)
>
> The implementation of those 3 functions is basically identical. I'd be
> inclined to move it to a helper function, and just pass (dev,
> pins->xxx_state) to it.

Just to follow up on this now that I'm adding one more state.

I tried to create a refactoring patch for this but couldn't come
up with anything apropriate along the lines above. For example
this function:

int pinctrl_pm_select_default_state(struct device *dev)
{
        struct dev_pin_info *pins = dev->pins;
        int ret;

        if (!pins)
                return 0;
        if (IS_ERR(pins->default_state))
                return 0; /* No default state */
        ret = pinctrl_select_state(pins->p, pins->default_state);
        if (ret)
                dev_err(dev, "failed to activate default pinctrl state\n");
        return ret;
}

Would be refactored into something like this:

static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *s)
{
        struct dev_pin_info *pins = dev->pins;

        if (IS_ERR(s))
                return 0;
        return pinctrl_select_state(pins->p, s);
}

int pinctrl_pm_select_default_state(struct device *dev)
{
        struct dev_pin_info *pins = dev->pins;
        int ret;

        if (!pins)
                return 0;
        if (IS_ERR(pins->default_state))
                return 0; /* No default state */
        ret = pinctrl_pm_select_state(dev, pins->default_state);
        if (ret)
                dev_err(dev, "failed to activate default pinctrl state\n");
        return ret;
}

That is not any elegant, I can cut down the lines by removing
debug messages but still we're dereferencing the pins twice and other
ugliness like that. Also pinctrl_pm_select_state() becomes more and more
a NULL wrapper around pinctrl_select_state() itself. If you have some other
suggestion or a patch ... I just can't see any elegant refactoring here.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-11  8:28     ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-11  8:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 5, 2013 at 7:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>
>> +int pinctrl_pm_select_default_state(struct device *dev)
>
>> +int pinctrl_pm_select_sleep_state(struct device *dev)
>
>> +int pinctrl_pm_select_idle_state(struct device *dev)
>
> The implementation of those 3 functions is basically identical. I'd be
> inclined to move it to a helper function, and just pass (dev,
> pins->xxx_state) to it.

Just to follow up on this now that I'm adding one more state.

I tried to create a refactoring patch for this but couldn't come
up with anything apropriate along the lines above. For example
this function:

int pinctrl_pm_select_default_state(struct device *dev)
{
        struct dev_pin_info *pins = dev->pins;
        int ret;

        if (!pins)
                return 0;
        if (IS_ERR(pins->default_state))
                return 0; /* No default state */
        ret = pinctrl_select_state(pins->p, pins->default_state);
        if (ret)
                dev_err(dev, "failed to activate default pinctrl state\n");
        return ret;
}

Would be refactored into something like this:

static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *s)
{
        struct dev_pin_info *pins = dev->pins;

        if (IS_ERR(s))
                return 0;
        return pinctrl_select_state(pins->p, s);
}

int pinctrl_pm_select_default_state(struct device *dev)
{
        struct dev_pin_info *pins = dev->pins;
        int ret;

        if (!pins)
                return 0;
        if (IS_ERR(pins->default_state))
                return 0; /* No default state */
        ret = pinctrl_pm_select_state(dev, pins->default_state);
        if (ret)
                dev_err(dev, "failed to activate default pinctrl state\n");
        return ret;
}

That is not any elegant, I can cut down the lines by removing
debug messages but still we're dereferencing the pins twice and other
ugliness like that. Also pinctrl_pm_select_state() becomes more and more
a NULL wrapper around pinctrl_select_state() itself. If you have some other
suggestion or a patch ... I just can't see any elegant refactoring here.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-11  8:28     ` Linus Walleij
@ 2013-06-13 22:02       ` Stephen Warren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stephen Warren @ 2013-06-13 22:02 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Linus Walleij, Greg Kroah-Hartman, Stephen Warren, Kevin Hilman,
	linux-kernel, linux-arm-kernel, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Wolfram Sang

On 06/11/2013 02:28 AM, Linus Walleij wrote:
> On Wed, Jun 5, 2013 at 7:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>
>>> +int pinctrl_pm_select_default_state(struct device *dev)
>>
>>> +int pinctrl_pm_select_sleep_state(struct device *dev)
>>
>>> +int pinctrl_pm_select_idle_state(struct device *dev)
>>
>> The implementation of those 3 functions is basically identical. I'd be
>> inclined to move it to a helper function, and just pass (dev,
>> pins->xxx_state) to it.
> 
> Just to follow up on this now that I'm adding one more state.
> 
> I tried to create a refactoring patch for this but couldn't come
> up with anything apropriate along the lines above. For example
> this function:
...

Don't you just want something very roughly like:

int pinctrl_pm_select_xxx_state(struct device *dev,
		unsigned long offset, char *name)
{
	struct dev_pin_info *pins = dev->pins;
	struct pinctrl_state **s = (void *)(((char *)pins) + offset)
	int ret;

	if (!pins)
		return 0;
	if (IS_ERR(*s))
		return 0; /* No default state */
	ret = pinctrl_select_state(pins->p, *s);
	if (ret)
		dev_err(dev, "failed to activate %s pinctrl state\n",
			name);
	return ret;
}

int pinctrl_pm_select_default_state(struct device *dev)
{
    return pinctrl_pm_select_xxx_state(dev,
		offsetof(struct dev_pin_info, default_state),
		"default");
}


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-13 22:02       ` Stephen Warren
  0 siblings, 0 replies; 36+ messages in thread
From: Stephen Warren @ 2013-06-13 22:02 UTC (permalink / raw)
  To: linux-arm-kernel

On 06/11/2013 02:28 AM, Linus Walleij wrote:
> On Wed, Jun 5, 2013 at 7:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>
>>> +int pinctrl_pm_select_default_state(struct device *dev)
>>
>>> +int pinctrl_pm_select_sleep_state(struct device *dev)
>>
>>> +int pinctrl_pm_select_idle_state(struct device *dev)
>>
>> The implementation of those 3 functions is basically identical. I'd be
>> inclined to move it to a helper function, and just pass (dev,
>> pins->xxx_state) to it.
> 
> Just to follow up on this now that I'm adding one more state.
> 
> I tried to create a refactoring patch for this but couldn't come
> up with anything apropriate along the lines above. For example
> this function:
...

Don't you just want something very roughly like:

int pinctrl_pm_select_xxx_state(struct device *dev,
		unsigned long offset, char *name)
{
	struct dev_pin_info *pins = dev->pins;
	struct pinctrl_state **s = (void *)(((char *)pins) + offset)
	int ret;

	if (!pins)
		return 0;
	if (IS_ERR(*s))
		return 0; /* No default state */
	ret = pinctrl_select_state(pins->p, *s);
	if (ret)
		dev_err(dev, "failed to activate %s pinctrl state\n",
			name);
	return ret;
}

int pinctrl_pm_select_default_state(struct device *dev)
{
    return pinctrl_pm_select_xxx_state(dev,
		offsetof(struct dev_pin_info, default_state),
		"default");
}

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
  2013-06-13 22:02       ` Stephen Warren
@ 2013-06-16 10:55         ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-16 10:55 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Linus Walleij, Greg Kroah-Hartman, Stephen Warren, Kevin Hilman,
	linux-kernel, linux-arm-kernel, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Wolfram Sang

On Fri, Jun 14, 2013 at 12:02 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/11/2013 02:28 AM, Linus Walleij wrote:
>> I tried to create a refactoring patch for this but couldn't come
>> up with anything apropriate along the lines above. For example
>> this function:
> ...
>
> Don't you just want something very roughly like:
>
> int pinctrl_pm_select_xxx_state(struct device *dev,
>                 unsigned long offset, char *name)
> {
>         struct dev_pin_info *pins = dev->pins;
>         struct pinctrl_state **s = (void *)(((char *)pins) + offset)
(...)
>     return pinctrl_pm_select_xxx_state(dev,
>                 offsetof(struct dev_pin_info, default_state),
>                 "default");
> }

Argh that seems a bit too esoteric to save these few
lines, maybe it's me being too stupid to parse this
but it makes the code less maintainable for the pinctrl
maintainer atleast so will not happen right now.

But it is clever still. :-)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] drivers: pinctrl sleep and idle states in the core
@ 2013-06-16 10:55         ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-16 10:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jun 14, 2013 at 12:02 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/11/2013 02:28 AM, Linus Walleij wrote:
>> I tried to create a refactoring patch for this but couldn't come
>> up with anything apropriate along the lines above. For example
>> this function:
> ...
>
> Don't you just want something very roughly like:
>
> int pinctrl_pm_select_xxx_state(struct device *dev,
>                 unsigned long offset, char *name)
> {
>         struct dev_pin_info *pins = dev->pins;
>         struct pinctrl_state **s = (void *)(((char *)pins) + offset)
(...)
>     return pinctrl_pm_select_xxx_state(dev,
>                 offsetof(struct dev_pin_info, default_state),
>                 "default");
> }

Argh that seems a bit too esoteric to save these few
lines, maybe it's me being too stupid to parse this
but it makes the code less maintainable for the pinctrl
maintainer atleast so will not happen right now.

But it is clever still. :-)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH] pinctrl: export pinctrl_pm_select_*_state
  2013-06-05 13:44 ` Linus Walleij
@ 2013-06-17 15:12   ` Arnd Bergmann
  -1 siblings, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2013-06-17 15:12 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Greg Kroah-Hartman, Stephen Warren, Kevin Hilman, linux-kernel,
	linux-arm-kernel, Linus Walleij, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Stephen Warren, Wolfram Sang

The three functions pinctrl_pm_select_default_state,
pinctrl_pm_select_sleep_state, and pinctrl_pm_select_idle_state
are used in drivers that can be loadable modules, and should
be exported.

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

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 33710b5..32eb7e2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1245,6 +1245,7 @@ int pinctrl_pm_select_default_state(struct device *dev)
 		dev_err(dev, "failed to activate default pinctrl state\n");
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
 
 /**
  * pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM
@@ -1264,6 +1265,7 @@ int pinctrl_pm_select_sleep_state(struct device *dev)
 		dev_err(dev, "failed to activate pinctrl sleep state\n");
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
 
 /**
  * pinctrl_pm_select_idle_state() - select idle pinctrl state for PM
@@ -1283,7 +1285,7 @@ int pinctrl_pm_select_idle_state(struct device *dev)
 		dev_err(dev, "failed to activate pinctrl idle state\n");
 	return ret;
 }
-
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
 #endif
 
 #ifdef CONFIG_DEBUG_FS


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH] pinctrl: export pinctrl_pm_select_*_state
@ 2013-06-17 15:12   ` Arnd Bergmann
  0 siblings, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2013-06-17 15:12 UTC (permalink / raw)
  To: linux-arm-kernel

The three functions pinctrl_pm_select_default_state,
pinctrl_pm_select_sleep_state, and pinctrl_pm_select_idle_state
are used in drivers that can be loadable modules, and should
be exported.

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

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 33710b5..32eb7e2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1245,6 +1245,7 @@ int pinctrl_pm_select_default_state(struct device *dev)
 		dev_err(dev, "failed to activate default pinctrl state\n");
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
 
 /**
  * pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM
@@ -1264,6 +1265,7 @@ int pinctrl_pm_select_sleep_state(struct device *dev)
 		dev_err(dev, "failed to activate pinctrl sleep state\n");
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
 
 /**
  * pinctrl_pm_select_idle_state() - select idle pinctrl state for PM
@@ -1283,7 +1285,7 @@ int pinctrl_pm_select_idle_state(struct device *dev)
 		dev_err(dev, "failed to activate pinctrl idle state\n");
 	return ret;
 }
-
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
 #endif
 
 #ifdef CONFIG_DEBUG_FS

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH] pinctrl: export pinctrl_pm_select_*_state
  2013-06-17 15:12   ` Arnd Bergmann
@ 2013-06-17 16:30     ` Linus Walleij
  -1 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-17 16:30 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Walleij, Greg Kroah-Hartman, Stephen Warren, Kevin Hilman,
	linux-kernel, linux-arm-kernel, Hebbar Gururaja, Mark Brown,
	Dmitry Torokhov, Stephen Warren, Wolfram Sang

On Mon, Jun 17, 2013 at 5:12 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> The three functions pinctrl_pm_select_default_state,
> pinctrl_pm_select_sleep_state, and pinctrl_pm_select_idle_state
> are used in drivers that can be loadable modules, and should
> be exported.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

That's right, sorry for missing this :-(

Patch applied!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH] pinctrl: export pinctrl_pm_select_*_state
@ 2013-06-17 16:30     ` Linus Walleij
  0 siblings, 0 replies; 36+ messages in thread
From: Linus Walleij @ 2013-06-17 16:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jun 17, 2013 at 5:12 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> The three functions pinctrl_pm_select_default_state,
> pinctrl_pm_select_sleep_state, and pinctrl_pm_select_idle_state
> are used in drivers that can be loadable modules, and should
> be exported.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

That's right, sorry for missing this :-(

Patch applied!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2013-06-17 16:30 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-05 13:44 [PATCH 1/3] drivers: pinctrl sleep and idle states in the core Linus Walleij
2013-06-05 13:44 ` Linus Walleij
2013-06-05 13:44 ` [PATCH 2/3] tty: serial: modify PL011 driver to use pinctrl PM helpers Linus Walleij
2013-06-05 13:44   ` Linus Walleij
2013-06-05 18:54   ` Greg Kroah-Hartman
2013-06-05 18:54     ` Greg Kroah-Hartman
2013-06-05 13:44 ` [PATCH 3/3] i2c: nomadik: " Linus Walleij
2013-06-05 13:44   ` Linus Walleij
2013-06-05 16:34   ` Kevin Hilman
2013-06-05 16:34     ` Kevin Hilman
2013-06-07  8:33     ` Linus Walleij
2013-06-07  8:33       ` Linus Walleij
2013-06-07 14:31       ` Kevin Hilman
2013-06-07 14:31         ` Kevin Hilman
2013-06-05 14:03 ` [PATCH 1/3] drivers: pinctrl sleep and idle states in the core Wolfram Sang
2013-06-05 14:03   ` Wolfram Sang
2013-06-05 14:47 ` Mark Brown
2013-06-05 14:47   ` Mark Brown
2013-06-05 15:57 ` Kevin Hilman
2013-06-05 15:57   ` Kevin Hilman
2013-06-05 17:22 ` Stephen Warren
2013-06-05 17:22   ` Stephen Warren
2013-06-07  7:53   ` Linus Walleij
2013-06-07  7:53     ` Linus Walleij
2013-06-11  8:28   ` Linus Walleij
2013-06-11  8:28     ` Linus Walleij
2013-06-13 22:02     ` Stephen Warren
2013-06-13 22:02       ` Stephen Warren
2013-06-16 10:55       ` Linus Walleij
2013-06-16 10:55         ` Linus Walleij
2013-06-05 18:54 ` Greg Kroah-Hartman
2013-06-05 18:54   ` Greg Kroah-Hartman
2013-06-17 15:12 ` [PATCH] pinctrl: export pinctrl_pm_select_*_state Arnd Bergmann
2013-06-17 15:12   ` Arnd Bergmann
2013-06-17 16:30   ` Linus Walleij
2013-06-17 16:30     ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.