Hi! I noticed that I have not reviewed this patch. Sorry for my low bandwidth. On 2022-01-30 17:10, Liam Beguin wrote: > Make use of well-defined SI metric prefixes to improve code readability. > > Signed-off-by: Liam Beguin <liambeguin@xxxxxxxxx> > --- > drivers/iio/afe/iio-rescale.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 67273de46843..27c6664915ff 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -51,11 +51,11 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > } > fallthrough; > case IIO_VAL_FRACTIONAL_LOG2: > - tmp = (s64)*val * 1000000000LL; > + tmp = (s64)*val * GIGA; > tmp = div_s64(tmp, rescale->denominator); > tmp *= rescale->numerator; > > - tmp = div_s64_rem(tmp, 1000000000LL, &rem); > + tmp = div_s64_rem(tmp, GIGA, &rem); It is NOT easy for me to say which of GIGA/NANO is most fitting. There are a couple of considerations: A) 1000000000 is just a big value (GIGA fits). Something big is needed to not lose too much precision. B) 1000000000 is what the IIO core uses to print fractional-log values with nano precision (NANO fits). This is not really relevant in this context. C) 1000000000 makes the int-plus-nano and fractional-log cases align (NANO fits). This last consideration is introduced with patch 4/11. There is simply no correct define to use. And whichever define is chosen makes the other interpretation less obvious. Which is not desirable, obscures things and make both GIGA and NANO bad options. So, I stepped back to the description provided by Andy in the comments of v11: On 2021-12-22 19:59, Andy Shevchenko wrote: | You should get the proper power after the operation. | Write a formula (mathematically speaking) and check each of them for this. | | 10^-5/10^-9 == 1*10^4 (Used NANO) | 10^-5/10^9 == 1/10^-14 (Used GIGA) | | See the difference? No, I don't really see the difference, that just makes me totally confused. Dividing by 10^-9 or multiplying by 10^9 is as we all know exactly the same, and the kernel cannot deal directly with 10^-9 so both will look the same in code (multiplying by 10^9). So, you must be referring to the "real formula" behind the code. But in that case, if the "real formula" behind the (then equivalent) code had instead been 10^-5*10^9 == 1*10^4 (Used GIGA) 10^-5*10^-9 == 1/10^-14 (Used NANO) the outcome is the opposite. NANO turns GIGA and vice versa. Since you can express the same thing differently in math too, it all crumbles for me. Because of this duality, it will be a matter of taste if GIGA or NANO fits best in any given instance. Sometimes (perhaps commonly) it will be decidedly easy to pick one of them, but in other cases (see above) we will end up with a conflict. What to do then? Or, what am I missing? My taste says NANO in this case, since A) is just some big number and not really about units and B) is as stated not really relevant. Which makes C) win the argument for me. > *val = tmp; > > if (!rem) > @@ -71,7 +71,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > > *val2 = rem / (int)tmp; > if (rem2) > - *val2 += div_s64((s64)rem2 * 1000000000LL, tmp); > + *val2 += div_s64((s64)rem2 * GIGA, tmp); Here, 1000000000 matches the above use. If we go with NANO above, we should go with NANO here as well. > return IIO_VAL_INT_PLUS_NANO; > case IIO_VAL_INT_PLUS_NANO: > @@ -332,8 +332,8 @@ static int rescale_current_sense_amplifier_props(struct device *dev, > * gain_div / (gain_mult * sense), while trying to keep the > * numerator/denominator from overflowing. > */ > - factor = gcd(sense, 1000000); > - rescale->numerator = 1000000 / factor; > + factor = gcd(sense, MEGA); > + rescale->numerator = MEGA / factor; Here, the 1000000 number comes from the unit of the sense resistor (micro-ohms), so I would have preferred MICRO. But who can tell if we -mathematically speaking- have divided the given resistance integer by 10^6 (MEGA) or multiplied it with 10^-6 (MICRO) to account for the unit? Or if we divided the other values with 10^6 (MEGA) (or multiplied by 10^-6, MICRO) to make them fit the unit of the shunt resistance? All of the above is of course equivalent so both MEGA and MICRO are correct. But as stated, MICRO makes to most sense as that is what connects the code to reality and hints at where the value is coming from. For me anyway. > rescale->denominator = sense / factor; > > factor = gcd(rescale->numerator, gain_mult); > @@ -361,8 +361,8 @@ static int rescale_current_sense_shunt_props(struct device *dev, > return ret; > } > > - factor = gcd(shunt, 1000000); > - rescale->numerator = 1000000 / factor; > + factor = gcd(shunt, MEGA); > + rescale->numerator = MEGA / factor; Same here, 1000000 comes from the micro-ohms unit of the shunt resistor, so I would have preferred MICRO. Sorry for the long mail. I blame the duality of these ambiguous SI-defines that are a bit confusing to me. Cheers, Peter > rescale->denominator = shunt / factor; > > return 0;
