Received-SPF: pass (google.com: domain of linux-kernel+bounces-170014-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com>
 <20240501-adding-new-ad738x-driver-v6-10-3c0741154728@baylibre.com>
 <a04d8015ea1606ce1eca86f7eaaa85a1c1b46d7a.camel@gmail.com> <20240506144616.0b90664b@jic23-huawei>
In-Reply-To: <20240506144616.0b90664b@jic23-huawei>
From: David Lechner <dlechner@baylibre.com>
Date: Mon, 6 May 2024 09:44:25 -0500
Message-ID: <CAMknhBHOXaff__QyU-wFSNNENvs23vDX5n_ddH-Dw3s6-sQ9sg@mail.gmail.com>
Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost
To: Jonathan Cameron <jic23@kernel.org>
Cc: =?UTF-8?B?TnVubyBTw6E=?= <noname.nuno@gmail.com>, 
	Julien Stephan <jstephan@baylibre.com>, Lars-Peter Clausen <lars@metafoo.de>, 
	Michael Hennerich <Michael.Hennerich@analog.com>, =?UTF-8?B?TnVubyBTw6E=?= <nuno.sa@analog.com>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, 
	Mark Brown <broonie@kernel.org>, kernel test robot <lkp@intel.com>, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

FYI, Julien is AFK for a bit so I'll try to respond to the non-trivial comm=
ents.

On Mon, May 6, 2024 at 8:46=E2=80=AFAM Jonathan Cameron <jic23@kernel.org> =
wrote:
>
> On Mon, 06 May 2024 10:55:46 +0200
> Nuno S=C3=A1 <noname.nuno@gmail.com> wrote:
>
> > On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> > > ad738x chips are able to use an additional 2 bits of resolution when
> > > using oversampling. The 14-bits chips can have up to 16 bits of
> > > resolution and the 16-bits chips can have up to 18 bits of resolution=
.
> > >
> > > In order to dynamically allow to enable/disable the resolution boost
> > > feature, we have to set iio realbits/storagebits to the maximum per c=
hips.
> > > This means that for iio, data will always be on the higher resolution
> > > available, and to cope with that we adjust the iio scale and iio offs=
et
> > > depending on the resolution boost status.
> > >
> > > The available scales can be displayed using the regular _scale_availa=
ble
> > > attributes and can be set by writing the regular _scale attribute.
> > > The available scales depend on the oversampling status.
> > >
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > >
> > > ---
> > >
> > > In order to support the resolution boost (additional 2 bits of resolu=
tion)
> > > we need to set realbits/storagebits to the maximum value i.e :
> > >   - realbits =3D 16 + 2 =3D 18, and storagebits =3D 32 for 16-bits ch=
ips
> > >   - realbits =3D 14 + 2 =3D 16, and storagebits =3D 16 for 14-bits ch=
ips
> > >
> > > For 14-bits chips this does not have a major impact, but this
> > > has the drawback of forcing 16-bits chip users to always use 32-bits
> > > words in buffers even if they are not using oversampling and resoluti=
on
> > > boost. Is there a better way of implementing this? For example
> > > implementing dynamic scan_type?
> > >
> >
> > Yeah, I don't think it's that bad in this case. But maybe dynamic scan =
types is
> > something we may need at some point yes (or IOW that I would like to se=
e supported
> > :)). We do some ADCs (eg: ad4630) where we use questionably use FW prop=
erties to set
> > a specific operating mode exactly because we have a different data layo=
ut (scan
> > elements) depending on the mode.
>
> Yeah. Fixed scan modes were somewhat of a bad design decision a long time=
 back.
> However, the big advantage is that it got people to think hard about whet=
her it is
> worth supporting low precision modes. For slow devices it very rarely is =
and
> forcing people to make a decision and the advantage we never supported
> low precision on those parts.
>
> Having said that there are good reasons for dynamic resolution changing
> (the main one being the storage case you have here) so I'd be happy to
> see some patches adding it. It might be easier than I've always thought
> to bolt on.
>
> This and inkernel event consumers have been the two significant features
> where I keep expecting it to happen, but every time people decide they re=
ally
> don't care enough to make them work :(
>

Supposing we knew someone willing and able :-) ...

Do you have any specific requirements for how dynamic resolution
changing should work? Any particular sticky points we should watch out
for?

I'm assuming this would just affect the bufferY/*_type attributes,
i.e. if you write a channel scale attribute to change the resolution,
then the scan_type info may change and the bufferY/*_type would need
to be read again.