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 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.
>
> 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.
>
> Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
>
> ---
>
> In order to support the resolution boost (additional 2 bits of resolution)
> we need to set realbits/storagebits to the maximum value i.e :
> - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
>
> 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?
>
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 see supported
:)). We do some ADCs (eg: ad4630) where we use questionably use FW properties to set
a specific operating mode exactly because we have a different data layout (scan
elements) depending on the mode.
> 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?
>
> Any suggestion would be very much appreciated.
> ---
> drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 194 insertions(+), 32 deletions(-)
>
> 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 @@
> #include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/units.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -58,6 +59,8 @@
> #define AD7380_CONFIG1_CRC_R BIT(4)
> #define AD7380_CONFIG1_ALERTEN BIT(3)
> #define AD7380_CONFIG1_RES BIT(2)
> +#define RESOLUTION_BOOST_DISABLE 0
> +#define RESOLUTION_BOOST_ENABLE 1
No ad7390 prefix?
> #define AD7380_CONFIG1_REFSEL BIT(1)
> #define AD7380_CONFIG1_PMODE BIT(0)
>
> @@ -86,6 +89,14 @@ struct ad7380_chip_info {
> const struct ad7380_timing_specs *timing_specs;
> };
>
> +/*
> + * realbits/storagebits cannot be dynamically changed, so in order to
> + * support the resolution boost (additional 2 bits of resolution)
> + * we need to set realbits/storagebits to the maximum value i.e :
> + * - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> + * - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> + * We need to adjust the scale depending on resolution boost status
> + */
> #define AD7380_CHANNEL(index, bits, diff) { \
> .type = IIO_VOLTAGE, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> @@ -93,6 +104,7 @@ struct ad7380_chip_info {
> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .indexed = 1, \
> .differential = (diff), \
> @@ -101,8 +113,8 @@ struct ad7380_chip_info {
> .scan_index = (index), \
> .scan_type = { \
> .sign = 's', \
> - .realbits = (bits), \
> - .storagebits = 16, \
> + .realbits = (bits) + 2, \
> + .storagebits = ((bits) + 2 > 16) ? 32 : 16, \
> .endianness = IIO_CPU, \
> }, \
> }
> @@ -259,6 +271,8 @@ struct ad7380_state {
> struct spi_device *spi;
> unsigned int oversampling_mode;
> unsigned int oversampling_ratio;
> + unsigned int scales[2][2];
> + bool resolution_boost_enable;
> struct regmap *regmap;
> unsigned int vref_mv;
> unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -270,7 +284,10 @@ struct ad7380_state {
> * As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for
> all parts
> */
> struct {
> - u16 raw[MAX_NUM_CHANNELS];
> + union {
> + u16 u16[MAX_NUM_CHANNELS];
> + u32 u32[MAX_NUM_CHANNELS];
> + } raw;
>
> s64 ts __aligned(8);
> } scan_data __aligned(IIO_DMA_MINALIGN);
> @@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> *indio_dev, u32 reg,
> unreachable();
> }
>
> +static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer
> *xfer)
> +{
> + int bits_per_word;
> +
> + memset(xfer, 0, sizeof(*xfer));
> +
> + xfer->rx_buf = &st->scan_data.raw;
> +
> + if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> + bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> + else
> + bits_per_word = st->chip_info->channels[0].scan_type.realbits - 2;
> +
> + xfer->bits_per_word = bits_per_word;
> +
> + xfer->len = (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 enablement... I
would expect that enabling/disabling resolution boost during buffering not to be a
great idea.
> +}
> +
> static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct ad7380_state *st = iio_priv(indio_dev);
> - struct spi_transfer xfer = {
> - .bits_per_word = st->chip_info->channels[0].scan_type.realbits,
> - .len = (st->chip_info->num_channels - 1) *
> - BITS_TO_BYTES(st->chip_info->channels-
> >scan_type.storagebits),
> - .rx_buf = st->scan_data.raw,
> - };
> - int ret;
> + struct spi_transfer xfer;
> + int bits_per_word, realbits, i, ret;
> +
> + realbits = st->chip_info->channels[0].scan_type.realbits;
> + bits_per_word = ad7380_prepare_spi_xfer(st, &xfer);
>
> ret = spi_sync_transfer(st->spi, &xfer, 1);
> if (ret)
> goto out;
>
> + /*
> + * If bits_per_word == 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 <= 16) {
> + /*
> + * Here realbits > 16 so storagebits is 32 and
> bits_per_word is <= 16
> + * so we need to sign extend u16 to u32 using reverse
> order to
> + * avoid writing over union data
> + */
> + for (i = st->chip_info->num_channels - 2; i >= 0; i--)
> + st->scan_data.raw.u32[i] = sign_extend32(st-
> >scan_data.raw.u16[i],
> +
> bits_per_word - 1);
> + } else if (bits_per_word < 16) {
Can't we have bits_per_word = 16 in case realbits <= 16?
- Nuno Sá
