Message-ID: <Z8bGHV4PIkY4te6V@linaro.org>
Date: Tue, 4 Mar 2025 11:21:33 +0200
From: Abel Vesa <abel.vesa@...aro.org>
To: 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 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.

> 
> > 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?

If so, then what happens when consumer asks for 5ms duty cycle?
Everything above the 4.26ms will just represent 100% duty cycle.

> 
> 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.

That might break other providers for the pwm_bl consumer, wouldn't it?

> 
> > [...]
> > @@ -532,13 +534,15 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> >  	if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
> >  		max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
> >  		clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
> > +		pwm_resolution_arr = lpg_pwm_resolution_hi_res;
> >  	} else {
> >  		max = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;
> >  		clk_rate = lpg_clk_rates[chan->clk_sel];
> > +		pwm_resolution_arr = lpg_pwm_resolution;
> >  	}
> >  
> > -	val = div64_u64(duty * clk_rate,
> > -			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div_sel] * (1 << chan->pre_div_exp));
> > +	step = div64_u64(period, max);
> > +	val = div64_u64(duty, step);
> 
> Such a pair of divisions is bound to loose precision. I didn't try to
> determine values that can happen in practise here, but consider:
> 
> 	period =  999996
> 	max =     500000
> 	duty =    499998
> 
> The exact value for val is 250000. However with integer division you
> get 499998. What you can do about that is calculating
> 
> 	val = duty * max / period
> 
> instead which gives you a more exact result. The challenge here however
> is that the multiplication might overflow. If you know that the result
> fits into a u64, mul_u64_u64_div_u64() is the function that gets this
> right for you.

I like this idea. Will use that instead.

> 
> >  	chan->pwm_value = min(val, max);
> >  }
> > [...]
> > ---
> > 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.

> 
> Best regards
> Uwe

Thanks for reviewing,
Abel

Powered by blists - more mailing lists