diff mbox series

[RFC,v4,01/15] pwm: core: export pwm_get_state_hw()

Message ID 20241023-dlech-mainline-spi-engine-offload-2-v4-1-f8125b99f5a1@baylibre.com
State Superseded
Headers show
Series spi: axi-spi-engine: add offload support | expand

Commit Message

David Lechner Oct. 23, 2024, 8:59 p.m. UTC
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.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v4 changes: new patch in v4

And FYI for Uwe and Jonathan, there are a couple of other series
introducing PWM conversion triggers that could make use of this
so that the sampling_frequency attribute can return the actual rate
rather than the requested rate.

Already applied:
https://lore.kernel.org/linux-iio/20241015-ad7606_add_iio_backend_support-v5-4-654faf1ae08c@baylibre.com/

Under review:
https://lore.kernel.org/linux-iio/aea7f92b-3d12-4ced-b1c8-90bcf1d992d3@baylibre.com/T/#m1377d5acd7e996acd1f59038bdd09f0742d3ac35
---
 drivers/pwm/core.c  | 55 +++++++++++++++++++++++++++++++++++++----------------
 include/linux/pwm.h |  1 +
 2 files changed, 40 insertions(+), 16 deletions(-)

Comments

Uwe Kleine-König Oct. 29, 2024, 8:05 a.m. UTC | #1
Hello David,

On Wed, Oct 23, 2024 at 03:59:08PM -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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v4 changes: new patch in v4
> 
> And FYI for Uwe and Jonathan, there are a couple of other series
> introducing PWM conversion triggers that could make use of this
> so that the sampling_frequency attribute can return the actual rate
> rather than the requested rate.
> 
> Already applied:
> https://lore.kernel.org/linux-iio/20241015-ad7606_add_iio_backend_support-v5-4-654faf1ae08c@baylibre.com/
> 
> Under review:
> https://lore.kernel.org/linux-iio/aea7f92b-3d12-4ced-b1c8-90bcf1d992d3@baylibre.com/T/#m1377d5acd7e996acd1f59038bdd09f0742d3ac35
> ---
>  drivers/pwm/core.c  | 55 +++++++++++++++++++++++++++++++++++++----------------
>  include/linux/pwm.h |  1 +
>  2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 634be56e204b..a214d0165d09 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -718,7 +718,7 @@ 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)
> +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;
> @@ -730,29 +730,50 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>  
>  		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;
>  }

I don't understand why you introduce __pwm_get_state_hw() (a variant of
pwm_get_state_hw() that expects the caller to hold the chip lock) when the
single caller (apart from plain pwm_get_state_hw()) could just continue
to use pwm_get_state_hw().

In principle I'm open to such a patch and wonder if there is already a
merge plan for this series. If you send a simpler patch soon with the
same objective, I'll make sure it goes into v6.13-rc1 in the assumption
that it's to late for the whole series to go in then. Or do you still
target 6.13-rc1 for the spi bits? Then it would probably better to let
this patch go in with the rest via the spi tree.

Best regards
Uwe
David Lechner Oct. 29, 2024, 3:30 p.m. UTC | #2
On 10/29/24 3:05 AM, Uwe Kleine-König wrote:
> Hello David,
> 
> On Wed, Oct 23, 2024 at 03:59:08PM -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.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
>> v4 changes: new patch in v4
>>
>> And FYI for Uwe and Jonathan, there are a couple of other series
>> introducing PWM conversion triggers that could make use of this
>> so that the sampling_frequency attribute can return the actual rate
>> rather than the requested rate.
>>
>> Already applied:
>> https://lore.kernel.org/linux-iio/20241015-ad7606_add_iio_backend_support-v5-4-654faf1ae08c@baylibre.com/
>>
>> Under review:
>> https://lore.kernel.org/linux-iio/aea7f92b-3d12-4ced-b1c8-90bcf1d992d3@baylibre.com/T/#m1377d5acd7e996acd1f59038bdd09f0742d3ac35
>> ---
>>  drivers/pwm/core.c  | 55 +++++++++++++++++++++++++++++++++++++----------------
>>  include/linux/pwm.h |  1 +
>>  2 files changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 634be56e204b..a214d0165d09 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -718,7 +718,7 @@ 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)
>> +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;
>> @@ -730,29 +730,50 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>>  
>>  		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;
>>  }
> 
> I don't understand why you introduce __pwm_get_state_hw() (a variant of
> pwm_get_state_hw() that expects the caller to hold the chip lock) when the
> single caller (apart from plain pwm_get_state_hw()) could just continue
> to use pwm_get_state_hw().

Hmm... it seems like I thought there was a good reason for it at the
time, but looking at it again, I agree with your assessment.

> 
> In principle I'm open to such a patch and wonder if there is already a
> merge plan for this series. If you send a simpler patch soon with the
> same objective, I'll make sure it goes into v6.13-rc1 in the assumption
> that it's to late for the whole series to go in then. Or do you still
> target 6.13-rc1 for the spi bits? Then it would probably better to let
> this patch go in with the rest via the spi tree.

The SPI offload stuff is not likely to be merged soon. But there is
ad7606 + AXI ADC support from Guillaume that was just merged that
could make use of this. So I can send this as a stand-alone patch
so that it can be made available for that too.
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 634be56e204b..a214d0165d09 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -718,7 +718,7 @@  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)
+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;
@@ -730,29 +730,50 @@  static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
 
 		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;
 }
 
+/**
+ * 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;
+
+	might_sleep();
+
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
+	return __pwm_get_state_hw(pwm, state);
+}
+EXPORT_SYMBOL_GPL(pwm_get_state_hw);
+
 /**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
  * @pwm: PWM device
@@ -906,9 +927,11 @@  static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		 */
 		struct pwm_state state = { 0, };
 
-		err = pwm_get_state_hw(pwm, &state);
-		if (!err)
-			pwm->state = state;
+		scoped_guard(pwmchip, chip) {
+			err = __pwm_get_state_hw(pwm, &state);
+			if (!err)
+				pwm->state = state;
+		}
 
 		if (IS_ENABLED(CONFIG_PWM_DEBUG))
 			pwm->last = pwm->state;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f1cb1e5b0a36..5bcbcf2911c3 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -370,6 +370,7 @@  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_get_state_hw(struct pwm_device *pwm, struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
 /**