Re: [PATCH v2 3/8] pwm: New abstraction for PWM waveforms

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2024-07-15 at 22:17 +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jul 15, 2024 at 01:55:43PM -0500, David Lechner wrote:
> > On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> > > @@ -213,18 +311,60 @@ static int __pwm_apply(struct pwm_device *pwm, const
> > > struct pwm_state *state)
> > >  	    state->usage_power == pwm->state.usage_power)
> > >  		return 0;
> > >  
> > > -	err = chip->ops->apply(chip, pwm, state);
> > > -	trace_pwm_apply(pwm, state, err);
> > > -	if (err)
> > > -		return err;
> > > +	if (ops->write_waveform) {
> > > +		struct pwm_waveform wf;
> > > +		char wfhw[WFHWSIZE];
> > >  
> > > -	pwm->state = *state;
> > > +		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> > 
> > Since this is already validated in pwm_ops_check(), do we really need the
> > BUG_ON() check?
> 
> It indeed should not happen, and I'm glad you seem to agree it's safe.
> The motivation to still keep it is that if (now or after some changes
> in the future) I missed a code path, it's IMHO better when the kernel
> dies on a BUG_ON (which indicates the error condition) than via some
> stack corruption some time later.
> 
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index 464054a45e57..2a1f1f25a56c 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -49,6 +49,30 @@ enum {
> > >  	PWMF_EXPORTED = 1,
> > >  };
> > >  
> > > +/*
> > > + * struct pwm_waveform - description of a PWM waveform
> > > + * @period_length: PWM period
> > > + * @duty_length: PWM duty cycle
> > > + * @duty_offset: offset of the rising edge from the period's start
> > > + *
> > > + * This is a representation of a PWM waveform alternative to struct
> > > pwm_state
> > > + * below. It's more expressive than struct pwm_state as it contains a
> > > + * duty_offset and so can represent offsets other than $period -
> > > $duty_cycle
> > > + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is
> > > no
> > > + * explicit bool for enabled. A "disabled" PWM is represented by .period
> > > = 0.
> > > + *
> > > + * Note that the behaviour of a "disabled" PWM is undefined. Depending on
> > > the
> > > + * hardware's capabilities it might drive the active or inactive level,
> > > go
> > > + * high-z or even continue to toggle.
> > > + *
> > > + * The unit for all three members is nanoseconds.
> > > + */
> > > +struct pwm_waveform {
> > > +	u64 period_length;
> > > +	u64 duty_length;
> > > +	u64 duty_offset;
> > > +};
> > 
> > Perhaps it would be helpful to take a hint from the IIO subsystem
> > and include the units of measurement in the field names here?
> > For example, period_length_ns or even just period_ns. This way,
> > the value is obvious even without reading the documentation.
> 
> Good idea. For duty_length the "length" part is more important than for
> period_length. And indeed I wasn't sure if I should rename period at
> all. Being a fan of consistency I prefer to keep "length" also for
> period (length). But I like the _ns suffix and will rework accordingly.
> 

Just as a side note. BUG_ON() usage is highly discouraged [1]... Even WARN_ON()
- which I don't agree much FWIW - is also being discouraged because of
panic_on_warn. But it actually seems that WARN* is fine but should really be
used with care.

[1]: https://docs.kernel.org/process/coding-style.html#use-warn-rather-than-bug

- Nuno Sá