Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
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 Intel Finland Oy
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
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 > 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.
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
> 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.
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
On Thu, Apr 20, 2017 at 5:12 PM, Sven Van Asbroeck 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
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
> 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.
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
On Tue, Apr 18, 2017 at 6:52 PM, Sven Van Asbroeck 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
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
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.
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
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 ?
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
+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
Intel Finland Oy
Re: [PATCH v4 1/1] pwm: pca9685: fix gpio-only operation.
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
> Signed-off-by: Sven Van Asbroeck
> Suggested-by: Mika Westerberg
> Reviewed-by: Mika Westerberg
> ---
> drivers/pwm/pwm-pca9685.c | 112
> --
> 1 file changed, 79 insertions(+), 33 deletions(-)
Applied with s/gpio/GPIO/ and s/pwm/PWM/.
Thanks,
Thierry
signature.asc
Description: PGP signature
