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>
In-Reply-To: <20200803151656.332559-1-cmo@melexis.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 3 Aug 2020 19:34:52 +0300
Message-ID: <CAHp75VfmSfmezqwwRfHZ797Y9rYDu3hgL5vGvPwbzGjCXsKWcQ@mail.gmail.com>
Subject: Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations
To:     Crt Mori <cmo@melexis.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

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?

...

> -       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).

...

> +       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