[PATCH] pwm: twl: Reliably disable TWL6030 PWMs

[PATCH] pwm: twl: Reliably disable TWL6030 PWMs

[Paul Kocialkowski]

The current TWL6030 code for the TWL PWM driver does not reliably disable the
PWM output, as tested with LEDs. The previous commit to that driver introduced
that regression.

However, it does make sense to disable the PWM clock after resetting the PWM,
but for some obscure reason, doing it all at once simply doesn't work.

The TWL6030 datasheet mentions that PWMs have to be disabled in two distinct
steps. However, clearing the clock enable bit in a second step (after issuing a
reset first) does not work.

The only approach that works is the one that was in place before the previous
commit to the driver. It consists in enabling the PWM clock after issuing a
reset. This is what TI kernel trees and production code seem to be using.

However, adding an extra step to disable the PWM clock seems to work reliably,
despite looking quite odd.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/pwm/pwm-twl.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
index 04f7672..7a993b0 100644
--- a/drivers/pwm/pwm-twl.c
+++ b/drivers/pwm/pwm-twl.c
@@ -269,6 +269,22 @@ static void twl6030_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 		goto out;
 	}
 
+	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXEN);
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
+		goto out;
+	}
+
+	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXEN);
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
+		goto out;
+	}
+
 	twl->twl6030_toggle3 = val;
 out:
 	mutex_unlock(&twl->mutex);
-- 
2.7.4

Re: [PATCH] pwm: twl: Reliably disable TWL6030 PWMs

[Thierry Reding]

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

On Tue, Mar 29, 2016 at 08:55:45PM +0200, Paul Kocialkowski wrote:
> The current TWL6030 code for the TWL PWM driver does not reliably disable the
> PWM output, as tested with LEDs. The previous commit to that driver introduced
> that regression.
> 
> However, it does make sense to disable the PWM clock after resetting the PWM,
> but for some obscure reason, doing it all at once simply doesn't work.
> 
> The TWL6030 datasheet mentions that PWMs have to be disabled in two distinct
> steps. However, clearing the clock enable bit in a second step (after issuing a
> reset first) does not work.
> 
> The only approach that works is the one that was in place before the previous
> commit to the driver. It consists in enabling the PWM clock after issuing a
> reset. This is what TI kernel trees and production code seem to be using.
> 
> However, adding an extra step to disable the PWM clock seems to work reliably,
> despite looking quite odd.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/pwm/pwm-twl.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Axel, Peter,

can you guys comment on this?

Thierry

> 
> diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
> index 04f7672..7a993b0 100644
> --- a/drivers/pwm/pwm-twl.c
> +++ b/drivers/pwm/pwm-twl.c
> @@ -269,6 +269,22 @@ static void twl6030_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  		goto out;
>  	}
>  
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +		goto out;
> +	}
> +
> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +		goto out;
> +	}
> +
>  	twl->twl6030_toggle3 = val;
>  out:
>  	mutex_unlock(&twl->mutex);
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pwm: twl: Reliably disable TWL6030 PWMs

[Peter Ujfalusi]

On 03/29/16 21:55, Paul Kocialkowski wrote:
> The current TWL6030 code for the TWL PWM driver does not reliably disable the
> PWM output, as tested with LEDs. The previous commit to that driver introduced
> that regression.
> 
> However, it does make sense to disable the PWM clock after resetting the PWM,
> but for some obscure reason, doing it all at once simply doesn't work.
> 
> The TWL6030 datasheet mentions that PWMs have to be disabled in two distinct
> steps. However, clearing the clock enable bit in a second step (after issuing a
> reset first) does not work.
> 
> The only approach that works is the one that was in place before the previous
> commit to the driver. It consists in enabling the PWM clock after issuing a
> reset. This is what TI kernel trees and production code seem to be using.
> 
> However, adding an extra step to disable the PWM clock seems to work reliably,
> despite looking quite odd.

Unfortunately I don't have board to test, but based on the documentation and
your explanation, this is the correct way.
Thank you for debugging and fixing it!

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/pwm/pwm-twl.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
> index 04f7672..7a993b0 100644
> --- a/drivers/pwm/pwm-twl.c
> +++ b/drivers/pwm/pwm-twl.c
> @@ -269,6 +269,22 @@ static void twl6030_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  		goto out;
>  	}
>  
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +		goto out;
> +	}
> +
> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +		goto out;
> +	}
> +
>  	twl->twl6030_toggle3 = val;
>  out:
>  	mutex_unlock(&twl->mutex);
> 


-- 
Péter

Re: [PATCH] pwm: twl: Reliably disable TWL6030 PWMs

[Thierry Reding]

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

On Tue, Mar 29, 2016 at 08:55:45PM +0200, Paul Kocialkowski wrote:
> The current TWL6030 code for the TWL PWM driver does not reliably disable the
> PWM output, as tested with LEDs. The previous commit to that driver introduced
> that regression.
> 
> However, it does make sense to disable the PWM clock after resetting the PWM,
> but for some obscure reason, doing it all at once simply doesn't work.
> 
> The TWL6030 datasheet mentions that PWMs have to be disabled in two distinct
> steps. However, clearing the clock enable bit in a second step (after issuing a
> reset first) does not work.
> 
> The only approach that works is the one that was in place before the previous
> commit to the driver. It consists in enabling the PWM clock after issuing a
> reset. This is what TI kernel trees and production code seem to be using.
> 
> However, adding an extra step to disable the PWM clock seems to work reliably,
> despite looking quite odd.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/pwm/pwm-twl.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]