Re: [PATCH v4 2/7] pwm: New abstraction for PWM waveforms

[David Lechner <dlechner@baylibre.com>]

On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> Up to now the configuration of a PWM setting is described exclusively by
> a struct pwm_state which contains information about period, duty_cycle,
> polarity and if the PWM is enabled. (There is another member usage_power
> which doesn't completely fit into pwm_state, I ignore it here for
> simplicity.)
> 

...

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 3ea73e075abe..6a26a5210dab 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -49,6 +49,31 @@ enum {
>  	PWMF_EXPORTED = 1,
>  };
>  
> +/*

Should this be /** for kernel-doc?

> + * struct pwm_waveform - description of a PWM waveform
> + * @period_length_ns: PWM period
> + * @duty_length_ns: PWM duty cycle
> + * @duty_offset_ns: 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_ns and so can represent offsets other than $period - $duty_cycle

According to [1], "$" is generally used for environment variables. It
suggests that &struct_name.member should be preferred. I don't have any
strong opinions on what is best, but $ jumped out at me as unusual.

[1]: https://docs.kernel.org/doc-guide/kernel-doc.html

> + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
> + * explicit bool for enabled. A "disabled" PWM is represented by
> + * .period_length_ns = 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_ns;
> +	u64 duty_length_ns;
> +	u64 duty_offset_ns;
> +};