* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-08 1:17 [PATCH] pinctrl: Really force states during suspend/resume Florian Fainelli
@ 2017-02-08 21:46 ` Andy Shevchenko
2017-02-08 22:09 ` Florian Fainelli
2017-02-23 10:30 ` Linus Walleij
2017-02-14 14:46 ` Florian Fainelli
2017-02-23 10:23 ` Linus Walleij
2 siblings, 2 replies; 8+ messages in thread
From: Andy Shevchenko @ 2017-02-08 21:46 UTC (permalink / raw)
To: Florian Fainelli
Cc: Linus Walleij, Al Cooper, swarren,
open list:PIN CONTROL SUBSYSTEM, open list
On Wed, Feb 8, 2017 at 3:17 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> In case a platform only defaults a "default" set of pins, but not a
> "sleep" set of pins, and this particular platform suspends and resumes
> in a way that the pin states are not preserved by the hardware, when we
> resume, we would call pinctrl_single_resume() -> pinctrl_force_default()
> -> pinctrl_select_state() and the first thing we do is check that the
> pins state is the same as before, and do nothing.
>
> In order to fix this, decouple pinctrl_select_state and make it become
> __pinctrl_select_state(), taking an additional ignore_state_check
> boolean which allows us to bypass the state check during suspend/resume,
> since we really want to re-apply the previous pin states in these case.
>
> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Linus,
>
> This is against your "fixes" branch, thank you!
Btw, I have got similar issue and thinking about those states they are
quite orthogonal to the pin states. Wouldn't be better to actually
differentiate PM related states and pin states?
In my case I have a ->probe() function where device is requested GPIO
in order to make it wake capable source without using anywhere else.
So, this requires to have "init" state to be defined which is kinda
inconvenient.
On resume/suspend it calls pinctrl_pm_state*() and requires "default"
and "sleep" states to be defined.
I think GPIO case is quite generic and pin control framework lacks of
something like switching some pins of the group to GPIO state and back
whenever they defined as wake capable sources.
I would work towards fixing this issue anyway (to get UART runtime PM
working on serial consoles).
>
> drivers/pinctrl/core.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index fb38e208f32d..8b8017dc9e15 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -992,17 +992,21 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>
> /**
> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW
> + * __pinctrl_select_state() - select/activate/program a pinctrl state to HW
> * @p: the pinctrl handle for the device that requests configuration
> * @state: the state handle to select/activate/program
> + * @force: ignore the state check (e.g: to re-apply default state during
> + * suspend/resume)
> */
> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
> +static int __pinctrl_select_state(struct pinctrl *p,
> + struct pinctrl_state *state,
> + bool ignore_state_check)
> {
> struct pinctrl_setting *setting, *setting2;
> struct pinctrl_state *old_state = p->state;
> int ret;
>
> - if (p->state == state)
> + if (p->state == state && !ignore_state_check)
> return 0;
>
> if (p->state) {
> @@ -1068,6 +1072,16 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>
> return ret;
> }
> +
> +/**
> + * pinctrl_select_state() - select/activate/program a pinctrl state to HW
> + * @p: the pinctrl handle for the device that requests configuration
> + * @state: the state handle to select/activate/program
> + */
> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
> +{
> + return __pinctrl_select_state(p, state, false);
> +}
> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>
> static void devm_pinctrl_release(struct device *dev, void *res)
> @@ -1236,7 +1250,8 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
> int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
> {
> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
> + return __pinctrl_select_state(pctldev->p,
> + pctldev->hog_sleep, true);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> @@ -1248,7 +1263,8 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> int pinctrl_force_default(struct pinctrl_dev *pctldev)
> {
> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
> - return pinctrl_select_state(pctldev->p, pctldev->hog_default);
> + return __pinctrl_select_state(pctldev->p,
> + pctldev->hog_default, true);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_default);
> --
> 2.9.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-08 21:46 ` Andy Shevchenko
@ 2017-02-08 22:09 ` Florian Fainelli
2017-02-23 10:30 ` Linus Walleij
1 sibling, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2017-02-08 22:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Al Cooper, swarren,
open list:PIN CONTROL SUBSYSTEM, open list
On 02/08/2017 01:46 PM, Andy Shevchenko wrote:
> On Wed, Feb 8, 2017 at 3:17 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> In case a platform only defaults a "default" set of pins, but not a
>> "sleep" set of pins, and this particular platform suspends and resumes
>> in a way that the pin states are not preserved by the hardware, when we
>> resume, we would call pinctrl_single_resume() -> pinctrl_force_default()
>> -> pinctrl_select_state() and the first thing we do is check that the
>> pins state is the same as before, and do nothing.
>>
>> In order to fix this, decouple pinctrl_select_state and make it become
>> __pinctrl_select_state(), taking an additional ignore_state_check
>> boolean which allows us to bypass the state check during suspend/resume,
>> since we really want to re-apply the previous pin states in these case.
>>
>> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Linus,
>>
>> This is against your "fixes" branch, thank you!
>
> Btw, I have got similar issue and thinking about those states they are
> quite orthogonal to the pin states. Wouldn't be better to actually
> differentiate PM related states and pin states?
This is the first time that I had to look that deep in
drivers/pinctrl/core.c, so you and Linus would probably know better
about how to get this fixed properly, this was simple enough it could be
backported to -stable kernels would that be a valid fix.
>
> In my case I have a ->probe() function where device is requested GPIO
> in order to make it wake capable source without using anywhere else.
> So, this requires to have "init" state to be defined which is kinda
> inconvenient.
>
> On resume/suspend it calls pinctrl_pm_state*() and requires "default"
> and "sleep" states to be defined.
>
> I think GPIO case is quite generic and pin control framework lacks of
> something like switching some pins of the group to GPIO state and back
> whenever they defined as wake capable sources.
>
> I would work towards fixing this issue anyway (to get UART runtime PM
> working on serial consoles).
Would this proposed patch here fix your case, or do you need to address
it differently because these are GPIOs and not a simple pinctrl-single
DT node with just a default function?
>
>>
>> drivers/pinctrl/core.c | 26 +++++++++++++++++++++-----
>> 1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index fb38e208f32d..8b8017dc9e15 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -992,17 +992,21 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>
>> /**
>> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> + * __pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> * @p: the pinctrl handle for the device that requests configuration
>> * @state: the state handle to select/activate/program
>> + * @force: ignore the state check (e.g: to re-apply default state during
>> + * suspend/resume)
>> */
>> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>> +static int __pinctrl_select_state(struct pinctrl *p,
>> + struct pinctrl_state *state,
>> + bool ignore_state_check)
>> {
>> struct pinctrl_setting *setting, *setting2;
>> struct pinctrl_state *old_state = p->state;
>> int ret;
>>
>> - if (p->state == state)
>> + if (p->state == state && !ignore_state_check)
>> return 0;
>>
>> if (p->state) {
>> @@ -1068,6 +1072,16 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>>
>> return ret;
>> }
>> +
>> +/**
>> + * pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> + * @p: the pinctrl handle for the device that requests configuration
>> + * @state: the state handle to select/activate/program
>> + */
>> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>> +{
>> + return __pinctrl_select_state(p, state, false);
>> +}
>> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>>
>> static void devm_pinctrl_release(struct device *dev, void *res)
>> @@ -1236,7 +1250,8 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
>> int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
>> {
>> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
>> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
>> + return __pinctrl_select_state(pctldev->p,
>> + pctldev->hog_sleep, true);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> @@ -1248,7 +1263,8 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> int pinctrl_force_default(struct pinctrl_dev *pctldev)
>> {
>> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
>> - return pinctrl_select_state(pctldev->p, pctldev->hog_default);
>> + return __pinctrl_select_state(pctldev->p,
>> + pctldev->hog_default, true);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_default);
>> --
>> 2.9.3
>>
>
>
>
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-08 21:46 ` Andy Shevchenko
2017-02-08 22:09 ` Florian Fainelli
@ 2017-02-23 10:30 ` Linus Walleij
2017-02-27 22:53 ` Andy Shevchenko
1 sibling, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2017-02-23 10:30 UTC (permalink / raw)
To: Andy Shevchenko, Ulf Hansson
Cc: Florian Fainelli, Al Cooper, Stephen Warren,
open list:PIN CONTROL SUBSYSTEM, open list
On Wed, Feb 8, 2017 at 10:46 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Btw, I have got similar issue and thinking about those states they are
> quite orthogonal to the pin states. Wouldn't be better to actually
> differentiate PM related states and pin states?
I don't fully understand what you mean here, but I like the sound
of it.
"sleep" and "default" were traditionally related to the system
suspend/resume states. It was suggested that the core handle this
automatically, but it doesn't work because of things like that
userspace can disable a TTY/UART and then it should sleep,
regardless of the state of the system.
Runtime PM "sleep" and "resume" is closer to what we want to
achieve here, and might be a good integration point.
(CC:ing Ulf, he's looking into things like this.)
> In my case I have a ->probe() function where device is requested GPIO
> in order to make it wake capable source without using anywhere else.
> So, this requires to have "init" state to be defined which is kinda
> inconvenient.
>
> On resume/suspend it calls pinctrl_pm_state*() and requires "default"
> and "sleep" states to be defined.
>
> I think GPIO case is quite generic and pin control framework lacks of
> something like switching some pins of the group to GPIO state and back
> whenever they defined as wake capable sources.
I guess by "GPIO state" you are referring to what is discussed in
Documentation/pinctrl.txt as "GPIO mode pitfalls", i.e. it is not really
used as a GPIO, but as part of a device functionality it just happens
that the TRM calls the asynchronous (low power mode) edge detector
mode "GPIO".
> I would work towards fixing this issue anyway (to get UART runtime PM
> working on serial consoles).
Everyone would be grateful for that.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-23 10:30 ` Linus Walleij
@ 2017-02-27 22:53 ` Andy Shevchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2017-02-27 22:53 UTC (permalink / raw)
To: Linus Walleij
Cc: Ulf Hansson, Florian Fainelli, Al Cooper, Stephen Warren,
open list:PIN CONTROL SUBSYSTEM, open list
On Thu, Feb 23, 2017 at 12:30 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Wed, Feb 8, 2017 at 10:46 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
>> Btw, I have got similar issue and thinking about those states they are
>> quite orthogonal to the pin states. Wouldn't be better to actually
>> differentiate PM related states and pin states?
>
> I don't fully understand what you mean here, but I like the sound
> of it.
>
> "sleep" and "default" were traditionally related to the system
> suspend/resume states. It was suggested that the core handle this
> automatically, but it doesn't work because of things like that
> userspace can disable a TTY/UART and then it should sleep,
> regardless of the state of the system.
>
> Runtime PM "sleep" and "resume" is closer to what we want to
> achieve here, and might be a good integration point.
> (CC:ing Ulf, he's looking into things like this.)
Yeah, so, what I meant is that pinctrl so called "states" are so
abstract and PM related that:
- doesn't allow clearly integrate them to ACPI (I suppose we will get
support of pin configuration and pin muxing support in ACPI
officially)
- requires too many explicit calls and hacks to make it work in more
or less standard cases (for example, when we request GPIO in ->probe()
solely to get a wake capable source, which requires to have defined 3
states instead of 2 or a hack, because pinctrl design checks for state
change during ->probe() and applies "default" if and only if there
were defined "default" state, meaning additional burden on definitions
in hardware pin control driver).
I hope it makes picture a bit clearer.
>> In my case I have a ->probe() function where device is requested GPIO
>> in order to make it wake capable source without using anywhere else.
>> So, this requires to have "init" state to be defined which is kinda
>> inconvenient.
>>
>> On resume/suspend it calls pinctrl_pm_state*() and requires "default"
>> and "sleep" states to be defined.
>>
>> I think GPIO case is quite generic and pin control framework lacks of
>> something like switching some pins of the group to GPIO state and back
>> whenever they defined as wake capable sources.
>
> I guess by "GPIO state" you are referring to what is discussed in
> Documentation/pinctrl.txt as "GPIO mode pitfalls", i.e. it is not really
> used as a GPIO, but as part of a device functionality it just happens
> that the TRM calls the asynchronous (low power mode) edge detector
> mode "GPIO".
Yes.
>> I would work towards fixing this issue anyway (to get UART runtime PM
>> working on serial consoles).
>
> Everyone would be grateful for that.
Please, check latest ACPI drafts you have access to.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-08 1:17 [PATCH] pinctrl: Really force states during suspend/resume Florian Fainelli
2017-02-08 21:46 ` Andy Shevchenko
@ 2017-02-14 14:46 ` Florian Fainelli
2017-02-23 10:23 ` Linus Walleij
2 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2017-02-14 14:46 UTC (permalink / raw)
To: linus.walleij
Cc: alcooperx, swarren, open list:PIN CONTROL SUBSYSTEM, open list
On 02/07/2017 05:17 PM, Florian Fainelli wrote:
> In case a platform only defaults a "default" set of pins, but not a
> "sleep" set of pins, and this particular platform suspends and resumes
> in a way that the pin states are not preserved by the hardware, when we
> resume, we would call pinctrl_single_resume() -> pinctrl_force_default()
> -> pinctrl_select_state() and the first thing we do is check that the
> pins state is the same as before, and do nothing.
>
> In order to fix this, decouple pinctrl_select_state and make it become
> __pinctrl_select_state(), taking an additional ignore_state_check
> boolean which allows us to bypass the state check during suspend/resume,
> since we really want to re-apply the previous pin states in these case.
>
> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Linus,
>
> This is against your "fixes" branch, thank you!
Linus, can you provide some feedback here so I know whether this should
be addressed differently? Thanks
>
> drivers/pinctrl/core.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index fb38e208f32d..8b8017dc9e15 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -992,17 +992,21 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>
> /**
> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW
> + * __pinctrl_select_state() - select/activate/program a pinctrl state to HW
> * @p: the pinctrl handle for the device that requests configuration
> * @state: the state handle to select/activate/program
> + * @force: ignore the state check (e.g: to re-apply default state during
> + * suspend/resume)
> */
> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
> +static int __pinctrl_select_state(struct pinctrl *p,
> + struct pinctrl_state *state,
> + bool ignore_state_check)
> {
> struct pinctrl_setting *setting, *setting2;
> struct pinctrl_state *old_state = p->state;
> int ret;
>
> - if (p->state == state)
> + if (p->state == state && !ignore_state_check)
> return 0;
>
> if (p->state) {
> @@ -1068,6 +1072,16 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>
> return ret;
> }
> +
> +/**
> + * pinctrl_select_state() - select/activate/program a pinctrl state to HW
> + * @p: the pinctrl handle for the device that requests configuration
> + * @state: the state handle to select/activate/program
> + */
> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
> +{
> + return __pinctrl_select_state(p, state, false);
> +}
> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>
> static void devm_pinctrl_release(struct device *dev, void *res)
> @@ -1236,7 +1250,8 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
> int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
> {
> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
> + return __pinctrl_select_state(pctldev->p,
> + pctldev->hog_sleep, true);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> @@ -1248,7 +1263,8 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> int pinctrl_force_default(struct pinctrl_dev *pctldev)
> {
> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
> - return pinctrl_select_state(pctldev->p, pctldev->hog_default);
> + return __pinctrl_select_state(pctldev->p,
> + pctldev->hog_default, true);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_default);
>
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-08 1:17 [PATCH] pinctrl: Really force states during suspend/resume Florian Fainelli
2017-02-08 21:46 ` Andy Shevchenko
2017-02-14 14:46 ` Florian Fainelli
@ 2017-02-23 10:23 ` Linus Walleij
2017-02-27 18:53 ` Florian Fainelli
2 siblings, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2017-02-23 10:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: alcooperx, Stephen Warren, open list:PIN CONTROL SUBSYSTEM,
open list
Hi Florian, a big sorry for not looking into this earlier, I've been
having a mess in my inbox... and too much fun with the Gemini
hehe :)
On Wed, Feb 8, 2017 at 2:17 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> In case a platform only defaults a "default" set of pins, but not a
> "sleep" set of pins, and this particular platform suspends and resumes
> in a way that the pin states are not preserved by the hardware, when we
> resume, we would call pinctrl_single_resume() -> pinctrl_force_default()
> -> pinctrl_select_state() and the first thing we do is check that the
> pins state is the same as before, and do nothing.
>
> In order to fix this, decouple pinctrl_select_state and make it become
> __pinctrl_select_state(), taking an additional ignore_state_check
> boolean which allows us to bypass the state check during suspend/resume,
> since we really want to re-apply the previous pin states in these case.
>
> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> /**
> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW
> + * __pinctrl_select_state() - select/activate/program a pinctrl state to HW
> * @p: the pinctrl handle for the device that requests configuration
> * @state: the state handle to select/activate/program
> + * @force: ignore the state check (e.g: to re-apply default state during
> + * suspend/resume)
> */
> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
> +static int __pinctrl_select_state(struct pinctrl *p,
> + struct pinctrl_state *state,
> + bool ignore_state_check)
OMG no __prefix functions I'm allergic to that :D
Just come up with a new name that describe what this function is
really doing. I'm picky about syntax matching semantics.
Name it pinctrl_commit_state() if nothing better comes to mind.
> static void devm_pinctrl_release(struct device *dev, void *res)
> @@ -1236,7 +1250,8 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
> int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
> {
> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
> + return __pinctrl_select_state(pctldev->p,
> + pctldev->hog_sleep, true);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> @@ -1248,7 +1263,8 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
> int pinctrl_force_default(struct pinctrl_dev *pctldev)
> {
> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
> - return pinctrl_select_state(pctldev->p, pctldev->hog_default);
> + return __pinctrl_select_state(pctldev->p,
> + pctldev->hog_default, true);
OK makes sense.
If you respin with a better function name I will queue it for fixes.
I guess also CC stable if you have problems with older kernels
including v4.10 at this point... sorry if I created a mess.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] pinctrl: Really force states during suspend/resume
2017-02-23 10:23 ` Linus Walleij
@ 2017-02-27 18:53 ` Florian Fainelli
0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2017-02-27 18:53 UTC (permalink / raw)
To: Linus Walleij
Cc: alcooperx, Stephen Warren, open list:PIN CONTROL SUBSYSTEM,
open list
On 02/23/2017 02:23 AM, Linus Walleij wrote:
> Hi Florian, a big sorry for not looking into this earlier, I've been
> having a mess in my inbox... and too much fun with the Gemini
> hehe :)
>
> On Wed, Feb 8, 2017 at 2:17 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>> In case a platform only defaults a "default" set of pins, but not a
>> "sleep" set of pins, and this particular platform suspends and resumes
>> in a way that the pin states are not preserved by the hardware, when we
>> resume, we would call pinctrl_single_resume() -> pinctrl_force_default()
>> -> pinctrl_select_state() and the first thing we do is check that the
>> pins state is the same as before, and do nothing.
>>
>> In order to fix this, decouple pinctrl_select_state and make it become
>> __pinctrl_select_state(), taking an additional ignore_state_check
>> boolean which allows us to bypass the state check during suspend/resume,
>> since we really want to re-apply the previous pin states in these case.
>>
>> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
>
>> /**
>> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> + * __pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> * @p: the pinctrl handle for the device that requests configuration
>> * @state: the state handle to select/activate/program
>> + * @force: ignore the state check (e.g: to re-apply default state during
>> + * suspend/resume)
>> */
>> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>> +static int __pinctrl_select_state(struct pinctrl *p,
>> + struct pinctrl_state *state,
>> + bool ignore_state_check)
>
> OMG no __prefix functions I'm allergic to that :D
>
> Just come up with a new name that describe what this function is
> really doing. I'm picky about syntax matching semantics.
> Name it pinctrl_commit_state() if nothing better comes to mind.
>
>> static void devm_pinctrl_release(struct device *dev, void *res)
>> @@ -1236,7 +1250,8 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
>> int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
>> {
>> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
>> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
>> + return __pinctrl_select_state(pctldev->p,
>> + pctldev->hog_sleep, true);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> @@ -1248,7 +1263,8 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> int pinctrl_force_default(struct pinctrl_dev *pctldev)
>> {
>> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
>> - return pinctrl_select_state(pctldev->p, pctldev->hog_default);
>> + return __pinctrl_select_state(pctldev->p,
>> + pctldev->hog_default, true);
>
> OK makes sense.
>
> If you respin with a better function name I will queue it for fixes.
>
> I guess also CC stable if you have problems with older kernels
> including v4.10 at this point... sorry if I created a mess.
Sounds good, let me respin a patch with the pinctrl_commit_state() used
as a name (could not come up with a better one).
Stay tuned.
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread