| Message ID | 20250303-leds-qcom-lpg-compute-pwm-value-using-period-v1-1-833e729e3da2@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [RFC] leds: rgb: leds-qcom-lpg: Compute PWM value based on period instead | expand |
Hi, 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. > 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. > > So change the way the PWM value is determined as a ratio between the > requested period and duty cycle, mapped on the resolution interval. > Do this in contrast to using the register configuration selected when > the best matching period was determined. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > This patch is based on the following patchset: > https://lore.kernel.org/all/20250303-leds-qcom-lpg-fix-max-pwm-on-hi-res-v3-0-62703c0ab76a@linaro.org/ > --- Tested-by: Sebastian Reichel <sre@kernel.org> Greetings, -- Sebastian > drivers/leds/rgb/leds-qcom-lpg.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c > index 4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2..80130c205dce7c6ae1c356b7a7c5f6460a2b0bb0 100644 > --- a/drivers/leds/rgb/leds-qcom-lpg.c > +++ b/drivers/leds/rgb/leds-qcom-lpg.c > @@ -523,8 +523,10 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period) > return 0; > } > > -static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) > +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty, uint64_t period) > { > + const unsigned int *pwm_resolution_arr; > + unsigned int step; > unsigned int max; > unsigned int val; > unsigned int clk_rate; > @@ -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); > > chan->pwm_value = min(val, max); > } > @@ -829,7 +833,7 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev, > lpg_calc_freq(chan, NSEC_PER_MSEC); > > duty = div_u64(brightness * chan->period, cdev->max_brightness); > - lpg_calc_duty(chan, duty); > + lpg_calc_duty(chan, duty, NSEC_PER_MSEC); > chan->enabled = true; > chan->ramp_enabled = false; > > @@ -906,7 +910,7 @@ static int lpg_blink_set(struct lpg_led *led, > chan = led->channels[i]; > > lpg_calc_freq(chan, period); > - lpg_calc_duty(chan, duty); > + lpg_calc_duty(chan, duty, period); > > chan->enabled = true; > chan->ramp_enabled = false; > @@ -1241,7 +1245,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (ret < 0) > goto out_unlock; > > - lpg_calc_duty(chan, state->duty_cycle); > + lpg_calc_duty(chan, state->duty_cycle, state->period); > } > chan->enabled = state->enabled; > > > --- > base-commit: 0067a4b21c9ab441bbe6bf3635b3ddd21f6ca7c3 > change-id: 20250303-leds-qcom-lpg-compute-pwm-value-using-period-0823ceb7542a > > Best regards, > -- > Abel Vesa <abel.vesa@linaro.org> >
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. > 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. 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. > 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)? 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. 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. > [...] > @@ -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. > 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. Best regards Uwe
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
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 >
On 04/03/2025 07:24, Uwe Kleine-König wrote: > 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. > >> 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 As you can easily see in the signature, this patchset was generated by b4 and such tag was added automatically. No point in stripping it even if it is not useful (life, happens). In the same time the dependency, so the base of this patchset, is explained in the changelog, exactly in the place where it should be. Exactly how we all want. I don't understand what you are poking now. This is exactly the process how to send an RFC, which you can find in "[PATCH RFC]" subject, based on some other work in flight/progress. This is basically an exemplary patch how to do this. You cannot do it better. Best regards, Krzysztof
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. > > > 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. > > 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. > > > [...] > > > --- > > > 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. Best regards Uwe
Hello Krzysztof, On Tue, Mar 04, 2025 at 10:53:53AM +0100, Krzysztof Kozlowski wrote: > On 04/03/2025 07:24, Uwe Kleine-König wrote: > > 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. > > > >> 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 > > As you can easily see in the signature, this patchset was generated by > b4 and such tag was added automatically. No point in stripping it even > if it is not useful (life, happens). My request was not about stripping it, but making it useful. I don't know the b4 patch sending side, but git send-email has the capability to make it more useful in this scenario. I didn't check, but `b4 --edit-deps` which Abel mentioned sounds about right. The relevant documentation for the git side is the paragraph "BASE TREE INFORMATION" in git-format-patch(1). Best regards Uwe
On 04/03/2025 17:03, Uwe Kleine-König wrote: > Hello Krzysztof, > > On Tue, Mar 04, 2025 at 10:53:53AM +0100, Krzysztof Kozlowski wrote: >> On 04/03/2025 07:24, Uwe Kleine-König wrote: >>> 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. >>> >>>> 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 >> >> As you can easily see in the signature, this patchset was generated by >> b4 and such tag was added automatically. No point in stripping it even >> if it is not useful (life, happens). > > My request was not about stripping it, but making it useful. I don't > know the b4 patch sending side, but git send-email has the capability to > make it more useful in this scenario. I didn't check, but > `b4 --edit-deps` which Abel mentioned sounds about right. > > The relevant documentation for the git side is the paragraph "BASE TREE > INFORMATION" in git-format-patch(1). Useful how? The dependency is on the lists, so there is no base-commit you would know. And regardless of edit-deps, that base-commit tag is standard from b4, so what do you expect from all submitters even if this was not RFC? Always base on known commit? But for most of the cases this is irrelevant. I can have intermediate commit between linux-next tip and my patch, thus base-commit will be bogus for you, but it does not matter for the patch - it's based on linux-next. Best regards, Krzysztof
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 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
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 >
Hello Neil, On Wed, Mar 05, 2025 at 03:42:56PM +0100, neil.armstrong@linaro.org wrote: > 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: > > > > > 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/ Yeah, I thought so. It's in my review queue. > > > > > > 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. Well, if you asked for .period = 5ms and .duty_cycle = 5ms you even asked for a 100% relative duty cycle. So while I agree that you don't usually get exactly what you requested, this is a bad example to rant about. > 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. In Linux .duty_cycle was expressed in ns and not relative to period. Apart from that being a historic choice that is hard to change, IMHO this is a sane choice because in the kernel we have to stick to integer math and then if you want to express a relative duty_cycle you probably have to pick a divisor D such that .rel_duty_cycle = n represents a relative duty_cycle of n / D. What to pick for D? 100 to get percent as unit? Something bigger to increase precision? A power of two to match usual hardwares but make it less intuitive for humans? Also note that for some hardwares the "natural" divisor is 255. > So is this a defect of the PWM API ? why would the API insist on > having an exact duty_cycle and a random period ? The way to determine the actual hardware dependent settings for a requested pair of duty_cycle and period is IMHO straight forward, so duty_cycle selection isn't more exact than the one for period and period isn't more random than the one for duty_cycle. It can also happen the other way round that your request results in an near exact match for period and a big deviation for duty_cycle. So judging the defects of the PWM API from just one example is short-sighted. But still I hear you and the rules were defined as they are as a trade-off between consumer needs, needed complexity in lowlevel drivers and what most drivers already did at that time. Regarding consumer needs: I agree that most consumers care about the relative duty_cycle. But there are exceptions. I remember a motor where the absolute length of the duty_cycle defines the rotation speed and the period was more or less irrelevant. While here the point mostly goes to "keep relative duty_cycle", it's still a "you cannot please everyone" situation. Regarding complexity: A simple PWM typically has a fixed clock input and there is a register to define the period as a number of clock cycles. Then there are essentially two subtypes: a) the duty_cycle register uses the same time base as the period register; and b) the duty_cycle register unit is relative to the period length. Let's assume a clock input rate of 32768 Hz, so one cycle tick is q := 1 / (32768 Hz) ≅ 0.030517578125 ms. So the typical PWM of type a) has a 16 bit register for period and another one for duty_cycle. I think it's quite obvious that the chosen policy is very simple to implement for such a device, so I won't go into that further. Considering the "keep relative duty_cycle + round down" policy instead and a request .period = 5ms and .duty_cycle = 3ms. The best match for .period is 163q ≅ 4.974365234375 ms. Then to calculate the duty_cycle you have do determine: 163q * 3 / 5 = 97q ≅ 2.960205078125 ms giving an actual relative duty_cycle of 0.5950920245398773. This is quite a good fit. Using the "use the absolute values" policy we end up with duty_cycle = 98q ≅ 2.99072265625 (and the same period) which gives a relative duty_cycle of 0.6012269938650306. The result is somewhat similar (with 0.6012269938650306 being a slightly better result as 0.5950920245398773?), but the calculation needed for the "use the absolute values" is a bit simpler. In summary we can say that it's quite natural to round down both values in the "use the absolute values" case independent of your preferred policy, while rounding down in combination with the "keep relative duty_cycle" policy is tends to be worse because the duty_cycle register value is rounding down the result of a calculation that has a rounded value as input, so precision suffers. If your reflex now is to not always round down but sometimes(?) round up, please consider maintenance effort: This must be reviewed and it must be explained to driver authors. So that's not a good idea. Now let's consider a PWM of type b) with the same input clock freq. To be able to define the duty cycle in time units relative to the period, the period can be a multiple of 256q and the .duty_cycle register is an 8 bit one. 256q is already above 5 ms, so to get a fairer comparison let's assume a request of .period = 1280 ms and .duty_cycle = 768 ms (which is the above request just scaled by 256). So period ends up being 163 * 256q = 1273.4375 ms. With the "use the absolute values" policy we end up with .duty_cycle = 163 * 154q ≅ 766.05224609375 ms giving an relative duty_cycle of 0.6015625 and with the "keep relative duty_cycle" policy you end up with .duty_cycle = 163 * 153q ≅ 761.077880859375 ms and a relative duty_cycle of 0.59765625. In summary for b) there is again not much difference in the resulting configurations and complexity is again similar with a slight advantage for "keep relative duty_cycle". Without having done a complete survey back when I decided about the policy to pick my impression was that PWMs of type a) were more common. Also in my impression back then the difference in complexity between the two policies to chose among is smaller for type b) than for type a) which gives another slight advantage to "use the absolute values". Also looking at the drivers back then, "use the absolute values" policy was the more common one. Additionally I didn't like the fact that for the "keep relative duty_cycle" policy you have to base the calculation of duty_cycle on rounded values. So overall this made me pick the "use the absolute values" policy. And please believe me when I say this wasn't a whim of the moment decision but I invested quite some thought. The example you gave is somewhat a corner case because the requested and the actual period quite a lot---as you noticed yourself---and can be worked around by picking a better value for .period as I wrote in my previous mail. And no matter which policy you pick, depending on the use case for your consumer you will be able to find such degenerated examples. Having said all that (and hoping that this made it better understandable why we're where we are), there is an effort to improve here and to give consumers a better control over what they get. (But for the needs of a backlight this is probably overkill, so I refer again to the suggestion to pick a period that better matches the hardware.) Best regards Uwe
Hello Krzysztof, On Tue, Mar 04, 2025 at 05:30:40PM +0100, Krzysztof Kozlowski wrote: > On 04/03/2025 17:03, Uwe Kleine-König wrote: > > On Tue, Mar 04, 2025 at 10:53:53AM +0100, Krzysztof Kozlowski wrote: > >> On 04/03/2025 07:24, Uwe Kleine-König wrote: > >>>> [...] > >>>> --- > >>>> 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 > >> > >> As you can easily see in the signature, this patchset was generated by > >> b4 and such tag was added automatically. No point in stripping it even > >> if it is not useful (life, happens). > > > > My request was not about stripping it, but making it useful. I don't > > know the b4 patch sending side, but git send-email has the capability to > > make it more useful in this scenario. I didn't check, but > > `b4 --edit-deps` which Abel mentioned sounds about right. > > > > The relevant documentation for the git side is the paragraph "BASE TREE > > INFORMATION" in git-format-patch(1). > > Useful how? The dependency is on the lists, so there is no base-commit > you would know. Have you tried to understand the part of the manpage I pointed out? It seems to me "base-commit" has different semantics for us and only mine is aligned to git's (and consequently b4's) meaning. The correct base commit would have been cd3215bbcb9d4321def93fea6cfad4d5b42b9d1d. > And regardless of edit-deps, that base-commit tag is standard from b4, > so what do you expect from all submitters even if this was not RFC? I don't understand this question. I expect from submitters to pick a publicly known commit as base no matter if the series is an RFC or who's standard this is. > Always base on known commit? Yes please. The manpage isn't explicit about that but the above referenced commit has: The base tree info consists of the "base commit", which is a well-known commit that is part of the stable part of the project history everybody else works off of, and zero or more "prerequisite patches", which are well-known patches in flight that is not yet part of the "base commit" that need to be applied on top of "base commit" in topological order before the patches can be applied. > But for most of the cases this is > irrelevant. I can have intermediate commit between linux-next tip and my > patch, thus base-commit will be bogus for you, but it does not matter > for the patch - it's based on linux-next. I agree, linux-next is the base. So the respective tip of linux-next is the right thing to pass to git format-patch --base (independent of if it's called directly or through b4). Ideally you also drop the irrelevant intermediate patches to make the build bots test exactly the changes you suggest with your series. I would expect that this is the tree you actually tested, so it shouldn't be a big hurdle. So summarizing we have: Iff you use --base with a non-public commit, it's useless and irrelevant. I fully agree. Our conclusion is different though. You accept it's useless (and even request from me that I do the same), and I asked the submitter to use --base as intended to make the resulting information usable. Best regards Uwe
On 07/03/2025 00:33, Uwe Kleine-König wrote: > Hello Krzysztof, > > On Tue, Mar 04, 2025 at 05:30:40PM +0100, Krzysztof Kozlowski wrote: >> On 04/03/2025 17:03, Uwe Kleine-König wrote: >>> On Tue, Mar 04, 2025 at 10:53:53AM +0100, Krzysztof Kozlowski wrote: >>>> On 04/03/2025 07:24, Uwe Kleine-König wrote: >>>>>> [...] >>>>>> --- >>>>>> 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 >>>> >>>> As you can easily see in the signature, this patchset was generated by >>>> b4 and such tag was added automatically. No point in stripping it even >>>> if it is not useful (life, happens). >>> >>> My request was not about stripping it, but making it useful. I don't >>> know the b4 patch sending side, but git send-email has the capability to >>> make it more useful in this scenario. I didn't check, but >>> `b4 --edit-deps` which Abel mentioned sounds about right. >>> >>> The relevant documentation for the git side is the paragraph "BASE TREE >>> INFORMATION" in git-format-patch(1). >> >> Useful how? The dependency is on the lists, so there is no base-commit >> you would know. > > Have you tried to understand the part of the manpage I pointed out? It > seems to me "base-commit" has different semantics for us and only mine > is aligned to git's (and consequently b4's) meaning. > The correct base commit would have been > cd3215bbcb9d4321def93fea6cfad4d5b42b9d1d. > >> And regardless of edit-deps, that base-commit tag is standard from b4, >> so what do you expect from all submitters even if this was not RFC? > > I don't understand this question. I expect from submitters to pick a > publicly known commit as base no matter if the series is an RFC or who's > standard this is. > >> Always base on known commit? > > Yes please. The manpage isn't explicit about that but the above > referenced commit has: > > The base tree info consists of the "base commit", which is a well-known > commit that is part of the stable part of the project history everybody > else works off of, and zero or more "prerequisite patches", which are > well-known patches in flight that is not yet part of the "base commit" > that need to be applied on top of "base commit" in topological order > before the patches can be applied. > >> But for most of the cases this is >> irrelevant. I can have intermediate commit between linux-next tip and my >> patch, thus base-commit will be bogus for you, but it does not matter >> for the patch - it's based on linux-next. > > I agree, linux-next is the base. So the respective tip of linux-next is > the right thing to pass to git format-patch --base (independent of if > it's called directly or through b4). Ideally you also drop the > irrelevant intermediate patches to make the build bots test exactly the > changes you suggest with your series. I would expect that this is the > tree you actually tested, so it shouldn't be a big hurdle. > > So summarizing we have: Iff you use --base with a non-public commit, it's Which is easily visible that it was not the case here. No human used format-patch thus no human used --base. > useless and irrelevant. I fully agree. Our conclusion is different > though. You accept it's useless (and even request from me that I do the > same), and I asked the submitter to use --base as intended to make the > resulting information usable. Because you expect additional steps for users of b4 and pointing in review standard use of b4 is extremely nitpicking. Best regards, Krzysztof
Hello Krzysztof, On Fri, Mar 07, 2025 at 08:07:42AM +0100, Krzysztof Kozlowski wrote: > On 07/03/2025 00:33, Uwe Kleine-König wrote: > > So summarizing we have: Iff you use --base with a non-public commit, it's > > Which is easily visible that it was not the case here. No human used > format-patch thus no human used --base. The human used b4 which used git format-patch --base (or something equivalent). It really doesn't matter that it was only used indirectly, my statement is true nevertheless. > > useless and irrelevant. I fully agree. Our conclusion is different > > though. You accept it's useless (and even request from me that I do the > > same), and I asked the submitter to use --base as intended to make the > > resulting information usable. > > Because you expect additional steps for users of b4 and pointing in > review standard use of b4 is extremely nitpicking. My request makes the difference between having build bot coverage and not having it. I consider that something important so I don't agree this to be nitpicking. Looking at https://lore.kernel.org/all/?q=prerequisite-patch-id+change-id it also doesn't seem soo unusual that people using b4 go through these additional steps. But I can hold out if you don't consider the result of patch checkers relevant. That's why I intend to stop participating in this discussion now. Best regards Uwe
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index 4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2..80130c205dce7c6ae1c356b7a7c5f6460a2b0bb0 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -523,8 +523,10 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period) return 0; } -static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty, uint64_t period) { + const unsigned int *pwm_resolution_arr; + unsigned int step; unsigned int max; unsigned int val; unsigned int clk_rate; @@ -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); chan->pwm_value = min(val, max); } @@ -829,7 +833,7 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev, lpg_calc_freq(chan, NSEC_PER_MSEC); duty = div_u64(brightness * chan->period, cdev->max_brightness); - lpg_calc_duty(chan, duty); + lpg_calc_duty(chan, duty, NSEC_PER_MSEC); chan->enabled = true; chan->ramp_enabled = false; @@ -906,7 +910,7 @@ static int lpg_blink_set(struct lpg_led *led, chan = led->channels[i]; lpg_calc_freq(chan, period); - lpg_calc_duty(chan, duty); + lpg_calc_duty(chan, duty, period); chan->enabled = true; chan->ramp_enabled = false; @@ -1241,7 +1245,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (ret < 0) goto out_unlock; - lpg_calc_duty(chan, state->duty_cycle); + lpg_calc_duty(chan, state->duty_cycle, state->period); } chan->enabled = state->enabled;
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. 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. So change the way the PWM value is determined as a ratio between the requested period and duty cycle, mapped on the resolution interval. Do this in contrast to using the register configuration selected when the best matching period was determined. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- This patch is based on the following patchset: https://lore.kernel.org/all/20250303-leds-qcom-lpg-fix-max-pwm-on-hi-res-v3-0-62703c0ab76a@linaro.org/ --- drivers/leds/rgb/leds-qcom-lpg.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) --- base-commit: 0067a4b21c9ab441bbe6bf3635b3ddd21f6ca7c3 change-id: 20250303-leds-qcom-lpg-compute-pwm-value-using-period-0823ceb7542a Best regards,