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 <andriy.shevche...@linux.intel.com>
But see below.

> Suggested-by: Andy Shevchenko <andriy.shevche...@linux.intel.com>
> Signed-off-by: Hans de Goede <hdego...@redhat.com>
> ---
>  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

Reply via email to