| Message ID | 20220418192907.763933-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | staging/iio: Clean up AD7746 CDC driver and move from staging. | expand |
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? 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 >
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 > > >
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]); + 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;