[v2,2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()
diff mbox series

Message ID 1546918094-13960-3-git-send-email-yoshihiro.shimoda.uh@renesas.com
State Superseded
Headers show
Series
  • pwm: rcar: Add support "atomic" API and improve calculation
Related show

Commit Message

Yoshihiro Shimoda Jan. 8, 2019, 3:28 a.m. UTC
To remove legacy API related functions in the future, this patch
uses "atomic" related function instead. No change in behavior.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Uwe Kleine-König Jan. 8, 2019, 7:47 a.m. UTC | #1
On Tue, Jan 08, 2019 at 12:28:12PM +0900, Yoshihiro Shimoda wrote:
> To remove legacy API related functions in the future, this patch
> uses "atomic" related function instead. No change in behavior.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index ba70e83..4987c12 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -316,18 +316,16 @@ static int rcar_pwm_suspend(struct device *dev)
>  static int rcar_pwm_resume(struct device *dev)
>  {
>  	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
> +	struct pwm_state state;
>  
>  	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
>  		return 0;
>  
>  	pm_runtime_get_sync(dev);
>  
> -	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
> -			pwm->state.period);
> -	if (pwm_is_enabled(pwm))
> -		rcar_pwm_enable(pwm->chip, pwm);
> +	pwm_get_state(pwm, &state);
>  
> -	return 0;
> +	return rcar_pwm_apply(pwm->chip, pwm, &state);
>  }

Orthogonal to this patch I wonder what the intended behaviour for a pwm
is on suspend. Should it stop oscilating unconditionally? Or should it
only stop if the consumer stops it as part of its own suspend callback?

As the patch only reworks and improves the code without a change in
behaviour, it is fine for me.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe
Yoshihiro Shimoda Jan. 8, 2019, 8:46 a.m. UTC | #2
Hello Uwe,

> From: Uwe Kleine-Konig, Sent: Tuesday, January 8, 2019 4:48 PM
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index ba70e83..4987c12 100644
> > --- a/drivers/pwm/pwm-rcar.c
> > +++ b/drivers/pwm/pwm-rcar.c
> > @@ -316,18 +316,16 @@ static int rcar_pwm_suspend(struct device *dev)
> >  static int rcar_pwm_resume(struct device *dev)
> >  {
> >  	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
> > +	struct pwm_state state;
> >
> >  	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> >  		return 0;
> >
> >  	pm_runtime_get_sync(dev);
> >
> > -	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
> > -			pwm->state.period);
> > -	if (pwm_is_enabled(pwm))
> > -		rcar_pwm_enable(pwm->chip, pwm);
> > +	pwm_get_state(pwm, &state);
> >
> > -	return 0;
> > +	return rcar_pwm_apply(pwm->chip, pwm, &state);
> >  }
> 
> Orthogonal to this patch I wonder what the intended behaviour for a pwm
> is on suspend. Should it stop oscilating unconditionally? Or should it
> only stop if the consumer stops it as part of its own suspend callback?

I think the second one is better. I checked drivers/video/backlight/pwm_bl.c
and it is possible to call pwm_apply_state() by the driver after rcar_pwm_suspend()
was called. So, I'll fix this as other patch.

> As the patch only reworks and improves the code without a change in
> behaviour, it is fine for me.
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks!

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Geert Uytterhoeven Jan. 8, 2019, 8:56 a.m. UTC | #3
Hi Uwe,

On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Orthogonal to this patch I wonder what the intended behaviour for a pwm
> is on suspend. Should it stop oscilating unconditionally? Or should it
> only stop if the consumer stops it as part of its own suspend callback?

I guess you mean system suspend, not runtime suspend, as the device is
runtime-resumed when a PWM is requested?

Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
suspend will power down the SoC, so PWM output will stop for sure.

On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
code will turn of the PWM module's clock. Hence it will stop oscillating, unless
you take special countermeasures, like for modules that need to stay powered
for wake-up handling.

Gr{oetje,eeting}s,

                        Geert
Uwe Kleine-König Jan. 8, 2019, 9:12 a.m. UTC | #4
On Tue, Jan 08, 2019 at 08:46:30AM +0000, Yoshihiro Shimoda wrote:
> Hello Uwe,
> 
> > From: Uwe Kleine-Konig, Sent: Tuesday, January 8, 2019 4:48 PM
> > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > > index ba70e83..4987c12 100644
> > > --- a/drivers/pwm/pwm-rcar.c
> > > +++ b/drivers/pwm/pwm-rcar.c
> > > @@ -316,18 +316,16 @@ static int rcar_pwm_suspend(struct device *dev)
> > >  static int rcar_pwm_resume(struct device *dev)
> > >  {
> > >  	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
> > > +	struct pwm_state state;
> > >
> > >  	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> > >  		return 0;
> > >
> > >  	pm_runtime_get_sync(dev);
> > >
> > > -	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
> > > -			pwm->state.period);
> > > -	if (pwm_is_enabled(pwm))
> > > -		rcar_pwm_enable(pwm->chip, pwm);
> > > +	pwm_get_state(pwm, &state);
> > >
> > > -	return 0;
> > > +	return rcar_pwm_apply(pwm->chip, pwm, &state);
> > >  }
> > 
> > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > is on suspend. Should it stop oscilating unconditionally? Or should it
> > only stop if the consumer stops it as part of its own suspend callback?
> 
> I think the second one is better.

I agree. @Thierry: Do you agree, too? Then we should document that.

Best regards
Uwe
Uwe Kleine-König Jan. 8, 2019, 9:19 a.m. UTC | #5
Hello Geert,

On Tue, Jan 08, 2019 at 09:56:28AM +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > is on suspend. Should it stop oscilating unconditionally? Or should it
> > only stop if the consumer stops it as part of its own suspend callback?
> 
> I guess you mean system suspend, not runtime suspend, as the device is
> runtime-resumed when a PWM is requested?

I admit that suspend (both system and runtime) is a bit of a black box
for me. So please take that into account when judging my statements.
 
> Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
> suspend will power down the SoC, so PWM output will stop for sure.
> 
> On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
> code will turn of the PWM module's clock. Hence it will stop oscillating, unless
> you take special countermeasures, like for modules that need to stay powered
> for wake-up handling.

Whatever "suspend" here means, I want to prevent that a stopping pwm is
a surprise for the consumer. So I think suspend should be inhibited if
the consumer might expect the pwm to continue running but the pwm is
about to stop. So if the suspend affects the consumer, too, it's the
consumer that should be responsible to stop the pwm in a controlled
manner.

Best regards
Uwe
Geert Uytterhoeven Jan. 8, 2019, 9:35 a.m. UTC | #6
Hi Uwe,

On Tue, Jan 8, 2019 at 10:19 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Jan 08, 2019 at 09:56:28AM +0100, Geert Uytterhoeven wrote:
> > On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > > is on suspend. Should it stop oscilating unconditionally? Or should it
> > > only stop if the consumer stops it as part of its own suspend callback?
> >
> > I guess you mean system suspend, not runtime suspend, as the device is
> > runtime-resumed when a PWM is requested?
>
> I admit that suspend (both system and runtime) is a bit of a black box
> for me. So please take that into account when judging my statements.
>
> > Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
> > suspend will power down the SoC, so PWM output will stop for sure.
> >
> > On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
> > code will turn of the PWM module's clock. Hence it will stop oscillating, unless
> > you take special countermeasures, like for modules that need to stay powered
> > for wake-up handling.
>
> Whatever "suspend" here means, I want to prevent that a stopping pwm is
> a surprise for the consumer. So I think suspend should be inhibited if
> the consumer might expect the pwm to continue running but the pwm is
> about to stop. So if the suspend affects the consumer, too, it's the
> consumer that should be responsible to stop the pwm in a controlled
> manner.

I think you can inhibit suspend by letting the .suspend() callback return an
error. However, I think the PWM driver cannot make that decision on its own.
Shutting down the PWM for a status LED is harmless, and the status LED being
enabled should not prevent a system suspend.
Shutting down e.g. a motor may be different.

Gr{oetje,eeting}s,

                        Geert
Uwe Kleine-König Jan. 10, 2019, 9:51 a.m. UTC | #7
On Tue, Jan 08, 2019 at 10:35:29AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> On Tue, Jan 8, 2019 at 10:19 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Jan 08, 2019 at 09:56:28AM +0100, Geert Uytterhoeven wrote:
> > > On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > > > is on suspend. Should it stop oscilating unconditionally? Or should it
> > > > only stop if the consumer stops it as part of its own suspend callback?
> > >
> > > I guess you mean system suspend, not runtime suspend, as the device is
> > > runtime-resumed when a PWM is requested?
> >
> > I admit that suspend (both system and runtime) is a bit of a black box
> > for me. So please take that into account when judging my statements.
> >
> > > Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
> > > suspend will power down the SoC, so PWM output will stop for sure.
> > >
> > > On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
> > > code will turn of the PWM module's clock. Hence it will stop oscillating, unless
> > > you take special countermeasures, like for modules that need to stay powered
> > > for wake-up handling.
> >
> > Whatever "suspend" here means, I want to prevent that a stopping pwm is
> > a surprise for the consumer. So I think suspend should be inhibited if
> > the consumer might expect the pwm to continue running but the pwm is
> > about to stop. So if the suspend affects the consumer, too, it's the
> > consumer that should be responsible to stop the pwm in a controlled
> > manner.
> 
> I think you can inhibit suspend by letting the .suspend() callback return an
> error. However, I think the PWM driver cannot make that decision on its own.
> Shutting down the PWM for a status LED is harmless, and the status LED being
> enabled should not prevent a system suspend.
> Shutting down e.g. a motor may be different.

Also this is something that should be implemented in the core (or at
least consistently in all drivers). So I think it's not sensible to
continue discussion that in the context of this rcar change. I added it
to my list of stuff to discuss.

Best regards
Uwe

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index ba70e83..4987c12 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -316,18 +316,16 @@  static int rcar_pwm_suspend(struct device *dev)
 static int rcar_pwm_resume(struct device *dev)
 {
 	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
+	struct pwm_state state;
 
 	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
 		return 0;
 
 	pm_runtime_get_sync(dev);
 
-	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
-			pwm->state.period);
-	if (pwm_is_enabled(pwm))
-		rcar_pwm_enable(pwm->chip, pwm);
+	pwm_get_state(pwm, &state);
 
-	return 0;
+	return rcar_pwm_apply(pwm->chip, pwm, &state);
 }
 #endif /* CONFIG_PM_SLEEP */
 static SIMPLE_DEV_PM_OPS(rcar_pwm_pm_ops, rcar_pwm_suspend, rcar_pwm_resume);