Hello David,
On Tue, Oct 29, 2024 at 04:18:49PM -0500, David Lechner wrote:
> Export the pwm_get_state_hw() function. This is useful in cases where
> we want to know what the hardware is actually doing, rather than what
> what we requested it should do.
>
> Locking had to be rearranged to ensure that the chip is still
> operational before trying to access ops now that this can be called
> from outside the pwm core.
Good point, I didn't notice that in my review of v1.
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
>
> v2 changes:
> * Dropped __pwm_get_state_hw() function.
> * Reworded commit message.
> ---
> drivers/pwm/core.c | 40 +++++++++++++++++++++++++++-------------
> include/linux/pwm.h | 1 +
> 2 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4399e793efaf..ccbdd6dd1410 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -718,40 +718,54 @@ 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)
> +/**
> + * pwm_get_state_hw() - get the current PWM state from hardware
> + * @pwm: PWM device
> + * @state: state to fill with the current PWM state
> + *
> + * Similar to pwm_get_state() but reads the current PWM state from hardware
> + * instead of the requested state.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + * Context: May sleep.
> + */
> +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;
>
> + might_sleep();
Maybe this should be better
if (!chip->atomic)
might_sleep();
but I'm open to keep it unconditional until someone wails about it.
> + guard(pwmchip)(chip);
> +
> + if (!chip->operational)
> + return -ENODEV;
> +
Huh, that means that __pwm_read_waveform() et al were called before
without holding the chip lock. How embarrassing. I think nothing bad
happens (because at this stage the PWM wasn't exposed to its consumer
yet and so no concurrency could happen), but still.
> if (ops->read_waveform) {
> char wfhw[WFHWSIZE];
> struct pwm_waveform wf;
>
> BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
>
> - scoped_guard(pwmchip, chip) {
> + ret = __pwm_read_waveform(chip, pwm, &wfhw);
> + if (ret)
> + return ret;
>
> - ret = __pwm_read_waveform(chip, pwm, &wfhw);
> - if (ret)
> - return ret;
> -
> - ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
> - if (ret)
> - return ret;
> - }
> + ret = __pwm_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);
> -
> + ret = ops->get_state(chip, pwm, state);
> trace_pwm_get(pwm, state, ret);
> }
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(pwm_get_state_hw);
>
> /**
> * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
I applied that patch to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
