| Message ID | 20220130161101.1067691-7-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
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@gmail.com> > --- > 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;
On Mon, Jan 31, 2022 at 4:50 PM Peter Rosin <peda@axentia.se> wrote: > On 2022-01-30 17:10, Liam Beguin wrote: ... > > - 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. What do you mean? The idea behind is the use of the macro depending on the actual power of 10 in use (taking into account the sign of the exponent part). > 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. Does it have a physical meaning? > 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. Same question. > 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. Huh?! I believe the answer to the A and B -- yes, which means there are the correct and incorrect variants 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. The (main) idea of the macros is to avoid mistyping 0s in the numbers and miscalculations. Besides that it also provides the same type for the constants. > 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. Sounds like we have a chat here between physicists and computer science folks :-) Let's try again, does the value in the tmp variable above have a _physical_ meaning? (I believe so, because all IIO subsystem is about physical values) > 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). Yes, and using proper macro makes much cleaner the mathematical and physical point of view to the values. > 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. Again, one needs to think carefully about the meaning. That's the point. If we do not understand the proper values and their scales, perhaps that is the issue we should solve first? > 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? Physical meaning of the values, certainly. > 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. ... > > *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. Why? (Maybe you are right, maybe not) ... > SI-defines that are a bit confusing to me. Yeah, people hate mathematics at scholls, at university, in life...
On 2022-01-31 16:23, Andy Shevchenko wrote: > On Mon, Jan 31, 2022 at 4:50 PM Peter Rosin <peda@axentia.se> wrote: >> On 2022-01-30 17:10, Liam Beguin wrote: > > ... > >>> - 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. > > What do you mean? The idea behind is the use of the macro depending on > the actual power of 10 in use (taking into account the sign of the > exponent part). > >> 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. > > Does it have a physical meaning? No, this is just a scaling factor which is moments later eliminted by a matching inverse operation. It's math purely about attempting to preserve precision and has nothing to do with the units of the values that are involved. >> 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. > > Same question. No, in the context of B) it's also just math without any physical quantity in the back seat. The iio core prints fractional-log *values* (of any and all units) with "nano precision" i.e. nine decimal digits. >> 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. > > Huh?! What I meant originally by "no correct define to use" is that there are arguments for both GIGA and for NANO and that if both are not wrong, then none of them is /the/ correct define to use. Now that you put the focus on physical quantities, there are other ascpects, see below. > I believe the answer to the A and B -- yes, which means there are the > correct and incorrect variants to use. This doesn't really parse for me. What question is "yes" answering? What is it that you think is correct and what is incorrect? I.e. what is your conclusion? Do you think NANO or GIGA fits best? Why? >> 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. > > The (main) idea of the macros is to avoid mistyping 0s in the numbers > and miscalculations. Besides that it also provides the same type for > the constants. Yes, but I wonder if it is worth the time to work out what the correct defines to use actually are. >> 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. > > Sounds like we have a chat here between physicists and computer > science folks :-) I don't get what is supposed to be funny. Which would I be and why? I of course saw the difference in exponents, what I don't see is why dividing with 10^-9 makes the result NANO while multiplying with 10^9 (presumably) makes the result GIGA. And yes, sigh, I do see that you have divisions above and not any multiplication, but what's confusing me is what happens when I extend your example with the multiplications I added below. I simply cannot make up clear rules with only numbers to go by. I for one need the units to make the call. But we have units neither here nor in the math the code is implementing, it's only plain numbers (or numbers with unknown units, or ops where the unit is of no consequence). > Let's try again, does the value in the tmp variable above have a > _physical_ meaning? (I believe so, because all IIO subsystem is about > physical values) See above. >> 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). > > Yes, and using proper macro makes much cleaner the mathematical and > physical point of view to the values. Only if you select the correct and relevant define. Otherwise they make things random and confusing. You can't go hunt for a define that happens to match the value you need and then argue it's a good idea to use it based on that. And this borders to that, because in 3 of 5 cases there are no units involved and the defines are about unit prefixes. In the remaining two cases (that you elided) the actual unit prefix involved (from micro-ohms) is not considered and MEGA is used instead of MICRO. >> 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. > > Again, one needs to think carefully about the meaning. > That's the point. If we do not understand the proper values and their > scales, perhaps that is the issue we should solve first? Oh, I have a good understanding of how this driver works and where the numbers are coming from. I have no issues to solve in that department. And I like to keep it that way, so I want to understand which of GIGA and NANO is better than the other, and why. For each instance. >> 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? > > Physical meaning of the values, certainly. Not helpful. You seem to be under the impression that all 10^x numbers are somehow used because there is a unit connected to some other number. That's simply not always the case. >> 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. > > ... > >>> *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. > > Why? (Maybe you are right, maybe not) Right, that's arguably a mistake. Here, 1000000000 is simply the number that's needed to adjust the 9 decimal digits for the int-plus-nano return value, whatever unit that int-plus-nano value has. So, there is only a match with the above from the prespective of C). But regardless, it would feel extremely odd to use one of the GIGA/NANO defines above, and the other here. > ... > >> SI-defines that are a bit confusing to me. > > Yeah, people hate mathematics at scholls, at university, in life... You seem to imply something. What, exactly? Care to spell it out? Cheers, Peter
On Tue, Feb 1, 2022 at 3:59 AM Peter Rosin <peda@axentia.se> wrote: > On 2022-01-31 16:23, Andy Shevchenko wrote: > > On Mon, Jan 31, 2022 at 4:50 PM Peter Rosin <peda@axentia.se> wrote: > >> On 2022-01-30 17:10, Liam Beguin wrote: > > > > ... > > > >>> - 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. > > > > What do you mean? The idea behind is the use of the macro depending on > > the actual power of 10 in use (taking into account the sign of the > > exponent part). > > > >> 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. > > > > Does it have a physical meaning? > > No, this is just a scaling factor which is moments later > eliminted by a matching inverse operation. It's math purely > about attempting to preserve precision and has nothing to do > with the units of the values that are involved. I see your point now, shame on me.
Hi Peter, On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote: > 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@gmail.com> > > --- > > 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: I agree with you that the choice behind GIGA/NANO can be a bit confusing. In my opinion, these defines makes the code easier to read if you consider them as multipliers with no physical meaning, basically a pretty name for a power of 10. By this logic, we wouldn't ever use FEMTO to DECI. Cheers, Liam > 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;
On Tue, 1 Feb 2022 14:28:28 -0500 Liam Beguin <liambeguin@gmail.com> wrote: > Hi Peter, > > On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote: > > 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@gmail.com> > > > --- > > > 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: > > I agree with you that the choice behind GIGA/NANO can be a bit > confusing. > > In my opinion, these defines makes the code easier to read if you > consider them as multipliers with no physical meaning, basically a > pretty name for a power of 10. > > By this logic, we wouldn't ever use FEMTO to DECI. Not sure if it would help but maybe it's worth a local define of something like #define MULT9 1000000000LL to loose the association with any particular SI basis and just indicate it's a bit number being used to retain precision in some maths? Would need a comment to stop people sending patches to replace it with GIGA though ;) My ultimate preference here is for whatever works for Peter and Liam as the people who are mostly likely to have to deal with any changes to this driver in the future. Jonathan > > Cheers, > Liam > > > 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;
On Sat, Feb 5, 2022 at 7:47 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 1 Feb 2022 14:28:28 -0500 > Liam Beguin <liambeguin@gmail.com> wrote: ... > Not sure if it would help but maybe it's worth a local define > of something like > > #define MULT9 1000000000LL > to loose the association with any particular SI basis and > just indicate it's a bit number being used to retain precision > in some maths? Would need a comment to stop people sending > patches to replace it with GIGA though ;) > > My ultimate preference here is for whatever works for Peter and > Liam as the people who are mostly likely to have to deal > with any changes to this driver in the future. SI multipliers are for values with the physical meaning. While Peter found them confusing, it's a pretty much win in my opinion when we talk about data from nature. For the pure mathematical scale it may be confusing.
On Sat, Feb 05, 2022 at 05:54:04PM +0000, Jonathan Cameron wrote: > On Tue, 1 Feb 2022 14:28:28 -0500 > Liam Beguin <liambeguin@gmail.com> wrote: > > > Hi Peter, > > > > On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote: > > > 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@gmail.com> > > > > --- > > > > 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: > > > > I agree with you that the choice behind GIGA/NANO can be a bit > > confusing. > > > > In my opinion, these defines makes the code easier to read if you > > consider them as multipliers with no physical meaning, basically a > > pretty name for a power of 10. > > > > By this logic, we wouldn't ever use FEMTO to DECI. > > Not sure if it would help but maybe it's worth a local define > of something like > > #define MULT9 1000000000LL > to loose the association with any particular SI basis and > just indicate it's a bit number being used to retain precision > in some maths? Would need a comment to stop people sending > patches to replace it with GIGA though ;) > > My ultimate preference here is for whatever works for Peter and > Liam as the people who are mostly likely to have to deal > with any changes to this driver in the future. Hi Jonathan, My preference here is to keep GIGA, if it makes everyone more comfortable, I can add a comment explaing the intention of the multiplication? Cheers, Liam > Jonathan > > > > > > Cheers, > > Liam > > > > > 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; >
On Mon, 7 Feb 2022 09:05:11 -0500 Liam Beguin <liambeguin@gmail.com> wrote: > On Sat, Feb 05, 2022 at 05:54:04PM +0000, Jonathan Cameron wrote: > > On Tue, 1 Feb 2022 14:28:28 -0500 > > Liam Beguin <liambeguin@gmail.com> wrote: > > > > > Hi Peter, > > > > > > On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote: > > > > 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@gmail.com> > > > > > --- > > > > > 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: > > > > > > I agree with you that the choice behind GIGA/NANO can be a bit > > > confusing. > > > > > > In my opinion, these defines makes the code easier to read if you > > > consider them as multipliers with no physical meaning, basically a > > > pretty name for a power of 10. > > > > > > By this logic, we wouldn't ever use FEMTO to DECI. > > > > Not sure if it would help but maybe it's worth a local define > > of something like > > > > #define MULT9 1000000000LL > > to loose the association with any particular SI basis and > > just indicate it's a bit number being used to retain precision > > in some maths? Would need a comment to stop people sending > > patches to replace it with GIGA though ;) > > > > My ultimate preference here is for whatever works for Peter and > > Liam as the people who are mostly likely to have to deal > > with any changes to this driver in the future. > > Hi Jonathan, > > My preference here is to keep GIGA, if it makes everyone more > comfortable, I can add a comment explaing the intention of the > multiplication? > Works for me. J > Cheers, > Liam > > > Jonathan > > > > > > > > > > Cheers, > > > Liam > > > > > > > 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; > >
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); *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); 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; 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; rescale->denominator = shunt / factor; return 0;
Make use of well-defined SI metric prefixes to improve code readability. Signed-off-by: Liam Beguin <liambeguin@gmail.com> --- drivers/iio/afe/iio-rescale.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)