Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations
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 wrote: > > On Mon, Aug 3, 2020 at 6:17 PM Crt Mori 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 * 100LL) >> 14ULL; > > - Hb_customer = ((s64)Hb * 100) >> 10ULL; > > + Ha_customer = div64_s64((s64)Ha * 100LL, 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 * 100LL))) >> 36LL; > > - Alpha_corr = div64_s64s64)(Fa * 100LL) >> 46LL) > > - * Ha_customer), 1000LL); > > > + calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * > > 1000LL) > > +* 1000LL), 68719476736); > > + calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 100LL)), > > 68719476736); > > + Alpha_corr = div64_s64(div64_s64((s64)(Fa * 100LL), > > 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, 1LL) + 27315) * > > + (div64_s64(TAdut, 1LL) + 27315) * > > + (div64_s64(TAdut, 1LL) + 27315) * > > + (div64_s64(TAdut, 1LL) + 27315); > > Shouldn't you switch to definitions from units.h? (perhaps as a separate > change) > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations
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
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, 1LL) + 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
* 100LL, 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 * 100LL)), 68719476736);
dd5b04efd05f22 Crt Mori 2020-08-03 390 Alpha_corr =
div64_s64(div64_s64((s64)(Fa * 100LL), 70368744177664)
dd5b04efd05f22 Crt Mori 2020-08-03 391*
Ha_customer, 1000LL);
c87742abfc80b3 Crt Mori 2018-01-11 392 Alpha_corr *= ((s64)(1 *
100LL + calcedKsTO + calcedKsTA));
c87742abfc80b3 Crt Mori 2018-01-11 393 Alpha_corr = emissivity *
div64_s64(Alpha_corr, 10LL);
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 * 1000LL, Alpha_corr);
c87742abfc80b3 Crt Mori 2018-01-11 396
c87742abfc80b3 Crt Mori 2018-01-11 397 return
(int_sqrt64(int_sqrt64(ir_Alpha * 1LL + 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
Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations
On Mon, Aug 3, 2020 at 6:17 PM Crt Mori 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 * 100LL) >> 14ULL; > - Hb_customer = ((s64)Hb * 100) >> 10ULL; > + Ha_customer = div64_s64((s64)Ha * 100LL, 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 * 100LL))) >> 36LL; > - Alpha_corr = div64_s64s64)(Fa * 100LL) >> 46LL) > - * Ha_customer), 1000LL); > + calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * > 1000LL) > +* 1000LL), 68719476736); > + calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 100LL)), > 68719476736); > + Alpha_corr = div64_s64(div64_s64((s64)(Fa * 100LL), > 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, 1LL) + 27315) * > + (div64_s64(TAdut, 1LL) + 27315) * > + (div64_s64(TAdut, 1LL) + 27315) * > + (div64_s64(TAdut, 1LL) + 27315); Shouldn't you switch to definitions from units.h? (perhaps as a separate change) -- With Best Regards, Andy Shevchenko
[PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations
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
---
drivers/iio/temperature/mlx90632.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
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 * 100LL) >> 14ULL;
- Hb_customer = ((s64)Hb * 100) >> 10ULL;
+ Ha_customer = div64_s64((s64)Ha * 100LL, 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 * 100LL))) >> 36LL;
- Alpha_corr = div64_s64s64)(Fa * 100LL) >> 46LL)
- * Ha_customer), 1000LL);
+ calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
+* 1000LL), 68719476736);
+ calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 100LL)),
68719476736);
+ Alpha_corr = div64_s64(div64_s64((s64)(Fa * 100LL),
70368744177664)
+ * Ha_customer, 1000LL);
Alpha_corr *= ((s64)(1 * 100LL + calcedKsTO + calcedKsTA));
Alpha_corr = emissivity * div64_s64(Alpha_corr, 10LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
ir_Alpha = div64_s64((s64)object * 1000LL, Alpha_corr);
- TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
- (div64_s64(TAdut, 1LL) + 27315) *
- (div64_s64(TAdut, 1LL) + 27315) *
- (div64_s64(TAdut, 1LL) + 27315);
return (int_sqrt64(int_sqrt64(ir_Alpha * 1LL + 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) * 100LL), kTA) + 25 * 100LL;
+ TAdut4 = (div64_s64(TAdut, 1LL) + 27315) *
+ (div64_s64(TAdut, 1LL) + 27315) *
+ (div64_s64(TAdut, 1LL) + 27315) *
+ (div64_s64(TAdut, 1LL) + 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);
}
--
2.25.1
