In order to change the pwm period, this chip must be put in sleep
mode. However, when coming out of sleep mode, the pwm channel
state is not completely restored: all pwm channels are off by
default.
This results in a bug in this driver: when the pwm period is changed
on a pwm channel, all other pwm channels will be deactivated.
Fix by clearing the RESTART bit when coming out of sleep mode - this
will restore all pwm channels to their pre-sleep on/off state.
Reported-by: Matthias Schiffer <[email protected]>
Cc: Matthias Schiffer <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Cc: Clemens Gruber <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
I no longer have access to pca9685 hardware, so I'm unable to test:
- if this is indeed a bug
- if this patch fixes it
Made against:
Tree-repo: git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git
Tree-branch: for-next
Tree-git-id: 9cc5f232a4b6a0ef6e9b57876d61b88f61bdd7c2
drivers/pwm/pwm-pca9685.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 76cd22bd6614..0a16f0122e0e 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -59,6 +59,7 @@
#define LED_FULL (1 << 4)
#define MODE1_SLEEP (1 << 4)
+#define MODE1_RESTART (1 << 7)
#define MODE2_INVRT (1 << 4)
#define MODE2_OUTDRV (1 << 2)
@@ -271,6 +272,15 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
/* Wake the chip up */
pca9685_set_sleep_mode(pca, false);
+ /* If any pwm channels were active when the chip was put
+ * in sleep mode, re-activate them.
+ */
+ if (!regmap_read(pca->regmap, PCA9685_MODE1, ®) &&
+ reg & MODE1_RESTART)
+ regmap_update_bits(pca->regmap, PCA9685_MODE1,
+ MODE1_RESTART,
+ MODE1_RESTART);
+
pca->period_ns = period_ns;
} else {
dev_err(chip->dev,
--
2.17.1
Hi,
On Fri, Apr 03, 2020 at 07:53:24PM -0400, Sven Van Asbroeck wrote:
> In order to change the pwm period, this chip must be put in sleep
> mode. However, when coming out of sleep mode, the pwm channel
> state is not completely restored: all pwm channels are off by
> default.
>
> This results in a bug in this driver: when the pwm period is changed
> on a pwm channel, all other pwm channels will be deactivated.
>
> Fix by clearing the RESTART bit when coming out of sleep mode - this
> will restore all pwm channels to their pre-sleep on/off state.
>
> Reported-by: Matthias Schiffer <[email protected]>
> Cc: Matthias Schiffer <[email protected]>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Cc: Clemens Gruber <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
>
> I no longer have access to pca9685 hardware, so I'm unable to test:
> - if this is indeed a bug
> - if this patch fixes it
>
> Made against:
> Tree-repo: git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git
> Tree-branch: for-next
> Tree-git-id: 9cc5f232a4b6a0ef6e9b57876d61b88f61bdd7c2
>
> drivers/pwm/pwm-pca9685.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 76cd22bd6614..0a16f0122e0e 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -59,6 +59,7 @@
>
> #define LED_FULL (1 << 4)
> #define MODE1_SLEEP (1 << 4)
> +#define MODE1_RESTART (1 << 7)
> #define MODE2_INVRT (1 << 4)
> #define MODE2_OUTDRV (1 << 2)
>
> @@ -271,6 +272,15 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> /* Wake the chip up */
> pca9685_set_sleep_mode(pca, false);
>
> + /* If any pwm channels were active when the chip was put
> + * in sleep mode, re-activate them.
> + */
> + if (!regmap_read(pca->regmap, PCA9685_MODE1, ®) &&
> + reg & MODE1_RESTART)
> + regmap_update_bits(pca->regmap, PCA9685_MODE1,
> + MODE1_RESTART,
> + MODE1_RESTART);
> +
> pca->period_ns = period_ns;
> } else {
> dev_err(chip->dev,
> --
> 2.17.1
>
According to the PCA9685 datasheet revision 4, page 15, the RESTART bit
is not only cleared by writing a 1 to it, but also by other actions like
a write to any of the PWM registers.
This seems to be the reason why I could not reproduce the reported
problem.
If I understand this correctly, clearing the RESTART bit would only be
necessary if we wanted every ON/OFF register to stay the same, but in
.config we might also get a different duty_ns value, so we have to
reprogram the ON/OFF time regs.
(Optimization: We could check if duty_ns to period_ns ratio stayed the
same and if so, clear the RESTART bit and return without reg writes)
Clemens
On Sat, Apr 4, 2020 at 1:58 PM Clemens Gruber
<[email protected]> wrote:
>
> According to the PCA9685 datasheet revision 4, page 15, the RESTART bit
> is not only cleared by writing a 1 to it, but also by other actions like
> a write to any of the PWM registers.
>
> This seems to be the reason why I could not reproduce the reported
> problem.
Thanks Clemens for checking this. If .config will always write at least one
of the pwm registers (which seems to be the case), then all pwm channels
should stay on after a period change. And we (fortunately) won't need this
patch.