[PATCH v4 0/2] pwm-pca9685: Bugfixes and prescaler support

[PATCH v4 0/2] pwm-pca9685: Bugfixes and prescaler support

[Clemens Gruber]

Hi,

the first patch from this series contains bugfixes and the second patch
adds support for changing the PWM output frequency of the PCA9685.

Regards,
Clemens

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Changes in v2:
- Only put chip into sleep mode if the bounds checking succeeds

Changes in v3:
- Fix checkpatch style warnings introduced in v2

Changes in v4:
- Set RESTART bit. Now the duty cycle to period ratio stays the same
  after only changing the period.
- Added a comment at the start of the file pointing out that the chip
  has only one prescaler and changing the period affects all outputs.

Clemens Gruber (2):
  pwm-pca9685: Fix several driver bugs
  pwm-pca9685: Support changing the output frequency

 drivers/pwm/pwm-pca9685.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

-- 
2.4.6

[PATCH v4 1/2] pwm-pca9685: Fix several driver bugs

[Clemens Gruber]

Problems:
- When duty_ns == period_ns, the full OFF bit was not cleared and the
  PWM output of the PCA9685 stayed off.
- When duty_ns == period_ns and the catch-all channel was used, the
  ALL_LED_OFF_L register was not cleared.
- The full ON bit was not cleared when setting the OFF time, therefore
  the exact OFF time was ignored when setting a duty_ns < period_ns

Solution: Clear both OFF registers when setting full ON and clear the
full ON bit when changing the OFF registers.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 34b5c27..f4a9c4a 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -85,6 +85,22 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	if (duty_ns == period_ns) {
+		/* Clear both OFF registers */
+		if (pwm->hwpwm >= PCA9685_MAXCHAN)
+			reg = PCA9685_ALL_LED_OFF_L;
+		else
+			reg = LED_N_OFF_L(pwm->hwpwm);
+
+		regmap_write(pca->regmap, reg, 0x0);
+
+		if (pwm->hwpwm >= PCA9685_MAXCHAN)
+			reg = PCA9685_ALL_LED_OFF_H;
+		else
+			reg = LED_N_OFF_H(pwm->hwpwm);
+
+		regmap_write(pca->regmap, reg, 0x0);
+
+		/* Set the full ON bit */
 		if (pwm->hwpwm >= PCA9685_MAXCHAN)
 			reg = PCA9685_ALL_LED_ON_H;
 		else
@@ -112,6 +128,14 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
 
+	/* Clear the full ON bit, otherwise the set OFF time has no effect */
+	if (pwm->hwpwm >= PCA9685_MAXCHAN)
+		reg = PCA9685_ALL_LED_ON_H;
+	else
+		reg = LED_N_ON_H(pwm->hwpwm);
+
+	regmap_write(pca->regmap, reg, 0);
+
 	return 0;
 }
 
-- 
2.4.6

[PATCH v4 2/2] pwm-pca9685: Support changing the output frequency

[Clemens Gruber]

Previously, period_ns and duty_ns were only used to determine the
ratio of ON and OFF time, the default frequency of 200 Hz was never
changed.
The PCA9685 however is capable of changing the PWM output frequency,
which is expected when changing the period.

This patch configures the prescaler accordingly, using the formula
and notes provided in the PCA9685 datasheet.
Bounds checking for the minimum and maximum frequencies, last updated
in revision v.4 of said datasheet, is also added.

The prescaler is only touched if the period changed, because we have to
put the chip into sleep mode to unlock the prescale register.
If it is changed, the PWM output frequency changes for all outputs,
because there is one prescaler per chip. This is documented in the
PCA9685 datasheet and in the comments.

If the duty cycle is not changed at the same time as the period, then
we restart the PWM output using the duty cycle to period ratio from
before the period change.
When using LEDs for example, previously set brightness levels stay the
same when the frequency changes.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index f4a9c4a..895fa16 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -2,6 +2,7 @@
  * Driver for PCA9685 16-channel 12-bit PWM LED controller
  *
  * Copyright (C) 2013 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ * Copyright (C) 2015 Clemens Gruber <clemens.gruber@pqgruber.com>
  *
  * based on the pwm-twl-led.c driver
  *
@@ -24,6 +25,15 @@
 #include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
+
+/*
+ * Because the PCA9685 has only one prescaler per chip, changing the period of
+ * one channel affects the period of all 16 PWM outputs!
+ * However, the ratio between each configured duty cycle and the chip-wide
+ * period remains constant, because the OFF time is set in proportion to the
+ * counter range.
+ */
 
 #define PCA9685_MODE1		0x00
 #define PCA9685_MODE2		0x01
@@ -42,10 +52,18 @@
 #define PCA9685_ALL_LED_OFF_H	0xFD
 #define PCA9685_PRESCALE	0xFE
 
+#define PCA9685_PRESCALE_MIN	0x03	/* => max. frequency of 1526 Hz */
+#define PCA9685_PRESCALE_MAX	0xFF	/* => min. frequency of 24 Hz */
+
+#define PCA9685_COUNTER_RANGE	4096
+#define PCA9685_DEFAULT_PERIOD	5000000	/* Default period_ns = 1/200 Hz */
+#define PCA9685_OSC_CLOCK_MHZ	25	/* Internal oscillator with 25 MHz */
+
 #define PCA9685_NUMREGS		0xFF
 #define PCA9685_MAXCHAN		0x10
 
 #define LED_FULL		(1 << 4)
+#define MODE1_RESTART		(1 << 7)
 #define MODE1_SLEEP		(1 << 4)
 #define MODE2_INVRT		(1 << 4)
 #define MODE2_OUTDRV		(1 << 2)
@@ -59,6 +77,8 @@ struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
 	int active_cnt;
+	int duty_ns;
+	int period_ns;
 };
 
 static inline struct pca9685 *to_pca(struct pwm_chip *chip)
@@ -72,6 +92,47 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pca9685 *pca = to_pca(chip);
 	unsigned long long duty;
 	unsigned int reg;
+	int prescale;
+
+	if (period_ns != pca->period_ns) {
+		prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
+					     PCA9685_COUNTER_RANGE * 1000) - 1;
+
+		if (prescale >= PCA9685_PRESCALE_MIN &&
+			prescale <= PCA9685_PRESCALE_MAX) {
+			/* Put chip into sleep mode */
+			regmap_update_bits(pca->regmap, PCA9685_MODE1,
+					   MODE1_SLEEP, MODE1_SLEEP);
+
+			/* Change the chip-wide output frequency */
+			regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
+
+			/* Wake the chip up */
+			regmap_update_bits(pca->regmap, PCA9685_MODE1,
+					   MODE1_SLEEP, 0x0);
+
+			/* Wait 500us for the oscillator to be back up */
+			udelay(500);
+
+			pca->period_ns = period_ns;
+
+			/*
+			 * If the duty cycle did not change, restart PWM with
+			 * the same duty cycle to period ratio and return.
+			 */
+			if (duty_ns == pca->duty_ns) {
+				regmap_update_bits(pca->regmap, PCA9685_MODE1,
+						   MODE1_RESTART, 0x1);
+				return 0;
+			}
+		} else {
+			dev_err(chip->dev,
+				"prescaler not set: period out of bounds!\n");
+			return -EINVAL;
+		}
+	}
+
+	pca->duty_ns = duty_ns;
 
 	if (duty_ns < 1) {
 		if (pwm->hwpwm >= PCA9685_MAXCHAN)
@@ -111,7 +172,7 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	duty = 4096 * (unsigned long long)duty_ns;
+	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
 	duty = DIV_ROUND_UP_ULL(duty, period_ns);
 
 	if (pwm->hwpwm >= PCA9685_MAXCHAN)
@@ -252,6 +313,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
+	pca->duty_ns = 0;
+	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
 
-- 
2.4.6

Re: [PATCH v4 0/2] pwm-pca9685: Bugfixes and prescaler support

[Clemens Gruber]

On Thu, Jul 23, 2015 at 05:19:00PM +0200, Clemens Gruber wrote:
> Hi,
> 
> the first patch from this series contains bugfixes and the second patch
> adds support for changing the PWM output frequency of the PCA9685.
> 
> Regards,
> Clemens
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> 
> Changes in v2:
> - Only put chip into sleep mode if the bounds checking succeeds
> 
> Changes in v3:
> - Fix checkpatch style warnings introduced in v2
> 
> Changes in v4:
> - Set RESTART bit. Now the duty cycle to period ratio stays the same
>   after only changing the period.
> - Added a comment at the start of the file pointing out that the chip
>   has only one prescaler and changing the period affects all outputs.
> 
> Clemens Gruber (2):
>   pwm-pca9685: Fix several driver bugs
>   pwm-pca9685: Support changing the output frequency
> 
>  drivers/pwm/pwm-pca9685.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)
> 
> -- 
> 2.4.6
> 

Hi Thierry,

I addressed the issue you raised about keeping the duty cycle to period ratio
when changing the period. This is achieved in v4 by setting the RESTART bit,
which resumes the PWM output with the same duty_cycle to period ratio as before
but with the new period / prescaler value. I also added a comment at the
beginning of the file to remind implementers, that this driver has a chip-wide
prescaler.

Without those two patches, the driver is really broken. See my description in
patch 1/2 for a list of its bugs.

Could we please get those two patches into 4.3?

Thanks,
Clemens

Re: [PATCH v4 0/2] pwm-pca9685: Bugfixes and prescaler support

[Thierry Reding]

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

On Thu, Jul 23, 2015 at 05:19:00PM +0200, Clemens Gruber wrote:
> Hi,
> 
> the first patch from this series contains bugfixes and the second patch
> adds support for changing the PWM output frequency of the PCA9685.
> 
> Regards,
> Clemens
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> 
> Changes in v2:
> - Only put chip into sleep mode if the bounds checking succeeds
> 
> Changes in v3:
> - Fix checkpatch style warnings introduced in v2
> 
> Changes in v4:
> - Set RESTART bit. Now the duty cycle to period ratio stays the same
>   after only changing the period.
> - Added a comment at the start of the file pointing out that the chip
>   has only one prescaler and changing the period affects all outputs.
> 
> Clemens Gruber (2):
>   pwm-pca9685: Fix several driver bugs
>   pwm-pca9685: Support changing the output frequency
> 
>  drivers/pwm/pwm-pca9685.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)

Applied, thanks.

Thierry

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