Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 25 Mar 2020 23:08:17 +0530
DEEPAK VARMA <mh12gx2825@gmail.com> 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@gmail.com> wrote:
> > > >   
> > > > > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:  
> > > > > > On Mon, 23 Mar 2020 01:44:20 +0200
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:    
> > > > > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com>
> > > > > > > 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