On 05/03/2025 15:32, Abel Vesa wrote:
On 25-03-04 16:38:57, Uwe Kleine-König wrote:
On Tue, Mar 04, 2025 at 11:21:33AM +0200, Abel Vesa wrote:
On 25-03-04 07:24:32, Uwe Kleine-König wrote:
Hello Abel,
On Mon, Mar 03, 2025 at 06:14:36PM +0200, Abel Vesa wrote:
Currently, the implementation computes the best matched period based
on the requested one, by looping through all possible register
configurations. The best matched period is below the requested period.
The best matched period *isn't above* the requested one. An exact match
is fine.
Yep, that's better. Will re-word.
This means the PWM consumer could request duty cycle values between
the best matched period and the requested period, which with the current
implementation for computing the PWM value, it will result in values out
of range with respect to the selected resolution.
I still don't understand what you mean with resolution here.
Resolution in this context means the number of bits the PWM value
(register value) is represented in. Currently, the driver supporst two PWM
HW subtypes: normal and Hi-Res. Normal ones recently got support for changing
the resolution between 6 bits or 9 bits. The high resolution ones support
anything between 8 bits and 15 bits.
I guess you spend some time understanding the workings of the driver and
you also have an example request that results in a hardware
configuration you don't like. Please share the latter to a) support your
case and b) make it easier for your reviewers to judge if your change is
indeed an improvement.
Sure, will bring up the 5ms period scenario again.
When the consumer requests a period of 5ms, the closest the HW can do in
this case is actually 4.26ms. Since the PWM API will continue to ask for
duty cycle values based on the 5ms period, for any duty cycle value
between 4.26ms and 5ms, the resulting PWM value will be above 255, which
has been selected as best resolution for the 4.26ms best matched period.
For example, when 5ms duty cycle value is requested, it will result in a
PWM value of 300, which overflows the 255 selected resolution.
this is the bug you have to fix then. The PWM value (that defines the
duty cycle) has to be calculated based on .period = 4.26 ms and capped
at 255. So assuming that 0 yields a duty cycle of 0 ms and 255 yields
4.26 ms, a request for .duty_cycle = 4; + .period = 5 should result in an
actual .duty_cycle = 239 / 255 * 4.26 ms = 3.992705882352941 ms;
+ .period = 4.26 ms.
OK then. The patchset that fixes this according to your suggestion is
already on the list (re-spun):
https://lore.kernel.org/all/20250305-leds-qcom-lpg-fix-max-pwm-on-hi-res-v4-0-bfe124a53a9f@xxxxxxxxxx/
So change the way the PWM value is determined as a ratio between the
requested period and duty cycle, mapped on the resolution interval.
Is the intention here that (for the picked period) a duty_cycle is
selected that approximates the requested relative duty_cycle (i.e.
.duty_cycle / .period)?
Yes, that exactly what the intention is.
If it's that: Nack. This might be the right thing for your use case, but
it's wrong for others, it complicates the driver because you have spend
more effort in the calculation and (from my POV even worse) the driver's
behaviour deviates from the usual one for pwm drivers. I admit there are
some other lowlevel pwm drivers that are not aligned to the procedure I
described that should be used to determine the register settings for a
given request. But I target a common behaviour of all pwm drivers
because that is the only way the pwm API functions can make a promise to
its consumers about the resulting behaviour. Reaching this is difficult,
because some consumers might depend on the "broken" behaviour of a given
lowlevel driver (and also because analysing a driver to check and fix
its behaviour is an effort). But "fixing" a driver to deviate from the
declared right behaviour is wrong and includes all downsides that make
me hesitate to align the old drivers to the common policy.
OK, fair enough. But I still don't get what you expect from the provider
that can't give the exact requested period. Do you expect the consumer
to request a period, then provider compute a best matched one, which in
our case is pretty far, and then still give exact duty cycle values ?
Like: request 5ms period, get 4.26ms instead, then request 4ms duty
cycle and get exact 4ms duty cycle when measured, instead of a
proportional value to the best matched period?
Yes.
If so, then what happens when consumer asks for 5ms duty cycle?
Everything above the 4.26ms will just represent 100% duty cycle.
Yes.
I still think this is wrong.
I also think this is very wrong, duty_cycle is a factor of the period,
so if the HW gives a lower period, the term Pulse Width Modulation implies
the ratio between the "duty_cycle" and the period is important,
not the exact duration of the components of the modulation.
So is this a defect of the PWM API ? why would the API insist on
having an exact duty_cycle and a random period ?
I mean if you look at the basis of PWM :
https://en.wikipedia.org/wiki/Pulse-width_modulation
The duty_cycle is expressed as a percentage of the period, because
this is the key feature of PWM here, can you explain more in detail
why we can't make an extra effort to keep the duty_cycle/period ratio ?
Neil
I do agree with the exact value. I advocated for it on the other
thread.
But the fact that the API allows requests with values above what the
provider can do is wrong.
In this specific case, we are talking about top 15% that it just
thrown away. But it becomes even worse for others.
The policy to pick a hardware setting is a compromise between consumer
needs and what is straight forward to implement for (most) hardware
drivers. Please stick to that. If you want more flexibility and
precision in your consumer, please consider converting the pwm driver to
the waveform API.
That means the pwm_bl driver will have to switch to waveform API, IIUC.
Yes, if the pwm_bl driver cares about that precision it has to switch.
While the waveform API isn't expressive enough, just use 4260000 as
period in the pwm_bl device, or ignore the missing precision.
That might break other providers for the pwm_bl consumer, wouldn't it?
Given that the consumer side of the waveform API only works with drivers
that are converted: maybe. You could fall-back to the legacy API.
Based on the provider's best matched period? Hm.
[...]
---
base-commit: 0067a4b21c9ab441bbe6bf3635b3ddd21f6ca7c3
My git repo doesn't know that commit. Given that you said your patch
bases on that other series, this isn't surprising. Please use a publicly
available commit as base parameter, otherwise you (and I) don't benefit
from the armada of build bots because they just silently fail to test in
this case.
Well, this is a pretty common practice. When the patch relies on other
patches that haven't been merged yet, but are still on the list, you
can't really base it on a publicly available commit.
And the fixes patchset that this is based on is needed for this to work.
So I really don't get how this can be done differently.
You can still use --base=$newestpubliccommit and git-format-patch will
at least give a chance to the build bots by emitting patch-ids for all
the commits between the public base and the start of your patch series.
Got it. I use b4 for most patches nowadays. I'll try to make use of it's
--edit-deps and see where that lands.
Best regards
Uwe
Thanks,
Abel
