Received-SPF: pass (google.com: domain of linux-kernel+bounces-169539-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <a04d8015ea1606ce1eca86f7eaaa85a1c1b46d7a.camel@gmail.com>
Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for
 resolution boost
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Julien Stephan <jstephan@baylibre.com>, Lars-Peter Clausen
 <lars@metafoo.de>,  Michael Hennerich <Michael.Hennerich@analog.com>, Nuno
 =?ISO-8859-1?Q?S=E1?= <nuno.sa@analog.com>, David Lechner
 <dlechner@baylibre.com>, Jonathan Cameron <jic23@kernel.org>, 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>
Cc: kernel test robot <lkp@intel.com>, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Date: Mon, 06 May 2024 10:55:46 +0200
In-Reply-To: <20240501-adding-new-ad738x-driver-v6-10-3c0741154728@baylibre.com>
References: 
	<20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com>
	 <20240501-adding-new-ad738x-driver-v6-10-3c0741154728@baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> In order to dynamically allow to enable/disable the resolution boost
> feature, we have to set iio realbits/storagebits to the maximum per chips=
.
> 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 offset
> depending on the resolution boost status.
>=20
> The available scales can be displayed using the regular _scale_available
> 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 resolution=
)
> 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-bits c=
hips
> =C2=A0 - realbits =3D 14 + 2 =3D 16, and storagebits =3D 16 for 14-bits c=
hips
>=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 resolution
> boost. Is there a better way of implementing this? For example
> implementing dynamic scan_type?
>=20

Yeah, I don't think it's that bad in this case. But maybe dynamic scan type=
s is
something we may need at some point yes (or IOW that I would like to see su=
pported
:)). We do some ADCs (eg: ad4630) where we use questionably use FW properti=
es to set
a specific operating mode exactly because we have a different data layout (=
scan
elements) depending on the mode.
=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 maybe
> add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> that an option?
>=20
> Any suggestion would be very much appreciated.
> ---
> =C2=A0drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++=
++++-------
> =C2=A01 file changed, 194 insertions(+), 32 deletions(-)
>=20
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 7b021bb9cf87..e240098708e9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -20,6 +20,7 @@
> =C2=A0#include <linux/err.h>
> =C2=A0#include <linux/kernel.h>
> =C2=A0#include <linux/module.h>
> +#include <linux/units.h>
> =C2=A0#include <linux/regmap.h>
> =C2=A0#include <linux/regulator/consumer.h>
> =C2=A0#include <linux/slab.h>
> @@ -58,6 +59,8 @@
> =C2=A0#define AD7380_CONFIG1_CRC_R		BIT(4)
> =C2=A0#define AD7380_CONFIG1_ALERTEN		BIT(3)
> =C2=A0#define AD7380_CONFIG1_RES		BIT(2)
> +#define RESOLUTION_BOOST_DISABLE	0
> +#define RESOLUTION_BOOST_ENABLE		1

No ad7390 prefix?

> =C2=A0#define AD7380_CONFIG1_REFSEL		BIT(1)
> =C2=A0#define AD7380_CONFIG1_PMODE		BIT(0)
> =C2=A0
> @@ -86,6 +89,14 @@ struct ad7380_chip_info {
> =C2=A0	const struct ad7380_timing_specs *timing_specs;
> =C2=A0};
> =C2=A0
> +/*
> + * realbits/storagebits cannot be dynamically changed, so in order to
> + * support the resolution boost (additional 2=C2=A0 bits of resolution)
> + * we need to set realbits/storagebits to the maximum value i.e :
> + *=C2=A0=C2=A0 - realbits =3D 16 + 2 =3D 18, and storagebits =3D 32 for =
16-bits chips
> + *=C2=A0=C2=A0 - realbits =3D 14 + 2 =3D 16, and storagebits =3D 16 for =
14-bits chips
> + * We need to adjust the scale depending on resolution boost status
> + */
> =C2=A0#define AD7380_CHANNEL(index, bits, diff) {			\
> =C2=A0	.type =3D IIO_VOLTAGE,					\
> =C2=A0	.info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW) |		\
> @@ -93,6 +104,7 @@ struct ad7380_chip_info {
> =C2=A0	.info_mask_shared_by_type =3D BIT(IIO_CHAN_INFO_SCALE) |	\
> =C2=A0		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
> =C2=A0	.info_mask_shared_by_type_available =3D			\
> +		BIT(IIO_CHAN_INFO_SCALE) |			\
> =C2=A0		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
> =C2=A0	.indexed =3D 1,						\
> =C2=A0	.differential =3D (diff),					\
> @@ -101,8 +113,8 @@ struct ad7380_chip_info {
> =C2=A0	.scan_index =3D (index),					\
> =C2=A0	.scan_type =3D {						\
> =C2=A0		.sign =3D 's',					\
> -		.realbits =3D (bits),				\
> -		.storagebits =3D 16,				\
> +		.realbits =3D (bits) + 2,				\
> +		.storagebits =3D ((bits) + 2 > 16) ? 32 : 16,	\
> =C2=A0		.endianness =3D IIO_CPU,				\
> =C2=A0	},							\
> =C2=A0}
> @@ -259,6 +271,8 @@ struct ad7380_state {
> =C2=A0	struct spi_device *spi;
> =C2=A0	unsigned int oversampling_mode;
> =C2=A0	unsigned int oversampling_ratio;
> +	unsigned int scales[2][2];
> +	bool resolution_boost_enable;
> =C2=A0	struct regmap *regmap;
> =C2=A0	unsigned int vref_mv;
> =C2=A0	unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -270,7 +284,10 @@ struct ad7380_state {
> =C2=A0	 * As MAX_NUM_CHANNELS is 4 the layout of the structure is the sam=
e for
> all parts
> =C2=A0	 */
> =C2=A0	struct {
> -		u16 raw[MAX_NUM_CHANNELS];
> +		union {
> +			u16 u16[MAX_NUM_CHANNELS];
> +			u32 u32[MAX_NUM_CHANNELS];
> +		} raw;
> =C2=A0
> =C2=A0		s64 ts __aligned(8);
> =C2=A0	} scan_data __aligned(IIO_DMA_MINALIGN);
> @@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> *indio_dev, u32 reg,
> =C2=A0	unreachable();
> =C2=A0}
> =C2=A0
> +static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_t=
ransfer
> *xfer)
> +{
> +	int bits_per_word;
> +
> +	memset(xfer, 0, sizeof(*xfer));
> +
> +	xfer->rx_buf =3D &st->scan_data.raw;
> +
> +	if (st->resolution_boost_enable =3D=3D RESOLUTION_BOOST_ENABLE)
> +		bits_per_word =3D st->chip_info->channels[0].scan_type.realbits;
> +	else
> +		bits_per_word =3D st->chip_info->channels[0].scan_type.realbits - 2;
> +
> +	xfer->bits_per_word =3D bits_per_word;
> +
> +	xfer->len =3D (st->chip_info->num_channels - 1) *
> BITS_TO_BYTES(bits_per_word);
> +
> +	return bits_per_word;

I think this may very well be something we can do once during buffer enable=
ment... I
would expect that enabling/disabling resolution boost during buffering not =
to be a
great idea.
=20
> +}
> +
> =C2=A0static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> =C2=A0{
> =C2=A0	struct iio_poll_func *pf =3D p;
> =C2=A0	struct iio_dev *indio_dev =3D pf->indio_dev;
> =C2=A0	struct ad7380_state *st =3D iio_priv(indio_dev);
> -	struct spi_transfer xfer =3D {
> -		.bits_per_word =3D st->chip_info->channels[0].scan_type.realbits,
> -		.len =3D (st->chip_info->num_channels - 1) *
> -		=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 BITS_TO_BYTES(st->chip_info->chan=
nels-
> >scan_type.storagebits),
> -		.rx_buf =3D st->scan_data.raw,
> -	};
> -	int ret;
> +	struct spi_transfer xfer;
> +	int bits_per_word, realbits, i, ret;
> +
> +	realbits =3D st->chip_info->channels[0].scan_type.realbits;
> +	bits_per_word =3D ad7380_prepare_spi_xfer(st, &xfer);
> =C2=A0
> =C2=A0	ret =3D spi_sync_transfer(st->spi, &xfer, 1);
> =C2=A0	if (ret)
> =C2=A0		goto out;
> =C2=A0
> +	/*
> +	 * If bits_per_word =3D=3D realbits (resolution boost enabled), we don'=
t
> +	 * need to manipulate the raw data, otherwise we may need to fix things
> +	 * up a bit to fit the scan_type specs
> +	 */
> +	if (bits_per_word < realbits) {
> +		if (realbits > 16 && bits_per_word <=3D 16) {
> +			/*
> +			 * Here realbits > 16 so storagebits is 32 and
> bits_per_word is <=3D 16
> +			 * so we need to sign extend u16 to u32 using reverse
> order to
> +			 * avoid writing over union data
> +			 */
> +			for (i =3D st->chip_info->num_channels - 2; i >=3D 0; i--)
> +				st->scan_data.raw.u32[i] =3D sign_extend32(st-
> >scan_data.raw.u16[i],
> +								=09
> bits_per_word - 1);
> +		} else if (bits_per_word < 16) {

Can't we have bits_per_word =3D 16 in case realbits <=3D 16?

- Nuno S=C3=A1