| Message ID | 20220208020441.3081162-3-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
Hi! As usual, sorry for my low bandwidth. On 2022-02-08 03:04, Liam Beguin wrote: > Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types. > Add support for these to allow using the iio-rescaler with them. > > Signed-off-by: Liam Beguin <liambeguin@gmail.com> > Reviewed-by: Peter Rosin <peda@axentia.se> > --- > drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 65832dd09249..f833eb38f8bb 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -14,6 +14,7 @@ > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/property.h> > +#include <linux/units.h> > > #include <linux/iio/afe/rescale.h> > #include <linux/iio/consumer.h> > @@ -23,6 +24,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > int *val, int *val2) > { > s64 tmp; > + s32 rem; > + u32 mult; > + u32 neg; > > switch (scale_type) { > case IIO_VAL_FRACTIONAL: > @@ -41,6 +45,37 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > tmp *= rescale->numerator; > tmp = div_s64(tmp, 1000000000LL); > *val = tmp; > + return scale_type; > + case IIO_VAL_INT_PLUS_NANO: > + case IIO_VAL_INT_PLUS_MICRO: > + mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA; By now, we all agree that the big numbers in this context have nothing to do with unit prefixes of physical quantities, so the macros are not really appropriate. However, in this case we have IIO_VAL_INT_PLUS_NANO and IIO_VAL_INT_PLUS_MICRO. Not using "NANO : MICRO" here, and instead go with "GIGA : MEGA" is just plain silly, if you ask me. So, either "NANO : MICRO" or "1000000000 : 1000000". I'm not sold on the macros. I frankly don't see all that much value in them and am perfectly fine with raw numbers. To me, it just looks like someone has read somewhere that constants should not appear in the code, and from that concluded that #define TEN 10 is a good thing without thinking very much about it. There is also the possibility that someone who has never seen these defines thinks MEGA is 2^20 instead of 10^6, because that is a much more likely candidate for a define in the frist place (not everybody knows all the digits of 1048576 by heart and 1 << 20 many times require extra brackets that might make it look more complicated than it needs to be). Back to this case; the connection to the naming of IIO_VAL_INT_PLUS_NANO (and ..._MICRO) makes it ok to use the defines. So if you feel strongly about not using "1000000000 : 1000000" I'm ok with that. Cheers, Peter > + > + /* > + * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if either *val > + * OR *val2 is negative the schan scale is negative, i.e. > + * *val = 1 and *val2 = -0.5 yields -1.5 not -0.5. > + */ > + neg = *val < 0 || *val2 < 0; > + > + tmp = (s64)abs(*val) * abs(rescale->numerator); > + *val = div_s64_rem(tmp, abs(rescale->denominator), &rem); > + > + tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator); > + tmp = div_s64(tmp, abs(rescale->denominator)); > + > + *val += div_s64_rem(tmp, mult, val2); > + > + /* > + * If only one of the rescaler elements or the schan scale is > + * negative, the combined scale is negative. > + */ > + if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { > + if (*val) > + *val = -*val; > + else > + *val2 = -*val2; > + } > + > return scale_type; > default: > return -EOPNOTSUPP;
Hi Peter, On Thu, Feb 10, 2022 at 05:42:25PM +0100, Peter Rosin wrote: > Hi! > > As usual, sorry for my low bandwidth. No worries, I understand. It's been pretty slow on my end as well. Thanks for still making time for this :-) > On 2022-02-08 03:04, Liam Beguin wrote: > > Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types. > > Add support for these to allow using the iio-rescaler with them. > > > > Signed-off-by: Liam Beguin <liambeguin@gmail.com> > > Reviewed-by: Peter Rosin <peda@axentia.se> > > --- > > drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) ... > > @@ -41,6 +45,37 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > > tmp *= rescale->numerator; > > tmp = div_s64(tmp, 1000000000LL); > > *val = tmp; > > + return scale_type; > > + case IIO_VAL_INT_PLUS_NANO: > > + case IIO_VAL_INT_PLUS_MICRO: > > + mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA; > > By now, we all agree that the big numbers in this context have nothing > to do with unit prefixes of physical quantities, so the macros are not > really appropriate. However, in this case we have IIO_VAL_INT_PLUS_NANO > and IIO_VAL_INT_PLUS_MICRO. Not using "NANO : MICRO" here, and instead > go with "GIGA : MEGA" is just plain silly, if you ask me. > > So, either "NANO : MICRO" or "1000000000 : 1000000". > > I'm not sold on the macros. I frankly don't see all that much value > in them and am perfectly fine with raw numbers. To me, it just looks > like someone has read somewhere that constants should not appear in > the code, and from that concluded that #define TEN 10 is a good thing > without thinking very much about it. There is also the possibility > that someone who has never seen these defines thinks MEGA is 2^20 > instead of 10^6, because that is a much more likely candidate for a > define in the frist place (not everybody knows all the digits of > 1048576 by heart and 1 << 20 many times require extra brackets that > might make it look more complicated than it needs to be). > > Back to this case; the connection to the naming of IIO_VAL_INT_PLUS_NANO > (and ..._MICRO) makes it ok to use the defines. So if you feel strongly > about not using "1000000000 : 1000000" I'm ok with that. I like that the macros make the number of zeros more "obvious" in a sense, but honestly at this point, I'd rather go back to not using them. Depending on who you ask, one way makes more sense than the other, at least with the raw values, there's no ambiguity. Cheers, Liam > Cheers, > Peter
On 2022-02-10 18:36, Liam Beguin wrote: > I like that the macros make the number of zeros more "obvious" in a > sense, but honestly at this point, I'd rather go back to not using them. > Depending on who you ask, one way makes more sense than the other, at > least with the raw values, there's no ambiguity. I'm fine with not using the macros in this series, obviously. While I can tolerate them, I do not get a warm and cozy feeling when I see them. Cheers, Peter
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 65832dd09249..f833eb38f8bb 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -14,6 +14,7 @@ #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/property.h> +#include <linux/units.h> #include <linux/iio/afe/rescale.h> #include <linux/iio/consumer.h> @@ -23,6 +24,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, int *val, int *val2) { s64 tmp; + s32 rem; + u32 mult; + u32 neg; switch (scale_type) { case IIO_VAL_FRACTIONAL: @@ -41,6 +45,37 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, tmp *= rescale->numerator; tmp = div_s64(tmp, 1000000000LL); *val = tmp; + return scale_type; + case IIO_VAL_INT_PLUS_NANO: + case IIO_VAL_INT_PLUS_MICRO: + mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA; + + /* + * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if either *val + * OR *val2 is negative the schan scale is negative, i.e. + * *val = 1 and *val2 = -0.5 yields -1.5 not -0.5. + */ + neg = *val < 0 || *val2 < 0; + + tmp = (s64)abs(*val) * abs(rescale->numerator); + *val = div_s64_rem(tmp, abs(rescale->denominator), &rem); + + tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator); + tmp = div_s64(tmp, abs(rescale->denominator)); + + *val += div_s64_rem(tmp, mult, val2); + + /* + * If only one of the rescaler elements or the schan scale is + * negative, the combined scale is negative. + */ + if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { + if (*val) + *val = -*val; + else + *val2 = -*val2; + } + return scale_type; default: return -EOPNOTSUPP;
Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types. Add support for these to allow using the iio-rescaler with them. Signed-off-by: Liam Beguin <liambeguin@gmail.com> Reviewed-by: Peter Rosin <peda@axentia.se> --- drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)