| Message ID | 1558575210-28112-1-git-send-email-cv-dong@jinso.co.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] pwm: renesas-tpu: Add suspend/resume function | expand |
Hi > +static int tpu_pwm_suspend(struct device *dev) > +{ > + struct tpu_device *tpu = dev_get_drvdata(dev); > + struct pwm_chip *chip = &tpu->chip; > + struct pwm_device *pwm; > + int i; > + > + for (i = 0; i <= 3; i++) { > + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { > + pwm = &chip->pwms[i]; > + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > + return 0; > + } > + } why 3 ? About code, how about this ? pwm = &chip->pwms[i]; if (pwm_get_chip_data(pwm)) { ... Can we use chip->pwms at driver ? I'm not sure but pwm.h say struct pwm_chip { ... => /* only used internally by the PWM framework */ struct list_head list; struct pwm_device *pwms; }; > + > + pm_runtime_put(dev); > + > + return 0; > +} Do we need to call pm_runtime_xxx here ? > +static int tpu_pwm_resume(struct device *dev) > +{ > + struct tpu_device *tpu = dev_get_drvdata(dev); > + struct pwm_chip *chip = &tpu->chip; > + struct pwm_device *pwm; > + int i; > + > + pm_runtime_get_sync(dev); > + > + for (i = 0; i <= 3; i++) { > + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { > + pwm = &chip->pwms[i]; > + tpu_pwm_restart_timer(pwm); > + } > + } ditto
Dear Morimoto-san, Thank for your feedback! On 2019/05/23 11:02, Kuninori Morimoto wrote: > Hi > >> +static int tpu_pwm_suspend(struct device *dev) >> +{ >> + struct tpu_device *tpu = dev_get_drvdata(dev); >> + struct pwm_chip *chip = &tpu->chip; >> + struct pwm_device *pwm; >> + int i; >> + >> + for (i = 0; i <= 3; i++) { >> + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { >> + pwm = &chip->pwms[i]; >> + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) >> + return 0; >> + } >> + } > why 3 ? According to Hardware manual, 16-Bit Timer Pulse Unit (TPU) supports four 16-bit timers for both R-car GEN2 and GEN3. > About code, how about this ? > > pwm = &chip->pwms[i]; > if (pwm_get_chip_data(pwm)) { > ... > > Can we use chip->pwms at driver ? I'm not sure but pwm.h say > > struct pwm_chip { > ... > => /* only used internally by the PWM framework */ > struct list_head list; > struct pwm_device *pwms; > }; As described in the header: "@pwms: array of PWM devices allocated by the framework" Therefore, we can not use chip->pwms and must specify the device that will be used. Otherwise the compilation will fail. >> + >> + pm_runtime_put(dev); >> + >> + return 0; >> +} > Do we need to call pm_runtime_xxx here ? "pm_runtime_put(dev);" function is called for runtime idle operations. >> +static int tpu_pwm_resume(struct device *dev) >> +{ >> + struct tpu_device *tpu = dev_get_drvdata(dev); >> + struct pwm_chip *chip = &tpu->chip; >> + struct pwm_device *pwm; >> + int i; >> + >> + pm_runtime_get_sync(dev); >> + >> + for (i = 0; i <= 3; i++) { >> + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { >> + pwm = &chip->pwms[i]; >> + tpu_pwm_restart_timer(pwm); >> + } >> + } > ditto Thank you, Dong
Hi > >> +static int tpu_pwm_suspend(struct device *dev) > >> +{ > >> + struct tpu_device *tpu = dev_get_drvdata(dev); > >> + struct pwm_chip *chip = &tpu->chip; > >> + struct pwm_device *pwm; > >> + int i; > >> + > >> + for (i = 0; i <= 3; i++) { > >> + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { > >> + pwm = &chip->pwms[i]; > >> + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > >> + return 0; > >> + } > >> + } > > why 3 ? > According to Hardware manual, 16-Bit Timer Pulse Unit (TPU) > supports four 16-bit timers for both R-car GEN2 and GEN3. Hmm... You need to use chip->npwm or TPU_CHANNEL_MAX then ? > >> + pm_runtime_put(dev); > >> + > >> + return 0; > >> +} > > Do we need to call pm_runtime_xxx here ? > > "pm_runtime_put(dev);" function is called for runtime idle operations. I know. I'm asking do we need to call it here ? Thank you for your help !! Best regards --- Kuninori Morimoto
Dear Morimoto-san, Thank for your feedback! On 2019/05/23 13:07, Kuninori Morimoto wrote: > Hi > >>>> +static int tpu_pwm_suspend(struct device *dev) >>>> +{ >>>> + struct tpu_device *tpu = dev_get_drvdata(dev); >>>> + struct pwm_chip *chip = &tpu->chip; >>>> + struct pwm_device *pwm; >>>> + int i; >>>> + >>>> + for (i = 0; i <= 3; i++) { >>>> + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { >>>> + pwm = &chip->pwms[i]; >>>> + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) >>>> + return 0; >>>> + } >>>> + } >>> why 3 ? >> According to Hardware manual, 16-Bit Timer Pulse Unit (TPU) >> supports four 16-bit timers for both R-car GEN2 and GEN3. > Hmm... > You need to use chip->npwm or TPU_CHANNEL_MAX then ? Thank for your opinion! I will resubmit v3 to change 3 to TPU_CHANNEL_MAX. >>>> + pm_runtime_put(dev); >>>> + >>>> + return 0; >>>> +} >>> Do we need to call pm_runtime_xxx here ? >> "pm_runtime_put(dev);" function is called for runtime idle operations. > I know. > I'm asking do we need to call it here ? I think we should have it here better. Thank you, Dong > Thank you for your help !! > Best regards > --- > Kuninori Morimoto
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 4a855a2..a6660cc 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -366,6 +366,60 @@ static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *_pwm) tpu_pwm_timer_stop(pwm); } +#ifdef CONFIG_PM_SLEEP +static int tpu_pwm_restart_timer(struct pwm_device *pwm) +{ + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) + return 0; + + /* Restart timer */ + tpu_pwm_disable(pwm->chip,pwm); + tpu_pwm_enable(pwm->chip,pwm); + + return 0; +} + +static int tpu_pwm_suspend(struct device *dev) +{ + struct tpu_device *tpu = dev_get_drvdata(dev); + struct pwm_chip *chip = &tpu->chip; + struct pwm_device *pwm; + int i; + + for (i = 0; i <= 3; i++) { + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { + pwm = &chip->pwms[i]; + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) + return 0; + } + } + + pm_runtime_put(dev); + + return 0; +} + +static int tpu_pwm_resume(struct device *dev) +{ + struct tpu_device *tpu = dev_get_drvdata(dev); + struct pwm_chip *chip = &tpu->chip; + struct pwm_device *pwm; + int i; + + pm_runtime_get_sync(dev); + + for (i = 0; i <= 3; i++) { + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { + pwm = &chip->pwms[i]; + tpu_pwm_restart_timer(pwm); + } + } + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ +static SIMPLE_DEV_PM_OPS(tpu_pwm_pm_ops, tpu_pwm_suspend, tpu_pwm_resume); + static const struct pwm_ops tpu_pwm_ops = { .request = tpu_pwm_request, .free = tpu_pwm_free, @@ -459,6 +513,7 @@ static struct platform_driver tpu_driver = { .remove = tpu_remove, .driver = { .name = "renesas-tpu-pwm", + .pm = &tpu_pwm_pm_ops, .of_match_table = of_match_ptr(tpu_of_table), } };
This patch adds suspend/resume function support for Renesas the 16-Bit Timer Pulse Unit (TPU) driver. This has been tested on the Salvator-XS board with R-Car M3-N and H3 at renesas-drivers-2019-05-21-v5.2-rc1 tag. I expect this to work on other SoCs. Test procedure: - Enable TPU and pin control in DTS. - Make sure switches { SW29-[1-2] are switched off or SW31-[1-4] are switched off(only for Salvator-xs) }. - Exercise userspace PWM control for pwm[2,3] of /sys/class/pwm/pwmchip1/ . - Inspect PWM signals on the input side of { CN29-[58,60] or SW31-[1,2] (only for Salvator-xs) } before and after suspend/resume using an oscilloscope. Signed-off-by: Cao Van Dong <cv-dong@jinso.co.jp> Tested-by: Cao Van Dong <cv-dong@jinso.co.jp> --- Changes v1 -> v2: - Repair the handling code to cover case of using multiple timers. --- drivers/pwm/pwm-renesas-tpu.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)