From: Florian Fainelli <f.fainelli@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
swarren@nvidia.com, Al Cooper <alcooperx@gmail.com>,
"open list:PIN CONTROL SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume
Date: Thu, 2 Mar 2017 14:19:49 -0800 [thread overview]
Message-ID: <304fb86d-0862-992b-4563-61151b37d060@gmail.com> (raw)
In-Reply-To: <CAHp75VdWVq3E3qrgtvvJK=A4yxK+yM_q3ZJd1hNnFBTj-H4Mxw@mail.gmail.com>
On 03/02/2017 12:54 AM, Andy Shevchenko wrote:
> On Wed, Mar 1, 2017 at 8:32 PM, 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 the actual state change from
>> pinctrl_select_state() and move it pinctrl_commit_state(), while keeping
>> the p->state == state check in pinctrl_select_state() not to change the
>> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
>> are updated to bypass the state check by calling pinctrl_commit_state().
>>
>> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> This change is indeed beautiful and clean.
>
> FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thanks Andy!
So actually, I have been thinking about this some more, and while this
definitively fixes my original problem with pinctrl-single, I just had
to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again,
same thing, it has got just one "default" state, so when we call
pinctrl_select_state() during driver resume, nothing happens.
So, with that in mind, it seems to me like we may actually just want to
remove the p->state == state statement entirely, even though that's an
optimization....
... or, what we could do is, expose a version of pinctrl_force_default
that takes a struct pinctrl reference instead of a struct pinctrl_dev
reference and named differently of course.
Thoughts?
>
>
>> ---
>> Changes in v3:
>>
>> - move the state check to pinctrl_select_state
>>
>> Changes in v2:
>>
>> - rename __pinctrl_select_state to pinctrl_commit_state
>>
>> drivers/pinctrl/core.c | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index fb38e208f32d..735d8f7f9d71 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -992,19 +992,16 @@ 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_commit_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)
>> +static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>> {
>> struct pinctrl_setting *setting, *setting2;
>> struct pinctrl_state *old_state = p->state;
>> int ret;
>>
>> - if (p->state == state)
>> - return 0;
>> -
>> if (p->state) {
>> /*
>> * For each pinmux setting in the old state, forget SW's record
>> @@ -1068,6 +1065,19 @@ 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)
>> +{
>> + if (p->state == state)
>> + return 0;
>> +
>> + return pinctrl_commit_state(p, state);
>> +}
>> EXPORT_SYMBOL_GPL(pinctrl_select_state);
>>
>> static void devm_pinctrl_release(struct device *dev, void *res)
>> @@ -1236,7 +1246,7 @@ 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_commit_state(pctldev->p, pctldev->hog_sleep);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> @@ -1248,7 +1258,7 @@ 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_commit_state(pctldev->p, pctldev->hog_default);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_force_default);
>> --
>> 2.9.3
>>
>
>
>
--
Florian
next prev parent reply other threads:[~2017-03-02 22:53 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-01 18:32 [PATCH fixes v3] pinctrl: Really force states during suspend/resume Florian Fainelli
2017-03-02 8:54 ` Andy Shevchenko
2017-03-02 22:19 ` Florian Fainelli [this message]
2017-03-14 10:16 ` Linus Walleij
2017-03-15 2:18 ` Florian Fainelli
2017-03-16 14:08 ` Linus Walleij
2017-06-21 21:23 ` Florian Fainelli
2017-06-29 9:17 ` Linus Walleij
2017-06-29 19:38 ` Florian Fainelli
2017-06-29 22:25 ` Linus Walleij
2018-02-19 17:25 ` Marc Zyngier
2018-02-19 18:03 ` Florian Fainelli
2018-02-19 18:57 ` Heiko Stuebner
2018-02-19 19:23 ` Marc Zyngier
2018-02-22 15:30 ` Linus Walleij
2018-02-22 18:11 ` Marc Zyngier
2018-02-19 19:11 ` Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=304fb86d-0862-992b-4563-61151b37d060@gmail.com \
--to=f.fainelli@gmail.com \
--cc=alcooperx@gmail.com \
--cc=andy.shevchenko@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=swarren@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).