On Mon, May 27, 2019 at 11:22:37AM +0900, Cao Van Dong wrote:
> 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>
> ---
> Changes v4 -> v5:
> - Remove test_bit(PWMF_REQUESTED, &pwm->flags) check.
> ---
> Changes v3 -> v4:
> - Use pwm_is_enabled(pwm) to check channel instead of pwm_get_chip_data(&chip->pwms[i]).
> - Move tpu_pwm_disable() to tpu_pwm_suspend(), tpu_pwm_enable() to tpu_pwm_resume().
> - Remove tpu_pwm_restart_timer() function and remove pm_runtime_get_sync() in tpu_pwm_resume().
> ---
> Changes v2 -> v3:
> - Changes '3' -> TPU_CHANNEL_MAX in loop.
> - Remove pm_runtime_put() function in tpu_pwm_suspend() function.
> ---
> Changes v1 -> v2:
> - Repair the handling code to cover case of using multiple timers.
> ---
> drivers/pwm/pwm-renesas-tpu.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
This has been discussed before, but this really shouldn't be done in the
PWM driver. Consumers should really be reconfiguring the PWM upon resume
as appropriate. This is the only way to ensure that everything is
resumed in the proper order.
Most, if not all, consumers already implement suspend/resume that way.
sysfs is the only one that I'm aware of that doesn't.
Since you've been using sysfs to test this, things are slightly more
complicated (i.e. we don't have a consumer driver in the conventional
way). However, you should be able to solve this by implementing
dev_pm_ops for the pwm_class.
Do you think you could give that a try?
Thierry
>
> diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
> index 4a855a2..86b7da4 100644
> --- a/drivers/pwm/pwm-renesas-tpu.c
> +++ b/drivers/pwm/pwm-renesas-tpu.c
> @@ -366,6 +366,41 @@ 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_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++) {
> + pwm = &chip->pwms[i];
> + if (pwm_is_enabled(pwm))
> + tpu_pwm_disable(pwm->chip, pwm);
> + }
> +
> + 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;
> +
> + for (i = 0; i < TPU_CHANNEL_MAX; i++) {
> + pwm = &chip->pwms[i];
> + if (pwm_is_enabled(pwm))
> + tpu_pwm_enable(pwm->chip, 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 +494,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),
> }
> };
> --
> 2.7.4
>
Attachment:
signature.asc
Description: PGP signature
