[PATCH v8 0/5] Fix bugs in kona pwm driver and pwm core

[PATCH v8 0/5] Fix bugs in kona pwm driver and pwm core

[Jonathan Richardson]

This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also
fixes a bug in the pwm core where the enabled state was incorrect on failed
calls to enable. Also, a new function was added to the pwm core to add pwm chips
with inversed polarity for chips that have a different default polarity than the
core. The prevents incorrect polarity being reported.

Debug info has been added to help debug configuring duty cycle and period.

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

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.


Arun Ramamurthy (1):
  drivers: pwm: bcm-kona: Dont set polarity in probe

Jonathan Richardson (3):
  pwm: kona: Fix incorrect config, disable, and polarity procedures
  pwm: kona: Add debug info to config function
  pwm: core: Set enable state properly on failed call to enable

Tim Kryger (1):
  drivers: pwm: core: Add pwmchip_add_inversed

 drivers/pwm/core.c         |   52 ++++++++++++++++++++++------
 drivers/pwm/pwm-bcm-kona.c |   81 ++++++++++++++++++++++++++++++++++----------
 include/linux/pwm.h        |    6 ++++
 3 files changed, 111 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[PATCH v8 0/5] Fix bugs in kona pwm driver and pwm core

[Jonathan Richardson]

This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also
fixes a bug in the pwm core where the enabled state was incorrect on failed
calls to enable. Also, a new function was added to the pwm core to add pwm chips
with inversed polarity for chips that have a different default polarity than the
core. The prevents incorrect polarity being reported.

Debug info has been added to help debug configuring duty cycle and period.

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

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.


Arun Ramamurthy (1):
  drivers: pwm: bcm-kona: Dont set polarity in probe

Jonathan Richardson (3):
  pwm: kona: Fix incorrect config, disable, and polarity procedures
  pwm: kona: Add debug info to config function
  pwm: core: Set enable state properly on failed call to enable

Tim Kryger (1):
  drivers: pwm: core: Add pwmchip_add_inversed

 drivers/pwm/core.c         |   52 ++++++++++++++++++++++------
 drivers/pwm/pwm-bcm-kona.c |   81 ++++++++++++++++++++++++++++++++++----------
 include/linux/pwm.h        |    6 ++++
 3 files changed, 111 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[PATCH v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed

[Jonathan Richardson]

From: Tim Kryger <tim.kryger@gmail.com>

Add a new function to register a PWM chip with channels that have their
initial polarity as inversed.  This benefits drivers of controllers that
by default operate with inversed polarity by removing the need to modify
the polarity during initialization.

Signed-off-by: Tim Kryger <tim.kryger@gmail.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/core.c  |   36 ++++++++++++++++++++++++++++--------
 include/linux/pwm.h |    6 ++++++
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ba34c7d..224645f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_get_chip_data);
 
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+				     enum pwm_polarity polarity)
 {
 	struct pwm_device *pwm;
 	unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
 		pwm->chip = chip;
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
+		pwm->polarity = polarity;
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -279,9 +274,34 @@ out:
 	mutex_unlock(&pwm_lock);
 	return ret;
 }
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used.  The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
 /**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used.  The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+	return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
  * pwmchip_remove() - remove a PWM chip
  * @chip: the PWM chip to remove
  *
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..358547f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
 void *pwm_get_chip_data(struct pwm_device *pwm);
 
 int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
 struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
 	return -EINVAL;
 }
 
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+	return -EINVAL;
+}
+
 static inline int pwmchip_remove(struct pwm_chip *chip)
 {
 	return -EINVAL;
-- 
1.7.9.5

[PATCH v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed

[Jonathan Richardson]

From: Tim Kryger <tim.kryger@gmail.com>

Add a new function to register a PWM chip with channels that have their
initial polarity as inversed.  This benefits drivers of controllers that
by default operate with inversed polarity by removing the need to modify
the polarity during initialization.

Signed-off-by: Tim Kryger <tim.kryger@gmail.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/core.c  |   36 ++++++++++++++++++++++++++++--------
 include/linux/pwm.h |    6 ++++++
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ba34c7d..224645f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_get_chip_data);
 
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+				     enum pwm_polarity polarity)
 {
 	struct pwm_device *pwm;
 	unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
 		pwm->chip = chip;
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
+		pwm->polarity = polarity;
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -279,9 +274,34 @@ out:
 	mutex_unlock(&pwm_lock);
 	return ret;
 }
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used.  The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
 /**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used.  The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+	return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
  * pwmchip_remove() - remove a PWM chip
  * @chip: the PWM chip to remove
  *
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..358547f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
 void *pwm_get_chip_data(struct pwm_device *pwm);
 
 int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
 struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
 	return -EINVAL;
 }
 
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+	return -EINVAL;
+}
+
 static inline int pwmchip_remove(struct pwm_chip *chip)
 {
 	return -EINVAL;
-- 
1.7.9.5

Re: [PATCH v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed

[Thierry Reding]

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

On Tue, May 26, 2015 at 01:08:16PM -0700, Jonathan Richardson wrote:
> From: Tim Kryger <tim.kryger@gmail.com>
> 
> Add a new function to register a PWM chip with channels that have their
> initial polarity as inversed.  This benefits drivers of controllers that
> by default operate with inversed polarity by removing the need to modify
> the polarity during initialization.
> 
> Signed-off-by: Tim Kryger <tim.kryger@gmail.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/core.c  |   36 ++++++++++++++++++++++++++++--------
>  include/linux/pwm.h |    6 ++++++
>  2 files changed, 34 insertions(+), 8 deletions(-)

I had to bikeshed this a little, so I ended up applying a variant that
exports pwmchip_add_with_polarity() instead of having the additional
wrapper. The rationale here is that pwmchip_add_with_polarity() is more
explicit than pwmchip_add_inversed().

Thierry

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

Re: [PATCH v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed

[Tim Kryger]

On Fri, Jun 12, 2015 at 2:45 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, May 26, 2015 at 01:08:16PM -0700, Jonathan Richardson wrote:
>> From: Tim Kryger <tim.kryger@gmail.com>
>>
>> Add a new function to register a PWM chip with channels that have their
>> initial polarity as inversed.  This benefits drivers of controllers that
>> by default operate with inversed polarity by removing the need to modify
>> the polarity during initialization.
>>
>> Signed-off-by: Tim Kryger <tim.kryger@gmail.com>
>> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
>> ---
>>  drivers/pwm/core.c  |   36 ++++++++++++++++++++++++++++--------
>>  include/linux/pwm.h |    6 ++++++
>>  2 files changed, 34 insertions(+), 8 deletions(-)
>
> I had to bikeshed this a little, so I ended up applying a variant that
> exports pwmchip_add_with_polarity() instead of having the additional
> wrapper. The rationale here is that pwmchip_add_with_polarity() is more
> explicit than pwmchip_add_inversed().
>
> Thierry

Sounds good.  Thanks.

-Tim

[PATCH v8 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe

[Jonathan Richardson]

From: Arun Ramamurthy <arunrama@broadcom.com>

Omit setting the polarity to normal during probe and instead use the
new pwmchip_add_inversed function to register a PWM chip with default
polarity of inversed for all channels as this is the actual hardware
default.

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

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..32b3ec6 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* Set smooth mode, push/pull, and normal polarity for all channels */
-	for (chan = 0; chan < kp->chip.npwm; chan++) {
-		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	/* Set push/pull for all channels */
+	for (chan = 0; chan < kp->chip.npwm; chan++)
 		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
-		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
-	}
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
 	clk_disable_unprepare(kp->clk);
 
-	ret = pwmchip_add(&kp->chip);
+	ret = pwmchip_add_inversed(&kp->chip);
 	if (ret < 0)
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
 
-- 
1.7.9.5

[PATCH v8 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe

[Jonathan Richardson]

From: Arun Ramamurthy <arunrama@broadcom.com>

Omit setting the polarity to normal during probe and instead use the
new pwmchip_add_inversed function to register a PWM chip with default
polarity of inversed for all channels as this is the actual hardware
default.

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

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..32b3ec6 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* Set smooth mode, push/pull, and normal polarity for all channels */
-	for (chan = 0; chan < kp->chip.npwm; chan++) {
-		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	/* Set push/pull for all channels */
+	for (chan = 0; chan < kp->chip.npwm; chan++)
 		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
-		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
-	}
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
 	clk_disable_unprepare(kp->clk);
 
-	ret = pwmchip_add(&kp->chip);
+	ret = pwmchip_add_inversed(&kp->chip);
 	if (ret < 0)
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
 
-- 
1.7.9.5

Re: [PATCH v8 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe

[Thierry Reding]

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

On Tue, May 26, 2015 at 01:08:17PM -0700, Jonathan Richardson wrote:
> From: Arun Ramamurthy <arunrama@broadcom.com>
> 
> Omit setting the polarity to normal during probe and instead use the
> new pwmchip_add_inversed function to register a PWM chip with default
> polarity of inversed for all channels as this is the actual hardware
> default.
> 
> Signed-off-by: Arun Ramamurthy <arunrama@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Tim Kryger <tim.kryger@gmail.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c |    9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

Applied, after adapting to the pwmchip_add_with_polarity() API, thanks.

Thierry

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

[PATCH v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures

[Jonathan Richardson]

The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.

The disable and polarity procedures now also follow the spec. The old
procedures would result in no change in signal when called.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.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 32b3ec6..c87621f 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 enable 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);
+
+	/* PWMOUT_ENABLE 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

[PATCH v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures

[Jonathan Richardson]

The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.

The disable and polarity procedures now also follow the spec. The old
procedures would result in no change in signal when called.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.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 32b3ec6..c87621f 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 enable 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);
+
+	/* PWMOUT_ENABLE 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 v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures

[Tim Kryger]

On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
> The config procedure didn't follow the spec which periodically resulted
> in failing to enable the output signal. This happened one in ten or
> twenty attempts. Following the spec and adding a 400ns delay in the
> appropriate locations resolves this problem.
>
> The disable and polarity procedures now also follow the spec. The old
> procedures would result in no change in signal when called.

I think you may want to adjust your commit title and message to more
clearly describe what this change is doing. Perhaps something like:

pwm: kona: Modify settings application sequence

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.

Otherwise, this patch looks reasonable so

Reviewed-by: Tim Kryger <tim.kryger@gmail.com>

>
> Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Tested-by: Scott Branden <sbranden@broadcom.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 32b3ec6..c87621f 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 enable and setting
> +        * it. Failing to do this may result in no PWM signal.
> +        */
> +       ndelay(400);
> +}

Since it doesn't function as an enable, please call it the trigger bit.

> +
> +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);
> +
> +       /* PWMOUT_ENABLE must be held high for at least 400 ns. */
> +       ndelay(400);
>  }

Same here.

>
>  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 v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures

[Jonathan Richardson]

On 15-05-30 09:41 AM, Tim Kryger wrote:
> On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> The config procedure didn't follow the spec which periodically resulted
>> in failing to enable the output signal. This happened one in ten or
>> twenty attempts. Following the spec and adding a 400ns delay in the
>> appropriate locations resolves this problem.
>>
>> The disable and polarity procedures now also follow the spec. The old
>> procedures would result in no change in signal when called.
> 
> I think you may want to adjust your commit title and message to more
> clearly describe what this change is doing. Perhaps something like:
> 
> pwm: kona: Modify settings application sequence
> 
> 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.
> 
> Otherwise, this patch looks reasonable so
> 
> Reviewed-by: Tim Kryger <tim.kryger@gmail.com>

Fine with me. Same with two comments below. Will re-spin when I can see
Thierry's modification to use pwmchip_add_with_polarity() instead of
pwmchip_add_inversed().

Thanks.

> 
>>
>> Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Tested-by: Scott Branden <sbranden@broadcom.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 32b3ec6..c87621f 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 enable and setting
>> +        * it. Failing to do this may result in no PWM signal.
>> +        */
>> +       ndelay(400);
>> +}
> 
> Since it doesn't function as an enable, please call it the trigger bit.
> 
>> +
>> +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);
>> +
>> +       /* PWMOUT_ENABLE must be held high for at least 400 ns. */
>> +       ndelay(400);
>>  }
> 
> Same here.
> 
>>
>>  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
>>

[PATCH v8 4/5] pwm: kona: Add debug info to config function

[Jonathan Richardson]

Adds debugging info to config function where duty cycle and period
are calculated and verified.

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c87621f..0ddf19b 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < PERIOD_COUNT_MIN) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+				__func__, chan, period_ns, pc, prescale);
 			return -EINVAL;
+		}
+
+		if (dc < DUTY_CYCLE_HIGH_MIN) {
+			if (0 != duty_ns) {
+				dev_warn(chip->dev,
+					"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+					__func__, chan, duty_ns, dc, prescale);
+			}
+			return -EINVAL;
+		}
 
 		/* If pc and dc are in bounds, the calculation is done */
 		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > PRESCALE_MAX) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+				__func__, chan, prescale, PRESCALE_MAX,
+				period_ns, duty_ns);
 			return -EINVAL;
+		}
 	}
 
+	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+		chan, pc, dc, prescale);
+
 	/*
 	 * Don't apply settings if disabled. The period and duty cycle are
 	 * always calculated above to ensure the new values are
-- 
1.7.9.5

[PATCH v8 4/5] pwm: kona: Add debug info to config function

[Jonathan Richardson]

Adds debugging info to config function where duty cycle and period
are calculated and verified.

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c87621f..0ddf19b 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < PERIOD_COUNT_MIN) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+				__func__, chan, period_ns, pc, prescale);
 			return -EINVAL;
+		}
+
+		if (dc < DUTY_CYCLE_HIGH_MIN) {
+			if (0 != duty_ns) {
+				dev_warn(chip->dev,
+					"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+					__func__, chan, duty_ns, dc, prescale);
+			}
+			return -EINVAL;
+		}
 
 		/* If pc and dc are in bounds, the calculation is done */
 		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > PRESCALE_MAX) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+				__func__, chan, prescale, PRESCALE_MAX,
+				period_ns, duty_ns);
 			return -EINVAL;
+		}
 	}
 
+	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+		chan, pc, dc, prescale);
+
 	/*
 	 * Don't apply settings if disabled. The period and duty cycle are
 	 * always calculated above to ensure the new values are
-- 
1.7.9.5

Re: [PATCH v8 4/5] pwm: kona: Add debug info to config function

[Tim Kryger]

On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
> Adds debugging info to config function where duty cycle and period
> are calculated and verified.
>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c |   25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index c87621f..0ddf19b 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                 dc = div64_u64(val, div);
>
>                 /* If duty_ns or period_ns are not achievable then return */
> -               if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)

The original code was based on the SPEAr PWM driver which has a non-zero
PWMDCR_MIN_DUTY such that the second condition here can evaluate to true.

This isn't the case for the Kona PWM where DUTY_CYCLE_HIGH_MIN is zero.

> +               if (pc < PERIOD_COUNT_MIN) {
> +                       dev_warn(chip->dev,
> +                               "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
> +                               __func__, chan, period_ns, pc, prescale);
>                         return -EINVAL;
> +               }

Why not just print the minimum allowable period with the provided clock?

I don't think pc and prescale will be particularly helpful to users.

Also, do we really need to print __func__ here?

> +
> +               if (dc < DUTY_CYCLE_HIGH_MIN) {
> +                       if (0 != duty_ns) {
> +                               dev_warn(chip->dev,
> +                                       "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
> +                                       __func__, chan, duty_ns, dc, prescale);
> +                       }
> +                       return -EINVAL;
> +               }

The above block is unreachable code.

>
>                 /* If pc and dc are in bounds, the calculation is done */
>                 if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
>                         break;
>
>                 /* Otherwise, increase prescale and recalculate pc and dc */
> -               if (++prescale > PRESCALE_MAX)
> +               if (++prescale > PRESCALE_MAX) {
> +                       dev_warn(chip->dev,
> +                               "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
> +                               __func__, chan, prescale, PRESCALE_MAX,
> +                               period_ns, duty_ns);
>                         return -EINVAL;
> +               }
>         }

The user got here because they specified a period larger than the maximum
supported so why not tell them largest value that can be supported instead
of confusing them with prescale and PRESCALE_MAX?

>
> +       dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
> +               chan, pc, dc, prescale);
> +

This could be more clear.  It prints pc but calls it period.

>         /*
>          * Don't apply settings if disabled. The period and duty cycle are
>          * always calculated above to ensure the new values are
> --
> 1.7.9.5
>

Re: [PATCH v8 4/5] pwm: kona: Add debug info to config function

[Jonathan Richardson]

On 15-05-30 09:42 AM, Tim Kryger wrote:
> On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> Adds debugging info to config function where duty cycle and period
>> are calculated and verified.
>>
>> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
>> ---
>>  drivers/pwm/pwm-bcm-kona.c |   25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>> index c87621f..0ddf19b 100644
>> --- a/drivers/pwm/pwm-bcm-kona.c
>> +++ b/drivers/pwm/pwm-bcm-kona.c
>> @@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>                 dc = div64_u64(val, div);
>>
>>                 /* If duty_ns or period_ns are not achievable then return */
>> -               if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
> 
> The original code was based on the SPEAr PWM driver which has a non-zero
> PWMDCR_MIN_DUTY such that the second condition here can evaluate to true.
> 
> This isn't the case for the Kona PWM where DUTY_CYCLE_HIGH_MIN is zero.
> 
>> +               if (pc < PERIOD_COUNT_MIN) {
>> +                       dev_warn(chip->dev,
>> +                               "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
>> +                               __func__, chan, period_ns, pc, prescale);
>>                         return -EINVAL;
>> +               }
> 
> Why not just print the minimum allowable period with the provided clock?
> 
> I don't think pc and prescale will be particularly helpful to users.
> 
> Also, do we really need to print __func__ here?
> 
>> +
>> +               if (dc < DUTY_CYCLE_HIGH_MIN) {
>> +                       if (0 != duty_ns) {
>> +                               dev_warn(chip->dev,
>> +                                       "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
>> +                                       __func__, chan, duty_ns, dc, prescale);
>> +                       }
>> +                       return -EINVAL;
>> +               }
> 
> The above block is unreachable code.
> 
>>
>>                 /* If pc and dc are in bounds, the calculation is done */
>>                 if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
>>                         break;
>>
>>                 /* Otherwise, increase prescale and recalculate pc and dc */
>> -               if (++prescale > PRESCALE_MAX)
>> +               if (++prescale > PRESCALE_MAX) {
>> +                       dev_warn(chip->dev,
>> +                               "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
>> +                               __func__, chan, prescale, PRESCALE_MAX,
>> +                               period_ns, duty_ns);
>>                         return -EINVAL;
>> +               }
>>         }
> 
> The user got here because they specified a period larger than the maximum
> supported so why not tell them largest value that can be supported instead
> of confusing them with prescale and PRESCALE_MAX?
> 
>>
>> +       dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
>> +               chan, pc, dc, prescale);
>> +
> 
> This could be more clear.  It prints pc but calls it period.
> 
>>         /*
>>          * Don't apply settings if disabled. The period and duty cycle are
>>          * always calculated above to ensure the new values are
>> --
>> 1.7.9.5
>>

We can defer this for now until I can look into it further. It's not a
priority. I'm more concerned with core changes and kona pwm fix.

Thanks,
Jon

[PATCH v8 5/5] 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 |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ 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;
+
+	if (!pwm)
+		return -EINVAL;
+
+	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);
+			return err;
+		}
+	}
 
-	return pwm ? 0 : -EINVAL;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
 
-- 
1.7.9.5

[PATCH v8 5/5] 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 |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ 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;
+
+	if (!pwm)
+		return -EINVAL;
+
+	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);
+			return err;
+		}
+	}
 
-	return pwm ? 0 : -EINVAL;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
 
-- 
1.7.9.5

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

[Thierry Reding]

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

On Tue, May 26, 2015 at 01:08:20PM -0700, 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 |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 224645f..18f5ac4 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -477,10 +477,20 @@ 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;
> +
> +	if (!pwm)
> +		return -EINVAL;
> +
> +	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);
> +			return err;
> +		}
> +	}

I think with this new pattern we're now actually going to need a lock to
make sure pwm->flags doesn't change between the test_and_set_bit() and
clear_bit() calls.

Thierry

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

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

[Jonathan Richardson]

On 15-06-12 02:49 AM, Thierry Reding wrote:
> On Tue, May 26, 2015 at 01:08:20PM -0700, 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 |   16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 224645f..18f5ac4 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -477,10 +477,20 @@ 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;
>> +
>> +	if (!pwm)
>> +		return -EINVAL;
>> +
>> +	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);
>> +			return err;
>> +		}
>> +	}
> 
> I think with this new pattern we're now actually going to need a lock to
> make sure pwm->flags doesn't change between the test_and_set_bit() and
> clear_bit() calls.
> 
> Thierry
> 

Ok. I'll add a lock per pwm_device and re-send the patch.

Jon

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

[Jonathan Richardson]

On 15-06-12 02:49 AM, Thierry Reding wrote:
> On Tue, May 26, 2015 at 01:08:20PM -0700, 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 |   16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 224645f..18f5ac4 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -477,10 +477,20 @@ 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;
>> +
>> +	if (!pwm)
>> +		return -EINVAL;
>> +
>> +	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);
>> +			return err;
>> +		}
>> +	}
> 
> I think with this new pattern we're now actually going to need a lock to
> make sure pwm->flags doesn't change between the test_and_set_bit() and
> clear_bit() calls.
> 
> Thierry
> 

Ok. I'll add a lock per pwm_device and re-send the patch.

Jon