* [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: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
* 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
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 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.