Re: [PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Nicolas Ferre]

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 <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> ---
> 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 <nicolas.ferre@xxxxxxxxx>

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 <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> @@ -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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/