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

[PATCH 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.

A dilemma that came up during development of this feature was whether or
not the implementation of oversampling ratios against sampling frequency
was actually correct. More specifically, it's unclear if the sampling
frequency attribute is supposed to be the conversion rate or the data
read rate (according to the IIO subsystem). If it's the former, then
this implementation is probably incorrect. David Lechner pointed out
during review that it would be easier if it were defined as the
conversion rate and that it was userspace's responsibility to handle
oversampling ratio, but that might also require more work in the IIO
subsystem. Two other ADC drivers that were referenced for inspiration
when working through this were the ad7380 and the rtq6056. The ad7380
has a global oversampling setting rather than per-channel, and the
rtq6056 seems at least partially broken because it only takes
oversampling ratio into account when getting the sampling frequency (but
not when setting it). Instead of per-driver implementation, these three
drivers might serve as inspiration for changes to how oversampling is
handled in IIO?

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>
---
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     | 378 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 383 insertions(+), 31 deletions(-)
---
base-commit: 0c6c3bf84f541fb4ec7097baf9eac10136f98c62
change-id: 20241217-ad4695-oversampling-2946fbe3aff3

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

[PATCH 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 | 378 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 348 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index c8cd73d19e86..320f2c1a0877 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,114 @@ 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)
+{
+	unsigned int reg_val;
+
+	switch (osr) {
+	case 4:
+		if (val2 >= 0 && val > S16_MAX / 2)
+			reg_val = S16_MAX;
+		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)
+			reg_val = S16_MIN;
+		else if (val2 < 0)
+			reg_val = clamp_t(int,
+				-(val * 2 + -val2 * 2 / MICRO),
+				S16_MIN, S16_MAX);
+		else if (val < 0)
+			reg_val = clamp_t(int,
+				val * 2 - val2 * 2 / MICRO,
+				S16_MIN, S16_MAX);
+		else
+			reg_val = clamp_t(int,
+				val * 2 + val2 * 2 / MICRO,
+				S16_MIN, S16_MAX);
+		return reg_val;
+	case 16:
+		if (val2 >= 0 && val > S16_MAX)
+			reg_val = S16_MAX;
+		else if ((val2 < 0 ? -val : val) < S16_MIN)
+			reg_val = S16_MIN;
+		else if (val2 < 0)
+			reg_val = clamp_t(int,
+				-(val + -val2 / MICRO),
+				S16_MIN, S16_MAX);
+		else if (val < 0)
+			reg_val = clamp_t(int,
+				val - val2 / MICRO,
+				S16_MIN, S16_MAX);
+		else
+			reg_val = clamp_t(int,
+				val + val2 / MICRO,
+				S16_MIN, S16_MAX);
+		return reg_val;
+	case 64:
+		if (val2 >= 0 && val > S16_MAX * 2)
+			reg_val = S16_MAX;
+		else if ((val2 < 0 ? -val : val) < S16_MIN * 2)
+			reg_val = S16_MIN;
+		else if (val2 < 0)
+			reg_val = clamp_t(int,
+				-(val / 2 + -val2 / 2 / MICRO),
+				S16_MIN, S16_MAX);
+		else if (val < 0)
+			reg_val = clamp_t(int,
+				val / 2 - val2 / 2 / MICRO,
+				S16_MIN, S16_MAX);
+		else
+			reg_val = clamp_t(int,
+				val / 2 + val2 / 2 / MICRO,
+				S16_MIN, S16_MAX);
+		return reg_val;
+	default:
+		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);
+		return reg_val;
+	}
+}
+
 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 +1330,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 +1339,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 +1372,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 +1420,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 +1483,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 +1511,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 +1535,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 +1706,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 +1757,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 +1773,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

[PATCH 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 0/2] iio: adc: ad4695: add oversampling support

[Jonathan Cameron]

On Tue, 17 Dec 2024 16:47:27 -0500
Trevor Gamblin <tgamblin@baylibre.com> 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.
> 
> A dilemma that came up during development of this feature was whether or
> not the implementation of oversampling ratios against sampling frequency
> was actually correct. More specifically, it's unclear if the sampling
> frequency attribute is supposed to be the conversion rate or the data
> read rate (according to the IIO subsystem). 

Intent was it's the rate at which the reading becomes available to the
software (which I think you are calling data read rate).

In global schemes, it's all of the scan (normally they are
self clocked with delays between scans) in per channel schemes intent
was to provide the sampling rate at which that channel was sampled
if no others were being read (assuming the times sum up in a linear
way etc).

> If it's the former, then
> this implementation is probably incorrect. David Lechner pointed out
> during review that it would be easier if it were defined as the
> conversion rate and that it was userspace's responsibility to handle
> oversampling ratio, but that might also require more work in the IIO
> subsystem.

IIRC there are devices out there were oversampling rate is on top of
their controlled read out rate (which is more about delays between
starting sampling than about how long it takes).  I think one of those
drove the implementation of oversampling in the first place.
It's also not the case that an oversampling rate of x4 always means
that the time to data availability is necessarily 1/4 (can pipeline
some stages depending on the sensor design).

My thinking (it was a long time ago) was that userspace wouldn't want
to deal with the scaling.  We only fairly recently defined the
clear concept of per channel sampling frequencies. Before that they
were (almost?) all global.

Another aspect is that when we originally added this, it was new to userspace
so having defined sampling frequency, we couldn't have it's meaning
changing because oversampling was say a minimum of 2 on a device.

> Two other ADC drivers that were referenced for inspiration
> when working through this were the ad7380 and the rtq6056. The ad7380
> has a global oversampling setting rather than per-channel, and the

I'm not sure it being global matters a lot for question of whether
it takes into account oversampling or not.

> rtq6056 seems at least partially broken because it only takes
> oversampling ratio into account when getting the sampling frequency (but
> not when setting it). 

That is definitely broken and could do with a fix.  We should always
read back more or less the same value written (subject to rounding
etc in some cases).


> Instead of per-driver implementation, these three
> drivers might serve as inspiration for changes to how oversampling is
> handled in IIO?

It is very tricky to make any modifications (other than the fix above)
without making a mess of the ABI.

Or are you suggesting we could so something in the IIO core?  Maybe
some helper functions are appropriate (similar to Matti's GTS stuff
for gains where they are a mixture of various factors on light sensors
and similar.).

Jonathan


> 
> 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>
> ---
> 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     | 378 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 383 insertions(+), 31 deletions(-)
> ---
> base-commit: 0c6c3bf84f541fb4ec7097baf9eac10136f98c62
> change-id: 20241217-ad4695-oversampling-2946fbe3aff3
> 
> Best regards,

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

[Jonathan Cameron]

On Tue, 17 Dec 2024 16:47:29 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> 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>
This describes what I was expecting so all looks good to me.

Obviously need to wait for the spi offload series anyway
so plenty of time for others to take a look.

Thanks

Jonathan

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

Ah. I forgot there is another series in flight for this and was going to say
that we needed a statement on the frequencies being a common control.
That is there in the spi offload series so all good!

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

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

[Jonathan Cameron]

On Tue, 17 Dec 2024 16:47:28 -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>
Hi Trevor,

The clamping fun of get_calibbias seems overkill. If this isn't going to ever
overflow an s64 maybe just use the high precision to do it the easy way.
I'm not sure you can't just fit it in an s32 for that matter. I've just
not done the maths to check.

Jonathan


> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
> +{
> +	unsigned int reg_val;
> +
> +	switch (osr) {
> +	case 4:
> +		if (val2 >= 0 && val > S16_MAX / 2)
> +			reg_val = S16_MAX;
> +		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)

It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise
we carry the sign in the val part.  Sometimes we generalize that to
make life easier for driver writers but I think you can use that here
to simplify things.

(for background look at __iio_str_to_fixpoint() - it's a bit of a hack
to deal with integers have no negative 0)

		if (val > S16_MAX / 2)
			...
		else if (val < S16_MIN / 2)
			...	
		else if (val2 < 0) etc

You may feel it is better to keep the code considering the val2 < 0 when
val != 0 case and I don't mind that as it's not wrong, just overly complex!

If you can easily clamp the overall range you can just do some maths
with enough precision to get one number (probably a s64) and clamp that.
Easy to sanity check for overflow based on val to ensure no overflows.

		


> +			reg_val = S16_MIN;
> +		else if (val2 < 0)
> +			reg_val = clamp_t(int,
> +				-(val * 2 + -val2 * 2 / MICRO),
> +				S16_MIN, S16_MAX);
> +		else if (val < 0)
> +			reg_val = clamp_t(int,
> +				val * 2 - val2 * 2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		else
> +			reg_val = clamp_t(int,
> +				val * 2 + val2 * 2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		return reg_val;
> +	case 16:
> +		if (val2 >= 0 && val > S16_MAX)
> +			reg_val = S16_MAX;
> +		else if ((val2 < 0 ? -val : val) < S16_MIN)
> +			reg_val = S16_MIN;
> +		else if (val2 < 0)
> +			reg_val = clamp_t(int,
> +				-(val + -val2 / MICRO),
> +				S16_MIN, S16_MAX);
> +		else if (val < 0)
> +			reg_val = clamp_t(int,
> +				val - val2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		else
> +			reg_val = clamp_t(int,
> +				val + val2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		return reg_val;
> +	case 64:
> +		if (val2 >= 0 && val > S16_MAX * 2)
> +			reg_val = S16_MAX;
> +		else if ((val2 < 0 ? -val : val) < S16_MIN * 2)
> +			reg_val = S16_MIN;
> +		else if (val2 < 0)
> +			reg_val = clamp_t(int,
> +				-(val / 2 + -val2 / 2 / MICRO),
> +				S16_MIN, S16_MAX);
> +		else if (val < 0)
> +			reg_val = clamp_t(int,
> +				val / 2 - val2 / 2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		else
> +			reg_val = clamp_t(int,
> +				val / 2 + val2 / 2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		return reg_val;
> +	default:
> +		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);
> +		return reg_val;
> +	}
> +}
> +

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

[Trevor Gamblin]

On 2024-12-19 11:13, Jonathan Cameron wrote:
> On Tue, 17 Dec 2024 16:47:28 -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>
> Hi Trevor,
>
> The clamping fun of get_calibbias seems overkill. If this isn't going to ever
> overflow an s64 maybe just use the high precision to do it the easy way.
> I'm not sure you can't just fit it in an s32 for that matter. I've just
> not done the maths to check.

Hi Jonathan,

Thanks for your feedback. I'll look it over again, do some testing, and 
see if it can be simplified.

Trevor

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

[Trevor Gamblin]

On 2024-12-19 11:13, Jonathan Cameron wrote:
> On Tue, 17 Dec 2024 16:47:28 -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>
> Hi Trevor,
>
> The clamping fun of get_calibbias seems overkill. If this isn't going to ever
> overflow an s64 maybe just use the high precision to do it the easy way.
> I'm not sure you can't just fit it in an s32 for that matter. I've just
> not done the maths to check.
>
> Jonathan
>
>
>> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
>> +{
>> +	unsigned int reg_val;
>> +
>> +	switch (osr) {
>> +	case 4:
>> +		if (val2 >= 0 && val > S16_MAX / 2)
>> +			reg_val = S16_MAX;
>> +		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)
> It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise
> we carry the sign in the val part.  Sometimes we generalize that to
> make life easier for driver writers but I think you can use that here
> to simplify things.
>
> (for background look at __iio_str_to_fixpoint() - it's a bit of a hack
> to deal with integers have no negative 0)
>
> 		if (val > S16_MAX / 2)
> 			...
> 		else if (val < S16_MIN / 2)
> 			...	
> 		else if (val2 < 0) etc
>
> You may feel it is better to keep the code considering the val2 < 0 when
> val != 0 case and I don't mind that as it's not wrong, just overly complex!
>
> If you can easily clamp the overall range you can just do some maths
> with enough precision to get one number (probably a s64) and clamp that.
> Easy to sanity check for overflow based on val to ensure no overflows.

Hi Jonathan,

I'm reviewing this again but I'm not entirely clear what you mean.

Are you suggesting that the entire switch block could be simplified 
(i.e. eliminating the previous simplification for the val2 < 0 case in 
the process), or that the calls to clamp_t can be combined?

I've tested out simplifying the val2 < 0 case locally and driver 
functionality still seems OK. Maybe I'm missing a third option.

- Trevor

>
> 		
>
>
>> +			reg_val = S16_MIN;
>> +		else if (val2 < 0)
>> +			reg_val = clamp_t(int,
>> +				-(val * 2 + -val2 * 2 / MICRO),
>> +				S16_MIN, S16_MAX);
>> +		else if (val < 0)
>> +			reg_val = clamp_t(int,
>> +				val * 2 - val2 * 2 / MICRO,
>> +				S16_MIN, S16_MAX);
>> +		else
>> +			reg_val = clamp_t(int,
>> +				val * 2 + val2 * 2 / MICRO,
>> +				S16_MIN, S16_MAX);
>> +		return reg_val;
>> +	case 16:
>> +		if (val2 >= 0 && val > S16_MAX)
>> +			reg_val = S16_MAX;
>> +		else if ((val2 < 0 ? -val : val) < S16_MIN)
>> +			reg_val = S16_MIN;
>> +		else if (val2 < 0)
>> +			reg_val = clamp_t(int,
>> +				-(val + -val2 / MICRO),
>> +				S16_MIN, S16_MAX);
>> +		else if (val < 0)
>> +			reg_val = clamp_t(int,
>> +				val - val2 / MICRO,
>> +				S16_MIN, S16_MAX);
>> +		else
>> +			reg_val = clamp_t(int,
>> +				val + val2 / MICRO,
>> +				S16_MIN, S16_MAX);
>> +		return reg_val;
>> +	case 64:
>> +		if (val2 >= 0 && val > S16_MAX * 2)
>> +			reg_val = S16_MAX;
>> +		else if ((val2 < 0 ? -val : val) < S16_MIN * 2)
>> +			reg_val = S16_MIN;
>> +		else if (val2 < 0)
>> +			reg_val = clamp_t(int,
>> +				-(val / 2 + -val2 / 2 / MICRO),
>> +				S16_MIN, S16_MAX);
>> +		else if (val < 0)
>> +			reg_val = clamp_t(int,
>> +				val / 2 - val2 / 2 / MICRO,
>> +				S16_MIN, S16_MAX);
>> +		else
>> +			reg_val = clamp_t(int,
>> +				val / 2 + val2 / 2 / MICRO,
>> +				S16_MIN, S16_MAX);
>> +		return reg_val;
>> +	default:
>> +		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);
>> +		return reg_val;
>> +	}
>> +}
>> +

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

[Jonathan Cameron]

On Thu, 2 Jan 2025 13:19:19 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> On 2024-12-19 11:13, Jonathan Cameron wrote:
> > On Tue, 17 Dec 2024 16:47:28 -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>  
> > Hi Trevor,
> >
> > The clamping fun of get_calibbias seems overkill. If this isn't going to ever
> > overflow an s64 maybe just use the high precision to do it the easy way.
> > I'm not sure you can't just fit it in an s32 for that matter. I've just
> > not done the maths to check.
> >
> > Jonathan
> >
> >  
> >> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
> >> +{
> >> +	unsigned int reg_val;
> >> +
> >> +	switch (osr) {
> >> +	case 4:
> >> +		if (val2 >= 0 && val > S16_MAX / 2)
> >> +			reg_val = S16_MAX;
> >> +		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)  
> > It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise
> > we carry the sign in the val part.  Sometimes we generalize that to
> > make life easier for driver writers but I think you can use that here
> > to simplify things.
> >
> > (for background look at __iio_str_to_fixpoint() - it's a bit of a hack
> > to deal with integers have no negative 0)
> >
> > 		if (val > S16_MAX / 2)
> > 			...
> > 		else if (val < S16_MIN / 2)
> > 			...	
> > 		else if (val2 < 0) etc
> >
> > You may feel it is better to keep the code considering the val2 < 0 when
> > val != 0 case and I don't mind that as it's not wrong, just overly complex!
> >
> > If you can easily clamp the overall range you can just do some maths
> > with enough precision to get one number (probably a s64) and clamp that.
> > Easy to sanity check for overflow based on val to ensure no overflows.  
> 
> Hi Jonathan,
> 
> I'm reviewing this again but I'm not entirely clear what you mean.
> 
> Are you suggesting that the entire switch block could be simplified 
> (i.e. eliminating the previous simplification for the val2 < 0 case in 
> the process), or that the calls to clamp_t can be combined?
> 
> I've tested out simplifying the val2 < 0 case locally and driver 
> functionality still seems OK. Maybe I'm missing a third option.
The extra info we can use is that val2 is always positive
if val != 0 and it never takes a value beyond +- MICRO because
otherwise val would be non 0 instead.


Taking original code and ruling out cases.
+	case 4:
+		if (val2 >= 0 && val > S16_MAX / 2)
// If val is non 0 then val2 is postive, so
//		if (val > S16_MAX / 2)
//			reg_val = S16_MAX;

+			reg_val = S16_MAX;
+		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)

// If val2 < 0 then val == 0 which is never less than S16_MIN / 2
// So this condition never happens.

+			reg_val = S16_MIN;
+		else if (val2 < 0)
// likewise, this is actually clamping val2 * 2 / MICRO which 
// is never going to be anywhere near S16_MIN or S16_MAX as I think
// it is always between +1 and -1 as val2 itself is limited to -MICRO to MICRO

+			reg_val = clamp_t(int,
+				-(val * 2 + -val2 * 2 / MICRO),
+				S16_MIN, S16_MAX);
+		else if (val < 0)
//This one is fine.
+			reg_val = clamp_t(int,
+				val * 2 - val2 * 2 / MICRO,
+				S16_MIN, S16_MAX);
+		else
//As is this one
+			reg_val = clamp_t(int,
+				val * 2 + val2 * 2 / MICRO,
+				S16_MIN, S16_MAX);
+		return reg_val;

Maybe trick is to reorder into 3 conditions and set the value in a temporary integer.
	int val_calc;
	if (val > 0)
		val_calc = val * 2 + val2 * 2 / MICRO;
	else if (val < 0)
		val_calc = -(val * 2 - val2 * 2 / MICRO);
	else /* Only now does val2 sign matter as val == 0 */
		val_calc = val2 * 2 / MICRO;

Which can simplify because we know val is 0 for last case.
Whether this is worth doing depends on trade off between
docs needed to explain the code and shorter code.

	/* Note that val2 > 0 if val != 0 and val2 range +- MICRO */
	if (val < 0)
		val_calc = val * 2 - val2 * 2 / MICRO;
	else
		val_calc = val * 2 + val2 * 2 / MICRO;

	reg_val = clamp_t(int, val_calc, S16_MIN, S16_MAX);
	
One trivial additional simplication below.

You might also be able to scale temporary up by 2 and ust
have the switch statement set a scaling value.

In this case scale == 4 in other cases below, 2, 1, and 8 for the default


	if (val < 0)
		val_calc = val * scale - val2 * scale / MICRO;
	else
		val_calc = val * scale + val2 * scale / MICRO;

	val_calc /= 2; /* to remove the factor of 2 */

	reg_val = clamp_t (int, val_calc, S16_MIN, S16_MAX);
after the switch statement with comments when setting scale on the * 2
multiplier to avoid the / 2 for case 64.

> 
> - Trevor
> 
> >
> > 		
> >
> >  
> >> +			reg_val = S16_MIN;
> >> +		else if (val2 < 0)
> >> +			reg_val = clamp_t(int,
> >> +				-(val * 2 + -val2 * 2 / MICRO),
> >> +				S16_MIN, S16_MAX);
> >> +		else if (val < 0)
> >> +			reg_val = clamp_t(int,
> >> +				val * 2 - val2 * 2 / MICRO,
> >> +				S16_MIN, S16_MAX);
> >> +		else
> >> +			reg_val = clamp_t(int,
> >> +				val * 2 + val2 * 2 / MICRO,
> >> +				S16_MIN, S16_MAX);
> >> +		return reg_val;
> >> +	case 16:
> >> +		if (val2 >= 0 && val > S16_MAX)
> >> +			reg_val = S16_MAX;
> >> +		else if ((val2 < 0 ? -val : val) < S16_MIN)
> >> +			reg_val = S16_MIN;
> >> +		else if (val2 < 0)
> >> +			reg_val = clamp_t(int,
> >> +				-(val + -val2 / MICRO),
> >> +				S16_MIN, S16_MAX);
> >> +		else if (val < 0)
> >> +			reg_val = clamp_t(int,
> >> +				val - val2 / MICRO,
> >> +				S16_MIN, S16_MAX);
> >> +		else
> >> +			reg_val = clamp_t(int,
> >> +				val + val2 / MICRO,
> >> +				S16_MIN, S16_MAX);
> >> +		return reg_val;
> >> +	case 64:
> >> +		if (val2 >= 0 && val > S16_MAX * 2)
> >> +			reg_val = S16_MAX;
> >> +		else if ((val2 < 0 ? -val : val) < S16_MIN * 2)
> >> +			reg_val = S16_MIN;
> >> +		else if (val2 < 0)
> >> +			reg_val = clamp_t(int,
> >> +				-(val / 2 + -val2 / 2 / MICRO),
> >> +				S16_MIN, S16_MAX);
> >> +		else if (val < 0)
> >> +			reg_val = clamp_t(int,
> >> +				val / 2 - val2 / 2 / MICRO,

For these val2 / 2 / MICRO always 0 so value of val2 never matters.

> >> +				S16_MIN, S16_MAX);
> >> +		else
> >> +			reg_val = clamp_t(int,
> >> +				val / 2 + val2 / 2 / MICRO,
> >> +				S16_MIN, S16_MAX);
> >> +		return reg_val;
> >> +	default:
> >> +		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);
> >> +		return reg_val;
> >> +	}
> >> +}
> >> +  

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

[Trevor Gamblin]

On 2025-01-04 07:30, Jonathan Cameron wrote:
> On Thu, 2 Jan 2025 13:19:19 -0500
> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>
>> On 2024-12-19 11:13, Jonathan Cameron wrote:
>>> On Tue, 17 Dec 2024 16:47:28 -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>
>>> Hi Trevor,
>>>
>>> The clamping fun of get_calibbias seems overkill. If this isn't going to ever
>>> overflow an s64 maybe just use the high precision to do it the easy way.
>>> I'm not sure you can't just fit it in an s32 for that matter. I've just
>>> not done the maths to check.
>>>
>>> Jonathan
>>>
>>>   
>>>> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
>>>> +{
>>>> +	unsigned int reg_val;
>>>> +
>>>> +	switch (osr) {
>>>> +	case 4:
>>>> +		if (val2 >= 0 && val > S16_MAX / 2)
>>>> +			reg_val = S16_MAX;
>>>> +		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)
>>> It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise
>>> we carry the sign in the val part.  Sometimes we generalize that to
>>> make life easier for driver writers but I think you can use that here
>>> to simplify things.
>>>
>>> (for background look at __iio_str_to_fixpoint() - it's a bit of a hack
>>> to deal with integers have no negative 0)
>>>
>>> 		if (val > S16_MAX / 2)
>>> 			...
>>> 		else if (val < S16_MIN / 2)
>>> 			...	
>>> 		else if (val2 < 0) etc
>>>
>>> You may feel it is better to keep the code considering the val2 < 0 when
>>> val != 0 case and I don't mind that as it's not wrong, just overly complex!
>>>
>>> If you can easily clamp the overall range you can just do some maths
>>> with enough precision to get one number (probably a s64) and clamp that.
>>> Easy to sanity check for overflow based on val to ensure no overflows.
>> Hi Jonathan,
>>
>> I'm reviewing this again but I'm not entirely clear what you mean.
>>
>> Are you suggesting that the entire switch block could be simplified
>> (i.e. eliminating the previous simplification for the val2 < 0 case in
>> the process), or that the calls to clamp_t can be combined?
>>
>> I've tested out simplifying the val2 < 0 case locally and driver
>> functionality still seems OK. Maybe I'm missing a third option.
Hi Jonathan,
> The extra info we can use is that val2 is always positive
> if val != 0 and it never takes a value beyond +- MICRO because
> otherwise val would be non 0 instead.
>
>
> Taking original code and ruling out cases.
> +	case 4:
> +		if (val2 >= 0 && val > S16_MAX / 2)
> // If val is non 0 then val2 is postive, so
> //		if (val > S16_MAX / 2)
> //			reg_val = S16_MAX;
>
> +			reg_val = S16_MAX;
> +		else if ((val2 < 0 ? -val : val) < S16_MIN / 2)
>
> // If val2 < 0 then val == 0 which is never less than S16_MIN / 2
> // So this condition never happens.
Thanks for catching these.
>
> +			reg_val = S16_MIN;
> +		else if (val2 < 0)
> // likewise, this is actually clamping val2 * 2 / MICRO which
> // is never going to be anywhere near S16_MIN or S16_MAX as I think
> // it is always between +1 and -1 as val2 itself is limited to -MICRO to MICRO
>
> +			reg_val = clamp_t(int,
> +				-(val * 2 + -val2 * 2 / MICRO),
> +				S16_MIN, S16_MAX);
> +		else if (val < 0)
> //This one is fine.
> +			reg_val = clamp_t(int,
> +				val * 2 - val2 * 2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		else
> //As is this one
> +			reg_val = clamp_t(int,
> +				val * 2 + val2 * 2 / MICRO,
> +				S16_MIN, S16_MAX);
> +		return reg_val;
>
> Maybe trick is to reorder into 3 conditions and set the value in a temporary integer.
> 	int val_calc;
> 	if (val > 0)
> 		val_calc = val * 2 + val2 * 2 / MICRO;
> 	else if (val < 0)
> 		val_calc = -(val * 2 - val2 * 2 / MICRO);
> 	else /* Only now does val2 sign matter as val == 0 */
> 		val_calc = val2 * 2 / MICRO;

I've been testing out these simplifications (but using the scaling 
suggestion from below, which is great - for some reason I had it in my 
head that doing so wasn't an option).

These seem to have some issues with signs for particularly small 
calibbias values. I think it's because while my (val2 < 0) case was 
doing unnecessary clamping, the math itself was OK.

I did some more experimenting, and came up with a new version of the 
function that looks like this:

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;
         }

         /* Note that val2 > 0 if val != 0 and val2 range +- 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 / MICRO);
         else
                 val_calc = val * scale + val2 * scale / MICRO;

         val_calc /= 2;

         return clamp_t(int, val_calc, S16_MIN, S16_MAX);
}

This seems to match all of the expected outputs for the 
pre-simplification version in this patch series when I test it. If there 
are no issues with it, I'll send a v2.

>
> Which can simplify because we know val is 0 for last case.
> Whether this is worth doing depends on trade off between
> docs needed to explain the code and shorter code.
>
> 	/* Note that val2 > 0 if val != 0 and val2 range +- MICRO */
> 	if (val < 0)
> 		val_calc = val * 2 - val2 * 2 / MICRO;
> 	else
> 		val_calc = val * 2 + val2 * 2 / MICRO;
>
> 	reg_val = clamp_t(int, val_calc, S16_MIN, S16_MAX);
> 	
> One trivial additional simplication below.
>
> You might also be able to scale temporary up by 2 and ust
> have the switch statement set a scaling value.
>
> In this case scale == 4 in other cases below, 2, 1, and 8 for the default
>
>
> 	if (val < 0)
> 		val_calc = val * scale - val2 * scale / MICRO;
> 	else
> 		val_calc = val * scale + val2 * scale / MICRO;
>
> 	val_calc /= 2; /* to remove the factor of 2 */
>
> 	reg_val = clamp_t (int, val_calc, S16_MIN, S16_MAX);
> after the switch statement with comments when setting scale on the * 2
> multiplier to avoid the / 2 for case 64.
>
>> - Trevor
>>
>>> 		
>>>
>>>   
>>>> +			reg_val = S16_MIN;
>>>> +		else if (val2 < 0)
>>>> +			reg_val = clamp_t(int,
>>>> +				-(val * 2 + -val2 * 2 / MICRO),
>>>> +				S16_MIN, S16_MAX);
>>>> +		else if (val < 0)
>>>> +			reg_val = clamp_t(int,
>>>> +				val * 2 - val2 * 2 / MICRO,
>>>> +				S16_MIN, S16_MAX);
>>>> +		else
>>>> +			reg_val = clamp_t(int,
>>>> +				val * 2 + val2 * 2 / MICRO,
>>>> +				S16_MIN, S16_MAX);
>>>> +		return reg_val;
>>>> +	case 16:
>>>> +		if (val2 >= 0 && val > S16_MAX)
>>>> +			reg_val = S16_MAX;
>>>> +		else if ((val2 < 0 ? -val : val) < S16_MIN)
>>>> +			reg_val = S16_MIN;
>>>> +		else if (val2 < 0)
>>>> +			reg_val = clamp_t(int,
>>>> +				-(val + -val2 / MICRO),
>>>> +				S16_MIN, S16_MAX);
>>>> +		else if (val < 0)
>>>> +			reg_val = clamp_t(int,
>>>> +				val - val2 / MICRO,
>>>> +				S16_MIN, S16_MAX);
>>>> +		else
>>>> +			reg_val = clamp_t(int,
>>>> +				val + val2 / MICRO,
>>>> +				S16_MIN, S16_MAX);
>>>> +		return reg_val;
>>>> +	case 64:
>>>> +		if (val2 >= 0 && val > S16_MAX * 2)
>>>> +			reg_val = S16_MAX;
>>>> +		else if ((val2 < 0 ? -val : val) < S16_MIN * 2)
>>>> +			reg_val = S16_MIN;
>>>> +		else if (val2 < 0)
>>>> +			reg_val = clamp_t(int,
>>>> +				-(val / 2 + -val2 / 2 / MICRO),
>>>> +				S16_MIN, S16_MAX);
>>>> +		else if (val < 0)
>>>> +			reg_val = clamp_t(int,
>>>> +				val / 2 - val2 / 2 / MICRO,
> For these val2 / 2 / MICRO always 0 so value of val2 never matters.
>
>>>> +				S16_MIN, S16_MAX);
>>>> +		else
>>>> +			reg_val = clamp_t(int,
>>>> +				val / 2 + val2 / 2 / MICRO,
>>>> +				S16_MIN, S16_MAX);
>>>> +		return reg_val;
>>>> +	default:
>>>> +		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);
>>>> +		return reg_val;
>>>> +	}
>>>> +}
>>>> +

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

[David Lechner]

On 1/7/25 2:21 PM, Trevor Gamblin wrote:
> 
> On 2025-01-04 07:30, Jonathan Cameron wrote:
>> On Thu, 2 Jan 2025 13:19:19 -0500
>> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>>
>>> On 2024-12-19 11:13, Jonathan Cameron wrote:
>>>> On Tue, 17 Dec 2024 16:47:28 -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>

...

>> Maybe trick is to reorder into 3 conditions and set the value in a temporary integer.
>>     int val_calc;
>>     if (val > 0)
>>         val_calc = val * 2 + val2 * 2 / MICRO;
>>     else if (val < 0)
>>         val_calc = -(val * 2 - val2 * 2 / MICRO);
>>     else /* Only now does val2 sign matter as val == 0 */
>>         val_calc = val2 * 2 / MICRO;
> 
> I've been testing out these simplifications (but using the scaling suggestion from below, which is great - for some reason I had it in my head that doing so wasn't an option).
> 
> These seem to have some issues with signs for particularly small calibbias values. I think it's because while my (val2 < 0) case was doing unnecessary clamping, the math itself was OK.
> 

Mail is easier to read when wrapped to 80 chars. ;-)


> I did some more experimenting, and came up with a new version of the function that looks like this:
> 
> 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;
>         }
> 
>         /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */

This comment doesn't seem 100% accurate. val2 range is (-MICRO, MICRO) if
val == 0 or [0, MICRO) if val != 0.

>         if (val < 0)
>                 val_calc = val * scale - val2 * scale / MICRO;
>         else if (val2 < 0)
>                 /* if val2 < 0 then val == 0 */
>                 val_calc = -(-val2 * scale / MICRO);

Could also write this as `val2 * scale / (int)MICRO` lest someone try to remove
the double negative and break it (because MICRO is unsigned).

This also calls into question if MICRO and similar macros should actually be
unsigned because it can lead to subtle bugs since it is perfectly reasonable
to expect -1 * MICRO to be -1000000, but it isn't.

>         else
>                 val_calc = val * scale + val2 * scale / MICRO;
> 
>         val_calc /= 2;
> 
>         return clamp_t(int, val_calc, S16_MIN, S16_MAX);
> }
> 
> This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2.

Probably not a big deal, but there is unhanded overflow when val is near S32_MAX
or S32_MIN.

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

[Trevor Gamblin]

On 2025-01-07 16:02, David Lechner wrote:
> On 1/7/25 2:21 PM, Trevor Gamblin wrote:
>> On 2025-01-04 07:30, Jonathan Cameron wrote:
>>> On Thu, 2 Jan 2025 13:19:19 -0500
>>> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>>>
>>>> On 2024-12-19 11:13, Jonathan Cameron wrote:
>>>>> On Tue, 17 Dec 2024 16:47:28 -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>
> ...
>
>>> Maybe trick is to reorder into 3 conditions and set the value in a temporary integer.
>>>      int val_calc;
>>>      if (val > 0)
>>>          val_calc = val * 2 + val2 * 2 / MICRO;
>>>      else if (val < 0)
>>>          val_calc = -(val * 2 - val2 * 2 / MICRO);
>>>      else /* Only now does val2 sign matter as val == 0 */
>>>          val_calc = val2 * 2 / MICRO;
>> I've been testing out these simplifications (but using the scaling suggestion from below, which is great - for some reason I had it in my head that doing so wasn't an option).
>>
>> These seem to have some issues with signs for particularly small calibbias values. I think it's because while my (val2 < 0) case was doing unnecessary clamping, the math itself was OK.
>>
> Mail is easier to read when wrapped to 80 chars. ;-)
My bad.
>
>
>> I did some more experimenting, and came up with a new version of the function that looks like this:
>>
>> 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;
>>          }
>>
>>          /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */
> This comment doesn't seem 100% accurate. val2 range is (-MICRO, MICRO) if
> val == 0 or [0, MICRO) if val != 0.
Alright, will fix this.
>
>>          if (val < 0)
>>                  val_calc = val * scale - val2 * scale / MICRO;
>>          else if (val2 < 0)
>>                  /* if val2 < 0 then val == 0 */
>>                  val_calc = -(-val2 * scale / MICRO);
> Could also write this as `val2 * scale / (int)MICRO` lest someone try to remove
> the double negative and break it (because MICRO is unsigned).
And this.
>
> This also calls into question if MICRO and similar macros should actually be
> unsigned because it can lead to subtle bugs since it is perfectly reasonable
> to expect -1 * MICRO to be -1000000, but it isn't.
>
>>          else
>>                  val_calc = val * scale + val2 * scale / MICRO;
>>
>>          val_calc /= 2;
>>
>>          return clamp_t(int, val_calc, S16_MIN, S16_MAX);
>> }
>>
>> This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2.
> Probably not a big deal, but there is unhanded overflow when val is near S32_MAX
> or S32_MIN.
Should I handle that with an extra call to clamp_t()?

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

[David Lechner]

On 1/8/25 1:54 PM, Trevor Gamblin wrote:
> 
> On 2025-01-07 16:02, David Lechner wrote:
>> On 1/7/25 2:21 PM, Trevor Gamblin wrote:
>>> On 2025-01-04 07:30, Jonathan Cameron wrote:
>>>> On Thu, 2 Jan 2025 13:19:19 -0500
>>>> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>>>>
>>>>> On 2024-12-19 11:13, Jonathan Cameron wrote:
>>>>>> On Tue, 17 Dec 2024 16:47:28 -0500
>>>>>> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>>>>>>  
...

>>>          else
>>>                  val_calc = val * scale + val2 * scale / MICRO;
>>>
>>>          val_calc /= 2;
>>>
>>>          return clamp_t(int, val_calc, S16_MIN, S16_MAX);
>>> }
>>>
>>> This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2.
>> Probably not a big deal, but there is unhanded overflow when val is near S32_MAX
>> or S32_MIN.
> Should I handle that with an extra call to clamp_t()?

It wouldn't hurt. val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8); before
the rest of the math should do the trick since the max scale is 8.

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

[Trevor Gamblin]

On 2024-12-19 10:46, Jonathan Cameron wrote:
> On Tue, 17 Dec 2024 16:47:27 -0500
> Trevor Gamblin <tgamblin@baylibre.com> 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.
>>
>> A dilemma that came up during development of this feature was whether or
>> not the implementation of oversampling ratios against sampling frequency
>> was actually correct. More specifically, it's unclear if the sampling
>> frequency attribute is supposed to be the conversion rate or the data
>> read rate (according to the IIO subsystem).
Hi Jonathan,
> Intent was it's the rate at which the reading becomes available to the
> software (which I think you are calling data read rate).
>
> In global schemes, it's all of the scan (normally they are
> self clocked with delays between scans) in per channel schemes intent
> was to provide the sampling rate at which that channel was sampled
> if no others were being read (assuming the times sum up in a linear
> way etc).
>
>> If it's the former, then
>> this implementation is probably incorrect. David Lechner pointed out
>> during review that it would be easier if it were defined as the
>> conversion rate and that it was userspace's responsibility to handle
>> oversampling ratio, but that might also require more work in the IIO
>> subsystem.
> IIRC there are devices out there were oversampling rate is on top of
> their controlled read out rate (which is more about delays between
> starting sampling than about how long it takes).  I think one of those
> drove the implementation of oversampling in the first place.
> It's also not the case that an oversampling rate of x4 always means
> that the time to data availability is necessarily 1/4 (can pipeline
> some stages depending on the sensor design).
>
> My thinking (it was a long time ago) was that userspace wouldn't want
> to deal with the scaling.  We only fairly recently defined the
> clear concept of per channel sampling frequencies. Before that they
> were (almost?) all global.
>
> Another aspect is that when we originally added this, it was new to userspace
> so having defined sampling frequency, we couldn't have it's meaning
> changing because oversampling was say a minimum of 2 on a device.
>
>> Two other ADC drivers that were referenced for inspiration
>> when working through this were the ad7380 and the rtq6056. The ad7380
>> has a global oversampling setting rather than per-channel, and the
> I'm not sure it being global matters a lot for question of whether
> it takes into account oversampling or not.
>
>> rtq6056 seems at least partially broken because it only takes
>> oversampling ratio into account when getting the sampling frequency (but
>> not when setting it).
> That is definitely broken and could do with a fix.  We should always
> read back more or less the same value written (subject to rounding
> etc in some cases).
>
>
>> Instead of per-driver implementation, these three
>> drivers might serve as inspiration for changes to how oversampling is
>> handled in IIO?
> It is very tricky to make any modifications (other than the fix above)
> without making a mess of the ABI.
>
> Or are you suggesting we could so something in the IIO core?  Maybe
> some helper functions are appropriate (similar to Matti's GTS stuff
> for gains where they are a mixture of various factors on light sensors
> and similar.).

Thanks for providing all of the info above. I think that clears things 
up enough for this series (i.e. that the current implementation is the 
right way to do it). I will take a note about adding some helper 
functions for this, and about updating rtq6056, although I don't have 
access to hardware to test for the latter case. v2 should be coming soon.

- Trevor