| 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 | expand |
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
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/ |
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
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
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
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
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
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);
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(-)