Message-ID: <997d4cf8-5256-4413-8059-569451962a83@linaro.org>
Date: Wed, 5 Mar 2025 15:42:56 +0100
From: neil.armstrong@...aro.org
To: Abel Vesa <abel.vesa@...aro.org>, Uwe Kleine-König
 <ukleinek@...nel.org>
Cc: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>,
 Anjelique Melendez <anjelique.melendez@....qualcomm.com>,
 Kamal Wadhwa <quic_kamalw@...cinc.com>,
 Jishnu Prakash <jishnu.prakash@....qualcomm.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>, Johan Hovold <johan@...nel.org>,
 Sebastian Reichel <sre@...nel.org>, Pavel Machek <pavel@....cz>,
 linux-leds@...r.kernel.org, linux-pwm@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] leds: rgb: leds-qcom-lpg: Compute PWM value based on
 period instead

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@linaro.org/
> 
>>
>>>>> 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
> 

Powered by blists - more mailing lists