[PATCHv2 0/4] improved support for runtime muxing for pinctrl

[PATCHv2 0/4] improved support for runtime muxing for pinctrl

[Tony Lindgren]

Hi all,

Here's this series again with hopefully all the comments addressed.

As discussed earlier, the pinctrl support for changing some of the
consumer device pins during runtime needs some improvment.
                                                                                                                
Regards,

Tony

---

Tony Lindgren (4):
      pinctrl: Remove duplicate code in pinctrl_pm_select_state functions
      pinctrl: Allow pinctrl to have multiple active states
      pinctrl: Add support for additional dynamic states
      drivers: Add pinctrl handling for dynamic pin states


 Documentation/pinctrl.txt             |   77 ++++++++-
 drivers/base/pinctrl.c                |   39 ++++
 drivers/pinctrl/core.c                |  291 +++++++++++++++++++++++++++------
 drivers/pinctrl/core.h                |   10 +
 include/linux/pinctrl/consumer.h      |   46 +++++
 include/linux/pinctrl/devinfo.h       |    4 
 include/linux/pinctrl/pinctrl-state.h |   15 +-
 7 files changed, 420 insertions(+), 62 deletions(-)

-- 
Signature

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Tony Lindgren]

There's no need to duplicate essentially the same functions. Let's
introduce static int pinctrl_pm_select_state() and make the other
related functions call that.

This allows us to add support later on for multiple active states,
and more optimized dynamic remuxing.

Note that we still need to export the various pinctrl_pm_select
functions as we want to keep struct pinctrl_state private to the
pinctrl code, and cannot replace those with inline functions.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c |   55 ++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 5b272bf..a97b717 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1227,23 +1227,36 @@ EXPORT_SYMBOL_GPL(pinctrl_force_default);
 #ifdef CONFIG_PM
 
 /**
- * pinctrl_pm_select_default_state() - select default pinctrl state for PM
+ * pinctrl_pm_select_state() - select pinctrl state for PM
  * @dev: device to select default state for
+ * @state: state to set
  */
-int pinctrl_pm_select_default_state(struct device *dev)
+static int pinctrl_pm_select_state(struct device *dev,
+				   struct pinctrl_state *state)
 {
 	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 (IS_ERR(state))
+		return 0; /* No such state */
+	ret = pinctrl_select_state(pins->p, state);
 	if (ret)
-		dev_err(dev, "failed to activate default pinctrl state\n");
+		dev_err(dev, "failed to activate pinctrl state %s\n",
+			state->name);
 	return ret;
 }
+
+/**
+ * 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)
+{
+	if (!dev->pins)
+		return 0;
+
+	return pinctrl_pm_select_state(dev, dev->pins->default_state);
+}
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
 
 /**
@@ -1252,17 +1265,10 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
  */
 int pinctrl_pm_select_sleep_state(struct device *dev)
 {
-	struct dev_pin_info *pins = dev->pins;
-	int ret;
-
-	if (!pins)
+	if (!dev->pins)
 		return 0;
-	if (IS_ERR(pins->sleep_state))
-		return 0; /* No sleep state */
-	ret = pinctrl_select_state(pins->p, pins->sleep_state);
-	if (ret)
-		dev_err(dev, "failed to activate pinctrl sleep state\n");
-	return ret;
+
+	return pinctrl_pm_select_state(dev, dev->pins->sleep_state);
 }
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
 
@@ -1272,17 +1278,10 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
  */
 int pinctrl_pm_select_idle_state(struct device *dev)
 {
-	struct dev_pin_info *pins = dev->pins;
-	int ret;
-
-	if (!pins)
+	if (!dev->pins)
 		return 0;
-	if (IS_ERR(pins->idle_state))
-		return 0; /* No idle state */
-	ret = pinctrl_select_state(pins->p, pins->idle_state);
-	if (ret)
-		dev_err(dev, "failed to activate pinctrl idle state\n");
-	return ret;
+
+	return pinctrl_pm_select_state(dev, dev->pins->idle_state);
 }
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
 #endif

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Linus Walleij]

On Thu, Jul 18, 2013 at 5:15 PM, Tony Lindgren <tony@atomide.com> wrote:

> There's no need to duplicate essentially the same functions. Let's
> introduce static int pinctrl_pm_select_state() and make the other
> related functions call that.
>
> This allows us to add support later on for multiple active states,
> and more optimized dynamic remuxing.
>
> Note that we still need to export the various pinctrl_pm_select
> functions as we want to keep struct pinctrl_state private to the
> pinctrl code, and cannot replace those with inline functions.
>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Aha that is how to do it. Stephen complained about it earlier
but I couldn't come up with a good enough refactoring.

Patch applied.

Yours,
Linus Walleij

[PATCH 2/4] pinctrl: Allow pinctrl to have multiple active states

[Tony Lindgren]

It's quite common that we need to dynamically change some pins for a
device for runtime PM, or toggle a pin between rx and tx. Changing all
the pins for a device is not efficient way of doing it.

So let's allow setting up multiple active states for pinctrl. Currently
we only need PINCTRL_STATIC and PINCTRL_DYNAMIC, where PINCTRL_STATIC
covers the current default pins, and PINCTRL_DYNAMIC holds the dynamic
pins that need to be toggled.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c |   18 ++++++++++--------
 drivers/pinctrl/core.h |   10 ++++++++--
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index a97b717..c9b709b 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -885,7 +885,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
 	mutex_lock(&pinctrl_list_mutex);
 	list_for_each_entry_safe(state, n1, &p->states, node) {
 		list_for_each_entry_safe(setting, n2, &state->settings, node) {
-			pinctrl_free_setting(state == p->state, setting);
+			pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
+					     setting);
 			list_del(&setting->node);
 			kfree(setting);
 		}
@@ -955,13 +956,13 @@ EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
 int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 {
 	struct pinctrl_setting *setting, *setting2;
-	struct pinctrl_state *old_state = p->state;
+	struct pinctrl_state *old_state = p->state[PINCTRL_STATIC];
 	int ret;
 
-	if (p->state == state)
+	if (old_state == state)
 		return 0;
 
-	if (p->state) {
+	if (old_state) {
 		/*
 		 * The set of groups with a mux configuration in the old state
 		 * may not be identical to the set of groups with a mux setting
@@ -971,7 +972,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 		 * but not in the new state, this code puts that group into a
 		 * safe/disabled state.
 		 */
-		list_for_each_entry(setting, &p->state->settings, node) {
+		list_for_each_entry(setting, &old_state->settings, node) {
 			bool found = false;
 			if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
 				continue;
@@ -989,7 +990,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 		}
 	}
 
-	p->state = NULL;
+	p->state[PINCTRL_STATIC] = NULL;
 
 	/* Apply all the settings for the new state */
 	list_for_each_entry(setting, &state->settings, node) {
@@ -1011,7 +1012,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 		}
 	}
 
-	p->state = state;
+	p->state[PINCTRL_STATIC] = state;
 
 	return 0;
 
@@ -1492,7 +1493,8 @@ static int pinctrl_show(struct seq_file *s, void *what)
 	list_for_each_entry(p, &pinctrl_list, node) {
 		seq_printf(s, "device: %s current state: %s\n",
 			   dev_name(p->dev),
-			   p->state ? p->state->name : "none");
+			   p->state[PINCTRL_STATIC] ?
+			   p->state[PINCTRL_STATIC]->name : "none");
 
 		list_for_each_entry(state, &p->states, node) {
 			seq_printf(s, "  state: %s\n", state->name);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 75476b3..ac5a269 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -53,12 +53,18 @@ struct pinctrl_dev {
 #endif
 };
 
+enum pinctrl_states {
+	PINCTRL_STATIC,
+	PINCTRL_DYNAMIC,
+	PINCTRL_NR_STATES,
+};
+
 /**
  * struct pinctrl - per-device pin control state holder
  * @node: global list node
  * @dev: the device using this pin control handle
  * @states: a list of states for this device
- * @state: the current state
+ * @state: the current state(s)
  * @dt_maps: the mapping table chunks dynamically parsed from device tree for
  *	this device, if any
  * @users: reference count
@@ -67,7 +73,7 @@ struct pinctrl {
 	struct list_head node;
 	struct device *dev;
 	struct list_head states;
-	struct pinctrl_state *state;
+	struct pinctrl_state *state[PINCTRL_NR_STATES];
 	struct list_head dt_maps;
 	struct kref users;
 };

[PATCH 2/4] pinctrl: Allow pinctrl to have multiple active states

[Linus Walleij]

On Thu, Jul 18, 2013 at 5:15 PM, Tony Lindgren <tony@atomide.com> wrote:

> It's quite common that we need to dynamically change some pins for a
> device for runtime PM, or toggle a pin between rx and tx. Changing all

What does "change" mean above?

Please reword to "remux" if that is what is meant.

> the pins for a device is not efficient way of doing it.
>
> So let's allow setting up multiple active states for pinctrl.

Do you mean multiple default states?

I have a hard time understanding so please help me out :-/

> Currently
> we only need PINCTRL_STATIC and PINCTRL_DYNAMIC, where PINCTRL_STATIC
> covers the current default pins, and PINCTRL_DYNAMIC holds the dynamic
> pins that need to be toggled.

Toggled when?

When changing state to another one? Any other state? Or back to
this state? Sorry not following ... this needs to be more verbose.

>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/pinctrl/core.c |   18 ++++++++++--------
>  drivers/pinctrl/core.h |   10 ++++++++--
>  2 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index a97b717..c9b709b 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -885,7 +885,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
>         mutex_lock(&pinctrl_list_mutex);
>         list_for_each_entry_safe(state, n1, &p->states, node) {
>                 list_for_each_entry_safe(setting, n2, &state->settings, node) {
> -                       pinctrl_free_setting(state == p->state, setting);
> +                       pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
> +                                            setting);
>                         list_del(&setting->node);
>                         kfree(setting);
>                 }
> @@ -955,13 +956,13 @@ EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>  int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>  {
>         struct pinctrl_setting *setting, *setting2;
> -       struct pinctrl_state *old_state = p->state;
> +       struct pinctrl_state *old_state = p->state[PINCTRL_STATIC];
>         int ret;
>
> -       if (p->state == state)
> +       if (old_state == state)
>                 return 0;
>
> -       if (p->state) {
> +       if (old_state) {
>                 /*
>                  * The set of groups with a mux configuration in the old state
>                  * may not be identical to the set of groups with a mux setting
> @@ -971,7 +972,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>                  * but not in the new state, this code puts that group into a
>                  * safe/disabled state.
>                  */
> -               list_for_each_entry(setting, &p->state->settings, node) {
> +               list_for_each_entry(setting, &old_state->settings, node) {
>                         bool found = false;
>                         if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>                                 continue;
> @@ -989,7 +990,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>                 }
>         }
>
> -       p->state = NULL;
> +       p->state[PINCTRL_STATIC] = NULL;
>
>         /* Apply all the settings for the new state */
>         list_for_each_entry(setting, &state->settings, node) {
> @@ -1011,7 +1012,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>                 }
>         }
>
> -       p->state = state;
> +       p->state[PINCTRL_STATIC] = state;
>
>         return 0;
>
> @@ -1492,7 +1493,8 @@ static int pinctrl_show(struct seq_file *s, void *what)
>         list_for_each_entry(p, &pinctrl_list, node) {
>                 seq_printf(s, "device: %s current state: %s\n",
>                            dev_name(p->dev),
> -                          p->state ? p->state->name : "none");
> +                          p->state[PINCTRL_STATIC] ?
> +                          p->state[PINCTRL_STATIC]->name : "none");
>
>                 list_for_each_entry(state, &p->states, node) {
>                         seq_printf(s, "  state: %s\n", state->name);
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 75476b3..ac5a269 100644
> --- a/drivers/pinctrl/core.h
> +++ b/drivers/pinctrl/core.h
> @@ -53,12 +53,18 @@ struct pinctrl_dev {
>  #endif
>  };
>
> +enum pinctrl_states {
> +       PINCTRL_STATIC,
> +       PINCTRL_DYNAMIC,
> +       PINCTRL_NR_STATES,
> +};

This needs some very precise kerneldoc so it is chrystal clear what these
states are all about. "pinctrl_states" is not a good name for this enum,
please find some more precise name, because we have functions
with names like pinctrl_select_state() which is not related to this.

Is this substates or?

> +
>  /**
>   * struct pinctrl - per-device pin control state holder
>   * @node: global list node
>   * @dev: the device using this pin control handle
>   * @states: a list of states for this device
> - * @state: the current state
> + * @state: the current state(s)

How can more than one state be the current state? Sorry I don't get this.
This needs to be more precise I think.

>   * @dt_maps: the mapping table chunks dynamically parsed from device tree for
>   *     this device, if any
>   * @users: reference count
> @@ -67,7 +73,7 @@ struct pinctrl {
>         struct list_head node;
>         struct device *dev;
>         struct list_head states;
> -       struct pinctrl_state *state;
> +       struct pinctrl_state *state[PINCTRL_NR_STATES];
>         struct list_head dt_maps;
>         struct kref users;
>  };

Yours,
Linus Walleij

[PATCH 2/4] pinctrl: Allow pinctrl to have multiple active states

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130722 16:01]:
> On Thu, Jul 18, 2013 at 5:15 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > It's quite common that we need to dynamically change some pins for a
> > device for runtime PM, or toggle a pin between rx and tx. Changing all
> 
> What does "change" mean above?
> 
> Please reword to "remux" if that is what is meant.

Yes remux or reconfigure the pinconf depending on the hardware.
And possibly based on the board specific configuration if both
options are available.
 
> > the pins for a device is not efficient way of doing it.
> >
> > So let's allow setting up multiple active states for pinctrl.
> 
> Do you mean multiple default states?
> 
> I have a hard time understanding so please help me out :-/

One default state, which should only contain the static pins that
don't need to be remuxed or reconfigured during runtime PM.

It should probably say "support for multiple currently activated
states", does that make it clearer?
 
> > Currently
> > we only need PINCTRL_STATIC and PINCTRL_DYNAMIC, where PINCTRL_STATIC
> > covers the current default pins, and PINCTRL_DYNAMIC holds the dynamic
> > pins that need to be toggled.
> 
> Toggled when?

Remuxed or reconfigured constantly for runtime PM.
 
> When changing state to another one? Any other state? Or back to
> this state? Sorry not following ... this needs to be more verbose.

How about "toggle dynamic pins between active and idle state
for runtime PM"?
 
> > --- a/drivers/pinctrl/core.h
> > +++ b/drivers/pinctrl/core.h
> > @@ -53,12 +53,18 @@ struct pinctrl_dev {
> >  #endif
> >  };
> >
> > +enum pinctrl_states {
> > +       PINCTRL_STATIC,
> > +       PINCTRL_DYNAMIC,
> > +       PINCTRL_NR_STATES,
> > +};
> 
> This needs some very precise kerneldoc so it is chrystal clear what these
> states are all about. "pinctrl_states" is not a good name for this enum,
> please find some more precise name, because we have functions
> with names like pinctrl_select_state() which is not related to this.
> 
> Is this substates or?

This lists the sets of pins that can be activated the same
time. So after this, we support two sets of pins that a
pinctrl consumer driver can have activated the same time:
static pins and dynamic pins. There can be any number of
named modes for dynamic pins, but typically only active and
idle.

Then if for some reason we need more than two sets activated
the same time, we can expand the array as needed. So that's
mostly to try to keep things future proof and not limit things
to two sets as noted by Stephen earlier.
 
> > +
> >  /**
> >   * struct pinctrl - per-device pin control state holder
> >   * @node: global list node
> >   * @dev: the device using this pin control handle
> >   * @states: a list of states for this device
> > - * @state: the current state
> > + * @state: the current state(s)
> 
> How can more than one state be the current state? Sorry I don't get this.
> This needs to be more precise I think.

Hopefully the explanation above helps with that, if not,
let me know.
 
Regards,

Tony

[PATCH 3/4] pinctrl: Add support for additional dynamic states

[Tony Lindgren]

To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().

Cc: Felipe Balbi <balbi@ti.com>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/pinctrl.txt             |   77 ++++++++++-
 drivers/pinctrl/core.c                |  226 ++++++++++++++++++++++++++++++---
 include/linux/pinctrl/consumer.h      |   46 +++++++
 include/linux/pinctrl/devinfo.h       |    4 +
 include/linux/pinctrl/pinctrl-state.h |   15 ++
 5 files changed, 342 insertions(+), 26 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 052e13a..0477ec5 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -1283,12 +1283,77 @@ This gives the exact same result as the above construction.
 Runtime pinmuxing
 =================
 
-It is possible to mux a certain function in and out at runtime, say to move
-an SPI port from one set of pins to another set of pins. Say for example for
-spi0 in the example above, we expose two different groups of pins for the same
-function, but with different named in the mapping as described under
-"Advanced mapping" above. So that for an SPI device, we have two states named
-"pos-A" and "pos-B".
+Typically runtime pinmuxing is done for runtime PM to mux a device RX pin
+to GPIO for wake-up events. And In some cases a shared RX/TX pin may need to
+be toggled. Sometimes pins can be also shared between two device drivers.
+
+In most cases runtime pinmuxing only is needed for some pins on a device, and
+most pins can be static. To avoid continuously having to check for all the
+pins of a device, pinctrl framework supports grouping device pin states to
+static and dynamic states.
+
+Most devices only need to use the static default state. If a devices has a
+need for runtime pinmuxing, the device must define the static default state,
+and then the optional dynamic states using the following rules:
+
+1. The default dynamic states are in addition to the static default state.
+
+2. The default dynamic states are active, sleep and idle, or active and idle.
+
+3. At least active and idle, or active and sleep states must be defined.
+
+4. Out of the dynamic pin states, only one of the active, sleep or idle states
+   can be active at a time.
+
+5. The dynamic pin states must all toggle the same pins to avoid unnecessary
+   checks during runtime.
+
+6. The pinctrl consumer driver must use pinctrl_select_dynamic() instead of
+   pinctrl_select_state() to toggle between the dynamic states.
+
+For example, to mux a serial driver RX pin to GPIO for runtime PM wake-up
+events, something like the following can be done. Most of the work is done
+by the pinctrl framework as long as the default, active and idle states are
+properly defined.
+
+#include <linux/pinctrl/consumer.h>
+
+serial_foo_probe()
+{
+	...
+	if (!pinctrl_pm_is_idle_state_error())
+		/* Set driver specific flags for runtime PM */
+	...
+}
+
+static int serial_foo_runtime_suspend(struct device *dev)
+{
+	...
+	res = pinctrl_pm_select_idle_state(dev);
+	if (ret < 0)
+		return ret;
+	...
+}
+
+static int serial_foo_runtime_resume(struct device *dev)
+{
+	...
+	res = pinctrl_pm_select_active_state(dev);
+	if (ret < 0)
+		return ret;
+	...
+}
+
+Currently only runtime muxing for runtime PM has been optimized as that can
+happen@high rate every time when idling and unidling devices. For less
+latency critical runtime remuxing it is possible to use pinctrl_select_state()
+as described below.
+
+For example, it is possible to move an SPI port from one set of pins to another
+set of pins. Say for example for spi0 in the example above, we expose two
+different groups of pins for the same function, but with different named in
+the mapping as described under "Advanced mapping" above. So that for an SPI
+device, we have two states named "pos-A" and "pos-B".
 
 This snippet first muxes the function in the pins defined by group A, enables
 it, disables and releases it, and muxes it in on the pins defined by group B:
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c9b709b..af399cc 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -885,8 +885,13 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
 	mutex_lock(&pinctrl_list_mutex);
 	list_for_each_entry_safe(state, n1, &p->states, node) {
 		list_for_each_entry_safe(setting, n2, &state->settings, node) {
-			pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
-					     setting);
+			bool disable_setting = false;
+
+			if ((state == p->state[PINCTRL_STATIC]) ||
+			    (state == p->state[PINCTRL_DYNAMIC]))
+				disable_setting = true;
+
+			pinctrl_free_setting(disable_setting, setting);
 			list_del(&setting->node);
 			kfree(setting);
 		}
@@ -949,6 +954,25 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
 EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
 
 /**
+ * pinctrl_apply_setting() - apply a setting
+ * @setting: pinctrl setting to apply
+ */
+static int pinctrl_apply_setting(struct pinctrl_setting *setting)
+{
+	switch (setting->type) {
+	case PIN_MAP_TYPE_MUX_GROUP:
+		return pinmux_enable_setting(setting);
+	case PIN_MAP_TYPE_CONFIGS_PIN:
+	case PIN_MAP_TYPE_CONFIGS_GROUP:
+		return pinconf_apply_setting(setting);
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+/**
  * pinctrl_select_state() - select/activate/program a pinctrl state to HW
  * @p: the pinctrl handle for the device that requests configuration
  * @state: the state handle to select/activate/program
@@ -994,19 +1018,7 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 
 	/* Apply all the settings for the new state */
 	list_for_each_entry(setting, &state->settings, node) {
-		switch (setting->type) {
-		case PIN_MAP_TYPE_MUX_GROUP:
-			ret = pinmux_enable_setting(setting);
-			break;
-		case PIN_MAP_TYPE_CONFIGS_PIN:
-		case PIN_MAP_TYPE_CONFIGS_GROUP:
-			ret = pinconf_apply_setting(setting);
-			break;
-		default:
-			ret = -EINVAL;
-			break;
-		}
-
+		ret = pinctrl_apply_setting(setting);
 		if (ret < 0) {
 			goto unapply_new_state;
 		}
@@ -1041,6 +1053,122 @@ unapply_new_state:
 }
 EXPORT_SYMBOL_GPL(pinctrl_select_state);
 
+/**
+ * pinctrl_setting_get_group_pins() - get group pins for a pinctrl setting
+ * @s: pinctrl setting pointer
+ * @pins: pins
+ * @num_pins: number of pins
+ */
+static int pinctrl_setting_get_group_pins(struct pinctrl_setting *s,
+				const unsigned **pins, unsigned *num_pins)
+{
+	struct pinctrl_dev *pctldev;
+	const struct pinctrl_ops *pctlops;
+
+	if (s->type != PIN_MAP_TYPE_MUX_GROUP)
+		return -EINVAL;
+
+	pctldev = s->pctldev;
+	pctlops = pctldev->desc->pctlops;
+
+	return pctlops->get_group_pins(pctldev, s->data.mux.group,
+					pins, num_pins);
+}
+
+/**
+ * pinctrl_state_get_pin_count() - get a pin count for a state
+ * @st: pinctrl state pointer
+ */
+static int pinctrl_state_get_pin_count(struct pinctrl_state *st)
+{
+	struct pinctrl_setting *s;
+	int found = 0;
+
+	list_for_each_entry(s, &st->settings, node) {
+		const unsigned *pins;
+		unsigned num_pins;
+		int res;
+
+		res = pinctrl_setting_get_group_pins(s, &pins, &num_pins);
+		if (res)
+			continue;
+
+		found += num_pins;
+	}
+
+	return found;
+}
+
+/**
+ * pinctrl_check_dynamic() - compare two states for the pins
+ * @dev: pinctrl consumer device pointer
+ * @st1: state handle
+ * @st2: state handle
+ *
+ * This function checks that the group pins match between the two
+ * states to avoid runtime checking. Note that currently the checking
+ * is very minimal, but can be enhanced as needed. Even this minimal
+ * check should be enough to avoid trying to use the default pins state
+ * with pinctrl_select_dynamic().
+ *
+ * Call this during pinctrl consumer driver probe time to check
+ * dynamic pin states used by the device for runtime PM etc.
+
+ * REVISIT: We can improve this further by adding device_get_pins()
+ * and adding a sorted state arrays for pins at device probe time.
+ * The comparing two states can be done just by comparing the two
+ * arrays.
+ */
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	int num_pins1, num_pins2;
+
+	num_pins1 = pinctrl_state_get_pin_count(st1);
+	num_pins2 = pinctrl_state_get_pin_count(st2);
+
+	if (num_pins1 != num_pins2) {
+		dev_err(dev, "device pins do not match for PM states\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_check_dynamic);
+
+/**
+ * pinctrl_select_dynamic() - fast path for toggling pre-validated sets of pins
+ * @p: the pinctrl handle for the device that requests configuration
+ * @state: the state handle to select/activate/program
+ *
+ * Note that as we've already checked that the PINCTRL_DYNAMIC pins always
+ * cover the same sets for active/idle, or rx/tx, there's no need to call
+ * pinux_disable_settings() on these pins. Calling it could also cause
+ * issues for the connected peripheral as it potentially could change the
+ * values of data lines for example.
+ */
+int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
+{
+	struct pinctrl_setting *setting;
+	int ret;
+
+	if (p->state[PINCTRL_DYNAMIC] == state)
+		return 0;
+
+	list_for_each_entry(setting, &state->settings, node) {
+		ret = pinctrl_apply_setting(setting);
+		if (ret < 0) {
+			dev_err(p->dev, "Error applying dynamic settings\n");
+			return ret;
+		}
+	}
+
+	p->state[PINCTRL_DYNAMIC] = state;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_select_dynamic);
+
 static void devm_pinctrl_release(struct device *dev, void *res)
 {
 	pinctrl_put(*(struct pinctrl **)res);
@@ -1240,7 +1368,13 @@ static int pinctrl_pm_select_state(struct device *dev,
 
 	if (IS_ERR(state))
 		return 0; /* No such state */
-	ret = pinctrl_select_state(pins->p, state);
+
+	/* Configured for dynamic muxing? */
+	if (!IS_ERR(dev->pins->active_state))
+		ret = pinctrl_select_dynamic(pins->p, state);
+	else
+		ret = pinctrl_select_state(pins->p, state);
+
 	if (ret)
 		dev_err(dev, "failed to activate pinctrl state %s\n",
 			state->name);
@@ -1261,6 +1395,19 @@ int pinctrl_pm_select_default_state(struct device *dev)
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
 
 /**
+ * pinctrl_pm_select_active_state() - select active pinctrl state for PM
+ * @dev: device to select default state for
+ */
+int pinctrl_pm_select_active_state(struct device *dev)
+{
+	if (!dev->pins)
+		return 0;
+
+	return pinctrl_pm_select_state(dev, dev->pins->active_state);
+}
+EXPORT_SYMBOL_GPL(pinctrl_pm_select_active_state);
+
+/**
  * pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM
  * @dev: device to select sleep state for
  */
@@ -1285,6 +1432,47 @@ int pinctrl_pm_select_idle_state(struct device *dev)
 	return pinctrl_pm_select_state(dev, dev->pins->idle_state);
 }
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
+
+static int pinctrl_pm_is_state_error(struct device *dev,
+				  struct pinctrl_state *state)
+{
+	if (!dev)
+		return -EINVAL;
+
+	return IS_ERR(state);
+}
+
+/**
+ * pinctrl_pm_is_sleep_state_error() - check if sleep state is configured
+ * @dev: consumer device
+ *
+ * Call this from consumer driver to check if the sleep state
+ * has been properly configured on the device.
+ */
+int pinctrl_pm_is_sleep_state_error(struct device *dev)
+{
+	if (!dev->pins)
+		return 0;
+
+	return pinctrl_pm_is_state_error(dev, dev->pins->sleep_state);
+}
+EXPORT_SYMBOL_GPL(pinctrl_pm_is_sleep_state_error);
+
+/**
+ * pinctrl_pm_is_idle_state_error() - check if idle state is configured
+ * @dev: consumer device
+ *
+ * Call this from consumer driver to check if the idle state
+ * has been properly configured on the device.
+ */
+int pinctrl_pm_is_idle_state_error(struct device *dev)
+{
+	if (!dev->pins)
+		return 0;
+
+	return pinctrl_pm_is_state_error(dev, dev->pins->idle_state);
+}
+EXPORT_SYMBOL_GPL(pinctrl_pm_is_idle_state_error);
 #endif
 
 #ifdef CONFIG_DEBUG_FS
@@ -1491,10 +1679,12 @@ static int pinctrl_show(struct seq_file *s, void *what)
 	mutex_lock(&pinctrl_list_mutex);
 
 	list_for_each_entry(p, &pinctrl_list, node) {
-		seq_printf(s, "device: %s current state: %s\n",
+		seq_printf(s, "device: %s current states: %s %s\n",
 			   dev_name(p->dev),
 			   p->state[PINCTRL_STATIC] ?
-			   p->state[PINCTRL_STATIC]->name : "none");
+			   p->state[PINCTRL_STATIC]->name : "none",
+			   p->state[PINCTRL_DYNAMIC] ?
+			   p->state[PINCTRL_DYNAMIC]->name : "none");
 
 		list_for_each_entry(state, &p->states, node) {
 			seq_printf(s, "  state: %s\n", state->name);
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 18eccef..333885b 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -36,19 +36,29 @@ extern struct pinctrl_state * __must_check pinctrl_lookup_state(
 							struct pinctrl *p,
 							const char *name);
 extern int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *s);
+extern int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *s1,
+				 struct pinctrl_state *s2);
+extern int pinctrl_select_dynamic(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_active_state(struct device *dev);
 extern int pinctrl_pm_select_sleep_state(struct device *dev);
 extern int pinctrl_pm_select_idle_state(struct device *dev);
+extern int pinctrl_pm_is_sleep_state_error(struct device *dev);
+extern int pinctrl_pm_is_idle_state_error(struct device *dev);
 #else
 static inline int pinctrl_pm_select_default_state(struct device *dev)
 {
 	return 0;
 }
+static inline int pinctrl_pm_select_active_state(struct device *dev)
+{
+	return 0;
+}
 static inline int pinctrl_pm_select_sleep_state(struct device *dev)
 {
 	return 0;
@@ -57,6 +67,14 @@ static inline int pinctrl_pm_select_idle_state(struct device *dev)
 {
 	return 0;
 }
+static inline int pinctrl_pm_is_sleep_state_error(struct device *dev)
+{
+	return -ENODEV;
+}
+static inline int pinctrl_pm_is_idle_state_error(struct device *dev)
+{
+	return -ENODEV;
+}
 #endif
 
 #else /* !CONFIG_PINCTRL */
@@ -102,6 +120,19 @@ static inline int pinctrl_select_state(struct pinctrl *p,
 	return 0;
 }
 
+static inline int pinctrl_check_dynamic(struct device *dev,
+					struct pinctrl_state *s1,
+					struct pinctrl_state *s2)
+{
+	return 0;
+}
+
+static inline int pinctrl_select_dynamic(struct pinctrl *p,
+				       struct pinctrl_state *s)
+{
+	return 0;
+}
+
 static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev)
 {
 	return NULL;
@@ -116,6 +147,11 @@ static inline int pinctrl_pm_select_default_state(struct device *dev)
 	return 0;
 }
 
+static inline int pinctrl_pm_select_active_state(struct device *dev)
+{
+	return 0;
+}
+
 static inline int pinctrl_pm_select_sleep_state(struct device *dev)
 {
 	return 0;
@@ -126,6 +162,16 @@ static inline int pinctrl_pm_select_idle_state(struct device *dev)
 	return 0;
 }
 
+static inline int pinctrl_pm_is_sleep_state_error(struct device *dev)
+{
+	return -ENODEV;
+}
+
+static inline int pinctrl_pm_is_idle_state_error(struct device *dev)
+{
+	return -ENODEV;
+}
+
 #endif /* CONFIG_PINCTRL */
 
 static inline struct pinctrl * __must_check pinctrl_get_select(
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index 281cb91..2857a7b 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
+ * @active_state: the default state for the handle, if found
+ * @sleep_state: the default state for the handle, if found
+ * @idle_state: the default state for the handle, if found
  */
 struct dev_pin_info {
 	struct pinctrl *p;
 	struct pinctrl_state *default_state;
+	struct pinctrl_state *active_state;
 #ifdef CONFIG_PM
 	struct pinctrl_state *sleep_state;
 	struct pinctrl_state *idle_state;
diff --git a/include/linux/pinctrl/pinctrl-state.h b/include/linux/pinctrl/pinctrl-state.h
index b5919f8..c136e82 100644
--- a/include/linux/pinctrl/pinctrl-state.h
+++ b/include/linux/pinctrl/pinctrl-state.h
@@ -9,16 +9,27 @@
  *	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_ACTIVE: optional state of dynamic pins in addition to
+ *	PINCTRL_STATE_DEFAULT that are needed during runtime.
  * @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
  *	example. Could typically be set from a pm_runtime_suspend() or
- *	pm_runtime_idle() operation.
+ *	pm_runtime_idle() operation. If PINCTRL_STATE_ACTIVE pins are
+ *	defined, PINCTRL_STATE_IDLE pin groups must cover the same pin
+ *	groups as PINCTRL_STATE_ACTIVE and selecting PINCTRL_STATE_IDLE
+ *	must be done using pinctrl_select_state_dynamic() instead of
+ *	pinctrl_select_state().
  * @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into
  *	when the pins are sleeping. This is a state where the system is in
  *	its lowest sleep state. Could typically be set from an
- *	ordinary .suspend() function.
+ *	ordinary .suspend() function. If PINCTRL_STATE_ACTIVE pins are
+ *	defined, PINCTRL_STATE_SLEEP pin groups must cover the same pin
+ *	groups as PINCTRL_STATE_ACTIVE and selecting PINCTRL_STATE_SLEEP
+ *	must be done using pinctrl_select_state_dynamic() instead of
+ *	pinctrl_select_state().
  */
 #define PINCTRL_STATE_DEFAULT "default"
+#define PINCTRL_STATE_ACTIVE "active"
 #define PINCTRL_STATE_IDLE "idle"
 #define PINCTRL_STATE_SLEEP "sleep"

[PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states

[Tony Lindgren]

We want to have static pin states handled separately from
dynamic pin states, so let's add optional state_active.

Then if state_active is defined, let's check and make sure
state_idle and state_sleep match state_active for the
pin groups to avoid checking them during runtime as the
active and idle pins may need to be toggled for many
devices every time we enter and exit idle.

See also Documentation/pinctrl.txt regarding runtime
muxing of pins.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/base/pinctrl.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 5fb74b4..49644ed 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -34,6 +34,7 @@ int pinctrl_bind_pins(struct device *dev)
 		goto cleanup_alloc;
 	}
 
+	/* Default static pins that don't need to change */
 	dev->pins->default_state = pinctrl_lookup_state(dev->pins->p,
 					PINCTRL_STATE_DEFAULT);
 	if (IS_ERR(dev->pins->default_state)) {
@@ -48,23 +49,57 @@ int pinctrl_bind_pins(struct device *dev)
 		goto cleanup_get;
 	}
 
+	/* Optional runtime dynamic pins in addition to the static pins */
+	dev->pins->active_state = pinctrl_lookup_state(dev->pins->p,
+					PINCTRL_STATE_ACTIVE);
+	if (IS_ERR(dev->pins->active_state)) {
+		/* Not supplying this state is perfectly legal */
+		dev_dbg(dev, "no active pinctrl state\n");
+	} else {
+		ret = pinctrl_select_dynamic(dev->pins->p,
+				dev->pins->active_state);
+		if (ret) {
+			dev_dbg(dev, "failed to select active pinctrl state\n");
+			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>
+	 *
+	 * Note that if active state is defined, sleep and idle states must
+	 * cover the same pin groups as active state.
 	 */
 	dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
 					PINCTRL_STATE_SLEEP);
-	if (IS_ERR(dev->pins->sleep_state))
+	if (IS_ERR(dev->pins->sleep_state)) {
 		/* Not supplying this state is perfectly legal */
 		dev_dbg(dev, "no sleep pinctrl state\n");
+	} else if (!IS_ERR(dev->pins->active_state)) {
+		ret = pinctrl_check_dynamic(dev, dev->pins->active_state,
+					    dev->pins->sleep_state);
+		if (ret) {
+			dev_err(dev, "sleep state groups do not match active state\n");
+			dev->pins->sleep_state = ERR_PTR(-EINVAL);
+		}
+	}
 
 	dev->pins->idle_state = pinctrl_lookup_state(dev->pins->p,
 					PINCTRL_STATE_IDLE);
-	if (IS_ERR(dev->pins->idle_state))
+	if (IS_ERR(dev->pins->idle_state)) {
 		/* Not supplying this state is perfectly legal */
 		dev_dbg(dev, "no idle pinctrl state\n");
+	} else if (!IS_ERR(dev->pins->active_state)) {
+		ret = pinctrl_check_dynamic(dev, dev->pins->active_state,
+					    dev->pins->idle_state);
+		if (ret) {
+			dev_err(dev, "idle state groups do not match active state\n");
+			dev->pins->idle_state = ERR_PTR(-EINVAL);
+		}
+	}
 #endif
 
 	return 0;

[PATCH 0/4] improved support for runtime muxing for pinctrl

[Tony Lindgren]

Hi all,

As discussed earlier, the pinctrl support for changing some of the
consumer device pins during runtime needs some improvment.

Here are the patches to do that, I'll also post a minimal sample
patch as a reply to this thread on how to do the muxing for
runtime PM.

Regards,

Tony

---

Tony Lindgren (4):
      pinctrl: Remove duplicate code in pinctrl_pm_select_state functions
      pinctrl: Allow pinctrl to have multiple active states
      pinctrl: Add support for additional dynamic states
      drivers: Add pinctrl handling for dynamic pin states


 drivers/base/pinctrl.c                |   39 +++++
 drivers/pinctrl/core.c                |  250 ++++++++++++++++++++++++++++-----
 drivers/pinctrl/core.h                |   10 +
 include/linux/pinctrl/consumer.h      |   46 ++++++
 include/linux/pinctrl/devinfo.h       |    4 +
 include/linux/pinctrl/pinctrl-state.h |   15 ++
 6 files changed, 321 insertions(+), 43 deletions(-)

-- 
Signature

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Tony Lindgren]

There's no need to duplicate essentially the same functions. Let's
introduce static int pinctrl_pm_select_state() and make the other
related functions call that.

This allows us to add support later on for multiple active states,
and more optimized dynamic remuxing.

Note that we still need to export the various pinctrl_pm_select
functions as we want to keep struct pinctrl_state private to the
pinctrl code, and cannot replace those with inline functions.

Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c |   47 +++++++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 5b272bf..bda2c61 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1227,23 +1227,34 @@ EXPORT_SYMBOL_GPL(pinctrl_force_default);
 #ifdef CONFIG_PM
 
 /**
- * pinctrl_pm_select_default_state() - select default pinctrl state for PM
+ * pinctrl_pm_select_state() - select pinctrl state for PM
  * @dev: device to select default state for
+ * @state: state to set
  */
-int pinctrl_pm_select_default_state(struct device *dev)
+static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *state)
 {
 	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 (IS_ERR(state))
+		return 0; /* No such state */
+	ret = pinctrl_select_state(pins->p, state);
 	if (ret)
-		dev_err(dev, "failed to activate default pinctrl state\n");
+		dev_err(dev, "failed to activate pinctrl state %s\n",
+			state->name);
 	return ret;
 }
+
+/**
+ * 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)
+{
+	return pinctrl_pm_select_state(dev, dev->pins->default_state);
+}
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
 
 /**
@@ -1252,17 +1263,7 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
  */
 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 sleep state */
-	ret = pinctrl_select_state(pins->p, pins->sleep_state);
-	if (ret)
-		dev_err(dev, "failed to activate pinctrl sleep state\n");
-	return ret;
+	return pinctrl_pm_select_state(dev, dev->pins->sleep_state);
 }
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
 
@@ -1272,17 +1273,7 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
  */
 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 idle state */
-	ret = pinctrl_select_state(pins->p, pins->idle_state);
-	if (ret)
-		dev_err(dev, "failed to activate pinctrl idle state\n");
-	return ret;
+	return pinctrl_pm_select_state(dev, dev->pins->idle_state);
 }
 EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
 #endif

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Grygorii Strashko]

Hi Tony,

This patch causes boot failure when I've applied my patch
"[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
https://lkml.org/lkml/2013/6/21/309

on top of it:

[    0.264007] L310 cache controller enabled
[    0.268310] l2x0: 16 ways, CACHE_ID 0x410000c4, AUX_CTRL 0x7e470000, 
Cache size: 1048576 B
[    0.282440] CPU1: Booted secondary processor
[    0.366760] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
[    0.367401] Brought up 2 CPUs
[    0.380920] SMP: Total of 2 processors activated (2387.96 BogoMIPS).
[    0.387573] CPU: All CPU(s) started in SVC mode.
[    0.395324] devtmpfs: initialized
[    0.468658] pinctrl core: initialized pinctrl subsystem
[    0.477996] regulator-dummy: no parameters
[    0.485412] NET: Registered protocol family 16
[    0.499145] DMA: preallocated 256 KiB pool for atomic coherent 
allocations
[    0.573181] Unable to handle kernel NULL pointer dereference at 
virtual address 00000008
[    0.581573] pgd = c0004000
[    0.584472] [00000008] *pgd=00000000
[    0.588226] Internal error: Oops: 5 [#1] SMP ARM
[    0.593078] Modules linked in:
[    0.596313] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
3.11.0-rc1-00005-g37e15e6-dirty #100
[    0.604888] task: de07f3c0 ti: de080000 task.ti: de080000
[    0.610565] PC is at pinctrl_pm_select_active_state+0x4/0xc
[    0.616394] LR is at _od_runtime_resume+0xc/0x20
[    0.621215] pc : [<c02d4e2c>]    lr : [<c002e930>]    psr: 60000193
[    0.621215] sp : de081cc0  ip : 60000193  fp : 00000000
[    0.633209] r10: de080000  r9 : c0067e90  r8 : 00000004
[    0.638671] r7 : c07800c0  r6 : c002e924  r5 : de0cf4a0  r4 : de0cf410
[    0.645477] r3 : 00000000  r2 : 00000004  r1 : 00000470  r0 : de0cf410
[    0.652282] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM 
Segment kernel
[    0.660003] Control: 10c53c7d  Table: 8000404a  DAC: 00000017
[    0.665985] Process swapper/0 (pid: 1, stack limit = 0xde080240)
[    0.672241] Stack: (0xde081cc0 to 0xde082000)
[    0.676818] 1cc0: de0cf410 c0335310 de0cf410 de0cb410 00000001 
c0335374 de0cf410 de0cb410
[    0.685333] 1ce0: 00000001 c033664c c0336b14 00000004 c1bbdedc 
00000000 00000000 c0507c5c
[    0.693847] 1d00: de0cf4a0 de0cf410 60000113 00000004 c1bbdedc 
00000000 00000000 c0336b24
[    0.702362] 1d20: de07f3c0 de11ea10 de0cf410 de0cf400 de0cd780 
c02df144 de0cd740 00000000
[    0.710845] 1d40: de0cf410 c0d6a634 de0cf410 00000000 00000000 
c07efe7c 00000000 00000000
[    0.719360] 1d60: 00000000 c032d794 c032d77c c032c554 de0cf410 
c032c784 00000000 00000000
[    0.727874] 1d80: c0d6a5f0 c032ac98 de07bed8 de0fd714 de0cf410 
de0cf444 c07f65e8 c032c48c
[    0.736389] 1da0: de0cf410 de0cf410 c07f65e8 c032b[RFC] ARM: OMAP2+: 
omap_device: add pinctrl handlinga94 de0cf410 de0cf418 de0cb410 c0329ef8
[    0.744873] 1dc0: 4a3101ff 00000000 00000200 00000000 00000000 
00000000 c076f630 00000000
[    0.753387] 1de0: de0cf400 00000000 de0cb410 c076f630 de0cb410 
00000000 00000000 c042c2dc
[    0.761901] 1e00: de0cb410 c1bbdedc 00000000 00000000 00000001 
c042c3dc 00000001 c076f630
[    0.770385] 1e20: 00000000 de0cb410 00000000 c0099068 60000113 
c080b22c c1bbdd98 00000001
[    0.778900] 1e40: c076f630 c1bbcaec 00000000 c1bbdedc 00000001 
c076f630 00000000 de0cb410
[    0.787414] 1e60: 00000000 c042c440 00000001 c076f630 00000000 
00000001 00000000 c0099068
[    0.795928] 1e80: 60000113 c080b22c 00000000 00000000 c076f630 
c1bbcaec c1bbc09c 00000000
[    0.804412] 1ea0: 00000000 c076f630 00000000 00000001 00000000 
c042c4e4 00000001 c072c37c
[    0.812927] 1ec0: c07221e0 de080000 c0762648 00000000 000000a4 
c077979c c071f1c8 c0730a6c
[    0.821441] 1ee0: c0760aec c07221fc 00000000 c0008978 00000083 
de07f3c0 60000193 00000000
[    0.829925] 1f00: 00000006 c07dbcd4 c07dbcd4 60000113 c02bf100 
00000000 c07dbcd0 c07dbcd0
[    0.838439] 1f20: 00000000 c0507c5c 00000002 de080000 c06f1920 
c1bc531d 000000a4 c00655ec
[    0.846954] 1f40: c06b2bd8 c06f0ee0 00000003 00000003 60000113 
c0762668 00000003 c0762648
[    0.855468] 1f60: c0817500 000000a4 c077979c c071f1c8 00000000 
c071f91c 00000003 00000003
[    0.863952] 1f80: c071f1c8 00000000 00000000 c04fd9ac 00000000 
00000000 00000000 00000000
[    0.872467] 1fa0: 00000000 c04fd9b4 00000000 c0013ac8 00000000 
00000000 00000000 00000000
[    0.880981] 1fc0: 00000000 00000000 00000000 00000000 00000000 
00000000 00000000 00000000
[    0.889465] 1fe0: 00000000 00000000 00000000 00000000 00000013 
00000000 ce5331ac 3153ceac
[    0.898010] [<c02d4e2c>] (pinctrl_pm_select_active_state+0x4/0xc) 
from [<c002e930>] (_od_runtime_resume+0xc/0x20)
[    0.908660] [<c002e930>] (_od_runtime_resume+0xc/0x20) from 
[<c0335310>] (__rpm_callback+0x34/0x70)
[    0.918060] [<c0335310>] (__rpm_callback+0x34/0x70) from [<c0335374>] 
(rpm_callback+0x28/0x88)
[    0.927032] [<c0335374>] (rpm_callback+0x28/0x88) from [<c033664c>] 
(rpm_resume+0x3c8/0x624)
[    0.935821] [<c033664c>] (rpm_resume+0x3c8/0x624) from [<c0336b24>] 
(__pm_runtime_resume+0x48/0x60)
[    0.945220] [<c0336b24>] (__pm_runtime_resume+0x48/0x60) from 
[<c02df144>] (omap_gpio_probe+0x254/0x6c0)
[    0.955078] [<c02df144>] (omap_gpio_probe+0x254/0x6c0) from 
[<c032d794>] (platform_drv_probe+0x18/0x1c)
[    0.964843] [<c032d794>] (platform_drv_probe+0x18/0x1c) from 
[<c032c554>] (driver_probe_device+0x88/0x220)
[    0.974884] [<c032c554>] (driver_probe_device+0x88/0x220) from 
[<c032ac98>] (bus_for_each_drv+0x5c/0x88)
[    0.984710] [<c032ac98>] (bus_for_each_drv+0x5c/0x88) from 
[<c032c48c>] (device_attach+0x80/0xa4)
[    0.993957] [<c032c48c>] (device_attach+0x80/0xa4) from [<c032ba94>] 
(bus_probe_device+0x88/0xac)
[    1.003173] [<c032ba94>] (bus_probe_device+0x88/0xac) from 
[<c0329ef8>] (device_add+0x418/0x61c)
[    1.012329] [<c0329ef8>] (device_add+0x418/0x61c) from [<c042c2dc>] 
(of_platform_device_create_pdata+0x5c/0x80)
[    1.022796] [<c042c2dc>] (of_platform_device_create_pdata+0x5c/0x80) 
from [<c042c3dc>] (of_platform_bus_create+0xdc/0x184)
[    1.034271] [<c042c3dc>] (of_platform_bus_create+0xdc/0x184) from 
[<c042c440>] (of_platform_bus_create+0x140/0x184)
[    1.045104] [<c042c440>] (of_platform_bus_create+0x140/0x184) from 
[<c042c4e4>] (of_platform_populate+0x60/0x98)
[    1.055664] [<c042c4e4>] (of_platform_populate+0x60/0x98) from 
[<c0730a6c>] (omap_generic_init+0x24/0x60)
[    1.065612] [<c0730a6c>] (omap_generic_init+0x24/0x60) from 
[<c07221fc>] (customize_machine+0x1c/0x40)
[    1.075286] [<c07221fc>] (customize_machine+0x1c/0x40) from 
[<c0008978>] (do_one_initcall+0xe4/0x148)
[    1.084869] [<c0008978>] (do_one_initcall+0xe4/0x148) from 
[<c071f91c>] (kernel_init_freeable+0xfc/0x1c8)
[    1.094818] [<c071f91c>] (kernel_init_freeable+0xfc/0x1c8) from 
[<c04fd9b4>] (kernel_init+0x8/0xe4)
[    1.104248] [<c04fd9b4>] (kernel_init+0x8/0xe4) from [<c0013ac8>] 
(ret_from_fork+0x14/0x2c)
[    1.112915] Code: e59031b4 e593100c eaffffde e59031b4 (e5931008)
[    1.119323] ---[ end trace fd7907bbe82cc699 ]---
[    1.124206] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b
[    1.124206]
[    1.133789] CPU1: stopping
[    1.136657] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D 
3.11.0-rc1-00005-g37e15e6-dirty #100
[    1.146270] [<c001b62c>] (unwind_backtrace+0x0/0xf0) from 
[<c00177fc>] (show_stack+0x10/0x14)
[    1.155151] [<c00177fc>] (show_stack+0x10/0x14) from [<c05027cc>] 
(dump_stack+0x70/0x8c)
[    1.163574] [<c05027cc>] (dump_stack+0x70/0x8c) from [<c0019384>] 
(handle_IPI+0x130/0x158)
[    1.172180] [<c0019384>] (handle_IPI+0x130/0x158) from [<c0008760>] 
(gic_handle_irq+0x54/0x5c)
[    1.181121] [<c0008760>] (gic_handle_irq+0x54/0x5c) from [<c0508624>] 
(__irq_svc+0x44/0x5c)
[    1.189819] Exception stack(0xde0a7f90 to 0xde0a7fd8)
[    1.195098] 7f80:                                     c0014ca8 
000002da 00000000 00000000
[    1.203613] 7fa0: de0a6000 00000000 10c03c7d c0817774 c0514820 
c0815e40 c07869a8 00000000
[    1.212097] 7fc0: 01000000 de0a7fd8 c0014ca8 c0014cac 60000113 ffffffff
[    1.219024] [<c0508624>] (__irq_svc+0x44/0x5c) from [<c0014cac>] 
(arch_cpu_idle+0x38/0x44)
[    1.227630] [<c0014cac>] (arch_cpu_idle+0x38/0x44) from [<c00875d4>] 
(cpu_startup_entry+0xa8/0x228)
[    1.237030] [<c00875d4>] (cpu_startup_entry+0xa8/0x228) from 
[<80008264>] (0x80008264)


On 07/16/2013 12:05 PM, Tony Lindgren wrote:
> There's no need to duplicate essentially the same functions. Let's
> introduce static int pinctrl_pm_select_state() and make the other
> related functions call that.
>
> This allows us to add support later on for multiple active states,
> and more optimized dynamic remuxing.
>
> Note that we still need to export the various pinctrl_pm_select
> functions as we want to keep struct pinctrl_state private to the
> pinctrl code, and cannot replace those with inline functions.
>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/pinctrl/core.c |   47 +++++++++++++++++++----------------------------
>   1 file changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 5b272bf..bda2c61 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1227,23 +1227,34 @@ EXPORT_SYMBOL_GPL(pinctrl_force_default);
>   #ifdef CONFIG_PM
>
>   /**
> - * pinctrl_pm_select_default_state() - select default pinctrl state for PM
> + * pinctrl_pm_select_state() - select pinctrl state for PM
>    * @dev: device to select default state for
> + * @state: state to set
>    */
> -int pinctrl_pm_select_default_state(struct device *dev)
> +static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *state)
>   {
>   	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 (IS_ERR(state))
> +		return 0; /* No such state */
> +	ret = pinctrl_select_state(pins->p, state);
>   	if (ret)
> -		dev_err(dev, "failed to activate default pinctrl state\n");
> +		dev_err(dev, "failed to activate pinctrl state %s\n",
> +			state->name);
>   	return ret;
>   }
> +
> +/**
> + * 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)
> +{

Seems, need to keep check for !dev->pins here
if (!dev->pins)
	                return 0;

> +	return pinctrl_pm_select_state(dev, dev->pins->default_state);
> +}
>   EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
>
>   /**
> @@ -1252,17 +1263,7 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
>    */
>   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 sleep state */
> -	ret = pinctrl_select_state(pins->p, pins->sleep_state);
> -	if (ret)
> -		dev_err(dev, "failed to activate pinctrl sleep state\n");
> -	return ret;
here
if (!dev->pins)
	                return 0;
> +	return pinctrl_pm_select_state(dev, dev->pins->sleep_state);
>   }
>   EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
>
> @@ -1272,17 +1273,7 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
>    */
>   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 idle state */
> -	ret = pinctrl_select_state(pins->p, pins->idle_state);
> -	if (ret)
> -		dev_err(dev, "failed to activate pinctrl idle state\n");
> -	return ret;
here
if (!dev->pins)
	                return 0;
> +	return pinctrl_pm_select_state(dev, dev->pins->idle_state);
>   }
>   EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
>   #endif
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [130716 06:22]:
> Hi Tony,
> 
> This patch causes boot failure when I've applied my patch
> "[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
> https://lkml.org/lkml/2013/6/21/309
>
> on top of it:

Hmm this patch alone removes duplicate code and if it causes
issues I must have made a typo somewhere.

I cannot see a typo though, but in your dmesg I see something..

> [    0.610565] PC is at pinctrl_pm_select_active_state+0x4/0xc

..looks like you have something applied for the active_state
that only gets introduced later on in this series.

Maybe you have the earlier version of drivers/base/pinctrl.c
active_state patch that was in linux next for a while but
got reverted as we noticed we had to rework some things?

So maybe try with v3.11-rc1 + these four patches + your
omap_device patch?

Regards,

Tony

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Grygorii Strashko]

Hi Tony,

On 07/16/2013 04:41 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [130716 06:22]:
>> Hi Tony,
>>
>> This patch causes boot failure when I've applied my patch
>> "[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
>> https://lkml.org/lkml/2013/6/21/309
>>
>> on top of it:
>
> Hmm this patch alone removes duplicate code and if it causes
> issues I must have made a typo somewhere.

No typo :) You've removed the check for !dev-pins.
And the failure place is:
int pinctrl_pm_select_active_state(struct device *dev)
{
    return pinctrl_pm_select_state(dev, dev->pins->active_state);
                                                 ^^^^ here
}

If I understand everything right, the pinctrl support in Device core
assumed to be optional - so, It's valid case, when there are no
definition for device's pinctrl in DT at all.

And in this case dev->pins == NULL and  pinctrl_pm_select_*() API
should return 0 always.

>
> I cannot see a typo though, but in your dmesg I see something..
>
>> [    0.610565] PC is at pinctrl_pm_select_active_state+0x4/0xc
Yep. This will happen if there are no pinctrl states defined in DT
- currently it's crashed when GPIO driver probed.
>
> ..looks like you have something applied for the active_state
> that only gets introduced later on in this series.
>
> Maybe you have the earlier version of drivers/base/pinctrl.c
> active_state patch that was in linux next for a while but
> got reverted as we noticed we had to rework some things?
>
> So maybe try with v3.11-rc1 + these four patches + your
> omap_device patch?
I'm on v3.11-rc1

>
> Regards,
>
> Tony
>

[PATCH 1/4] pinctrl: Remove duplicate code in pinctrl_pm_select_state functions

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [130716 07:32]:
> Hi Tony,
> 
> On 07/16/2013 04:41 PM, Tony Lindgren wrote:
> >* Grygorii Strashko <grygorii.strashko@ti.com> [130716 06:22]:
> >>Hi Tony,
> >>
> >>This patch causes boot failure when I've applied my patch
> >>"[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
> >>https://lkml.org/lkml/2013/6/21/309
> >>
> >>on top of it:
> >
> >Hmm this patch alone removes duplicate code and if it causes
> >issues I must have made a typo somewhere.
> 
> No typo :) You've removed the check for !dev-pins.

Oh OK, sorry that was not intentional.

> And the failure place is:
> int pinctrl_pm_select_active_state(struct device *dev)
> {
>    return pinctrl_pm_select_state(dev, dev->pins->active_state);
>                                                 ^^^^ here
> }
> 
> If I understand everything right, the pinctrl support in Device core
> assumed to be optional - so, It's valid case, when there are no
> definition for device's pinctrl in DT at all.
> 
> And in this case dev->pins == NULL and  pinctrl_pm_select_*() API
> should return 0 always.

Care to post your patch as it sounds like you have it fixed
and tested?

Regards,

Tony