[PATCH 2/6] pwm: New abstraction for PWM waveforms
Trevor Gamblin
tgamblin at baylibre.com
Mon Jul 8 11:12:40 PDT 2024
On 2024-07-08 6:52 a.m., Uwe Kleine-König wrote:
> Up to now the configuration of a PWM setting is decribed 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.)
>
> Instead of a polarity the new abstraction has a member duty_offset that
> defines when the rising edge happens after the period start. This is
> more general, as with a pwm_state the rising edge can only happen at the
> period's start or such that the falling edge is at the end of the period
> (i.e. duty_offset == 0 or duty_offset == period_lengh - duty_length).
>
> A disabled PWM is modeled by .period_length = 0. In my eyes this is a
> nice usage of that otherwise unusable setting, as it doesn't define
> anything about the future which matches the fact that consumers should
> consider the state of the output as undefined and it's just there to say
> "No further requirements about the output, you can save some power.".
>
> Further I renamed period and duty_cycle to period_length and
> duty_length. In the past there was confusion from time to time about
> duty_cycle being measured in nanoseconds because people expected a
> percentage of period instead. With "length" as suffix the semantic
> should be more obvious to people unfamiliar with the pwm subsystem.
> period is renamed period_length for consistency.
>
> The API for consumers doesn't change yet, but lowlevel drivers can
> implement callbacks that work with pwm_waveforms instead of pwm_states.
> A new thing about these callbacks is that the calculation of hardware
> settings needed to implement a certain waveform is separated from
> actually writing these settings. The motivation for that is that this
> allows a consumer to query the hardware capabilities without actually
> modifying the hardware state.
>
> The rounding rules that are expected to be implemented in the
> round_waveform_tohw() are: First pick the biggest possible period not
> bigger than wf->period_length. For that period pick the biggest possible
> duty setting not bigger than wf->duty_length. Third pick the biggest
> possible offset not bigger than wf->duty_offset. If the requested period
> is too small for the hardware, it's expected that a setting with the
> minimal period and duty_length = duty_offset = 0 is returned and this
> fact is signaled by a return value of 1.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at baylibre.com>
Reviewed-by: Trevor Gamblin <tgamblin at baylibre.com>
> ---
> drivers/pwm/core.c | 194 +++++++++++++++++++++++++++++++++++++++-----
> include/linux/pwm.h | 35 ++++++++
> 2 files changed, 208 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index c31e12e76495..8e68481a7b33 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -49,6 +49,72 @@ static void pwmchip_unlock(struct pwm_chip *chip)
>
> DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
>
> +static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state)
> +{
> + if (wf->period_length) {
> + if (wf->duty_length + wf->duty_offset < wf->period_length)
> + *state = (struct pwm_state){
> + .enabled = true,
> + .polarity = PWM_POLARITY_NORMAL,
> + .period = wf->period_length,
> + .duty_cycle = wf->duty_length,
> + };
> + else
> + *state = (struct pwm_state){
> + .enabled = true,
> + .polarity = PWM_POLARITY_INVERSED,
> + .period = wf->period_length,
> + .duty_cycle = wf->period_length - wf->duty_length,
> + };
> + } else {
> + *state = (struct pwm_state){
> + .enabled = false,
> + };
> + }
> +}
> +
> +static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf)
> +{
> + if (state->enabled) {
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + *wf = (struct pwm_waveform){
> + .period_length = state->period,
> + .duty_length = state->duty_cycle,
> + .duty_offset = 0,
> + };
> + else
> + *wf = (struct pwm_waveform){
> + .period_length = state->period,
> + .duty_length = state->period - state->duty_cycle,
> + .duty_offset = state->duty_cycle,
> + };
> + } else {
> + *wf = (struct pwm_waveform){
> + .period_length = 0,
> + };
> + }
> +}
> +
> +static int pwm_check_rounding(const struct pwm_waveform *wf,
> + const struct pwm_waveform *wf_rounded)
> +{
> + if (!wf->period_length)
> + return 0;
> +
> + if (wf->period_length < wf_rounded->period_length)
> + return 1;
> +
> + if (wf->duty_length < wf_rounded->duty_length)
> + return 1;
> +
> + if (wf->duty_offset < wf_rounded->duty_offset)
> + return 1;
> +
> + return 0;
> +}
> +
> +#define WFHWSIZE 20
> +
> static void pwm_apply_debug(struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> @@ -182,6 +248,7 @@ static bool pwm_state_valid(const struct pwm_state *state)
> static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
> {
> struct pwm_chip *chip;
> + const struct pwm_ops *ops;
> int err;
>
> if (!pwm || !state)
> @@ -205,6 +272,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
> }
>
> chip = pwm->chip;
> + ops = chip->ops;
>
> if (state->period == pwm->state.period &&
> state->duty_cycle == pwm->state.duty_cycle &&
> @@ -213,18 +281,59 @@ 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);
>
> - /*
> - * only do this after pwm->state was applied as some
> - * implementations of .get_state depend on this
> - */
> - pwm_apply_debug(pwm, state);
> + pwm_state2wf(state, &wf);
> +
> + /*
> + * XXX The rounding is wrong here for states with inverted
> + * polarity. While .apply() rounds down duty_cycle (which
> + * represents the time from the start of the period to the inner
> + * edge), .round_waveform_tohw() rounds down the time the PWM is
> + * high.
> + */
> +
> + err = ops->round_waveform_tohw(chip, pwm, &wf, &wfhw);
> + if (err)
> + return err;
> +
> + if (IS_ENABLED(PWM_DEBUG)) {
> + struct pwm_waveform wf_rounded;
> +
> + err = ops->round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
> + if (err)
> + return err;
> +
> + if (pwm_check_rounding(&wf, &wf_rounded))
> + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
> + wf.duty_length, wf.period_length, wf.duty_offset,
> + wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
> + }
> +
> + err = ops->write_waveform(chip, pwm, &wfhw);
> + if (err)
> + return err;
> +
> + pwm->state = *state;
> +
> + } else {
> + err = ops->apply(chip, pwm, state);
> + trace_pwm_apply(pwm, state, err);
> + if (err)
> + return err;
> +
> + pwm->state = *state;
> +
> + /*
> + * only do this after pwm->state was applied as some
> + * implementations of .get_state depend on this
> + */
> + pwm_apply_debug(pwm, state);
> + }
>
> return 0;
> }
> @@ -292,6 +401,41 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
> }
> EXPORT_SYMBOL_GPL(pwm_apply_atomic);
>
> +static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
> +{
> + struct pwm_chip *chip = pwm->chip;
> + const struct pwm_ops *ops = chip->ops;
> + int ret = -EOPNOTSUPP;
> +
> + if (ops->read_waveform) {
> + char wfhw[WFHWSIZE];
> + struct pwm_waveform wf;
> +
> + BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> +
> + scoped_guard(pwmchip, chip) {
> +
> + ret = ops->read_waveform(chip, pwm, &wfhw);
> + if (ret)
> + return ret;
> +
> + ret = ops->round_waveform_fromhw(chip, pwm, &wfhw, &wf);
> + if (ret)
> + return ret;
> + }
> +
> + pwm_wf2state(&wf, state);
> +
> + } else if (ops->get_state) {
> + scoped_guard(pwmchip, chip)
> + ret = ops->get_state(chip, pwm, state);
> +
> + trace_pwm_get(pwm, state, ret);
> + }
> +
> + return ret;
> +}
> +
> /**
> * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
> * @pwm: PWM device
> @@ -433,7 +577,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
> }
> }
>
> - if (ops->get_state) {
> + if (ops->read_waveform || ops->get_state) {
> /*
> * Zero-initialize state because most drivers are unaware of
> * .usage_power. The other members of state are supposed to be
> @@ -443,11 +587,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
> */
> struct pwm_state state = { 0, };
>
> - scoped_guard(pwmchip, chip)
> - err = ops->get_state(chip, pwm, &state);
> -
> - trace_pwm_get(pwm, &state, err);
> -
> + err = pwm_get_state_hw(pwm, &state);
> if (!err)
> pwm->state = state;
>
> @@ -1134,12 +1274,24 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
> {
> const struct pwm_ops *ops = chip->ops;
>
> - if (!ops->apply)
> - return false;
> + if (ops->write_waveform) {
> + if (!ops->round_waveform_tohw ||
> + !ops->round_waveform_fromhw ||
> + !ops->write_waveform)
> + return false;
>
> - if (IS_ENABLED(CONFIG_PWM_DEBUG) && !ops->get_state)
> - dev_warn(pwmchip_parent(chip),
> - "Please implement the .get_state() callback\n");
> + if (WFHWSIZE < ops->sizeof_wfhw) {
> + dev_warn(pwmchip_parent(chip), "WFHWSIZE < %zu\n", ops->sizeof_wfhw);
> + return false;
> + }
> + } else {
> + if (!ops->apply)
> + return false;
> +
> + if (IS_ENABLED(CONFIG_PWM_DEBUG) && !ops->get_state)
> + dev_warn(pwmchip_parent(chip),
> + "Please implement the .get_state() callback\n");
> + }
>
> return true;
> }
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 5176dfebfbfd..b5dff2a99038 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;
> +};
> +
> /*
> * struct pwm_state - state of a PWM channel
> * @period: PWM period (in nanoseconds)
> @@ -259,6 +283,17 @@ struct pwm_ops {
> void (*free)(struct pwm_chip *chip, struct pwm_device *pwm);
> int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_capture *result, unsigned long timeout);
> +
> + size_t sizeof_wfhw;
> + int (*round_waveform_tohw)(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_waveform *wf, void *wfhw);
> + int (*round_waveform_fromhw)(struct pwm_chip *chip, struct pwm_device *pwm,
> + const void *wfhw, struct pwm_waveform *wf);
> + int (*read_waveform)(struct pwm_chip *chip, struct pwm_device *pwm,
> + void *wfhw);
> + int (*write_waveform)(struct pwm_chip *chip, struct pwm_device *pwm,
> + const void *wfhw);
> +
> int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
> const struct pwm_state *state);
> int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
More information about the linux-arm-kernel
mailing list