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

From: Thierry Reding
Date: Fri Jun 21 2013 - 06:03:04 EST


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

Attachment: pgp00000.pgp
Description: PGP signature