Re: [PATCH v14 02/11] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support

[Peter Rosin]

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@xxxxxxxxx>
> Reviewed-by: Peter Rosin <peda@xxxxxxxxxx>
> ---
>  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;