linux-kernel - Re: [PATCH] pwm: atmel-pwm: fix calculation of prescale value

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-next>] [day] [month] [year] [list]

Message-ID: <20140925065229.GD12423@ulmo>
Date:	Thu, 25 Sep 2014 08:52:30 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Nikolaus Voss <n.voss@...nmann-emt.de>
Cc:	Bo Shen <voice.shen@...el.com>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pwm: atmel-pwm: fix calculation of prescale value

On Tue, Sep 23, 2014 at 03:30:21PM +0200, Nikolaus Voss wrote:
> The prescale value used for calculating the period was incremented
> afterwards, thus the resulting prescale value is by one too high.
> This resulted in a pwm frequency only half as high as requested.
> 
> This patch moves the 64 bit division out of the prescale loop to
> correct the above issue and make the calculation more efficient.
> 
> Signed-off-by: Nikolaus Voss <n.voss@...nmann-emt.de>
> ---
>  drivers/pwm/pwm-atmel.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)

Hi Nikolaus,

Please Cc the linux-pwm@...r.kernel.org mailing list for PWM-related
patches in the future.

Also a couple more comments:

In the patch description: "pwm frequency" should be "PWM frequency".

> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 6e700a5..ff17b5d 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -102,7 +102,7 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    int duty_ns, int period_ns)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> -	unsigned long clk_rate, prd, dty;
> +	unsigned long prd, dty;
>  	unsigned long long div;
>  	unsigned int pres = 0;
>  	u32 val;
> @@ -113,20 +113,18 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return -EBUSY;
>  	}
>  
> -	clk_rate = clk_get_rate(atmel_pwm->clk);
> -	div = clk_rate;
> +	/* Calculate the period cycles and prescale value */
> +	div = (unsigned long long)clk_get_rate(atmel_pwm->clk) * period_ns;
> +	do_div(div, (int)1e9);

1e9 should be NSEC_PER_SEC.

> -	/* Calculate the period cycles */
>  	while (div > PWM_MAX_PRD) {
> -		div = clk_rate / (1 << pres);
> -		div = div * period_ns;
> -		/* 1/Hz = 100000000 ns */
> -		do_div(div, 1000000000);
> -
> -		if (pres++ > PRD_MAX_PRES) {
> -			dev_err(chip->dev, "pres exceeds the maximum value\n");
> -			return -EINVAL;
> -		}
> +		div >>= 1;
> +		++pres;

Unless you really need the prefix increment behaviour (you don't in this
case) I prefer using the postfix operator because it is slightly more
idiomatic.

No need for you to respin the patch, I've fixed up the above when
applying.

Thierry

Content of type "application/pgp-signature" skipped