linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] drivers/pinctrl: Add the concept of an "init" state
@ 2015-10-21  4:15 Douglas Anderson
  2015-10-21  5:06 ` Greg KH
  2015-10-27 10:25 ` Linus Walleij
  0 siblings, 2 replies; 3+ messages in thread
From: Douglas Anderson @ 2015-10-21  4:15 UTC (permalink / raw)
  To: Linus Walleij, Greg KH
  Cc: Heiko Stuebner, Caesar Wang, Chris Zhong, Douglas Anderson,
	linux-kernel, linux-gpio

For pinctrl the "default" state is applied to pins before the driver's
probe function is called.  This is normally a sensible thing to do,
but in some cases can cause problems.  That's because the pins will
change state before the driver is given a chance to program how those
pins should behave.

As an example you might have a regulator that is controlled by a PWM
(output high = high voltage, output low = low voltage).  The firmware
might leave this pin as driven high.  If we allow the driver core to
reconfigure this pin as a PWM pin before the PWM's probe function runs
then you might end up running at too low of a voltage while we probe.

Let's introudce a new "init" state.  If this is defined we'll set
pinctrl to this state before probe and then "default" after probe
(unless the driver explicitly changed states already).

An alternative idea that was thought of was to use the pre-existing
"sleep" or "idle" states and add a boolean property that we should
start in that mode.  This was not done because the "init" state is
needed for correctness and those other states are only present (and
only transitioned in to and out of) when (optional) power management
is enabled.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Caesar Wang <wxt@rock-chips.com>
---
Changes in v3:
- Moved declarations to pinctrl/devinfo.h
- Fixed author/SoB

Changes in v2:
- Added comment to pinctrl_init_done() as per Linus W.

As mentioned in v2 repost, reposted after 1 year of no activity since
Caesar Wang found a use for this.  See
<https://patchwork.kernel.org/patch/7444161/>.  I hope it's OK that I
left Greg KH's Ack...

 drivers/base/dd.c                     |  2 ++
 drivers/base/pinctrl.c                | 15 +++++++++++++--
 drivers/pinctrl/core.c                | 32 ++++++++++++++++++++++++++++++++
 include/linux/pinctrl/devinfo.h       | 10 ++++++++++
 include/linux/pinctrl/pinctrl-state.h |  8 ++++++++
 5 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c4a3f298e..217eafc 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -322,6 +322,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 			goto probe_failed;
 	}
 
+	pinctrl_init_done(dev);
+
 	if (dev->pm_domain && dev->pm_domain->sync)
 		dev->pm_domain->sync(dev);
 
diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 5fb74b4..0762975 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -42,9 +42,20 @@ int pinctrl_bind_pins(struct device *dev)
 		goto cleanup_get;
 	}
 
-	ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state);
+	dev->pins->init_state = pinctrl_lookup_state(dev->pins->p,
+					PINCTRL_STATE_INIT);
+	if (IS_ERR(dev->pins->init_state)) {
+		/* Not supplying this state is perfectly legal */
+		dev_dbg(dev, "no init pinctrl state\n");
+
+		ret = pinctrl_select_state(dev->pins->p,
+					   dev->pins->default_state);
+	} else {
+		ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
+	}
+
 	if (ret) {
-		dev_dbg(dev, "failed to activate default pinctrl state\n");
+		dev_dbg(dev, "failed to activate initial pinctrl state\n");
 		goto cleanup_get;
 	}
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9638a00..2686a44 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1240,6 +1240,38 @@ int pinctrl_force_default(struct pinctrl_dev *pctldev)
 }
 EXPORT_SYMBOL_GPL(pinctrl_force_default);
 
+/**
+ * pinctrl_init_done() - tell pinctrl probe is done
+ *
+ * We'll use this time to switch the pins from "init" to "default" unless the
+ * driver selected some other state.
+ *
+ * @dev: device to that's done probing
+ */
+int pinctrl_init_done(struct device *dev)
+{
+	struct dev_pin_info *pins = dev->pins;
+	int ret;
+
+	if (!pins)
+		return 0;
+
+	if (IS_ERR(pins->init_state))
+		return 0; /* No such state */
+
+	if (pins->p->state != pins->init_state)
+		return 0; /* Not at init anyway */
+
+	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;
+}
+
 #ifdef CONFIG_PM
 
 /**
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index 281cb91..05082e4 100644
--- a/include/linux/pinctrl/devinfo.h
+++ b/include/linux/pinctrl/devinfo.h
@@ -24,10 +24,14 @@
  * struct dev_pin_info - pin state container for devices
  * @p: pinctrl handle for the containing device
  * @default_state: the default state for the handle, if found
+ * @init_state: the state at probe time, if found
+ * @sleep_state: the state at suspend time, if found
+ * @idle_state: the state at idle (runtime suspend) time, if found
  */
 struct dev_pin_info {
 	struct pinctrl *p;
 	struct pinctrl_state *default_state;
+	struct pinctrl_state *init_state;
 #ifdef CONFIG_PM
 	struct pinctrl_state *sleep_state;
 	struct pinctrl_state *idle_state;
@@ -35,6 +39,7 @@ struct dev_pin_info {
 };
 
 extern int pinctrl_bind_pins(struct device *dev);
+extern int pinctrl_init_done(struct device *dev);
 
 #else
 
@@ -45,5 +50,10 @@ static inline int pinctrl_bind_pins(struct device *dev)
 	return 0;
 }
 
+static inline int pinctrl_init_done(struct device *dev)
+{
+	return 0;
+}
+
 #endif /* CONFIG_PINCTRL */
 #endif /* PINCTRL_DEVINFO_H */
diff --git a/include/linux/pinctrl/pinctrl-state.h b/include/linux/pinctrl/pinctrl-state.h
index b5919f8..2307351 100644
--- a/include/linux/pinctrl/pinctrl-state.h
+++ b/include/linux/pinctrl/pinctrl-state.h
@@ -9,6 +9,13 @@
  *	hogs to configure muxing and pins at boot, and also as a state
  *	to go into when returning from sleep and idle in
  *	.pm_runtime_resume() or ordinary .resume() for example.
+ * @PINCTRL_STATE_INIT: normally the pinctrl will be set to "default"
+ *	before the driver's probe() function is called.  There are some
+ *	drivers where that is not appropriate becausing doing so would
+ *	glitch the pins.  In those cases you can add an "init" pinctrl
+ *	which is the state of the pins before drive probe.  After probe
+ *	if the pins are still in "init" state they'll be moved to
+ *	"default".
  * @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into
  *	when the pins are idle. This is a state where the system is relaxed
  *	but not fully sleeping - some power may be on but clocks gated for
@@ -20,5 +27,6 @@
  *	ordinary .suspend() function.
  */
 #define PINCTRL_STATE_DEFAULT "default"
+#define PINCTRL_STATE_INIT "init"
 #define PINCTRL_STATE_IDLE "idle"
 #define PINCTRL_STATE_SLEEP "sleep"
-- 
2.6.0.rc2.230.g3dd15c0


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

* Re: [PATCH v3] drivers/pinctrl: Add the concept of an "init" state
  2015-10-21  4:15 [PATCH v3] drivers/pinctrl: Add the concept of an "init" state Douglas Anderson
@ 2015-10-21  5:06 ` Greg KH
  2015-10-27 10:25 ` Linus Walleij
  1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2015-10-21  5:06 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Linus Walleij, Heiko Stuebner, Caesar Wang, Chris Zhong,
	linux-kernel, linux-gpio

On Tue, Oct 20, 2015 at 09:15:06PM -0700, Douglas Anderson wrote:
> For pinctrl the "default" state is applied to pins before the driver's
> probe function is called.  This is normally a sensible thing to do,
> but in some cases can cause problems.  That's because the pins will
> change state before the driver is given a chance to program how those
> pins should behave.
> 
> As an example you might have a regulator that is controlled by a PWM
> (output high = high voltage, output low = low voltage).  The firmware
> might leave this pin as driven high.  If we allow the driver core to
> reconfigure this pin as a PWM pin before the PWM's probe function runs
> then you might end up running at too low of a voltage while we probe.
> 
> Let's introudce a new "init" state.  If this is defined we'll set
> pinctrl to this state before probe and then "default" after probe
> (unless the driver explicitly changed states already).
> 
> An alternative idea that was thought of was to use the pre-existing
> "sleep" or "idle" states and add a boolean property that we should
> start in that mode.  This was not done because the "init" state is
> needed for correctness and those other states are only present (and
> only transitioned in to and out of) when (optional) power management
> is enabled.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Caesar Wang <wxt@rock-chips.com>
> ---
> Changes in v3:
> - Moved declarations to pinctrl/devinfo.h
> - Fixed author/SoB
> 
> Changes in v2:
> - Added comment to pinctrl_init_done() as per Linus W.
> 
> As mentioned in v2 repost, reposted after 1 year of no activity since
> Caesar Wang found a use for this.  See
> <https://patchwork.kernel.org/patch/7444161/>.  I hope it's OK that I
> left Greg KH's Ack...

The ack from me is fine.

thanks,

greg k-h

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

* Re: [PATCH v3] drivers/pinctrl: Add the concept of an "init" state
  2015-10-21  4:15 [PATCH v3] drivers/pinctrl: Add the concept of an "init" state Douglas Anderson
  2015-10-21  5:06 ` Greg KH
@ 2015-10-27 10:25 ` Linus Walleij
  1 sibling, 0 replies; 3+ messages in thread
From: Linus Walleij @ 2015-10-27 10:25 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Greg KH, Heiko Stuebner, Caesar Wang, Chris Zhong, linux-kernel,
	linux-gpio

On Wed, Oct 21, 2015 at 6:15 AM, Douglas Anderson <dianders@chromium.org> wrote:

> For pinctrl the "default" state is applied to pins before the driver's
> probe function is called.  This is normally a sensible thing to do,
> but in some cases can cause problems.  That's because the pins will
> change state before the driver is given a chance to program how those
> pins should behave.
>
> As an example you might have a regulator that is controlled by a PWM
> (output high = high voltage, output low = low voltage).  The firmware
> might leave this pin as driven high.  If we allow the driver core to
> reconfigure this pin as a PWM pin before the PWM's probe function runs
> then you might end up running at too low of a voltage while we probe.
>
> Let's introudce a new "init" state.  If this is defined we'll set
> pinctrl to this state before probe and then "default" after probe
> (unless the driver explicitly changed states already).
>
> An alternative idea that was thought of was to use the pre-existing
> "sleep" or "idle" states and add a boolean property that we should
> start in that mode.  This was not done because the "init" state is
> needed for correctness and those other states are only present (and
> only transitioned in to and out of) when (optional) power management
> is enabled.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Caesar Wang <wxt@rock-chips.com>

Patch applied.

Yours,
Linus Walleij

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

end of thread, other threads:[~2015-10-27 10:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-21  4:15 [PATCH v3] drivers/pinctrl: Add the concept of an "init" state Douglas Anderson
2015-10-21  5:06 ` Greg KH
2015-10-27 10:25 ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).