Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753644AbbEZNLD (ORCPT ); Tue, 26 May 2015 09:11:03 -0400 Received: from eusmtp01.atmel.com ([212.144.249.242]:49220 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753282AbbEZNKn (ORCPT ); Tue, 26 May 2015 09:10:43 -0400 Message-ID: <55644AD4.6080703@atmel.com> Date: Tue, 26 May 2015 12:28:36 +0200 From: Nicolas Ferre Organization: atmel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Alexandre Belloni , Thierry Reding CC: , , Subject: Re: [PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling References: <1432559996-4415-2-git-send-email-alexandre.belloni@free-electrons.com> <1432570309-17819-1-git-send-email-alexandre.belloni@free-electrons.com> In-Reply-To: <1432570309-17819-1-git-send-email-alexandre.belloni@free-electrons.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.161.30.18] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4406 Lines: 123 Le 25/05/2015 18:11, Alexandre Belloni a ?crit : > pwm-leds calls .config() and .disable() in a row. This exhibits that it may > happen that the channel gets disabled before CDTY has been updated with CUPD. > The issue gets quite worse with long periods. > So, ensure that at least one period has past before disabling the channel by > polling ISR. > > Signed-off-by: Alexandre Belloni > --- > I had the previous patch tested on a multicolor LED and unfortunately, the delay > is not nice in that case. I wanted to avoid a mutex. > This version uses a variable to store the state of the update of the pwm. This > is reset everytime is gets a new configuration and updated when it is disabled > or another pwm is configured. > When disabling a multicolor LED, we only wait for the first pwm to update then > we remember that we already seen the other one having their period expire and it > looks much better. It seems valid: Acked-by: Nicolas Ferre Thanks! > > drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > index 89f9ca41d9af..a947c9095d9d 100644 > --- a/drivers/pwm/pwm-atmel.c > +++ b/drivers/pwm/pwm-atmel.c > @@ -8,9 +8,11 @@ > */ > > #include > +#include > #include > #include > #include > +#include > #include > #include > #include > @@ -21,6 +23,7 @@ > #define PWM_ENA 0x04 > #define PWM_DIS 0x08 > #define PWM_SR 0x0C > +#define PWM_ISR 0x1C > /* Bit field in SR */ > #define PWM_SR_ALL_CH_ON 0x0F > > @@ -60,6 +63,9 @@ struct atmel_pwm_chip { > struct clk *clk; > void __iomem *base; > > + unsigned int updated_pwms; > + struct mutex isr_lock; /* ISR is cleared when read, ensure only one thread does that */ > + > void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, > unsigned long dty, unsigned long prd); > }; > @@ -144,6 +150,10 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK); > atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); > atmel_pwm->config(chip, pwm, dty, prd); > + mutex_lock(&atmel_pwm->isr_lock); > + atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR); > + atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm); > + mutex_unlock(&atmel_pwm->isr_lock); > > clk_disable(atmel_pwm->clk); > return ret; > @@ -243,7 +253,22 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + unsigned long timeout = jiffies + 2 * HZ; > + > + /* > + * Wait for at least a complete period to have passed before disabling a > + * channel to be sure that CDTY has been updated > + */ > + mutex_lock(&atmel_pwm->isr_lock); > + atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR); > + > + while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) && > + time_before(jiffies, timeout)) { > + usleep_range(10, 100); > + atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR); > + } > > + mutex_unlock(&atmel_pwm->isr_lock); > atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm); > > clk_disable(atmel_pwm->clk); > @@ -358,6 +383,8 @@ static int atmel_pwm_probe(struct platform_device *pdev) > atmel_pwm->chip.npwm = 4; > atmel_pwm->chip.can_sleep = true; > atmel_pwm->config = data->config; > + atmel_pwm->updated_pwms = 0; > + mutex_init(&atmel_pwm->isr_lock); > > ret = pwmchip_add(&atmel_pwm->chip); > if (ret < 0) { > @@ -379,6 +406,7 @@ static int atmel_pwm_remove(struct platform_device *pdev) > struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev); > > clk_unprepare(atmel_pwm->clk); > + mutex_destroy(&atmel_pwm->isr_lock); > > return pwmchip_remove(&atmel_pwm->chip); > } > -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/