* Re: [PATCH] pinctrl: samsung: add pinctrl_force_sleep() for "sleep" pinctrl state
2017-09-21 12:32 ` [PATCH] pinctrl: samsung: add pinctrl_force_sleep() for "sleep" pinctrl state 남영민
@ 2017-09-21 16:14 ` Tomasz Figa
2017-09-22 13:14 ` Linus Walleij
2017-09-22 13:09 ` Linus Walleij
1 sibling, 1 reply; 4+ messages in thread
From: Tomasz Figa @ 2017-09-21 16:14 UTC (permalink / raw)
To: 남영민
Cc: Krzysztof Kozlowski, Sylwester Nawrocki, linus.walleij@linaro.org,
linux-arm-kernel, linux-samsung-soc@vger.kernel.org,
linux-gpio@vger.kernel.org
Hi Youngmin,
2017-09-21 21:32 GMT+09:00 남영민 <youngmin.nam@samsung.com>:
> This patch adds pinctrl_for_sleep() in samsung_pinctrl_suspend()
> to support configuration of "sleep" pinctrl state.
>
> For example, we can configure "sleep" pinctrl state of "gpf1-6"
> as "INPUT, PULL DOWN" by configuring power down mode register.
>
> &pinctrl_5 {
> pinctrl-names = "default","sleep";
> pinctrl-0 = <&initial5>;
> pinctrl-1 = <&sleep5>;
>
> initial5: initial-state {
> samsung,pins = gpf1-6;
> samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> };
> sleep5: sleep-state {
> samsung,pins = gpf1-6;
> samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_DOWN>;
This isn't a very good example, because pin-con-pdn and pin-pud-pdn
are used to program dedicated sleep state registers in the pin
controller and they can be programmed at any time. Typically one would
include them in "default" state.
As for pins which don't have dedicated sleep state registers, i.e. the
alive banks, which retain their value, the DT node of device
associated with the pins would have "sleep" pinctrl state and its
driver would activate it on suspend.
The only remaining use case would be pins in alive banks not
associated with any device, but I can't imagine any practical case
that would need to change the value on suspend in such case.
Do you have an example of a real use case for this patch?
> };
> };
>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> ---
> drivers/pinctrl/samsung/pinctrl-samsung.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index e04f7fe0a65d..b4d12f8db475 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1099,6 +1099,11 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
> {
> struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
> int i;
> + int ret;
> +
> + ret = pinctrl_force_sleep(drvdata->pctl_dev);
> + if (ret)
> + dev_err(dev, "could not set sleep pinstate %d\n", ret);
>
> for (i = 0; i < drvdata->nr_banks; i++) {
> struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
> @@ -1187,6 +1192,11 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> if (drvdata->retention_ctrl && drvdata->retention_ctrl->disable)
> drvdata->retention_ctrl->disable(drvdata);
>
> + /* For changing state without writing register. */
> + if (!IS_ERR(drvdata->pctl_dev->p) &&
> + !IS_ERR(drvdata->pctl_dev->hog_default))
> + drvdata->pctl_dev->p->state = drvdata->pctl_dev->hog_default;
How is this supposed to work? The pins were reconfigured at suspend,
where are they restored back to the values before suspend?
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] pinctrl: samsung: add pinctrl_force_sleep() for "sleep" pinctrl state
2017-09-21 12:32 ` [PATCH] pinctrl: samsung: add pinctrl_force_sleep() for "sleep" pinctrl state 남영민
2017-09-21 16:14 ` Tomasz Figa
@ 2017-09-22 13:09 ` Linus Walleij
1 sibling, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2017-09-22 13:09 UTC (permalink / raw)
To: 남영민, Florian Fainelli, Charles Keepax,
Baolin Wang
Cc: Tomasz Figa, Krzysztof Kozlowski, Sylwester Nawrocki,
linux-arm-kernel@lists.infradead.org, linux-samsung-soc,
linux-gpio@vger.kernel.org
On Thu, Sep 21, 2017 at 2:32 PM, 남영민 <youngmin.nam@samsung.com> wrote:
I'm copying the whole patch so that everyone added to To: can see it.
> This patch adds pinctrl_for_sleep() in samsung_pinctrl_suspend()
> to support configuration of "sleep" pinctrl state.
>
> For example, we can configure "sleep" pinctrl state of "gpf1-6"
> as "INPUT, PULL DOWN" by configuring power down mode register.
>
> &pinctrl_5 {
> pinctrl-names = "default","sleep";
> pinctrl-0 = <&initial5>;
> pinctrl-1 = <&sleep5>;
>
> initial5: initial-state {
> samsung,pins = gpf1-6;
> samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> };
> sleep5: sleep-state {
> samsung,pins = gpf1-6;
> samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_DOWN>;
> };
> };
>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> ---
> drivers/pinctrl/samsung/pinctrl-samsung.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index e04f7fe0a65d..b4d12f8db475 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1099,6 +1099,11 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
> {
> struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
> int i;
> + int ret;
> +
> + ret = pinctrl_force_sleep(drvdata->pctl_dev);
> + if (ret)
> + dev_err(dev, "could not set sleep pinstate %d\n", ret);
>
> for (i = 0; i < drvdata->nr_banks; i++) {
> struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
> @@ -1187,6 +1192,11 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> if (drvdata->retention_ctrl && drvdata->retention_ctrl->disable)
> drvdata->retention_ctrl->disable(drvdata);
>
> + /* For changing state without writing register. */
> + if (!IS_ERR(drvdata->pctl_dev->p) &&
> + !IS_ERR(drvdata->pctl_dev->hog_default))
> + drvdata->pctl_dev->p->state = drvdata->pctl_dev->hog_default;
> +
> return 0;
> }
Oddly this business of forcing the hardware into different "sleep" states
is coming up from the left and right at the moment.
Please check a bit at:
commit 6606bc9dee63 ("pinctrl: Add sleep related state to indicate
sleep related configs")
and how the Spreadtrum driver handles this.
That is for the case where the hardware autonomously defines
a sleep state that need to be programmed from somewhere.
This seems to be what this hardware needs to use?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 4+ messages in thread