Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update

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

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

On Mon, Jul 20, 2015 at 11:48:27AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:59:40 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
[...]
> > > +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.

Yeah, it could be a static function in core.c. What I want to avoid is
having to change a bunch of code if ever state assignment becomes
something other than merely copying a structure.

[...]
> > > 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).

I don't mind the pwm_apply_state() name very much. It's pretty accurate
with regards to what it does.

Thierry

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