* [PATCH v4 0/1] pwm: pca9685: fix gpio-only operation. @ 2017-04-13 12:58 Sven Van Asbroeck 2017-04-13 12:58 ` [PATCH v4 1/1] " Sven Van Asbroeck 0 siblings, 1 reply; 12+ messages in thread From: Sven Van Asbroeck @ 2017-04-13 12:58 UTC (permalink / raw) To: thierry.reding Cc: linux-pwm, linux-kernel, clemens.gruber, mika.westerberg, andriy.shevchenko v4: fix coding style for multi-line comment added Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> v3: remove unnecessary call to pm_runtime_suspend() fix coding style for multi-line comment (checkpatch.pl should ideally catch this, but did not?) v2: the pm_runtime framework controls the SLEEP bit, as suggested by Mika Westerberg. v1: the SLEEP bit is always on. Sven Van Asbroeck (1): pwm: pca9685: fix gpio-only operation. drivers/pwm/pwm-pca9685.c | 112 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 79 insertions(+), 33 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-13 12:58 [PATCH v4 0/1] pwm: pca9685: fix gpio-only operation Sven Van Asbroeck @ 2017-04-13 12:58 ` Sven Van Asbroeck 2017-04-13 15:34 ` Thierry Reding 2017-04-18 9:14 ` Andy Shevchenko 0 siblings, 2 replies; 12+ messages in thread From: Sven Van Asbroeck @ 2017-04-13 12:58 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 letting the runtime_pm framework control the SLEEP bit. This will put the chip to SLEEP if no pwms/gpios are exported/in use. Fixes: bccec89f0a35 ("Allow any of the 16 PWMs to be used as a GPIO") Reported-by: Sven Van Asbroeck <TheSven73@googlemail.com> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pwm/pwm-pca9685.c | 112 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 79 insertions(+), 33 deletions(-) diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 0cfb357..5f55cfa 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -30,6 +30,7 @@ #include <linux/regmap.h> #include <linux/slab.h> #include <linux/delay.h> +#include <linux/pm_runtime.h> /* * Because the PCA9685 has only one prescaler per chip, changing the period of @@ -79,7 +80,6 @@ struct pca9685 { struct pwm_chip chip; struct regmap *regmap; - int active_cnt; int duty_ns; int period_ns; #if IS_ENABLED(CONFIG_GPIOLIB) @@ -111,20 +111,10 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset) pwm_set_chip_data(pwm, (void *)1); mutex_unlock(&pca->lock); + pm_runtime_get_sync(pca->chip.dev); return 0; } -static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset) -{ - struct pca9685 *pca = gpiochip_get_data(gpio); - struct pwm_device *pwm; - - mutex_lock(&pca->lock); - pwm = &pca->chip.pwms[offset]; - pwm_set_chip_data(pwm, NULL); - mutex_unlock(&pca->lock); -} - static bool pca9685_pwm_is_gpio(struct pca9685 *pca, struct pwm_device *pwm) { bool is_gpio = false; @@ -177,6 +167,19 @@ static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset, regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on); } +static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset) +{ + struct pca9685 *pca = gpiochip_get_data(gpio); + struct pwm_device *pwm; + + pca9685_pwm_gpio_set(gpio, offset, 0); + pm_runtime_put(pca->chip.dev); + mutex_lock(&pca->lock); + pwm = &pca->chip.pwms[offset]; + pwm_set_chip_data(pwm, NULL); + mutex_unlock(&pca->lock); +} + static int pca9685_pwm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { @@ -238,6 +241,16 @@ static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca) } #endif +static void pca9685_set_sleep_mode(struct pca9685 *pca, int sleep) +{ + regmap_update_bits(pca->regmap, PCA9685_MODE1, + MODE1_SLEEP, sleep ? MODE1_SLEEP : 0); + if (!sleep) { + /* Wait 500us for the oscillator to be back up */ + udelay(500); + } +} + static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { @@ -252,19 +265,20 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, if (prescale >= PCA9685_PRESCALE_MIN && prescale <= PCA9685_PRESCALE_MAX) { + /* + * putting the chip briefly into SLEEP mode + * at this point won't interfere with the + * pm_runtime framework, because the pm_runtime + * state is guaranteed active here. + */ /* Put chip into sleep mode */ - regmap_update_bits(pca->regmap, PCA9685_MODE1, - MODE1_SLEEP, MODE1_SLEEP); + pca9685_set_sleep_mode(pca, 1); /* Change the chip-wide output frequency */ 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); + pca9685_set_sleep_mode(pca, 0); pca->period_ns = period_ns; } else { @@ -406,21 +420,15 @@ 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); + pm_runtime_get_sync(chip->dev); 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); + pca9685_pwm_disable(chip, pwm); + pm_runtime_put(chip->dev); } static const struct pwm_ops pca9685_pwm_ops = { @@ -492,22 +500,54 @@ static int pca9685_pwm_probe(struct i2c_client *client, return ret; ret = pca9685_pwm_gpio_probe(pca); - if (ret < 0) + if (ret < 0) { pwmchip_remove(&pca->chip); + return ret; + } + + /* the chip comes out of power-up in the active state */ + pm_runtime_set_active(&client->dev); + /* + * enable will put the chip into suspend, which is what we + * want as all outputs are disabled at this point + */ + pm_runtime_enable(&client->dev); - return ret; + return 0; } static int pca9685_pwm_remove(struct i2c_client *client) { struct pca9685 *pca = i2c_get_clientdata(client); + int ret; - regmap_update_bits(pca->regmap, PCA9685_MODE1, MODE1_SLEEP, - MODE1_SLEEP); + ret = pwmchip_remove(&pca->chip); + if (ret) + return ret; + pm_runtime_disable(&client->dev); + return 0; +} - return pwmchip_remove(&pca->chip); +#ifdef CONFIG_PM +static int pca9685_pwm_runtime_suspend(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct pca9685 *pca = i2c_get_clientdata(client); + + pca9685_set_sleep_mode(pca, 1); + return 0; } +static int pca9685_pwm_runtime_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct pca9685 *pca = i2c_get_clientdata(client); + + pca9685_set_sleep_mode(pca, 0); + return 0; +} +#endif + static const struct i2c_device_id pca9685_id[] = { { "pca9685", 0 }, { /* sentinel */ }, @@ -530,11 +570,17 @@ static int pca9685_pwm_remove(struct i2c_client *client) MODULE_DEVICE_TABLE(of, pca9685_dt_ids); #endif +static const struct dev_pm_ops pca9685_pwm_pm = { + SET_RUNTIME_PM_OPS(pca9685_pwm_runtime_suspend, + pca9685_pwm_runtime_resume, NULL) +}; + static struct i2c_driver pca9685_i2c_driver = { .driver = { .name = "pca9685-pwm", .acpi_match_table = ACPI_PTR(pca9685_acpi_ids), .of_match_table = of_match_ptr(pca9685_dt_ids), + .pm = &pca9685_pwm_pm, }, .probe = pca9685_pwm_probe, .remove = pca9685_pwm_remove, -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-13 12:58 ` [PATCH v4 1/1] " Sven Van Asbroeck @ 2017-04-13 15:34 ` Thierry Reding 2017-04-18 9:14 ` Andy Shevchenko 1 sibling, 0 replies; 12+ messages in thread From: Thierry Reding @ 2017-04-13 15:34 UTC (permalink / raw) To: Sven Van Asbroeck Cc: linux-pwm, linux-kernel, clemens.gruber, mika.westerberg, andriy.shevchenko, Sven Van Asbroeck [-- Attachment #1: Type: text/plain, Size: 1175 bytes --] On Thu, Apr 13, 2017 at 08:58:11AM -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 letting the runtime_pm framework > control the SLEEP bit. This will put the chip to SLEEP if > no pwms/gpios are exported/in use. > > Fixes: bccec89f0a35 ("Allow any of the 16 PWMs to be used as a GPIO") > Reported-by: Sven Van Asbroeck <TheSven73@googlemail.com> > Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pwm/pwm-pca9685.c | 112 ++++++++++++++++++++++++++++++++-------------- > 1 file changed, 79 insertions(+), 33 deletions(-) Applied with s/gpio/GPIO/ and s/pwm/PWM/. Thanks, Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-13 12:58 ` [PATCH v4 1/1] " Sven Van Asbroeck 2017-04-13 15:34 ` Thierry Reding @ 2017-04-18 9:14 ` Andy Shevchenko 2017-04-18 15:52 ` Sven Van Asbroeck 1 sibling, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2017-04-18 9:14 UTC (permalink / raw) To: Sven Van Asbroeck, thierry.reding, Rafael J. Wysocki Cc: linux-pwm, linux-kernel, clemens.gruber, mika.westerberg, Sven Van Asbroeck +Cc: Rafael (one question to you below) On Thu, 2017-04-13 at 08:58 -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 letting the runtime_pm framework > control the SLEEP bit. This will put the chip to SLEEP if > no pwms/gpios are exported/in use. > I know the patch is applied already, though please consider below to be addressed as usual (w/o Fixes tag). > +static void pca9685_set_sleep_mode(struct pca9685 *pca, int sleep) > +{ > + regmap_update_bits(pca->regmap, PCA9685_MODE1, > + MODE1_SLEEP, sleep ? MODE1_SLEEP : 0); > + if (!sleep) { > + /* Wait 500us for the oscillator to be back up */ > + udelay(500); > + } I would go with /* Wait for @sleep microseconds for the oscillator to be back up */ if (sleep) udelay(sleep); Otherwise int sleep is oddly here. Or bool sleep /* Wait 500us ... */ if (sleep) udelay(500); > +} > +#ifdef CONFIG_PM > +static int pca9685_pwm_runtime_suspend(struct device *dev) __maybe_unused and remove ugly #ifdef:ery. > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct pca9685 *pca = i2c_get_clientdata(client); > + > + pca9685_set_sleep_mode(pca, 1); > + return 0; > } > > +static int pca9685_pwm_runtime_resume(struct device *dev) Ditto. > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct pca9685 *pca = i2c_get_clientdata(client); > + > + pca9685_set_sleep_mode(pca, 0); > + return 0; > +} > +#endif > +static const struct dev_pm_ops pca9685_pwm_pm = { > + SET_RUNTIME_PM_OPS(pca9685_pwm_runtime_suspend, > + pca9685_pwm_runtime_resume, NULL) > +}; > + Perhaps we may introduce RUNTIME_DEV_PM_OPS() macro and re-use it here. Rafael? -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-18 9:14 ` Andy Shevchenko @ 2017-04-18 15:52 ` Sven Van Asbroeck 2017-04-19 20:24 ` Mika Westerberg 2017-04-20 7:29 ` Andy Shevchenko 0 siblings, 2 replies; 12+ messages in thread From: Sven Van Asbroeck @ 2017-04-18 15:52 UTC (permalink / raw) To: Andy Shevchenko Cc: Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, clemens.gruber, Mika Westerberg, Sven Van Asbroeck Thanks for the feedback Andy !! > I would go with > > /* Wait for @sleep microseconds for the oscillator to be back up */ > if (sleep) > udelay(sleep); > > Otherwise int sleep is oddly here. > > Or > > bool sleep > > /* Wait 500us ... */ > if (sleep) > udelay(500); > >> +} I think you may be getting confused between: - the chip's SLEEP bit (int sleep) - the amount of time to delay after chip comes _out of_ sleep. (always 500 us) If it's confusing for you, it might be confusing for others? Perhaps change the parameter to 'bool sleep_bit' or 'bool do_sleep' to make the distinction clearer? > __maybe_unused and remove ugly #ifdef:ery. If this works on non- CONFIG_PM systems, I'm all for it ! Grepping the drivers/ directory, I see that some drivers use #ifdef CONFIG_PM, some use __maybe_unused for runtime_pm. Mika and Thierry, thoughts ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-18 15:52 ` Sven Van Asbroeck @ 2017-04-19 20:24 ` Mika Westerberg 2017-04-20 7:29 ` Andy Shevchenko 1 sibling, 0 replies; 12+ messages in thread From: Mika Westerberg @ 2017-04-19 20:24 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Andy Shevchenko, Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, clemens.gruber, Sven Van Asbroeck On Tue, Apr 18, 2017 at 11:52:49AM -0400, Sven Van Asbroeck wrote: > > __maybe_unused and remove ugly #ifdef:ery. > > If this works on non- CONFIG_PM systems, I'm all for it ! > Grepping the drivers/ directory, I see that some drivers use > #ifdef CONFIG_PM, some use __maybe_unused for runtime_pm. > > Mika and Thierry, thoughts ? I actually prefer CONFIG_PM here but up to Thierry to decide, I guess. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-18 15:52 ` Sven Van Asbroeck 2017-04-19 20:24 ` Mika Westerberg @ 2017-04-20 7:29 ` Andy Shevchenko 2017-04-20 14:12 ` Sven Van Asbroeck 1 sibling, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2017-04-20 7:29 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Andy Shevchenko, Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, Clemens Gruber, Mika Westerberg, Sven Van Asbroeck On Tue, Apr 18, 2017 at 6:52 PM, Sven Van Asbroeck <thesven73@gmail.com> wrote: > Thanks for the feedback Andy !! You're welcome. > >> I would go with >> >> /* Wait for @sleep microseconds for the oscillator to be back up */ >> if (sleep) >> udelay(sleep); >> >> Otherwise int sleep is oddly here. >> >> Or >> >> bool sleep >> >> /* Wait 500us ... */ >> if (sleep) >> udelay(500); >> >>> +} > > I think you may be getting confused between: > - the chip's SLEEP bit (int sleep) > - the amount of time to delay after chip comes _out of_ sleep. > (always 500 us) > > If it's confusing for you, it might be confusing for others? > Perhaps change the parameter to 'bool sleep_bit' or 'bool do_sleep' > to make the distinction clearer? Taking above into consideration perhaps sleep is not quite good word at all. By functional description it sounds like latency tolerance to me. >> __maybe_unused and remove ugly #ifdef:ery. > > If this works on non- CONFIG_PM systems, I'm all for it ! > Grepping the drivers/ directory, I see that some drivers use > #ifdef CONFIG_PM, some use __maybe_unused for runtime_pm. This approach kinda new that's why you see variety of approaches. > Mika and Thierry, thoughts ? At the end it's Thierry's call, so, I'm not insisting. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-20 7:29 ` Andy Shevchenko @ 2017-04-20 14:12 ` Sven Van Asbroeck 2017-04-20 15:07 ` Andy Shevchenko 0 siblings, 1 reply; 12+ messages in thread From: Sven Van Asbroeck @ 2017-04-20 14:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, Mika Westerberg, Sven Van Asbroeck > Taking above into consideration perhaps sleep is not quite good word > at all. By functional description it sounds like latency tolerance to > me. That's true, but the bit description in the chip datasheet is 'SLEEP'. (its real function is suspend/low power, but the chip designers called it 'SLEEP') Calling the bit/function something else is likely to confuse someone who's reading the driver in combination with the chip datasheet. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-20 14:12 ` Sven Van Asbroeck @ 2017-04-20 15:07 ` Andy Shevchenko 2017-04-20 15:50 ` Sven Van Asbroeck 2017-04-20 15:55 ` Mika Westerberg 0 siblings, 2 replies; 12+ messages in thread From: Andy Shevchenko @ 2017-04-20 15:07 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Andy Shevchenko, Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, Mika Westerberg, Sven Van Asbroeck On Thu, Apr 20, 2017 at 5:12 PM, Sven Van Asbroeck <thesven73@gmail.com> wrote: >> Taking above into consideration perhaps sleep is not quite good word >> at all. By functional description it sounds like latency tolerance to >> me. > > That's true, but the bit description in the chip datasheet is 'SLEEP'. > (its real function is suspend/low power, but the chip designers called > it 'SLEEP') > > Calling the bit/function something else is likely to confuse someone > who's reading the driver in combination with the chip datasheet. Looking again into the patch I have noticed: 1) word 'sleep' is used as a part of a function name; 2) int sleep is used as binary value. Thus, I would suggest: int sleep -> bool enable (or alike). Would we agree on that? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-20 15:07 ` Andy Shevchenko @ 2017-04-20 15:50 ` Sven Van Asbroeck 2017-04-20 16:13 ` Andy Shevchenko 2017-04-20 15:55 ` Mika Westerberg 1 sibling, 1 reply; 12+ messages in thread From: Sven Van Asbroeck @ 2017-04-20 15:50 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, Mika Westerberg, Sven Van Asbroeck > Thus, I would suggest: int sleep -> bool enable (or alike). > > Would we agree on that? I would. Perhaps also: set_sleep_mode(int sleep) -> enable_sleep_mode(bool enable) ? Let's see what Mika and Thierry think. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-20 15:50 ` Sven Van Asbroeck @ 2017-04-20 16:13 ` Andy Shevchenko 0 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2017-04-20 16:13 UTC (permalink / raw) To: Sven Van Asbroeck, Andy Shevchenko Cc: Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, Mika Westerberg, Sven Van Asbroeck On Thu, 2017-04-20 at 11:50 -0400, Sven Van Asbroeck wrote: > > Thus, I would suggest: int sleep -> bool enable (or alike). > > > > Would we agree on that? > > I would. Perhaps also: > set_sleep_mode(int sleep) -> enable_sleep_mode(bool enable) ? I'm okay with a such (don't forget to change 0/1 in call sites to false/true as well). > Let's see what Mika and Thierry think. I suppose Mika's answer is an acknowledge to the change. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation. 2017-04-20 15:07 ` Andy Shevchenko 2017-04-20 15:50 ` Sven Van Asbroeck @ 2017-04-20 15:55 ` Mika Westerberg 1 sibling, 0 replies; 12+ messages in thread From: Mika Westerberg @ 2017-04-20 15:55 UTC (permalink / raw) To: Andy Shevchenko Cc: Sven Van Asbroeck, Andy Shevchenko, Thierry Reding, Rafael J. Wysocki, linux-pwm, linux-kernel, Sven Van Asbroeck On Thu, Apr 20, 2017 at 06:07:37PM +0300, Andy Shevchenko wrote: > On Thu, Apr 20, 2017 at 5:12 PM, Sven Van Asbroeck <thesven73@gmail.com> wrote: > >> Taking above into consideration perhaps sleep is not quite good word > >> at all. By functional description it sounds like latency tolerance to > >> me. > > > > That's true, but the bit description in the chip datasheet is 'SLEEP'. > > (its real function is suspend/low power, but the chip designers called > > it 'SLEEP') > > > > Calling the bit/function something else is likely to confuse someone > > who's reading the driver in combination with the chip datasheet. > > Looking again into the patch I have noticed: > 1) word 'sleep' is used as a part of a function name; > 2) int sleep is used as binary value. > > Thus, I would suggest: int sleep -> bool enable (or alike). > > Would we agree on that? That sounds good to me. I guess it will have to be an incremental patch since this one has already been applied. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-04-20 16:13 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-13 12:58 [PATCH v4 0/1] pwm: pca9685: fix gpio-only operation Sven Van Asbroeck 2017-04-13 12:58 ` [PATCH v4 1/1] " Sven Van Asbroeck 2017-04-13 15:34 ` Thierry Reding 2017-04-18 9:14 ` Andy Shevchenko 2017-04-18 15:52 ` Sven Van Asbroeck 2017-04-19 20:24 ` Mika Westerberg 2017-04-20 7:29 ` Andy Shevchenko 2017-04-20 14:12 ` Sven Van Asbroeck 2017-04-20 15:07 ` Andy Shevchenko 2017-04-20 15:50 ` Sven Van Asbroeck 2017-04-20 16:13 ` Andy Shevchenko 2017-04-20 15:55 ` Mika Westerberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).