| Message ID | 1437381369-8502-2-git-send-email-clemens.gruber@pqgruber.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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; }
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(+)