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

From: Robin van der Gracht
Date: Wed Jun 26 2013 - 02:53:05 EST


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/