Re: [Intel-gfx] [PATCH v5 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper
Hi,
On 7/28/20 8:45 PM, Andy Shevchenko wrote:
On Fri, Jul 17, 2020 at 03:37:42PM +0200, Hans de Goede wrote:
In the not-enabled -> enabled path pwm_lpss_apply() needs to get a
runtime-pm reference; and then on any errors it needs to release it
again.
This leads to somewhat hard to read code. This commit introduces a new
pwm_lpss_prepare_enable() helper and moves all the steps necessary for
the not-enabled -> enabled transition there, so that we can error check
the entire transition in a single place and only have one pm_runtime_put()
on failure call site.
While working on this I noticed that the enabled -> enabled (update
settings) path was quite similar, so I've added an enable parameter to
the new pwm_lpss_prepare_enable() helper, which allows using it in that
path too.
Reviewed-by: Andy Shevchenko
But see below.
Suggested-by: Andy Shevchenko
Signed-off-by: Hans de Goede
---
drivers/pwm/pwm-lpss.c | 45 --
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index da9bc3d10104..8a136ba2a583 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -122,41 +122,48 @@ static inline void pwm_lpss_cond_enable(struct pwm_device
*pwm, bool cond)
pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
}
+static int pwm_lpss_prepare_enable(struct pwm_lpss_chip *lpwm,
+ struct pwm_device *pwm,
+ const struct pwm_state *state,
+ bool enable)
+{
+ int ret;
+
+ ret = pwm_lpss_is_updating(pwm);
+ if (ret)
+ return ret;
+
+ pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
+ pwm_lpss_cond_enable(pwm, enable && lpwm->info->bypass == false);
+ ret = pwm_lpss_wait_for_update(pwm);
+ if (ret)
+ return ret;
+
+ pwm_lpss_cond_enable(pwm, enable && lpwm->info->bypass == true);
+ return 0;
+}
+
static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
struct pwm_lpss_chip *lpwm = to_lpwm(chip);
- int ret;
+ int ret = 0;
We can avoid this change...
if (state->enabled) {
if (!pwm_is_enabled(pwm)) {
pm_runtime_get_sync(chip->dev);
- ret = pwm_lpss_is_updating(pwm);
- if (ret) {
- pm_runtime_put(chip->dev);
- return ret;
- }
- pwm_lpss_prepare(lpwm, pwm, state->duty_cycle,
state->period);
- pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
- ret = pwm_lpss_wait_for_update(pwm);
- if (ret) {
+ ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true);
+ if (ret)
pm_runtime_put(chip->dev);
- return ret;
- }
- pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
} else {
- ret = pwm_lpss_is_updating(pwm);
- if (ret)
- return ret;
- pwm_lpss_prepare(lpwm, pwm, state->duty_cycle,
state->period);
- return pwm_lpss_wait_for_update(pwm);
+ ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false);
...by simple return directly from here. But I admit I haven't seen the next
patch yet.
True, but I'm not a big fan of earlier returns except for errors.
Regards,
Hans
}
} else if (pwm_is_enabled(pwm)) {
pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
pm_runtime_put(chip->dev);
}
- return 0;
+ return ret;
}
static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
--
2.26.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper
On Fri, Jul 17, 2020 at 03:37:42PM +0200, Hans de Goede wrote:
> In the not-enabled -> enabled path pwm_lpss_apply() needs to get a
> runtime-pm reference; and then on any errors it needs to release it
> again.
>
> This leads to somewhat hard to read code. This commit introduces a new
> pwm_lpss_prepare_enable() helper and moves all the steps necessary for
> the not-enabled -> enabled transition there, so that we can error check
> the entire transition in a single place and only have one pm_runtime_put()
> on failure call site.
>
> While working on this I noticed that the enabled -> enabled (update
> settings) path was quite similar, so I've added an enable parameter to
> the new pwm_lpss_prepare_enable() helper, which allows using it in that
> path too.
Reviewed-by: Andy Shevchenko
But see below.
> Suggested-by: Andy Shevchenko
> Signed-off-by: Hans de Goede
> ---
> drivers/pwm/pwm-lpss.c | 45 --
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index da9bc3d10104..8a136ba2a583 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -122,41 +122,48 @@ static inline void pwm_lpss_cond_enable(struct
> pwm_device *pwm, bool cond)
> pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
> }
>
> +static int pwm_lpss_prepare_enable(struct pwm_lpss_chip *lpwm,
> +struct pwm_device *pwm,
> +const struct pwm_state *state,
> +bool enable)
> +{
> + int ret;
> +
> + ret = pwm_lpss_is_updating(pwm);
> + if (ret)
> + return ret;
> +
> + pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
> + pwm_lpss_cond_enable(pwm, enable && lpwm->info->bypass == false);
> + ret = pwm_lpss_wait_for_update(pwm);
> + if (ret)
> + return ret;
> +
> + pwm_lpss_cond_enable(pwm, enable && lpwm->info->bypass == true);
> + return 0;
> +}
> +
> static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> - int ret;
> + int ret = 0;
We can avoid this change...
> if (state->enabled) {
> if (!pwm_is_enabled(pwm)) {
> pm_runtime_get_sync(chip->dev);
> - ret = pwm_lpss_is_updating(pwm);
> - if (ret) {
> - pm_runtime_put(chip->dev);
> - return ret;
> - }
> - pwm_lpss_prepare(lpwm, pwm, state->duty_cycle,
> state->period);
> - pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
> - ret = pwm_lpss_wait_for_update(pwm);
> - if (ret) {
> + ret = pwm_lpss_prepare_enable(lpwm, pwm, state, true);
> + if (ret)
> pm_runtime_put(chip->dev);
> - return ret;
> - }
> - pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
> } else {
> - ret = pwm_lpss_is_updating(pwm);
> - if (ret)
> - return ret;
> - pwm_lpss_prepare(lpwm, pwm, state->duty_cycle,
> state->period);
> - return pwm_lpss_wait_for_update(pwm);
> + ret = pwm_lpss_prepare_enable(lpwm, pwm, state, false);
...by simple return directly from here. But I admit I haven't seen the next
patch yet.
> }
> } else if (pwm_is_enabled(pwm)) {
> pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
> pm_runtime_put(chip->dev);
> }
>
> - return 0;
> + return ret;
> }
>
> static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
