[PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states
We want to have static pin states handled separately from dynamic pin states, so let's add optional state_active. Then if state_active is defined, let's check and make sure state_idle and state_sleep match state_active for the pin groups to avoid checking them during runtime as the active and idle pins may need to be toggled for many devices every time we enter and exit idle. See also Documentation/pinctrl.txt regarding runtime muxing of pins. Cc: Felipe Balbi Cc: Greg Kroah-Hartman Cc: Grygorii Strashko Cc: Stephen Warren Signed-off-by: Tony Lindgren --- 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 * +* +* 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; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states
* Tony Lindgren [130718 00:57]: > * Stephen Warren [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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states
* Stephen Warren [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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states
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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states
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 Cc: Greg Kroah-Hartman Signed-off-by: Tony Lindgren --- 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 * +* +* 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; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/