| Message ID | 20241023-dlech-mainline-spi-engine-offload-2-v4-1-f8125b99f5a1@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
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
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 --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); /**
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(-)