[PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state

[PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state

[Shenwei Wang]

Apply the pinctrl setting of sleep state when system enters
suspend state.
Restore to the default pinctrl setting when system resumes.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/pwm/pwm-imx-tpm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index c50ddbac43c8..19245790c67c 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -393,7 +393,7 @@ static int pwm_imx_tpm_suspend(struct device *dev)
 
 	clk_disable_unprepare(tpm->clk);
 
-	return 0;
+	return pinctrl_pm_select_sleep_state(dev);
 }
 
 static int pwm_imx_tpm_resume(struct device *dev)
@@ -401,6 +401,10 @@ static int pwm_imx_tpm_resume(struct device *dev)
 	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
 	int ret = 0;
 
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		return ret;
+
 	ret = clk_prepare_enable(tpm->clk);
 	if (ret)
 		dev_err(dev, "failed to prepare or enable clock: %d\n", ret);
-- 
2.34.1

RE: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state

[Shenwei Wang]

> -----Original Message-----
> From: Shenwei Wang <shenwei.wang@nxp.com>
> Sent: Tuesday, May 14, 2024 3:06 PM
> To: Uwe Kleine-König <ukleinek@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; linux-pwm@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> Shenwei Wang <shenwei.wang@nxp.com>
> Subject: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state
> 
> Apply the pinctrl setting of sleep state when system enters suspend state.
> Restore to the default pinctrl setting when system resumes.
> 

Ping @Uwe

Thanks,
Shenwei

> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  drivers/pwm/pwm-imx-tpm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c index
> c50ddbac43c8..19245790c67c 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -393,7 +393,7 @@ static int pwm_imx_tpm_suspend(struct device *dev)
> 
>  	clk_disable_unprepare(tpm->clk);
> 
> -	return 0;
> +	return pinctrl_pm_select_sleep_state(dev);
>  }
> 
>  static int pwm_imx_tpm_resume(struct device *dev) @@ -401,6 +401,10 @@
> static int pwm_imx_tpm_resume(struct device *dev)
>  	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
>  	int ret = 0;
> 
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = clk_prepare_enable(tpm->clk);
>  	if (ret)
>  		dev_err(dev, "failed to prepare or enable clock: %d\n", ret);
> --
> 2.34.1

RE: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state

[Shenwei Wang]

> -----Original Message-----
> From: Shenwei Wang
> Sent: Friday, June 14, 2024 1:43 PM
> To: Uwe Kleine-König <ukleinek@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>
> > -----Original Message-----
> > From: Shenwei Wang <shenwei.wang@nxp.com>
> > Sent: Tuesday, May 14, 2024 3:06 PM
> > To: Uwe Kleine-König <ukleinek@kernel.org>; Shawn Guo
> > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> > <festevam@gmail.com>; linux-pwm@vger.kernel.org; imx@lists.linux.dev;
> > linux-arm-kernel@lists.infradead.org; dl-linux-imx
> > <linux-imx@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>
> > Subject: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state
> >
> > Apply the pinctrl setting of sleep state when system enters suspend state.
> > Restore to the default pinctrl setting when system resumes.
> >
> 
> Ping @Uwe

2nd Ping. 

Hi Uwe,
Can you please provide your feedback on this patch?

Thanks,
Shenwei
> 
> Thanks,
> Shenwei
> 
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > ---
> >  drivers/pwm/pwm-imx-tpm.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> > index c50ddbac43c8..19245790c67c 100644
> > --- a/drivers/pwm/pwm-imx-tpm.c
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -393,7 +393,7 @@ static int pwm_imx_tpm_suspend(struct device
> *dev)
> >
> >  	clk_disable_unprepare(tpm->clk);
> >
> > -	return 0;
> > +	return pinctrl_pm_select_sleep_state(dev);
> >  }
> >
> >  static int pwm_imx_tpm_resume(struct device *dev) @@ -401,6 +401,10
> > @@ static int pwm_imx_tpm_resume(struct device *dev)
> >  	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> >  	int ret = 0;
> >
> > +	ret = pinctrl_pm_select_default_state(dev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = clk_prepare_enable(tpm->clk);
> >  	if (ret)
> >  		dev_err(dev, "failed to prepare or enable clock: %d\n", ret);
> > --
> > 2.34.1

Re: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

Hello,

On Tue, May 14, 2024 at 03:05:34PM -0500, Shenwei Wang wrote:
> Apply the pinctrl setting of sleep state when system enters
> suspend state.
> Restore to the default pinctrl setting when system resumes.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  drivers/pwm/pwm-imx-tpm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> index c50ddbac43c8..19245790c67c 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -393,7 +393,7 @@ static int pwm_imx_tpm_suspend(struct device *dev)
>  
>  	clk_disable_unprepare(tpm->clk);
>  
> -	return 0;
> +	return pinctrl_pm_select_sleep_state(dev);
>  }
>  
>  static int pwm_imx_tpm_resume(struct device *dev)
> @@ -401,6 +401,10 @@ static int pwm_imx_tpm_resume(struct device *dev)
>  	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
>  	int ret = 0;
>  
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = clk_prepare_enable(tpm->clk);
>  	if (ret)
>  		dev_err(dev, "failed to prepare or enable clock: %d\n", ret);

If .resume() failed (consider clk_prepare_enable() failing), and
the resume is retried later: Is there an unexpected (though maybe
harmless) imbalance because pinctrl_pm_select_default_state() isn't
undone in .resume()'s error path?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state

[Shenwei Wang]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Sent: Monday, July 1, 2024 3:46 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; linux-
> pwm@vger.kernel.org; imx@lists.linux.dev; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH] pwm: imx-tpm: Enable pinctrl setting for sleep state
> 
> Hello,
> 
> On Tue, May 14, 2024 at 03:05:34PM -0500, Shenwei Wang wrote:
> > Apply the pinctrl setting of sleep state when system enters suspend
> > state.
> > Restore to the default pinctrl setting when system resumes.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > ---
> >  drivers/pwm/pwm-imx-tpm.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> > index c50ddbac43c8..19245790c67c 100644
> > --- a/drivers/pwm/pwm-imx-tpm.c
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -393,7 +393,7 @@ static int pwm_imx_tpm_suspend(struct device
> *dev)
> >
> >  	clk_disable_unprepare(tpm->clk);
> >
> > -	return 0;
> > +	return pinctrl_pm_select_sleep_state(dev);
> >  }
> >
> >  static int pwm_imx_tpm_resume(struct device *dev) @@ -401,6 +401,10
> > @@ static int pwm_imx_tpm_resume(struct device *dev)
> >  	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> >  	int ret = 0;
> >
> > +	ret = pinctrl_pm_select_default_state(dev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = clk_prepare_enable(tpm->clk);
> >  	if (ret)
> >  		dev_err(dev, "failed to prepare or enable clock: %d\n", ret);
> 
> If .resume() failed (consider clk_prepare_enable() failing), and the resume is
> retried later: Is there an unexpected (though maybe
> harmless) imbalance because pinctrl_pm_select_default_state() isn't undone
> in .resume()'s error path?
> 

If we need to take care the imbalance of pinctrl states in the error path in .resume
function, should I also take care of the imbalance of clk states in the error path in .suspend function?

Thank you for the review!
Shenwei

> Best regards
> Uwe