[PATCH 0/1] pwm: pca9685: fix gpio-only operation.

[PATCH 0/1] pwm: pca9685: fix gpio-only operation.

[Sven Van Asbroeck]

The solution to the bug presented here (always keep SLEEP cleared)
works for me, but perhaps other users might want to set SLEEP at
some point while the driver instance is still loaded ?

If so, I'd love to hear feedback, so we can find a solution which
works for all users.

Sven Van Asbroeck (1):
  pwm: pca9685: fix gpio-only operation.

 drivers/pwm/pwm-pca9685.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 1/1] pwm: pca9685: fix gpio-only operation.

[Sven Van Asbroeck]

gpio-only driver operation never clears the SLEEP bit, which can
cause the gpios to become unusable.

Example:
1. user requests first pwm  ->      driver clears SLEEP bit
2. user frees last pwm      ->      driver sets SLEEP bit
3. user requests gpio
4. user switches gpio on    ->      output does not turn on
                                    because SLEEP bit is set

Prevent this behaviour by keeping the SLEEP bit cleared
for the lifetime of the driver instance.

Fixes: bccec89f0a35 ("Allow any of the 16 PWMs to be used as a GPIO")
Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
---
 drivers/pwm/pwm-pca9685.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 0cfb357..4b6adaf 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -79,7 +79,6 @@
 struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
-	int active_cnt;
 	int duty_ns;
 	int period_ns;
 #if IS_ENABLED(CONFIG_GPIOLIB)
@@ -407,28 +406,14 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (pca9685_pwm_is_gpio(pca, pwm))
 		return -EBUSY;
 
-	if (pca->active_cnt++ == 0)
-		return regmap_update_bits(pca->regmap, PCA9685_MODE1,
-					  MODE1_SLEEP, 0x0);
-
 	return 0;
 }
 
-static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pca9685 *pca = to_pca(chip);
-
-	if (--pca->active_cnt == 0)
-		regmap_update_bits(pca->regmap, PCA9685_MODE1, MODE1_SLEEP,
-				   MODE1_SLEEP);
-}
-
 static const struct pwm_ops pca9685_pwm_ops = {
 	.enable = pca9685_pwm_enable,
 	.disable = pca9685_pwm_disable,
 	.config = pca9685_pwm_config,
 	.request = pca9685_pwm_request,
-	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,
 };
 
@@ -479,6 +464,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	/* clear all "full off" bits */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
+	/* clear the "sleep" bit */
+	regmap_update_bits(pca->regmap, PCA9685_MODE1,
+					  MODE1_SLEEP, 0x0);
 
 	pca->chip.ops = &pca9685_pwm_ops;
 	/* add an extra channel for ALL_LED */
-- 
1.9.1

Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation.

[Mika Westerberg]

On Sun, Apr 09, 2017 at 12:11:24PM -0400, Sven Van Asbroeck wrote:
> gpio-only driver operation never clears the SLEEP bit, which can
> cause the gpios to become unusable.
> 
> Example:
> 1. user requests first pwm  ->      driver clears SLEEP bit
> 2. user frees last pwm      ->      driver sets SLEEP bit
> 3. user requests gpio
> 4. user switches gpio on    ->      output does not turn on
>                                     because SLEEP bit is set
> 
> Prevent this behaviour by keeping the SLEEP bit cleared
> for the lifetime of the driver instance.
> 
> Fixes: bccec89f0a35 ("Allow any of the 16 PWMs to be used as a GPIO")
> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>

How about implementing a real runtime PM in the driver? Then when the
device is idle regardless of whether it is GPIO or PWM, the SLEEP bit is
set and cleared accordingly.

Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation.

[Sven Van Asbroeck]

Thanks for the feedback Mika, I really appreciate it.

> How about implementing a real runtime PM in the driver? Then when the
> device is idle regardless of whether it is GPIO or PWM, the SLEEP bit is
> set and cleared accordingly.

The current code will keep driving pwms/leds even when these are
currently not exported in sysfs. Until the last export disappears.
That behaviour feels counter-intuitive to me.

Example:
1. export pwm1 and pwm2
    echo 1 > export
    echo 2 > export
2. configure/enable pwm1 and pwm2
3. enjoy the blinkenlights on your board
4. unexport pwm1
    echo 1 > unexport
    both pwms are still driven !
5. unexport pwm2
    echo 2 > unexport
    both pwms shut down !
6. export pwm1
echo 1 > export
both pwms come back on !

Do we want to get rid of this behaviour? I see two intuitive alternatives:

a) keep SLEEP off all the time, so LEDs are still driven even when no
pwms/gpios are exported.

Pro: simple, predictable
Con: not power efficient

b) when a pwm/gpio is unexported, turn off the LED. when all pwms/gpios
are unexported, set the SLEEP bit.

Pro: power efficient
Con: output turns off when gpio/pwm is unexported, this may surprise users

Sven

Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation.

[Sven Van Asbroeck]

> How about implementing a real runtime PM in the driver? Then when the
> device is idle regardless of whether it is GPIO or PWM, the SLEEP bit is
> set and cleared accordingly.

You mean: increase the runtime_pm refcnt when a pwm/gpio is enabled,
and vice versa ? And don't touch the refcnt on pwm/gpio export/unexport?

Neat. I will test such a solution and try to post it within the next few days.

Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation.

[Mika Westerberg]

On Mon, Apr 10, 2017 at 11:35:39PM -0400, Sven Van Asbroeck wrote:
> > How about implementing a real runtime PM in the driver? Then when the
> > device is idle regardless of whether it is GPIO or PWM, the SLEEP bit is
> > set and cleared accordingly.
> 
> You mean: increase the runtime_pm refcnt when a pwm/gpio is enabled,
> and vice versa ? And don't touch the refcnt on pwm/gpio export/unexport?

I mean whenever the thing is in use, it is in runtime PM active state.
Then when the last user closes the device, it goest to runtime PM
suspend state where you effectively set that SLEEP bit. The functions
for refcounting are called pm_runtime_get* and pm_runtime_put*.

You need to implement runtime PM callbacks for the driver as well.

See Documentation/power/runtime_pm.txt for more information.