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.shevchenko@linux.intel.com But see below.
Suggested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Hans de Goede hdegoede@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);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