Messages in this thread | | | Date | Mon, 19 Nov 2018 11:14:22 +0000 | From | Priit Laes <> | Subject | Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library |
| |
On Mon, Nov 19, 2018 at 12:43:32PM +0200, Kalle Valo wrote: > Larry Finger <Larry.Finger@lwfinger.net> writes: > > >>>> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max, > >>>> angle = 0; > >>>> for (i = 0; i < len; i++) { > >>>> - samples[i] = b43_cordic(angle); > >>>> + samples[i] = cordic_calc_iq(angle); > >>>> angle += rot; > >>>> - samples[i].q = CORDIC_CONVERT(samples[i].q * max); > >>>> - samples[i].i = CORDIC_CONVERT(samples[i].i * max); > >>>> + samples[i].q = CORDIC_FLOAT(samples[i].q * max); > >>>> + samples[i].i = CORDIC_FLOAT(samples[i].i * max); > >>>> } > >>>> i = b43_nphy_load_samples(dev, samples, len); > >>> > >>> There is a fundamental flaw in this patch. Routine b43_cordic() takes an > >>> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in > >>> degrees. For a given input, the two routines must get different answers. At > >>> a minimum, the calculation of rot would need to remove the left shift of 16. > >> > >> Thanks for the hint. I modified my "test harness" a bit to plot out values > >> from -360 .. 360 and transformed the theta for b43_cordic argument > >> using CORDIC_FIXED macro: > >> > >> b43_cordic(CORDIC_FIXED(theta)); > >> cordic_calc_iq(theta); > >> > >> Then I plotted the results and well.. they are not that pretty. > >> While the results give > >> identical answers between certain ranges of degrees, the cordic > >> algorithm for b43 seems > >> to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360). > >> > >> You can find my test harnesses here: > >> > >> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874 > > > > I found a problem with the b43 implementation. The local variables for > > that routine includes > > > > u32 angle = 0; > > > > If one looks further down in the algorithm, if the reduced value of > > "theta" is less than 0, then "angle" should be negative. That causes > > the calculation to blow up. This explains why some ranges of angles > > got the same result for both routines. When that declaration is > > changed to "int angle = 0", the two routines give the same answer for > > all inputs. > > > > My test setup has a hardware failure, thus I cannot test your patch, > > but I now believe it to be correct. Thus your first and third patches > > may be annotated with > > ACKed-by: Larry Finger <Larry.Finger@lwfinger.net> > > > > One thing that should be done is to fix the error in the b43 code for > > stable as it was introduced in 2.6.34. I propose adding the attached > > patched to your series placed between your current 2nd and 3rd patches > > so that the old kernels get fixed. Of course, your 3rd patch will need > > to be revised. If all 4 of the patches get submitted together there > > will be no problems with the timing. My change will exist for seconds > > in the mainline kernel, but it will get propagated back through > > stable. > > Sorry Larry, I'm not fully understanding what you mean here. So I'm > going to just drop the whole series and assume that Priit will submit a > new version. Please let me know if I should do something else.
Yes, drop this one and I will submit v4 with one extra patch fixing the cordic algorithm in the stable kernel.
> > -- > Kalle Valo
| |