Re: [PATCH v2 1/5] pwm: core: add pwm_get_waveform_step_might_sleep()

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Nov 04, 2026 at 03:21:39PM +0200, Nicolae-Daniel Deaconescu wrote:
> A consumer that has to meet minimum times, or place an edge relative to
> the cycles of a clock that a PWM gates, cannot do that with the waveform
> API alone. The rounding functions round down, so a minimum has to be
> requested with some margin, and the size of that margin is the step of
> the PWM, which only the provider knows.
> 
> A provider can report the rate of the clock its timing is derived from
> and how many cycles of that clock one step lasts. A consumer can then
> work in whole steps, convert them to nanoseconds rounding up, and get
> that waveform back from the hardware. Without this, a consumer has to
> read the rate from a clock it gets on its own, which describes
> a relationship the hardware doesn't have, and is wrong if
> PWM divides the clock.

...

> +/**
> + * pwm_get_waveform_step_might_sleep - Query the granularity of the waveforms
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @step: output parameter
> + *
> + * Stores in @step the clock rate the timing of @pwm is derived from and how
> + * many cycles of that clock one step lasts. Period, duty length and duty
> + * offset are multiples of that step, so a waveform asked for in whole steps
> + * is not changed by rounding. Compute the length in nanoseconds of n steps
> + * as n * @step->clk_cycles_per_step * NSEC_PER_SEC / @step->clk_rate_hz and
> + * round it up, because pwm_round_waveform_might_sleep() and
> + * pwm_set_waveform_might_sleep() round down.
> + *
> + * The values can change when a clock changes its rate, so they are only valid
> + * as long as the provider's input clocks are not reconfigured.
> + *
> + * Returns: 0 on success, -EOPNOTSUPP if the provider doesn't report it or a
> + * negative errno

> + * Context: May sleep.

This is incorrect. The Returns section has to be the last one in the
kernel-doc. It's documented.

> + */

> +int pwm_get_waveform_step_might_sleep(struct pwm_device *pwm,
> +				      struct pwm_waveform_step *step)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	const struct pwm_ops *ops = chip->ops;
> +	int ret;
> +
> +	if (!pwmchip_supports_waveform(chip) || !ops->get_waveform_step)
> +		return -EOPNOTSUPP;
> +
> +	might_sleep();
> +
> +	guard(pwmchip)(chip);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
> +		/*
> +		 * Catch any drivers that have been marked as atomic but
> +		 * that will sleep anyway.
> +		 */
> +		non_block_start();
> +		ret = ops->get_waveform_step(chip, pwm, step);
> +		non_block_end();
> +	} else {
> +		ret = ops->get_waveform_step(chip, pwm, step);
> +	}
> +	if (ret)
> +		return ret;
> +
> +	if (!step->clk_rate_hz || !step->clk_cycles_per_step ||

The second check is unneeded, the third one covers it.

> +	    step->clk_rate_hz > (u64)step->clk_cycles_per_step * NSEC_PER_SEC)
> +		return -EINVAL;
> +
> +	return 0;
> +}

...

> +/**
> + * struct pwm_waveform_step - granularity of the waveforms a PWM can generate
> + * @clk_rate_hz: rate of the clock the PWM's timing is derived from
> + * @clk_cycles_per_step: number of those clock cycles one step lasts
> + *
> + * The period, the duty length and the duty offset of a waveform are multiples
> + * of one step, which lasts @clk_cycles_per_step / @clk_rate_hz seconds. A PWM
> + * that divides its input clock in hardware has @clk_cycles_per_step > 1.
> + *
> + * The step is the same for every waveform and for the period, the duty length
> + * and the duty offset, so a provider whose granularity depends on the
> + * requested period (for example through a prescaler) can't describe itself
> + * with it. One step also has to last at least 1 ns, otherwise rounding a
> + * length in nanoseconds up can add a whole step.
> + *
> + * A consumer that needs a minimum time, or that has to place an edge relative
> + * to the cycles of the clock a PWM channel gates, can use this to ask for
> + * the next possible value instead of one that is rounded down, and to compute
> + * the frequency a rounded period results in without the error of the
> + * nanosecond resolution of struct pwm_waveform.
> + */
> +struct pwm_waveform_step {
> +	unsigned long clk_rate_hz;

_Hz ?

> +	unsigned int clk_cycles_per_step;
> +};

-- 
With Best Regards,
Andy Shevchenko