Message-ID: <Z8bNUI9Fj2TaV/2m@linaro.org>
Date: Tue, 4 Mar 2025 11:52:16 +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 11:21:33, 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.
> 
> > 
> > > 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.

Ignore this comment.

Just learned about 'b4 --edit-deps' today, so will do that from now on.

> 
> > 
> > Best regards
> > Uwe
> 
> Thanks for reviewing,
> Abel
> 

Powered by blists - more mailing lists