* [PATCH 0/1] pwm: pca9685: fix gpio-only operation. @ 2017-04-09 16:11 Sven Van Asbroeck 2017-04-09 16:11 ` [PATCH 1/1] " Sven Van Asbroeck 0 siblings, 1 reply; 6+ messages in thread From: Sven Van Asbroeck @ 2017-04-09 16:11 UTC (permalink / raw) To: thierry.reding Cc: linux-pwm, linux-kernel, clemens.gruber, mika.westerberg, andriy.shevchenko 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-09 16:11 [PATCH 0/1] pwm: pca9685: fix gpio-only operation Sven Van Asbroeck @ 2017-04-09 16:11 ` Sven Van Asbroeck 2017-04-10 10:33 ` Mika Westerberg 0 siblings, 1 reply; 6+ messages in thread From: Sven Van Asbroeck @ 2017-04-09 16:11 UTC (permalink / raw) To: thierry.reding Cc: linux-pwm, linux-kernel, clemens.gruber, mika.westerberg, andriy.shevchenko, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-09 16:11 ` [PATCH 1/1] " Sven Van Asbroeck @ 2017-04-10 10:33 ` Mika Westerberg 2017-04-10 16:08 ` Sven Van Asbroeck 2017-04-11 3:35 ` Sven Van Asbroeck 0 siblings, 2 replies; 6+ messages in thread From: Mika Westerberg @ 2017-04-10 10:33 UTC (permalink / raw) To: Sven Van Asbroeck Cc: thierry.reding, linux-pwm, linux-kernel, clemens.gruber, andriy.shevchenko, Sven Van Asbroeck 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-10 10:33 ` Mika Westerberg @ 2017-04-10 16:08 ` Sven Van Asbroeck 2017-04-11 3:35 ` Sven Van Asbroeck 1 sibling, 0 replies; 6+ messages in thread From: Sven Van Asbroeck @ 2017-04-10 16:08 UTC (permalink / raw) To: Mika Westerberg Cc: Thierry Reding, linux-pwm, linux-kernel, clemens.gruber, andriy.shevchenko 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-10 10:33 ` Mika Westerberg 2017-04-10 16:08 ` Sven Van Asbroeck @ 2017-04-11 3:35 ` Sven Van Asbroeck 2017-04-11 9:11 ` Mika Westerberg 1 sibling, 1 reply; 6+ messages in thread From: Sven Van Asbroeck @ 2017-04-11 3:35 UTC (permalink / raw) To: Mika Westerberg Cc: Thierry Reding, linux-pwm, linux-kernel, clemens.gruber, andriy.shevchenko, 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-11 3:35 ` Sven Van Asbroeck @ 2017-04-11 9:11 ` Mika Westerberg 0 siblings, 0 replies; 6+ messages in thread From: Mika Westerberg @ 2017-04-11 9:11 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Thierry Reding, linux-pwm, linux-kernel, clemens.gruber, andriy.shevchenko, Sven Van Asbroeck 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-11 9:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-09 16:11 [PATCH 0/1] pwm: pca9685: fix gpio-only operation Sven Van Asbroeck 2017-04-09 16:11 ` [PATCH 1/1] " Sven Van Asbroeck 2017-04-10 10:33 ` Mika Westerberg 2017-04-10 16:08 ` Sven Van Asbroeck 2017-04-11 3:35 ` Sven Van Asbroeck 2017-04-11 9:11 ` Mika Westerberg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.