Re: [PATCH v5 3/8] pwm: Provide new consumer API functions for waveforms

[Trevor Gamblin <tgamblin@baylibre.com>]

On 2024-09-20 04:57, 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>
Tested-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>   drivers/pwm/core.c  | 261 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/pwm.h |   6 +-
>   2 files changed, 266 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index e3e26aafa461..1cefc6e9c10d 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -49,6 +49,30 @@ static void pwmchip_unlock(struct pwm_chip *chip)
>   
>   DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
>   
> +static bool pwm_wf_valid(const struct pwm_waveform *wf)
> +{
> +	/*
> +	 * For now restrict waveforms to period_length_ns <= S64_MAX to provide
> +	 * some space for future extensions. One possibility is to simplify
> +	 * representing waveforms with inverted polarity using negative values
> +	 * somehow.
> +	 */
> +	if (wf->period_length_ns > S64_MAX)
> +		return false;
> +
> +	if (wf->duty_length_ns > wf->period_length_ns)
> +		return false;
> +
> +	/*
> +	 * .duty_offset_ns is supposed to be smaller than .period_length_ns, apart
> +	 * from the corner case .duty_offset_ns == 0 && .period_length_ns == 0.
> +	 */
> +	if (wf->duty_offset_ns && wf->duty_offset_ns >= wf->period_length_ns)
> +		return false;
> +
> +	return true;
> +}
> +
>   static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state)
>   {
>   	if (wf->period_length_ns) {
> @@ -95,6 +119,29 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf)
>   	}
>   }
>   
> +static int pwmwfcmp(const struct pwm_waveform *a, const struct pwm_waveform *b)
> +{
> +	if (a->period_length_ns > b->period_length_ns)
> +		return 1;
> +
> +	if (a->period_length_ns < b->period_length_ns)
> +		return -1;
> +
> +	if (a->duty_length_ns > b->duty_length_ns)
> +		return 1;
> +
> +	if (a->duty_length_ns < b->duty_length_ns)
> +		return -1;
> +
> +	if (a->duty_offset_ns > b->duty_offset_ns)
> +		return 1;
> +
> +	if (a->duty_offset_ns < b->duty_offset_ns)
> +		return -1;
> +
> +	return 0;
> +}
> +
>   static int pwm_check_rounding(const struct pwm_waveform *wf,
>   			      const struct pwm_waveform *wf_rounded)
>   {
> @@ -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
> + *
> + * 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)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	const struct pwm_ops *ops = chip->ops;
> +	struct pwm_waveform wf_req = *wf;
> +	char wfhw[WFHWSIZE];
> +	int ret_tohw, ret_fromhw;
> +
> +	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> +
> +	if (!pwm_wf_valid(wf))
> +		return -EINVAL;
> +
> +	guard(pwmchip)(chip);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	ret_tohw = __pwm_round_waveform_tohw(chip, pwm, wf, wfhw);
> +	if (ret_tohw < 0)
> +		return ret_tohw;
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ret_tohw > 1)
> +		dev_err(&chip->dev, "Unexpected return value from __pwm_round_waveform_tohw: requested %llu/%llu [+%llu], return value %d\n",
> +			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, ret_tohw);
> +
> +	ret_fromhw = __pwm_round_waveform_fromhw(chip, pwm, wfhw, wf);
> +	if (ret_fromhw < 0)
> +		return ret_fromhw;
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ret_fromhw > 0)
> +		dev_err(&chip->dev, "Unexpected return value from __pwm_round_waveform_fromhw: requested %llu/%llu [+%llu], return value %d\n",
> +			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, ret_tohw);
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) &&
> +	    ret_tohw == 0 && pwm_check_rounding(&wf_req, wf))
> +		dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
> +			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns,
> +			wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);
> +
> +	return ret_tohw;
> +}
> +EXPORT_SYMBOL_GPL(pwm_round_waveform_might_sleep);
> +
> +/**
> + * pwm_get_waveform_might_sleep - Query hardware about current configuration
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: output parameter
> + *
> + * Stores the current configuration of the PWM in @wf. Note this is the
> + * equivalent of pwm_get_state_hw() (and not pwm_get_state()) for pwm_waveform.
> + */
> +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	const struct pwm_ops *ops = chip->ops;
> +	char wfhw[WFHWSIZE];
> +	int err;
> +
> +	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> +
> +	guard(pwmchip)(chip);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	err = __pwm_read_waveform(chip, pwm, &wfhw);
> +	if (err)
> +		return err;
> +
> +	return __pwm_round_waveform_fromhw(chip, pwm, &wfhw, wf);
> +}
> +EXPORT_SYMBOL_GPL(pwm_get_waveform_might_sleep);
> +
> +/* Called with the pwmchip lock held */
> +static int __pwm_set_waveform(struct pwm_device *pwm,
> +			      const struct pwm_waveform *wf,
> +			      bool exact)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	const struct pwm_ops *ops = chip->ops;
> +	char wfhw[WFHWSIZE];
> +	struct pwm_waveform wf_rounded;
> +	int err;
> +
> +	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> +
> +	if (!pwm_wf_valid(wf))
> +		return -EINVAL;
> +
> +	err = __pwm_round_waveform_tohw(chip, pwm, wf, &wfhw);
> +	if (err)
> +		return err;
> +
> +	if ((IS_ENABLED(CONFIG_PWM_DEBUG) || exact) && wf->period_length_ns) {
> +		err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
> +		if (err)
> +			return err;
> +
> +		if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm_check_rounding(wf, &wf_rounded))
> +			dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
> +				wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> +				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
> +
> +		if (exact && pwmwfcmp(wf, &wf_rounded)) {
> +			dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n",
> +				wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> +				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
> +
> +			return 1;
> +		}
> +	}
> +
> +	err = __pwm_write_waveform(chip, pwm, &wfhw);
> +	if (err)
> +		return err;
> +
> +	/* update .state */
> +	pwm_wf2state(wf, &pwm->state);
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ops->read_waveform && wf->period_length_ns) {
> +		struct pwm_waveform wf_set;
> +
> +		err = __pwm_read_waveform(chip, pwm, &wfhw);
> +		if (err)
> +			/* maybe ignore? */
> +			return err;
> +
> +		err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_set);
> +		if (err)
> +			/* maybe ignore? */
> +			return err;
> +
> +		if (pwmwfcmp(&wf_set, &wf_rounded) != 0)
> +			dev_err(&chip->dev,
> +				"Unexpected setting: requested %llu/%llu [+%llu], expected %llu/%llu [+%llu], set %llu/%llu [+%llu]\n",
> +				wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> +				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns,
> +				wf_set.duty_length_ns, wf_set.period_length_ns, wf_set.duty_offset_ns);
> +	}
> +	return 0;
> +}
> +
> +/**
> + * pwm_set_waveform_might_sleep - Apply a new waveform
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: The waveform to apply
> + * @exact: If true no rounding is allowed
> + *
> + * Typically a requested waveform cannot be implemented exactly, e.g. because
> + * you requested .period_length_ns = 100 ns, but the hardware can only set
> + * periods that are a multiple of 8.5 ns. With that hardware passing exact =
> + * true results in pwm_set_waveform_might_sleep() failing and returning 1. If
> + * exact = false you get a period of 93.5 ns (i.e. the biggest period not bigger
> + * than the requested value).
> + * Note that even with exact = true, some rounding by less than 1 is
> + * possible/needed. In the above example requesting .period_length_ns = 94 and
> + * exact = true, you get the hardware configured with period = 93.5 ns.
> + */
> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm,
> +				 const struct pwm_waveform *wf, bool exact)
> +{
> +	struct pwm_chip *chip = pwm->chip;
> +	int err;
> +
> +	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();
> +		err = __pwm_set_waveform(pwm, wf, exact);
> +		non_block_end();
> +	} else {
> +		err = __pwm_set_waveform(pwm, wf, exact);
> +	}
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(pwm_set_waveform_might_sleep);
> +
>   static void pwm_apply_debug(struct pwm_device *pwm,
>   			    const struct pwm_state *state)
>   {
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d8cfe1c9b19d..c3d9ddeafa65 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -358,7 +358,11 @@ static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
>   }
>   
>   #if IS_ENABLED(CONFIG_PWM)
> -/* PWM user APIs */
> +
> +/* PWM consumer APIs */
> +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
>   int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
>   int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
>   int pwm_adjust_config(struct pwm_device *pwm);