Re: [PATCH] pwm-imx: Fix config / enable / disable
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: 2012-08-28 07:37:56
Also in:
linux-arm-kernel, linux-fbdev, linux-input
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
quoted hunk ↗ jump to hunk
Cc: Thierry Reding <redacted> Cc: <redacted> Cc: Sascha Hauer <kernel@pengutronix.de> Cc: <redacted> Cc: Benoît Thébaudeau <redacted> Signed-off-by: Benoît Thébaudeau <redacted> --- .../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 |