Hi Cao
If you post v3 patches, you should post whole series
> 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@xxxxxxxxxxx>
> Tested-by: Cao Van Dong <cv-dong@xxxxxxxxxxx>
> Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
Drop my name from here, I'm not Acked yet.
> +#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 ?
You call pm_runtime_get_sync() at resume,
but removed pm_runtime_put() suspend ??
Thank you for your help !!
Best regards
---
Kuninori Morimoto
