Received-SPF: pass (google.com: domain of linux-kernel+bounces-169976-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 6 May 2024 15:20:39 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Nuno =?UTF-8?B?U8Oh?= <noname.nuno@gmail.com>
Cc: Julien Stephan <jstephan@baylibre.com>, Lars-Peter Clausen
 <lars@metafoo.de>, Michael Hennerich <Michael.Hennerich@analog.com>, Nuno
 =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>, David Lechner
 <dlechner@baylibre.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
Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for
 resolution boost
Message-ID: <20240506152022.58794348@jic23-huawei>
In-Reply-To: <20240506144616.0b90664b@jic23-huawei>
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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, 6 May 2024 14:46:16 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 06 May 2024 10:55:46 +0200
> Nuno S=C3=A1 <noname.nuno@gmail.com> wrote:
>=20
> > On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote: =20
> > > 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.
> > >=20
> > > 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.
> > >=20
> > > 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.
> > >=20
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > >=20
> > > ---
> > >=20
> > > 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 :
> > > =C2=A0 - realbits =3D 16 + 2 =3D 18, and storagebits =3D 32 for 16-bi=
ts chips
> > > =C2=A0 - realbits =3D 14 + 2 =3D 16, and storagebits =3D 16 for 14-bi=
ts chips
> > >=20
> > > 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?
> > >    =20
> >=20
> > 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. =20
>=20
> 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.
>=20
> 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.
>=20
> 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 :(
>=20
> >   =20
> > > Another issue is the location of the timestamps. I understood the need
> > > for ts to be consistent between chips, but right now I do not have a
> > > better solution.. I was thinking of maybe adding the timestamps at the
> > > beginning? That would imply to change the iio_chan_spec struct and ma=
ybe
> > > add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> > > that an option? =20
>=20
> You have lost me on this one.  Why does the timestamp need to be in a con=
sistent
> location?  We have lots of drivers where it moves about between different
> chips they support, or indeed what channels are currently turned on.
Maybe I now understand this?
The concern is the structure used for the iio_push_to_buffers_with_timestam=
p()
That doesn't need to be a structure and if you look at drivers where it isn=
't
the most common reason is because the timestamp needs to be able to move ar=
ound.

So do something similar here.

Jonathan

>=20
> I haven't actually looked at the latest code yet, so maybe it will become
> obvious!
>=20
> Jonathan
>=20
>=20
>=20