| Message ID | 20170102091647.86910-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jan 02, 2017 at 11:16:47AM +0200, Andy Shevchenko wrote: > Instead of doing things separately, which is not so reliable on some platforms, > switch the driver to use new atomic API, i.e. ->apply() callback. > > The change has been tested on Intel platforms such as Broxton, BayTrail, and > Merrifield. > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pwm/pwm-lpss.c | 63 +++++++++++++++++++++++++++----------------------- > 1 file changed, 34 insertions(+), 29 deletions(-) > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index e7d612e9df51..7d3ac8204618 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -85,15 +85,20 @@ static inline void pwm_lpss_write(const struct pwm_device *pwm, u32 value) > > static void pwm_lpss_update(struct pwm_device *pwm) > { > + /* > + * Set a limit for busyloop since not all implementations correctly > + * clear PWM_SW_UPDATE bit (at least it's not visible on OS side). > + */ > + unsigned int count = 10; > + > pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE); > - /* Give it some time to propagate */ > - usleep_range(10, 50); > + while (pwm_lpss_read(pwm) & PWM_SW_UPDATE && --count) > + usleep_range(10, 20); > } > > -static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > +static int pwm_lpss_config(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, > int duty_ns, int period_ns) > { > - struct pwm_lpss_chip *lpwm = to_lpwm(chip); > unsigned long long on_time_div; > unsigned long c = lpwm->info->clk_rate, base_unit_range; > unsigned long long base_unit, freq = NSEC_PER_SEC; > @@ -114,8 +119,6 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > do_div(on_time_div, period_ns); > on_time_div = 255ULL - on_time_div; > > - pm_runtime_get_sync(chip->dev); > - > ctrl = pwm_lpss_read(pwm); > ctrl &= ~PWM_ON_TIME_DIV_MASK; > ctrl &= ~(base_unit_range << PWM_BASE_UNIT_SHIFT); > @@ -124,41 +127,43 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > ctrl |= on_time_div; > pwm_lpss_write(pwm, ctrl); > > - /* > - * If the PWM is already enabled we need to notify the hardware > - * about the change by setting PWM_SW_UPDATE. > - */ > - if (pwm_is_enabled(pwm)) > - pwm_lpss_update(pwm); > - > - pm_runtime_put(chip->dev); > - > + pwm_lpss_update(pwm); > return 0; > } > > -static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +static void pwm_lpss_enable(struct pwm_device *pwm) > { > - pm_runtime_get_sync(chip->dev); > - > - /* > - * Hardware must first see PWM_SW_UPDATE before the PWM can be > - * enabled. > - */ > - pwm_lpss_update(pwm); > pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); > - return 0; > } > > -static void pwm_lpss_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +static void pwm_lpss_disable(struct pwm_device *pwm) > { > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > - 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); > + > + if (state->enabled) { > + if (!pwm_is_enabled(pwm)) { > + pm_runtime_get_sync(chip->dev); > + pwm_lpss_config(lpwm, pwm, state->duty_cycle, state->period); > + pwm_lpss_enable(pwm); > + } else { > + pwm_lpss_config(lpwm, pwm, state->duty_cycle, state->period); > + } > + } else if (pwm_is_enabled(pwm)) { > + pwm_lpss_disable(pwm); > + pm_runtime_put(chip->dev); > + } Would you mind doing another pass over this and inline the pwm_lpss_enable(), pwm_lpss_disable() and pwm_lpss_config() functions into pwm_lpss_apply()? Your current version effectively duplicates the transitional helpers, but the goal should be to fully get rid of the remainders of the legacy API. Besides being much cleaner this also gets rid of slight inconsistencies between the two APIs. Thierry
On Wed, 2017-01-18 at 12:15 +0100, Thierry Reding wrote: > On Mon, Jan 02, 2017 at 11:16:47AM +0200, Andy Shevchenko wrote: > > Instead of doing things separately, which is not so reliable on some > > platforms, > > switch the driver to use new atomic API, i.e. ->apply() callback. > > > > The change has been tested on Intel platforms such as Broxton, > > BayTrail, and > > Merrifield. > > +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); > > + > > + if (state->enabled) { > > + if (!pwm_is_enabled(pwm)) { > > + pm_runtime_get_sync(chip->dev); > > + pwm_lpss_config(lpwm, pwm, state- > > >duty_cycle, state->period); > > + pwm_lpss_enable(pwm); > > + } else { > > + pwm_lpss_config(lpwm, pwm, state- > > >duty_cycle, state->period); > > + } > > + } else if (pwm_is_enabled(pwm)) { > > + pwm_lpss_disable(pwm); > > + pm_runtime_put(chip->dev); > > + } > > Would you mind doing another pass over this and inline the > pwm_lpss_enable(), pwm_lpss_disable() and pwm_lpss_config() functions > into pwm_lpss_apply()? Your current version effectively duplicates the > transitional helpers, but the goal should be to fully get rid of the > remainders of the legacy API. I don't see how inlining them helps. For me readability of code is more important than names of the functions. I can rename functions, but I would like to have them separate from the ->apply() callback. Compiler inlines them during optimization. > Besides being much cleaner this also gets rid of slight > inconsistencies > between the two APIs. I don't see how One Big Function can be cleaner than split version. But I give a try to see it on my side when you have settled with Mika's patch.
On Thu, Jan 19, 2017 at 04:32:46PM +0200, Andy Shevchenko wrote: > On Wed, 2017-01-18 at 12:15 +0100, Thierry Reding wrote: > > On Mon, Jan 02, 2017 at 11:16:47AM +0200, Andy Shevchenko wrote: > > > Instead of doing things separately, which is not so reliable on some > > > platforms, > > > switch the driver to use new atomic API, i.e. ->apply() callback. > > > > > > The change has been tested on Intel platforms such as Broxton, > > > BayTrail, and > > > Merrifield. > > > > +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); > > > + > > > + if (state->enabled) { > > > + if (!pwm_is_enabled(pwm)) { > > > + pm_runtime_get_sync(chip->dev); > > > + pwm_lpss_config(lpwm, pwm, state- > > > >duty_cycle, state->period); > > > + pwm_lpss_enable(pwm); > > > + } else { > > > + pwm_lpss_config(lpwm, pwm, state- > > > >duty_cycle, state->period); > > > + } > > > + } else if (pwm_is_enabled(pwm)) { > > > + pwm_lpss_disable(pwm); > > > + pm_runtime_put(chip->dev); > > > + } > > > > Would you mind doing another pass over this and inline the > > pwm_lpss_enable(), pwm_lpss_disable() and pwm_lpss_config() functions > > into pwm_lpss_apply()? Your current version effectively duplicates the > > transitional helpers, but the goal should be to fully get rid of the > > remainders of the legacy API. > > I don't see how inlining them helps. For me readability of code is more > important than names of the functions. > > I can rename functions, but I would like to have them separate from the > ->apply() callback. > > Compiler inlines them during optimization. > > > Besides being much cleaner this also gets rid of slight > > inconsistencies > > between the two APIs. > > I don't see how One Big Function can be cleaner than split version. But > I give a try to see it on my side when you have settled with Mika's > patch. It doesn't necessarily have to be one big function. You obviously need to find the right balance. If the combined function is still readable, there's no reason, in my opinion, to split it up. If it gets too large I think a good split is to keep all of the register programming to be within ->apply() and have a helper that computes the values to program and that is called from ->apply(). What I want to avoid is people converting to atomic by mindlessly duplicating the fallback path in pwm_apply_state(). The goal of the atomic API is that programming of the hardware becomes atomic. So if you have any computations that may fail because of invalid inputs or similar, then all of that should happen before any registers are touched. That's difficult to do with the old enable/config/disable callbacks, so I don't think we should be following it when converting. Thierry
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index e7d612e9df51..7d3ac8204618 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -85,15 +85,20 @@ static inline void pwm_lpss_write(const struct pwm_device *pwm, u32 value) static void pwm_lpss_update(struct pwm_device *pwm) { + /* + * Set a limit for busyloop since not all implementations correctly + * clear PWM_SW_UPDATE bit (at least it's not visible on OS side). + */ + unsigned int count = 10; + pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE); - /* Give it some time to propagate */ - usleep_range(10, 50); + while (pwm_lpss_read(pwm) & PWM_SW_UPDATE && --count) + usleep_range(10, 20); } -static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, +static int pwm_lpss_config(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, int duty_ns, int period_ns) { - struct pwm_lpss_chip *lpwm = to_lpwm(chip); unsigned long long on_time_div; unsigned long c = lpwm->info->clk_rate, base_unit_range; unsigned long long base_unit, freq = NSEC_PER_SEC; @@ -114,8 +119,6 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, do_div(on_time_div, period_ns); on_time_div = 255ULL - on_time_div; - pm_runtime_get_sync(chip->dev); - ctrl = pwm_lpss_read(pwm); ctrl &= ~PWM_ON_TIME_DIV_MASK; ctrl &= ~(base_unit_range << PWM_BASE_UNIT_SHIFT); @@ -124,41 +127,43 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, ctrl |= on_time_div; pwm_lpss_write(pwm, ctrl); - /* - * If the PWM is already enabled we need to notify the hardware - * about the change by setting PWM_SW_UPDATE. - */ - if (pwm_is_enabled(pwm)) - pwm_lpss_update(pwm); - - pm_runtime_put(chip->dev); - + pwm_lpss_update(pwm); return 0; } -static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm) +static void pwm_lpss_enable(struct pwm_device *pwm) { - pm_runtime_get_sync(chip->dev); - - /* - * Hardware must first see PWM_SW_UPDATE before the PWM can be - * enabled. - */ - pwm_lpss_update(pwm); pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE); - return 0; } -static void pwm_lpss_disable(struct pwm_chip *chip, struct pwm_device *pwm) +static void pwm_lpss_disable(struct pwm_device *pwm) { pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); - 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); + + if (state->enabled) { + if (!pwm_is_enabled(pwm)) { + pm_runtime_get_sync(chip->dev); + pwm_lpss_config(lpwm, pwm, state->duty_cycle, state->period); + pwm_lpss_enable(pwm); + } else { + pwm_lpss_config(lpwm, pwm, state->duty_cycle, state->period); + } + } else if (pwm_is_enabled(pwm)) { + pwm_lpss_disable(pwm); + pm_runtime_put(chip->dev); + } + + return 0; } static const struct pwm_ops pwm_lpss_ops = { - .config = pwm_lpss_config, - .enable = pwm_lpss_enable, - .disable = pwm_lpss_disable, + .apply = pwm_lpss_apply, .owner = THIS_MODULE, };