Re: [PATCH 1/1] iio: core: Improve precision of __iio_format_value for FRACTIONAL values

[Alexandru Ardelean <ardeleanalex@gmail.com>]

On Tue, Jan 29, 2019 at 8:28 AM Phil Reid <preid@electromag.com.au> wrote:
>
> Currently FRACTIONAL values are outputed with 9 digits after the decimal
> place. This is not always sufficient to resolve the raw value with 1 bit.
> Output FRACTIONAL values to 15 decimal places of precision, regardless
> of the number of leading zeros.
>
> Currently for a 2.5V ref with 24 bits of precision the code outputs only
> to 9 decimal places.
>
> Cur: 0.00014901100000000000 * 16777216 = 2499.989733
> New: 0.00014901161193847600 * 16777216 = 2500.000000
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>
> Notes:
>     Alternatively I could add additonal FRACTIONAL types that select the new
>     behaviour to prevent any possible regressions.
>
>  drivers/iio/industrialio-core.c | 55 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a062cfd..bd9da64 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -571,11 +571,53 @@ int of_iio_read_mount_matrix(const struct device *dev,
>  #endif
>  EXPORT_SYMBOL(of_iio_read_mount_matrix);
>
> +static ssize_t __iio_format_div_prec(char *buf, unsigned int len, s64 x, s32 y)
> +{
> +       unsigned int prec = 0;
> +       unsigned int idx = 0;
> +       s64 d;
> +
> +       if (!len)
> +               return 0;
> +
> +       if (!y)
> +               return snprintf(buf, len, "inf");
> +
> +       if (!x)
> +               return snprintf(buf, len, "0");
> +
> +       if (((x > 0) && (y < 0)) || ((x < 0) && (y > 0))) {
> +               buf[idx++] = '-';
> +               x = x > 0 ? x : -x;
> +               y = y > 0 ? y : -y;
> +       }
> +
> +       d = div64_s64(x, y);
> +       idx += snprintf(buf+idx, len-idx, "%d", (int)d);
> +       x = x - (y * d);
> +       if ((x != 0) && (idx < len-1)) {
> +               buf[idx++] = '.';
> +               x = x * 10;
> +               d = div64_s64(x, y);
> +
> +               while ((idx < len-1) && (prec < 15)) {
> +                       if (d || prec)
> +                               prec++;
> +                       buf[idx++] = '0' + (char)d;
> +                       x = x - (y * d);
> +                       if (!x)
> +                               break;
> +                       x = x * 10;
> +                       d = div64_s64(x, y);
> +               }
> +               buf[idx] = 0;
> +       }
> +       return idx;
> +}
> +
>  static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
>                                   int size, const int *vals)
>  {
> -       unsigned long long tmp;
> -       int tmp0, tmp1;
>         bool scale_db = false;
>
>         switch (type) {
> @@ -598,14 +640,9 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
>                 else
>                         return snprintf(buf, len, "%d.%09u", vals[0], vals[1]);
>         case IIO_VAL_FRACTIONAL:
> -               tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
> -               tmp1 = vals[1];
> -               tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1);
> -               return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
> +               return __iio_format_div_prec(buf, len, vals[0], vals[1]);

Maybe I'm a bit naive, but I'm also a bit curious.
If you just bump the numbers here, would it work the same ?

i.e.   10^9 -> 10^15 and "snprintf(buf, len, "%d.%15u", tmp0, abs(tmp1));"

But in any case, what would be interesting now, is to extend the IIO
core logic to provide [somehow] a precision number, default being 9.
This could probably be specified on per-channel basis [somehow],
similar to other channel params.

So, for example in the default case, if you have "uint32_t precision =
9", you would have the same behavior, with something like

      tmp = div_s64((s64)vals[0] * pow_of_10(precision), vals[1]);
      tmp1 = vals[1];
      tmp0 = (int)div_s64_rem(tmp, pow_of_10(precision), &tmp1);
      return snprintf(buf, len, "%d.%" precision "u", tmp0, abs(tmp1));

Obviously, the above code is just pseudo-code, where pow_of_10()
multiplies 10 a "precision" number of times, and the snprintf() would
need a temporary buffer to create a format string, which then would be
used.

Thanks
Alex

>         case IIO_VAL_FRACTIONAL_LOG2:
> -               tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]);
> -               tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1);
> -               return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
> +               return __iio_format_div_prec(buf, len, vals[0], 1 << vals[1]);
>         case IIO_VAL_INT_MULTIPLE:
>         {
>                 int i;
> --
> 1.8.3.1
>