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

[PATCH v3 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.

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

Changes from v1:
- Only put chip into sleep mode if the bounds checking succeeds

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

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

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

-- 
2.4.6

[PATCH v3 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

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

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 10:36:08AM +0200, Clemens Gruber wrote:
> 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);

Doesn't this undo the setting of this register back up in the duty_ns ==
period_ns conditional block?

Thierry

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

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

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 11:27:23AM +0200, Thierry Reding wrote:
> On Mon, Jul 20, 2015 at 10:36:08AM +0200, Clemens Gruber wrote:
> > 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);
> 
> Doesn't this undo the setting of this register back up in the duty_ns ==
> period_ns conditional block?

Nevermind, looking at the full driver code I see that the branch returns
0.

Thierry

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

[PATCH v3 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.

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 | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index f4a9c4a..26767d9 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,7 @@
 #include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
 
 #define PCA9685_MODE1		0x00
 #define PCA9685_MODE2		0x01
@@ -42,6 +44,13 @@
 #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
 
@@ -59,6 +68,7 @@ struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
 	int active_cnt;
+	int period_ns;
 };
 
 static inline struct pca9685 *to_pca(struct pwm_chip *chip)
@@ -72,6 +82,40 @@ 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) {
+		/*
+		 * Set prescale register according to the configured period,
+		 * which in turn changes the frequency of all 16 PWM outputs.
+		 */
+		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);
+
+			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;
+		} else {
+			dev_err(chip->dev,
+				"prescaler not set: period out of bounds\n");
+			return -EINVAL;
+		}
+
+	}
 
 	if (duty_ns < 1) {
 		if (pwm->hwpwm >= PCA9685_MAXCHAN)
@@ -111,7 +155,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 +296,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
+	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
 
-- 
2.4.6

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

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 10:36:09AM +0200, Clemens Gruber wrote:
> 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.

I think the reason why the driver doesn't support changing the frequency
is precisely because it has a per-chip prescaler. So you'd be changing
the frequency from other all other users if one of the channels requests
to do so. If I remember correctly there had been some discussion back at
the time that this wasn't safe to do.

If you have to do this, I'd think you'd need to at least update the duty
cycle of all other users according to the new period.

Thierry

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

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

[Clemens Gruber]

On Mon, Jul 20, 2015 at 11:30:22AM +0200, Thierry Reding wrote:
> On Mon, Jul 20, 2015 at 10:36:09AM +0200, Clemens Gruber wrote:
> > 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.
> 
> I think the reason why the driver doesn't support changing the frequency
> is precisely because it has a per-chip prescaler. So you'd be changing
> the frequency from other all other users if one of the channels requests
> to do so. If I remember correctly there had been some discussion back at
> the time that this wasn't safe to do.
> 
> If you have to do this, I'd think you'd need to at least update the duty
> cycle of all other users according to the new period.

Hi, thanks for your feedback.

Yes, that's true. You'd have to set the prescaler first and then manually set
all duty cycle values. Although this is a hardware feature, I can understand
that this might be confusing.

I will send a v4 to update the duty cycle of all other users after the chip
woke up again!
(Using the ratio from duty cycle to old period to set the new duty cycle)

Clemens

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

[Clemens Gruber]

On Mon, Jul 20, 2015 at 11:46:12AM +0200, Clemens Gruber wrote:
> On Mon, Jul 20, 2015 at 11:30:22AM +0200, Thierry Reding wrote:
> > On Mon, Jul 20, 2015 at 10:36:09AM +0200, Clemens Gruber wrote:
> > > 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.
> > 
> > I think the reason why the driver doesn't support changing the frequency
> > is precisely because it has a per-chip prescaler. So you'd be changing
> > the frequency from other all other users if one of the channels requests
> > to do so. If I remember correctly there had been some discussion back at
> > the time that this wasn't safe to do.
> > 
> > If you have to do this, I'd think you'd need to at least update the duty
> > cycle of all other users according to the new period.
> 
> Hi, thanks for your feedback.
> 
> Yes, that's true. You'd have to set the prescaler first and then manually set
> all duty cycle values. Although this is a hardware feature, I can understand
> that this might be confusing.
> 
> I will send a v4 to update the duty cycle of all other users after the chip
> woke up again!
> (Using the ratio from duty cycle to old period to set the new duty cycle)
> 
> Clemens

Do you think it would be a good idea to print something to the kernel log when
changing the prescaler frequency? At the moment I only print an error when it is
out of bounds.
To notify the users that the frequency of all channels changed and the duty
cycle values have been adjusted?

Clemens

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

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 11:50:55AM +0200, Clemens Gruber wrote:
> On Mon, Jul 20, 2015 at 11:46:12AM +0200, Clemens Gruber wrote:
> > On Mon, Jul 20, 2015 at 11:30:22AM +0200, Thierry Reding wrote:
> > > On Mon, Jul 20, 2015 at 10:36:09AM +0200, Clemens Gruber wrote:
> > > > 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.
> > > 
> > > I think the reason why the driver doesn't support changing the frequency
> > > is precisely because it has a per-chip prescaler. So you'd be changing
> > > the frequency from other all other users if one of the channels requests
> > > to do so. If I remember correctly there had been some discussion back at
> > > the time that this wasn't safe to do.
> > > 
> > > If you have to do this, I'd think you'd need to at least update the duty
> > > cycle of all other users according to the new period.
> > 
> > Hi, thanks for your feedback.
> > 
> > Yes, that's true. You'd have to set the prescaler first and then manually set
> > all duty cycle values. Although this is a hardware feature, I can understand
> > that this might be confusing.
> > 
> > I will send a v4 to update the duty cycle of all other users after the chip
> > woke up again!
> > (Using the ratio from duty cycle to old period to set the new duty cycle)
> > 
> > Clemens
> 
> Do you think it would be a good idea to print something to the kernel log when
> changing the prescaler frequency? At the moment I only print an error when it is
> out of bounds.
> To notify the users that the frequency of all channels changed and the duty
> cycle values have been adjusted?

None of the users would actually notice the kernel log entry, so I don't
think it makes much sense. One problem I see with this is that we might
start to see some sort of ping-pong happening between various users of
the PWMs of the same chip.

I guess it depends a bit on what the typical use-cases are for this
chip. Is it usually used only for LEDs (the register names suggest it)?
If so, how likely is it that people will want to drive them at different
periods? If it's reasonable to require the same period for all of them,
maybe it would be worth adding some sort of WARN_ON() or dev_warn() or
something to notify integrators of this fact.

Thierry

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

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

[Clemens Gruber]

On Mon, Jul 20, 2015 at 11:58:14AM +0200, Thierry Reding wrote:
> On Mon, Jul 20, 2015 at 11:50:55AM +0200, Clemens Gruber wrote:
> > Do you think it would be a good idea to print something to the kernel log when
> > changing the prescaler frequency? At the moment I only print an error when it is
> > out of bounds.
> > To notify the users that the frequency of all channels changed and the duty
> > cycle values have been adjusted?
> 
> None of the users would actually notice the kernel log entry, so I don't
> think it makes much sense. One problem I see with this is that we might
> start to see some sort of ping-pong happening between various users of
> the PWMs of the same chip.
> 
> I guess it depends a bit on what the typical use-cases are for this
> chip. Is it usually used only for LEDs (the register names suggest it)?
> If so, how likely is it that people will want to drive them at different
> periods? If it's reasonable to require the same period for all of them,
> maybe it would be worth adding some sort of WARN_ON() or dev_warn() or
> something to notify integrators of this fact.

The typical use-cases are LEDs but my specific use case is controlling fluid
valves. I need one frequency per chip and only the duty cycle is changed
regularly (to open or close the valves).
The current version not only forces the user to 200 Hz and of course also to
having the same frequency for all channels as this is a hardware feature.

What's interesting is that the ON and OFF registers are just values from 0 to
4095 and only specify the on and off time ratio, independent of the prescaler.
So I think I do not even have to change the duty cycles, if the user changes the
prescaler, the ratio remains.
I'll just have to set the RESTART bit after changing the prescaler, then it will
come back up with the ON and OFF time ratios from before.

Or did you mean, the driver should update the duty cycle values, which could be
read from the duty_cycle file in the pwm sysfs interface?

Clemens

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

[Clemens Gruber]

On Mon, Jul 20, 2015 at 11:58:14AM +0200, Thierry Reding wrote:
> I guess it depends a bit on what the typical use-cases are for this
> chip. Is it usually used only for LEDs (the register names suggest it)?
> If so, how likely is it that people will want to drive them at different
> periods? If it's reasonable to require the same period for all of them,
> maybe it would be worth adding some sort of WARN_ON() or dev_warn() or
> something to notify integrators of this fact.

Not likely. If they read the datasheet of the PCA9685, they should know that
this is not even possible with that chip.

On what condition would you WARN / dev_warn in this case? If the user tries to
set different prescaler values to different channels?

Clemens