Re: [PATCH 01/17] iio: core: Increase precision of IIO_VAL_FRACTIONAL_LOG2

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 26 Apr 2022 14:00:49 +0200
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > With some high resolution sensors such as the ad7746 the
> > build up of error when multiplying the _raw and _scale
> > values together can be significant.  Reduce this affect by
> > providing additional resolution in both calculation and
> > formatting of result.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/iio/industrialio-core.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 2f48e9a97274..d831835770da 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -683,14 +683,21 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> >  		else
> >  			return sysfs_emit_at(buf, offset, "%d.%09u", tmp0,
> >  					     abs(tmp1));
> > -	case IIO_VAL_FRACTIONAL_LOG2:
> > -		tmp2 = shift_right((s64)vals[0] * 1000000000LL, vals[1]);
> > -		tmp0 = (int)div_s64_rem(tmp2, 1000000000LL, &tmp1);
> > -		if (tmp0 == 0 && tmp2 < 0)
> > -			return sysfs_emit_at(buf, offset, "-0.%09u", abs(tmp1));
> > -		else
> > -			return sysfs_emit_at(buf, offset, "%d.%09u", tmp0,
> > -					     abs(tmp1));
> > +	case IIO_VAL_FRACTIONAL_LOG2: {
> > +		u64 t1, t2;
> > +		int integer;
> > +		bool neg = vals[0] < 0;
> > +
> > +		t1 = shift_right((u64)abs(vals[0]) * 1000000000000ULL, vals[1]);  
> 
> While the multiplication is safe if val[0] is 24 bits or less, I
> wonder if that's OK for *all* the existing stuff? I would have guessed
> that something somewhere needs more bits than that? Did you consider the
> rescaler? And if everything currently really does fit in 24 bits, do we
> really want to make it difficult to add something beyond 24 bits later
> on?

Good point. Perhaps we can do something a bit nefarious and check that
val[0] is sufficiently small and if not fallback to lower precision
as we had before?  Or if adventurous adjust the precision automatically
so we can get as many digits (at least 9) as can be computed without
overflow...  For now, I think 2 steps is enough though.

Jonathan
 
> 
> Cheers,
> Peter
> 
> > +		integer = (int)div64_u64_rem(t1, 1000000000000ULL, &t2);
> > +		if (integer == 0 && neg)
> > +			return sysfs_emit_at(buf, offset, "-0.%012llu", abs(t2));
> > +		if (neg)
> > +			integer *= -1;
> > +		return sysfs_emit_at(buf, offset, "%d.%012llu", integer,
> > +				     abs(t2));
> > +		}
> > +	}
> >  	case IIO_VAL_INT_MULTIPLE:
> >  	{
> >  		int i;
> > -- 
> > 2.35.3
> >   
>