Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751872AbdCBWxc (ORCPT ); Thu, 2 Mar 2017 17:53:32 -0500 Received: from mail-qk0-f196.google.com ([209.85.220.196]:35585 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751468AbdCBWx1 (ORCPT ); Thu, 2 Mar 2017 17:53:27 -0500 Subject: Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume To: Andy Shevchenko References: <20170301183257.6554-1-f.fainelli@gmail.com> Cc: "linux-kernel@vger.kernel.org" , Linus Walleij , swarren@nvidia.com, Al Cooper , "open list:PIN CONTROL SUBSYSTEM" From: Florian Fainelli Message-ID: <304fb86d-0862-992b-4563-61151b37d060@gmail.com> Date: Thu, 2 Mar 2017 14:19:49 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4895 Lines: 129 On 03/02/2017 12:54 AM, Andy Shevchenko wrote: > On Wed, Mar 1, 2017 at 8:32 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 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 > > This change is indeed beautiful and clean. > > FWIW: > Reviewed-by: Andy Shevchenko 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