[BUG] leds-pwm: First setting of brightness does nothing

[BUG] leds-pwm: First setting of brightness does nothing

[Benoît Thébaudeau]

Hi all,

I have found a bug using leds-pwm with the i.MX PWM driver. The first time a
non-0 brightness is set through sysfs, this has no effect. It works fine from
the 2nd time.

The cause of this bug is that leds-pwm.c:led_pwm_set() calls pwm_config() before
pwm_enable(), while in the i.MX PWM driver pwm_enable() enables the PWM IP
clock, so that a call to pwm_config() has no effect before the first call to
pwm_enable().

Several other PWM drivers in drivers/pwm/ work in the same way. pwm-beeper.c and
pwm_bl.c also call pwm_config() before pwm_enable(). The PWM documentation does
not say if the wrong doers are these PWM users or the i.MX-like PWM drivers.

So what should be fixed? Fixing these PWM users would be quite easy since it's a
simple swap of the pwm_config() and pwm_enable() calls. This should not produce
any glitch since the duty cycle is always set to 0 by these PWM users before
calling pwm_disable().

Best regards,
Benoît

[PATCH] pwm: Call pwm_enable() before pwm_config()

[Benoît Thébaudeau]

Some PWM drivers enable the clock of the PWM peripheral in pwm_enable(). Hence,
for these drivers, a call to pwm_config() does not have any effect before
pwm_enable() has been called.

This patch fixes the PWM users to make sure that they call pwm_enable() before
pwm_config().

This fixes the first setting of brightness through sysfs that had no effect with
leds-pwm and the i.MX PWM driver.

Cc: Thierry Reding <thierry.reding@avionic-design.de>
Cc: <linux-kernel@vger.kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: <linux-arm-kernel@lists.infradead.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: <linux-input@vger.kernel.org>
Cc: Bryan Wu <bryan.wu@canonical.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: <linux-leds@vger.kernel.org>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: <linux-fbdev@vger.kernel.org>
Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../drivers/input/misc/pwm-beeper.c                |    6 +++---
 .../drivers/leds/leds-pwm.c                        |    2 +-
 .../drivers/video/backlight/pwm_bl.c               |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git linux-next-c94456b.orig/drivers/input/misc/pwm-beeper.c linux-next-c94456b/drivers/input/misc/pwm-beeper.c
index fc84c8a..97d322b 100644
--- linux-next-c94456b.orig/drivers/input/misc/pwm-beeper.c
+++ linux-next-c94456b/drivers/input/misc/pwm-beeper.c
@@ -53,10 +53,10 @@ static int pwm_beeper_event(struct input_dev *input,
 		pwm_disable(beeper->pwm);
 	} else {
 		period = HZ_TO_NANOSECONDS(value);
-		ret = pwm_config(beeper->pwm, period / 2, period);
+		ret = pwm_enable(beeper->pwm);
 		if (ret)
 			return ret;
-		ret = pwm_enable(beeper->pwm);
+		ret = pwm_config(beeper->pwm, period / 2, period);
 		if (ret)
 			return ret;
 		beeper->period = period;
@@ -156,8 +156,8 @@ static int pwm_beeper_resume(struct device *dev)
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
 	if (beeper->period) {
-		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
 		pwm_enable(beeper->pwm);
+		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
 	}
 
 	return 0;
diff --git linux-next-c94456b.orig/drivers/leds/leds-pwm.c linux-next-c94456b/drivers/leds/leds-pwm.c
index f2e44c7..c2e0c22 100644
--- linux-next-c94456b.orig/drivers/leds/leds-pwm.c
+++ linux-next-c94456b/drivers/leds/leds-pwm.c
@@ -42,8 +42,8 @@ static void led_pwm_set(struct led_classdev *led_cdev,
 		pwm_config(led_dat->pwm, 0, period);
 		pwm_disable(led_dat->pwm);
 	} else {
-		pwm_config(led_dat->pwm, brightness * period / max, period);
 		pwm_enable(led_dat->pwm);
+		pwm_config(led_dat->pwm, brightness * period / max, period);
 	}
 }
 
diff --git linux-next-c94456b.orig/drivers/video/backlight/pwm_bl.c linux-next-c94456b/drivers/video/backlight/pwm_bl.c
index 995f016..a4bb95c 100644
--- linux-next-c94456b.orig/drivers/video/backlight/pwm_bl.c
+++ linux-next-c94456b/drivers/video/backlight/pwm_bl.c
@@ -65,8 +65,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 
 		duty_cycle = pb->lth_brightness +
 		     (duty_cycle * (pb->period - pb->lth_brightness) / max);
-		pwm_config(pb->pwm, duty_cycle, pb->period);
 		pwm_enable(pb->pwm);
+		pwm_config(pb->pwm, duty_cycle, pb->period);
 	}
 
 	if (pb->notify_after)

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Lars-Peter Clausen]

On 08/23/2012 04:19 PM, Benoît Thébaudeau wrote:
> Some PWM drivers enable the clock of the PWM peripheral in pwm_enable(). Hence,
> for these drivers, a call to pwm_config() does not have any effect before
> pwm_enable() has been called.
> 
> This patch fixes the PWM users to make sure that they call pwm_enable() before
> pwm_config().
> 
> This fixes the first setting of brightness through sysfs that had no effect with
> leds-pwm and the i.MX PWM driver.

But isn't this a bug in the PWM peripheral driver? With this change the PWM
will start with the old settings first. While this is not so much of a problem
for a backlight (although it might cause a short flickering) it might cause
problems for other applications, like using the PWM pin as a timing generator.
In my opinion it's better to fix the PWM peripheral drivers which have this
problem instead of trying to work around it in every user of the PWM API.

- Lars

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Benoît Thébaudeau]

On Thursday, August 23, 2012 5:43:32 PM, Lars-Peter Clausen wrote:
> On 08/23/2012 04:19 PM, Benoît Thébaudeau wrote:
> > Some PWM drivers enable the clock of the PWM peripheral in
> > pwm_enable(). Hence,
> > for these drivers, a call to pwm_config() does not have any effect
> > before
> > pwm_enable() has been called.
> > 
> > This patch fixes the PWM users to make sure that they call
> > pwm_enable() before
> > pwm_config().
> > 
> > This fixes the first setting of brightness through sysfs that had
> > no effect with
> > leds-pwm and the i.MX PWM driver.
> 
> But isn't this a bug in the PWM peripheral driver? With this change
> the PWM
> will start with the old settings first. While this is not so much of
> a problem
> for a backlight (although it might cause a short flickering) it might
> cause
> problems for other applications, like using the PWM pin as a timing
> generator.
> In my opinion it's better to fix the PWM peripheral drivers which
> have this
> problem instead of trying to work around it in every user of the PWM
> API.

I don't know. See my detailed description of this issue here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-August/115667.html

Where the bug is depends on the detailed definition of the PWM API, which I
don't find documented anywhere.

If pwm_enable() means "start PWM timer with the configured settings", then the
bug is in the drivers. If it means "enable the PWM peripheral so that we can
work with it", then the bug is in the PWM users.

I don't really have time to work on this, so I suggested this patch as a simple
solution. Otherwise, it means reworking several PWM drivers for different
hardware that is not available to everyone for testing.

If we decide to only change the i.MX PWM driver, the fix would be:
pwm_config()
{
        save passed config in private data;
        if (pwm enabled)
                apply passed config;
}

pwm_enable()
{
        if (!(pwm enabled)) {
                enable pwm ip clk;
                apply config from private data;
        }
}

If we fix only this driver, we must not forget that the same issue probably
exists with several other PWM drivers.

As I said in my bug description, the PWM users set the duty cycle to 0 before
calling pwm_disable(), so fixing the PWM users should not be an issue, even in
the timing generator use case you're talking about.

I don't have a strong opinion about what should be fixed here.

Best regards,
Benoît

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Lars-Peter Clausen]

On 08/23/2012 06:57 PM, Benoît Thébaudeau wrote:
> On Thursday, August 23, 2012 5:43:32 PM, Lars-Peter Clausen wrote:
>> On 08/23/2012 04:19 PM, Benoît Thébaudeau wrote:
>>> Some PWM drivers enable the clock of the PWM peripheral in
>>> pwm_enable(). Hence,
>>> for these drivers, a call to pwm_config() does not have any effect
>>> before
>>> pwm_enable() has been called.
>>>
>>> This patch fixes the PWM users to make sure that they call
>>> pwm_enable() before
>>> pwm_config().
>>>
>>> This fixes the first setting of brightness through sysfs that had
>>> no effect with
>>> leds-pwm and the i.MX PWM driver.
>>
>> But isn't this a bug in the PWM peripheral driver? With this change
>> the PWM
>> will start with the old settings first. While this is not so much of
>> a problem
>> for a backlight (although it might cause a short flickering) it might
>> cause
>> problems for other applications, like using the PWM pin as a timing
>> generator.
>> In my opinion it's better to fix the PWM peripheral drivers which
>> have this
>> problem instead of trying to work around it in every user of the PWM
>> API.
> 
> I don't know. See my detailed description of this issue here:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-August/115667.html
> 
> Where the bug is depends on the detailed definition of the PWM API, which I
> don't find documented anywhere.
> 
> If pwm_enable() means "start PWM timer with the configured settings", then the
> bug is in the drivers. If it means "enable the PWM peripheral so that we can
> work with it", then the bug is in the PWM users.

It really is the former. See the description of pwm_enable() in drivers/pwm/core.c

> 
> I don't really have time to work on this, so I suggested this patch as a simple
> solution. Otherwise, it means reworking several PWM drivers for different
> hardware that is not available to everyone for testing.
> 
> If we decide to only change the i.MX PWM driver, the fix would be:
> pwm_config()
> {
>         save passed config in private data;
>         if (pwm enabled)
>                 apply passed config;
> }
> 
> pwm_enable()
> {
>         if (!(pwm enabled)) {
>                 enable pwm ip clk;
>                 apply config from private data;
>         }
> }

Another option is to enable the clock if it is disabled when the device is
configured. E.g. that's what tegra does.

> 
> If we fix only this driver, we must not forget that the same issue probably
> exists with several other PWM drivers.
> 

Since this seems to be a common pattern in a number of PWM drivers it might
make sense to simply add support for enabling/disabling a clk to the pwm core.
Or maybe just use the runtime pm API for this. This probably makes even more
sense and grab a reference to the pm context when the enable() is called,
release it when disable() is called and also grab it before calling the
device's config callback and release it afterward.

- Lars

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Lars-Peter Clausen]

On 08/23/2012 07:12 PM, Lars-Peter Clausen wrote:
>[...]
> Or maybe just use the runtime pm API for this. This probably makes even more
> sense and grab a reference to the pm context when the enable() is called,
> release it when disable() is called and also grab it before calling the
> device's config callback and release it afterward.

Btw. this seems to be exactly what the pwm-tiecap and pwm-tiehrpwm drivers
already do today. I'd just make that a core PWM framework feature.

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Thu, Aug 23, 2012 at 07:19:36PM +0200, Lars-Peter Clausen wrote:
> On 08/23/2012 07:12 PM, Lars-Peter Clausen wrote:
> >[...]
> > Or maybe just use the runtime pm API for this. This probably makes even more
> > sense and grab a reference to the pm context when the enable() is called,
> > release it when disable() is called and also grab it before calling the
> > device's config callback and release it afterward.
> 
> Btw. this seems to be exactly what the pwm-tiecap and pwm-tiehrpwm drivers
> already do today. I'd just make that a core PWM framework feature.

Using runtime PM for this sounds indeed like the most generic approach.
I'm not very familiar with the API, but I thought it required explicit
architecture or bus support (or the driver itself can provide hooks via
pm_ops), so it might be difficult to implement for platforms that don't
have working runtime PM support. I'll have to look into this some more.

Anyway, if we add this kind of support to the PWM core we should take it
through next first. For drivers that are currently broken, they should
be, as you said, fixed by enabling the clock before reconfiguring and
disable it after (unless the PWM is enabled) like Tegra does.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Thu, Aug 23, 2012 at 07:12:04PM +0200, Lars-Peter Clausen wrote:
> On 08/23/2012 06:57 PM, Benoît Thébaudeau wrote:
> > On Thursday, August 23, 2012 5:43:32 PM, Lars-Peter Clausen wrote:
> >> On 08/23/2012 04:19 PM, Benoît Thébaudeau wrote:
> >>> Some PWM drivers enable the clock of the PWM peripheral in
> >>> pwm_enable(). Hence,
> >>> for these drivers, a call to pwm_config() does not have any effect
> >>> before
> >>> pwm_enable() has been called.
> >>>
> >>> This patch fixes the PWM users to make sure that they call
> >>> pwm_enable() before
> >>> pwm_config().
> >>>
> >>> This fixes the first setting of brightness through sysfs that had
> >>> no effect with
> >>> leds-pwm and the i.MX PWM driver.
> >>
> >> But isn't this a bug in the PWM peripheral driver? With this change
> >> the PWM
> >> will start with the old settings first. While this is not so much of
> >> a problem
> >> for a backlight (although it might cause a short flickering) it might
> >> cause
> >> problems for other applications, like using the PWM pin as a timing
> >> generator.
> >> In my opinion it's better to fix the PWM peripheral drivers which
> >> have this
> >> problem instead of trying to work around it in every user of the PWM
> >> API.
> > 
> > I don't know. See my detailed description of this issue here:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-August/115667.html
> > 
> > Where the bug is depends on the detailed definition of the PWM API, which I
> > don't find documented anywhere.
> > 
> > If pwm_enable() means "start PWM timer with the configured settings", then the
> > bug is in the drivers. If it means "enable the PWM peripheral so that we can
> > work with it", then the bug is in the PWM users.
> 
> It really is the former. See the description of pwm_enable() in drivers/pwm/core.c

Yes. pwm_enable() is only for starting the PWM and *not* the peripheral.
I should update the documentation to make this clearer.

> > I don't really have time to work on this, so I suggested this patch as a simple
> > solution. Otherwise, it means reworking several PWM drivers for different
> > hardware that is not available to everyone for testing.
> > 
> > If we decide to only change the i.MX PWM driver, the fix would be:
> > pwm_config()
> > {
> >         save passed config in private data;
> >         if (pwm enabled)
> >                 apply passed config;
> > }
> > 
> > pwm_enable()
> > {
> >         if (!(pwm enabled)) {
> >                 enable pwm ip clk;
> >                 apply config from private data;
> >         }
> > }
> 
> Another option is to enable the clock if it is disabled when the device is
> configured. E.g. that's what tegra does.

Yes, that would have been my proposal as well.

> > If we fix only this driver, we must not forget that the same issue probably
> > exists with several other PWM drivers.
> 
> Since this seems to be a common pattern in a number of PWM drivers it might
> make sense to simply add support for enabling/disabling a clk to the pwm core.
> Or maybe just use the runtime pm API for this. This probably makes even more
> sense and grab a reference to the pm context when the enable() is called,
> release it when disable() is called and also grab it before calling the
> device's config callback and release it afterward.

The problem with adding this kind of support in the core is that the
device or platform doesn't necessarily support runtime PM. Perhaps
adding a flag to mark compatible drivers would be an option. If the
runtime PM API gracefully handles cases where no callbacks are
implemented (and therefore assumed unneeded) things should also work.

On the other hand this really is very driver-specific stuff. A lot of
hardware doesn't require an explicit clock being enabled to write
registers so those wouldn't need the implicit PM calls either.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] pwm-imx: Fix config / enable / disable

[Benoît Thébaudeau]

imx_pwm_config() did not enable the PWM IP clock while accessing the registers.
Hence, a call to pwm_config() had no effect before pwm_enable() had been called,
which does not comply to the PWM API.

Moreover, calling pwm_disable() then pwm_enable() must be a transparent
operation.

This fixes the first setting of brightness through sysfs that had no effect with
leds-pwm.

Cc: Thierry Reding <thierry.reding@avionic-design.de>
Cc: <linux-kernel@vger.kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: <linux-arm-kernel@lists.infradead.org>
Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../drivers/pwm/pwm-imx.c                          |   55 +++++++++++++++-----
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git linux-next-c94456b.orig/drivers/pwm/pwm-imx.c linux-next-c94456b/drivers/pwm/pwm-imx.c
index 2a0b353..0519bf2 100644
--- linux-next-c94456b.orig/drivers/pwm/pwm-imx.c
+++ linux-next-c94456b/drivers/pwm/pwm-imx.c
@@ -55,6 +55,16 @@ static int imx_pwm_config(struct pwm_chip *chip,
 {
 	struct imx_chip *imx = to_imx_chip(chip);
 
+	/*
+	 * If the PWM is disabled, make sure to turn on the clock before
+	 * accessing the registers.
+	 */
+	if (!imx->clk_enabled) {
+		int rc = clk_prepare_enable(imx->clk);
+		if (rc)
+			return rc;
+	}
+
 	if (!(cpu_is_mx1() || cpu_is_mx21())) {
 		unsigned long long c;
 		unsigned long period_cycles, duty_cycles, prescale;
@@ -85,8 +95,11 @@ static int imx_pwm_config(struct pwm_chip *chip,
 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 		cr = MX3_PWMCR_PRESCALER(prescale) |
-			MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-			MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
+			MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
+
+		/* If the PWM is enabled, keep it so. */
+		if (imx->clk_enabled)
+			cr |= MX3_PWMCR_EN;
 
 		if (cpu_is_mx25())
 			cr |= MX3_PWMCR_CLKSRC_IPG;
@@ -118,32 +131,50 @@ static int imx_pwm_config(struct pwm_chip *chip,
 		BUG();
 	}
 
+	/* If the PWM is disabled, turn the clock off again to save power. */
+	if (!imx->clk_enabled)
+		clk_disable_unprepare(imx->clk);
+
 	return 0;
 }
 
 static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
-	int rc = 0;
+	int rc;
 
-	if (!imx->clk_enabled) {
-		rc = clk_prepare_enable(imx->clk);
-		if (!rc)
-			imx->clk_enabled = 1;
+	if (imx->clk_enabled)
+		return 0;
+
+	rc = clk_prepare_enable(imx->clk);
+	if (rc)
+		return rc;
+
+	if (!(cpu_is_mx1() || cpu_is_mx21())) {
+		u32 cr = readl(imx->mmio_base + MX3_PWMCR);
+		cr |= MX3_PWMCR_EN;
+		writel(cr, imx->mmio_base + MX3_PWMCR);
 	}
-	return rc;
+
+	imx->clk_enabled = 1;
+	return 0;
 }
 
 static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
 
-	writel(0, imx->mmio_base + MX3_PWMCR);
+	if (!imx->clk_enabled)
+		return;
 
-	if (imx->clk_enabled) {
-		clk_disable_unprepare(imx->clk);
-		imx->clk_enabled = 0;
+	if (!(cpu_is_mx1() || cpu_is_mx21())) {
+		u32 cr = readl(imx->mmio_base + MX3_PWMCR);
+		cr &= ~MX3_PWMCR_EN;
+		writel(cr, imx->mmio_base + MX3_PWMCR);
 	}
+
+	clk_disable_unprepare(imx->clk);
+	imx->clk_enabled = 0;
 }
 
 static struct pwm_ops imx_pwm_ops = {

Re: [PATCH] pwm-imx: Fix config / enable / disable

[Sascha Hauer]

On Thu, Aug 23, 2012 at 11:03:57PM +0200, Benoît Thébaudeau wrote:
> imx_pwm_config() did not enable the PWM IP clock while accessing the registers.
> Hence, a call to pwm_config() had no effect before pwm_enable() had been called,
> which does not comply to the PWM API.
> 
> Moreover, calling pwm_disable() then pwm_enable() must be a transparent
> operation.
> 
> This fixes the first setting of brightness through sysfs that had no effect with
> leds-pwm.

I don't really like this patch. I'd like to have this one first, it
makes further cleanups easier:

https://lkml.org/lkml/2012/8/28/24

Simililarly, we should probably introduce a imx_pwm_[en|dis]able_v[12]

Then, the pwm core already makes sure that pwm_enable/disable are called
only once, so the if (imx->clk_enabled) in pwm_enable/disable is
unnecessary.

Sascha

> 
> Cc: Thierry Reding <thierry.reding@avionic-design.de>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> ---
>  .../drivers/pwm/pwm-imx.c                          |   55 +++++++++++++++-----
>  1 file changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git linux-next-c94456b.orig/drivers/pwm/pwm-imx.c linux-next-c94456b/drivers/pwm/pwm-imx.c
> index 2a0b353..0519bf2 100644
> --- linux-next-c94456b.orig/drivers/pwm/pwm-imx.c
> +++ linux-next-c94456b/drivers/pwm/pwm-imx.c
> @@ -55,6 +55,16 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
>  
> +	/*
> +	 * If the PWM is disabled, make sure to turn on the clock before
> +	 * accessing the registers.
> +	 */
> +	if (!imx->clk_enabled) {
> +		int rc = clk_prepare_enable(imx->clk);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	if (!(cpu_is_mx1() || cpu_is_mx21())) {
>  		unsigned long long c;
>  		unsigned long period_cycles, duty_cycles, prescale;
> @@ -85,8 +95,11 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>  
>  		cr = MX3_PWMCR_PRESCALER(prescale) |
> -			MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> -			MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
> +			MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> +
> +		/* If the PWM is enabled, keep it so. */
> +		if (imx->clk_enabled)
> +			cr |= MX3_PWMCR_EN;
>  
>  		if (cpu_is_mx25())
>  			cr |= MX3_PWMCR_CLKSRC_IPG;
> @@ -118,32 +131,50 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  		BUG();
>  	}
>  
> +	/* If the PWM is disabled, turn the clock off again to save power. */
> +	if (!imx->clk_enabled)
> +		clk_disable_unprepare(imx->clk);
> +
>  	return 0;
>  }
>  
>  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> -	int rc = 0;
> +	int rc;
>  
> -	if (!imx->clk_enabled) {
> -		rc = clk_prepare_enable(imx->clk);
> -		if (!rc)
> -			imx->clk_enabled = 1;
> +	if (imx->clk_enabled)
> +		return 0;
> +
> +	rc = clk_prepare_enable(imx->clk);
> +	if (rc)
> +		return rc;
> +
> +	if (!(cpu_is_mx1() || cpu_is_mx21())) {
> +		u32 cr = readl(imx->mmio_base + MX3_PWMCR);
> +		cr |= MX3_PWMCR_EN;
> +		writel(cr, imx->mmio_base + MX3_PWMCR);
>  	}
> -	return rc;
> +
> +	imx->clk_enabled = 1;
> +	return 0;
>  }
>  
>  static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
>  
> -	writel(0, imx->mmio_base + MX3_PWMCR);
> +	if (!imx->clk_enabled)
> +		return;
>  
> -	if (imx->clk_enabled) {
> -		clk_disable_unprepare(imx->clk);
> -		imx->clk_enabled = 0;
> +	if (!(cpu_is_mx1() || cpu_is_mx21())) {
> +		u32 cr = readl(imx->mmio_base + MX3_PWMCR);
> +		cr &= ~MX3_PWMCR_EN;
> +		writel(cr, imx->mmio_base + MX3_PWMCR);
>  	}
> +
> +	clk_disable_unprepare(imx->clk);
> +	imx->clk_enabled = 0;
>  }
>  
>  static struct pwm_ops imx_pwm_ops = {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Jingoo Han]

On Friday, August 24, 2012 12:44 AM, Lars-Peter Clausen wrote:
> 
> On 08/23/2012 04:19 PM, Benoît Thébaudeau wrote:
> > Some PWM drivers enable the clock of the PWM peripheral in pwm_enable(). Hence,
> > for these drivers, a call to pwm_config() does not have any effect before
> > pwm_enable() has been called.
> >
> > This patch fixes the PWM users to make sure that they call pwm_enable() before
> > pwm_config().
> >
> > This fixes the first setting of brightness through sysfs that had no effect with
> > leds-pwm and the i.MX PWM driver.
> 
> But isn't this a bug in the PWM peripheral driver? With this change the PWM
> will start with the old settings first. While this is not so much of a problem
> for a backlight (although it might cause a short flickering) it might cause
> problems for other applications, like using the PWM pin as a timing generator.
> In my opinion it's better to fix the PWM peripheral drivers which have this
> problem instead of trying to work around it in every user of the PWM API.

Hi Lars-Peter Clausen,

I think so.
It would be better to fix the PWM peripheral drivers.


Best regards,
Jingoo Han

> 
> - Lars
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Mark Brown]

On Thu, Aug 23, 2012 at 07:12:04PM +0200, Lars-Peter Clausen wrote:
> On 08/23/2012 06:57 PM, Beno??t Th??baudeau wrote:

> > exists with several other PWM drivers.

> Since this seems to be a common pattern in a number of PWM drivers it might
> make sense to simply add support for enabling/disabling a clk to the pwm core.
> Or maybe just use the runtime pm API for this. This probably makes even more
> sense and grab a reference to the pm context when the enable() is called,
> release it when disable() is called and also grab it before calling the
> device's config callback and release it afterward.

It's a problem with IPs in general, a good proportion of modern SoCs
need a clock supplying in order to interact with the registers on most
of their IPs.  runtime PM is the normal way forwards, Tegra recently
started using regmap's cache code to handle this transparently in the
driver.

Re: [PATCH] pwm: Call pwm_enable() before pwm_config()

[Mark Brown]

On Thu, Aug 23, 2012 at 09:04:02PM +0200, Thierry Reding wrote:

> Using runtime PM for this sounds indeed like the most generic approach.
> I'm not very familiar with the API, but I thought it required explicit
> architecture or bus support (or the driver itself can provide hooks via
> pm_ops), so it might be difficult to implement for platforms that don't
> have working runtime PM support. I'll have to look into this some more.

It doesn't require explicit arch support, though many architectures
choose to integrate their PM frameworks with it either directly or via
power domains.  If the architecture doesn't do anything with it then it
should just work at the driver level.