linux-kernel - Re: [PATCH] pwm: pwm-mxs: Apply configuration before disabling PWM.

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]

Date:	Fri, 21 Jun 2013 12:02:54 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Robin van der Gracht <robin@...tonic.nl>
Cc:	linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
	shawn.guo@...aro.org
Subject: Re: [PATCH] pwm: pwm-mxs: Apply configuration before disabling PWM.

Please use my new email address and Cc the linux-pwm mailing list.

The subject implies some active procedure is used to make sure the
configuration is applied, but you really only wait for some amount of
time. Perhaps something like:

	pwm: pwm-mxs: Wait for configuration to apply before disabling PWM

is more accurate?

On Wed, Jun 19, 2013 at 01:51:26PM +0200, Robin van der Gracht wrote:
> When disabling the pwm, the output state locks at its current state.

Please use the proper spelling "PWM" in prose.

> We have to be sure the last configuration applied. Which in most
> cases sets duty cycle to 0%. To prevent the pwm from taking on
> 100% duty cycle when disabled during a high state.
> 
> Configuration applies at the beginning of a new output period.

I have some trouble understanding this, but I think you mean:

We have to be sure that the last configuration has been applied. In most
cases drivers will have set the duty-cycle to 0%. To prevent the PWM
from locking at a 100% duty-cycle for example, we delay disabling the
PWM for a whole period to make sure any new configuration has been
latched.

> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 3febddd..4ddc063 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -21,6 +21,7 @@
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
>  #include <linux/stmp_device.h>
> +#include <linux/delay.h>

Please keep the includes sorted alphabetically.

>  
>  #define SET	0x4
>  #define CLR	0x8
> @@ -40,6 +41,7 @@ struct mxs_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
>  	void __iomem *base;
> +	unsigned long period_ns;

This is not the proper place to put it. The period can be different for
each PWM channel. But you also don't need to store this separately as in
latest linux-next this is already done by the core. You can use the
pwm_get_period() function to obtain the current period from a PWM
device.

> @@ -113,6 +116,11 @@ static void mxs_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct mxs_pwm_chip *mxs = to_mxs_pwm_chip(chip);
>  
> +	/*
> +	 * Ensure latest configuration applied.
> +	 */

This comment can go on a single line.

> +	ndelay(mxs->period_ns);

This introduces a potentially long delay. How about changing this to
something like:

	period = pwm_get_period(pwm);
	period = DIV_ROUND_UP(period, 1000);
	usleep_range(period, period + 1000);

?

Thierry

Content of type "application/pgp-signature" skipped