Re: [PATCH v2 4/4] pwm: lpss: Switch to new atomic API

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2917 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]