Messages in this thread | | | From | Arnd Bergmann <> | Date | Fri, 3 Apr 2020 21:13:18 +0200 | Subject | Re: [PATCH v11 06/12] pwm: imx27: Use 64-bit division macro and function |
| |
On Fri, Apr 3, 2020 at 7:37 PM Guru Das Srinagesh <gurus@codeaurora.org> wrote: > > On Thu, Apr 02, 2020 at 11:16:22PM +0200, Arnd Bergmann wrote: > > This looks correct, but very expensive, and you don't really have to > > go this far, given that c1 is guaranteed to be a 32-bit number, and > > you divide by a constant in the end. > > > > Why not do something like > > > > #define SHIFT 41 /* arbitrarily picked, not too big, not too small */ > > #define MUL 2199 /* 2^SHIFT / NSEC_PER_SEC */ > > period_cycles = clk_get_rate(imx->clk_per) * ((state->period * MUL) >> SHIFT); > > I have two concerns with this: > > 1. This actually results in the division by 1000010575.5125057 instead > of NSECS_PER_SEC whereas both the existing as well as the proposed logic > divide exactly by NSECS_PER_SEC. > 2. What method shall be used to pick the SHIFT value? How is this to be > chosen?
I picked one that would result in a fairly accurate rounding and did not seem too likely to overflow.
> Also, this seems sort of similar to my initial attempt at this > problem, where period was being pre-divided prior to the multiplication, > which was (rightly) NACKed. > > c *= div_u64(state->period, 1000000000);
Ah, right, I did make the same mistake here. What I actually indended was to do two separate shifts that add up to 41. I suppose there is also still the fundamental problem that without bounds checking the calculation can always overflow for large inputs.
Arnd
| |