[PATCH v9 0/2] Fix bugs in kona pwm driver and pwm core

[PATCH v9 0/2] Fix bugs in kona pwm driver and pwm core

[Jonathan Richardson]

This patchset fixes a bug in the Broadcom Kona pwm driver. A new procedure for
changing the pwm settings has been introduced. Also fixed a bug in the pwm core
where the enabled state was incorrect on failed calls to enable.

These changes are required for the Kona PWM driver to work on Cygnus. The same
PWM IP is being used.

Changes from v8:
  - Accepted patches not included. Patch to introduce debug statements to config
	in kona pwm driver not included - will be addressed later.
  - Added mutex to pwm core enable function to prevent potential concurrency
	issue, as suggested by Thierry.
  - Fixed commit message for kona pwm changes. Also changed wording in comments
	from enable to trigger.

Changes from v7:
  - Polarity changes take effect immediately instead of being deferred until
    enable is called.

Changes from v6:
  - Move new debugging info for duty cycle and period in config function into
    its own commit.
  - Add kona_pwmc_prepare_for_settings() function to remove duplicated code.

Changes from v5:
  - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core.
  - Including all patches from different contributors in this patchset. Some
    were left out in v4.

Changes from v4:
  - Rebased to Tim Kryger's patch that adds support in pwm core to add driver
    with inversed polarity.
  - Removed patch 2 that resolved difference between hardware default polarity
    and pwm framework default polarity. The default polarity is set properly now
    when the pwm driver is registered with the pwm framework.

Jonathan Richardson (2):
  pwm: kona: Modify settings application sequence
  pwm: core: Set enable state properly on failed call to enable

 drivers/pwm/core.c         |   19 +++++++++++++++---
 drivers/pwm/pwm-bcm-kona.c |   47 +++++++++++++++++++++++++++++++++++---------
 include/linux/pwm.h        |    2 ++
 3 files changed, 56 insertions(+), 12 deletions(-)

-- 
1.7.9.5

[PATCH v9 1/2] pwm: kona: Modify settings application sequence

[Jonathan Richardson]

Update the driver so that settings are applied in accordance with the
most recent version of the hardware spec.  The revised sequence clears
the trigger bit, waits 400ns, writes settings, sets the trigger bit,
and waits another 400ns.  This corrects an issue where occasionally a
requested change was not properly reflected in the PWM output.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Reviewed-by: Tim Kryger <tim.kryger@gmail.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   47 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 7af8fea..9b1d397 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
 	return container_of(_chip, struct kona_pwmc, chip);
 }
 
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+	unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-	/* Clear trigger bit but set smooth bit to maintain old output */
 	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
 	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
+	/*
+	 * There must be a min 400ns delay between clearing trigger and setting
+	 * it. Failing to do this may result in no PWM signal.
+	 */
+	ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
 	/* Set trigger bit and clear smooth bit to apply new settings */
 	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	/* Trigger bit must be held high for at least 400 ns. */
+	ndelay(400);
 }
 
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			return -EINVAL;
 	}
 
-	/* If the PWM channel is enabled, write the settings to the HW */
+	/*
+	 * Don't apply settings if disabled. The period and duty cycle are
+	 * always calculated above to ensure the new values are
+	 * validated immediately instead of on enable.
+	 */
 	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		kona_pwmc_prepare_for_settings(kp, chan);
+
 		value = readl(kp->base + PRESCALE_OFFSET);
 		value &= ~PRESCALE_MASK(chan);
 		value |= prescale << PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 		return ret;
 	}
 
+	kona_pwmc_prepare_for_settings(kp, chan);
+
 	value = readl(kp->base + PWM_CONTROL_OFFSET);
 
 	if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	kona_pwmc_apply_settings(kp, chan);
 
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
-
 	clk_disable_unprepare(kp->clk);
 
 	return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	unsigned int chan = pwm->hwpwm;
+	unsigned int value;
+
+	kona_pwmc_prepare_for_settings(kp, chan);
 
 	/* Simulate a disable by configuring for zero duty */
 	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-	kona_pwmc_apply_settings(kp, chan);
+	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
+	/* Set prescale to 0 for this channel */
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	clk_disable_unprepare(kp->clk);
 }
-- 
1.7.9.5

Re: [PATCH v9 1/2] pwm: kona: Modify settings application sequence

[Jonathan Richardson]

Hi Theirry,

If there are no more comments can this patch be applied?

Thanks,
Jon

On 15-06-15 02:21 PM, Jonathan Richardson wrote:
> Update the driver so that settings are applied in accordance with the
> most recent version of the hardware spec.  The revised sequence clears
> the trigger bit, waits 400ns, writes settings, sets the trigger bit,
> and waits another 400ns.  This corrects an issue where occasionally a
> requested change was not properly reflected in the PWM output.
> 
> Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Tested-by: Scott Branden <sbranden@broadcom.com>
> Reviewed-by: Tim Kryger <tim.kryger@gmail.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c |   47 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 7af8fea..9b1d397 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
>  	return container_of(_chip, struct kona_pwmc, chip);
>  }
>  
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +/*
> + * Clear trigger bit but set smooth bit to maintain old output.
> + */
> +static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
> +	unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
> -	/* Clear trigger bit but set smooth bit to maintain old output */
>  	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>  	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> +	/*
> +	 * There must be a min 400ns delay between clearing trigger and setting
> +	 * it. Failing to do this may result in no PWM signal.
> +	 */
> +	ndelay(400);
> +}
> +
> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +{
> +	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
>  	/* Set trigger bit and clear smooth bit to apply new settings */
>  	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>  	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
> +
> +	/* Trigger bit must be held high for at least 400 ns. */
> +	ndelay(400);
>  }
>  
>  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			return -EINVAL;
>  	}
>  
> -	/* If the PWM channel is enabled, write the settings to the HW */
> +	/*
> +	 * Don't apply settings if disabled. The period and duty cycle are
> +	 * always calculated above to ensure the new values are
> +	 * validated immediately instead of on enable.
> +	 */
>  	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> +		kona_pwmc_prepare_for_settings(kp, chan);
> +
>  		value = readl(kp->base + PRESCALE_OFFSET);
>  		value &= ~PRESCALE_MASK(chan);
>  		value |= prescale << PRESCALE_SHIFT(chan);
> @@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return ret;
>  	}
>  
> +	kona_pwmc_prepare_for_settings(kp, chan);
> +
>  	value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
>  	if (polarity == PWM_POLARITY_NORMAL)
> @@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	kona_pwmc_apply_settings(kp, chan);
>  
> -	/* Wait for waveform to settle before gating off the clock */
> -	ndelay(400);
> -
>  	clk_disable_unprepare(kp->clk);
>  
>  	return 0;
> @@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
>  	unsigned int chan = pwm->hwpwm;
> +	unsigned int value;
> +
> +	kona_pwmc_prepare_for_settings(kp, chan);
>  
>  	/* Simulate a disable by configuring for zero duty */
>  	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> -	kona_pwmc_apply_settings(kp, chan);
> +	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>  
> -	/* Wait for waveform to settle before gating off the clock */
> -	ndelay(400);
> +	/* Set prescale to 0 for this channel */
> +	value = readl(kp->base + PRESCALE_OFFSET);
> +	value &= ~PRESCALE_MASK(chan);
> +	writel(value, kp->base + PRESCALE_OFFSET);
> +
> +	kona_pwmc_apply_settings(kp, chan);
>  
>  	clk_disable_unprepare(kp->clk);
>  }
> 

Re: [PATCH v9 1/2] pwm: kona: Modify settings application sequence

[Thierry Reding]

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

On Mon, Jun 15, 2015 at 02:21:01PM -0700, Jonathan Richardson wrote:
> Update the driver so that settings are applied in accordance with the
> most recent version of the hardware spec.  The revised sequence clears
> the trigger bit, waits 400ns, writes settings, sets the trigger bit,
> and waits another 400ns.  This corrects an issue where occasionally a
> requested change was not properly reflected in the PWM output.
> 
> Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Tested-by: Scott Branden <sbranden@broadcom.com>
> Reviewed-by: Tim Kryger <tim.kryger@gmail.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c |   47 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 9 deletions(-)

Applied, thanks.

Thierry

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

[PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable

[Jonathan Richardson]

The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.

Tested-by: Jonathan Richardson <jonathar@broadcom.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/core.c  |   19 ++++++++++++++++---
 include/linux/pwm.h |    2 ++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 76b0386..c255267 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
 		pwm->polarity = polarity;
+		mutex_init(&pwm->lock);
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
-		return pwm->chip->ops->enable(pwm->chip, pwm);
+	int err = 0;
 
-	return pwm ? 0 : -EINVAL;
+	if (!pwm)
+		return -EINVAL;
+
+	mutex_lock(&pwm->lock);
+
+	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
+		err = pwm->chip->ops->enable(pwm->chip, pwm);
+		if (err)
+			clear_bit(PWMF_ENABLED, &pwm->flags);
+	}
+
+	mutex_unlock(&pwm->lock);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 9daf0ef..50f28e6 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,7 @@
 
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/mutex.h>
 
 struct pwm_device;
 struct seq_file;
@@ -86,6 +87,7 @@ struct pwm_device {
 	unsigned int		pwm;
 	struct pwm_chip		*chip;
 	void			*chip_data;
+	struct mutex		lock;
 
 	unsigned int		period; 	/* in nanoseconds */
 	unsigned int		duty_cycle;	/* in nanoseconds */
-- 
1.7.9.5

Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable

[Jonathan Richardson]

On 15-06-15 02:21 PM, Jonathan Richardson wrote:
> The pwm_enable function didn't clear the enabled bit if a call to a
> clients enable function returned an error. The result was that the state
> of the pwm core was wrong. Clearing the bit when enable returns an error
> ensures the state is properly set.
> 
> Tested-by: Jonathan Richardson <jonathar@broadcom.com>
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/core.c  |   19 ++++++++++++++++---
>  include/linux/pwm.h |    2 ++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 76b0386..c255267 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->pwm = chip->base + i;
>  		pwm->hwpwm = i;
>  		pwm->polarity = polarity;
> +		mutex_init(&pwm->lock);
>  
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
> @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
>   */
>  int pwm_enable(struct pwm_device *pwm)
>  {
> -	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
> -		return pwm->chip->ops->enable(pwm->chip, pwm);
> +	int err = 0;
>  
> -	return pwm ? 0 : -EINVAL;
> +	if (!pwm)
> +		return -EINVAL;
> +
> +	mutex_lock(&pwm->lock);
> +
> +	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> +		err = pwm->chip->ops->enable(pwm->chip, pwm);
> +		if (err)
> +			clear_bit(PWMF_ENABLED, &pwm->flags);
> +	}
> +
> +	mutex_unlock(&pwm->lock);
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(pwm_enable);

I meant to add the mutex check in disable also, but what about when
PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?

Thanks.

Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable

[Thierry Reding]

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

On Mon, Jun 15, 2015 at 04:22:27PM -0700, Jonathan Richardson wrote:
> On 15-06-15 02:21 PM, Jonathan Richardson wrote:
> > The pwm_enable function didn't clear the enabled bit if a call to a
> > clients enable function returned an error. The result was that the state
> > of the pwm core was wrong. Clearing the bit when enable returns an error
> > ensures the state is properly set.
> > 
> > Tested-by: Jonathan Richardson <jonathar@broadcom.com>
> > Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> > ---
> >  drivers/pwm/core.c  |   19 ++++++++++++++++---
> >  include/linux/pwm.h |    2 ++
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 76b0386..c255267 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> >  		pwm->pwm = chip->base + i;
> >  		pwm->hwpwm = i;
> >  		pwm->polarity = polarity;
> > +		mutex_init(&pwm->lock);
> >  
> >  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> >  	}
> > @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> >   */
> >  int pwm_enable(struct pwm_device *pwm)
> >  {
> > -	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
> > -		return pwm->chip->ops->enable(pwm->chip, pwm);
> > +	int err = 0;
> >  
> > -	return pwm ? 0 : -EINVAL;
> > +	if (!pwm)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&pwm->lock);
> > +
> > +	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> > +		err = pwm->chip->ops->enable(pwm->chip, pwm);
> > +		if (err)
> > +			clear_bit(PWMF_ENABLED, &pwm->flags);
> > +	}
> > +
> > +	mutex_unlock(&pwm->lock);
> > +
> > +	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(pwm_enable);
> 
> I meant to add the mutex check in disable also, but what about when
> PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?

I think for debugfs we're fine since there's no potential to race there.
It'll simply show the state of the PWM at the point where it was queried
even though that may change immediately after. I suppose we could keep
the lock across the body of the loop just to make sure debugfs will show
a consistent view of the PWM.

For pwm_disable() I don't think we need the lock, since the test_and_
clear_bit() is atomic and ->disable() cannot fail.

As for pwm_set_polarity(), I think it would need to be something like
the below:

---- >8 ----

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3ea3350..8488c7a19bf6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -473,16 +473,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
        if (!pwm->chip->ops->set_polarity)
                return -ENOSYS;
 
-       if (pwm_is_enabled(pwm))
-               return -EBUSY;
+       mutex_lock(&pwm->lock);
+
+       if (pwm_is_enabled(pwm)) {
+               err = -EBUSY;
+               goto unlock;
+       }
 
        err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
        if (err)
-               return err;
+               goto unlock;
 
        pwm->polarity = polarity;
 
-       return 0;
+unlock:
+       mutex_unlock(&pwm->lock);
+       return err;
 }
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 

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

Re: [PATCH v9 2/2] pwm: core: Set enable state properly on failed call to enable

[Jonathan Richardson]

On 15-08-17 07:31 AM, Thierry Reding wrote:
> On Mon, Jun 15, 2015 at 04:22:27PM -0700, Jonathan Richardson wrote:
>> On 15-06-15 02:21 PM, Jonathan Richardson wrote:
>>> The pwm_enable function didn't clear the enabled bit if a call to a
>>> clients enable function returned an error. The result was that the state
>>> of the pwm core was wrong. Clearing the bit when enable returns an error
>>> ensures the state is properly set.
>>>
>>> Tested-by: Jonathan Richardson <jonathar@broadcom.com>
>>> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
>>> ---
>>>  drivers/pwm/core.c  |   19 ++++++++++++++++---
>>>  include/linux/pwm.h |    2 ++
>>>  2 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>> index 76b0386..c255267 100644
>>> --- a/drivers/pwm/core.c
>>> +++ b/drivers/pwm/core.c
>>> @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>>  		pwm->pwm = chip->base + i;
>>>  		pwm->hwpwm = i;
>>>  		pwm->polarity = polarity;
>>> +		mutex_init(&pwm->lock);
>>>  
>>>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>>>  	}
>>> @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
>>>   */
>>>  int pwm_enable(struct pwm_device *pwm)
>>>  {
>>> -	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
>>> -		return pwm->chip->ops->enable(pwm->chip, pwm);
>>> +	int err = 0;
>>>  
>>> -	return pwm ? 0 : -EINVAL;
>>> +	if (!pwm)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&pwm->lock);
>>> +
>>> +	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>> +		err = pwm->chip->ops->enable(pwm->chip, pwm);
>>> +		if (err)
>>> +			clear_bit(PWMF_ENABLED, &pwm->flags);
>>> +	}
>>> +
>>> +	mutex_unlock(&pwm->lock);
>>> +
>>> +	return err;
>>>  }
>>>  EXPORT_SYMBOL_GPL(pwm_enable);
>>
>> I meant to add the mutex check in disable also, but what about when
>> PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?
> 
> I think for debugfs we're fine since there's no potential to race there.
> It'll simply show the state of the PWM at the point where it was queried
> even though that may change immediately after. I suppose we could keep
> the lock across the body of the loop just to make sure debugfs will show
> a consistent view of the PWM.
> 
> For pwm_disable() I don't think we need the lock, since the test_and_
> clear_bit() is atomic and ->disable() cannot fail.
> 
> As for pwm_set_polarity(), I think it would need to be something like
> the below:
> 
> ---- >8 ----
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3f9df3ea3350..8488c7a19bf6 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -473,16 +473,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>         if (!pwm->chip->ops->set_polarity)
>                 return -ENOSYS;
>  
> -       if (pwm_is_enabled(pwm))
> -               return -EBUSY;
> +       mutex_lock(&pwm->lock);
> +
> +       if (pwm_is_enabled(pwm)) {
> +               err = -EBUSY;
> +               goto unlock;
> +       }
>  
>         err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
>         if (err)
> -               return err;
> +               goto unlock;
>  
>         pwm->polarity = polarity;
>  
> -       return 0;
> +unlock:
> +       mutex_unlock(&pwm->lock);
> +       return err;
>  }
>  EXPORT_SYMBOL_GPL(pwm_set_polarity);
>  
> 

Thierry,

Sounds good to me. I'll send a patch out for this hopefully today. I
don't see a need to complicate debugfs with obscure functionality so the
patch will just add polarity as you have shown it here and the enable
routine as the previous patchset.

Jon