[PATCH] pinctrl: Really force states during suspend/resume

[PATCH] pinctrl: Really force states during suspend/resume

[Florian Fainelli]

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!

 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

[PATCH] pinctrl: Really force states during suspend/resume

[Florian Fainelli]

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!

 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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Andy Shevchenko]

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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Florian Fainelli]

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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Florian Fainelli]

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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Linus Walleij]

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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Linus Walleij]

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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Florian Fainelli]

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

Re: [PATCH] pinctrl: Really force states during suspend/resume

[Andy Shevchenko]

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