mbox series

[RFC,v6,00/10] iio: adc: add new ad7380 driver

Message ID 20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com
Headers show
Series iio: adc: add new ad7380 driver | expand

Message

Julien Stephan May 1, 2024, 2:55 p.m. UTC
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,

Comments

Nuno Sá May 6, 2024, 8:20 a.m. UTC | #1
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,
>
Nuno Sá May 6, 2024, 8:33 a.m. UTC | #2
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á
Nuno Sá May 6, 2024, 8:55 a.m. UTC | #3
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á
Jonathan Cameron May 6, 2024, 1:46 p.m. UTC | #4
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
Jonathan Cameron May 6, 2024, 1:56 p.m. UTC | #5
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();
> +}
Jonathan Cameron May 6, 2024, 2:05 p.m. UTC | #6
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,
>
Jonathan Cameron May 6, 2024, 2:17 p.m. UTC | #7
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
Jonathan Cameron May 6, 2024, 2:20 p.m. UTC | #8
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
> 
> 
>
Jonathan Cameron May 6, 2024, 2:29 p.m. UTC | #9
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
>  	 */
David Lechner May 6, 2024, 2:44 p.m. UTC | #10
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.
David Lechner May 6, 2024, 3:04 p.m. UTC | #11
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?
David Lechner May 6, 2024, 3:09 p.m. UTC | #12
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.
David Lechner May 6, 2024, 9:45 p.m. UTC | #13
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;
> +}
> +
Jonathan Cameron May 8, 2024, 11:25 a.m. UTC | #14
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
Jonathan Cameron May 8, 2024, 11:32 a.m. UTC | #15
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


>
David Lechner May 9, 2024, 10:01 p.m. UTC | #16
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.
Jonathan Cameron May 11, 2024, 11:47 a.m. UTC | #17
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