Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200803151656.332559-1-cmo@melexis.com> <CAHp75VfmSfmezqwwRfHZ797Y9rYDu3hgL5vGvPwbzGjCXsKWcQ@mail.gmail.com>
In-Reply-To: <CAHp75VfmSfmezqwwRfHZ797Y9rYDu3hgL5vGvPwbzGjCXsKWcQ@mail.gmail.com>
From:   Crt Mori <cmo@melexis.com>
Date:   Tue, 4 Aug 2020 09:57:38 +0200
Message-ID: <CAKv63uuFNOksmDUKEapvX60gg9QE+32Dak_2=M2cYeueFiHjPg@mail.gmail.com>
Subject: Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Andy,
Thanks for the comments. This is indeed a cut-out section of what I
wanted to submit next.

On Mon, 3 Aug 2020 at 18:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Mon, Aug 3, 2020 at 6:17 PM Crt Mori <cmo@melexis.com> wrote:
> >
> > TAdut4 was calculated each iteration although it did not change. In light
> > of near future additions of the Extended range DSP calculations, this
> > function refactoring will help reduce unrelated changes in that series as
> > well as reduce the number of new functions needed.
>
> Okay!
>
> > Also converted shifts in this function of signed integers to divisions as
> > that is less implementation-defined behavior.
>
> This is what I'm wondering about. Why?
>
> ...

The reason for this is that whenever something is wrong with the
calculation I am looking into the shifts which are
implementation-defined and might not keep the signed bit. Division
however would.

>
> > -       Ha_customer = ((s64)Ha * 1000000LL) >> 14ULL;
> > -       Hb_customer = ((s64)Hb * 100) >> 10ULL;
> > +       Ha_customer = div64_s64((s64)Ha * 1000000LL, 16384);
> > +       Hb_customer = div64_s64((s64)Hb * 100, 1024);
>
> Have you checked the code on 32-bit machines?
> As far as I can see the div64_*64() do not have power of two divisor
> optimizations. I bet it will generate a bulk of unneeded code.
>
> ...
>
> > -       calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
> > -                            * 1000LL)) >> 36LL;
> > -       calcedKsTA = ((s64)(Fb * (TAdut - 25 * 1000000LL))) >> 36LL;
> > -       Alpha_corr = div64_s64((((s64)(Fa * 10000000000LL) >> 46LL)
> > -                               * Ha_customer), 1000LL);
>
> > +       calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
> > +                                    * 1000LL), 68719476736);
> > +       calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 1000000LL)), 68719476736);
> > +       Alpha_corr = div64_s64(div64_s64((s64)(Fa * 10000000000LL), 70368744177664)
> > +                              * Ha_customer, 1000LL);
>
> This is less readable and full of magic numbers in comparison to the
> above (however, also full of magics, but at least gives better hint).
>
> ...

These are coefficients so there is not much to unmagic. I can keep the
shifts, if you think that is more readable or add comments after lines
with 2^46 or something?
>
> > +       TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
> > +               (div64_s64(TAdut, 10000LL) + 27315) *
> > +               (div64_s64(TAdut, 10000LL)  + 27315) *
> > +               (div64_s64(TAdut, 10000LL) + 27315);
>
> Shouldn't you switch to definitions from units.h? (perhaps as a separate change)
>
> --
> With Best Regards,
> Andy Shevchenko