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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131203094326.GB21178@ulmo.nvidia.com>
Date:	Tue, 3 Dec 2013 10:43:27 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Bo Shen <voice.shen@...el.com>
Cc:	linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
	nicolas.ferre@...el.com, linux-kernel@...r.kernel.org,
	alexandre.belloni@...e-electrons.com, galak@...eaurora.org,
	plagnioj@...osoft.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v8 1/2] PWM: atmel-pwm: add PWM controller driver

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

Content of type "application/pgp-signature" skipped