Re: [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms

[David Lechner <dlechner@baylibre.com>]

On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> Provide API functions for consumers to work with waveforms.
> 
> Note that one relevant difference between pwm_get_state() and
> pwm_get_waveform*() is that the latter yields the actually configured
> hardware state, while the former yields the last state passed to
> pwm_apply*() and so doesn't account for hardware specific rounding.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

One thing I am wondering about is how to implement rounding up instead
of down. For example, I we need a >= 10 ns duty cycle.

Here is my attempt...

#define MIN_DUTY_NS 10

int some_func(struct pwm_device *pwm)
{
	struct pwm_waveform wf = {
		.period_length_ns = 400,
	};
	u64 trial_ns = MIN_DUTY_NS;
	int ret;

	do {
		wf.duty_length_ns = trial_ns;

		ret = pwm_round_waveform_might_sleep(pwm, &wf);
		if (ret < 0)
			return ret;

		/*
		 * ret == 1 could be either because duty or period
		 * is not attainable. In any case, we have to wait
		 * until the last trial to rule out earlier trials
		 * failing because of too small duty since we try
		 * again with larger duty. Maybe this check isn't
		 * needed though since pwm_round_waveform_might_sleep()
		 * should fail when trial_ns > wf.period_length_ns?
		 */
		if (ret == 1 && trial_ns == wf.period_length_ns)
			return -ERANGE;

		trial_ns++;
	} while (wf.duty_length_ns < MIN_DUTY_NS);

	return pwm_set_waveform_might_sleep(pwm, &wf, true);
}


1. This seems like it would waste time trying each 1 ns increment
   compared to being able to tell the low level driver which way
   we want to round.
2. Even with this, we could end up with an actual period of 9.5 ns
   which is < 10 ns but have to way to know since the returned value
   will be 10. Probably not likely that 0.5 ns is going to cause
   something to malfunction, but you never know.
3. Handling ret == 1 seems kind of messy since it could be caused
   by multiple different problems.

Maybe we could consider including a rounding direction (up/down/closest)
for each of the waveform parameters and pass that along to low level
driver to avoid much of this? Or at least have these parameters for
the high-level pwm_round_waveform_might_sleep() so each consumer doesn't
have to try to figure out how to do the rounding right?

> @@ -145,6 +192,220 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c
>  
>  #define WFHWSIZE 20
>  
> +/**
> + * pwm_round_waveform_might_sleep - Query hardware capabilities
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: waveform to round and output parameter

It would be helpful to spell out in the description below that @wf will
be modified upon non-error return and what the modified values will
actually be. (Or refer to the other functions where the values are already
documented to avoid duplication.)

> + *
> + * Typically a given waveform cannot be implemented exactly by hardware, e.g.
> + * because hardware only supports coarse period resolution or no duty_offset.
> + * This function returns the actually implemented waveform if you pass wf to
> + * pwm_set_waveform_might_sleep now.
> + *
> + * Note however that the world doesn't stop turning when you call it, so when
> + * doing
> + *
> + * 	pwm_round_waveform_might_sleep(mypwm, &wf);
> + * 	pwm_set_waveform_might_sleep(mypwm, &wf, true);
> + *
> + * the latter might fail, e.g. because an input clock changed its rate between
> + * these two calls and the waveform determined by
> + * pwm_round_waveform_might_sleep() cannot be implemented any more.
> + *
> + * Returns 0 on success, 1 if there is no valid hardware configuration matching
> + * the input waveform under the PWM rounding rules or a negative errno.
> + */
> +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf)
> +{