| Message ID | 20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com |
|---|---|
| Headers | show |
| Series | iio: adc: add new ad7380 driver | expand |
Hi Julien, On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote: > ad7380x(-4) parts are able to do oversampling to increase accuracy. > They support 2 average modes: normal average and rolling overage. > This commits focus on enabling normal average oversampling, which is the > default one. > > Normal averaging involves taking a number of samples, adding them together, > and dividing the result by the number of samples taken. > This result is then output from the device. The sample data is cleared when > the process completes. Because we need more samples to output a value, > the data output rate decrease with the oversampling ratio. > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > drivers/iio/adc/ad7380.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > index 020959759170..1e3869f5e48c 100644 > --- a/drivers/iio/adc/ad7380.c > +++ b/drivers/iio/adc/ad7380.c > @@ -88,7 +88,10 @@ struct ad7380_chip_info { > .type = IIO_VOLTAGE, \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .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_OVERSAMPLING_RATIO), \ > .indexed = 1, \ > .differential = (diff), \ > .channel = (diff) ? (2 * (index)) : (index), \ > @@ -156,6 +159,16 @@ static const struct ad7380_timing_specs ad7380_4_timing = { > .t_csh_ns = 20, > }; > > +/* > + * Available oversampling ratios. The indices correspond > + * with the bit value expected by the chip. > + * The available ratios depend on the averaging mode, > + * only normal averaging is supported for now > + */ > +static const int ad7380_normal_average_oversampling_ratios[] = { > + 1, 2, 4, 8, 16, 32, > +}; > + > static const struct ad7380_chip_info ad7380_chip_info = { > .name = "ad7380", > .channels = ad7380_channels, > @@ -231,6 +244,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = { > struct ad7380_state { > const struct ad7380_chip_info *chip_info; > struct spi_device *spi; > + unsigned int oversampling_ratio; nit: move this to the other 'unsigned int' fields... > struct regmap *regmap; > unsigned int vref_mv; > unsigned int vcm_mv[MAX_NUM_CHANNELS]; > @@ -386,6 +400,12 @@ static int ad7380_read_direct(struct ad7380_state *st, > }; > int ret; > > + /* > + * In normal average oversampling we need to wait for multiple conversions > to be done > + */ > + if (st->oversampling_ratio > 1) > + xfers[0].delay.value = T_CONVERT_NS + 500 * st- > >oversampling_ratio; > + > ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); > if (ret < 0) > return ret; > @@ -428,6 +448,91 @@ static int ad7380_read_raw(struct iio_dev *indio_dev, > / st->vref_mv; > > return IIO_VAL_INT; > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + *val = st->oversampling_ratio; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad7380_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, int *length, > + long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + *vals = ad7380_normal_average_oversampling_ratios; > + *length = ARRAY_SIZE(ad7380_normal_average_oversampling_ratios); > + *type = IIO_VAL_INT; > + > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > +} > + > +/** > + * check_osr - Check the oversampling ratio > + * @available_ratio: available ratios's array > + * @size: size of the available_ratio array > + * ratio: ratio to check > + * > + * Check if ratio is present in @available_ratio. Check for exact match. > + * @available_ratio is an array of the available ratios (depending on the > oversampling mode). > + * The indices must correspond with the bit value expected by the chip. > + */ > +static inline int check_osr(const int *available_ratio, int size, int ratio) Please name the function ad7380_check_osr(). Also, drop the inline... The compiler should be smart enough to take of that for us. > +{ > + int i; > + > + for (i = 0; i < size; i++) { > + if (ratio == available_ratio[i]) > + return i; > + } > + > + return -EINVAL; > +} > + > +static int ad7380_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + int ret, osr; > + > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + osr = check_osr(ad7380_normal_average_oversampling_ratios, > + ARRAY_SIZE(ad7380_normal_average_oversampling_rati > os), > + val); > + > + if (osr < 0) > + return osr; > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + ret = regmap_update_bits(st->regmap, > AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_OSR, > + FIELD_PREP(AD7380_CONFIG1_OSR, > osr)); > + > + if (ret) > + return ret; > + > + st->oversampling_ratio = val; > + > + /* > + * Perform a soft reset. > + * This will flush the oversampling block and FIFO but > will > + * maintain the content of the configurable registers. > + */ > + ret = regmap_update_bits(st->regmap, > AD7380_REG_ADDR_CONFIG2, > + AD7380_CONFIG2_RESET, > + FIELD_PREP(AD7380_CONFIG2_RESET, > + > AD7380_CONFIG2_RESET_SOFT)); > + } > + return 0; return ret; Or you may be asked to directly return in regmap_update_bits() and use unreachable() here. > default: > return -EINVAL; > } > @@ -435,6 +540,8 @@ static int ad7380_read_raw(struct iio_dev *indio_dev, > > static const struct iio_info ad7380_info = { > .read_raw = &ad7380_read_raw, > + .read_avail = &ad7380_read_avail, > + .write_raw = &ad7380_write_raw, > .debugfs_reg_access = &ad7380_debugfs_reg_access, > }; > > @@ -458,6 +565,12 @@ static int ad7380_init(struct ad7380_state *st, struct > regulator *vref) > if (ret < 0) > return ret; > > + /* Disable oversampling by default. > + * This is the default value after reset, > + * so just initialize internal data > + */ Your comment block is not in accordance with coding style. checkpatch should complain about this. > + st->oversampling_ratio = 1; > + > /* SPI 1-wire mode */ > return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > AD7380_CONFIG2_SDO, >
On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote: > Adds support for rolling average oversampling mode. > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of > the most recent samples in the averaging calculation, allowing the ADC > throughput rate and output data rate to stay the same, since we only need > to take only one sample for each new conversion. > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8 > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average) > > In order to be able to change the averaging mode, this commit also adds > the new "oversampling_mode" and "oversampling_mode_available" custom > attributes along with the according documentation file in > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard > attributes correspond to this use case. > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 | 38 ++++++ > MAINTAINERS | 1 + > drivers/iio/adc/ad7380.c | 143 +++++++++++++++++++-- > 3 files changed, 174 insertions(+), 8 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 > b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 > new file mode 100644 > index 000000000000..0a560ef3e32a > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 > @@ -0,0 +1,38 @@ > +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode > +KernelVersion: 6.9 > +Contact: Michael Hennerich <michael.hennerich@analog.com> > +Description: > + Writing this attribute sets the oversampling average mode. > + Reading it, shows the configured mode. > + Available modes can be displayed using the oversampling_mode_available > + attribute. > + When writing this attribute to change the oversampling mode, this will > + have the following side effects: > + > + - soft reset the ADC to flush the oversampling block and FIFO > + > + - the available oversampling ratios depend on the oversampling mode > + configured so to avoid misconfiguration, changing the mode will disable > + the oversampling by setting the ratio to 1. Hmm, can we somehow re-enable it again with a proper configuration (after the mode change)? > + > + - the list of available ratios (displayed by reading the > + oversampling_ratio_available attribute) will be updated when changing > + the oversampling mode. > + > +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode_available > +KernelVersion: 6.9 > +Contact: Michael Hennerich <michael.hennerich@analog.com> > +Description: > + Display the available oversampling average modes. The two available modes > + are "normal" and "rolling" where "normal" average mode is the default one. > + > + - normal averaging involves taking a number of samples, adding them > + together, and dividing the result by the number of samples taken. > + This result is then output from the device. The sample data is cleared > + when the process completes. Because we need more samples to output a > + value, the data output rate decrease with the oversampling ratio. > + > + - rolling oversampling mode uses a first in, first out (FIFO) buffer of > + the most recent samples in the averaging calculation, allowing the ADC > + throughput rate and output data rate to stay the same, since we only need > + to take only one sample for each new conversion. > diff --git a/MAINTAINERS b/MAINTAINERS > index 87724a9e9f9f..ca1e115f2aff 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -434,6 +434,7 @@ R: David Lechner <dlechner@baylibre.com> > S: Supported > W: > https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x > W: https://ez.analog.com/linux-software-drivers > +F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 > F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml > F: drivers/iio/adc/ad7380.c > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > index 1e3869f5e48c..7b021bb9cf87 100644 > --- a/drivers/iio/adc/ad7380.c > +++ b/drivers/iio/adc/ad7380.c > @@ -51,6 +51,8 @@ > #define AD7380_REG_ADDR_ALERT_HIGH_TH 0x5 > > #define AD7380_CONFIG1_OS_MODE BIT(9) > +#define OS_MODE_NORMAL_AVERAGE 0 > +#define OS_MODE_ROLLING_AVERAGE 1 no AD7380 prefix? > #define AD7380_CONFIG1_OSR GENMASK(8, 6) > #define AD7380_CONFIG1_CRC_W BIT(5) > #define AD7380_CONFIG1_CRC_R BIT(4) > @@ -159,16 +161,27 @@ static const struct ad7380_timing_specs ad7380_4_timing = { > .t_csh_ns = 20, > }; > ... > > +static ssize_t oversampling_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct ad7380_state *st = iio_priv(indio_dev); > + int os_mode, ret; > + > + ret = sysfs_match_string(ad7380_oversampling_average_modes, buf); > + if (ret < 0) > + return ret; > + > + os_mode = ret; > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_OS_MODE, > + FIELD_PREP(AD7380_CONFIG1_OS_MODE, > os_mode)); > + > + if (ret) > + return ret; > + > + st->oversampling_mode = os_mode; > + > + /* > + * Oversampling ratio depends on oversampling mode, to avoid > + * misconfiguration when changing oversampling mode, > + * disable oversampling by setting OSR to 0. > + */ Given the comment, I would expect you to disable osr before changing the mode. > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_OSR, > FIELD_PREP(AD7380_CONFIG1_OSR, 0)); > + > + if (ret) > + return ret; > + > + st->oversampling_ratio = 1; > + 1? Also, it would be nice if we could re-enabled osr. So the flow would maybe be: 1) disable osr; 2) change mode; 3) do whatever we need so configuration makes sense for new mode? 4) re-enable osr; 5) soft-reset; Also not sure if 4) and 5) are in the correct order. > + /* > + * Perform a soft reset. > + * This will flush the oversampling block and FIFO but will > + * maintain the content of the configurable registers. > + */ > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > + AD7380_CONFIG2_RESET, > + FIELD_PREP(AD7380_CONFIG2_RESET, > + AD7380_CONFIG2_RESET_SOFT)); > + } > + return ret ?: len; > +} > + > +static ssize_t oversampling_mode_available_show(struct device *dev, > + struct device_attribute *attr, > char *buf) > +{ > + int i; > + size_t len = 0; > + > + for (i = 0; i < ARRAY_SIZE(ad7380_oversampling_average_modes); i++) > + len += sysfs_emit_at(buf, len, "%s ", > ad7380_oversampling_average_modes[i]); > + > + buf[len - 1] = '\n'; > + > + return len; > +} > + > +static IIO_DEVICE_ATTR_RW(oversampling_mode, 0); > +static IIO_DEVICE_ATTR_RO(oversampling_mode_available, 0); > + > +static struct attribute *ad7380_attributes[] = { > + &iio_dev_attr_oversampling_mode.dev_attr.attr, > + &iio_dev_attr_oversampling_mode_available.dev_attr.attr, > + NULL > +}; > + Maybe use IIO_ENUM... It allows the core to do some of the stuff you're doing for you. - Nuno Sá
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@baylibre.com> > > --- > > 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á
On Mon, 06 May 2024 10:55:46 +0200 Nuno Sá <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 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@baylibre.com> > > > > --- > > > > 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. 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 whether 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 really don't care enough to make them work :( > > > 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? You have lost me on this one. Why does the timestamp need to be in a consistent location? We have lots of drivers where it moves about between different chips they support, or indeed what channels are currently turned on. I haven't actually looked at the latest code yet, so maybe it will become obvious! Jonathan
On Wed, 01 May 2024 16:55:35 +0200 Julien Stephan <jstephan@baylibre.com> wrote: > From: David Lechner <dlechner@baylibre.com> > > This adds a new driver for the AD7380 family ADCs. > > The driver currently implements basic support for the AD7380, AD7381, > 2-channel differential ADCs. Support for additional single-ended, > pseudo-differential and 4-channel chips that use the same register map > as well as additional features of the chip will be added in future patches. > > Co-developed-by: Stefan Popa <stefan.popa@analog.com> > Signed-off-by: Stefan Popa <stefan.popa@analog.com> > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > Signed-off-by: David Lechner <dlechner@baylibre.com> > [Julien Stephan: add datasheet links of supported parts] > [Julien Stephan: fix rx/tx buffer for regmap access] > [Julien Stephan: fix scale issue] > [Julien Stephan: use the new iio_device_claim_direct_scoped > instead of iio_device_claim_direct_mode] > Signed-off-by: Julien Stephan <jstephan@baylibre.com> I took another look, mostly so I could refresh my memory of the driver before getting to the newer patches. A few minor things inline, + I wondering if the structure for the scan is what was meant about keeping timestamps in the same place. > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > new file mode 100644 > index 000000000000..a218f59c276e > --- /dev/null > +++ b/drivers/iio/adc/ad7380.c > @@ -0,0 +1,443 @@ > + > +#include <linux/bitfield.h> > +#include <linux/bitops.h> > +#include <linux/cleanup.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <linux/spi/spi.h> > +#include <linux/sysfs.h> > + > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> This and linux/sysfs.h are are normally only needed it you are providing custom attrs. I don't see those here so not sure those headers are used directly by this driver. > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > + > +struct ad7380_state { > + const struct ad7380_chip_info *chip_info; > + struct spi_device *spi; > + struct regmap *regmap; > + unsigned int vref_mv; > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + * Make the buffer large enough for 2 16-bit samples and one 64-bit > + * aligned 64 bit timestamp. > + */ > + struct { > + u16 raw[2]; > + > + s64 ts __aligned(8); Ah is the comment in patch 10 about consistent timestamps coming from this? If so then don't worry about that - you just loose the pretty option of using a structure to define the data layout. union { u16 raw[4 + 4] __aligned(8); /* include the ts*/ u32 raw[4 + 2] __aligned(8); /* include ts */ } scan_data __aligned(IIO_DMA_MINALIGN); There are lots of drivers that are highly flexible in how many channels we have so they always have to take this more manual path to data sizing. Some do magic with ALIGN() so if you want you could do something like that here. That magic ensures the right amount of padding to have the naturally aligned 8 byte timestamp in an array of smaller type. Note you can't solve this by putting the timestamp first because it is always optional whether it is enabled (that bit is just dealt with by the IIO core rather than a driver). > + } scan_data __aligned(IIO_DMA_MINALIGN); > + u16 tx; > + u16 rx; > +}; > +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg, > + u32 writeval, u32 *readval) > +{ > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + struct ad7380_state *st = iio_priv(indio_dev); > + int ret; > + > + if (readval) > + ret = regmap_read(st->regmap, reg, readval); Trivial, but return regmap_read() > + else > + ret = regmap_write(st->regmap, reg, writeval); return regmap_write() unless something gets added here later in the series. > + > + return ret; > + } > + unreachable(); > +}
On Wed, 01 May 2024 16:55:41 +0200 Julien Stephan <jstephan@baylibre.com> wrote: > ad7380x(-4) parts are able to do oversampling to increase accuracy. > They support 2 average modes: normal average and rolling overage. > This commits focus on enabling normal average oversampling, which is the > default one. The other case got me curious. If you do want to support the rolling average in future, it could probably be handled as a low pass filter control rather than a form of oversampling. Anyhow, not relevant here! > > Normal averaging involves taking a number of samples, adding them together, > and dividing the result by the number of samples taken. > This result is then output from the device. The sample data is cleared when > the process completes. Because we need more samples to output a value, > the data output rate decrease with the oversampling ratio. > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> Hi Julien. A few additional comments inline. Jonathan > --- > drivers/iio/adc/ad7380.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > index 020959759170..1e3869f5e48c 100644 > --- a/drivers/iio/adc/ad7380.c > +++ b/drivers/iio/adc/ad7380.c > @@ -88,7 +88,10 @@ struct ad7380_chip_info { > .type = IIO_VOLTAGE, \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .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_OVERSAMPLING_RATIO), \ > .indexed = 1, \ > .differential = (diff), \ > .channel = (diff) ? (2 * (index)) : (index), \ > @@ -156,6 +159,16 @@ static const struct ad7380_timing_specs ad7380_4_timing = { > .t_csh_ns = 20, > }; > > +/* > + * Available oversampling ratios. The indices correspond > + * with the bit value expected by the chip. > + * The available ratios depend on the averaging mode, > + * only normal averaging is supported for now > + */ > +static const int ad7380_normal_average_oversampling_ratios[] = { > + 1, 2, 4, 8, 16, 32, > +}; > + > static const struct ad7380_chip_info ad7380_chip_info = { > .name = "ad7380", > .channels = ad7380_channels, > @@ -231,6 +244,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = { > struct ad7380_state { > const struct ad7380_chip_info *chip_info; > struct spi_device *spi; > + unsigned int oversampling_ratio; > struct regmap *regmap; > unsigned int vref_mv; > unsigned int vcm_mv[MAX_NUM_CHANNELS]; > @@ -386,6 +400,12 @@ static int ad7380_read_direct(struct ad7380_state *st, > }; > int ret; > > + /* > + * In normal average oversampling we need to wait for multiple conversions to be done Wrap comment at 80 chars. Generally I prefer we keep to old limit of 80 unless there is a readability advantage. I don't see such an advantage in this case. > + */ > + if (st->oversampling_ratio > 1) > + xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio; > + > + > +/** > + * check_osr - Check the oversampling ratio > + * @available_ratio: available ratios's array > + * @size: size of the available_ratio array > + * ratio: ratio to check > + * > + * Check if ratio is present in @available_ratio. Check for exact match. > + * @available_ratio is an array of the available ratios (depending on the oversampling mode). > + * The indices must correspond with the bit value expected by the chip. > + */ > +static inline int check_osr(const int *available_ratio, int size, int ratio) > +{ > + int i; > + > + for (i = 0; i < size; i++) { > + if (ratio == available_ratio[i]) > + return i; > + } > + > + return -EINVAL; > +} > + > +static int ad7380_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + int ret, osr; > + > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + osr = check_osr(ad7380_normal_average_oversampling_ratios, Nuno already pointed out function name should be prefixed. > + ARRAY_SIZE(ad7380_normal_average_oversampling_ratios), > + val); If this is just checking, why does it return osr? Feels like name needs to be ad7380_osr_to_regval() or something like that. > + > + if (osr < 0) > + return osr; > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_OSR, > + FIELD_PREP(AD7380_CONFIG1_OSR, osr)); > + > + if (ret) > + return ret; > + > + st->oversampling_ratio = val; > + > + /* > + * Perform a soft reset. > + * This will flush the oversampling block and FIFO but will > + * maintain the content of the configurable registers. > + */ > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > + AD7380_CONFIG2_RESET, > + FIELD_PREP(AD7380_CONFIG2_RESET, > + AD7380_CONFIG2_RESET_SOFT)); > + } > + return 0; > default: > return -EINVAL; > } > @@ -435,6 +540,8 @@ static int ad7380_read_raw(struct iio_dev *indio_dev, > > static const struct iio_info ad7380_info = { > .read_raw = &ad7380_read_raw, > + .read_avail = &ad7380_read_avail, > + .write_raw = &ad7380_write_raw, > .debugfs_reg_access = &ad7380_debugfs_reg_access, > }; > > @@ -458,6 +565,12 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref) > if (ret < 0) > return ret; > > + /* Disable oversampling by default. IIO comment syntax is /* * Disable oversampling by default. Also, curiously short lines that could definitely be wrapped nearer 80 chars! > + * This is the default value after reset, > + * so just initialize internal data > + */ > + st->oversampling_ratio = 1; > + > /* SPI 1-wire mode */ > return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > AD7380_CONFIG2_SDO, >
On Wed, 01 May 2024 16:55:42 +0200 Julien Stephan <jstephan@baylibre.com> wrote: > Adds support for rolling average oversampling mode. > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of > the most recent samples in the averaging calculation, allowing the ADC > throughput rate and output data rate to stay the same, since we only need > to take only one sample for each new conversion. > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8 > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average) Ah. I should have read on! > > In order to be able to change the averaging mode, this commit also adds > the new "oversampling_mode" and "oversampling_mode_available" custom > attributes along with the according documentation file in > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard > attributes correspond to this use case. This comes to the comment I stuck in the previous patch. To most people this is not a form of oversampling because the data rate remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at: https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter in_voltage_low_pass_3db_frequency would be the most appropriate standard ABI for this if we do treat it as a low pass filter control. I'm not necessarily saying we don't want new ABI for this, but I would like to consider the pros and cons of just using the 3db frequency. So would that work for this part or am I missing something? > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 | 38 ++++++ > MAINTAINERS | 1 + > drivers/iio/adc/ad7380.c | 143 +++++++++++++++++++-- > 3 files changed, 174 insertions(+), 8 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 > new file mode 100644 > index 000000000000..0a560ef3e32a > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 > @@ -0,0 +1,38 @@ > +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode > +KernelVersion: 6.9 > +Contact: Michael Hennerich <michael.hennerich@analog.com> > +Description: > + Writing this attribute sets the oversampling average mode. > + Reading it, shows the configured mode. > + Available modes can be displayed using the oversampling_mode_available > + attribute. > + When writing this attribute to change the oversampling mode, this will > + have the following side effects: Where possible, write ABI docs with the assumption we will generalize them in future. Annoyingly the documentation system doesn't allow for multiple descriptions. As such, additional information like this doesn't belong in the ABI docs. > + > + - soft reset the ADC to flush the oversampling block and FIFO I think this was already picked up on in another review, but my inclination is make this something you can't change with the buffer enabled. The results will be rather unpredictable anyway and it will simplify the handling a little to just block that corner with a claim (or failure to claim) direct mode when setting this. > + > + - the available oversampling ratios depend on the oversampling mode > + configured so to avoid misconfiguration, changing the mode will disable > + the oversampling by setting the ratio to 1. Better to get a close as possible. If they've configured it to something we can't do then it's user error. If they have picked a value that is still possible then allowing that is a nice usability improvement. > + > + - the list of available ratios (displayed by reading the > + oversampling_ratio_available attribute) will be updated when changing > + the oversampling mode. In general an ABI element is allowed to modify any other (because this sort of chaining is common.) As such I don't think this needs to be in the ABI docs but would be a useful detail to add to a chip specific main document elsewhere. > + > +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode_available > +KernelVersion: 6.9 > +Contact: Michael Hennerich <michael.hennerich@analog.com> > +Description: > + Display the available oversampling average modes. The two available modes > + are "normal" and "rolling" where "normal" average mode is the default one. > + > + - normal averaging involves taking a number of samples, adding them > + together, and dividing the result by the number of samples taken. > + This result is then output from the device. The sample data is cleared > + when the process completes. Because we need more samples to output a > + value, the data output rate decrease with the oversampling ratio. > + > + - rolling oversampling mode uses a first in, first out (FIFO) buffer of > + the most recent samples in the averaging calculation, allowing the ADC > + throughput rate and output data rate to stay the same, since we only need > + to take only one sample for each new conversion. If we keep this, I wonder if "moving" or "rolling" is the more common term for this. > + > +static IIO_DEVICE_ATTR_RW(oversampling_mode, 0); > +static IIO_DEVICE_ATTR_RO(oversampling_mode_available, 0); > + > +static struct attribute *ad7380_attributes[] = { > + &iio_dev_attr_oversampling_mode.dev_attr.attr, > + &iio_dev_attr_oversampling_mode_available.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group ad7380_attribute_group = { > + .attrs = ad7380_attributes, > +}; Bring the sysfs includes in here rather than in the original driver patch. Thanks, Jonathan
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á <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 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@baylibre.com> > > > > > > --- > > > > > > 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. > > 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 whether 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 really > don't care enough to make them work :( > > > > > > 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? > > You have lost me on this one. Why does the timestamp need to be in a consistent > 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_timestamp() 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 around. So do something similar here. Jonathan > > I haven't actually looked at the latest code yet, so maybe it will become > obvious! > > Jonathan > > >
On Wed, 01 May 2024 16:55:43 +0200 Julien Stephan <jstephan@baylibre.com> 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@baylibre.com> > > --- > > 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? > > 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? Questions discussed in another branch of the thread. Jonathan > > 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 If the field is defined, the values should be obvious. Also you used this as a boolean where simply passing in true or false would be less confusing. > #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; > }; > @@ -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); As per earlier comments, roll this timestamp into the union as well because it will move around. > } 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_set_resolution_boost(struct iio_dev *indio_dev, bool enable) You pass 1 or 0 in here rather than true or false which would make more sense. > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + int ret; > + > + if (st->resolution_boost_enable == enable) > + return 0; > + > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_RES, > + FIELD_PREP(AD7380_CONFIG1_RES, enable)); Mapping true / false to 1 / 0 whilst correct doesn't give particularly readable code. So useful to just have an enable ? 1 : 0 in there to make the mapping more obvious. > + > + if (ret) > + return ret; > + > + st->resolution_boost_enable = enable; Trivial: blank line here. > + return 0; > +} > > static int ad7380_init(struct ad7380_state *st, struct regulator *vref) > { > int ret; > @@ -691,12 +849,16 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref) > if (ret < 0) > return ret; > > - /* Disable oversampling by default. > - * This is the default value after reset, > + /* Disable oversampling and resolution boost by default. Follow through from earlier. Wrong comment syntax + wrap lines nearer 80 chars. > + * This are the default values after reset, > * so just initialize internal data > */
FYI, Julien is AFK for a bit so I'll try to respond to the non-trivial comments. On Mon, May 6, 2024 at 8:46 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 06 May 2024 10:55:46 +0200 > Nuno Sá <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 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@baylibre.com> > > > > > > --- > > > > > > 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. > > 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 whether 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 really > 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.
On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 01 May 2024 16:55:42 +0200 > Julien Stephan <jstephan@baylibre.com> wrote: > > > Adds support for rolling average oversampling mode. > > > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of > > the most recent samples in the averaging calculation, allowing the ADC > > throughput rate and output data rate to stay the same, since we only need > > to take only one sample for each new conversion. > > > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8 > > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average) > > Ah. I should have read on! > > > > > In order to be able to change the averaging mode, this commit also adds > > the new "oversampling_mode" and "oversampling_mode_available" custom > > attributes along with the according documentation file in > > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard > > attributes correspond to this use case. > > This comes to the comment I stuck in the previous patch. > > To most people this is not a form of oversampling because the data rate > remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at: > https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter > > in_voltage_low_pass_3db_frequency would be the most appropriate standard > ABI for this if we do treat it as a low pass filter control. > > I'm not necessarily saying we don't want new ABI for this, but I would > like to consider the pros and cons of just using the 3db frequency. > > So would that work for this part or am I missing something? > I like the idea. But from the link, it looks like the 3dB frequency depends on the sampling frequency which is unknown (e.g. could come from hrtimer trigger). Would it be reasonable to calculate the 3db frequency at the max sample rate that the chip allows and just use those numbers?
On Mon, May 6, 2024 at 3:55 AM Nuno Sá <noname.nuno@gmail.com> wrote: > > On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote: ... > > + /* > > + * 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? > This case is handled by the outermost if, so can't have that here. (In that case, no manipulation is required so the whole big if statement is skipped). realbits will never be < bits_per_word.
On Wed, May 1, 2024 at 9:56 AM Julien Stephan <jstephan@baylibre.com> wrote: > ... > > +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); This needs to be based on storagebits, not realbits. > + > + return bits_per_word; > +} > +
On Mon, 6 May 2024 10:04:10 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 01 May 2024 16:55:42 +0200 > > Julien Stephan <jstephan@baylibre.com> wrote: > > > > > Adds support for rolling average oversampling mode. > > > > > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of > > > the most recent samples in the averaging calculation, allowing the ADC > > > throughput rate and output data rate to stay the same, since we only need > > > to take only one sample for each new conversion. > > > > > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8 > > > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average) > > > > Ah. I should have read on! > > > > > > > > In order to be able to change the averaging mode, this commit also adds > > > the new "oversampling_mode" and "oversampling_mode_available" custom > > > attributes along with the according documentation file in > > > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard > > > attributes correspond to this use case. > > > > This comes to the comment I stuck in the previous patch. > > > > To most people this is not a form of oversampling because the data rate > > remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at: > > https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter > > > > in_voltage_low_pass_3db_frequency would be the most appropriate standard > > ABI for this if we do treat it as a low pass filter control. > > > > I'm not necessarily saying we don't want new ABI for this, but I would > > like to consider the pros and cons of just using the 3db frequency. > > > > So would that work for this part or am I missing something? > > > > I like the idea. But from the link, it looks like the 3dB frequency > depends on the sampling frequency which is unknown (e.g. could come > from hrtimer trigger). > > Would it be reasonable to calculate the 3db frequency at the max > sample rate that the chip allows and just use those numbers? > Ah. So looking at datasheet the normal average oversampling is self clocked, but this version is not. So, I'll ask the dumb question. What is this feature for? We have to pump the SPI bus anyway why not just do the maths in userspace? Oversampling is normally about data rate reduction with a bonus in precision obtained. Jonathan
On Mon, 6 May 2024 09:44:25 -0500 David Lechner <dlechner@baylibre.com> wrote: > FYI, Julien is AFK for a bit so I'll try to respond to the non-trivial comments. > > On Mon, May 6, 2024 at 8:46 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Mon, 06 May 2024 10:55:46 +0200 > > Nuno Sá <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 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@baylibre.com> > > > > > > > > --- > > > > > > > > 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. > > > > 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 whether 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 really > > 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. I think you'd use those sysfs files for the control as well. So write to the *_type for a channel. That might well effect a bunch of other channels. Another interface would perhaps be more confusing than anything and something simple won't be specific enough as we are sure to get a device with multiple channels only some of which have variable scale. Definitely don't allow such a write when buffered capture is going on as that would be chaos. Also for this one we'd want control over the file attributes for those files so they are only writeable if we support changing this (which is rare). Also need a way of knowing what values are available so probably *_type_available - the presence of that also indicating if the scale can be changed. J >
On Wed, May 8, 2024 at 6:26 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Mon, 6 May 2024 10:04:10 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Wed, 01 May 2024 16:55:42 +0200 > > > Julien Stephan <jstephan@baylibre.com> wrote: > > > > > > > Adds support for rolling average oversampling mode. > > > > > > > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of > > > > the most recent samples in the averaging calculation, allowing the ADC > > > > throughput rate and output data rate to stay the same, since we only need > > > > to take only one sample for each new conversion. > > > > > > > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8 > > > > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average) > > > > > > Ah. I should have read on! > > > > > > > > > > > In order to be able to change the averaging mode, this commit also adds > > > > the new "oversampling_mode" and "oversampling_mode_available" custom > > > > attributes along with the according documentation file in > > > > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard > > > > attributes correspond to this use case. > > > > > > This comes to the comment I stuck in the previous patch. > > > > > > To most people this is not a form of oversampling because the data rate > > > remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at: > > > https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter > > > > > > in_voltage_low_pass_3db_frequency would be the most appropriate standard > > > ABI for this if we do treat it as a low pass filter control. > > > > > > I'm not necessarily saying we don't want new ABI for this, but I would > > > like to consider the pros and cons of just using the 3db frequency. > > > > > > So would that work for this part or am I missing something? > > > > > > > I like the idea. But from the link, it looks like the 3dB frequency > > depends on the sampling frequency which is unknown (e.g. could come > > from hrtimer trigger). > > > > Would it be reasonable to calculate the 3db frequency at the max > > sample rate that the chip allows and just use those numbers? > > > Ah. So looking at datasheet the normal average oversampling is > self clocked, but this version is not. > > So, I'll ask the dumb question. What is this feature for? > We have to pump the SPI bus anyway why not just do the maths in > userspace? Oversampling is normally about data rate reduction > with a bonus in precision obtained. > > Jonathan > I asked the apps engineers and the answer I got is that it a way to enable oversampling while still maintaining a high sample rate. Another thing to consider here is that we can only enable the extra resolution bits if oversampling is enabled (normal or rolling mode). The chip might not work right if we try to enable the extra bits without oversampling enabled. So my thinking is perhaps it is better to keep the rolling mode as oversampling rather than trying to call it a low pass filter. As you said, normal mode is about data rate reduction with bonus precision. Rolling average oversampling mode then would then just be for the bonus precision.
On Thu, 9 May 2024 17:01:14 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Wed, May 8, 2024 at 6:26 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Mon, 6 May 2024 10:04:10 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > On Wed, 01 May 2024 16:55:42 +0200 > > > > Julien Stephan <jstephan@baylibre.com> wrote: > > > > > > > > > Adds support for rolling average oversampling mode. > > > > > > > > > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of > > > > > the most recent samples in the averaging calculation, allowing the ADC > > > > > throughput rate and output data rate to stay the same, since we only need > > > > > to take only one sample for each new conversion. > > > > > > > > > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8 > > > > > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average) > > > > > > > > Ah. I should have read on! > > > > > > > > > > > > > > In order to be able to change the averaging mode, this commit also adds > > > > > the new "oversampling_mode" and "oversampling_mode_available" custom > > > > > attributes along with the according documentation file in > > > > > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard > > > > > attributes correspond to this use case. > > > > > > > > This comes to the comment I stuck in the previous patch. > > > > > > > > To most people this is not a form of oversampling because the data rate > > > > remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at: > > > > https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter > > > > > > > > in_voltage_low_pass_3db_frequency would be the most appropriate standard > > > > ABI for this if we do treat it as a low pass filter control. > > > > > > > > I'm not necessarily saying we don't want new ABI for this, but I would > > > > like to consider the pros and cons of just using the 3db frequency. > > > > > > > > So would that work for this part or am I missing something? > > > > > > > > > > I like the idea. But from the link, it looks like the 3dB frequency > > > depends on the sampling frequency which is unknown (e.g. could come > > > from hrtimer trigger). > > > > > > Would it be reasonable to calculate the 3db frequency at the max > > > sample rate that the chip allows and just use those numbers? > > > > > Ah. So looking at datasheet the normal average oversampling is > > self clocked, but this version is not. > > > > So, I'll ask the dumb question. What is this feature for? > > We have to pump the SPI bus anyway why not just do the maths in > > userspace? Oversampling is normally about data rate reduction > > with a bonus in precision obtained. > > > > Jonathan > > > > I asked the apps engineers and the answer I got is that it a way to > enable oversampling while still maintaining a high sample rate. If I read it correctly (and based on how this is done on some other devices) it's exactly the same as not enabling this mode and in software adding up the last 8 readings (so software oversampling). Data rate is the same and arguably the larger spi transfers that might result from their solution are more expensive than doing the sum on the CPU. A long time back there was IIRC some discussion of implementing this sort of filtering as a pluggable component between an IIO device driver and the kfifo - idea being to reduce data being passed to userspace - there would be no point in doing this variant though as the data rate isn't reduced. It would be easy to do as a buffer consumer IIO device that just does maths on incoming data before providing it's own kfifo, but no one ever implemented it. > > Another thing to consider here is that we can only enable the extra > resolution bits if oversampling is enabled (normal or rolling mode). > The chip might not work right if we try to enable the extra bits > without oversampling enabled. I think the key question is where do those extra bits come from? If this is conventional oversampling then we just sum up the readings and divide by how many there are. (A1 + A2 + A3 + A4 + A5 + A6 + A7) / 8 So sum plus shift right 3. However you don't actually do the div 8 you just change the scale to reflect that your new LSB is 1/8th of that before giving greater precision. This is giving 2 extra bits, so it's slightly worse than the 3 we'd get doing it in software (trade off to keep the SPI transfers smaller). So they are either dropping the LSB or just possibly dithering it. So far I'm not seeing a strong reason to support this functionality. > > So my thinking is perhaps it is better to keep the rolling mode as > oversampling rather than trying to call it a low pass filter. As you > said, normal mode is about data rate reduction with bonus precision. > Rolling average oversampling mode then would then just be for the > bonus precision. True if there actually is any. I took another close read through the datasheet sections and the description aligns with my assumptions above. Jonathan
Taking over this series with David Lechner's approval, to add some fixes, proper handling of pseudo differential parts and some extra commits to add support for 4-channel compatible parts, oversampling and resolution boost. We are happy with most of the series other than perhaps patch 10/10 (adding the resolution boost feature), so adding RFC prefix as suggested by Jonathan. This patch changes the userspace interface which is why we didn't want the earlier patches to be applied yet. More details on this, on the corresponding patch 10/10 commit message. Here is David's cover letter: This series is adding a new driver for the Analog Devices Inc. AD7380, AD7381, AD7383, and AD7384 ADCs. These chips are part of a family of simultaneous sampling SAR ADCs. To keep things simple, the initial driver implementation only supports the 2/4-channel differential chips listed above. There are also 4-channel single-ended chips in the family that can be added later. Furthermore, the driver is just implementing basic support for capturing data. Additional features like interrupts, CRC, etc. can be added later. This work is being done by BayLibre and on behalf of Analog Devices Inc. hence the maintainers are @analog.com. Changes in v6: - Added a comment for math in IIO_CHAN_INFO_OFFSET - Added a comment for raw buffer - use iio_device_claim_direct_scoped instead of iio_device_claim_direct_mode Adding the following commits on top of v5: - add oversampling support - add resolution boost support - Link to v5: https://lore.kernel.org/r/20240319-adding-new-ad738x-driver-v5-0-ce7df004ceb3@baylibre.com Changes in v5: - make ad7380_regmap_config static Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202401280629.5kknB57C-lkp@intel.com/ - don't use bool in FIELD_PREP Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202401280629.5kknB57C-lkp@intel.com/ - fix rx/tx buffer for regmap access - add datasheet links of supported parts - move reading reference voltage to probe function - removed DIFFERENTIAL from a few macro names Adding the following commits on top of the v4 - add supplies for pseudo-differential chips - prepare driver to add more compatible parts - add support for 4-channel compatible parts - Link to v4: https://lore.kernel.org/all/20240110-ad7380-mainline-v4-0-93a1d96b50fa@baylibre.com Changes in v4: - Dropped SPI bindings patch. - Removed `spi-rx-bus-channels` from the adi,ad7380 bindings. - Link to v3: https://lore.kernel.org/r/20231215-ad7380-mainline-v3-0-7a11ebf642b9@baylibre.com Changes in v3: - dt-bindings: - Picked up Conor's Reviewed-By on the adi,ad7380 bindings - driver: - Removed extra indent in DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL macro - Removed scan mask that included timestamp channel - Removed parent device assignment - Picked up Nuno's Reviewed-by - Link to v2: https://lore.kernel.org/r/20231213-ad7380-mainline-v2-0-cd32150d84a3@baylibre.com Changes in v2: - dt-bindings: - Added new patch with generic spi-rx-bus-channels property - Added maxItems to reg property - Replaced adi,sdo-mode property with spi-rx-bus-channels - Made spi-rx-bus-channels property optional with default value of 1 (this made the if: check more complex) - Changed example to use gpio for interrupt - driver: - Fixed CONFIG_AD7380 in Makefile - rx_buf = st->scan_data.raw instead of rx_buf = &st->scan_data - Moved iio_push_to_buffers_with_timestamp() outside of if statement - Removed extra blank lines - Renamed regulator disable function - Dropped checking of adi,sdo-mode property (regardless of the actual wiring, we can always use 1-wire mode) - Added available_scan_masks - Added check for missing driver match data - Link to v1: https://lore.kernel.org/r/20231208-ad7380-mainline-v1-0-2b33fe2f44ae@baylibre.com To: Lars-Peter Clausen <lars@metafoo.de> To: Michael Hennerich <Michael.Hennerich@analog.com> To: Nuno Sá <nuno.sa@analog.com> To: David Lechner <dlechner@baylibre.com> To: Jonathan Cameron <jic23@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Liam Girdwood <lgirdwood@gmail.com> To: Mark Brown <broonie@kernel.org> Cc: linux-iio@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Julien Stephan <jstephan@baylibre.com> --- David Lechner (4): dt-bindings: iio: adc: Add binding for AD7380 ADCs iio: adc: ad7380: new driver for AD7380 ADCs dt-bindings: iio: adc: ad7380: add pseudo-differential parts iio: adc: ad7380: add support for pseudo-differential parts Julien Stephan (6): iio: adc: ad7380: prepare for parts with more channels dt-bindings: iio: adc: ad7380: add support for ad738x-4 4 channels variants iio: adc: ad7380: add support for ad738x-4 4 channels variants iio: adc: ad7380: add oversampling support iio: adc: ad7380: add support for rolling average oversampling mode iio: adc: ad7380: add support for resolution boost Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 | 38 + .../devicetree/bindings/iio/adc/adi,ad7380.yaml | 148 +++ MAINTAINERS | 11 + drivers/iio/adc/Kconfig | 16 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7380.c | 1019 ++++++++++++++++++++ 6 files changed, 1233 insertions(+) --- base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91 change-id: 20240311-adding-new-ad738x-driver-5f9b54ad7c74 Best regards,