| Message ID | 20240501-adding-new-ad738x-driver-v6-8-3c0741154728@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| 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, 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, >
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 + */ + 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) +{ + 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_ratios), + 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; 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 + */ + st->oversampling_ratio = 1; + /* SPI 1-wire mode */ return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, AD7380_CONFIG2_SDO,
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(-)