Dear Morimoto-san, Thank for your feedback!
On 2019/05/24 10:21, Kuninori Morimoto wrote:
Hi Cao If you post v3 patches, you should post whole seriesThis 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 <[email protected]> Tested-by: Cao Van Dong <[email protected]> Acked-by: Kuninori Morimoto <[email protected]>Drop my name from here, I'm not Acked yet.
I'm so sorry !!
+#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 < TPU_CHANNEL_MAX; i++) { + if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) { + pwm = &chip->pwms[i]; + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) + return 0; + } + } + + 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 < TPU_CHANNEL_MAX; 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 */again pwm = &chip->pwms[i]; if (pwm_get_chip_data(pwms)) { Why you "restart" at resume ? I think "disable" at suspend, and "enable" at resume is generic ?
This is a good idea.
You call pm_runtime_get_sync() at resume, but removed pm_runtime_put() suspend ??
Very sorry for the inconvenience. I'm mistake. I will remove pm_runtime_get_sync() at resume.
Thank you, Dong
Thank you for your help !! Best regards --- Kuninori Morimoto
