| Message ID | 20200803151656.332559-1-cmo@melexis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio:temperature:mlx90632: Reduce number of equal calulcations | expand |
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)
Hi Crt,
I love your patch! Yet something to improve:
[auto build test ERROR on iio/togreg]
[also build test ERROR on v5.8 next-20200803]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Crt-Mori/iio-temperature-mlx90632-Reduce-number-of-equal-calulcations/20200803-231936
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-a001-20200803 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4ffa6a27aca17fe88fa6bdd605b198df6632a570)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/iio/temperature/mlx90632.c:381:40: error: redefinition of 'TAdut4'
s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
^
drivers/iio/temperature/mlx90632.c:377:28: note: previous definition is here
s64 TAdut, s64 TAdut4, s32 Fa, s32 Fb,
^
>> drivers/iio/temperature/mlx90632.c:412:2: error: use of undeclared identifier 'TAdut4'; did you mean 'TAdut'?
TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
^~~~~~
TAdut
drivers/iio/temperature/mlx90632.c:405:17: note: 'TAdut' declared here
s64 kTA, kTA0, TAdut;
^
drivers/iio/temperature/mlx90632.c:419:67: error: use of undeclared identifier 'TAdut4'; did you mean 'TAdut'?
temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, TAdut4,
^~~~~~
TAdut
drivers/iio/temperature/mlx90632.c:405:17: note: 'TAdut' declared here
s64 kTA, kTA0, TAdut;
^
3 errors generated.
vim +/TAdut4 +381 drivers/iio/temperature/mlx90632.c
c87742abfc80b3 Crt Mori 2018-01-11 375
c87742abfc80b3 Crt Mori 2018-01-11 376 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object,
dd5b04efd05f22 Crt Mori 2020-08-03 377 s64 TAdut, s64 TAdut4, s32 Fa, s32 Fb,
c87742abfc80b3 Crt Mori 2018-01-11 378 s32 Ga, s16 Ha, s16 Hb,
c87742abfc80b3 Crt Mori 2018-01-11 379 u16 emissivity)
c87742abfc80b3 Crt Mori 2018-01-11 380 {
c87742abfc80b3 Crt Mori 2018-01-11 @381 s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
c87742abfc80b3 Crt Mori 2018-01-11 382 s64 Ha_customer, Hb_customer;
c87742abfc80b3 Crt Mori 2018-01-11 383
dd5b04efd05f22 Crt Mori 2020-08-03 384 Ha_customer = div64_s64((s64)Ha * 1000000LL, 16384);
dd5b04efd05f22 Crt Mori 2020-08-03 385 Hb_customer = div64_s64((s64)Hb * 100, 1024);
c87742abfc80b3 Crt Mori 2018-01-11 386
dd5b04efd05f22 Crt Mori 2020-08-03 387 calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
dd5b04efd05f22 Crt Mori 2020-08-03 388 * 1000LL), 68719476736);
dd5b04efd05f22 Crt Mori 2020-08-03 389 calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 1000000LL)), 68719476736);
dd5b04efd05f22 Crt Mori 2020-08-03 390 Alpha_corr = div64_s64(div64_s64((s64)(Fa * 10000000000LL), 70368744177664)
dd5b04efd05f22 Crt Mori 2020-08-03 391 * Ha_customer, 1000LL);
c87742abfc80b3 Crt Mori 2018-01-11 392 Alpha_corr *= ((s64)(1 * 1000000LL + calcedKsTO + calcedKsTA));
c87742abfc80b3 Crt Mori 2018-01-11 393 Alpha_corr = emissivity * div64_s64(Alpha_corr, 100000LL);
c87742abfc80b3 Crt Mori 2018-01-11 394 Alpha_corr = div64_s64(Alpha_corr, 1000LL);
c87742abfc80b3 Crt Mori 2018-01-11 395 ir_Alpha = div64_s64((s64)object * 10000000LL, Alpha_corr);
c87742abfc80b3 Crt Mori 2018-01-11 396
c87742abfc80b3 Crt Mori 2018-01-11 397 return (int_sqrt64(int_sqrt64(ir_Alpha * 1000000000000LL + TAdut4))
c87742abfc80b3 Crt Mori 2018-01-11 398 - 27315 - Hb_customer) * 10;
c87742abfc80b3 Crt Mori 2018-01-11 399 }
c87742abfc80b3 Crt Mori 2018-01-11 400
c87742abfc80b3 Crt Mori 2018-01-11 401 static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
c87742abfc80b3 Crt Mori 2018-01-11 402 s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
c87742abfc80b3 Crt Mori 2018-01-11 403 u16 tmp_emi)
c87742abfc80b3 Crt Mori 2018-01-11 404 {
c87742abfc80b3 Crt Mori 2018-01-11 405 s64 kTA, kTA0, TAdut;
c87742abfc80b3 Crt Mori 2018-01-11 406 s64 temp = 25000;
c87742abfc80b3 Crt Mori 2018-01-11 407 s8 i;
c87742abfc80b3 Crt Mori 2018-01-11 408
c87742abfc80b3 Crt Mori 2018-01-11 409 kTA = (Ea * 1000LL) >> 16LL;
c87742abfc80b3 Crt Mori 2018-01-11 410 kTA0 = (Eb * 1000LL) >> 8LL;
c87742abfc80b3 Crt Mori 2018-01-11 411 TAdut = div64_s64(((ambient - kTA0) * 1000000LL), kTA) + 25 * 1000000LL;
dd5b04efd05f22 Crt Mori 2020-08-03 @412 TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
dd5b04efd05f22 Crt Mori 2020-08-03 413 (div64_s64(TAdut, 10000LL) + 27315) *
dd5b04efd05f22 Crt Mori 2020-08-03 414 (div64_s64(TAdut, 10000LL) + 27315) *
dd5b04efd05f22 Crt Mori 2020-08-03 415 (div64_s64(TAdut, 10000LL) + 27315);
c87742abfc80b3 Crt Mori 2018-01-11 416
c87742abfc80b3 Crt Mori 2018-01-11 417 /* Iterations of calculation as described in datasheet */
c87742abfc80b3 Crt Mori 2018-01-11 418 for (i = 0; i < 5; ++i) {
dd5b04efd05f22 Crt Mori 2020-08-03 419 temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, TAdut4,
c87742abfc80b3 Crt Mori 2018-01-11 420 Fa, Fb, Ga, Ha, Hb,
c87742abfc80b3 Crt Mori 2018-01-11 421 tmp_emi);
c87742abfc80b3 Crt Mori 2018-01-11 422 }
c87742abfc80b3 Crt Mori 2018-01-11 423 return temp;
c87742abfc80b3 Crt Mori 2018-01-11 424 }
c87742abfc80b3 Crt Mori 2018-01-11 425
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c index eaca6ba06864..d7ba0b2fe3c0 100644 --- a/drivers/iio/temperature/mlx90632.c +++ b/drivers/iio/temperature/mlx90632.c @@ -374,29 +374,25 @@ static s32 mlx90632_calc_temp_ambient(s16 ambient_new_raw, s16 ambient_old_raw, } static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object, - s64 TAdut, s32 Fa, s32 Fb, + s64 TAdut, s64 TAdut4, s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb, u16 emissivity) { s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr; s64 Ha_customer, Hb_customer; - 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); - 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); Alpha_corr *= ((s64)(1 * 1000000LL + calcedKsTO + calcedKsTA)); Alpha_corr = emissivity * div64_s64(Alpha_corr, 100000LL); Alpha_corr = div64_s64(Alpha_corr, 1000LL); ir_Alpha = div64_s64((s64)object * 10000000LL, Alpha_corr); - TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) * - (div64_s64(TAdut, 10000LL) + 27315) * - (div64_s64(TAdut, 10000LL) + 27315) * - (div64_s64(TAdut, 10000LL) + 27315); return (int_sqrt64(int_sqrt64(ir_Alpha * 1000000000000LL + TAdut4)) - 27315 - Hb_customer) * 10; @@ -413,10 +409,14 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb, kTA = (Ea * 1000LL) >> 16LL; kTA0 = (Eb * 1000LL) >> 8LL; TAdut = div64_s64(((ambient - kTA0) * 1000000LL), kTA) + 25 * 1000000LL; + TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) * + (div64_s64(TAdut, 10000LL) + 27315) * + (div64_s64(TAdut, 10000LL) + 27315) * + (div64_s64(TAdut, 10000LL) + 27315); /* Iterations of calculation as described in datasheet */ for (i = 0; i < 5; ++i) { - temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, + temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, TAdut4, Fa, Fb, Ga, Ha, Hb, tmp_emi); }
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. Also converted shifts in this function of signed integers to divisions as that is less implementation-defined behavior. Signed-off-by: Crt Mori <cmo@melexis.com> --- drivers/iio/temperature/mlx90632.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)