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

[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

Re: [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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [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

Re: [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
>

Re: [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

[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: 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 bda2c61..8da11d5 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;
 
@@ -1484,7 +1485,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..b99e4b3 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -53,12 +53,18 @@ struct pinctrl_dev {
 #endif
 };
 
+enum pinctr_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;
 };

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

[Stephen Warren]

On 07/16/2013 03:05 AM, Tony Lindgren 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
> 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.

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

> +enum pinctr_states {

s/pinctr/pinctrl/

[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: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c                |  189 ++++++++++++++++++++++++++++++++-
 include/linux/pinctrl/consumer.h      |   46 ++++++++
 include/linux/pinctrl/devinfo.h       |    4 +
 include/linux/pinctrl/pinctrl-state.h |   15 ++-
 4 files changed, 249 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 8da11d5..4f58a97 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
 		list_for_each_entry_safe(setting, n2, &state->settings, node) {
 			pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
 					     setting);
+			pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
+					     setting);
 			list_del(&setting->node);
 			kfree(setting);
 		}
@@ -1041,6 +1043,134 @@ unapply_new_state:
 }
 EXPORT_SYMBOL_GPL(pinctrl_select_state);
 
+/**
+ * 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. Use this to check dynamic pin
+ * states during init.
+ */
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	struct pinctrl_setting *s1, *s2;
+
+	list_for_each_entry(s1, &st1->settings, node) {
+		struct pinctrl_dev *pctldev1;
+		const struct pinctrl_ops *pctlops1;
+		const unsigned *pins1;
+		unsigned num_pins1;
+		int res;
+
+		if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
+			continue;
+
+		pctldev1 = s1->pctldev;
+		pctlops1 = pctldev1->desc->pctlops;
+		res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
+					       &pins1, &num_pins1);
+		if (res) {
+			dev_dbg(dev, "could not get state1 group pins\n");
+			return -EINVAL;
+		}
+
+		list_for_each_entry(s2, &st2->settings, node) {
+			struct pinctrl_dev *pctldev2;
+			const struct pinctrl_ops *pctlops2;
+			const unsigned *pins2;
+			unsigned num_pins2;
+			int i, j, found = 0;
+
+			if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
+				continue;
+
+			pctldev2 = s2->pctldev;
+			if (pctldev1 != pctldev2) {
+				dev_dbg(dev, "pctldev must be the same for states\n");
+				return -EINVAL;
+			}
+			pctlops2 = pctldev2->desc->pctlops;
+			res = pctlops2->get_group_pins(pctldev2,
+						       s2->data.mux.group,
+						       &pins2, &num_pins2);
+			if (res) {
+				dev_dbg(dev, "could not get state2 group pins\n");
+				return -EINVAL;
+			}
+
+			for (i = 0; i < num_pins1; i++) {
+				int pin1 = pins1[i];
+
+				for (j = 0; j < num_pins2; j++) {
+					int pin2 = pins2[j];
+
+					if (pin1 == pin2) {
+						found++;
+						break;
+					}
+				}
+			}
+
+			if (found != num_pins1) {
+				dev_dbg(dev, "found only %i of %i pins\n",
+					found, num_pins1);
+				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) {
+		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;
+		}
+
+		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);
@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
 		return 0;
 	if (IS_ERR(state))
 		return 0; /* No such state */
-	ret = pinctrl_select_state(pins->p, state);
+
+	/* Configured for proper 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);
@@ -1259,6 +1395,16 @@ 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)
+{
+	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
  */
@@ -1277,6 +1423,41 @@ 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_check_state(struct device *dev,
+				  struct pinctrl_state *state)
+{
+	if (!dev)
+		return -EINVAL;
+
+	return IS_ERR(state);
+}
+
+/**
+ * pinctrl_pm_check_sleep_state() - 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_check_sleep_state(struct device *dev)
+{
+	return pinctrl_pm_check_state(dev, dev->pins->sleep_state);
+}
+EXPORT_SYMBOL_GPL(pinctrl_pm_check_sleep_state);
+
+/**
+ * pinctrl_pm_check_idle_state() - 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_check_idle_state(struct device *dev)
+{
+	return pinctrl_pm_check_state(dev, dev->pins->idle_state);
+}
+EXPORT_SYMBOL_GPL(pinctrl_pm_check_idle_state);
 #endif
 
 #ifdef CONFIG_DEBUG_FS
@@ -1483,10 +1664,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..73a82f1 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_check_sleep_state(struct device *dev);
+extern int pinctrl_pm_check_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_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_check_sleep_state(struct device *dev)
+{
+	return -ENODEV;
+}
+static inline int pinctrl_pm_check_idle_state(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_check_sleep_state(struct device *dev)
+{
+	return -ENODEV;
+}
+
+static inline int pinctrl_pm_check_idle_state(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"

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

[Felipe Balbi]

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

Hi,

On Tue, Jul 16, 2013 at 02:05:36AM -0700, Tony Lindgren wrote:
> +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> +			  struct pinctrl_state *st2)
> +{
> +	struct pinctrl_setting *s1, *s2;
> +
> +	list_for_each_entry(s1, &st1->settings, node) {
> +		struct pinctrl_dev *pctldev1;
> +		const struct pinctrl_ops *pctlops1;
> +		const unsigned *pins1;
> +		unsigned num_pins1;
> +		int res;
> +
> +		if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
> +			continue;
> +
> +		pctldev1 = s1->pctldev;
> +		pctlops1 = pctldev1->desc->pctlops;
> +		res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
> +					       &pins1, &num_pins1);
> +		if (res) {
> +			dev_dbg(dev, "could not get state1 group pins\n");
> +			return -EINVAL;
> +		}
> +
> +		list_for_each_entry(s2, &st2->settings, node) {
> +			struct pinctrl_dev *pctldev2;
> +			const struct pinctrl_ops *pctlops2;
> +			const unsigned *pins2;
> +			unsigned num_pins2;
> +			int i, j, found = 0;
> +
> +			if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
> +				continue;
> +
> +			pctldev2 = s2->pctldev;
> +			if (pctldev1 != pctldev2) {
> +				dev_dbg(dev, "pctldev must be the same for states\n");
> +				return -EINVAL;
> +			}
> +			pctlops2 = pctldev2->desc->pctlops;
> +			res = pctlops2->get_group_pins(pctldev2,
> +						       s2->data.mux.group,
> +						       &pins2, &num_pins2);
> +			if (res) {
> +				dev_dbg(dev, "could not get state2 group pins\n");
> +				return -EINVAL;
> +			}
> +
> +			for (i = 0; i < num_pins1; i++) {
> +				int pin1 = pins1[i];
> +
> +				for (j = 0; j < num_pins2; j++) {
> +					int pin2 = pins2[j];
> +
> +					if (pin1 == pin2) {
> +						found++;
> +						break;
> +					}
> +				}
> +			}

4 levels of nested loops ? Isn't this way too much ? OTOH, it points to
the fact that, perhaps, a list isn't the best data structure for
pinctrl ??

Or perhaps you could just assume that if num_pins1 == num_pins2 it's
enough ? But that will, likely, leave some uncovered corners...

-- 
balbi

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

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

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [130716 02:42]:
> Hi,
> 
> On Tue, Jul 16, 2013 at 02:05:36AM -0700, Tony Lindgren wrote:
> > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > +			  struct pinctrl_state *st2)
> > +{
> > +	struct pinctrl_setting *s1, *s2;
> > +
> > +	list_for_each_entry(s1, &st1->settings, node) {
> > +		struct pinctrl_dev *pctldev1;
> > +		const struct pinctrl_ops *pctlops1;
> > +		const unsigned *pins1;
> > +		unsigned num_pins1;
> > +		int res;
> > +
> > +		if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
> > +			continue;
> > +
> > +		pctldev1 = s1->pctldev;
> > +		pctlops1 = pctldev1->desc->pctlops;
> > +		res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
> > +					       &pins1, &num_pins1);
> > +		if (res) {
> > +			dev_dbg(dev, "could not get state1 group pins\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		list_for_each_entry(s2, &st2->settings, node) {
> > +			struct pinctrl_dev *pctldev2;
> > +			const struct pinctrl_ops *pctlops2;
> > +			const unsigned *pins2;
> > +			unsigned num_pins2;
> > +			int i, j, found = 0;
> > +
> > +			if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
> > +				continue;
> > +
> > +			pctldev2 = s2->pctldev;
> > +			if (pctldev1 != pctldev2) {
> > +				dev_dbg(dev, "pctldev must be the same for states\n");
> > +				return -EINVAL;
> > +			}
> > +			pctlops2 = pctldev2->desc->pctlops;
> > +			res = pctlops2->get_group_pins(pctldev2,
> > +						       s2->data.mux.group,
> > +						       &pins2, &num_pins2);
> > +			if (res) {
> > +				dev_dbg(dev, "could not get state2 group pins\n");
> > +				return -EINVAL;
> > +			}
> > +
> > +			for (i = 0; i < num_pins1; i++) {
> > +				int pin1 = pins1[i];
> > +
> > +				for (j = 0; j < num_pins2; j++) {
> > +					int pin2 = pins2[j];
> > +
> > +					if (pin1 == pin2) {
> > +						found++;
> > +						break;
> > +					}
> > +				}
> > +			}
> 
> 4 levels of nested loops ? Isn't this way too much ? OTOH, it points to
> the fact that, perhaps, a list isn't the best data structure for
> pinctrl ??

This check is only done during init to avoid constantly diffing the
pins during runtime like we currently do. And it's only done for the
pins that need to change during runtime for wake-up events etc. So
that's typically few to ten pins per device that need to be checked.

I agree there are things to improve for the data structures,
but that's a different patch set.
 
> Or perhaps you could just assume that if num_pins1 == num_pins2 it's
> enough ? But that will, likely, leave some uncovered corners...

Yes I considered that, but checking for the number of pins is not
enough. It's best to check them properly during init.

Regards,

Tony

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

[Stephen Warren]

On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> 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().

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

> @@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
>  		list_for_each_entry_safe(setting, n2, &state->settings, node) {
>  			pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
>  					     setting);
> +			pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
> +					     setting);

This will cause pinmux_free_setting() or pinconf_free_setting() to be
called twice on the setting object. Right now they don't do anything,
but if they start to kfree() some dynamically-allocated data that's
stored within the setting, that'll cause problems. I would suggest a
loop body something more like:

bool disable_setting = state == XXX || state == YYY;
pinctrl_free_setting(disable_setting, setting);

> +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> +			  struct pinctrl_state *st2)
> +{
> +	struct pinctrl_setting *s1, *s2;
> +
> +	list_for_each_entry(s1, &st1->settings, node) {
...
> +		list_for_each_entry(s2, &st2->settings, node) {
...
> +			if (pctldev1 != pctldev2) {
> +				dev_dbg(dev, "pctldev must be the same for states\n");
> +				return -EINVAL;
> +			}

I don't think we should require that; it's perfectly legal at the moment
for some device's pinctrl configuration to include settings from
multiple different pin controllers.

> +			for (i = 0; i < num_pins1; i++) {
> +				int pin1 = pins1[i];
> +
> +				for (j = 0; j < num_pins2; j++) {
> +					int pin2 = pins2[j];
> +
> +					if (pin1 == pin2) {
> +						found++;
> +						break;
> +					}
> +				}
> +			}
> +
> +			if (found != num_pins1) {

I guess this make sure that every entry in the dynamic state is in the
state state, but not vice-versa; the static state can affect more stuff
than the dynamic state?

If so, shouldn't that check be if (found != num_pins2)?

> +int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)

The body of this function is rather duplicated with pinctrl_select().

> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
>  		return 0;
>  	if (IS_ERR(state))
>  		return 0; /* No such state */
> -	ret = pinctrl_select_state(pins->p, state);
> +
> +	/* Configured for proper dynamic muxing? */

I don't think "proper" is correct here; it's just fine not to have any
dynamic configuration.

> +	if (!IS_ERR(dev->pins->active_state))
> +		ret = pinctrl_select_dynamic(pins->p, state);
> +	else
> +		ret = pinctrl_select_state(pins->p, state);

Hmmm. I'm not quite sure this is right... Surely this function should
just do nothing if the dynamic states aren't defined? The system should
just, and only, be in the "default" state and never do anything else.

Looking back at patch 1, I see the are
pinctrl_pm_select_{default,sleep,idle}_state(). Shouldn't that list be
active/sleep/idle, since I assume default is that "static" state, and
the other three are the "dynamic" states?

> +static int pinctrl_pm_check_state(struct device *dev,
> +				  struct pinctrl_state *state)

Naming this pinctrl_pm_is_state_error() or pinctrl_pm_is_state_ok()
might give more clue what its return value is expected to be.

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130717 14:21]:
> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > 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().
> 
> > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> 
> > @@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
> >  		list_for_each_entry_safe(setting, n2, &state->settings, node) {
> >  			pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
> >  					     setting);
> > +			pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
> > +					     setting);
> 
> This will cause pinmux_free_setting() or pinconf_free_setting() to be
> called twice on the setting object. Right now they don't do anything,
> but if they start to kfree() some dynamically-allocated data that's
> stored within the setting, that'll cause problems. I would suggest a
> loop body something more like:
> 
> bool disable_setting = state == XXX || state == YYY;
> pinctrl_free_setting(disable_setting, setting);

OK good point, I'll check it.
 
> > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > +			  struct pinctrl_state *st2)
> > +{
> > +	struct pinctrl_setting *s1, *s2;
> > +
> > +	list_for_each_entry(s1, &st1->settings, node) {
> ...
> > +		list_for_each_entry(s2, &st2->settings, node) {
> ...
> > +			if (pctldev1 != pctldev2) {
> > +				dev_dbg(dev, "pctldev must be the same for states\n");
> > +				return -EINVAL;
> > +			}
> 
> I don't think we should require that; it's perfectly legal at the moment
> for some device's pinctrl configuration to include settings from
> multiple different pin controllers.

Yes that's fine for pinctrl_select(), but let's not do that for
runtime muxing.

Here we want to do a one-time check during init to make  sure that
if active_state is defined, then the idle_state and sleep_state
must match active_state for pins.

This way we can avoid checking things over and over again during
runtime like pinctrl_select() is currently doing.
 
> > +			for (i = 0; i < num_pins1; i++) {
> > +				int pin1 = pins1[i];
> > +
> > +				for (j = 0; j < num_pins2; j++) {
> > +					int pin2 = pins2[j];
> > +
> > +					if (pin1 == pin2) {
> > +						found++;
> > +						break;
> > +					}
> > +				}
> > +			}
> > +
> > +			if (found != num_pins1) {
> 
> I guess this make sure that every entry in the dynamic state is in the
> state state, but not vice-versa; the static state can affect more stuff
> than the dynamic state?
> 
> If so, shouldn't that check be if (found != num_pins2)?

The check is that idle_state and sleep_state pins must match the
active_state pins. This is intentionally different from the current
pinctrl_select() logic.
 
> > +int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
> 
> The body of this function is rather duplicated with pinctrl_select().

Well we may be able to make pinctrl_select() use this too, I'll take
a look. Initially I'd rather not start messing with pinctrl_select()
to avoid breaking existing users.
 
> > @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
> >  		return 0;
> >  	if (IS_ERR(state))
> >  		return 0; /* No such state */
> > -	ret = pinctrl_select_state(pins->p, state);
> > +
> > +	/* Configured for proper dynamic muxing? */
> 
> I don't think "proper" is correct here; it's just fine not to have any
> dynamic configuration.

Right, that's the most common case. I'll drop the "proper".
 
> > +	if (!IS_ERR(dev->pins->active_state))
> > +		ret = pinctrl_select_dynamic(pins->p, state);
> > +	else
> > +		ret = pinctrl_select_state(pins->p, state);
> 
> Hmmm. I'm not quite sure this is right... Surely this function should
> just do nothing if the dynamic states aren't defined? The system should
> just, and only, be in the "default" state and never do anything else.

This keeps the existing behaviour. We should be able to drop the
call to pinctrl_select_state() here after the existing use in
drivers has been fixed.
 
> Looking back at patch 1, I see the are
> pinctrl_pm_select_{default,sleep,idle}_state(). Shouldn't that list be
> active/sleep/idle, since I assume default is that "static" state, and
> the other three are the "dynamic" states?

Yes after this patch set that's the case, then we have default
plus optional active. Then if active is defined, sleep and idle
pins must match active. 
 
> > +static int pinctrl_pm_check_state(struct device *dev,
> > +				  struct pinctrl_state *state)
> 
> Naming this pinctrl_pm_is_state_error() or pinctrl_pm_is_state_ok()
> might give more clue what its return value is expected to be.

OK yeah I did not like that naming either, pinctrl_pm_is_state_error()
sounds good to me.

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [130718 00:31]:
> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:21]:
> > On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > > +			  struct pinctrl_state *st2)
> > > +{
> > > +	struct pinctrl_setting *s1, *s2;
> > > +
> > > +	list_for_each_entry(s1, &st1->settings, node) {
> > ...
> > > +		list_for_each_entry(s2, &st2->settings, node) {
> > ...
> > > +			if (pctldev1 != pctldev2) {
> > > +				dev_dbg(dev, "pctldev must be the same for states\n");
> > > +				return -EINVAL;
> > > +			}
> > 
> > I don't think we should require that; it's perfectly legal at the moment
> > for some device's pinctrl configuration to include settings from
> > multiple different pin controllers.
> 
> Yes that's fine for pinctrl_select(), but let's not do that for
> runtime muxing.

Hmm reading this again, you're right, there should not be anything
preventing mixing multiple controllers as long as the states match.
 
> > > +			for (i = 0; i < num_pins1; i++) {
> > > +				int pin1 = pins1[i];
> > > +
> > > +				for (j = 0; j < num_pins2; j++) {
> > > +					int pin2 = pins2[j];
> > > +
> > > +					if (pin1 == pin2) {
> > > +						found++;
> > > +						break;
> > > +					}
> > > +				}
> > > +			}
> > > +
> > > +			if (found != num_pins1) {
> > 
> > I guess this make sure that every entry in the dynamic state is in the
> > state state, but not vice-versa; the static state can affect more stuff
> > than the dynamic state?
> > 
> > If so, shouldn't that check be if (found != num_pins2)?
> 
> The check is that idle_state and sleep_state pins must match the
> active_state pins. This is intentionally different from the current
> pinctrl_select() logic.

And here things will get messed up if the order of settings is diffrent..
So yes, pinctrl_check_dynamic() needs more work.

Regards,

Tony

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

[Stephen Warren]

On 07/18/2013 01:25 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:21]:
>> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
>>> 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().

>>> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
>>>  		return 0;
>>>  	if (IS_ERR(state))
>>>  		return 0; /* No such state */
>>> -	ret = pinctrl_select_state(pins->p, state);
>>> +
>>> +	/* Configured for proper dynamic muxing? */
>>> +	if (!IS_ERR(dev->pins->active_state))
>>> +		ret = pinctrl_select_dynamic(pins->p, state);
>>> +	else
>>> +		ret = pinctrl_select_state(pins->p, state);
>>
>> Hmmm. I'm not quite sure this is right... Surely this function should
>> just do nothing if the dynamic states aren't defined? The system should
>> just, and only, be in the "default" state and never do anything else.
> 
> This keeps the existing behaviour. We should be able to drop the
> call to pinctrl_select_state() here after the existing use in
> drivers has been fixed.

How many DT-based systems already have multiple of default/idle/sleep
states defined in DT? Right now, since the kernel code uses
pinctrl_select_state() to switch between those, the state definitions
need to define /all/ pins used by those states, not just the dynamic
ones. However, with this series in place, the default state should only
include the static pins, and the active/idle/sleep states should only
include the dynamic pins. That's a change to the DT bindings, since it
changes how the DT must be written, and causes older DTs to be
incompatible with newer kernels and vice-versa.

So, do we just ignore this DT ABI change again, or insist on doing it in
some compatible way? DT ABI-ness is a PITA:-(

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130718 12:27]:
> On 07/18/2013 01:25 AM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [130717 14:21]:
> >> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> >>> 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().
> 
> >>> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
> >>>  		return 0;
> >>>  	if (IS_ERR(state))
> >>>  		return 0; /* No such state */
> >>> -	ret = pinctrl_select_state(pins->p, state);
> >>> +
> >>> +	/* Configured for proper dynamic muxing? */
> >>> +	if (!IS_ERR(dev->pins->active_state))
> >>> +		ret = pinctrl_select_dynamic(pins->p, state);
> >>> +	else
> >>> +		ret = pinctrl_select_state(pins->p, state);
> >>
> >> Hmmm. I'm not quite sure this is right... Surely this function should
> >> just do nothing if the dynamic states aren't defined? The system should
> >> just, and only, be in the "default" state and never do anything else.
> > 
> > This keeps the existing behaviour. We should be able to drop the
> > call to pinctrl_select_state() here after the existing use in
> > drivers has been fixed.
> 
> How many DT-based systems already have multiple of default/idle/sleep
> states defined in DT? Right now, since the kernel code uses
> pinctrl_select_state() to switch between those, the state definitions
> need to define /all/ pins used by those states, not just the dynamic
> ones. However, with this series in place, the default state should only
> include the static pins, and the active/idle/sleep states should only
> include the dynamic pins. That's a change to the DT bindings, since it
> changes how the DT must be written, and causes older DTs to be
> incompatible with newer kernels and vice-versa.

Well we can keep the current behaviour with pinctrl_select_state() around
as long as needed. For the legacy cases, there is no active state defined
and we fall back to pinctrl_select_state().
 
> So, do we just ignore this DT ABI change again, or insist on doing it in
> some compatible way? DT ABI-ness is a PITA:-(

I'd vote for keeping the existing behaviour with pinctrl_select_state()
when no active state is defined.

Regards,

Tony

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

[Stephen Warren]

On 07/19/2013 01:29 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130718 12:27]:
>> On 07/18/2013 01:25 AM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:21]:
>>>> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
>>>>> 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().
>>
>>>>> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
>>>>>  		return 0;
>>>>>  	if (IS_ERR(state))
>>>>>  		return 0; /* No such state */
>>>>> -	ret = pinctrl_select_state(pins->p, state);
>>>>> +
>>>>> +	/* Configured for proper dynamic muxing? */
>>>>> +	if (!IS_ERR(dev->pins->active_state))
>>>>> +		ret = pinctrl_select_dynamic(pins->p, state);
>>>>> +	else
>>>>> +		ret = pinctrl_select_state(pins->p, state);
>>>>
>>>> Hmmm. I'm not quite sure this is right... Surely this function should
>>>> just do nothing if the dynamic states aren't defined? The system should
>>>> just, and only, be in the "default" state and never do anything else.
>>>
>>> This keeps the existing behaviour. We should be able to drop the
>>> call to pinctrl_select_state() here after the existing use in
>>> drivers has been fixed.
>>
>> How many DT-based systems already have multiple of default/idle/sleep
>> states defined in DT? Right now, since the kernel code uses
>> pinctrl_select_state() to switch between those, the state definitions
>> need to define /all/ pins used by those states, not just the dynamic
>> ones. However, with this series in place, the default state should only
>> include the static pins, and the active/idle/sleep states should only
>> include the dynamic pins. That's a change to the DT bindings, since it
>> changes how the DT must be written, and causes older DTs to be
>> incompatible with newer kernels and vice-versa.
> 
> Well we can keep the current behaviour with pinctrl_select_state() around
> as long as needed. For the legacy cases, there is no active state defined
> and we fall back to pinctrl_select_state().
>  
>> So, do we just ignore this DT ABI change again, or insist on doing it in
>> some compatible way? DT ABI-ness is a PITA:-(
> 
> I'd vote for keeping the existing behaviour with pinctrl_select_state()
> when no active state is defined.

Yes, I think that will work, since the active state cannot exist before
this new scheme is in place.

But, this needs to be very clearly spell out in the DT binding
documentation: If you have states default/idle/sleep, they're complete
alternatives, whereas if you have states default/active/idle/sleep, the
latter 3 are alternatives that build on top of the first. I foresee mass
confusion, but perhaps I'm being pessimistic.

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130719 11:59]:
> On 07/19/2013 01:29 AM, Tony Lindgren wrote:
> > 
> > I'd vote for keeping the existing behaviour with pinctrl_select_state()
> > when no active state is defined.
> 
> Yes, I think that will work, since the active state cannot exist before
> this new scheme is in place.

Right.
 
> But, this needs to be very clearly spell out in the DT binding
> documentation: If you have states default/idle/sleep, they're complete
> alternatives, whereas if you have states default/active/idle/sleep, the
> latter 3 are alternatives that build on top of the first. I foresee mass
> confusion, but perhaps I'm being pessimistic.

I'm hoping we can automate the runtime PM handling with default/active/idle
completely from the consumer driver point of view. And then when that's
working, we can probably deprecate any runtime PM related handling using
pinctr_select_state() and print warnings. And we can also improve the
documentation so no new users will use the default/idle/sleep for runtime
PM unless they really want to.

Regards,

Tony

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

[Stephen Warren]

On 07/29/2013 03:05 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130719 11:59]:
>> On 07/19/2013 01:29 AM, Tony Lindgren wrote:
>>>
>>> I'd vote for keeping the existing behaviour with pinctrl_select_state()
>>> when no active state is defined.
>>
>> Yes, I think that will work, since the active state cannot exist before
>> this new scheme is in place.
> 
> Right.
>  
>> But, this needs to be very clearly spell out in the DT binding
>> documentation: If you have states default/idle/sleep, they're complete
>> alternatives, whereas if you have states default/active/idle/sleep, the
>> latter 3 are alternatives that build on top of the first. I foresee mass
>> confusion, but perhaps I'm being pessimistic.
> 
> I'm hoping we can automate the runtime PM handling with default/active/idle
> completely from the consumer driver point of view. And then when that's
> working, we can probably deprecate any runtime PM related handling using
> pinctr_select_state() and print warnings. And we can also improve the
> documentation so no new users will use the default/idle/sleep for runtime
> PM unless they really want to.

I was thinking more about people writing the device trees that define
these states; they need to explicitly make the choice re: overlapping
states or independent states. We should not plan to obsolete any current
usage of overlapping states since that will mean an incompatible change
to the DT ABI (deprecate yes so that no more usage is added, but the
kernel should still support the old way).

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

[Linus Walleij]

On Tue, Jul 30, 2013 at 12:01 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> I was thinking more about people writing the device trees that define
> these states; they need to explicitly make the choice re: overlapping
> states or independent states. We should not plan to obsolete any current
> usage of overlapping states since that will mean an incompatible change
> to the DT ABI (deprecate yes so that no more usage is added, but the
> kernel should still support the old way).

This is another reason to not group and encode explicitly the pins
that remain unchanged during state transitions.

I prefer that either:

- when we build up the state containers in the subsystem,
 we identify overlapping pins and encode them in the state
 container somehow

or:

- when transitioning from state A -> state B we identify
 ovelapping pins or groups of pins and do not touch them
 by making calls down to the driver ->free() and ->request()
 callback pair.

or:

the pinctrl-single.c driver in it's callbacks like ->enable()
->disable(), ->request(), ->free() internally short cuts from
its knowledge of such pin shortcuts and no other drivers
are affected (nor helped) by this optimization.

Yours,
Linus Walleij

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

[Stephen Warren]

On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> 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().

Something in this series should edit Documentation/pinctrl.txt to
explain the philosophy behind the static/dynamic state split. That
philosophy and/or semantics are more important to review than the code...

Related to that, I'm worried that using pinctrl_select_state() and
pinctrl_select_dynamic() appear to be mutually-exclusive options here.
Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
PM? Does the mux setting select which states are used for runtime PM, or
does runtime PM override the basic mux setting, or must the pincrl-I2C
mux manually implement custom runtime-PM/pinctrl interaction since
there's no generic answer to those questions? How many more custom
exceptions will there be?

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130717 14:30]:
> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > 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().
> 
> Something in this series should edit Documentation/pinctrl.txt to
> explain the philosophy behind the static/dynamic state split. That
> philosophy and/or semantics are more important to review than the code...

Sure, I'll write up something on that.
 
> Related to that, I'm worried that using pinctrl_select_state() and
> pinctrl_select_dynamic() appear to be mutually-exclusive options here.

Not currently, but eventually I think that's a good idea. We should
use pinctrl_select_state() only during init time eventually because
of the diffing of states it does.

> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
> PM? Does the mux setting select which states are used for runtime PM, or
> does runtime PM override the basic mux setting, or must the pincrl-I2C
> mux manually implement custom runtime-PM/pinctrl interaction since
> there's no generic answer to those questions? How many more custom
> exceptions will there be?

The idea is that runtime PM will never touch the basic mux settings
at all. The "default" state should be considered a static state
that is claimed during driver probe, and released when the driver
is unloaded. This is typically let's say 90% of the pins for any
device.

For runtime PM, we can just toggle the PM related pinctrl states as
they have been verified to match the active state during init.

So I don't see why pinctrl-I2C would have to do anything specific.
All that is required is that the pins are grouped for the consumer
driver, and we can provide some automated checks on the states for
runtime PM.

Regards,

Tony

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

[Stephen Warren]

On 07/18/2013 01:36 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:30]:
>> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
...
>> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
>> PM? Does the mux setting select which states are used for runtime PM, or
>> does runtime PM override the basic mux setting, or must the pincrl-I2C
>> mux manually implement custom runtime-PM/pinctrl interaction since
>> there's no generic answer to those questions? How many more custom
>> exceptions will there be?
> 
> The idea is that runtime PM will never touch the basic mux settings
> at all. The "default" state should be considered a static state
> that is claimed during driver probe, and released when the driver
> is unloaded. This is typically let's say 90% of the pins for any
> device.
> 
> For runtime PM, we can just toggle the PM related pinctrl states as
> they have been verified to match the active state during init.
> 
> So I don't see why pinctrl-I2C would have to do anything specific.
> All that is required is that the pins are grouped for the consumer
> driver, and we can provide some automated checks on the states for
> runtime PM.

So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C
buses:

a) bus 1: I2C controller is muxed out onto one set of pins.

b) bus 2: I2C controller is muxed out onto another set of pins.

Now, the system could go idle in either of those 2 states, and then
later need to return to one of those states. I just don't see how that
would work, since the runtime PM code in this series switches to *an*
active state when the device becomes non-idle. If the definition of the
idle state switches the mux function for both sets of pins to some
idle/quiescent value, then you'd need to do different reprogramming when
going back to "the" active state; I guess the system would need to
remember which state was active before switching to idle, then switch
back to that same state rather than hard-coding the active state name as
"active"...

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130718 12:33]:
> On 07/18/2013 01:36 AM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [130717 14:30]:
> >> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> ...
> >> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
> >> PM? Does the mux setting select which states are used for runtime PM, or
> >> does runtime PM override the basic mux setting, or must the pincrl-I2C
> >> mux manually implement custom runtime-PM/pinctrl interaction since
> >> there's no generic answer to those questions? How many more custom
> >> exceptions will there be?
> > 
> > The idea is that runtime PM will never touch the basic mux settings
> > at all. The "default" state should be considered a static state
> > that is claimed during driver probe, and released when the driver
> > is unloaded. This is typically let's say 90% of the pins for any
> > device.
> > 
> > For runtime PM, we can just toggle the PM related pinctrl states as
> > they have been verified to match the active state during init.
> > 
> > So I don't see why pinctrl-I2C would have to do anything specific.
> > All that is required is that the pins are grouped for the consumer
> > driver, and we can provide some automated checks on the states for
> > runtime PM.
> 
> So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C
> buses:
> 
> a) bus 1: I2C controller is muxed out onto one set of pins.
> 
> b) bus 2: I2C controller is muxed out onto another set of pins.
> 
> Now, the system could go idle in either of those 2 states, and then
> later need to return to one of those states. I just don't see how that
> would work, since the runtime PM code in this series switches to *an*
> active state when the device becomes non-idle. If the definition of the
> idle state switches the mux function for both sets of pins to some
> idle/quiescent value, then you'd need to do different reprogramming when
> going back to "the" active state; I guess the system would need to
> remember which state was active before switching to idle, then switch
> back to that same state rather than hard-coding the active state name as
> "active"...

I think the only sane way to deal with this is to make the I2C controller
to show up as two separate I2C controller instances. Then use runtime
PM to save and restore the state for each instance, and locking between
the two driver instances.

For the pin muxing part, I'd do this:

			i2c1 instance	i2c2 instance	notes
default_state		0 pins		0 pins		(or dedicated pins only)
active_state		all pins	alls pins
idle_state		safe mode	safe mode

Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
to safe mode, or some nop state. Then when i2c2 instance is woken, it's
runtime PM resume muxes pins to i2c2.

Regards,

Tony

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

[Grygorii Strashko]

Hi Tony, Stephen

On 07/19/2013 10:39 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130718 12:33]:
>> On 07/18/2013 01:36 AM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:30]:
>>>> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
>> ...
>>>> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
>>>> PM? Does the mux setting select which states are used for runtime PM, or
>>>> does runtime PM override the basic mux setting, or must the pincrl-I2C
>>>> mux manually implement custom runtime-PM/pinctrl interaction since
>>>> there's no generic answer to those questions? How many more custom
>>>> exceptions will there be?
>>>
>>> The idea is that runtime PM will never touch the basic mux settings
>>> at all. The "default" state should be considered a static state
>>> that is claimed during driver probe, and released when the driver
>>> is unloaded. This is typically let's say 90% of the pins for any
>>> device.
>>>
>>> For runtime PM, we can just toggle the PM related pinctrl states as
>>> they have been verified to match the active state during init.
>>>
>>> So I don't see why pinctrl-I2C would have to do anything specific.
>>> All that is required is that the pins are grouped for the consumer
>>> driver, and we can provide some automated checks on the states for
>>> runtime PM.
>>
>> So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C
>> buses:
>>
>> a) bus 1: I2C controller is muxed out onto one set of pins.
>>
>> b) bus 2: I2C controller is muxed out onto another set of pins.
>>
>> Now, the system could go idle in either of those 2 states, and then
>> later need to return to one of those states. I just don't see how that
>> would work, since the runtime PM code in this series switches to *an*
>> active state when the device becomes non-idle. If the definition of the
>> idle state switches the mux function for both sets of pins to some
>> idle/quiescent value, then you'd need to do different reprogramming when
>> going back to "the" active state; I guess the system would need to
>> remember which state was active before switching to idle, then switch
>> back to that same state rather than hard-coding the active state name as
>> "active"...
>
> I think the only sane way to deal with this is to make the I2C controller
> to show up as two separate I2C controller instances. Then use runtime
> PM to save and restore the state for each instance, and locking between
> the two driver instances.
>
> For the pin muxing part, I'd do this:
>
> 			i2c1 instance	i2c2 instance	notes
> default_state		0 pins		0 pins		(or dedicated pins only)
> active_state		all pins	alls pins
> idle_state		safe mode	safe mode
>
> Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
> to safe mode, or some nop state. Then when i2c2 instance is woken, it's
> runtime PM resume muxes pins to i2c2.

First of all, I'd like to mention that these patches do *not* connect
pinctrl to PM runtime, so until driver will call pinctrl_select_state()
or pinctrl_pm_select_*() there will be no pins state changes.
(As result, i2c-mux is not good example, seems:))

And I think, some sort of summary need to be done to explain how system 
will behave after these patches in comparison to how it was before:

1) Device has pins states defined and driver uses pinctrl_select_state
(lets say legacy mode):
- "default" - no changes
- "default"+"idle"/"sleep" - no changes
- "default" + any other states - no chages
- "default"+"active" + "idle"/"sleep" - behavior will be *changed*
   pinctrl_bind_pins() will do:
   a) pinctrl_select_state("default")
   b) pinctrl_select_dynamic("active")
   but, driver uses pinctrl_select_state() to change pins state during
its work -- Is it ok?

2) Device has pins states defined and driver uses pinctrl_pm_select_*() 
API only:
- if no "active" defined - behavior will be the same as for legacy mode
- "default"+"active" + "idle"/"sleep" - will behave as explained in doc

3)  Device has pins states defined and driver uses
pinctrl_pm_select_*() API and pinctrl_select_state() simultaneously:
- if no "active" defined - behavior will be the same as for legacy mode
- "default"+"active" + "idle"/"sleep" + "stateX"
   How it will work if during suspend driver will do:??
   if (conditionY)
         pinctrl_select_state(stateX);
   pinctrl_pm_select_sleep_state("sleep")
Is it valid?

   How it will work if during runtime resuming driver will do:??
   if (conditionY)
         pinctrl_select_state(stateX);
   pinctrl_pm_select_active_state("active");
Is it valid?

Any way, if driver don't want support introduced default/common behavior 
- all it need is to not define "active" state.

Uh, hope it will be useful and I have correct understandings here :)

>
> Regards,
>
> Tony

Regards,
- grygorii

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

[Stephen Warren]

On 07/19/2013 04:29 AM, Grygorii Strashko wrote:
> Hi Tony, Stephen
> 
> On 07/19/2013 10:39 AM, Tony Lindgren wrote:
>> * Stephen Warren <swarren@wwwdotorg.org> [130718 12:33]:
>>> On 07/18/2013 01:36 AM, Tony Lindgren wrote:
>>>> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:30]:
>>>>> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
>>> ...
>>>>> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
>>>>> PM? Does the mux setting select which states are used for runtime
>>>>> PM, or
>>>>> does runtime PM override the basic mux setting, or must the pincrl-I2C
>>>>> mux manually implement custom runtime-PM/pinctrl interaction since
>>>>> there's no generic answer to those questions? How many more custom
>>>>> exceptions will there be?
>>>>
>>>> The idea is that runtime PM will never touch the basic mux settings
>>>> at all. The "default" state should be considered a static state
>>>> that is claimed during driver probe, and released when the driver
>>>> is unloaded. This is typically let's say 90% of the pins for any
>>>> device.
>>>>
>>>> For runtime PM, we can just toggle the PM related pinctrl states as
>>>> they have been verified to match the active state during init.
>>>>
>>>> So I don't see why pinctrl-I2C would have to do anything specific.
>>>> All that is required is that the pins are grouped for the consumer
>>>> driver, and we can provide some automated checks on the states for
>>>> runtime PM.
>>>
>>> So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C
>>> buses:
>>>
>>> a) bus 1: I2C controller is muxed out onto one set of pins.
>>>
>>> b) bus 2: I2C controller is muxed out onto another set of pins.
>>>
>>> Now, the system could go idle in either of those 2 states, and then
>>> later need to return to one of those states. I just don't see how that
>>> would work, since the runtime PM code in this series switches to *an*
>>> active state when the device becomes non-idle. If the definition of the
>>> idle state switches the mux function for both sets of pins to some
>>> idle/quiescent value, then you'd need to do different reprogramming when
>>> going back to "the" active state; I guess the system would need to
>>> remember which state was active before switching to idle, then switch
>>> back to that same state rather than hard-coding the active state name as
>>> "active"...
>>
>> I think the only sane way to deal with this is to make the I2C controller
>> to show up as two separate I2C controller instances. Then use runtime
>> PM to save and restore the state for each instance, and locking between
>> the two driver instances.
>>
>> For the pin muxing part, I'd do this:
>>
>>             i2c1 instance    i2c2 instance    notes
>> default_state        0 pins        0 pins        (or dedicated pins only)
>> active_state        all pins    alls pins
>> idle_state        safe mode    safe mode
>>
>> Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
>> to safe mode, or some nop state. Then when i2c2 instance is woken, it's
>> runtime PM resume muxes pins to i2c2.
> 
> First of all, I'd like to mention that these patches do *not* connect
> pinctrl to PM runtime, so until driver will call pinctrl_select_state()
> or pinctrl_pm_select_*() there will be no pins state changes.

Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be
called automatically by the runtime PM core, so that we don't need to
write code to do this in every single driver, just like we moved the
call to pinctrl_select_state(default) into the device core so that we
didn't have to make every device do that manually?

> (As result, i2c-mux is not good example, seems:))

As such, I think all situations are good examples, because a generic
feature has to work in all cases.

The description you gave of the behavioural changes this patch creates
seems accurate at a quick glance.

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

[Linus Walleij]

On Fri, Jul 19, 2013 at 9:03 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/19/2013 04:29 AM, Grygorii Strashko wrote:

>> First of all, I'd like to mention that these patches do *not* connect
>> pinctrl to PM runtime, so until driver will call pinctrl_select_state()
>> or pinctrl_pm_select_*() there will be no pins state changes.
>
> Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be
> called automatically by the runtime PM core,

Nah I had no such complete ambitions, just factoring around.

There are examples, such as deactivating a TTY from userspace,
that should result in the pins going to sleep while it may have nothing
to do with runtime PM.

> so that we don't need to
> write code to do this in every single driver, just like we moved the
> call to pinctrl_select_state(default) into the device core so that we
> didn't have to make every device do that manually?

I am pretty convinced that if this dynamic muxing stuff shall be
implemented, it should be a pinctrl subsystem intrinsic optimization
detail and should not be exposed to the outside with all these extra
functions at all. It looks overly complicated to me.

Yours,
Linus Walleij

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130719 12:10]:
> On 07/19/2013 04:29 AM, Grygorii Strashko wrote:
> > 
> > First of all, I'd like to mention that these patches do *not* connect
> > pinctrl to PM runtime, so until driver will call pinctrl_select_state()
> > or pinctrl_pm_select_*() there will be no pins state changes.
> 
> Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be
> called automatically by the runtime PM core, so that we don't need to
> write code to do this in every single driver, just like we moved the
> call to pinctrl_select_state(default) into the device core so that we
> didn't have to make every device do that manually?

Yes I think we can make it all automatic. So far it seems that the
last missing piece was Linus' suggestion of making it mostly happen
using irqchip with calls to pinctrl so consumer drivers may not need
to do anything.
 
> > (As result, i2c-mux is not good example, seems:))
> 
> As such, I think all situations are good examples, because a generic
> feature has to work in all cases.

Yes we need to support both runtime PM, and more complex cases of
sharing pins between devices for example that are not runtime PM
related.

Regards,

Tony

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

[Stephen Warren]

On 07/19/2013 01:39 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130718 12:33]:
>> On 07/18/2013 01:36 AM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:30]:
>>>> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
>> ...
>>>> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
>>>> PM? Does the mux setting select which states are used for runtime PM, or
>>>> does runtime PM override the basic mux setting, or must the pincrl-I2C
>>>> mux manually implement custom runtime-PM/pinctrl interaction since
>>>> there's no generic answer to those questions? How many more custom
>>>> exceptions will there be?
>>>
>>> The idea is that runtime PM will never touch the basic mux settings
>>> at all. The "default" state should be considered a static state
>>> that is claimed during driver probe, and released when the driver
>>> is unloaded. This is typically let's say 90% of the pins for any
>>> device.
>>>
>>> For runtime PM, we can just toggle the PM related pinctrl states as
>>> they have been verified to match the active state during init.
>>>
>>> So I don't see why pinctrl-I2C would have to do anything specific.
>>> All that is required is that the pins are grouped for the consumer
>>> driver, and we can provide some automated checks on the states for
>>> runtime PM.
>>
>> So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C
>> buses:
>>
>> a) bus 1: I2C controller is muxed out onto one set of pins.
>>
>> b) bus 2: I2C controller is muxed out onto another set of pins.
>>
>> Now, the system could go idle in either of those 2 states, and then
>> later need to return to one of those states. I just don't see how that
>> would work, since the runtime PM code in this series switches to *an*
>> active state when the device becomes non-idle. If the definition of the
>> idle state switches the mux function for both sets of pins to some
>> idle/quiescent value, then you'd need to do different reprogramming when
>> going back to "the" active state; I guess the system would need to
>> remember which state was active before switching to idle, then switch
>> back to that same state rather than hard-coding the active state name as
>> "active"...
> 
> I think the only sane way to deal with this is to make the I2C controller
> to show up as two separate I2C controller instances. Then use runtime
> PM to save and restore the state for each instance, and locking between
> the two driver instances.
> 
> For the pin muxing part, I'd do this:
> 
> 			i2c1 instance	i2c2 instance	notes
> default_state		0 pins		0 pins		(or dedicated pins only)
> active_state		all pins	alls pins
> idle_state		safe mode	safe mode
> 
> Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
> to safe mode, or some nop state. Then when i2c2 instance is woken, it's
> runtime PM resume muxes pins to i2c2.

I see two issues with that approach:

1) Runtime PM doesn't always put a device into an idle state as soon as
its work is done. Sometimes (always?) there is a delay between when the
device is last used and when the HW is actually made idle so that if the
device is re-activated quickly, the kernel hasn't wasted time making it
idle then active again. You'd have to force that delay to complete when
switching between the two virtual controller devices.

2) This requires two separate device objects for the two I2C instances.
I guess you could have the driver for the one I2C mux node end up
instantiating two child devices for this purpose, and hence make that
happen without needing to change the DT ABI. However, that sure feels
complex...

I wonder if it wouldn't be better to have active/idle/sleep as
"modifiers" to the state name rather than alternatives, so you end up
with states named:

default
nobus:active
nobus:idle
nobus:sleep
bus0:active
bus0:idle
bus0:sleep
bus1:active
bus1:idle
bus1:sleep

Of course, if you continue on with that approach (i.e. you add more
sub-fields each separated by a colon), you end up with a huge
combinatorial mess of state names.

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130719 12:05]:
> On 07/19/2013 01:39 AM, Tony Lindgren wrote:
> > 
> > I think the only sane way to deal with this is to make the I2C controller
> > to show up as two separate I2C controller instances. Then use runtime
> > PM to save and restore the state for each instance, and locking between
> > the two driver instances.
> > 
> > For the pin muxing part, I'd do this:
> > 
> > 			i2c1 instance	i2c2 instance	notes
> > default_state		0 pins		0 pins		(or dedicated pins only)
> > active_state		all pins	alls pins
> > idle_state		safe mode	safe mode
> > 
> > Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
> > to safe mode, or some nop state. Then when i2c2 instance is woken, it's
> > runtime PM resume muxes pins to i2c2.
> 
> I see two issues with that approach:
> 
> 1) Runtime PM doesn't always put a device into an idle state as soon as
> its work is done. Sometimes (always?) there is a delay between when the
> device is last used and when the HW is actually made idle so that if the
> device is re-activated quickly, the kernel hasn't wasted time making it
> idle then active again. You'd have to force that delay to complete when
> switching between the two virtual controller devices.

There is the autosuspend timeout for delayed transitions, but I think
runtime PM already has calls for making the state change immediate also.
 
> 2) This requires two separate device objects for the two I2C instances.
> I guess you could have the driver for the one I2C mux node end up
> instantiating two child devices for this purpose, and hence make that
> happen without needing to change the DT ABI. However, that sure feels
> complex...

Yes but you will also automatically get quite a bit of sanity to your
I2C driver code rather than trying to handle the two separate instances
within the driver alone. Consider things like scanning the I2C buses for
devices and just dev_dbg().
 
> I wonder if it wouldn't be better to have active/idle/sleep as
> "modifiers" to the state name rather than alternatives, so you end up
> with states named:
> 
> default
> nobus:active
> nobus:idle
> nobus:sleep
> bus0:active
> bus0:idle
> bus0:sleep
> bus1:active
> bus1:idle
> bus1:sleep
> 
> Of course, if you continue on with that approach (i.e. you add more
> sub-fields each separated by a colon), you end up with a huge
> combinatorial mess of state names.

Right :) Sooner or later we'll have some totally messed up piece of
hardware pop up that multiplexes one controller across a large number
number buses..

Regards,

Tony

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

[Stephen Warren]

On 07/29/2013 03:21 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [130719 12:05]:
>> On 07/19/2013 01:39 AM, Tony Lindgren wrote:
>>>
>>> I think the only sane way to deal with this is to make the I2C controller
>>> to show up as two separate I2C controller instances. Then use runtime
>>> PM to save and restore the state for each instance, and locking between
>>> the two driver instances.
>>>
>>> For the pin muxing part, I'd do this:
>>>
>>> 			i2c1 instance	i2c2 instance	notes
>>> default_state		0 pins		0 pins		(or dedicated pins only)
>>> active_state		all pins	alls pins
>>> idle_state		safe mode	safe mode
>>>
>>> Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
>>> to safe mode, or some nop state. Then when i2c2 instance is woken, it's
>>> runtime PM resume muxes pins to i2c2.
>>
>> I see two issues with that approach:
>>
>> 1) Runtime PM doesn't always put a device into an idle state as soon as
>> its work is done. Sometimes (always?) there is a delay between when the
>> device is last used and when the HW is actually made idle so that if the
>> device is re-activated quickly, the kernel hasn't wasted time making it
>> idle then active again. You'd have to force that delay to complete when
>> switching between the two virtual controller devices.
> 
> There is the autosuspend timeout for delayed transitions, but I think
> runtime PM already has calls for making the state change immediate also.

True, but I /think/ that then you could never use the APIs that perform
delayed idle, since you'd always need to force immediate idle to
guarantee you could always immediately switch to the other
state/virtual-controller.

>> 2) This requires two separate device objects for the two I2C instances.
>> I guess you could have the driver for the one I2C mux node end up
>> instantiating two child devices for this purpose, and hence make that
>> happen without needing to change the DT ABI. However, that sure feels
>> complex...
> 
> Yes but you will also automatically get quite a bit of sanity to your
> I2C driver code rather than trying to handle the two separate instances
> within the driver alone. Consider things like scanning the I2C buses for
> devices and just dev_dbg().

The I2C mux is already very simple. IIRC, it instantiates a separate
"struct i2c_adapter" for each "virtual" I2C controller. It actually
looks like each of those already embeds its own "struct device", so
perhaps this issue is a little moot. However, we'd have to find some way
of redirecting pinctrl requests from those child devices to the
top-level I2C mux struct device itself, since that's what the pinctrl
mapping table entries are defined for. It seems much simpler to just
leave the pinctrl stuff controlled by that top-level device, rather than
pushing it down to the child virtual devices/adapters.

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

[Linus Walleij]

On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren <tony@atomide.com> wrote:

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

OK...

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

I do not understand why this complexity need to be exposed outside
of the subsystem.

pinctrl_select_state() and the PM accessors are enough IMO. Why
should say a driver care about whether it is dynamic or not?

Surely the checking and different paths for static/dynamic configurations
can be an intrinsic detail of the pinctrl subsystem, by adding flags and
members to private structs like struct pinctrl itself in worst case.

So I'm not buying into this, it looks like it is making things complicated
for consumers outside the subsystem for no reason.

Yours,
Linus Walleij

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

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130722 16:14]:
> On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > 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.
> 
> OK...
> 
> > 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.
> 
> I do not understand why this complexity need to be exposed outside
> of the subsystem.

Unfortunately it's mostly to deal with supporting the current behaviour
of pinctrl_select_state() which is not quite suitable for runtime PM.
 
> pinctrl_select_state() and the PM accessors are enough IMO. Why
> should say a driver care about whether it is dynamic or not?

I think we can make this all transparent to the consumer drivers
for runtime PM. Basically drivers/base/pinctrl.c needs these for the
checks because of the current pinctrl_select_state().
 
> Surely the checking and different paths for static/dynamic configurations
> can be an intrinsic detail of the pinctrl subsystem, by adding flags and
> members to private structs like struct pinctrl itself in worst case.

I'll take a look if we can bury more things inside the pinctrl
subsystem.
 
> So I'm not buying into this, it looks like it is making things complicated
> for consumers outside the subsystem for no reason.

I don't think the consumer drivers eventually need to do much
anything ideally. We're missing runtime PM related set_irq_wake()
but that's a minor detail as we can initially keep the runtime
PM related wake-up events always enabled.

Regards,

Tony

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

Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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;

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

[Stephen Warren]

On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> 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.

> +	 * 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);

Oh, I see you're trying to check that the set of pins in the active,
sleep, and idle states are identical.

But I think that pinctrl_check_dynamic() only checks that one state is a
subset of the other, not that the two states are equal. Instead, I think
you want to comparison coded in pinctrl_check_dynamic() to be:

gen_group_list_of_pinctrl_state(s1, array1);
gen_group_list_of_pinctrl_state(s2, array2);
mismatch = memcmp(array1, array2, length);

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

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [130717 14:28]:
> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > 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.
> 
> > +	 * 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);
> 
> Oh, I see you're trying to check that the set of pins in the active,
> sleep, and idle states are identical.

Right, that's to avoid any further checking during runtime for runtime PM.
 
> But I think that pinctrl_check_dynamic() only checks that one state is a
> subset of the other, not that the two states are equal. Instead, I think
> you want to comparison coded in pinctrl_check_dynamic() to be:

In pinctrl_check_dynamic() we check that the pins match between the
states, and the number of found pins matches the first set. I'll
take a look if we check the total pins between the two sets.
 
> gen_group_list_of_pinctrl_state(s1, array1);
> gen_group_list_of_pinctrl_state(s2, array2);
> mismatch = memcmp(array1, array2, length);

Well we could allocate and sort the pins, but the number of pins
for runtime PM is typically very small for each pin consumer device.
Typically you just need to toggle RX pin to GPIO mode for idle. And
this check is only done during consumer driver probe time. So
optimizing it for larger sets could be done at any point later on
as needed.

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [130718 00:57]:
> * Stephen Warren <swarren@wwwdotorg.org> [130717 14:28]:
> > 
> > Oh, I see you're trying to check that the set of pins in the active,
> > sleep, and idle states are identical.
> 
> Right, that's to avoid any further checking during runtime for runtime PM.
>  
> > But I think that pinctrl_check_dynamic() only checks that one state is a
> > subset of the other, not that the two states are equal. Instead, I think
> > you want to comparison coded in pinctrl_check_dynamic() to be:
> 
> In pinctrl_check_dynamic() we check that the pins match between the
> states, and the number of found pins matches the first set. I'll
> take a look if we check the total pins between the two sets.

That that is a bit painful right now to check properly as we don't
have any sorting, and we could use that elsewhere too for checks
probably..
  
> > gen_group_list_of_pinctrl_state(s1, array1);
> > gen_group_list_of_pinctrl_state(s2, array2);
> > mismatch = memcmp(array1, array2, length);
> 
> Well we could allocate and sort the pins, but the number of pins
> for runtime PM is typically very small for each pin consumer device.
> Typically you just need to toggle RX pin to GPIO mode for idle. And
> this check is only done during consumer driver probe time. So
> optimizing it for larger sets could be done at any point later on
> as needed.

..so for now, let's just check the total number of pins for the sets
like Felipe suggested. I think we're better off improving the pinctrl
data first to make various checks easier.

What you're suggesting with the mepcmp() can be done easily if we add
something like device_get_pins() and have the pins sorted for the
various states for a device at the device probe time.

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [130716 02:12]:
> 
> 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.

Here's a patch showing the suggested usage remuxing SDIO IRQ
to GPIO mode for runtime PM. Note that this patch depends on
three other patches for the hsmmc driver, I'll post that series
separately.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Mon, 15 Jul 2013 07:39:39 -0700
Subject: [PATCH] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

On some omaps we need to remux MMC pins for PM, and for some omaps
we need to remux the SDIO IRQ pin.

Based on an earlier patch by Andreas Fenkart <afenkart@gmail.com>.

Cc: Andreas Fenkart <afenkart@gmail.com>
Cc: Balaji T K <balajitk@ti.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1869,7 +1869,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	dma_cap_mask_t mask;
 	unsigned tx_req, rx_req;
-	struct pinctrl *pinctrl;
 
 	match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev);
 	if (match) {
@@ -2114,21 +2113,19 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 
 	omap_hsmmc_disable_irq(host);
 
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev,
-			"pins are not configured from the driver\n");
-
 	/*
-	 * For now, only support SDIO interrupt if we are doing
-	 * muxing of dat1 when booted with DT. This is because the
+	 * For now, only support SDIO interrupt if we are doing dynamic
+	 * remuxing of dat1 when booted with DT. This is because the
 	 * supposedly the wake-up events for CTPL don't work from deeper
 	 * idle states. And we don't want to add new legacy mux platform
 	 * init code callbacks any longer as we are moving to DT based
 	 * booting anyways.
 	 */
 	if (match) {
-		if (!IS_ERR(pinctrl) && mmc_slot(host).sdio_irq)
+		struct device *dev = &pdev->dev;
+
+		if (!pinctrl_pm_check_idle_state(dev) &&
+		    mmc_slot(host).sdio_irq)
 			mmc->caps |= MMC_CAP_SDIO_IRQ;
 	}
 
@@ -2348,6 +2345,10 @@ static int omap_hsmmc_runtime_suspend(struct device *dev)
 		OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
 		spin_unlock_irqrestore(&host->irq_lock, flags);
 
+		ret = pinctrl_pm_select_idle_state(dev);
+		if (ret < 0)
+			dev_err(dev, "Unable to select idle pinmux\n");
+
 		if (mmc_slot(host).sdio_irq)
 			enable_irq(mmc_slot(host).sdio_irq);
 	}
@@ -2371,6 +2372,10 @@ static int omap_hsmmc_runtime_resume(struct device *dev)
 		if (mmc_slot(host).sdio_irq)
 			disable_irq(mmc_slot(host).sdio_irq);
 
+		ret = pinctrl_pm_select_active_state(dev);
+		if (ret < 0)
+			dev_err(dev, "Unable to select active pinmux\n");
+
 		spin_lock_irqsave(&host->irq_lock, flags);
 		host->active_pinmux = true;
 

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

[Grygorii Strashko]

Hi Tony,

On 07/16/2013 12:05 PM, Tony Lindgren wrote:
> 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.

I've posted my patch and tested this patch series.
Seems everything is ok (except patch "pinctrl: Remove duplicate code in 
pinctrl_pm_select_state functions").
See http://www.spinics.net/lists/arm-kernel/msg259180.html

So,  Tested-By: Grygorii Strashko <grygorii.strashko@ti.com>

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

Regards,
-grygorii

[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

Re: [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