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

[Robin van der Gracht]

Hello Thierry,


On 06/21/2013 12:02 PM, Thierry Reding wrote:
> 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?
Agreed
> 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.
Ok
> > 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.
Yes that is correct.
> > 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.
I'll update that.
> >   
> >   #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);
Thanks for the input, I agree on your comment. I'll resubmit the patch.
> ?
>
> Thierry


--
Robin van der Gracht
Protonic Holland.
tel.: +31 (0) 229 212928
fax.: +31 (0) 229 210930
Factorij 36 / 1689 AL Zwaag

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/