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

[David Lechner <dlechner@baylibre.com>]

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?


> 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.