Re: [PATCH v3] pwm: renesas-tpu: Add suspend/resume function

[Cao Van Dong <cv-dong@xxxxxxxxxxx>]

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 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.

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