LinuxLists.cc - [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

2024-05-07 19:03:13

Subject: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

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 <[email protected]>
---
drivers/iio/adc/ad7380.c | 185 ++++++++++++++++++++++-------------------------
1 file changed, 86 insertions(+), 99 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index e240098708e9..ca317e3a72d9 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -89,14 +89,22 @@ 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
- */
+/** scan type for 14-bit chips with resolution boost enabled. */
+static const struct iio_scan_type ad7380_scan_type_14_boost = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_CPU,
+};
+
+/** scan type for 16-bit chips with resolution boost enabled. */
+static const struct iio_scan_type ad7380_scan_type_16_boost = {
+ .sign = 's',
+ .realbits = 18,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+};
+
#define AD7380_CHANNEL(index, bits, diff) { \
.type = IIO_VOLTAGE, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
@@ -113,10 +121,12 @@ struct ad7380_chip_info {
.scan_index = (index), \
.scan_type = { \
.sign = 's', \
- .realbits = (bits) + 2, \
- .storagebits = ((bits) + 2 > 16) ? 32 : 16, \
+ .realbits = (bits), \
+ .storagebits = ((bits) > 16) ? 32 : 16, \
.endianness = IIO_CPU, \
}, \
+ .ext_scan_type = &ad7380_scan_type_##bits##_boost, \
+ .num_ext_scan_type = 1, \
}

#define DEFINE_AD7380_2_CHANNEL(name, bits, diff) \
@@ -376,67 +386,62 @@ 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)
+/**
+ * Reads one set of samples from the device. This is a simultaneous sampling
+ * chip, so all channels are always read at the same time.
+ *
+ * On successful return, the raw data is stored in st->scan_data.raw.
+ */
+static int ad7380_read_one_sample(struct ad7380_state *st,
+ const struct iio_scan_type *scan_type)
{
- 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;
+ struct spi_transfer xfers[2] = {
+ /* toggle CS (no data xfer) to trigger a conversion */
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = scan_type->realbits,
+ .delay = {
+ .value = T_CONVERT_NS,
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ .cs_change = 1,
+ .cs_change_delay = {
+ .value = st->chip_info->timing_specs->t_csh_ns,
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ },
+ {
+ .rx_buf = &st->scan_data.raw,
+ .len = BITS_TO_BYTES(scan_type->storagebits) *
+ (st->chip_info->num_channels - 1),
+ .bits_per_word = scan_type->realbits,
+ },
+ };

- xfer->len = (st->chip_info->num_channels - 1) * BITS_TO_BYTES(bits_per_word);
+ /*
+ * In normal average oversampling we need to wait for multiple conversions to be done
+ */
+ if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
+ xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;

- return bits_per_word;
+ return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
}

static irqreturn_t ad7380_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
+ const struct iio_chan_spec *chan = &indio_dev->channels[0];
+ const struct iio_scan_type *scan_type = iio_get_current_scan_type(
+ indio_dev, chan);
struct ad7380_state *st = iio_priv(indio_dev);
- struct spi_transfer xfer;
- int bits_per_word, realbits, i, ret;
+ int 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);
+ ret = ad7380_read_one_sample(st, scan_type);
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) {
- /*
- * Here realbits <= 16 so storagebits is 16.
- * We only need to sign extend only if bits_per_word is < 16
- */
- for (i = 0; i < st->chip_info->num_channels - 1; i++)
- st->scan_data.raw.u16[i] = sign_extend32(st->scan_data.raw.u16[i],
- bits_per_word - 1);
- }
- }
-
iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
pf->timestamp);

@@ -447,47 +452,21 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
}

static int ad7380_read_direct(struct ad7380_state *st,
- struct iio_chan_spec const *chan, int *val)
+ struct iio_chan_spec const *chan,
+ const struct iio_scan_type *scan_type,
+ int *val)
{
- struct spi_transfer xfers[2] = {
- /* toggle CS (no data xfer) to trigger a conversion */
- {
- .speed_hz = AD7380_REG_WR_SPEED_HZ,
- .bits_per_word = chan->scan_type.realbits,
- .delay = {
- .value = T_CONVERT_NS,
- .unit = SPI_DELAY_UNIT_NSECS,
- },
- .cs_change = 1,
- .cs_change_delay = {
- .value = st->chip_info->timing_specs->t_csh_ns,
- .unit = SPI_DELAY_UNIT_NSECS,
- },
- },
- /* then read all channels, it will be filled by ad7380_prepare_spi_xfer */
- {
- },
- };
- int bits_per_word, ret;
-
- /*
- * In normal average oversampling we need to wait for multiple conversions to be done
- */
- if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
- xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
-
- bits_per_word = ad7380_prepare_spi_xfer(st, &xfers[1]);
+ int ret;

- ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
- if (ret < 0)
+ ret = ad7380_read_one_sample(st, scan_type);
+ if (ret)
return ret;
-
- if (bits_per_word > 16)
+ if (scan_type->storagebits > 16)
*val = sign_extend32(st->scan_data.raw.u32[chan->scan_index],
- bits_per_word - 1);
+ scan_type->realbits - 1);
else
*val = sign_extend32(st->scan_data.raw.u16[chan->scan_index],
- bits_per_word - 1);
+ scan_type->realbits - 1);

return IIO_VAL_INT;
}
@@ -496,18 +475,14 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long info)
{
+ const struct iio_scan_type *scan_type = iio_get_current_scan_type(
+ indio_dev, chan);
struct ad7380_state *st = iio_priv(indio_dev);
- int realbits;
-
- if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
- realbits = chan->scan_type.realbits;
- else
- realbits = chan->scan_type.realbits - 2;

switch (info) {
case IIO_CHAN_INFO_RAW:
iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- return ad7380_read_direct(st, chan, val);
+ return ad7380_read_direct(st, chan, scan_type, val);
}
unreachable();
case IIO_CHAN_INFO_SCALE:
@@ -520,7 +495,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
* According to IIO ABI, offset is applied before scale,
* so offset is: vcm_mv / scale
*/
- *val = st->vcm_mv[chan->channel] * (1 << realbits)
+ *val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
/ st->vref_mv;

return IIO_VAL_INT;
@@ -700,6 +675,17 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
}
}

+static const struct iio_scan_type *ad7380_get_current_scan_type(
+ const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
+{
+ struct ad7380_state *st = iio_priv(indio_dev);
+
+ if (st->resolution_boost_enable && chan->num_ext_scan_type)
+ return chan->ext_scan_type;
+
+ return &chan->scan_type;
+}
+
static ssize_t oversampling_mode_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -796,6 +782,7 @@ static const struct iio_info ad7380_info = {
.read_raw = &ad7380_read_raw,
.read_avail = &ad7380_read_avail,
.write_raw = &ad7380_write_raw,
+ .get_current_scan_type = &ad7380_get_current_scan_type,
.debugfs_reg_access = &ad7380_debugfs_reg_access,
};

--
2.43.2

2024-05-08 11:47:22

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

On Tue, 7 May 2024 14:02:08 -0500
David Lechner <[email protected]> wrote:

> 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 <[email protected]>

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 too many
years :)

It will get messy though in some cases as the device might have a PGA on 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.

> ---
> drivers/iio/adc/ad7380.c | 185 ++++++++++++++++++++++-------------------------
> 1 file changed, 86 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index e240098708e9..ca317e3a72d9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -89,14 +89,22 @@ 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
> - */
> +/** scan type for 14-bit chips with resolution boost enabled. */
> +static const struct iio_scan_type ad7380_scan_type_14_boost = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_CPU,
> +};
> +
> +/** scan type for 16-bit chips with resolution boost enabled. */
> +static const struct iio_scan_type ad7380_scan_type_16_boost = {
> + .sign = 's',
> + .realbits = 18,
> + .storagebits = 32,
> + .endianness = IIO_CPU,
> +};
> +
> #define AD7380_CHANNEL(index, bits, diff) { \
> .type = IIO_VOLTAGE, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> @@ -113,10 +121,12 @@ struct ad7380_chip_info {
> .scan_index = (index), \
> .scan_type = { \
> .sign = 's', \
> - .realbits = (bits) + 2, \
> - .storagebits = ((bits) + 2 > 16) ? 32 : 16, \
> + .realbits = (bits), \
> + .storagebits = ((bits) > 16) ? 32 : 16, \
> .endianness = IIO_CPU, \
> }, \
> + .ext_scan_type = &ad7380_scan_type_##bits##_boost, \
> + .num_ext_scan_type = 1, \
> }
>
> #define DEFINE_AD7380_2_CHANNEL(name, bits, diff) \
> @@ -376,67 +386,62 @@ 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)
> +/**
> + * Reads one set of samples from the device. This is a simultaneous sampling
> + * chip, so all channels are always read at the same time.
> + *
> + * On successful return, the raw data is stored in st->scan_data.raw.
> + */
> +static int ad7380_read_one_sample(struct ad7380_state *st,
> + const struct iio_scan_type *scan_type)
> {
> - 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;
> + struct spi_transfer xfers[2] = {
> + /* toggle CS (no data xfer) to trigger a conversion */
> + {
> + .speed_hz = AD7380_REG_WR_SPEED_HZ,
> + .bits_per_word = scan_type->realbits,
> + .delay = {
> + .value = T_CONVERT_NS,
> + .unit = SPI_DELAY_UNIT_NSECS,
> + },
> + .cs_change = 1,
> + .cs_change_delay = {
> + .value = st->chip_info->timing_specs->t_csh_ns,
> + .unit = SPI_DELAY_UNIT_NSECS,
> + },
> + },
> + {
> + .rx_buf = &st->scan_data.raw,
> + .len = BITS_TO_BYTES(scan_type->storagebits) *
> + (st->chip_info->num_channels - 1),
> + .bits_per_word = scan_type->realbits,
> + },
> + };
>
> - xfer->len = (st->chip_info->num_channels - 1) * BITS_TO_BYTES(bits_per_word);
> + /*
> + * In normal average oversampling we need to wait for multiple conversions to be done
> + */
> + if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
> + xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
>
> - return bits_per_word;
> + return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> }
>
> static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> + const struct iio_chan_spec *chan = &indio_dev->channels[0];
> + const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> + indio_dev, chan);
> struct ad7380_state *st = iio_priv(indio_dev);
> - struct spi_transfer xfer;
> - int bits_per_word, realbits, i, ret;
> + int 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);
> + ret = ad7380_read_one_sample(st, scan_type);
> 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) {
> - /*
> - * Here realbits <= 16 so storagebits is 16.
> - * We only need to sign extend only if bits_per_word is < 16
> - */
> - for (i = 0; i < st->chip_info->num_channels - 1; i++)
> - st->scan_data.raw.u16[i] = sign_extend32(st->scan_data.raw.u16[i],
> - bits_per_word - 1);
> - }
> - }
> -
> iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
> pf->timestamp);
>
> @@ -447,47 +452,21 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> }
>
> static int ad7380_read_direct(struct ad7380_state *st,
> - struct iio_chan_spec const *chan, int *val)
> + struct iio_chan_spec const *chan,
> + const struct iio_scan_type *scan_type,
> + int *val)
> {
> - struct spi_transfer xfers[2] = {
> - /* toggle CS (no data xfer) to trigger a conversion */
> - {
> - .speed_hz = AD7380_REG_WR_SPEED_HZ,
> - .bits_per_word = chan->scan_type.realbits,
> - .delay = {
> - .value = T_CONVERT_NS,
> - .unit = SPI_DELAY_UNIT_NSECS,
> - },
> - .cs_change = 1,
> - .cs_change_delay = {
> - .value = st->chip_info->timing_specs->t_csh_ns,
> - .unit = SPI_DELAY_UNIT_NSECS,
> - },
> - },
> - /* then read all channels, it will be filled by ad7380_prepare_spi_xfer */
> - {
> - },
> - };
> - int bits_per_word, ret;
> -
> - /*
> - * In normal average oversampling we need to wait for multiple conversions to be done
> - */
> - if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
> - xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
> -
> - bits_per_word = ad7380_prepare_spi_xfer(st, &xfers[1]);
> + int ret;
>
> - ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> - if (ret < 0)
> + ret = ad7380_read_one_sample(st, scan_type);
> + if (ret)
> return ret;
> -
> - if (bits_per_word > 16)
> + if (scan_type->storagebits > 16)
> *val = sign_extend32(st->scan_data.raw.u32[chan->scan_index],
> - bits_per_word - 1);
> + scan_type->realbits - 1);
> else
> *val = sign_extend32(st->scan_data.raw.u16[chan->scan_index],
> - bits_per_word - 1);
> + scan_type->realbits - 1);
>
> return IIO_VAL_INT;
> }
> @@ -496,18 +475,14 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long info)
> {
> + const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> + indio_dev, chan);
> struct ad7380_state *st = iio_priv(indio_dev);
> - int realbits;
> -
> - if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> - realbits = chan->scan_type.realbits;
> - else
> - realbits = chan->scan_type.realbits - 2;
>
> switch (info) {
> case IIO_CHAN_INFO_RAW:
> iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> - return ad7380_read_direct(st, chan, val);
> + return ad7380_read_direct(st, chan, scan_type, val);
> }
> unreachable();
> case IIO_CHAN_INFO_SCALE:
> @@ -520,7 +495,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> * According to IIO ABI, offset is applied before scale,
> * so offset is: vcm_mv / scale
> */
> - *val = st->vcm_mv[chan->channel] * (1 << realbits)
> + *val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
> / st->vref_mv;
>
> return IIO_VAL_INT;
> @@ -700,6 +675,17 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> +static const struct iio_scan_type *ad7380_get_current_scan_type(
> + const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
> +{
> + struct ad7380_state *st = iio_priv(indio_dev);
> +
> + if (st->resolution_boost_enable && chan->num_ext_scan_type)
> + return chan->ext_scan_type;
> +
> + return &chan->scan_type;
> +}
> +
> static ssize_t oversampling_mode_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -796,6 +782,7 @@ static const struct iio_info ad7380_info = {
> .read_raw = &ad7380_read_raw,
> .read_avail = &ad7380_read_avail,
> .write_raw = &ad7380_write_raw,
> + .get_current_scan_type = &ad7380_get_current_scan_type,
> .debugfs_reg_access = &ad7380_debugfs_reg_access,
> };
>
>

2024-05-08 17:21:33

by David Lechner

[permalink] [raw]

Subject: Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

On Wed, May 8, 2024 at 6:40 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Tue, 7 May 2024 14:02:08 -0500
> David Lechner <[email protected]> wrote:
>
> > 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 <[email protected]>
>
> 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 too many
> years :)
>
> It will get messy though in some cases as the device might have a PGA on 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.
>

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.)

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.

2024-05-19 19:17:18

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

2024-05-19 19:21:00

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

On Wed, 8 May 2024 12:21:09 -0500
David Lechner <[email protected]> wrote:

> On Wed, May 8, 2024 at 6:40 AM Jonathan Cameron
> <[email protected]> wrote:
> >
> > On Tue, 7 May 2024 14:02:08 -0500
> > David Lechner <[email protected]> wrote:
> >
> > > 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 <[email protected]>
> >
> > 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 too many
> > years :)
> >
> > It will get messy though in some cases as the device might have a PGA on 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.
> >
>
> 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.)
>
> 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.
>

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.

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

2024-05-20 13:59:23

by David Lechner

[permalink] [raw]

Subject: Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

On 5/19/24 2:20 PM, Jonathan Cameron wrote:
> On Wed, 8 May 2024 12:21:09 -0500
> David Lechner <[email protected]> wrote:
>
>> On Wed, May 8, 2024 at 6:40 AM Jonathan Cameron
>> <[email protected]> wrote:
>>>
>>> On Tue, 7 May 2024 14:02:08 -0500
>>> David Lechner <[email protected]> wrote:
>>>
>>>> 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 <[email protected]>
>>>
>>> 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 too many
>>> years :)
>>>
>>> It will get messy though in some cases as the device might have a PGA on 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.
>>>
>>
>> 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.)
>>
>> 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.
>>
>
> 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.
>
> 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

Great! I'll do a v2 with only the core code. Julian, Esteban and I
are all working on drivers that should make use of this. So if all goes
well, we should have 3 drivers for you this release cycle.