Re: [PATCH v2 1/2] pwm: core: export pwm_get_state_hw()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux