Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations

[Andy Shevchenko]

On Mon, Aug 3, 2020 at 6:17 PM Crt Mori <cmo@xxxxxxxxxxx> 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