[PATCH 0/2] pwm: lpss: Atomic PWM support for LPSS

[PATCH 0/2] pwm: lpss: Atomic PWM support for LPSS

[Hans de Goede]

Hi All,

This is an old series which got burried under a bunch of other stuff,
but now that I'm touching the LPSS pwm code anyways I thought I would
brush of the dust and submit it again.

I believe that this version addresses all the remarks of previous
reviews. Since it has been a long time I've lost track of which
version of the series this is, so I'm resetting the versioning of
the series to 1.

Regards,

Hans

[PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Hans de Goede]

Add a helper function to properly get / put the pwm's runtime_pm
reference when changing from enabled to disable or vice versa.

And use this to ensure the pwm's runtime_pm reference is properly
released on remove.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.c | 47 +++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index e602835fd6de..d39158800baa 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -120,43 +120,58 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
 		pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
 }
 
+static void pwm_lpss_get_put_runtime_pm(struct pwm_chip *chip,
+					bool old_enabled, bool new_enabled)
+{
+	if (new_enabled == old_enabled)
+		return;
+
+	if (new_enabled)
+		pm_runtime_get(chip->dev);
+	else
+		pm_runtime_put(chip->dev);
+}
+
 static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state)
 {
 	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
-	int ret;
+	int ret = 0;
+
+	pm_runtime_get_sync(chip->dev);
 
 	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;
-			}
+			if (ret)
+				goto out;
+
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
 			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
 			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
 			ret = pwm_lpss_wait_for_update(pwm);
-			if (ret) {
-				pm_runtime_put(chip->dev);
-				return ret;
-			}
+			if (ret)
+				goto out;
+
 			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
 		} else {
 			ret = pwm_lpss_is_updating(pwm);
 			if (ret)
-				return ret;
+				goto out;
+
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
 			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
-			return pwm_lpss_wait_for_update(pwm);
+			ret = pwm_lpss_wait_for_update(pwm);
 		}
 	} else if (pwm_is_enabled(pwm)) {
 		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
-		pm_runtime_put(chip->dev);
 	}
 
-	return 0;
+	pwm_lpss_get_put_runtime_pm(chip, pwm_is_enabled(pwm), state->enabled);
+
+out:
+	pm_runtime_put(chip->dev);
+	return ret;
 }
 
 static const struct pwm_ops pwm_lpss_ops = {
@@ -205,6 +220,10 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
 
 int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
 {
+	bool enabled = pwm_is_enabled(&lpwm->chip.pwms[0]);
+
+	pwm_lpss_get_put_runtime_pm(&lpwm->chip, enabled, false);
+
 	return pwmchip_remove(&lpwm->chip);
 }
 EXPORT_SYMBOL_GPL(pwm_lpss_remove);
-- 
2.19.0.rc0

[PATCH 2/2] pwm: lpss: Add get_state callback

[Hans de Goede]

Add a get_state callback so that the initial state correctly reflects
the actual hardware state.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/pwm/pwm-lpss.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index d39158800baa..882a71f1a66c 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -174,8 +174,41 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
+/* This function gets called once from pwmchip_add to get the initial state */
+static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+	unsigned long base_unit_range;
+	unsigned long long base_unit, freq, on_time_div;
+	u32 ctrl;
+
+	base_unit_range = BIT(lpwm->info->base_unit_bits);
+
+	ctrl = pwm_lpss_read(pwm);
+	on_time_div = 255 - (ctrl & PWM_ON_TIME_DIV_MASK);
+	base_unit = (ctrl >> PWM_BASE_UNIT_SHIFT) & (base_unit_range - 1);
+
+	freq = base_unit * lpwm->info->clk_rate;
+	do_div(freq, base_unit_range);
+	if (freq == 0)
+		state->period = NSEC_PER_SEC;
+	else
+		state->period = NSEC_PER_SEC / (unsigned long)freq;
+
+	on_time_div *= state->period;
+	do_div(on_time_div, 255);
+	state->duty_cycle = on_time_div;
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = !!(ctrl & PWM_ENABLE);
+
+	pwm_lpss_get_put_runtime_pm(chip, false, state->enabled);
+}
+
 static const struct pwm_ops pwm_lpss_ops = {
 	.apply = pwm_lpss_apply,
+	.get_state = pwm_lpss_get_state,
 	.owner = THIS_MODULE,
 };
 
-- 
2.19.0.rc0

Re: [PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Andy Shevchenko]

On Tue, Sep 11, 2018 at 07:45:32PM +0200, Hans de Goede wrote:
> Add a helper function to properly get / put the pwm's runtime_pm
> reference when changing from enabled to disable or vice versa.
> 
> And use this to ensure the pwm's runtime_pm reference is properly
> released on remove.
> 

Can you provide a test sequence to reproduce the issue?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss.c | 47 +++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index e602835fd6de..d39158800baa 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -120,43 +120,58 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
>  		pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
>  }
>  
> +static void pwm_lpss_get_put_runtime_pm(struct pwm_chip *chip,
> +					bool old_enabled, bool new_enabled)
> +{
> +	if (new_enabled == old_enabled)
> +		return;
> +
> +	if (new_enabled)
> +		pm_runtime_get(chip->dev);
> +	else
> +		pm_runtime_put(chip->dev);
> +}
> +
>  static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			  struct pwm_state *state)
>  {
>  	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> -	int ret;
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(chip->dev);
>  
>  	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;
> -			}
> +			if (ret)
> +				goto out;
> +
>  			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
>  			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
>  			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
>  			ret = pwm_lpss_wait_for_update(pwm);
> -			if (ret) {
> -				pm_runtime_put(chip->dev);
> -				return ret;
> -			}
> +			if (ret)
> +				goto out;
> +
>  			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
>  		} else {
>  			ret = pwm_lpss_is_updating(pwm);
>  			if (ret)
> -				return ret;
> +				goto out;
> +
>  			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
>  			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
> -			return pwm_lpss_wait_for_update(pwm);
> +			ret = pwm_lpss_wait_for_update(pwm);
>  		}
>  	} else if (pwm_is_enabled(pwm)) {
>  		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
> -		pm_runtime_put(chip->dev);
>  	}
>  
> -	return 0;
> +	pwm_lpss_get_put_runtime_pm(chip, pwm_is_enabled(pwm), state->enabled);
> +
> +out:
> +	pm_runtime_put(chip->dev);
> +	return ret;
>  }
>  
>  static const struct pwm_ops pwm_lpss_ops = {
> @@ -205,6 +220,10 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
>  
>  int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>  {
> +	bool enabled = pwm_is_enabled(&lpwm->chip.pwms[0]);
> +
> +	pwm_lpss_get_put_runtime_pm(&lpwm->chip, enabled, false);
> +
>  	return pwmchip_remove(&lpwm->chip);
>  }
>  EXPORT_SYMBOL_GPL(pwm_lpss_remove);
> -- 
> 2.19.0.rc0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Hans de Goede]

Hi,

On 24-09-18 11:16, Andy Shevchenko wrote:
> On Tue, Sep 11, 2018 at 07:45:32PM +0200, Hans de Goede wrote:
>> Add a helper function to properly get / put the pwm's runtime_pm
>> reference when changing from enabled to disable or vice versa.
>>
>> And use this to ensure the pwm's runtime_pm reference is properly
>> released on remove.
>>
> 
> Can you provide a test sequence to reproduce the issue?

This patch is mostly a preparation patch for adding the get_state
callback to make the driver fully support the pwm-atomic model.

When I first tried to upstream the get_state code about a year
ago, there was some not pretty code in there to deal with getting
the runtime pm reference if the backlight was already enabled when
the driver loads. One of the review requests was to factor out the
code dealing with getting/putting the runtime_pm reference when
changing state from enabled to disabled or the other way around.
That is the mean reason for this commit.

When writing this I noticed that if we rmmod the driver while
the backlight has been enabled through the driver (e.g. through
the sysfs interface) that we then leak the runtime-pm reference
which we get when we enable the pwm.

Note the purpose of adding a get_state callback is to have the i915
driver eventually read back the backlight state at boot and avoid
the backlight always jumping to 100% brightness at boot which
causes an ugly visual flicker.

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/pwm/pwm-lpss.c | 47 +++++++++++++++++++++++++++++-------------
>>   1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
>> index e602835fd6de..d39158800baa 100644
>> --- a/drivers/pwm/pwm-lpss.c
>> +++ b/drivers/pwm/pwm-lpss.c
>> @@ -120,43 +120,58 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
>>   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
>>   }
>>   
>> +static void pwm_lpss_get_put_runtime_pm(struct pwm_chip *chip,
>> +					bool old_enabled, bool new_enabled)
>> +{
>> +	if (new_enabled == old_enabled)
>> +		return;
>> +
>> +	if (new_enabled)
>> +		pm_runtime_get(chip->dev);
>> +	else
>> +		pm_runtime_put(chip->dev);
>> +}
>> +
>>   static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>   			  struct pwm_state *state)
>>   {
>>   	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
>> -	int ret;
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_sync(chip->dev);
>>   
>>   	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;
>> -			}
>> +			if (ret)
>> +				goto out;
>> +
>>   			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
>>   			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
>>   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
>>   			ret = pwm_lpss_wait_for_update(pwm);
>> -			if (ret) {
>> -				pm_runtime_put(chip->dev);
>> -				return ret;
>> -			}
>> +			if (ret)
>> +				goto out;
>> +
>>   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
>>   		} else {
>>   			ret = pwm_lpss_is_updating(pwm);
>>   			if (ret)
>> -				return ret;
>> +				goto out;
>> +
>>   			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
>>   			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
>> -			return pwm_lpss_wait_for_update(pwm);
>> +			ret = pwm_lpss_wait_for_update(pwm);
>>   		}
>>   	} else if (pwm_is_enabled(pwm)) {
>>   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
>> -		pm_runtime_put(chip->dev);
>>   	}
>>   
>> -	return 0;
>> +	pwm_lpss_get_put_runtime_pm(chip, pwm_is_enabled(pwm), state->enabled);
>> +
>> +out:
>> +	pm_runtime_put(chip->dev);
>> +	return ret;
>>   }
>>   
>>   static const struct pwm_ops pwm_lpss_ops = {
>> @@ -205,6 +220,10 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
>>   
>>   int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>>   {
>> +	bool enabled = pwm_is_enabled(&lpwm->chip.pwms[0]);
>> +
>> +	pwm_lpss_get_put_runtime_pm(&lpwm->chip, enabled, false);
>> +
>>   	return pwmchip_remove(&lpwm->chip);
>>   }
>>   EXPORT_SYMBOL_GPL(pwm_lpss_remove);
>> -- 
>> 2.19.0.rc0
>>
> 

Re: [PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 11:58:42AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 24-09-18 11:16, Andy Shevchenko wrote:
> > On Tue, Sep 11, 2018 at 07:45:32PM +0200, Hans de Goede wrote:
> > > Add a helper function to properly get / put the pwm's runtime_pm
> > > reference when changing from enabled to disable or vice versa.
> > > 
> > > And use this to ensure the pwm's runtime_pm reference is properly
> > > released on remove.
> > > 
> > 
> > Can you provide a test sequence to reproduce the issue?
> 
> This patch is mostly a preparation patch for adding the get_state
> callback to make the driver fully support the pwm-atomic model.
> 
> When I first tried to upstream the get_state code about a year
> ago, there was some not pretty code in there to deal with getting
> the runtime pm reference if the backlight was already enabled when
> the driver loads. One of the review requests was to factor out the
> code dealing with getting/putting the runtime_pm reference when
> changing state from enabled to disabled or the other way around.
> That is the mean reason for this commit.
> 
> When writing this I noticed that if we rmmod the driver while
> the backlight has been enabled through the driver (e.g. through
> the sysfs interface) that we then leak the runtime-pm reference
> which we get when we enable the pwm.
> 
> Note the purpose of adding a get_state callback is to have the i915
> driver eventually read back the backlight state at boot and avoid
> the backlight always jumping to 100% brightness at boot which
> causes an ugly visual flicker.

Thanks.

I'm just wondering if we can leave pwm_lpss_apply() untouched and use a new helper for ->remove() and ->get_state() only.

> > > --- a/drivers/pwm/pwm-lpss.c
> > > +++ b/drivers/pwm/pwm-lpss.c
> > > @@ -120,43 +120,58 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
> > >   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
> > >   }
> > > +static void pwm_lpss_get_put_runtime_pm(struct pwm_chip *chip,
> > > +					bool old_enabled, bool new_enabled)
> > > +{
> > > +	if (new_enabled == old_enabled)
> > > +		return;
> > > +
> > > +	if (new_enabled)
> > > +		pm_runtime_get(chip->dev);
> > > +	else
> > > +		pm_runtime_put(chip->dev);
> > > +}
> > > +
> > >   static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >   			  struct pwm_state *state)
> > >   {
> > >   	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > > -	int ret;
> > > +	int ret = 0;
> > > +
> > > +	pm_runtime_get_sync(chip->dev);
> > >   	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;
> > > -			}
> > > +			if (ret)
> > > +				goto out;
> > > +
> > >   			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
> > >   			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
> > >   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
> > >   			ret = pwm_lpss_wait_for_update(pwm);
> > > -			if (ret) {
> > > -				pm_runtime_put(chip->dev);
> > > -				return ret;
> > > -			}
> > > +			if (ret)
> > > +				goto out;
> > > +
> > >   			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
> > >   		} else {
> > >   			ret = pwm_lpss_is_updating(pwm);
> > >   			if (ret)
> > > -				return ret;
> > > +				goto out;
> > > +
> > >   			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
> > >   			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
> > > -			return pwm_lpss_wait_for_update(pwm);
> > > +			ret = pwm_lpss_wait_for_update(pwm);
> > >   		}
> > >   	} else if (pwm_is_enabled(pwm)) {
> > >   		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
> > > -		pm_runtime_put(chip->dev);
> > >   	}
> > > -	return 0;
> > > +	pwm_lpss_get_put_runtime_pm(chip, pwm_is_enabled(pwm), state->enabled);
> > > +
> > > +out:
> > > +	pm_runtime_put(chip->dev);
> > > +	return ret;
> > >   }
> > >   static const struct pwm_ops pwm_lpss_ops = {
> > > @@ -205,6 +220,10 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
> > >   int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
> > >   {
> > > +	bool enabled = pwm_is_enabled(&lpwm->chip.pwms[0]);
> > > +
> > > +	pwm_lpss_get_put_runtime_pm(&lpwm->chip, enabled, false);
> > > +
> > >   	return pwmchip_remove(&lpwm->chip);
> > >   }
> > >   EXPORT_SYMBOL_GPL(pwm_lpss_remove);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Hans de Goede]

Hi,

On 24-09-18 12:05, Andy Shevchenko wrote:
> On Mon, Sep 24, 2018 at 11:58:42AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 24-09-18 11:16, Andy Shevchenko wrote:
>>> On Tue, Sep 11, 2018 at 07:45:32PM +0200, Hans de Goede wrote:
>>>> Add a helper function to properly get / put the pwm's runtime_pm
>>>> reference when changing from enabled to disable or vice versa.
>>>>
>>>> And use this to ensure the pwm's runtime_pm reference is properly
>>>> released on remove.
>>>>
>>>
>>> Can you provide a test sequence to reproduce the issue?
>>
>> This patch is mostly a preparation patch for adding the get_state
>> callback to make the driver fully support the pwm-atomic model.
>>
>> When I first tried to upstream the get_state code about a year
>> ago, there was some not pretty code in there to deal with getting
>> the runtime pm reference if the backlight was already enabled when
>> the driver loads. One of the review requests was to factor out the
>> code dealing with getting/putting the runtime_pm reference when
>> changing state from enabled to disabled or the other way around.
>> That is the mean reason for this commit.
>>
>> When writing this I noticed that if we rmmod the driver while
>> the backlight has been enabled through the driver (e.g. through
>> the sysfs interface) that we then leak the runtime-pm reference
>> which we get when we enable the pwm.
>>
>> Note the purpose of adding a get_state callback is to have the i915
>> driver eventually read back the backlight state at boot and avoid
>> the backlight always jumping to 100% brightness at boot which
>> causes an ugly visual flicker.
> 
> Thanks.
> 
> I'm just wondering if we can leave pwm_lpss_apply() untouched and use a new helper for ->remove() and ->get_state() only.

The idea was to have a single place doing the pm_runtime_get() and
pm_runtime_put() calls. Leaving pwm_lpss_apply() as is and this not
using this helper there is fine with me, but then we might just as
well directly do the [un]ref directly in >remove() and ->get_state()
as well, that is just 2 lines (instead of 1) for each.

I'm not against leaving pwm_lpss_apply() as is, but then we might
just as well drop this patch, so do you want to drop this patch
for v2 ?

Also do you have any remarks on the patch adding the get_stage
callback before I send out a v2?

Regards,

Hans




>>>> --- a/drivers/pwm/pwm-lpss.c
>>>> +++ b/drivers/pwm/pwm-lpss.c
>>>> @@ -120,43 +120,58 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
>>>>    		pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
>>>>    }
>>>> +static void pwm_lpss_get_put_runtime_pm(struct pwm_chip *chip,
>>>> +					bool old_enabled, bool new_enabled)
>>>> +{
>>>> +	if (new_enabled == old_enabled)
>>>> +		return;
>>>> +
>>>> +	if (new_enabled)
>>>> +		pm_runtime_get(chip->dev);
>>>> +	else
>>>> +		pm_runtime_put(chip->dev);
>>>> +}
>>>> +
>>>>    static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>    			  struct pwm_state *state)
>>>>    {
>>>>    	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
>>>> -	int ret;
>>>> +	int ret = 0;
>>>> +
>>>> +	pm_runtime_get_sync(chip->dev);
>>>>    	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;
>>>> -			}
>>>> +			if (ret)
>>>> +				goto out;
>>>> +
>>>>    			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
>>>>    			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
>>>>    			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == false);
>>>>    			ret = pwm_lpss_wait_for_update(pwm);
>>>> -			if (ret) {
>>>> -				pm_runtime_put(chip->dev);
>>>> -				return ret;
>>>> -			}
>>>> +			if (ret)
>>>> +				goto out;
>>>> +
>>>>    			pwm_lpss_cond_enable(pwm, lpwm->info->bypass == true);
>>>>    		} else {
>>>>    			ret = pwm_lpss_is_updating(pwm);
>>>>    			if (ret)
>>>> -				return ret;
>>>> +				goto out;
>>>> +
>>>>    			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
>>>>    			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
>>>> -			return pwm_lpss_wait_for_update(pwm);
>>>> +			ret = pwm_lpss_wait_for_update(pwm);
>>>>    		}
>>>>    	} else if (pwm_is_enabled(pwm)) {
>>>>    		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
>>>> -		pm_runtime_put(chip->dev);
>>>>    	}
>>>> -	return 0;
>>>> +	pwm_lpss_get_put_runtime_pm(chip, pwm_is_enabled(pwm), state->enabled);
>>>> +
>>>> +out:
>>>> +	pm_runtime_put(chip->dev);
>>>> +	return ret;
>>>>    }
>>>>    static const struct pwm_ops pwm_lpss_ops = {
>>>> @@ -205,6 +220,10 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
>>>>    int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>>>>    {
>>>> +	bool enabled = pwm_is_enabled(&lpwm->chip.pwms[0]);
>>>> +
>>>> +	pwm_lpss_get_put_runtime_pm(&lpwm->chip, enabled, false);
>>>> +
>>>>    	return pwmchip_remove(&lpwm->chip);
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(pwm_lpss_remove);
> 

Re: [PATCH 1/2] pwm: lpss: Add pwm_lpss_get_put_runtime_pm helper function

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 02:13:03PM +0200, Hans de Goede wrote:
> On 24-09-18 12:05, Andy Shevchenko wrote:
> > On Mon, Sep 24, 2018 at 11:58:42AM +0200, Hans de Goede wrote:

> > Thanks.

> > I'm just wondering if we can leave pwm_lpss_apply() untouched and use a new helper for ->remove() and ->get_state() only.
> 
> The idea was to have a single place doing the pm_runtime_get() and
> pm_runtime_put() calls. Leaving pwm_lpss_apply() as is and this not
> using this helper there is fine with me, but then we might just as
> well directly do the [un]ref directly in >remove() and ->get_state()
> as well, that is just 2 lines (instead of 1) for each.
> 
> I'm not against leaving pwm_lpss_apply() as is, but then we might
> just as well drop this patch, so do you want to drop this patch
> for v2 ?

I would prefer not to change ->apply().
So, please, modify (I guess we still need some pm calls in ->remove()).
Perhaps it would also require Fixes tag to be applied.

> 
> Also do you have any remarks on the patch adding the get_stage
> callback before I send out a v2?

No, looks sane, just same comment as above per pm calls.


-- 
With Best Regards,
Andy Shevchenko