Re: [PATCH v8 1/2] PWM: atmel-pwm: add PWM controller driver

[Thierry Reding]

On Tue, Dec 03, 2013 at 11:09:12AM +0800, Bo Shen wrote:
> On 12/02/2013 06:59 PM, Thierry Reding wrote:
> >On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote:
[...]
> >>diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
[...]
> >>+	/* Calculate the period cycles */
> >>+	while (div > PWM_MAX_PRD) {
> >>+		div = clk_rate / (1 << pres);
> >>+		div = div * period_ns;
> >>+		/* 1/Hz = 100000000 ns */
> >
> >I don't think that comment is needed.
> 
> This is asked to be added.
> And, I think keep it and it won't hurt, what do you think?

I think it's obvious that you're converting from nanoseconds because of
the _ns prefix in period_ns. But if somebody requested this and everyone
else thinks it's useful, I'm okay with keeping it.

> >>+	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> >>+		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty);
> >>+	} else {
> >>+		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty);
> >>+		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd);
> >>+	}
> >>+}
> >
> >Neither version 1 nor version 2 seem to be able to change the period
> >while the channel is enabled. Perhaps that should be checked for in
> >atmel_pwm_config() and an error (-EBUSY) returned?
> 
> The period is configured in dt in device tree, or platform data in non
> device tree. Nowhere will update period. So, not code to update period.
> Am I right? If not, please figure out.

The .config() operation allows the period to be specified. Just because
nobody currently changes it at runtime doesn't mean it can't be done.

It is also possible that whoever wrote the device tree or platform data
didn't know that the period must be the same for all PWM channels and
therefore might use different values. If you check for this, at least
they'll notice. If you don't check for it, then they may end up having
the period reconfigured behind their backs, which may cause parts of
their setup to behave unexpectedly.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature