[Intel-gfx] [PATCH v5 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper
Hans de Goede
hdegoede at redhat.com
Tue Jul 28 19:49:35 UTC 2020
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 <andriy.shevchenko at linux.intel.com>
> But see below.
>
>> Suggested-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede at 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.
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
>>
>
More information about the Intel-gfx
mailing list