[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <82edfbb9-5e65-4292-b15b-d5cde7b53e42@baylibre.com>
Date: Tue, 29 Oct 2024 10:30:43 -0500
From: David Lechner <dlechner@...libre.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>,
Guillaume Stols <gstols@...libre.com>
Cc: Mark Brown <broonie@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Nuno Sá
<nuno.sa@...log.com>, Michael Hennerich <Michael.Hennerich@...log.com>,
Lars-Peter Clausen <lars@...afoo.de>, David Jander <david@...tonic.nl>,
Martin Sperl <kernel@...tin.sperl.org>, linux-spi@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH RFC v4 01/15] pwm: core: export pwm_get_state_hw()
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@...libre.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.
Powered by blists - more mailing lists