[PATCH v2 0/2] iio: adc: ad4695: add oversampling support

[PATCH v2 0/2] iio: adc: ad4695: add oversampling support

[Trevor Gamblin]

Add driver logic and documentation for the oversampling feature of the
AD469x parts from Analog Devices. For now, this only works with offload
support, and takes advantage of that mode's higher performance to make
oversampling possible on multiple channels with varying sampling
frequencies. Some significant rework of the driver had to be done in
order to conditionally support this feature, including use of
iio_scan_types to help determine the appropriate spi message
configurations depending on oversampling ratio.

This series depends on David's recent SPI engine changes for adding
offload support:

https://lore.kernel.org/all/20241211-dlech-mainline-spi-engine-offload-2-v6-0-88ee574d5d03@baylibre.com/

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
Changes in v2:
- Removed section in cover letter about correct implementation
- Simplify math of ad4695_get_calibbias() in ad4695.c, based on
  Jonathan and David's suggestions
- Link to v1: https://lore.kernel.org/r/20241217-ad4695-oversampling-v1-0-0b045d835dac@baylibre.com

---
Trevor Gamblin (2):
      iio: adc: ad4695: add offload-based oversampling support
      doc: iio: ad4695: describe oversampling support

 Documentation/iio/ad4695.rst |  36 ++++-
 drivers/iio/adc/ad4695.c     | 333 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 338 insertions(+), 31 deletions(-)
---
base-commit: 0c6c3bf84f541fb4ec7097baf9eac10136f98c62
change-id: 20241217-ad4695-oversampling-2946fbe3aff3

Best regards,
-- 
Trevor Gamblin <tgamblin@baylibre.com>

[PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Trevor Gamblin]

Add support for the ad4695's oversampling feature when SPI offload is
available. This allows the ad4695 to set oversampling ratios on a
per-channel basis, raising the effective-number-of-bits from 16
(OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
full cycle through the auto-sequencer). The logic for reading and
writing sampling frequency for a given channel is also adjusted based on
the current oversampling ratio.

The non-offload case isn't supported as there isn't a good way to
trigger the CNV pin in this mode. Support could be added in the future
if a use-case arises.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 303 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index c8cd73d19e86..0caaeaa310ed 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -79,6 +79,7 @@
 #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
 #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
 #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
+#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
 #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
 #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
 #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
@@ -127,6 +128,7 @@ struct ad4695_channel_config {
 	bool bipolar;
 	enum ad4695_in_pair pin_pairing;
 	unsigned int common_mode_mv;
+	unsigned int oversampling_ratio;
 };
 
 struct ad4695_state {
@@ -306,6 +308,65 @@ static const struct regmap_bus ad4695_regmap_bus = {
 	.val_format_endian_default = REGMAP_ENDIAN_BIG,
 };
 
+enum {
+	AD4695_SCAN_TYPE_OSR_1,
+	AD4695_SCAN_TYPE_OSR_4,
+	AD4695_SCAN_TYPE_OSR_16,
+	AD4695_SCAN_TYPE_OSR_64,
+};
+
+static const struct iio_scan_type ad4695_scan_type_offload_u[] = {
+	[AD4695_SCAN_TYPE_OSR_1] = {
+		.sign = 'u',
+		.realbits = 16,
+		.shift = 3,
+		.storagebits = 32,
+	},
+	[AD4695_SCAN_TYPE_OSR_4] = {
+		.sign = 'u',
+		.realbits = 17,
+		.shift = 2,
+		.storagebits = 32,
+	},
+	[AD4695_SCAN_TYPE_OSR_16] = {
+		.sign = 'u',
+		.realbits = 18,
+		.shift = 1,
+		.storagebits = 32,
+	},
+	[AD4695_SCAN_TYPE_OSR_64] = {
+		.sign = 'u',
+		.realbits = 19,
+		.storagebits = 32,
+	},
+};
+
+static const struct iio_scan_type ad4695_scan_type_offload_s[] = {
+	[AD4695_SCAN_TYPE_OSR_1] = {
+		.sign = 's',
+		.realbits = 16,
+		.shift = 3,
+		.storagebits = 32,
+	},
+	[AD4695_SCAN_TYPE_OSR_4] = {
+		.sign = 's',
+		.realbits = 17,
+		.shift = 2,
+		.storagebits = 32,
+	},
+	[AD4695_SCAN_TYPE_OSR_16] = {
+		.sign = 's',
+		.realbits = 18,
+		.shift = 1,
+		.storagebits = 32,
+	},
+	[AD4695_SCAN_TYPE_OSR_64] = {
+		.sign = 's',
+		.realbits = 19,
+		.storagebits = 32,
+	},
+};
+
 static const struct iio_chan_spec ad4695_channel_template = {
 	.type = IIO_VOLTAGE,
 	.indexed = 1,
@@ -343,6 +404,10 @@ static const char * const ad4695_power_supplies[] = {
 	"avdd", "vio"
 };
 
+static const int ad4695_oversampling_ratios[] = {
+	1, 4, 16, 64,
+};
+
 static const struct ad4695_chip_info ad4695_chip_info = {
 	.name = "ad4695",
 	.max_sample_rate = 500 * KILO,
@@ -519,6 +584,29 @@ static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv)
 				  FIELD_PREP(AD4695_REG_REF_CTRL_VREF_SET, val));
 }
 
+/**
+ * ad4695_osr_to_regval - convert ratio to OSR register value
+ * @ratio: ratio to check
+ *
+ * Check if ratio is present in the list of available ratios and return
+ * the corresponding value that needs to be written to the register to
+ * select that ratio.
+ *
+ * Returns: register value (0 to 3) or -EINVAL if there is not an exact
+ * match
+ */
+static int ad4695_osr_to_regval(int ratio)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ad4695_oversampling_ratios); i++) {
+		if (ratio == ad4695_oversampling_ratios[i])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
 static int ad4695_write_chn_cfg(struct ad4695_state *st,
 				struct ad4695_channel_config *cfg)
 {
@@ -945,10 +1033,18 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct ad4695_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
 	struct ad4695_channel_config *cfg = &st->channels_cfg[chan->scan_index];
-	u8 realbits = chan->scan_type.realbits;
+	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
 	unsigned int reg_val;
 	int ret, tmp;
+	u8 realbits;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	realbits = scan_type->realbits;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -957,7 +1053,7 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 			if (ret)
 				return ret;
 
-			if (chan->scan_type.sign == 's')
+			if (scan_type->sign == 's')
 				*val = sign_extend32(st->raw_data, realbits - 1);
 			else
 				*val = st->raw_data;
@@ -969,7 +1065,7 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 		switch (chan->type) {
 		case IIO_VOLTAGE:
 			*val = st->vref_mv;
-			*val2 = chan->scan_type.realbits;
+			*val2 = realbits;
 			return IIO_VAL_FRACTIONAL_LOG2;
 		case IIO_TEMP:
 			/* T_scale (°C) = raw * V_REF (mV) / (-1.8 mV/°C * 2^16) */
@@ -1030,8 +1126,26 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 
 				tmp = sign_extend32(reg_val, 15);
 
-				*val = tmp / 4;
-				*val2 = abs(tmp) % 4 * MICRO / 4;
+				switch (cfg->oversampling_ratio) {
+				case 1:
+					*val = tmp / 4;
+					*val2 = abs(tmp) % 4 * MICRO / 4;
+					break;
+				case 4:
+					*val = tmp / 2;
+					*val2 = abs(tmp) % 2 * MICRO / 2;
+					break;
+				case 16:
+					*val = tmp;
+					*val2 = 0;
+					break;
+				case 64:
+					*val = tmp * 2;
+					*val2 = 0;
+					break;
+				default:
+					return -EINVAL;
+				}
 
 				if (tmp < 0 && *val2) {
 					*val *= -1;
@@ -1044,6 +1158,14 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = st->channels_cfg[chan->scan_index].oversampling_ratio;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_SAMP_FREQ: {
 		struct pwm_state state;
 
@@ -1051,7 +1173,11 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state.period);
+		/*
+		 * The effective sampling frequency for a channel is the input
+		 * frequency divided by the channel's OSR value.
+		 */
+		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state.period * osr);
 
 		return IIO_VAL_INT;
 	}
@@ -1072,12 +1198,69 @@ static int ad4695_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+static int ad4695_set_osr_val(struct ad4695_state *st,
+			      struct iio_chan_spec const *chan,
+			      int val)
+{
+	int osr = ad4695_osr_to_regval(val);
+
+	if (osr < 0)
+		return osr;
+
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		st->channels_cfg[chan->scan_index].oversampling_ratio = val;
+		return regmap_update_bits(st->regmap,
+				AD4695_REG_CONFIG_IN(chan->scan_index),
+				AD4695_REG_CONFIG_IN_OSR_SET,
+				FIELD_PREP(AD4695_REG_CONFIG_IN_OSR_SET, osr));
+	default:
+		return -EINVAL;
+	}
+}
+
+static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
+{
+	int val_calc, scale;
+
+	switch (osr) {
+	case 4:
+		scale = 4;
+		break;
+	case 16:
+		scale = 2;
+		break;
+	case 64:
+		scale = 1;
+		break;
+	default:
+		scale = 8;
+		break;
+	}
+
+	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
+
+	/* val2 range is (-MICRO, MICRO) if val == 0, otherwise [0, MICRO) */
+	if (val < 0)
+		val_calc = val * scale - val2 * scale / MICRO;
+	else if (val2 < 0)
+		/* if val2 < 0 then val == 0 */
+		val_calc = val2 * scale / (int)MICRO;
+	else
+		val_calc = val * scale + val2 * scale / MICRO;
+
+	val_calc /= 2;
+
+	return clamp_t(int, val_calc, S16_MIN, S16_MAX);
+}
+
 static int ad4695_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long mask)
 {
 	struct ad4695_state *st = iio_priv(indio_dev);
 	unsigned int reg_val;
+	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
 
 	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
 		switch (mask) {
@@ -1102,23 +1285,7 @@ static int ad4695_write_raw(struct iio_dev *indio_dev,
 		case IIO_CHAN_INFO_CALIBBIAS:
 			switch (chan->type) {
 			case IIO_VOLTAGE:
-				if (val2 >= 0 && val > S16_MAX / 4)
-					reg_val = S16_MAX;
-				else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
-					reg_val = S16_MIN;
-				else if (val2 < 0)
-					reg_val = clamp_t(int,
-						-(val * 4 + -val2 * 4 / MICRO),
-						S16_MIN, S16_MAX);
-				else if (val < 0)
-					reg_val = clamp_t(int,
-						val * 4 - val2 * 4 / MICRO,
-						S16_MIN, S16_MAX);
-				else
-					reg_val = clamp_t(int,
-						val * 4 + val2 * 4 / MICRO,
-						S16_MIN, S16_MAX);
-
+				reg_val = ad4695_get_calibbias(val, val2, osr);
 				return regmap_write(st->regmap16,
 					AD4695_REG_OFFSET_IN(chan->scan_index),
 					reg_val);
@@ -1127,15 +1294,27 @@ static int ad4695_write_raw(struct iio_dev *indio_dev,
 			}
 		case IIO_CHAN_INFO_SAMP_FREQ: {
 			struct pwm_state state;
-
-			if (val <= 0 || val > st->chip_info->max_sample_rate)
+			/*
+			 * Limit the maximum acceptable sample rate according to
+			 * the channel's oversampling ratio.
+			 */
+			u64 max_osr_rate = DIV_ROUND_UP_ULL(st->chip_info->max_sample_rate,
+							    osr);
+
+			if (val <= 0 || val > max_osr_rate)
 				return -EINVAL;
 
 			guard(mutex)(&st->cnv_pwm_lock);
 			pwm_get_state(st->cnv_pwm, &state);
-			state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val);
+			/*
+			 * The required sample frequency for a given OSR is the
+			 * input frequency multiplied by it.
+			 */
+			state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val * osr);
 			return pwm_apply_might_sleep(st->cnv_pwm, &state);
 		}
+		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+			return ad4695_set_osr_val(st, chan, val);
 		default:
 			return -EINVAL;
 		}
@@ -1148,18 +1327,40 @@ static int ad4695_read_avail(struct iio_dev *indio_dev,
 			     const int **vals, int *type, int *length,
 			     long mask)
 {
+	int ret;
 	static const int ad4695_calibscale_available[6] = {
 		/* Range of 0 (inclusive) to 2 (exclusive) */
 		0, 15, 1, 15, U16_MAX, 15
 	};
-	static const int ad4695_calibbias_available[6] = {
+	static const int ad4695_calibbias_available[4][6] = {
 		/*
 		 * Datasheet says FSR/8 which translates to signed/4. The step
-		 * depends on oversampling ratio which is always 1 for now.
+		 * depends on oversampling ratio, so we need four different
+		 * ranges to select from.
 		 */
-		S16_MIN / 4, 0, 0, MICRO / 4, S16_MAX / 4, S16_MAX % 4 * MICRO / 4
+		{
+			S16_MIN / 4, 0,
+			0, MICRO / 4,
+			S16_MAX / 4, S16_MAX % 4 * MICRO / 4
+		},
+		{
+			S16_MIN / 2, 0,
+			0, MICRO / 2,
+			S16_MAX / 2, S16_MAX % 2 * MICRO / 2,
+		},
+		{
+			S16_MIN, 0,
+			1, 0,
+			S16_MAX, 0,
+		},
+		{
+			S16_MIN * 2, 0,
+			2, 0,
+			S16_MAX * 2, 0,
+		},
 	};
 	struct ad4695_state *st = iio_priv(indio_dev);
+	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBSCALE:
@@ -1174,16 +1375,36 @@ static int ad4695_read_avail(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_CALIBBIAS:
 		switch (chan->type) {
 		case IIO_VOLTAGE:
-			*vals = ad4695_calibbias_available;
+			ret = ad4695_osr_to_regval(osr);
+			if (ret < 0)
+				return ret;
+			/*
+			 * Select the appropriate calibbias array based on the
+			 * OSR value in the register.
+			 */
+			*vals = ad4695_calibbias_available[ret];
 			*type = IIO_VAL_INT_PLUS_MICRO;
 			return IIO_AVAIL_RANGE;
 		default:
 			return -EINVAL;
 		}
 	case IIO_CHAN_INFO_SAMP_FREQ:
+		/* Max sample rate for the channel depends on OSR */
+		st->sample_freq_range[2] =
+			DIV_ROUND_UP_ULL(st->chip_info->max_sample_rate, osr);
 		*vals = st->sample_freq_range;
 		*type = IIO_VAL_INT;
 		return IIO_AVAIL_RANGE;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*vals = ad4695_oversampling_ratios;
+			*length = ARRAY_SIZE(ad4695_oversampling_ratios);
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -1217,6 +1438,26 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ad4695_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4695_state *st = iio_priv(indio_dev);
+	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
+
+	switch (osr) {
+	case 1:
+		return AD4695_SCAN_TYPE_OSR_1;
+	case 4:
+		return AD4695_SCAN_TYPE_OSR_4;
+	case 16:
+		return AD4695_SCAN_TYPE_OSR_16;
+	case 64:
+		return AD4695_SCAN_TYPE_OSR_64;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_info ad4695_info = {
 	.read_raw = &ad4695_read_raw,
 	.write_raw_get_fmt = &ad4695_write_raw_get_fmt,
@@ -1225,6 +1466,15 @@ static const struct iio_info ad4695_info = {
 	.debugfs_reg_access = &ad4695_debugfs_reg_access,
 };
 
+static const struct iio_info ad4695_offload_info = {
+	.read_raw = &ad4695_read_raw,
+	.write_raw_get_fmt = &ad4695_write_raw_get_fmt,
+	.write_raw = &ad4695_write_raw,
+	.get_current_scan_type = &ad4695_get_current_scan_type,
+	.read_avail = &ad4695_read_avail,
+	.debugfs_reg_access = &ad4695_debugfs_reg_access,
+};
+
 static int ad4695_parse_channel_cfg(struct ad4695_state *st)
 {
 	struct device *dev = &st->spi->dev;
@@ -1240,6 +1490,9 @@ static int ad4695_parse_channel_cfg(struct ad4695_state *st)
 		chan_cfg->highz_en = true;
 		chan_cfg->channel = i;
 
+		/* This is the default OSR after reset */
+		chan_cfg->oversampling_ratio = 1;
+
 		*iio_chan = ad4695_channel_template;
 		iio_chan->channel = i;
 		iio_chan->scan_index = i;
@@ -1408,6 +1661,7 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
 	struct dma_chan *rx_dma;
 	int ret, i;
 
+	indio_dev->info = &ad4695_offload_info;
 	indio_dev->num_channels = st->chip_info->num_voltage_inputs + 1;
 	indio_dev->setup_ops = &ad4695_offload_buffer_setup_ops;
 
@@ -1458,6 +1712,7 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
 
 	for (i = 0; i < indio_dev->num_channels; i++) {
 		struct iio_chan_spec *chan = &st->iio_chan[i];
+		struct ad4695_channel_config *cfg = &st->channels_cfg[i];
 
 		/*
 		 * NB: When using offload support, all channels need to have the
@@ -1473,6 +1728,24 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
 		/* add sample frequency for PWM CNV trigger */
 		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SAMP_FREQ);
 		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SAMP_FREQ);
+
+		/* Add the oversampling properties only for voltage channels */
+		if (chan->type != IIO_VOLTAGE)
+			continue;
+
+		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
+		chan->info_mask_separate_available |=
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
+		chan->has_ext_scan_type = 1;
+		if (cfg->bipolar) {
+			chan->ext_scan_type = ad4695_scan_type_offload_s;
+			chan->num_ext_scan_type =
+				ARRAY_SIZE(ad4695_scan_type_offload_s);
+		} else {
+			chan->ext_scan_type = ad4695_scan_type_offload_u;
+			chan->num_ext_scan_type =
+				ARRAY_SIZE(ad4695_scan_type_offload_u);
+		}
 	}
 
 	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev,

-- 
2.39.5

Re: [PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Jonathan Cameron]

On Thu, 09 Jan 2025 13:47:23 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> Add support for the ad4695's oversampling feature when SPI offload is
> available. This allows the ad4695 to set oversampling ratios on a
> per-channel basis, raising the effective-number-of-bits from 16
> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
> full cycle through the auto-sequencer). The logic for reading and
> writing sampling frequency for a given channel is also adjusted based on
> the current oversampling ratio.
> 
> The non-offload case isn't supported as there isn't a good way to
> trigger the CNV pin in this mode. Support could be added in the future
> if a use-case arises.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
LGTM
Assuming no requests for changes otherwise feel free to poke me when
the dependency is available upstream. I'll keep it live in patchwork
but I tend only to loop back over outstanding stuff once or twice a cycle
and maybe that will be long after needed for this one!

Jonathan

Re: [PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Nuno Sá]

On Thu, 2025-01-09 at 13:47 -0500, Trevor Gamblin wrote:
> Add support for the ad4695's oversampling feature when SPI offload is
> available. This allows the ad4695 to set oversampling ratios on a
> per-channel basis, raising the effective-number-of-bits from 16
> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
> full cycle through the auto-sequencer). The logic for reading and
> writing sampling frequency for a given channel is also adjusted based on
> the current oversampling ratio.
> 
> The non-offload case isn't supported as there isn't a good way to
> trigger the CNV pin in this mode. Support could be added in the future
> if a use-case arises.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---

LGTM, just one small thing inline... Either way:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++----
> -
>  1 file changed, 303 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index c8cd73d19e86..0caaeaa310ed 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -79,6 +79,7 @@
>  #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
>  #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
>  #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
> +#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
>  #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
>  #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
>  #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
> @@ -127,6 +128,7 @@ struct ad4695_channel_config {
>  	bool bipolar;
>  	enum ad4695_in_pair pin_pairing;
>  	unsigned int common_mode_mv;
> +	unsigned int oversampling_ratio;
>  };
>  

...

> +
> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
> +{
> +	int val_calc, scale;
> +
> +	switch (osr) {
> +	case 4:
> +		scale = 4;
> +		break;
> +	case 16:
> +		scale = 2;
> +		break;
> +	case 64:
> +		scale = 1;
> +		break;
> +	default:
> +		scale = 8;
> +		break;
> +	}
> +
> +	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
> +

Why not clamp()? AFAICS, we have the same type on all the arguments. I also
think clamp*() macros got the same improvements as min/max() ones which means
that using the ones with explicit casts are not so often needed anymore. My
understanding is also that those macros are not that encouraged as it's easy to
go wrong with the casts.

- Nuno Sá
> 

Re: [PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Trevor Gamblin]

On 2025-01-13 09:35, Nuno Sá wrote:
> On Thu, 2025-01-09 at 13:47 -0500, Trevor Gamblin wrote:
>> Add support for the ad4695's oversampling feature when SPI offload is
>> available. This allows the ad4695 to set oversampling ratios on a
>> per-channel basis, raising the effective-number-of-bits from 16
>> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
>> full cycle through the auto-sequencer). The logic for reading and
>> writing sampling frequency for a given channel is also adjusted based on
>> the current oversampling ratio.
>>
>> The non-offload case isn't supported as there isn't a good way to
>> trigger the CNV pin in this mode. Support could be added in the future
>> if a use-case arises.
>>
>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
>> ---
> LGTM, just one small thing inline... Either way:
>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>
>>   drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++----
>> -
>>   1 file changed, 303 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
>> index c8cd73d19e86..0caaeaa310ed 100644
>> --- a/drivers/iio/adc/ad4695.c
>> +++ b/drivers/iio/adc/ad4695.c
>> @@ -79,6 +79,7 @@
>>   #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
>>   #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
>>   #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
>> +#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
>>   #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
>>   #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
>>   #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
>> @@ -127,6 +128,7 @@ struct ad4695_channel_config {
>>   	bool bipolar;
>>   	enum ad4695_in_pair pin_pairing;
>>   	unsigned int common_mode_mv;
>> +	unsigned int oversampling_ratio;
>>   };
>>   
> ...
>
>> +
>> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
>> +{
>> +	int val_calc, scale;
>> +
>> +	switch (osr) {
>> +	case 4:
>> +		scale = 4;
>> +		break;
>> +	case 16:
>> +		scale = 2;
>> +		break;
>> +	case 64:
>> +		scale = 1;
>> +		break;
>> +	default:
>> +		scale = 8;
>> +		break;
>> +	}
>> +
>> +	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
>> +
> Why not clamp()? AFAICS, we have the same type on all the arguments. I also
> think clamp*() macros got the same improvements as min/max() ones which means
> that using the ones with explicit casts are not so often needed anymore. My
> understanding is also that those macros are not that encouraged as it's easy to
> go wrong with the casts.
I have no preference, this is just a recent habitual use of clamp_t(). If
clamp() is preferred I can send a v3. Or maybe Jonathan can tweak it 
when it is

eventually applied?

- Trevor

>
> - Nuno Sá

Re: [PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Jonathan Cameron]

On Mon, 13 Jan 2025 11:49:49 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> On 2025-01-13 09:35, Nuno Sá wrote:
> > On Thu, 2025-01-09 at 13:47 -0500, Trevor Gamblin wrote:  
> >> Add support for the ad4695's oversampling feature when SPI offload is
> >> available. This allows the ad4695 to set oversampling ratios on a
> >> per-channel basis, raising the effective-number-of-bits from 16
> >> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
> >> full cycle through the auto-sequencer). The logic for reading and
> >> writing sampling frequency for a given channel is also adjusted based on
> >> the current oversampling ratio.
> >>
> >> The non-offload case isn't supported as there isn't a good way to
> >> trigger the CNV pin in this mode. Support could be added in the future
> >> if a use-case arises.
> >>
> >> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> >> ---  
> > LGTM, just one small thing inline... Either way:
> >
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> >  
> >>   drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++----
> >> -
> >>   1 file changed, 303 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> >> index c8cd73d19e86..0caaeaa310ed 100644
> >> --- a/drivers/iio/adc/ad4695.c
> >> +++ b/drivers/iio/adc/ad4695.c
> >> @@ -79,6 +79,7 @@
> >>   #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
> >>   #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
> >>   #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
> >> +#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
> >>   #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
> >>   #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
> >>   #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
> >> @@ -127,6 +128,7 @@ struct ad4695_channel_config {
> >>   	bool bipolar;
> >>   	enum ad4695_in_pair pin_pairing;
> >>   	unsigned int common_mode_mv;
> >> +	unsigned int oversampling_ratio;
> >>   };
> >>     
> > ...
> >  
> >> +
> >> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
> >> +{
> >> +	int val_calc, scale;
> >> +
> >> +	switch (osr) {
> >> +	case 4:
> >> +		scale = 4;
> >> +		break;
> >> +	case 16:
> >> +		scale = 2;
> >> +		break;
> >> +	case 64:
> >> +		scale = 1;
> >> +		break;
> >> +	default:
> >> +		scale = 8;
> >> +		break;
> >> +	}
> >> +
> >> +	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
> >> +  
> > Why not clamp()? AFAICS, we have the same type on all the arguments. I also
> > think clamp*() macros got the same improvements as min/max() ones which means
> > that using the ones with explicit casts are not so often needed anymore. My
> > understanding is also that those macros are not that encouraged as it's easy to
> > go wrong with the casts.  
> I have no preference, this is just a recent habitual use of clamp_t(). If
> clamp() is preferred I can send a v3. Or maybe Jonathan can tweak it 
> when it is

I've left it as clamp_T for now.  We can always follow up with a series
to tidy these up general.

Series applied though a bit provisionally as the SPI offload set needed
a few tweaks that might get changed.

Pushed out as testing for now.

Thanks,

Jonathan

> 
> eventually applied?
> 
> - Trevor
> 
> >
> > - Nuno Sá  

Re: [PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Trevor Gamblin]

On 2025-02-10 14:03, Jonathan Cameron wrote:
> On Mon, 13 Jan 2025 11:49:49 -0500
> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>
>> On 2025-01-13 09:35, Nuno Sá wrote:
>>> On Thu, 2025-01-09 at 13:47 -0500, Trevor Gamblin wrote:
>>>> Add support for the ad4695's oversampling feature when SPI offload is
>>>> available. This allows the ad4695 to set oversampling ratios on a
>>>> per-channel basis, raising the effective-number-of-bits from 16
>>>> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
>>>> full cycle through the auto-sequencer). The logic for reading and
>>>> writing sampling frequency for a given channel is also adjusted based on
>>>> the current oversampling ratio.
>>>>
>>>> The non-offload case isn't supported as there isn't a good way to
>>>> trigger the CNV pin in this mode. Support could be added in the future
>>>> if a use-case arises.
>>>>
>>>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
>>>> ---
>>> LGTM, just one small thing inline... Either way:
>>>
>>> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>>>   
>>>>    drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++----
>>>> -
>>>>    1 file changed, 303 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
>>>> index c8cd73d19e86..0caaeaa310ed 100644
>>>> --- a/drivers/iio/adc/ad4695.c
>>>> +++ b/drivers/iio/adc/ad4695.c
>>>> @@ -79,6 +79,7 @@
>>>>    #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
>>>>    #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
>>>>    #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
>>>> +#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
>>>>    #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
>>>>    #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
>>>>    #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
>>>> @@ -127,6 +128,7 @@ struct ad4695_channel_config {
>>>>    	bool bipolar;
>>>>    	enum ad4695_in_pair pin_pairing;
>>>>    	unsigned int common_mode_mv;
>>>> +	unsigned int oversampling_ratio;
>>>>    };
>>>>      
>>> ...
>>>   
>>>> +
>>>> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
>>>> +{
>>>> +	int val_calc, scale;
>>>> +
>>>> +	switch (osr) {
>>>> +	case 4:
>>>> +		scale = 4;
>>>> +		break;
>>>> +	case 16:
>>>> +		scale = 2;
>>>> +		break;
>>>> +	case 64:
>>>> +		scale = 1;
>>>> +		break;
>>>> +	default:
>>>> +		scale = 8;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
>>>> +
>>> Why not clamp()? AFAICS, we have the same type on all the arguments. I also
>>> think clamp*() macros got the same improvements as min/max() ones which means
>>> that using the ones with explicit casts are not so often needed anymore. My
>>> understanding is also that those macros are not that encouraged as it's easy to
>>> go wrong with the casts.
>> I have no preference, this is just a recent habitual use of clamp_t(). If
>> clamp() is preferred I can send a v3. Or maybe Jonathan can tweak it
>> when it is
> I've left it as clamp_T for now.  We can always follow up with a series
> to tidy these up general.
>
> Series applied though a bit provisionally as the SPI offload set needed
> a few tweaks that might get changed.
>
> Pushed out as testing for now.
>
> Thanks,
>
> Jonathan
Thank you!

[PATCH v2 2/2] doc: iio: ad4695: describe oversampling support

[Trevor Gamblin]

Add a section to the ad4695 documentation describing how to use the
oversampling feature. Also add some clarification on how the
oversampling ratio influences effective sample rate in the offload
section.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 Documentation/iio/ad4695.rst | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/iio/ad4695.rst b/Documentation/iio/ad4695.rst
index ead0faadff4b..f40593bcc37d 100644
--- a/Documentation/iio/ad4695.rst
+++ b/Documentation/iio/ad4695.rst
@@ -179,12 +179,38 @@ Gain/offset calibration
 System calibration is supported using the channel gain and offset registers via
 the ``calibscale`` and ``calibbias`` attributes respectively.
 
+Oversampling
+------------
+
+The chip supports per-channel oversampling when SPI offload is being used, with
+available oversampling ratios (OSR) of 1 (default), 4, 16, and 64.  Enabling
+oversampling on a channel raises the effective number of bits of sampled data to
+17 (OSR == 4), 18 (16), or 19 (64), respectively. This can be set via the
+``oversampling_ratio`` attribute.
+
+Setting the oversampling ratio for a channel also changes the sample rate for
+that channel, since it requires multiple conversions per 1 sample. Specifically,
+the new sampling frequency is the PWM sampling frequency divided by the
+particular OSR. This is set automatically by the driver when setting the
+``oversampling_ratio`` attribute. For example, if the device's current
+``sampling_frequency`` is 10000 and an OSR of 4 is set on channel ``voltage0``,
+the new reported sampling rate for that channel will be 2500 (ignoring PWM API
+rounding), while all others will remain at 10000.  Subsequently setting the
+sampling frequency to a higher value on that channel will adjust the CNV trigger
+period for all channels, e.g. if ``voltage0``'s sampling frequency is adjusted
+from 2500 (with an OSR of 4) to 10000, the value reported by
+``in_voltage0_sampling_frequency`` will be 10000, but all other channels will
+now report 40000.
+
+For simplicity, the sampling frequency of the device should be set (considering
+the highest desired OSR value to be used) first, before configuring oversampling
+for specific channels.
+
 Unimplemented features
 ----------------------
 
 - Additional wiring modes
 - Threshold events
-- Oversampling
 - GPIO support
 - CRC support
 
@@ -233,3 +259,11 @@ words, it is the value of the ``in_voltageY_sampling_frequency`` attribute
 divided by the number of enabled channels. So if 4 channels are enabled, with
 the ``in_voltageY_sampling_frequency`` attributes set to 1 MHz, the effective
 sample rate is 250 kHz.
+
+With oversampling enabled, the effective sample rate also depends on the OSR
+assigned to each channel. For example, if one of the 4 channels mentioned in the
+previous case is configured with an OSR of 4, the effective sample rate for that
+channel becomes (1 MHz / 4 ) = 250 kHz. The effective sample rate for all
+four channels is then 1 / ( (3 / 1 MHz) + ( 1 / 250 kHz) ) ~= 142.9 kHz. Note
+that in this case "sample" refers to one read of all enabled channels (i.e. one
+full cycle through the auto-sequencer).

-- 
2.39.5

Re: [PATCH v2 0/2] iio: adc: ad4695: add oversampling support

[David Lechner]

On 1/9/25 12:47 PM, Trevor Gamblin wrote:
> Add driver logic and documentation for the oversampling feature of the
> AD469x parts from Analog Devices. For now, this only works with offload
> support, and takes advantage of that mode's higher performance to make
> oversampling possible on multiple channels with varying sampling
> frequencies. Some significant rework of the driver had to be done in
> order to conditionally support this feature, including use of
> iio_scan_types to help determine the appropriate spi message
> configurations depending on oversampling ratio.
> 
> This series depends on David's recent SPI engine changes for adding
> offload support:
> 
> https://lore.kernel.org/all/20241211-dlech-mainline-spi-engine-offload-2-v6-0-88ee574d5d03@baylibre.com/
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---

Tested-by: David Lechner <dlechner@baylibre.com>