Received-SPF: pass (google.com: domain of linux-kernel+bounces-183224-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 19 May 2024 20:20:39 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>, Michael Hennerich
 <Michael.Hennerich@analog.com>, Nuno =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>,
 Julien Stephan <jstephan@baylibre.com>, Esteban Blanc
 <eblanc@baylibre.com>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan
 type
Message-ID: <20240519202039.5a70157d@jic23-huawei>
In-Reply-To: <CAMknhBFob4Wd8Gm8W8NKSuL9UbBCY8+fAN_voGPhb4Fy1tAT-w@mail.gmail.com>
References: <20240507-iio-add-support-for-multiple-scan-types-v1-0-95ac33ee51e9@baylibre.com>
	<20240507-iio-add-support-for-multiple-scan-types-v1-4-95ac33ee51e9@baylibre.com>
	<20240508124049.00001661@Huawei.com>
	<CAMknhBFob4Wd8Gm8W8NKSuL9UbBCY8+fAN_voGPhb4Fy1tAT-w@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, 8 May 2024 12:21:09 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Wed, May 8, 2024 at 6:40=E2=80=AFAM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Tue,  7 May 2024 14:02:08 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> > =20
> > > The AD783x chips have a resolution boost feature that allows for 2
> > > extra bits of resolution. Previously, we had to choose a scan type to
> > > fit the largest resolution and manipulate the raw data to fit when the
> > > resolution was lower. This patch adds support for multiple scan types
> > > for the voltage input channels so that we can support both resolutions
> > > without having to manipulate the raw data.
> > >
> > > Signed-off-by: David Lechner <dlechner@baylibre.com> =20
> >
> > I'm wondering about the control mechanism.  I was thinking we'd poke
> > the scan type directly but this may well make more sense.
> >
> > This is relying on _scale change to trigger the change in the scan type.
> > That may well be sufficient and I've been over thinking this for far to=
o many
> > years :)
> >
> > It will get messy though in some cases as the device might have a PGA o=
n the
> > front end so we will have a trade off between actual scaling control and
> > resolution related scale changes. We've had a few device where the scale
> > calculation is already complex and involves various different hardware
> > controls, but none have affected the storage format like this.
> >
> > I'll think some more.
> > =20
>=20
> Here is some more food for thought. The AD4630 family of chips we are
> working on is similar to this one in that it also has oversampling
> with increased resolution. Except in that case, they are strictly tied
> together. With oversampling disabled, we must only read 24-bits (or 16
> depending on the exact model) and when oversampling is enabled, we
> must read 32-bits (30 real bits with 2-bit shift). So in that case,
> the scan_type would depend only on oversampling ratio > 0. (Writing
> the oversampling ratio attribute would affect scale, but scale
> wouldn't be writable like on ad7380.)
>=20
> It seems more intuitive to me that to enable oversampling, we would
> just write to the oversampling ratio attribute rather than having to
> write to a buffer _type attribute to enable oversampling in the first
> place. And other than requiring reading the documentation it would be
> pretty hard to guess that writing le:s30/32>>2 is what you need to do
> to enable oversampling.
>=20

Ok. Few weeks thinking and I've no better ideas.  Generally I'm fine
with how you did this but I wouldn't have a 'special / default'
scan_type.  Just put them all in the array and pick between them.
That avoids fun of people trying to work out on what basis to
prefer one over another.=20

So tidy the loose ends up and I'd be delighted to see a non RFC version.
It 'might' be worth waiting until we have a couple of suitable drivers
though and then show the feature works well for them all.
Whilst I think I'd take it with just one though as can see how it fits
together, but more than one driver would boost my confidence level.

Jonathan