Re: [PATCH v2 01/13] pwm: Add new helpers to create/manipulate PWM states

[Boris Brezillon]

Hi Thierry,

On Fri, 10 Jun 2016 17:21:09 +0200
Thierry Reding <thierry.reding@xxxxxxxxx> wrote:

> On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
> > The pwm_prepare_new_state() helper prepares a new state object
> > containing the current PWM state except for the polarity and period
> > fields which are set to the reference values.
> > This is particurly useful for PWM users who want to apply a new
> > duty-cycle expressed relatively to the reference period without
> > changing the enable state.
> > 
> > The PWM framework expect PWM users to configure the duty cycle in
> > nanosecond, but most users just want to express this duty cycle
> > relatively to the period value (i.e. duty_cycle = 33% of the period).
> > Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
> > conversion.  
> 
> This looks pretty useful and good, though I think these could be two
> separate patches. A couple of suggestions on wording and such below.

Sure, I can split those changes.

> 
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > ---
> >  include/linux/pwm.h | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> > 
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 17018f3..e09babf 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -148,6 +148,87 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
> >  }
> >  
> >  /**
> > + * pwm_prepare_new_state() - prepare a new state to be applied with
> > + *			     pwm_apply_state()  
> 
> This is weirdly indented.

I'll fix the indentation.

> 
> > + * @pwm: PWM device
> > + * @state: state to fill with the prepared PWM state
> > + *
> > + * This functions prepares a state that can later be tweaked and applied
> > + * to the PWM device with pwm_apply_state(). This is a convenient function
> > + * that first retrieves the current PWM state and the replaces the period
> > + * and polarity fields with the reference values defined in pwm->args.
> > + * Once the new state is created you can adjust the ->enable and ->duty_cycle  
> 
> "created" has the connotation of allocating. Perhaps: "Once the function
> returns, you can adjust the ->enabled and ->duty_cycle fields according
> to your needs before calling pwm_apply_state()."?

Okay.

> 
> Also note that the field is called ->enabled.
> 
> > + * according to your need before calling pwm_apply_state().  
> 
> Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
> again, do we really need it? If users are going to overwrite it anyway,
> do we even need to bother? I suppose it makes some sense because the
> current duty cycle is stale when the ->period gets set to the value from
> args. I think the documentation should mention this in some way.

Yes, if we keep the current duty_cycle it can exceed the period value.
I'm fine dropping the ->duty_cycle = 0 assignment and documenting the
behavior.

> 
> > + */
> > +static inline void pwm_prepare_new_state(const struct pwm_device *pwm,
> > +					 struct pwm_state *state)  
> 
> Perhaps choose a shorter name, such as: pwm_init_state()?

Sure.

> 
> > +{
> > +	struct pwm_args args;
> > +
> > +	/* First get the current state. */
> > +	pwm_get_state(pwm, state);
> > +
> > +	/* Then fill it with the reference config */
> > +	pwm_get_args(pwm, &args);
> > +
> > +	state->period = args.period;
> > +	state->polarity = args.polarity;
> > +	state->duty_cycle = 0;
> > +}
> > +
> > +/**
> > + * pwm_get_relative_duty_cycle() - Get a relative duty_cycle value
> > + * @state: the PWM state to extract period and duty_cycle from
> > + * @scale: the scale you want to use for you relative duty_cycle value  
> 
> Other kerneldoc comments in this file don't prefix the description with
> "the".
> 
> Also, perhaps the following descriptions would be slightly less
> confusing:
> 
> 	@state: PWM state to extract the duty cycle from

Agreed.

> 
> We don't extract (i.e. return) period from the state, so it's a little
> confusing to mention it here.
> 
> 	@scale: scale in which to return the relative duty cycle
> 
> This is slightly more explicit in that it says what the returned duty
> cycle will be in. Perhaps as an alternative:
> 
> 	@scale: target scale of the relative duty cycle

I'll go for this one.

> 
> > + *
> > + * This functions converts the absolute duty_cycle stored in @state
> > + * (expressed in nanosecond) into a relative value.  
> 
> Perhaps: "... into a value relative to the period."?

Okay.

> 
> > + * For example if you want to get the duty_cycle expressed in nanosecond,  
> 
> The example below would give you the duty cycle in percent, not
> nanoseconds, right?

Absolutely. I'll fix that.

> 
> > + * call:
> > + *
> > + * pwm_get_state(pwm, &state);
> > + * duty = pwm_get_relative_duty_cycle(&state, 100);
> > + */
> > +static inline unsigned int
> > +pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
> > +{
> > +	if (!state->period)
> > +		return 0;
> > +
> > +	return DIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle * scale,
> > +				     state->period);
> > +}
> > +
> > +/**
> > + * pwm_set_relative_duty_cycle() - Set a relative duty_cycle value
> > + * @state: the PWM state to fill
> > + * @val: the relative duty_cycle value
> > + * @scale: the scale you use for you relative duty_cycle value  
> 
> "scale in which @duty_cycle is expressed"?

Yep.

> 
> > + *
> > + * This functions converts a relative duty_cycle stored into an absolute
> > + * one (expressed in nanoseconds), and put the result in state->duty_cycle.
> > + * For example if you want to change configure a 50% duty_cycle, call:
> > + *
> > + * pwm_prepare_new_state(pwm, &state);
> > + * pwm_set_relative_duty_cycle(&state, 50, 100);
> > + * pwm_apply_state(pwm, &state);
> > + */
> > +static inline void
> > +pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int val,  
> 
> Any reason why we can't call "val" "duty_cycle"? That's what it is,
> after all.

Nope, I'll switch to duty_cycle.

> 
> > +			    unsigned int scale)
> > +{
> > +	if (!scale)
> > +		return;
> > +
> > +	/* Make sure we don't have duty_cycle > period */
> > +	if (val > scale)
> > +		val = scale;  
> 
> Can we return -EINVAL for both of the above checks? I don't like
> silently clamping the duty cycle that the user passed in.

I'm fine returning -EINVAL in this case.

Thanks for rewording some of my sentences and reviewing the patch.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com