On Mon, 20 Jul 2015 10:59:40 +0200
Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> > Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
> > implement atomic update.
> > This method will be prefered over the ->enable(), ->disable() and
> > ->config() methods if available.
> >
> > Add the pwm_get_state(), pwm_get_default_state() and pwm_apply_state()
> > functions for PWM users to be able to use the atomic update feature.
> >
> > Note that the pwm_apply_state() does not guarantee the atomicity of the
> > update operation, it all depends on the availability and implementation
> > of the ->apply() method.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
> > include/linux/pwm.h | 26 +++++++++++++
> > 2 files changed, 124 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 30631f5..6dafd8e 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -238,8 +238,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > unsigned int i;
> > int ret;
> >
> > - if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
> > - !chip->ops->enable || !chip->ops->disable || !chip->npwm)
> > + if (!chip || !chip->dev || !chip->ops || (!chip->ops->apply &&
> > + (!chip->ops->config || !chip->ops->enable ||
> > + !chip->ops->disable)) || !chip->npwm)
> > return -EINVAL;
>
> This is becoming really unreadable, perhaps split it into two checks, or
> even split out the sanity check on the ops into a separate function to
> make the negations easier to read:
>
> static bool pwm_ops_check(const struct pwm_ops *ops)
> {
> /* driver supports legacy, non-atomic operation */
> if (ops->config && ops->enable && ops->disable)
> return true;
>
> /* driver supports atomic operation */
> if (ops->apply)
> return true;
>
> return false;
> }
>
> and then use this:
>
> if (!chip || !chip->dev || !chip->ops || !chip->npwm)
> return -EINVAL;
>
> if (!pwm_ops_check(chip->ops))
> return -EINVAL;
>
Sure, I'll change that to make it more readable.
> > mutex_lock(&pwm_lock);
> > @@ -430,7 +431,17 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> > if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> > return -EINVAL;
> >
> > - err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> > + if (pwm->chip->ops->apply) {
> > + struct pwm_state state = pwm->state;
>
> Shouldn't this use pwm_get_state()?
Yes, I'll fix all of them
[...]
>
> > +
> > + state.enabled = true;
> > + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
>
> There should be a space between the above two lines.
I'll add an empty line.
>
> >
> > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > +{
> > + int err = 0;
> > +
> > + if (!pwm)
> > + return -EINVAL;
> > +
> > + if (!memcmp(state, &pwm->state, sizeof(*state)))
> > + return 0;
> > +
> > + if (pwm->chip->ops->apply) {
> > + err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > + if (!err)
> > + pwm->state = *state;
>
> Maybe we want pwm_set_state() for this?
I'm not opposed to the addition of the pwm_set_state() function as long
as it's a private one: I don't want to let PMW drivers or users mess up
with the current PWM state.
>
> > + } else {
> > + /*
> > + * FIXME: restore the initial state in case of error.
> > + */
> > + if (state->polarity != pwm->state.polarity) {
> > + pwm_disable(pwm);
> > + err = pwm_set_polarity(pwm, state->polarity);
> > + if (err)
> > + goto out;
> > + }
> > +
> > + if (state->period != pwm->state.period ||
> > + state->duty_cycle != pwm->state.duty_cycle) {
> > + err = pwm_config(pwm, state->period, state->duty_cycle);
> > + if (err)
> > + goto out;
> > + }
> > +
> > + if (state->enabled != pwm->state.enabled) {
> > + if (state->enabled)
> > + err = pwm_enable(pwm);
> > + else
> > + pwm_disable(pwm);
> > + }
> > + }
> > +
> > +out:
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(pwm_apply_state);
> > +
> > static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> > {
> > struct pwm_chip *chip;
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index b47244a..7e99679 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> > return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> > }
> >
> > +/*
> > + * pwm_apply_state - apply a new state to the PWM device
> > + */
> > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
>
> If you add kerneldoc, please add it properly. It should start with /**
> and you need to list at least the parameters and return value.
Yes, I'll fix that.
BTW, I remember that you were expecting another name for this function
(pwm_update IIRC).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
