[PATCH RFC 0/4] iio: add support for multiple scan types

[PATCH RFC 0/4] iio: add support for multiple scan types

[David Lechner]

Following up from this thread [1]...

Unless I've overlooked something important, I think adding support for
multiple scan types per channels should be rather trivial, at least in
the kernel. Userspace tools will need to learn to re-read buffer _type
attributes though. For example, it looks like libiio caches these values.
I had to restart iiod to get a proper capture with the iio-oscilloscope
after changing the scan type at runtime.

Here is a bit of background for those not following along:

Up to now, the IIO subsystem has only supported a single scan type per
channel. This scan type determines the binary format of the data in the
buffer when doing buffered reads.

For simple devices, there is only one scan type and all is well. But
for more complex devices, there may be multiple scan types. For example,
ADCs with a resolution boost feature that adds more bits to the raw
sample data. Traditionally, for slow devices, we've just always used the
highest resolution mode, but for high performance ADCs, this may not be
always practical. Manipulating data after every read can hurt performance
and in the case of hardware buffers, it may not be possible to change the
format of the data in the buffer at all. There are also ADCs where
enabling the higher resolution can only be done if oversampling is also
enabled which may not be desireable.

To allow for more flexibility, we would like to add support for multiple
scan types per channel.

To avoid having to touch every driver, we implemented this in a way that
preserves the existing scan_type field. See the "iio: add support for
multiple scan types per channel" the details. The first couple of patches
are just preparation for this.

The last patch shows how to use this new feature in the ad7380 driver
which is still under review at [2].

[1]: https://lore.kernel.org/linux-iio/CAMknhBHOXaff__QyU-wFSNNENvs23vDX5n_ddH-Dw3s6-sQ9sg@mail.gmail.com/
[2]: https://lore.kernel.org/linux-iio/20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com/T/#t

---
David Lechner (4):
      iio: introduce struct iio_scan_type
      iio: buffer: use struct iio_scan_type to simplify code
      iio: add support for multiple scan types per channel
      iio: adc: ad7380: add support for multiple scan type

 drivers/iio/adc/ad7380.c          | 185 ++++++++++++++++++--------------------
 drivers/iio/industrialio-buffer.c |  77 +++++++++++-----
 include/linux/iio/iio.h           |  74 +++++++++++----
 3 files changed, 194 insertions(+), 142 deletions(-)
---
base-commit: 32cf3c865729172e67dced11c0b73e658444888a
change-id: 20240507-iio-add-support-for-multiple-scan-types-f4dbcf4c2cb8

[PATCH RFC 1/4] iio: introduce struct iio_scan_type

[David Lechner]

This gives the channel scan_type a named type so that it can be used
to simplify code in later commits.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 include/linux/iio/iio.h | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 55e2b22086a1..19de573a944a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -173,6 +173,27 @@ struct iio_event_spec {
 	unsigned long mask_shared_by_all;
 };
 
+/**
+ * struct iio_scan_type - specification for channel data format in buffer
+ * @sign:		's' or 'u' to specify signed or unsigned
+ * @realbits:		Number of valid bits of data
+ * @storagebits:	Realbits + padding
+ * @shift:		Shift right by this before masking out realbits.
+ * @repeat:		Number of times real/storage bits repeats. When the
+ *			repeat element is more than 1, then the type element in
+ *			sysfs will show a repeat value. Otherwise, the number
+ *			of repetitions is omitted.
+ * @endianness:		little or big endian
+ */
+struct iio_scan_type {
+	char	sign;
+	u8	realbits;
+	u8	storagebits;
+	u8	shift;
+	u8	repeat;
+	enum iio_endian endianness;
+};
+
 /**
  * struct iio_chan_spec - specification of a single channel
  * @type:		What type of measurement is the channel making.
@@ -184,17 +205,6 @@ struct iio_event_spec {
  * @scan_index:		Monotonic index to give ordering in scans when read
  *			from a buffer.
  * @scan_type:		struct describing the scan type
- * @scan_type.sign:		's' or 'u' to specify signed or unsigned
- * @scan_type.realbits:		Number of valid bits of data
- * @scan_type.storagebits:	Realbits + padding
- * @scan_type.shift:		Shift right by this before masking out
- *				realbits.
- * @scan_type.repeat:		Number of times real/storage bits repeats.
- *				When the repeat element is more than 1, then
- *				the type element in sysfs will show a repeat
- *				value. Otherwise, the number of repetitions
- *				is omitted.
- * @scan_type.endianness:	little or big endian
  * @info_mask_separate: What information is to be exported that is specific to
  *			this channel.
  * @info_mask_separate_available: What availability information is to be
@@ -245,14 +255,7 @@ struct iio_chan_spec {
 	int			channel2;
 	unsigned long		address;
 	int			scan_index;
-	struct {
-		char	sign;
-		u8	realbits;
-		u8	storagebits;
-		u8	shift;
-		u8	repeat;
-		enum iio_endian endianness;
-	} scan_type;
+	struct iio_scan_type scan_type;
 	long			info_mask_separate;
 	long			info_mask_separate_available;
 	long			info_mask_shared_by_type;

-- 
2.43.2

[PATCH RFC 2/4] iio: buffer: use struct iio_scan_type to simplify code

[David Lechner]

By using struct iio_scan_type, we can simplify the code by removing
lots of duplicate pointer deferences. This make the code a bit easier to
read.

This also prepares for a future where channels may have more than one
scan_type.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/industrialio-buffer.c | 48 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index cec58a604d73..08103a9e77f7 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -366,7 +366,8 @@ static ssize_t iio_show_fixed_type(struct device *dev,
 				   char *buf)
 {
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	u8 type = this_attr->c->scan_type.endianness;
+	const struct iio_scan_type *scan_type = &this_attr->c->scan_type;
+	u8 type = scan_type->endianness;
 
 	if (type == IIO_CPU) {
 #ifdef __LITTLE_ENDIAN
@@ -375,21 +376,21 @@ static ssize_t iio_show_fixed_type(struct device *dev,
 		type = IIO_BE;
 #endif
 	}
-	if (this_attr->c->scan_type.repeat > 1)
+	if (scan_type->repeat > 1)
 		return sysfs_emit(buf, "%s:%c%d/%dX%d>>%u\n",
 		       iio_endian_prefix[type],
-		       this_attr->c->scan_type.sign,
-		       this_attr->c->scan_type.realbits,
-		       this_attr->c->scan_type.storagebits,
-		       this_attr->c->scan_type.repeat,
-		       this_attr->c->scan_type.shift);
+		       scan_type->sign,
+		       scan_type->realbits,
+		       scan_type->storagebits,
+		       scan_type->repeat,
+		       scan_type->shift);
 	else
 		return sysfs_emit(buf, "%s:%c%d/%d>>%u\n",
 		       iio_endian_prefix[type],
-		       this_attr->c->scan_type.sign,
-		       this_attr->c->scan_type.realbits,
-		       this_attr->c->scan_type.storagebits,
-		       this_attr->c->scan_type.shift);
+		       scan_type->sign,
+		       scan_type->realbits,
+		       scan_type->storagebits,
+		       scan_type->shift);
 }
 
 static ssize_t iio_scan_el_show(struct device *dev,
@@ -694,12 +695,16 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev,
 					     unsigned int scan_index)
 {
 	const struct iio_chan_spec *ch;
+	const struct iio_scan_type *scan_type;
 	unsigned int bytes;
 
 	ch = iio_find_channel_from_si(indio_dev, scan_index);
-	bytes = ch->scan_type.storagebits / 8;
-	if (ch->scan_type.repeat > 1)
-		bytes *= ch->scan_type.repeat;
+	scan_type = &ch->scan_type;
+	bytes = scan_type->storagebits / 8;
+
+	if (scan_type->repeat > 1)
+		bytes *= scan_type->repeat;
+
 	return bytes;
 }
 
@@ -1616,18 +1621,21 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	if (channels) {
 		/* new magic */
 		for (i = 0; i < indio_dev->num_channels; i++) {
+			const struct iio_scan_type *scan_type;
+
 			if (channels[i].scan_index < 0)
 				continue;
 
+			scan_type = &channels[i].scan_type;
+
 			/* Verify that sample bits fit into storage */
-			if (channels[i].scan_type.storagebits <
-			    channels[i].scan_type.realbits +
-			    channels[i].scan_type.shift) {
+			if (scan_type->storagebits <
+			    scan_type->realbits + scan_type->shift) {
 				dev_err(&indio_dev->dev,
 					"Channel %d storagebits (%d) < shifted realbits (%d + %d)\n",
-					i, channels[i].scan_type.storagebits,
-					channels[i].scan_type.realbits,
-					channels[i].scan_type.shift);
+					i, scan_type->storagebits,
+					scan_type->realbits,
+					scan_type->shift);
 				ret = -EINVAL;
 				goto error_cleanup_dynamic;
 			}

-- 
2.43.2

[PATCH RFC 3/4] iio: add support for multiple scan types per channel

[David Lechner]

This adds new fields to the iio_channel structure to support multiple
scan types per channel. This is useful for devices that support multiple
resolution modes or other modes that require different data formats of
the raw data.

To make use of this, drivers can still use the old scan_type field for
the "default" scan type and use the new scan_type_ext field for any
additional scan types. And they must implement the new callback
get_current_scan_type() to return the current scan type based on the
current state of the device.

The buffer code is the only code in the IIO core code that is using the
scan_type field. This patch updates the buffer code to use the new
iio_channel_validate_scan_type() function to ensure it is returning the
correct scan type for the current state of the device when reading the
sysfs attributes. The buffer validation code is also update to validate
any additional scan types that are set in the scan_type_ext field. Part
of that code is refactored to a new function to avoid duplication.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/industrialio-buffer.c | 43 +++++++++++++++++++++++++++++----------
 include/linux/iio/iio.h           | 33 ++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 08103a9e77f7..ef27ce71ec25 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -365,8 +365,10 @@ static ssize_t iio_show_fixed_type(struct device *dev,
 				   struct device_attribute *attr,
 				   char *buf)
 {
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	const struct iio_scan_type *scan_type = &this_attr->c->scan_type;
+	const struct iio_scan_type *scan_type =
+				iio_get_current_scan_type(indio_dev, this_attr->c);
 	u8 type = scan_type->endianness;
 
 	if (type == IIO_CPU) {
@@ -699,7 +701,7 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev,
 	unsigned int bytes;
 
 	ch = iio_find_channel_from_si(indio_dev, scan_index);
-	scan_type = &ch->scan_type;
+	scan_type = iio_get_current_scan_type(indio_dev, ch);
 	bytes = scan_type->storagebits / 8;
 
 	if (scan_type->repeat > 1)
@@ -1597,6 +1599,22 @@ static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp
 	}
 }
 
+static int iio_channel_validate_scan_type(struct device *dev, int ch,
+					  const struct iio_scan_type *scan_type)
+{
+	/* Verify that sample bits fit into storage */
+	if (scan_type->storagebits < scan_type->realbits + scan_type->shift) {
+		dev_err(dev,
+			"Channel %d storagebits (%d) < shifted realbits (%d + %d)\n",
+			ch, scan_type->storagebits,
+			scan_type->realbits,
+			scan_type->shift);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 					     struct iio_dev *indio_dev,
 					     int index)
@@ -1622,22 +1640,25 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		/* new magic */
 		for (i = 0; i < indio_dev->num_channels; i++) {
 			const struct iio_scan_type *scan_type;
+			int j;
 
 			if (channels[i].scan_index < 0)
 				continue;
 
 			scan_type = &channels[i].scan_type;
 
-			/* Verify that sample bits fit into storage */
-			if (scan_type->storagebits <
-			    scan_type->realbits + scan_type->shift) {
-				dev_err(&indio_dev->dev,
-					"Channel %d storagebits (%d) < shifted realbits (%d + %d)\n",
-					i, scan_type->storagebits,
-					scan_type->realbits,
-					scan_type->shift);
-				ret = -EINVAL;
+			ret = iio_channel_validate_scan_type(&indio_dev->dev,
+							     i, scan_type);
+			if (ret)
 				goto error_cleanup_dynamic;
+
+			for (j = 0; j < channels[i].num_ext_scan_type; j++) {
+				scan_type = &channels[i].ext_scan_type[j];
+
+				ret = iio_channel_validate_scan_type(
+						&indio_dev->dev, i, scan_type);
+				if (ret)
+					goto error_cleanup_dynamic;
 			}
 
 			ret = iio_buffer_add_channel_sysfs(indio_dev, buffer,
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 19de573a944a..66f0b4c68f53 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -205,6 +205,9 @@ struct iio_scan_type {
  * @scan_index:		Monotonic index to give ordering in scans when read
  *			from a buffer.
  * @scan_type:		struct describing the scan type
+ * @ext_scan_type:	Used in rare cases where there is more than one scan
+ *			format for a channel. When this is used, omit scan_type.
+ * @num_ext_scan_type:	Number of elements in ext_scan_type.
  * @info_mask_separate: What information is to be exported that is specific to
  *			this channel.
  * @info_mask_separate_available: What availability information is to be
@@ -256,6 +259,8 @@ struct iio_chan_spec {
 	unsigned long		address;
 	int			scan_index;
 	struct iio_scan_type scan_type;
+	const struct iio_scan_type *ext_scan_type;
+	unsigned int		num_ext_scan_type;
 	long			info_mask_separate;
 	long			info_mask_separate_available;
 	long			info_mask_shared_by_type;
@@ -435,6 +440,9 @@ struct iio_trigger; /* forward declaration */
  *			for better event identification.
  * @validate_trigger:	function to validate the trigger when the
  *			current trigger gets changed.
+ * @get_current_scan_type: must be implemented by drivers that use ext_scan_type
+ *			in the channel spec to return the currently active scan
+ *			type based on the current state of the device.
  * @update_scan_mode:	function to configure device and scan buffer when
  *			channels have changed
  * @debugfs_reg_access:	function to read or write register value of device
@@ -519,6 +527,9 @@ struct iio_info {
 
 	int (*validate_trigger)(struct iio_dev *indio_dev,
 				struct iio_trigger *trig);
+	const struct iio_scan_type *(*get_current_scan_type)(
+					const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan);
 	int (*update_scan_mode)(struct iio_dev *indio_dev,
 				const unsigned long *scan_mask);
 	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
@@ -804,6 +815,28 @@ static inline bool iio_read_acpi_mount_matrix(struct device *dev,
 }
 #endif
 
+/**
+ * iio_get_current_scan_type - Get the current scan type for a channel
+ * @indio_dev:	the IIO device to get the scan type for
+ * @chan:	the channel to get the scan type for
+ *
+ * Most devices only have one scan type per channel and can just access it
+ * directly without calling this function. Core IIO code and drivers that
+ * implement ext_scan_type in the channel spec should use this function to
+ * get the current scan type for a channel.
+ *
+ * Returns: the current scan type for the channel
+ */
+static inline const struct iio_scan_type *iio_get_current_scan_type(
+					const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	if (indio_dev->info->get_current_scan_type)
+		return indio_dev->info->get_current_scan_type(indio_dev, chan);
+
+	return &chan->scan_type;
+}
+
 ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
 
 int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,

-- 
2.43.2

[PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[David Lechner]

The AD783x chips have a resolution boost feature that allows for 2
extra bits of resolution. Previously, we had to choose a scan type to
fit the largest resolution and manipulate the raw data to fit when the
resolution was lower. This patch adds support for multiple scan types
for the voltage input channels so that we can support both resolutions
without having to manipulate the raw data.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 185 ++++++++++++++++++++++-------------------------
 1 file changed, 86 insertions(+), 99 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index e240098708e9..ca317e3a72d9 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -89,14 +89,22 @@ struct ad7380_chip_info {
 	const struct ad7380_timing_specs *timing_specs;
 };
 
-/*
- * realbits/storagebits cannot be dynamically changed, so in order to
- * support the resolution boost (additional 2  bits of resolution)
- * we need to set realbits/storagebits to the maximum value i.e :
- *   - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
- *   - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
- * We need to adjust the scale depending on resolution boost status
- */
+/** scan type for 14-bit chips with resolution boost enabled. */
+static const struct iio_scan_type ad7380_scan_type_14_boost = {
+	.sign = 's',
+	.realbits = 16,
+	.storagebits = 16,
+	.endianness = IIO_CPU,
+};
+
+/** scan type for 16-bit chips with resolution boost enabled. */
+static const struct iio_scan_type ad7380_scan_type_16_boost = {
+	.sign = 's',
+	.realbits = 18,
+	.storagebits = 32,
+	.endianness = IIO_CPU,
+};
+
 #define AD7380_CHANNEL(index, bits, diff) {			\
 	.type = IIO_VOLTAGE,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
@@ -113,10 +121,12 @@ struct ad7380_chip_info {
 	.scan_index = (index),					\
 	.scan_type = {						\
 		.sign = 's',					\
-		.realbits = (bits) + 2,				\
-		.storagebits = ((bits) + 2 > 16) ? 32 : 16,	\
+		.realbits = (bits),				\
+		.storagebits = ((bits) > 16) ? 32 : 16,		\
 		.endianness = IIO_CPU,				\
 	},							\
+	.ext_scan_type = &ad7380_scan_type_##bits##_boost,	\
+	.num_ext_scan_type = 1,					\
 }
 
 #define DEFINE_AD7380_2_CHANNEL(name, bits, diff)	\
@@ -376,67 +386,62 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 	unreachable();
 }
 
-static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer *xfer)
+/**
+ * Reads one set of samples from the device. This is a simultaneous sampling
+ * chip, so all channels are always read at the same time.
+ *
+ * On successful return, the raw data is stored in st->scan_data.raw.
+ */
+static int ad7380_read_one_sample(struct ad7380_state *st,
+				  const struct iio_scan_type *scan_type)
 {
-	int bits_per_word;
-
-	memset(xfer, 0, sizeof(*xfer));
-
-	xfer->rx_buf = &st->scan_data.raw;
-
-	if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
-		bits_per_word = st->chip_info->channels[0].scan_type.realbits;
-	else
-		bits_per_word = st->chip_info->channels[0].scan_type.realbits - 2;
-
-	xfer->bits_per_word = bits_per_word;
+	struct spi_transfer xfers[2] = {
+		/* toggle CS (no data xfer) to trigger a conversion */
+		{
+			.speed_hz = AD7380_REG_WR_SPEED_HZ,
+			.bits_per_word = scan_type->realbits,
+			.delay = {
+				.value = T_CONVERT_NS,
+				.unit = SPI_DELAY_UNIT_NSECS,
+			},
+			.cs_change = 1,
+			.cs_change_delay = {
+				.value = st->chip_info->timing_specs->t_csh_ns,
+				.unit = SPI_DELAY_UNIT_NSECS,
+			},
+		},
+		{
+			.rx_buf = &st->scan_data.raw,
+			.len = BITS_TO_BYTES(scan_type->storagebits) *
+						(st->chip_info->num_channels - 1),
+			.bits_per_word = scan_type->realbits,
+		},
+	};
 
-	xfer->len = (st->chip_info->num_channels - 1) * BITS_TO_BYTES(bits_per_word);
+	/*
+	 * In normal average oversampling we need to wait for multiple conversions to be done
+	 */
+	if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
+		xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
 
-	return bits_per_word;
+	return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
 }
 
 static irqreturn_t ad7380_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
+	const struct iio_chan_spec *chan = &indio_dev->channels[0];
+	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
+								indio_dev, chan);
 	struct ad7380_state *st = iio_priv(indio_dev);
-	struct spi_transfer xfer;
-	int bits_per_word, realbits, i, ret;
+	int ret;
 
-	realbits = st->chip_info->channels[0].scan_type.realbits;
-	bits_per_word = ad7380_prepare_spi_xfer(st, &xfer);
 
-	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	ret = ad7380_read_one_sample(st, scan_type);
 	if (ret)
 		goto out;
 
-	/*
-	 * If bits_per_word == realbits (resolution boost enabled), we don't
-	 * need to manipulate the raw data, otherwise we may need to fix things
-	 * up a bit to fit the scan_type specs
-	 */
-	if (bits_per_word < realbits) {
-		if (realbits > 16 && bits_per_word <= 16) {
-			/*
-			 * Here realbits > 16 so storagebits is 32 and bits_per_word is <= 16
-			 * so we need to sign extend u16 to u32 using reverse order to
-			 * avoid writing over union data
-			 */
-			for (i = st->chip_info->num_channels - 2; i >= 0; i--)
-				st->scan_data.raw.u32[i] = sign_extend32(st->scan_data.raw.u16[i],
-									 bits_per_word - 1);
-		} else if (bits_per_word < 16) {
-			/*
-			 * Here realbits <= 16 so storagebits is 16.
-			 * We only need to sign extend only if bits_per_word is < 16
-			 */
-			for (i = 0; i < st->chip_info->num_channels - 1; i++)
-				st->scan_data.raw.u16[i] = sign_extend32(st->scan_data.raw.u16[i],
-									 bits_per_word - 1);
-		}
-	}
-
 	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
 					   pf->timestamp);
 
@@ -447,47 +452,21 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
 }
 
 static int ad7380_read_direct(struct ad7380_state *st,
-			      struct iio_chan_spec const *chan, int *val)
+			      struct iio_chan_spec const *chan,
+			      const struct iio_scan_type *scan_type,
+			      int *val)
 {
-	struct spi_transfer xfers[2] = {
-		/* toggle CS (no data xfer) to trigger a conversion */
-		{
-			.speed_hz = AD7380_REG_WR_SPEED_HZ,
-			.bits_per_word = chan->scan_type.realbits,
-			.delay = {
-				.value = T_CONVERT_NS,
-				.unit = SPI_DELAY_UNIT_NSECS,
-			},
-			.cs_change = 1,
-			.cs_change_delay = {
-				.value = st->chip_info->timing_specs->t_csh_ns,
-				.unit = SPI_DELAY_UNIT_NSECS,
-			},
-		},
-		/* then read all channels, it will be filled by ad7380_prepare_spi_xfer */
-		{
-		},
-	};
-	int bits_per_word, ret;
-
-	/*
-	 * In normal average oversampling we need to wait for multiple conversions to be done
-	 */
-	if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
-		xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
-
-	bits_per_word = ad7380_prepare_spi_xfer(st, &xfers[1]);
+	int ret;
 
-	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
-	if (ret < 0)
+	ret = ad7380_read_one_sample(st, scan_type);
+	if (ret)
 		return ret;
-
-	if (bits_per_word > 16)
+	if (scan_type->storagebits > 16)
 		*val = sign_extend32(st->scan_data.raw.u32[chan->scan_index],
-				     bits_per_word - 1);
+				     scan_type->realbits - 1);
 	else
 		*val = sign_extend32(st->scan_data.raw.u16[chan->scan_index],
-				     bits_per_word - 1);
+				     scan_type->realbits - 1);
 
 	return IIO_VAL_INT;
 }
@@ -496,18 +475,14 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long info)
 {
+	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
+								indio_dev, chan);
 	struct ad7380_state *st = iio_priv(indio_dev);
-	int realbits;
-
-	if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
-		realbits = chan->scan_type.realbits;
-	else
-		realbits = chan->scan_type.realbits - 2;
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
 		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			return ad7380_read_direct(st, chan, val);
+			return ad7380_read_direct(st, chan, scan_type, val);
 		}
 		unreachable();
 	case IIO_CHAN_INFO_SCALE:
@@ -520,7 +495,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 		 * According to IIO ABI, offset is applied before scale,
 		 * so offset is: vcm_mv / scale
 		 */
-		*val = st->vcm_mv[chan->channel] * (1 << realbits)
+		*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
 			/ st->vref_mv;
 
 		return IIO_VAL_INT;
@@ -700,6 +675,17 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static const struct iio_scan_type *ad7380_get_current_scan_type(
+		const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+
+	if (st->resolution_boost_enable && chan->num_ext_scan_type)
+		return chan->ext_scan_type;
+
+	return &chan->scan_type;
+}
+
 static ssize_t oversampling_mode_show(struct device *dev,
 				      struct device_attribute *attr, char *buf)
 {
@@ -796,6 +782,7 @@ static const struct iio_info ad7380_info = {
 	.read_raw = &ad7380_read_raw,
 	.read_avail = &ad7380_read_avail,
 	.write_raw = &ad7380_write_raw,
+	.get_current_scan_type = &ad7380_get_current_scan_type,
 	.debugfs_reg_access = &ad7380_debugfs_reg_access,
 };
 

-- 
2.43.2

Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[Jonathan Cameron]

On Tue,  7 May 2024 14:02:08 -0500
David Lechner <dlechner@baylibre.com> wrote:

> The AD783x chips have a resolution boost feature that allows for 2
> extra bits of resolution. Previously, we had to choose a scan type to
> fit the largest resolution and manipulate the raw data to fit when the
> resolution was lower. This patch adds support for multiple scan types
> for the voltage input channels so that we can support both resolutions
> without having to manipulate the raw data.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

I'm wondering about the control mechanism.  I was thinking we'd poke
the scan type directly but this may well make more sense.

This is relying on _scale change to trigger the change in the scan type.
That may well be sufficient and I've been over thinking this for far too many
years :)

It will get messy though in some cases as the device might have a PGA on the
front end so we will have a trade off between actual scaling control and
resolution related scale changes. We've had a few device where the scale
calculation is already complex and involves various different hardware
controls, but none have affected the storage format like this.

I'll think some more.

> ---
>  drivers/iio/adc/ad7380.c | 185 ++++++++++++++++++++++-------------------------
>  1 file changed, 86 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index e240098708e9..ca317e3a72d9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -89,14 +89,22 @@ struct ad7380_chip_info {
>  	const struct ad7380_timing_specs *timing_specs;
>  };
>  
> -/*
> - * realbits/storagebits cannot be dynamically changed, so in order to
> - * support the resolution boost (additional 2  bits of resolution)
> - * we need to set realbits/storagebits to the maximum value i.e :
> - *   - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - *   - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> - * We need to adjust the scale depending on resolution boost status
> - */
> +/** scan type for 14-bit chips with resolution boost enabled. */
> +static const struct iio_scan_type ad7380_scan_type_14_boost = {
> +	.sign = 's',
> +	.realbits = 16,
> +	.storagebits = 16,
> +	.endianness = IIO_CPU,
> +};
> +
> +/** scan type for 16-bit chips with resolution boost enabled. */
> +static const struct iio_scan_type ad7380_scan_type_16_boost = {
> +	.sign = 's',
> +	.realbits = 18,
> +	.storagebits = 32,
> +	.endianness = IIO_CPU,
> +};
> +
>  #define AD7380_CHANNEL(index, bits, diff) {			\
>  	.type = IIO_VOLTAGE,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> @@ -113,10 +121,12 @@ struct ad7380_chip_info {
>  	.scan_index = (index),					\
>  	.scan_type = {						\
>  		.sign = 's',					\
> -		.realbits = (bits) + 2,				\
> -		.storagebits = ((bits) + 2 > 16) ? 32 : 16,	\
> +		.realbits = (bits),				\
> +		.storagebits = ((bits) > 16) ? 32 : 16,		\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.ext_scan_type = &ad7380_scan_type_##bits##_boost,	\
> +	.num_ext_scan_type = 1,					\
>  }
>  
>  #define DEFINE_AD7380_2_CHANNEL(name, bits, diff)	\
> @@ -376,67 +386,62 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  	unreachable();
>  }
>  
> -static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer *xfer)
> +/**
> + * Reads one set of samples from the device. This is a simultaneous sampling
> + * chip, so all channels are always read at the same time.
> + *
> + * On successful return, the raw data is stored in st->scan_data.raw.
> + */
> +static int ad7380_read_one_sample(struct ad7380_state *st,
> +				  const struct iio_scan_type *scan_type)
>  {
> -	int bits_per_word;
> -
> -	memset(xfer, 0, sizeof(*xfer));
> -
> -	xfer->rx_buf = &st->scan_data.raw;
> -
> -	if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> -		bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> -	else
> -		bits_per_word = st->chip_info->channels[0].scan_type.realbits - 2;
> -
> -	xfer->bits_per_word = bits_per_word;
> +	struct spi_transfer xfers[2] = {
> +		/* toggle CS (no data xfer) to trigger a conversion */
> +		{
> +			.speed_hz = AD7380_REG_WR_SPEED_HZ,
> +			.bits_per_word = scan_type->realbits,
> +			.delay = {
> +				.value = T_CONVERT_NS,
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +			.cs_change = 1,
> +			.cs_change_delay = {
> +				.value = st->chip_info->timing_specs->t_csh_ns,
> +				.unit = SPI_DELAY_UNIT_NSECS,
> +			},
> +		},
> +		{
> +			.rx_buf = &st->scan_data.raw,
> +			.len = BITS_TO_BYTES(scan_type->storagebits) *
> +						(st->chip_info->num_channels - 1),
> +			.bits_per_word = scan_type->realbits,
> +		},
> +	};
>  
> -	xfer->len = (st->chip_info->num_channels - 1) * BITS_TO_BYTES(bits_per_word);
> +	/*
> +	 * In normal average oversampling we need to wait for multiple conversions to be done
> +	 */
> +	if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
> +		xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
>  
> -	return bits_per_word;
> +	return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
>  }
>  
>  static irqreturn_t ad7380_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> +	const struct iio_chan_spec *chan = &indio_dev->channels[0];
> +	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> +								indio_dev, chan);
>  	struct ad7380_state *st = iio_priv(indio_dev);
> -	struct spi_transfer xfer;
> -	int bits_per_word, realbits, i, ret;
> +	int ret;
>  
> -	realbits = st->chip_info->channels[0].scan_type.realbits;
> -	bits_per_word = ad7380_prepare_spi_xfer(st, &xfer);
>  
> -	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	ret = ad7380_read_one_sample(st, scan_type);
>  	if (ret)
>  		goto out;
>  
> -	/*
> -	 * If bits_per_word == realbits (resolution boost enabled), we don't
> -	 * need to manipulate the raw data, otherwise we may need to fix things
> -	 * up a bit to fit the scan_type specs
> -	 */
> -	if (bits_per_word < realbits) {
> -		if (realbits > 16 && bits_per_word <= 16) {
> -			/*
> -			 * Here realbits > 16 so storagebits is 32 and bits_per_word is <= 16
> -			 * so we need to sign extend u16 to u32 using reverse order to
> -			 * avoid writing over union data
> -			 */
> -			for (i = st->chip_info->num_channels - 2; i >= 0; i--)
> -				st->scan_data.raw.u32[i] = sign_extend32(st->scan_data.raw.u16[i],
> -									 bits_per_word - 1);
> -		} else if (bits_per_word < 16) {
> -			/*
> -			 * Here realbits <= 16 so storagebits is 16.
> -			 * We only need to sign extend only if bits_per_word is < 16
> -			 */
> -			for (i = 0; i < st->chip_info->num_channels - 1; i++)
> -				st->scan_data.raw.u16[i] = sign_extend32(st->scan_data.raw.u16[i],
> -									 bits_per_word - 1);
> -		}
> -	}
> -
>  	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
>  					   pf->timestamp);
>  
> @@ -447,47 +452,21 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
>  }
>  
>  static int ad7380_read_direct(struct ad7380_state *st,
> -			      struct iio_chan_spec const *chan, int *val)
> +			      struct iio_chan_spec const *chan,
> +			      const struct iio_scan_type *scan_type,
> +			      int *val)
>  {
> -	struct spi_transfer xfers[2] = {
> -		/* toggle CS (no data xfer) to trigger a conversion */
> -		{
> -			.speed_hz = AD7380_REG_WR_SPEED_HZ,
> -			.bits_per_word = chan->scan_type.realbits,
> -			.delay = {
> -				.value = T_CONVERT_NS,
> -				.unit = SPI_DELAY_UNIT_NSECS,
> -			},
> -			.cs_change = 1,
> -			.cs_change_delay = {
> -				.value = st->chip_info->timing_specs->t_csh_ns,
> -				.unit = SPI_DELAY_UNIT_NSECS,
> -			},
> -		},
> -		/* then read all channels, it will be filled by ad7380_prepare_spi_xfer */
> -		{
> -		},
> -	};
> -	int bits_per_word, ret;
> -
> -	/*
> -	 * In normal average oversampling we need to wait for multiple conversions to be done
> -	 */
> -	if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
> -		xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
> -
> -	bits_per_word = ad7380_prepare_spi_xfer(st, &xfers[1]);
> +	int ret;
>  
> -	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> -	if (ret < 0)
> +	ret = ad7380_read_one_sample(st, scan_type);
> +	if (ret)
>  		return ret;
> -
> -	if (bits_per_word > 16)
> +	if (scan_type->storagebits > 16)
>  		*val = sign_extend32(st->scan_data.raw.u32[chan->scan_index],
> -				     bits_per_word - 1);
> +				     scan_type->realbits - 1);
>  	else
>  		*val = sign_extend32(st->scan_data.raw.u16[chan->scan_index],
> -				     bits_per_word - 1);
> +				     scan_type->realbits - 1);
>  
>  	return IIO_VAL_INT;
>  }
> @@ -496,18 +475,14 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long info)
>  {
> +	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> +								indio_dev, chan);
>  	struct ad7380_state *st = iio_priv(indio_dev);
> -	int realbits;
> -
> -	if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> -		realbits = chan->scan_type.realbits;
> -	else
> -		realbits = chan->scan_type.realbits - 2;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
>  		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			return ad7380_read_direct(st, chan, val);
> +			return ad7380_read_direct(st, chan, scan_type, val);
>  		}
>  		unreachable();
>  	case IIO_CHAN_INFO_SCALE:
> @@ -520,7 +495,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		 * According to IIO ABI, offset is applied before scale,
>  		 * so offset is: vcm_mv / scale
>  		 */
> -		*val = st->vcm_mv[chan->channel] * (1 << realbits)
> +		*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
>  			/ st->vref_mv;
>  
>  		return IIO_VAL_INT;
> @@ -700,6 +675,17 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static const struct iio_scan_type *ad7380_get_current_scan_type(
> +		const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	if (st->resolution_boost_enable && chan->num_ext_scan_type)
> +		return chan->ext_scan_type;
> +
> +	return &chan->scan_type;
> +}
> +
>  static ssize_t oversampling_mode_show(struct device *dev,
>  				      struct device_attribute *attr, char *buf)
>  {
> @@ -796,6 +782,7 @@ static const struct iio_info ad7380_info = {
>  	.read_raw = &ad7380_read_raw,
>  	.read_avail = &ad7380_read_avail,
>  	.write_raw = &ad7380_write_raw,
> +	.get_current_scan_type = &ad7380_get_current_scan_type,
>  	.debugfs_reg_access = &ad7380_debugfs_reg_access,
>  };
>  
> 

Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[David Lechner]

On Wed, May 8, 2024 at 6:40 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue,  7 May 2024 14:02:08 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > The AD783x chips have a resolution boost feature that allows for 2
> > extra bits of resolution. Previously, we had to choose a scan type to
> > fit the largest resolution and manipulate the raw data to fit when the
> > resolution was lower. This patch adds support for multiple scan types
> > for the voltage input channels so that we can support both resolutions
> > without having to manipulate the raw data.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
>
> I'm wondering about the control mechanism.  I was thinking we'd poke
> the scan type directly but this may well make more sense.
>
> This is relying on _scale change to trigger the change in the scan type.
> That may well be sufficient and I've been over thinking this for far too many
> years :)
>
> It will get messy though in some cases as the device might have a PGA on the
> front end so we will have a trade off between actual scaling control and
> resolution related scale changes. We've had a few device where the scale
> calculation is already complex and involves various different hardware
> controls, but none have affected the storage format like this.
>
> I'll think some more.
>

Here is some more food for thought. The AD4630 family of chips we are
working on is similar to this one in that it also has oversampling
with increased resolution. Except in that case, they are strictly tied
together. With oversampling disabled, we must only read 24-bits (or 16
depending on the exact model) and when oversampling is enabled, we
must read 32-bits (30 real bits with 2-bit shift). So in that case,
the scan_type would depend only on oversampling ratio > 0. (Writing
the oversampling ratio attribute would affect scale, but scale
wouldn't be writable like on ad7380.)

It seems more intuitive to me that to enable oversampling, we would
just write to the oversampling ratio attribute rather than having to
write to a buffer _type attribute to enable oversampling in the first
place. And other than requiring reading the documentation it would be
pretty hard to guess that writing le:s30/32>>2 is what you need to do
to enable oversampling.

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[Jonathan Cameron]

On Tue,  7 May 2024 14:02:07 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This adds new fields to the iio_channel structure to support multiple
> scan types per channel. This is useful for devices that support multiple
> resolution modes or other modes that require different data formats of
> the raw data.
> 
> To make use of this, drivers can still use the old scan_type field for
> the "default" scan type and use the new scan_type_ext field for any
> additional scan types.

Comment inline says that you should commit scan_type if scan_type_ext
is provided.  That makes sense to me rather that a default no one reads.

The example that follows in patch 4 uses both the scan_type and
the scan_type_ext which is even more confusing.

> And they must implement the new callback
> get_current_scan_type() to return the current scan type based on the
> current state of the device.
> 
> The buffer code is the only code in the IIO core code that is using the
> scan_type field. This patch updates the buffer code to use the new
> iio_channel_validate_scan_type() function to ensure it is returning the
> correct scan type for the current state of the device when reading the
> sysfs attributes. The buffer validation code is also update to validate
> any additional scan types that are set in the scan_type_ext field. Part
> of that code is refactored to a new function to avoid duplication.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 19de573a944a..66f0b4c68f53 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -205,6 +205,9 @@ struct iio_scan_type {
>   * @scan_index:		Monotonic index to give ordering in scans when read
>   *			from a buffer.
>   * @scan_type:		struct describing the scan type
> + * @ext_scan_type:	Used in rare cases where there is more than one scan
> + *			format for a channel. When this is used, omit scan_type.

Here is the disagreement with the patch description.

> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
>   * @info_mask_separate: What information is to be exported that is specific to
>   *			this channel.
>   * @info_mask_separate_available: What availability information is to be
> @@ -256,6 +259,8 @@ struct iio_chan_spec {
>  	unsigned long		address;
>  	int			scan_index;
>  	struct iio_scan_type scan_type;
> +	const struct iio_scan_type *ext_scan_type;
> +	unsigned int		num_ext_scan_type;

Let's make it explicit that you can't do both.

	union {
		struct iio_scan_type scan_type;
		struct {
			const struct iio_scan_type *ext_scan_type;
			unsigned int num_ext_scan_type;
		};
	};
should work for that I think.

However this is I think only used for validation. If that's the case
do we care about values not in use?  Can we move the validation to
be runtime if the get_current_scan_type() callback is used.


>  	long			info_mask_separate;
>  	long			info_mask_separate_available;
>  	long			info_mask_shared_by_type;
> @@ -435,6 +440,9 @@ struct iio_trigger; /* forward declaration */
>   *			for better event identification.
>   * @validate_trigger:	function to validate the trigger when the
>   *			current trigger gets changed.
> + * @get_current_scan_type: must be implemented by drivers that use ext_scan_type
> + *			in the channel spec to return the currently active scan
> + *			type based on the current state of the device.
>   * @update_scan_mode:	function to configure device and scan buffer when
>   *			channels have changed
>   * @debugfs_reg_access:	function to read or write register value of device
> @@ -519,6 +527,9 @@ struct iio_info {
>  
>  	int (*validate_trigger)(struct iio_dev *indio_dev,
>  				struct iio_trigger *trig);
> +	const struct iio_scan_type *(*get_current_scan_type)(
> +					const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan);
>  	int (*update_scan_mode)(struct iio_dev *indio_dev,
>  				const unsigned long *scan_mask);
>  	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
> @@ -804,6 +815,28 @@ static inline bool iio_read_acpi_mount_matrix(struct device *dev,
>  }
>  #endif
>  
> +/**
> + * iio_get_current_scan_type - Get the current scan type for a channel
> + * @indio_dev:	the IIO device to get the scan type for
> + * @chan:	the channel to get the scan type for
> + *
> + * Most devices only have one scan type per channel and can just access it
> + * directly without calling this function. Core IIO code and drivers that
> + * implement ext_scan_type in the channel spec should use this function to
> + * get the current scan type for a channel.
> + *
> + * Returns: the current scan type for the channel
> + */
> +static inline const struct iio_scan_type *iio_get_current_scan_type(
> +					const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	if (indio_dev->info->get_current_scan_type)
> +		return indio_dev->info->get_current_scan_type(indio_dev, chan);
> +
> +	return &chan->scan_type;
> +}
> +
>  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>  
>  int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
> 

Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[Jonathan Cameron]

On Tue,  7 May 2024 14:02:08 -0500
David Lechner <dlechner@baylibre.com> wrote:

> The AD783x chips have a resolution boost feature that allows for 2
> extra bits of resolution. Previously, we had to choose a scan type to
> fit the largest resolution and manipulate the raw data to fit when the
> resolution was lower. This patch adds support for multiple scan types
> for the voltage input channels so that we can support both resolutions
> without having to manipulate the raw data.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A few comments inline.

> ---
>  drivers/iio/adc/ad7380.c | 185 ++++++++++++++++++++++-------------------------
>  1 file changed, 86 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index e240098708e9..ca317e3a72d9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -89,14 +89,22 @@ struct ad7380_chip_info {
>  	const struct ad7380_timing_specs *timing_specs;
>  };
>  
> -/*
> - * realbits/storagebits cannot be dynamically changed, so in order to
> - * support the resolution boost (additional 2  bits of resolution)
> - * we need to set realbits/storagebits to the maximum value i.e :
> - *   - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - *   - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> - * We need to adjust the scale depending on resolution boost status
> - */
> +/** scan type for 14-bit chips with resolution boost enabled. */
> +static const struct iio_scan_type ad7380_scan_type_14_boost = {
> +	.sign = 's',
> +	.realbits = 16,
> +	.storagebits = 16,
> +	.endianness = IIO_CPU,
> +};
> +
> +/** scan type for 16-bit chips with resolution boost enabled. */
Not kernel-doc. Fix all these.

> +static const struct iio_scan_type ad7380_scan_type_16_boost = {
> +	.sign = 's',
> +	.realbits = 18,
> +	.storagebits = 32,
> +	.endianness = IIO_CPU,
> +};
> +
>  #define AD7380_CHANNEL(index, bits, diff) {			\
>  	.type = IIO_VOLTAGE,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> @@ -113,10 +121,12 @@ struct ad7380_chip_info {
>  	.scan_index = (index),					\
>  	.scan_type = {						\
>  		.sign = 's',					\
> -		.realbits = (bits) + 2,				\
> -		.storagebits = ((bits) + 2 > 16) ? 32 : 16,	\
> +		.realbits = (bits),				\
> +		.storagebits = ((bits) > 16) ? 32 : 16,		\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.ext_scan_type = &ad7380_scan_type_##bits##_boost,	\
> +	.num_ext_scan_type = 1,					\
>  }
>  
>  #define DEFINE_AD7380_2_CHANNEL(name, bits, diff)	\
> @@ -376,67 +386,62 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  	unreachable();
>  }
>  
> -static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer *xfer)
> +/**
This isn't kernel-doc, so /* only

> + * Reads one set of samples from the device. This is a simultaneous sampling
> + * chip, so all channels are always read at the same time.
> + *
> + * On successful return, the raw data is stored in st->scan_data.raw.
> + */
> +static int ad7380_read_one_sample(struct ad7380_state *st,
> +				  const struct iio_scan_type *scan_type)

>  
>  static irqreturn_t ad7380_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> +	const struct iio_chan_spec *chan = &indio_dev->channels[0];
> +	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> +								indio_dev, chan);

As below, pull iio_get_current_scan_type( down to the line below.


> @@ -496,18 +475,14 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long info)
>  {
> +	const struct iio_scan_type *scan_type = iio_get_current_scan_type(
> +								indio_dev, chan);

Pull the iio_get_current_scan_type( down to the next line and use one tab.

>  	struct ad7380_state *st = iio_priv(indio_dev);
> -	int realbits;
> -
> -	if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> -		realbits = chan->scan_type.realbits;
> -	else
> -		realbits = chan->scan_type.realbits - 2;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
>  		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			return ad7380_read_direct(st, chan, val);
> +			return ad7380_read_direct(st, chan, scan_type, val);
>  		}
>  		unreachable();
>  	case IIO_CHAN_INFO_SCALE:
> @@ -520,7 +495,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		 * According to IIO ABI, offset is applied before scale,
>  		 * so offset is: vcm_mv / scale
>  		 */
> -		*val = st->vcm_mv[chan->channel] * (1 << realbits)
> +		*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
>  			/ st->vref_mv;
>  
>  		return IIO_VAL_INT;
> @@ -700,6 +675,17 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static const struct iio_scan_type *ad7380_get_current_scan_type(
> +		const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	if (st->resolution_boost_enable && chan->num_ext_scan_type)

I'd put all the scan types in ext_scan_type, then pick rather than falling back
to the main scan_type.

> +		return chan->ext_scan_type;
> +
> +	return &chan->scan_type;
> +}
> +

> 

Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[Jonathan Cameron]

On Wed, 8 May 2024 12:21:09 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Wed, May 8, 2024 at 6:40 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Tue,  7 May 2024 14:02:08 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >  
> > > The AD783x chips have a resolution boost feature that allows for 2
> > > extra bits of resolution. Previously, we had to choose a scan type to
> > > fit the largest resolution and manipulate the raw data to fit when the
> > > resolution was lower. This patch adds support for multiple scan types
> > > for the voltage input channels so that we can support both resolutions
> > > without having to manipulate the raw data.
> > >
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> >
> > I'm wondering about the control mechanism.  I was thinking we'd poke
> > the scan type directly but this may well make more sense.
> >
> > This is relying on _scale change to trigger the change in the scan type.
> > That may well be sufficient and I've been over thinking this for far too many
> > years :)
> >
> > It will get messy though in some cases as the device might have a PGA on the
> > front end so we will have a trade off between actual scaling control and
> > resolution related scale changes. We've had a few device where the scale
> > calculation is already complex and involves various different hardware
> > controls, but none have affected the storage format like this.
> >
> > I'll think some more.
> >  
> 
> Here is some more food for thought. The AD4630 family of chips we are
> working on is similar to this one in that it also has oversampling
> with increased resolution. Except in that case, they are strictly tied
> together. With oversampling disabled, we must only read 24-bits (or 16
> depending on the exact model) and when oversampling is enabled, we
> must read 32-bits (30 real bits with 2-bit shift). So in that case,
> the scan_type would depend only on oversampling ratio > 0. (Writing
> the oversampling ratio attribute would affect scale, but scale
> wouldn't be writable like on ad7380.)
> 
> It seems more intuitive to me that to enable oversampling, we would
> just write to the oversampling ratio attribute rather than having to
> write to a buffer _type attribute to enable oversampling in the first
> place. And other than requiring reading the documentation it would be
> pretty hard to guess that writing le:s30/32>>2 is what you need to do
> to enable oversampling.
> 

Ok. Few weeks thinking and I've no better ideas.  Generally I'm fine
with how you did this but I wouldn't have a 'special / default'
scan_type.  Just put them all in the array and pick between them.
That avoids fun of people trying to work out on what basis to
prefer one over another. 

So tidy the loose ends up and I'd be delighted to see a non RFC version.
It 'might' be worth waiting until we have a couple of suitable drivers
though and then show the feature works well for them all.
Whilst I think I'd take it with just one though as can see how it fits
together, but more than one driver would boost my confidence level.

Jonathan

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[David Lechner]

On 5/19/24 2:12 PM, Jonathan Cameron wrote:
> On Tue,  7 May 2024 14:02:07 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> This adds new fields to the iio_channel structure to support multiple
>> scan types per channel. This is useful for devices that support multiple
>> resolution modes or other modes that require different data formats of
>> the raw data.
>>
>> To make use of this, drivers can still use the old scan_type field for
>> the "default" scan type and use the new scan_type_ext field for any
>> additional scan types.
> 
> Comment inline says that you should commit scan_type if scan_type_ext
> is provided.  That makes sense to me rather that a default no one reads.
> 
> The example that follows in patch 4 uses both the scan_type and
> the scan_type_ext which is even more confusing.
> 
>> And they must implement the new callback
>> get_current_scan_type() to return the current scan type based on the
>> current state of the device.
>>
>> The buffer code is the only code in the IIO core code that is using the
>> scan_type field. This patch updates the buffer code to use the new
>> iio_channel_validate_scan_type() function to ensure it is returning the
>> correct scan type for the current state of the device when reading the
>> sysfs attributes. The buffer validation code is also update to validate
>> any additional scan types that are set in the scan_type_ext field. Part
>> of that code is refactored to a new function to avoid duplication.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
> 
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 19de573a944a..66f0b4c68f53 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -205,6 +205,9 @@ struct iio_scan_type {
>>   * @scan_index:		Monotonic index to give ordering in scans when read
>>   *			from a buffer.
>>   * @scan_type:		struct describing the scan type
>> + * @ext_scan_type:	Used in rare cases where there is more than one scan
>> + *			format for a channel. When this is used, omit scan_type.
> 
> Here is the disagreement with the patch description.
> 
>> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
>>   * @info_mask_separate: What information is to be exported that is specific to
>>   *			this channel.
>>   * @info_mask_separate_available: What availability information is to be
>> @@ -256,6 +259,8 @@ struct iio_chan_spec {
>>  	unsigned long		address;
>>  	int			scan_index;
>>  	struct iio_scan_type scan_type;
>> +	const struct iio_scan_type *ext_scan_type;
>> +	unsigned int		num_ext_scan_type;
> 
> Let's make it explicit that you can't do both.
> 
> 	union {
> 		struct iio_scan_type scan_type;
> 		struct {
> 			const struct iio_scan_type *ext_scan_type;
> 			unsigned int num_ext_scan_type;
> 		};
> 	};
> should work for that I think.
> 
> However this is I think only used for validation. If that's the case
> do we care about values not in use?  Can we move the validation to
> be runtime if the get_current_scan_type() callback is used.

I like the suggestion of the union to use one or the other. But I'm not
sure I understand the comments about validation.

If you are referring to iio_channel_validate_scan_type(), it only checks
for programmer error of realbits > storagebits, so it seems better to
keep it where it is to fail as early as possible.

> 
> 
>>  	long			info_mask_separate;
>>  	long			info_mask_separate_available;
>>  	long			info_mask_shared_by_type;
>> @@ -435,6 +440,9 @@ struct iio_trigger; /* forward declaration */
>>   *			for better event identification.
>>   * @validate_trigger:	function to validate the trigger when the
>>   *			current trigger gets changed.
>> + * @get_current_scan_type: must be implemented by drivers that use ext_scan_type
>> + *			in the channel spec to return the currently active scan
>> + *			type based on the current state of the device.
>>   * @update_scan_mode:	function to configure device and scan buffer when
>>   *			channels have changed
>>   * @debugfs_reg_access:	function to read or write register value of device
>> @@ -519,6 +527,9 @@ struct iio_info {
>>  
>>  	int (*validate_trigger)(struct iio_dev *indio_dev,
>>  				struct iio_trigger *trig);
>> +	const struct iio_scan_type *(*get_current_scan_type)(
>> +					const struct iio_dev *indio_dev,
>> +					const struct iio_chan_spec *chan);
>>  	int (*update_scan_mode)(struct iio_dev *indio_dev,
>>  				const unsigned long *scan_mask);
>>  	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
>> @@ -804,6 +815,28 @@ static inline bool iio_read_acpi_mount_matrix(struct device *dev,
>>  }
>>  #endif
>>  
>> +/**
>> + * iio_get_current_scan_type - Get the current scan type for a channel
>> + * @indio_dev:	the IIO device to get the scan type for
>> + * @chan:	the channel to get the scan type for
>> + *
>> + * Most devices only have one scan type per channel and can just access it
>> + * directly without calling this function. Core IIO code and drivers that
>> + * implement ext_scan_type in the channel spec should use this function to
>> + * get the current scan type for a channel.
>> + *
>> + * Returns: the current scan type for the channel
>> + */
>> +static inline const struct iio_scan_type *iio_get_current_scan_type(
>> +					const struct iio_dev *indio_dev,
>> +					const struct iio_chan_spec *chan)
>> +{
>> +	if (indio_dev->info->get_current_scan_type)
>> +		return indio_dev->info->get_current_scan_type(indio_dev, chan);
>> +
>> +	return &chan->scan_type;
>> +}
>> +
>>  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>>  
>>  int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
>>
> 

Re: [PATCH RFC 4/4] iio: adc: ad7380: add support for multiple scan type

[David Lechner]

On 5/19/24 2:20 PM, Jonathan Cameron wrote:
> On Wed, 8 May 2024 12:21:09 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On Wed, May 8, 2024 at 6:40 AM Jonathan Cameron
>> <Jonathan.Cameron@huawei.com> wrote:
>>>
>>> On Tue,  7 May 2024 14:02:08 -0500
>>> David Lechner <dlechner@baylibre.com> wrote:
>>>  
>>>> The AD783x chips have a resolution boost feature that allows for 2
>>>> extra bits of resolution. Previously, we had to choose a scan type to
>>>> fit the largest resolution and manipulate the raw data to fit when the
>>>> resolution was lower. This patch adds support for multiple scan types
>>>> for the voltage input channels so that we can support both resolutions
>>>> without having to manipulate the raw data.
>>>>
>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>  
>>>
>>> I'm wondering about the control mechanism.  I was thinking we'd poke
>>> the scan type directly but this may well make more sense.
>>>
>>> This is relying on _scale change to trigger the change in the scan type.
>>> That may well be sufficient and I've been over thinking this for far too many
>>> years :)
>>>
>>> It will get messy though in some cases as the device might have a PGA on the
>>> front end so we will have a trade off between actual scaling control and
>>> resolution related scale changes. We've had a few device where the scale
>>> calculation is already complex and involves various different hardware
>>> controls, but none have affected the storage format like this.
>>>
>>> I'll think some more.
>>>  
>>
>> Here is some more food for thought. The AD4630 family of chips we are
>> working on is similar to this one in that it also has oversampling
>> with increased resolution. Except in that case, they are strictly tied
>> together. With oversampling disabled, we must only read 24-bits (or 16
>> depending on the exact model) and when oversampling is enabled, we
>> must read 32-bits (30 real bits with 2-bit shift). So in that case,
>> the scan_type would depend only on oversampling ratio > 0. (Writing
>> the oversampling ratio attribute would affect scale, but scale
>> wouldn't be writable like on ad7380.)
>>
>> It seems more intuitive to me that to enable oversampling, we would
>> just write to the oversampling ratio attribute rather than having to
>> write to a buffer _type attribute to enable oversampling in the first
>> place. And other than requiring reading the documentation it would be
>> pretty hard to guess that writing le:s30/32>>2 is what you need to do
>> to enable oversampling.
>>
> 
> Ok. Few weeks thinking and I've no better ideas.  Generally I'm fine
> with how you did this but I wouldn't have a 'special / default'
> scan_type.  Just put them all in the array and pick between them.
> That avoids fun of people trying to work out on what basis to
> prefer one over another. 
> 
> So tidy the loose ends up and I'd be delighted to see a non RFC version.
> It 'might' be worth waiting until we have a couple of suitable drivers
> though and then show the feature works well for them all.
> Whilst I think I'd take it with just one though as can see how it fits
> together, but more than one driver would boost my confidence level.
> 
> Jonathan

Great! I'll do a v2 with only the core code. Julian, Esteban and I
are all working on drivers that should make use of this. So if all goes
well, we should have 3 drivers for you this release cycle.

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[Jonathan Cameron]

On Mon, 20 May 2024 08:51:52 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/19/24 2:12 PM, Jonathan Cameron wrote:
> > On Tue,  7 May 2024 14:02:07 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> This adds new fields to the iio_channel structure to support multiple
> >> scan types per channel. This is useful for devices that support multiple
> >> resolution modes or other modes that require different data formats of
> >> the raw data.
> >>
> >> To make use of this, drivers can still use the old scan_type field for
> >> the "default" scan type and use the new scan_type_ext field for any
> >> additional scan types.  
> > 
> > Comment inline says that you should commit scan_type if scan_type_ext
> > is provided.  That makes sense to me rather that a default no one reads.
> > 
> > The example that follows in patch 4 uses both the scan_type and
> > the scan_type_ext which is even more confusing.
> >   
> >> And they must implement the new callback
> >> get_current_scan_type() to return the current scan type based on the
> >> current state of the device.
> >>
> >> The buffer code is the only code in the IIO core code that is using the
> >> scan_type field. This patch updates the buffer code to use the new
> >> iio_channel_validate_scan_type() function to ensure it is returning the
> >> correct scan type for the current state of the device when reading the
> >> sysfs attributes. The buffer validation code is also update to validate
> >> any additional scan types that are set in the scan_type_ext field. Part
> >> of that code is refactored to a new function to avoid duplication.
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >> ---  
> >   
> >> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> >> index 19de573a944a..66f0b4c68f53 100644
> >> --- a/include/linux/iio/iio.h
> >> +++ b/include/linux/iio/iio.h
> >> @@ -205,6 +205,9 @@ struct iio_scan_type {
> >>   * @scan_index:		Monotonic index to give ordering in scans when read
> >>   *			from a buffer.
> >>   * @scan_type:		struct describing the scan type
> >> + * @ext_scan_type:	Used in rare cases where there is more than one scan
> >> + *			format for a channel. When this is used, omit scan_type.  
> > 
> > Here is the disagreement with the patch description.
> >   
> >> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
> >>   * @info_mask_separate: What information is to be exported that is specific to
> >>   *			this channel.
> >>   * @info_mask_separate_available: What availability information is to be
> >> @@ -256,6 +259,8 @@ struct iio_chan_spec {
> >>  	unsigned long		address;
> >>  	int			scan_index;
> >>  	struct iio_scan_type scan_type;
> >> +	const struct iio_scan_type *ext_scan_type;
> >> +	unsigned int		num_ext_scan_type;  
> > 
> > Let's make it explicit that you can't do both.
> > 
> > 	union {
> > 		struct iio_scan_type scan_type;
> > 		struct {
> > 			const struct iio_scan_type *ext_scan_type;
> > 			unsigned int num_ext_scan_type;
> > 		};
> > 	};
> > should work for that I think.
> > 
> > However this is I think only used for validation. If that's the case
> > do we care about values not in use?  Can we move the validation to
> > be runtime if the get_current_scan_type() callback is used.  
> 
> I like the suggestion of the union to use one or the other. But I'm not
> sure I understand the comments about validation.
> 
> If you are referring to iio_channel_validate_scan_type(), it only checks
> for programmer error of realbits > storagebits, so it seems better to
> keep it where it is to fail as early as possible.

That requires the possible scan masks to be listed here but there is
nothing enforcing the callback returning one from here.  Maybe make it
return an index instead?

> 
> > 
> >   
> >>  	long			info_mask_separate;
> >>  	long			info_mask_separate_available;
> >>  	long			info_mask_shared_by_type;
> >> @@ -435,6 +440,9 @@ struct iio_trigger; /* forward declaration */
> >>   *			for better event identification.
> >>   * @validate_trigger:	function to validate the trigger when the
> >>   *			current trigger gets changed.
> >> + * @get_current_scan_type: must be implemented by drivers that use ext_scan_type
> >> + *			in the channel spec to return the currently active scan
> >> + *			type based on the current state of the device.
> >>   * @update_scan_mode:	function to configure device and scan buffer when
> >>   *			channels have changed
> >>   * @debugfs_reg_access:	function to read or write register value of device
> >> @@ -519,6 +527,9 @@ struct iio_info {
> >>  
> >>  	int (*validate_trigger)(struct iio_dev *indio_dev,
> >>  				struct iio_trigger *trig);
> >> +	const struct iio_scan_type *(*get_current_scan_type)(
> >> +					const struct iio_dev *indio_dev,
> >> +					const struct iio_chan_spec *chan);
> >>  	int (*update_scan_mode)(struct iio_dev *indio_dev,
> >>  				const unsigned long *scan_mask);
> >>  	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
> >> @@ -804,6 +815,28 @@ static inline bool iio_read_acpi_mount_matrix(struct device *dev,
> >>  }
> >>  #endif
> >>  
> >> +/**
> >> + * iio_get_current_scan_type - Get the current scan type for a channel
> >> + * @indio_dev:	the IIO device to get the scan type for
> >> + * @chan:	the channel to get the scan type for
> >> + *
> >> + * Most devices only have one scan type per channel and can just access it
> >> + * directly without calling this function. Core IIO code and drivers that
> >> + * implement ext_scan_type in the channel spec should use this function to
> >> + * get the current scan type for a channel.
> >> + *
> >> + * Returns: the current scan type for the channel
> >> + */
> >> +static inline const struct iio_scan_type *iio_get_current_scan_type(
> >> +					const struct iio_dev *indio_dev,
> >> +					const struct iio_chan_spec *chan)
> >> +{
> >> +	if (indio_dev->info->get_current_scan_type)
> >> +		return indio_dev->info->get_current_scan_type(indio_dev, chan);
> >> +
> >> +	return &chan->scan_type;
> >> +}
> >> +
> >>  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
> >>  
> >>  int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
> >>  
> >   
> 
> 

Re: [PATCH RFC 0/4] iio: add support for multiple scan types

[Nuno Sá]

On Tue, 2024-05-07 at 14:02 -0500, David Lechner wrote:
> Following up from this thread [1]...
> 
> Unless I've overlooked something important, I think adding support for
> multiple scan types per channels should be rather trivial, at least in
> the kernel. Userspace tools will need to learn to re-read buffer _type
> attributes though. For example, it looks like libiio caches these values.
> I had to restart iiod to get a proper capture with the iio-oscilloscope
> after changing the scan type at runtime.

No for now but to add more future fun, we may consider in having something
similar as hwmon [1]. Hence, userspace could do things like poll(2) on the
specific file rather than having to read it over and over...

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/hwmon/hwmon.c#L649
- Nuno Sá

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[David Lechner]

On 5/20/24 11:12 AM, Jonathan Cameron wrote:
> On Mon, 20 May 2024 08:51:52 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 5/19/24 2:12 PM, Jonathan Cameron wrote:
>>> On Tue,  7 May 2024 14:02:07 -0500
>>> David Lechner <dlechner@baylibre.com> wrote:
>>>   
>>>> This adds new fields to the iio_channel structure to support multiple
>>>> scan types per channel. This is useful for devices that support multiple
>>>> resolution modes or other modes that require different data formats of
>>>> the raw data.
>>>>
>>>> To make use of this, drivers can still use the old scan_type field for
>>>> the "default" scan type and use the new scan_type_ext field for any
>>>> additional scan types.  
>>>
>>> Comment inline says that you should commit scan_type if scan_type_ext
>>> is provided.  That makes sense to me rather that a default no one reads.
>>>
>>> The example that follows in patch 4 uses both the scan_type and
>>> the scan_type_ext which is even more confusing.
>>>   
>>>> And they must implement the new callback
>>>> get_current_scan_type() to return the current scan type based on the
>>>> current state of the device.
>>>>
>>>> The buffer code is the only code in the IIO core code that is using the
>>>> scan_type field. This patch updates the buffer code to use the new
>>>> iio_channel_validate_scan_type() function to ensure it is returning the
>>>> correct scan type for the current state of the device when reading the
>>>> sysfs attributes. The buffer validation code is also update to validate
>>>> any additional scan types that are set in the scan_type_ext field. Part
>>>> of that code is refactored to a new function to avoid duplication.
>>>>
>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>>> ---  
>>>   
>>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>>> index 19de573a944a..66f0b4c68f53 100644
>>>> --- a/include/linux/iio/iio.h
>>>> +++ b/include/linux/iio/iio.h
>>>> @@ -205,6 +205,9 @@ struct iio_scan_type {
>>>>   * @scan_index:		Monotonic index to give ordering in scans when read
>>>>   *			from a buffer.
>>>>   * @scan_type:		struct describing the scan type
>>>> + * @ext_scan_type:	Used in rare cases where there is more than one scan
>>>> + *			format for a channel. When this is used, omit scan_type.  
>>>
>>> Here is the disagreement with the patch description.
>>>   
>>>> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
>>>>   * @info_mask_separate: What information is to be exported that is specific to
>>>>   *			this channel.
>>>>   * @info_mask_separate_available: What availability information is to be
>>>> @@ -256,6 +259,8 @@ struct iio_chan_spec {
>>>>  	unsigned long		address;
>>>>  	int			scan_index;
>>>>  	struct iio_scan_type scan_type;
>>>> +	const struct iio_scan_type *ext_scan_type;
>>>> +	unsigned int		num_ext_scan_type;  
>>>
>>> Let's make it explicit that you can't do both.
>>>
>>> 	union {
>>> 		struct iio_scan_type scan_type;
>>> 		struct {
>>> 			const struct iio_scan_type *ext_scan_type;
>>> 			unsigned int num_ext_scan_type;
>>> 		};
>>> 	};
>>> should work for that I think.
>>>
>>> However this is I think only used for validation. If that's the case
>>> do we care about values not in use?  Can we move the validation to
>>> be runtime if the get_current_scan_type() callback is used.  
>>
>> I like the suggestion of the union to use one or the other. But I'm not
>> sure I understand the comments about validation.
>>
>> If you are referring to iio_channel_validate_scan_type(), it only checks
>> for programmer error of realbits > storagebits, so it seems better to
>> keep it where it is to fail as early as possible.
> 
> That requires the possible scan masks to be listed here but there is
> nothing enforcing the callback returning one from here.  Maybe make it
> return an index instead?
> 

Sorry, still not understanding what we are trying to catch here. Why
would the scan mask have any effect of checking if realbits > storagebits?

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[Jonathan Cameron]

On Fri, 24 May 2024 10:56:55 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/20/24 11:12 AM, Jonathan Cameron wrote:
> > On Mon, 20 May 2024 08:51:52 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 5/19/24 2:12 PM, Jonathan Cameron wrote:  
> >>> On Tue,  7 May 2024 14:02:07 -0500
> >>> David Lechner <dlechner@baylibre.com> wrote:
> >>>     
> >>>> This adds new fields to the iio_channel structure to support multiple
> >>>> scan types per channel. This is useful for devices that support multiple
> >>>> resolution modes or other modes that require different data formats of
> >>>> the raw data.
> >>>>
> >>>> To make use of this, drivers can still use the old scan_type field for
> >>>> the "default" scan type and use the new scan_type_ext field for any
> >>>> additional scan types.    
> >>>
> >>> Comment inline says that you should commit scan_type if scan_type_ext
> >>> is provided.  That makes sense to me rather that a default no one reads.
> >>>
> >>> The example that follows in patch 4 uses both the scan_type and
> >>> the scan_type_ext which is even more confusing.
> >>>     
> >>>> And they must implement the new callback
> >>>> get_current_scan_type() to return the current scan type based on the
> >>>> current state of the device.
> >>>>
> >>>> The buffer code is the only code in the IIO core code that is using the
> >>>> scan_type field. This patch updates the buffer code to use the new
> >>>> iio_channel_validate_scan_type() function to ensure it is returning the
> >>>> correct scan type for the current state of the device when reading the
> >>>> sysfs attributes. The buffer validation code is also update to validate
> >>>> any additional scan types that are set in the scan_type_ext field. Part
> >>>> of that code is refactored to a new function to avoid duplication.
> >>>>
> >>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >>>> ---    
> >>>     
> >>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> >>>> index 19de573a944a..66f0b4c68f53 100644
> >>>> --- a/include/linux/iio/iio.h
> >>>> +++ b/include/linux/iio/iio.h
> >>>> @@ -205,6 +205,9 @@ struct iio_scan_type {
> >>>>   * @scan_index:		Monotonic index to give ordering in scans when read
> >>>>   *			from a buffer.
> >>>>   * @scan_type:		struct describing the scan type
> >>>> + * @ext_scan_type:	Used in rare cases where there is more than one scan
> >>>> + *			format for a channel. When this is used, omit scan_type.    
> >>>
> >>> Here is the disagreement with the patch description.
> >>>     
> >>>> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
> >>>>   * @info_mask_separate: What information is to be exported that is specific to
> >>>>   *			this channel.
> >>>>   * @info_mask_separate_available: What availability information is to be
> >>>> @@ -256,6 +259,8 @@ struct iio_chan_spec {
> >>>>  	unsigned long		address;
> >>>>  	int			scan_index;
> >>>>  	struct iio_scan_type scan_type;
> >>>> +	const struct iio_scan_type *ext_scan_type;
> >>>> +	unsigned int		num_ext_scan_type;    
> >>>
> >>> Let's make it explicit that you can't do both.
> >>>
> >>> 	union {
> >>> 		struct iio_scan_type scan_type;
> >>> 		struct {
> >>> 			const struct iio_scan_type *ext_scan_type;
> >>> 			unsigned int num_ext_scan_type;
> >>> 		};
> >>> 	};
> >>> should work for that I think.
> >>>
> >>> However this is I think only used for validation. If that's the case
> >>> do we care about values not in use?  Can we move the validation to
> >>> be runtime if the get_current_scan_type() callback is used.    
> >>
> >> I like the suggestion of the union to use one or the other. But I'm not
> >> sure I understand the comments about validation.
> >>
> >> If you are referring to iio_channel_validate_scan_type(), it only checks
> >> for programmer error of realbits > storagebits, so it seems better to
> >> keep it where it is to fail as early as possible.  
> > 
> > That requires the possible scan masks to be listed here but there is
> > nothing enforcing the callback returning one from here.  Maybe make it
> > return an index instead?
> >   
> 
> Sorry, still not understanding what we are trying to catch here. Why
> would the scan mask have any effect of checking if realbits > storagebits?
Hmm. I seem to be failing to explain this!  Key is the complete lack of
association between what is returned by the get_current_scan_type() callback
and this ext_scan_type array.

So either:
1) Make it do so - easiest being to return an index into the array rather than
   a possibly unrelated scan_type - that would guarantee the scan_type returned
   by the callback was one that has been validated.
or
2) Drop validation at initial probe because you are validating something
   that is irrelevant to what actually gets returned later. Validate
   when the scan type is read back via get_current_scan_type()

I prefer option 1.
> 

Re: [PATCH RFC 0/4] iio: add support for multiple scan types

[Jonathan Cameron]

On Tue, 21 May 2024 11:18:24 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2024-05-07 at 14:02 -0500, David Lechner wrote:
> > Following up from this thread [1]...
> > 
> > Unless I've overlooked something important, I think adding support for
> > multiple scan types per channels should be rather trivial, at least in
> > the kernel. Userspace tools will need to learn to re-read buffer _type
> > attributes though. For example, it looks like libiio caches these values.
> > I had to restart iiod to get a proper capture with the iio-oscilloscope
> > after changing the scan type at runtime.  
> 
> No for now but to add more future fun, we may consider in having something
> similar as hwmon [1]. Hence, userspace could do things like poll(2) on the
> specific file rather than having to read it over and over...
> 
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/hwmon/hwmon.c#L649
> - Nuno Sá
> 

It would take a well reasoned usecase to convince me sysfs notifications
are useful in cases where an explicit userspace action caused the value that
would be read from another file to change immediately.

Jonathan

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[David Lechner]

On 5/25/24 11:14 AM, Jonathan Cameron wrote:
> On Fri, 24 May 2024 10:56:55 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 5/20/24 11:12 AM, Jonathan Cameron wrote:
>>> On Mon, 20 May 2024 08:51:52 -0500
>>> David Lechner <dlechner@baylibre.com> wrote:
>>>   
>>>> On 5/19/24 2:12 PM, Jonathan Cameron wrote:  
>>>>> On Tue,  7 May 2024 14:02:07 -0500
>>>>> David Lechner <dlechner@baylibre.com> wrote:
>>>>>     
>>>>>> This adds new fields to the iio_channel structure to support multiple
>>>>>> scan types per channel. This is useful for devices that support multiple
>>>>>> resolution modes or other modes that require different data formats of
>>>>>> the raw data.
>>>>>>
>>>>>> To make use of this, drivers can still use the old scan_type field for
>>>>>> the "default" scan type and use the new scan_type_ext field for any
>>>>>> additional scan types.    
>>>>>
>>>>> Comment inline says that you should commit scan_type if scan_type_ext
>>>>> is provided.  That makes sense to me rather that a default no one reads.
>>>>>
>>>>> The example that follows in patch 4 uses both the scan_type and
>>>>> the scan_type_ext which is even more confusing.
>>>>>     
>>>>>> And they must implement the new callback
>>>>>> get_current_scan_type() to return the current scan type based on the
>>>>>> current state of the device.
>>>>>>
>>>>>> The buffer code is the only code in the IIO core code that is using the
>>>>>> scan_type field. This patch updates the buffer code to use the new
>>>>>> iio_channel_validate_scan_type() function to ensure it is returning the
>>>>>> correct scan type for the current state of the device when reading the
>>>>>> sysfs attributes. The buffer validation code is also update to validate
>>>>>> any additional scan types that are set in the scan_type_ext field. Part
>>>>>> of that code is refactored to a new function to avoid duplication.
>>>>>>
>>>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>>>>> ---    
>>>>>     
>>>>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>>>>> index 19de573a944a..66f0b4c68f53 100644
>>>>>> --- a/include/linux/iio/iio.h
>>>>>> +++ b/include/linux/iio/iio.h
>>>>>> @@ -205,6 +205,9 @@ struct iio_scan_type {
>>>>>>   * @scan_index:		Monotonic index to give ordering in scans when read
>>>>>>   *			from a buffer.
>>>>>>   * @scan_type:		struct describing the scan type
>>>>>> + * @ext_scan_type:	Used in rare cases where there is more than one scan
>>>>>> + *			format for a channel. When this is used, omit scan_type.    
>>>>>
>>>>> Here is the disagreement with the patch description.
>>>>>     
>>>>>> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
>>>>>>   * @info_mask_separate: What information is to be exported that is specific to
>>>>>>   *			this channel.
>>>>>>   * @info_mask_separate_available: What availability information is to be
>>>>>> @@ -256,6 +259,8 @@ struct iio_chan_spec {
>>>>>>  	unsigned long		address;
>>>>>>  	int			scan_index;
>>>>>>  	struct iio_scan_type scan_type;
>>>>>> +	const struct iio_scan_type *ext_scan_type;
>>>>>> +	unsigned int		num_ext_scan_type;    
>>>>>
>>>>> Let's make it explicit that you can't do both.
>>>>>
>>>>> 	union {
>>>>> 		struct iio_scan_type scan_type;
>>>>> 		struct {
>>>>> 			const struct iio_scan_type *ext_scan_type;
>>>>> 			unsigned int num_ext_scan_type;
>>>>> 		};
>>>>> 	};
>>>>> should work for that I think.
>>>>>
>>>>> However this is I think only used for validation. If that's the case
>>>>> do we care about values not in use?  Can we move the validation to
>>>>> be runtime if the get_current_scan_type() callback is used.    
>>>>
>>>> I like the suggestion of the union to use one or the other. But I'm not
>>>> sure I understand the comments about validation.
>>>>
>>>> If you are referring to iio_channel_validate_scan_type(), it only checks
>>>> for programmer error of realbits > storagebits, so it seems better to
>>>> keep it where it is to fail as early as possible.  
>>>
>>> That requires the possible scan masks to be listed here but there is
>>> nothing enforcing the callback returning one from here.  Maybe make it
>>> return an index instead?
>>>   
>>
>> Sorry, still not understanding what we are trying to catch here. Why
>> would the scan mask have any effect of checking if realbits > storagebits?
> Hmm. I seem to be failing to explain this!  

Maybe we are talking about two different things but calling them the same thing?

> Key is the complete lack of
> association between what is returned by the get_current_scan_type() callback
> and this ext_scan_type array.

Why would the caller of get_current_scan_type() need to know that the
returned value is associated with ext_scan_type?

> 
> So either:
> 1) Make it do so - easiest being to return an index into the array rather than
>    a possibly unrelated scan_type -

Unrelated to what?

> that would guarantee the scan_type returned
>    by the callback was one that has been validated.

Since all scan types are const data and not changed after the iio device
is registered, the validation done at registration seems sufficient to
me (validation happens in __iio_buffer_alloc_sysfs_and_mask()). All scan
types are validated at this time, including all ext_scan_types. So all
are guaranteed to be validated already when the get_current_scan_type
callback is called.

What other validation would need to be done later?

> or
> 2) Drop validation at initial probe because you are validating something
>    that is irrelevant to what actually gets returned later. Validate>    when the scan type is read back via get_current_scan_type()

The validation is just checking for programmer error, so it seems better
to catch that at probe where we are guaranteed to catch it for all scan
types. If the driver fails to probe, the programmer should notice this and
fix their mistake, but if we don't validate until later, the programmer
might not check every single configuration every time a change is made.

> 
> I prefer option 1.
>>
> 

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[Jonathan Cameron]

On Sat, 25 May 2024 12:04:46 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/25/24 11:14 AM, Jonathan Cameron wrote:
> > On Fri, 24 May 2024 10:56:55 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 5/20/24 11:12 AM, Jonathan Cameron wrote:  
> >>> On Mon, 20 May 2024 08:51:52 -0500
> >>> David Lechner <dlechner@baylibre.com> wrote:
> >>>     
> >>>> On 5/19/24 2:12 PM, Jonathan Cameron wrote:    
> >>>>> On Tue,  7 May 2024 14:02:07 -0500
> >>>>> David Lechner <dlechner@baylibre.com> wrote:
> >>>>>       
> >>>>>> This adds new fields to the iio_channel structure to support multiple
> >>>>>> scan types per channel. This is useful for devices that support multiple
> >>>>>> resolution modes or other modes that require different data formats of
> >>>>>> the raw data.
> >>>>>>
> >>>>>> To make use of this, drivers can still use the old scan_type field for
> >>>>>> the "default" scan type and use the new scan_type_ext field for any
> >>>>>> additional scan types.      
> >>>>>
> >>>>> Comment inline says that you should commit scan_type if scan_type_ext
> >>>>> is provided.  That makes sense to me rather that a default no one reads.
> >>>>>
> >>>>> The example that follows in patch 4 uses both the scan_type and
> >>>>> the scan_type_ext which is even more confusing.
> >>>>>       
> >>>>>> And they must implement the new callback
> >>>>>> get_current_scan_type() to return the current scan type based on the
> >>>>>> current state of the device.
> >>>>>>
> >>>>>> The buffer code is the only code in the IIO core code that is using the
> >>>>>> scan_type field. This patch updates the buffer code to use the new
> >>>>>> iio_channel_validate_scan_type() function to ensure it is returning the
> >>>>>> correct scan type for the current state of the device when reading the
> >>>>>> sysfs attributes. The buffer validation code is also update to validate
> >>>>>> any additional scan types that are set in the scan_type_ext field. Part
> >>>>>> of that code is refactored to a new function to avoid duplication.
> >>>>>>
> >>>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >>>>>> ---      
> >>>>>       
> >>>>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> >>>>>> index 19de573a944a..66f0b4c68f53 100644
> >>>>>> --- a/include/linux/iio/iio.h
> >>>>>> +++ b/include/linux/iio/iio.h
> >>>>>> @@ -205,6 +205,9 @@ struct iio_scan_type {
> >>>>>>   * @scan_index:		Monotonic index to give ordering in scans when read
> >>>>>>   *			from a buffer.
> >>>>>>   * @scan_type:		struct describing the scan type
> >>>>>> + * @ext_scan_type:	Used in rare cases where there is more than one scan
> >>>>>> + *			format for a channel. When this is used, omit scan_type.      
> >>>>>
> >>>>> Here is the disagreement with the patch description.
> >>>>>       
> >>>>>> + * @num_ext_scan_type:	Number of elements in ext_scan_type.
> >>>>>>   * @info_mask_separate: What information is to be exported that is specific to
> >>>>>>   *			this channel.
> >>>>>>   * @info_mask_separate_available: What availability information is to be
> >>>>>> @@ -256,6 +259,8 @@ struct iio_chan_spec {
> >>>>>>  	unsigned long		address;
> >>>>>>  	int			scan_index;
> >>>>>>  	struct iio_scan_type scan_type;
> >>>>>> +	const struct iio_scan_type *ext_scan_type;
> >>>>>> +	unsigned int		num_ext_scan_type;      
> >>>>>
> >>>>> Let's make it explicit that you can't do both.
> >>>>>
> >>>>> 	union {
> >>>>> 		struct iio_scan_type scan_type;
> >>>>> 		struct {
> >>>>> 			const struct iio_scan_type *ext_scan_type;
> >>>>> 			unsigned int num_ext_scan_type;
> >>>>> 		};
> >>>>> 	};
> >>>>> should work for that I think.
> >>>>>
> >>>>> However this is I think only used for validation. If that's the case
> >>>>> do we care about values not in use?  Can we move the validation to
> >>>>> be runtime if the get_current_scan_type() callback is used.      
> >>>>
> >>>> I like the suggestion of the union to use one or the other. But I'm not
> >>>> sure I understand the comments about validation.
> >>>>
> >>>> If you are referring to iio_channel_validate_scan_type(), it only checks
> >>>> for programmer error of realbits > storagebits, so it seems better to
> >>>> keep it where it is to fail as early as possible.    
> >>>
> >>> That requires the possible scan masks to be listed here but there is
> >>> nothing enforcing the callback returning one from here.  Maybe make it
> >>> return an index instead?
> >>>     
> >>
> >> Sorry, still not understanding what we are trying to catch here. Why
> >> would the scan mask have any effect of checking if realbits > storagebits?  
> > Hmm. I seem to be failing to explain this!    
> 
> Maybe we are talking about two different things but calling them the same thing?

I'm not sure.  Sounds like we both think our point is entirely obvious and clearly
it isn't :(

> > Key is the complete lack of
> > association between what is returned by the get_current_scan_type() callback
> > and this ext_scan_type array.  
> 
> Why would the caller of get_current_scan_type() need to know that the
> returned value is associated with ext_scan_type?

Because you are validating ext_scan_type, not the return of get_current_scan_type().
They may or may not include the same data - to make this a good interface, that isn't
error prone, get_current_scan_type() must return one that has been validated - i.e.
is in the ext_scan_type array.

I've looked several times and maybe I'm just failing to spot what ensures the validation
is sufficient.

> 
> > 
> > So either:
> > 1) Make it do so - easiest being to return an index into the array rather than
> >    a possibly unrelated scan_type -  
> 
> Unrelated to what?

Unrelated to anything the IIO core is currently aware of. You could have a list
of types of cats that you've validated for feline characteristics
and this callback returns a donkey.

> 
> > that would guarantee the scan_type returned
> >    by the callback was one that has been validated.  
> 
> Since all scan types are const data and not changed after the iio device
> is registered, the validation done at registration seems sufficient to
> me (validation happens in __iio_buffer_alloc_sysfs_and_mask()). All scan
> types are validated at this time, including all ext_scan_types. So all
> are guaranteed to be validated already when the get_current_scan_type
> callback is called.
> 
> What other validation would need to be done later?

What makes get_current_scan_type() return a scan type that is in the ext_scan_type
array?

A possible implementation (which should not be possible!) is

static const struct iio_scan_type scan_type_A = {
	.sign = 's',
	.realbits = 16,
	.storagebits = 16,
	.endianness = IIO_CPU,
};

static const struct iio_scan_type scan_type_B = {
	.sign = 's',
	.realbits = 18,
	.storagebits = 16,
	.endianness = IIO_CPU,
};

.ext_scan_type = &ad7380_scan_type_A,


static const struct iio_scan_type *get_current_scan_type(
		const struct iio_dev *indio_dev, struct iio_chan_spec const *chan)
{
	//some stuff to select

	return scan_type_B; 
}

> 
> > or
> > 2) Drop validation at initial probe because you are validating something
> >    that is irrelevant to what actually gets returned later. Validate>    when the scan type is read back via get_current_scan_type()  
> 
> The validation is just checking for programmer error, so it seems better
> to catch that at probe where we are guaranteed to catch it for all scan
> types. If the driver fails to probe, the programmer should notice this and
> fix their mistake, but if we don't validate until later, the programmer
> might not check every single configuration every time a change is made.

I agreed - but today that isn't happening in the above example.
You need to enforce that the scan_type returned is one that has been validated.

Maybe I'm missing that validation occurring somewhere?

Jonathan


> 
> > 
> > I prefer option 1.  
> >>  
> >   
> 
> 

Re: [PATCH RFC 3/4] iio: add support for multiple scan types per channel

[David Lechner]

On Sun, May 26, 2024 at 7:11 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 25 May 2024 12:04:46 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On 5/25/24 11:14 AM, Jonathan Cameron wrote:
> > > On Fri, 24 May 2024 10:56:55 -0500
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >
> > >> On 5/20/24 11:12 AM, Jonathan Cameron wrote:
> > >>> On Mon, 20 May 2024 08:51:52 -0500
> > >>> David Lechner <dlechner@baylibre.com> wrote:
> > >>>
> > >>>> On 5/19/24 2:12 PM, Jonathan Cameron wrote:
> > >>>>> On Tue,  7 May 2024 14:02:07 -0500
> > >>>>> David Lechner <dlechner@baylibre.com> wrote:
> > >>>>>

...

> >
> > Maybe we are talking about two different things but calling them the same thing?
>
> I'm not sure.  Sounds like we both think our point is entirely obvious and clearly
> it isn't :(
>
> > > Key is the complete lack of
> > > association between what is returned by the get_current_scan_type() callback
> > > and this ext_scan_type array.
> >
> > Why would the caller of get_current_scan_type() need to know that the
> > returned value is associated with ext_scan_type?
>
> Because you are validating ext_scan_type, not the return of get_current_scan_type().
> They may or may not include the same data - to make this a good interface, that isn't
> error prone, get_current_scan_type() must return one that has been validated - i.e.
> is in the ext_scan_type array.
>
> I've looked several times and maybe I'm just failing to spot what ensures the validation
> is sufficient.
>

Ah, I finally get it now. I was having tunnel vision and it didn't
even occur to me that someone might be tempted to return anything that
wasn't a pointer to the ext_scan types array.

> >
> > >
> > > So either:
> > > 1) Make it do so - easiest being to return an index into the array rather than
> > >    a possibly unrelated scan_type -
> >

This option 1) makes sense to me now.

Do we also need to validate that the index returned is <
num_ext_scan_types in iio_get_current_scan_type()?