diff mbox series

[v5,05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper

Message ID 20200717133753.127282-6-hdegoede@redhat.com
State Changes Requested
Headers show
Series acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand

Commit Message

Hans de Goede July 17, 2020, 1:37 p.m. UTC
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.

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(-)

Comments

Andy Shevchenko July 28, 2020, 6:45 p.m. UTC | #1
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);
>  		} 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
>
Hans de Goede July 28, 2020, 7:49 p.m. UTC | #2
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@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);
>>   		} 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
>>
>
diff mbox series

Patch

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;
 
 	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);
 		}
 	} 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,