On Wed, 25 Mar 2020 23:08:17 +0530 DEEPAK VARMA <mh12gx2825@xxxxxxxxx> wrote: > On Tue, Mar 24, 2020 at 08:06:34AM +0000, Ardelean, Alexandru wrote: > > On Mon, 2020-03-23 at 23:22 +0530, DEEPAK VARMA wrote: > > > [External] > > > > > > On Mon, Mar 23, 2020 at 01:15:31PM +0100, Stefano Brivio wrote: > > > > On Mon, 23 Mar 2020 11:28:52 +0200 > > > > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > > > > > > > > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@xxxxxxxxxx> wrote: > > > > > > On Mon, 23 Mar 2020 01:44:20 +0200 > > > > > > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > > > > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@xxxxxxxxx> > > > > > > > wrote: > > > > > > > > Current implementation of the function > > > > > > > > ad7192_get_available_filter_freq > > > > > > > > repeats calculation of output data rate a few times. We can simplify > > > > > > > > these steps by refactoring out the calculation of fADC. This would > > > > > > > > also > > > > > > > > addresses the checkpatch warning of line exceeding 80 character. > > > > > > > > > > > > > > I'm not sure you did an equivalent changes. I believe in the original > > > > > > > code precision is better. Consider low clock frequencies when 10 bit > > > > > > > right shift may hide some bits of the division. > > > > > > > > > > > > Note that those bits are eventually "hidden" in the same way later, > > > > > > > > > > Even if mathematically (arithmetically) evaluation is correct, we have > > > > > to remember that computers are bad with floating point and especially > > > > > kernel, which uses integer arithmetic. That said, it's easy to get > > > > > off-by-one error (due to precision lost) if we do big division before > > > > > (not so big) multiplication. > > > > > > > > That's exactly the point I was trying to explain below: swapping steps > > > > in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions), > > > > *should* not affect quantisation ("off-by-one") error. > > > > > > > > I'm not entirely sure in this case, so a quick "demonstration" in > > > > Python or suchlike as you suggested would be nice to have, indeed. > > > > > > > > > > despite the different sequence, due to DIV_ROUND_CLOSEST() being used > > > > > > at every step (both before and after the change) without other > > > > > > operations occurring. > > > > > > > > > > By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER > > > > > multiplications disappear and why? > > > > > > > > Those were in fact divisions (multiplications of the divisor). Overall, > > > > these steps are now arranged in a way closer to how they are presented > > > > in the datasheet mentioned here (up to "Chop Enabled" paragraph, page > > > > 26). > > > > > > > > > > Thank you Andy and Stefano for your comments. Its very thoughtful. I am > > > not much familiar with Python so far, but thinking on evaluating your > > > suggestion in a sample c program. I will share the outcome shortly. > > > > +adding Mircea Caprioru > > > > Umm, this math-cleanup looks pretty dangerous. > > If possible, I wouldn't change it. > > At least without some testing on HW. > > > > Maybe doing math-simulations in Python scripts would also work, but not sure. > > > > Hello All, > I further reviewed current and proposed implementation of the > get_filter_freq() function[Thank you Stefano for your time]. We realised that I > was wrong in swapping DIV_ROUND_CLOSEST calls with mixing > multiplication in it. It is indeed incorrect to mix multiplication if we > want to reorder the calls. Specifically, my wrong assumption was that we were just reordering DIV_ROUND_CLOSEST() operations -- that's actually fine: with rounding, integer division (not mixed with other operations) is associative. However, here, we had those 0.23, 0.24, 0.272 factors. I missed them. If we factor together these multiplications and do them first, of course, the result is more accurate, which, I assume from the usage of DIV_ROUND_CLOSEST() and the typical application of the ADC, is quite relevant here. The observation here is that, by doing all of them first, the code is more accurate (e.g. with a fclk of 2458464 and 904 as "mode" we get 722 for freq[4] instead of 723 -- 722.377747 with real division) and actually becomes more readable, too. > [...] > > May I please request you to review the attached test program, verify the > results and share your feedback. As an alternative to what Andy suggested, I guess you could also post it inline, just like we do for patch reviews. Commenting becomes natural and the discussion can be referenced later via archives. -- Stefano