Re: [PATCH v2 00/17] staging/iio: Clean up AD7746 CDC driver and move from staging. - Joe Simmons-Talbott

From: Joe Simmons-Talbott <joetalbott@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>,
	Peter Rosin <peda@axentia.se>,
	Michael Hennerich <michael.hennerich@analog.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v2 00/17] staging/iio: Clean up AD7746 CDC driver and move from staging.
Date: Tue, 21 Jun 2022 10:34:36 -0400	[thread overview]
Message-ID: <YrHW/F1gpCFN3tnX@spruce> (raw)
In-Reply-To: <CAHp75VdjNuXeNPsjV4owGt5yg9N9kN8iYMKUX8rn=PRFpLKa+A@mail.gmail.com>

On Mon, Jun 20, 2022 at 01:23:39AM +0200, Andy Shevchenko wrote:
> On Sun, Jun 19, 2022 at 8:58 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > Vincent: I'm getting some instability with roadtest after forwards porting to
> > current mainline. Tests run 'sometimes'.  Other times I get a crash
> > in um_set_signal.  Seems unrelated to the test this series adds.
> >
> > Changes since v1: Thanks to Andy and Peter for reviews.
> > - Use sizeof(data) for be24 buffer data[3]
> > - Make the precision patch fall back to old maths if overflow would
> >   have occured.
> >
> > This started out as an experiment with Vincent Whitchurch's roadtest
> > testing framework [1] and that worked well so I carried on cleaning up the
> > driver.
> >
> > Mostly this is standard tidy up, move to new interfaces, then move the driver
> > out of staging, but there are a few other things in here.
> >
> > 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2.
> >    The ad7746 is a 24 bit sensor and this highlighted that 9 decimal
> >    places wasn't enough to keep the error introduced by
> >    _raw * _scale from growing too large over the whole range (I couldn't
> >    write a sensible test for it).  So I've proposed increasing the precision
> >    of the calculation used to provide the sysfs attributes with this value
> >    type and printing a few more decimal places (12), but only if overflow
> >    will not occur due to val[0] > ULLONG_MAX / PICO
> > 2) _inputoffset ABI addition.  This driver had an odd use of _offset for
> >    the case of differential channels that applied the offset to both
> >    of the differential pair (hence usespace shouldn't not apply it when
> 
> userspace?
> 

Also, there's a confusing double negative.  Either "should" or "should
not".

Thanks,
Joe