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
Attachment:
signature.asc
Description: PGP signature
