[RFC PATCH 0/3] iio: buffer: add output buffer support for chrdev

[RFC PATCH 0/3] iio: buffer: add output buffer support for chrdev

[Alexandru Ardelean]

[This description is also present on patch 3/3, there have been some
cosmetic stuff since that one that I did not remove. These patches
probably won't make it in a final version, but they're there because
I changed my mind a few times and got to this RFC]

There have been some offline discussions about how to go about this.
Since I wasn't involved in any of those discussions, this kind of tries to
re-build things from various bits.
It's incomplete work, since I don't have a clear idea of what the accepted/acceptable
approach would be.

1. First approach is: we keep 1 buffer per device, and we make it either
in/out, which means that for devices that for devices that have both in/out
2 iio_dev instances are required, or an ADC needs to be connected on the in
path and a DAC on out-path. This is predominantly done in the ADI tree.

2. One discussion/proposal was to have multiple buffers per-device. But the
details are vague since they were relayed to me.
One detail was, to have indexes for devices that have more than 1
buffer:

Iio_deviceX:
        buffer
        scan_elements

Iio_deviceX:
        BufferY
        scan_elementsY
        BufferZ
        scan_elementsZ

I am not sure how feasible this is for a single chrdev, as when you look at
the fileops that get assigned to a chrdev, it looks like it can have at
most two buffers (one for input, out for output).

Multiplexing input buffers can work (from ADCs), but demultiplexing output
buffers into a DAC, not so well. Especially on a single chrdev.

Question 1: do we want to support more than 2 buffers per chrdev?

This is what ADI currently has in it's tree (and works).

Example, ADC:
 # ls iio\:device3/buffer/
 data_available  enable  length  length_align_bytes  watermark
 #  ls iio\:device3/scan_elements/
 in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_voltage1_index  in_voltage1_type

Example, DAC:
 #  ls iio\:device4/buffer/
 data_available  enable  length  length_align_bytes  watermark
 # ls iio\:device4/scan_elements/
 out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en     out_voltage2_type  out_voltage3_index
 out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_index  out_voltage3_en    out_voltage3_type

The direction of each element is encoded into the filename of each channel.

Another question is:
 Does it make sense to have more than 1 'scan_elements' folder?
 That is, for devices that would have both in & out channels.

For 'buffer' folders I was thinking that it may make sense to have,
'buffer_in' && 'buffer_out'.

So, one idea is:

Iio_deviceX:
        buffer_in
        buffer_out
        scan_elements

Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
But the format is:

Iio_deviceX:
        buffer_in
        buffer_out
        scan_elements_in
        scan_elements_out

Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
some glimpse of where this could go.

3. A side question is about the 'iio_buffer -> pollq' field. I was
wondering if it would make sense to move that on to 'iio_dev  pollq' if
adding multiple buffers are added per-device. It almost makes sense to
unify the 'pollq' on indio_dev.
But, it looks a bit difficult, and would require some more change [which is
doable] if it makes sense for whatever reason.
The only reason to do it, is because the iio_buffer_fileops has a .poll =
iio_buffer_poll() function attached to it. Adding multiple buffers for an
IIO device may require some consideration on the iio_buffer_poll() function
as well.

Alexandru Ardelean (3):
  iio: core: rename 'indio_dev->buffer_list' ->
    'indio_dev->active_buffers'
  iio: buffer: extend short-hand use for 'indio_dev->buffer'
  iio: buffer: add output buffer support for chrdev

 .../buffer/industrialio-buffer-dmaengine.c    |   3 +-
 drivers/iio/industrialio-buffer.c             | 267 +++++++++++++-----
 drivers/iio/industrialio-core.c               |   5 +-
 include/linux/iio/buffer.h                    |   9 +
 include/linux/iio/buffer_impl.h               |   7 +
 include/linux/iio/iio.h                       |  12 +-
 6 files changed, 225 insertions(+), 78 deletions(-)

-- 
2.20.1

[RFC PATCH 1/3] iio: core: rename 'indio_dev->buffer_list' -> 'indio_dev->active_buffers'

[Alexandru Ardelean]

Since we want to add support for attaching multiple buffers, and we want to
add a new list to 'struct iio_dev', it's a good idea to rename the current
'indio->buffer_list' to 'indio_dev->active_buffers'.

Fortunately, this is a private field, which is used in
'drivers/iio/industrial-buffer.c', so this is simple to rename.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 28 ++++++++++++++--------------
 drivers/iio/industrialio-core.c   |  2 +-
 include/linux/iio/iio.h           |  4 ++--
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e6fa1a4e135d..a585c304cad4 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -591,7 +591,7 @@ static void iio_buffer_activate(struct iio_dev *indio_dev,
 	struct iio_buffer *buffer)
 {
 	iio_buffer_get(buffer);
-	list_add(&buffer->buffer_list, &indio_dev->buffer_list);
+	list_add(&buffer->buffer_list, &indio_dev->active_buffers);
 }
 
 static void iio_buffer_deactivate(struct iio_buffer *buffer)
@@ -606,7 +606,7 @@ static void iio_buffer_deactivate_all(struct iio_dev *indio_dev)
 	struct iio_buffer *buffer, *_buffer;
 
 	list_for_each_entry_safe(buffer, _buffer,
-			&indio_dev->buffer_list, buffer_list)
+			&indio_dev->active_buffers, buffer_list)
 		iio_buffer_deactivate(buffer);
 }
 
@@ -701,12 +701,12 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	 * to verify.
 	 */
 	if (remove_buffer && !insert_buffer &&
-		list_is_singular(&indio_dev->buffer_list))
+		list_is_singular(&indio_dev->active_buffers))
 			return 0;
 
 	modes = indio_dev->modes;
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
 		if (buffer == remove_buffer)
 			continue;
 		modes &= buffer->access->modes;
@@ -727,7 +727,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 		 * Keep things simple for now and only allow a single buffer to
 		 * be connected in hardware mode.
 		 */
-		if (insert_buffer && !list_empty(&indio_dev->buffer_list))
+		if (insert_buffer && !list_empty(&indio_dev->active_buffers))
 			return -EINVAL;
 		config->mode = INDIO_BUFFER_HARDWARE;
 		strict_scanmask = true;
@@ -747,7 +747,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 
 	scan_timestamp = false;
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
 		if (buffer == remove_buffer)
 			continue;
 		bitmap_or(compound_mask, compound_mask, buffer->scan_mask,
@@ -896,7 +896,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
 	struct iio_buffer *buffer;
 	int ret;
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
 		ret = iio_buffer_update_demux(indio_dev, buffer);
 		if (ret < 0)
 			goto error_clear_mux_table;
@@ -904,7 +904,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
 	return 0;
 
 error_clear_mux_table:
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list)
+	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list)
 		iio_buffer_demux_free(buffer);
 
 	return ret;
@@ -948,7 +948,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 		indio_dev->info->hwfifo_set_watermark(indio_dev,
 			config->watermark);
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
 		ret = iio_buffer_enable(buffer, indio_dev);
 		if (ret)
 			goto err_disable_buffers;
@@ -968,7 +968,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	return 0;
 
 err_disable_buffers:
-	list_for_each_entry_continue_reverse(buffer, &indio_dev->buffer_list,
+	list_for_each_entry_continue_reverse(buffer, &indio_dev->active_buffers,
 					     buffer_list)
 		iio_buffer_disable(buffer, indio_dev);
 err_run_postdisable:
@@ -988,7 +988,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 	int ret2;
 
 	/* Wind down existing buffers - iff there are any */
-	if (list_empty(&indio_dev->buffer_list))
+	if (list_empty(&indio_dev->active_buffers))
 		return 0;
 
 	/*
@@ -1004,7 +1004,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 			ret = ret2;
 	}
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
 		ret2 = iio_buffer_disable(buffer, indio_dev);
 		if (ret2 && !ret)
 			ret = ret2;
@@ -1052,7 +1052,7 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
 		iio_buffer_activate(indio_dev, insert_buffer);
 
 	/* If no buffers in list, we are done */
-	if (list_empty(&indio_dev->buffer_list))
+	if (list_empty(&indio_dev->active_buffers))
 		return 0;
 
 	ret = iio_enable_buffers(indio_dev, &new_config);
@@ -1413,7 +1413,7 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
 	int ret;
 	struct iio_buffer *buf;
 
-	list_for_each_entry(buf, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buf, &indio_dev->active_buffers, buffer_list) {
 		ret = iio_push_to_buffer(buf, data);
 		if (ret < 0)
 			return ret;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 157d95a24faa..a13957644c1d 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1523,7 +1523,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
 			return NULL;
 		}
 		dev_set_name(&dev->dev, "iio:device%d", dev->id);
-		INIT_LIST_HEAD(&dev->buffer_list);
+		INIT_LIST_HEAD(&dev->active_buffers);
 	}
 
 	return dev;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index e975020abaa6..a123f8acb192 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -490,7 +490,7 @@ struct iio_buffer_setup_ops {
  *			and owner
  * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
  * @buffer:		[DRIVER] any buffer present
- * @buffer_list:	[INTERN] list of all buffers currently attached
+ * @active_buffers:	[INTERN] list of all buffers currently enabled/active
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
  * @mlock:		[INTERN] lock used to prevent simultaneous device state
  *			changes
@@ -534,7 +534,7 @@ struct iio_dev {
 	struct iio_event_interface	*event_interface;
 
 	struct iio_buffer		*buffer;
-	struct list_head		buffer_list;
+	struct list_head		active_buffers;
 	int				scan_bytes;
 	struct mutex			mlock;
 
-- 
2.20.1

[RFC PATCH 2/3] iio: buffer: extend short-hand use for 'indio_dev->buffer'

[Alexandru Ardelean]

This change is both cosmetic and a prequel to adding support for attaching
multiple buffers per IIO device.

The IIO buffer sysfs attrs are mostly designed to support only one attached
buffer, and in order to support more, we need to centralize [in each attr
function] the buffer which is being accessed.

This also makes it a bit more uniform, as in some functions there is a
short-hand 'buffer' variable and at the same time the 'indio_dev->buffer'
is still access directly.

In the 'iio_buffer_add_channel_sysfs()' the 'buffer' is passed as a
parameter. This gives control to 'iio_buffer_alloc_sysfs_and_mask()' over
which buffer gets accessed.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 55 +++++++++++++++++--------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a585c304cad4..c6af18448dd5 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -262,10 +262,11 @@ static ssize_t iio_scan_el_show(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
 
 	/* Ensure ret is 0 or 1. */
 	ret = !!test_bit(to_iio_dev_attr(attr)->address,
-		       indio_dev->buffer->scan_mask);
+		       buffer->scan_mask);
 
 	return sprintf(buf, "%d\n", ret);
 }
@@ -381,7 +382,7 @@ static ssize_t iio_scan_el_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
 	}
@@ -410,7 +411,9 @@ static ssize_t iio_scan_el_ts_show(struct device *dev,
 				   char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	return sprintf(buf, "%d\n", indio_dev->buffer->scan_timestamp);
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	return sprintf(buf, "%d\n", buffer->scan_timestamp);
 }
 
 static ssize_t iio_scan_el_ts_store(struct device *dev,
@@ -420,6 +423,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
 	bool state;
 
 	ret = strtobool(buf, &state);
@@ -427,11 +431,11 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 		return ret;
 
 	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
 	}
-	indio_dev->buffer->scan_timestamp = state;
+	buffer->scan_timestamp = state;
 error_ret:
 	mutex_unlock(&indio_dev->mlock);
 
@@ -439,10 +443,10 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 }
 
 static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
+					struct iio_buffer *buffer,
 					const struct iio_chan_spec *chan)
 {
 	int ret, attrcount = 0;
-	struct iio_buffer *buffer = indio_dev->buffer;
 
 	ret = __iio_add_chan_devattr("index",
 				     chan,
@@ -518,7 +522,7 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 		return len;
 
 	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 	} else {
 		buffer->access->set_length(buffer, val);
@@ -539,7 +543,9 @@ static ssize_t iio_buffer_show_enable(struct device *dev,
 				      char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	return sprintf(buf, "%d\n", iio_buffer_is_active(indio_dev->buffer));
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
 }
 
 static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev,
@@ -1129,6 +1135,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	int ret;
 	bool requested_state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
 	bool inlist;
 
 	ret = strtobool(buf, &requested_state);
@@ -1138,17 +1145,15 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	mutex_lock(&indio_dev->mlock);
 
 	/* Find out if it is in the list */
-	inlist = iio_buffer_is_active(indio_dev->buffer);
+	inlist = iio_buffer_is_active(buffer);
 	/* Already in desired state */
 	if (inlist == requested_state)
 		goto done;
 
 	if (requested_state)
-		ret = __iio_update_buffers(indio_dev,
-					 indio_dev->buffer, NULL);
+		ret = __iio_update_buffers(indio_dev, buffer, NULL);
 	else
-		ret = __iio_update_buffers(indio_dev,
-					 NULL, indio_dev->buffer);
+		ret = __iio_update_buffers(indio_dev, NULL, buffer);
 
 done:
 	mutex_unlock(&indio_dev->mlock);
@@ -1190,7 +1195,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 		goto out;
 	}
 
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -1207,11 +1212,9 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
 						char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	size_t bytes;
-
-	bytes = iio_buffer_data_available(indio_dev->buffer);
+	struct iio_buffer *buffer = indio_dev->buffer;
 
-	return sprintf(buf, "%zu\n", bytes);
+	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
@@ -1297,7 +1300,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 			if (channels[i].scan_index < 0)
 				continue;
 
-			ret = iio_buffer_add_channel_sysfs(indio_dev,
+			ret = iio_buffer_add_channel_sysfs(indio_dev, buffer,
 							 &channels[i]);
 			if (ret < 0)
 				goto error_cleanup_dynamic;
@@ -1340,20 +1343,22 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	bitmap_free(buffer->scan_mask);
 error_cleanup_dynamic:
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
-	kfree(indio_dev->buffer->buffer_group.attrs);
+	kfree(buffer->buffer_group.attrs);
 
 	return ret;
 }
 
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 {
-	if (!indio_dev->buffer)
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	if (!buffer)
 		return;
 
-	bitmap_free(indio_dev->buffer->scan_mask);
-	kfree(indio_dev->buffer->buffer_group.attrs);
-	kfree(indio_dev->buffer->scan_el_group.attrs);
-	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
+	bitmap_free(buffer->scan_mask);
+	kfree(buffer->buffer_group.attrs);
+	kfree(buffer->scan_el_group.attrs);
+	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
 }
 
 /**
-- 
2.20.1

[RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Alexandru Ardelean]

This is WIP.
It hasn't been tested yet. Mostly serves as base for some discussion.

There have been some offline discussions about how to go about this.
Since I wasn't involved in any of those discussions, this kind of tries to
re-build things from various bits.

1. First approach is: we keep 1 buffer per device, and we make it either
in/out, which means that for devices that for devices that have both in/out
2 iio_dev instances are required, or an ADC needs to be connected on the in
path and a DAC on out-path. This is predominantly done in the ADI tree.

2. One discussion/proposal was to have multiple buffers per-device. But the
details are vague since they were relayed to me.
One detail was, to have indexes for devices that have more than 1
buffer:

Iio_deviceX:
        buffer
        scan_elements

Iio_deviceX:
        BufferY
        scan_elementsY
        BufferZ
        scan_elementsZ

I am not sure how feasible this is for a single chrdev, as when you look at
the fileops that get assigned to a chrdev, it looks like it can have at
most two buffers (one for input, out for output).

Multiplexing input buffers can work (from ADCs), but demultiplexing output
buffers into a DAC, not so well. Especially on a single chrdev.

Question 1: do we want to support more than 2 buffers per chrdev?

This is what ADI currently has in it's tree (and works).

Example, ADC:
 # ls iio\:device3/buffer/
 data_available  enable  length  length_align_bytes  watermark
 #  ls iio\:device3/scan_elements/
 in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_voltage1_index  in_voltage1_type

Example, DAC:
 #  ls iio\:device4/buffer/
 data_available  enable  length  length_align_bytes  watermark
 # ls iio\:device4/scan_elements/
 out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en     out_voltage2_type  out_voltage3_index
 out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_index  out_voltage3_en    out_voltage3_type

The direction of each element is encoded into the filename of each channel.

Another question is:
 Does it make sense to have more than 1 'scan_elements' folder?
 That is, for devices that would have both in & out channels.

For 'buffer' folders I was thinking that it may make sense to have,
'buffer_in' && 'buffer_out'.

So, one idea is:

Iio_deviceX:
        buffer_in
        buffer_out
        scan_elements

Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
But the format is:

Iio_deviceX:
        buffer_in
        buffer_out
        scan_elements_in
        scan_elements_out

Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
some glimpse of where this could go.

3. A side question is about the 'iio_buffer -> pollq' field. I was
wondering if it would make sense to move that on to 'iio_dev  pollq' if
adding multiple buffers are added per-device. It almost makes sense to
unify the 'pollq' on indio_dev.
But, it looks a bit difficult, and would require some more change [which is
doable] if it makes sense for whatever reason.
The only reason to do it, is because the iio_buffer_fileops has a .poll =
iio_buffer_poll() function attached to it. Adding multiple buffers for an
IIO device may require some consideration on the iio_buffer_poll() function
as well.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../buffer/industrialio-buffer-dmaengine.c    |   3 +-
 drivers/iio/industrialio-buffer.c             | 198 ++++++++++++++----
 drivers/iio/industrialio-core.c               |   3 +-
 include/linux/iio/buffer.h                    |   9 +
 include/linux/iio/buffer_impl.h               |   7 +
 include/linux/iio/iio.h                       |   8 +-
 6 files changed, 185 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 6dedf12b69a4..ae9d4c06d6b5 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -131,8 +131,9 @@ static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	/* FIXME: see about indio_dev->buffer_out */
 	struct dmaengine_buffer *dmaengine_buffer =
-		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
+		iio_buffer_to_dmaengine_buffer(indio_dev->buffer_in);
 
 	return sprintf(buf, "%zu\n", dmaengine_buffer->align);
 }
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index c6af18448dd5..79adda6fcbac 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -103,7 +103,7 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
 			      size_t n, loff_t *f_ps)
 {
 	struct iio_dev *indio_dev = filp->private_data;
-	struct iio_buffer *rb = indio_dev->buffer;
+	struct iio_buffer *rb = indio_dev->buffer_in;
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	size_t datum_size;
 	size_t to_wait;
@@ -156,6 +156,46 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
 	return ret;
 }
 
+static bool iio_buffer_space_available(struct iio_buffer *buf)
+{
+	if (buf->access->space_available)
+		return buf->access->space_available(buf);
+
+	return true;
+}
+
+ssize_t iio_buffer_chrdev_write(struct file *filp, const char __user *buf,
+				      size_t n, loff_t *f_ps)
+{
+	struct iio_dev *indio_dev = filp->private_data;
+	struct iio_buffer *wb = indio_dev->buffer_out;
+	int ret;
+
+	if (!wb || !wb->access->write)
+		return -EINVAL;
+
+	do {
+		if (!iio_buffer_space_available(wb)) {
+			if (filp->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			ret = wait_event_interruptible(wb->pollq,
+					iio_buffer_space_available(wb) ||
+					indio_dev->info == NULL);
+			if (ret)
+				return ret;
+			if (indio_dev->info == NULL)
+				return -ENODEV;
+		}
+
+		ret = wb->access->write(wb, n, buf);
+		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
+			ret = -EAGAIN;
+	} while (ret == 0);
+
+	return ret;
+}
+
 /**
  * iio_buffer_poll() - poll the buffer to find out if it has data
  * @filp:	File structure pointer for device access
@@ -169,15 +209,24 @@ __poll_t iio_buffer_poll(struct file *filp,
 			     struct poll_table_struct *wait)
 {
 	struct iio_dev *indio_dev = filp->private_data;
-	struct iio_buffer *rb = indio_dev->buffer;
+	struct iio_buffer *rb = indio_dev->buffer_in;
+	struct iio_buffer *wb = indio_dev->buffer_out;
+	__poll_t poll_flags = 0;
 
-	if (!indio_dev->info || rb == NULL)
+	if (!indio_dev->info)
 		return 0;
 
-	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
-		return EPOLLIN | EPOLLRDNORM;
-	return 0;
+	if (rb && iio_buffer_ready(indio_dev, rb, rb->watermark, 0)) {
+		poll_wait(filp, &rb->pollq, wait);
+		poll_flags |= EPOLLIN | EPOLLRDNORM;
+	}
+
+	if (wb && iio_buffer_space_available(wb)) {
+		poll_wait(filp, &wb->pollq, wait);
+		poll_flags |= EPOLLOUT | EPOLLWRNORM;
+	}
+
+	return poll_flags;
 }
 
 /**
@@ -189,10 +238,11 @@ __poll_t iio_buffer_poll(struct file *filp,
  */
 void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
 {
-	if (!indio_dev->buffer)
-		return;
+	if (indio_dev->buffer_in)
+		wake_up(&indio_dev->buffer_in->pollq);
 
-	wake_up(&indio_dev->buffer->pollq);
+	if (indio_dev->buffer_out)
+		wake_up(&indio_dev->buffer_out->pollq);
 }
 
 void iio_buffer_init(struct iio_buffer *buffer)
@@ -256,13 +306,26 @@ static ssize_t iio_show_fixed_type(struct device *dev,
 		       this_attr->c->scan_type.shift);
 }
 
+static struct iio_buffer *iio_scan_el_attr_get_buffer(struct iio_dev *indio_dev,
+						      struct device_attribute *attr)
+{
+	/* FIXME; which to check here? */
+	if (false /*belongs to scan_elements_out group */)
+		return indio_dev->buffer_out;
+	/* FIXME; which to check here? */
+	if (false /* strncmp(attr->name, "out_", sizeof("out_") - 1) == 0 */)
+		return indio_dev->buffer_out;
+
+	return indio_dev->buffer_in;
+}
+
 static ssize_t iio_scan_el_show(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_scan_el_attr_get_buffer(indio_dev, attr);
 
 	/* Ensure ret is 0 or 1. */
 	ret = !!test_bit(to_iio_dev_attr(attr)->address,
@@ -375,7 +438,7 @@ static ssize_t iio_scan_el_store(struct device *dev,
 	int ret;
 	bool state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_scan_el_attr_get_buffer(indio_dev, attr);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = strtobool(buf, &state);
@@ -411,7 +474,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev,
 				   char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_scan_el_attr_get_buffer(indio_dev, attr);
 
 	return sprintf(buf, "%d\n", buffer->scan_timestamp);
 }
@@ -423,7 +486,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_scan_el_attr_get_buffer(indio_dev, attr);
 	bool state;
 
 	ret = strtobool(buf, &state);
@@ -495,12 +558,21 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static struct iio_buffer *iio_buffer_attr_get_buffer(struct iio_dev *indio_dev,
+						     struct device_attribute *attr)
+{
+	if (false /*belongs to buffer_out group */)
+		return indio_dev->buffer_out;
+
+	return indio_dev->buffer_in;
+}
+
 static ssize_t iio_buffer_read_length(struct device *dev,
 				      struct device_attribute *attr,
 				      char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 
 	return sprintf(buf, "%d\n", buffer->length);
 }
@@ -510,7 +582,7 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 				       const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 	unsigned int val;
 	int ret;
 
@@ -543,7 +615,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev,
 				      char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 
 	return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
 }
@@ -1135,7 +1207,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	int ret;
 	bool requested_state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 	bool inlist;
 
 	ret = strtobool(buf, &requested_state);
@@ -1160,14 +1232,12 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	return (ret < 0) ? ret : len;
 }
 
-static const char * const iio_scan_elements_group_name = "scan_elements";
-
 static ssize_t iio_buffer_show_watermark(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 
 	return sprintf(buf, "%u\n", buffer->watermark);
 }
@@ -1178,7 +1248,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 					  size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 	unsigned int val;
 	int ret;
 
@@ -1212,7 +1282,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
 						char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_buffer *buffer = iio_buffer_attr_get_buffer(indio_dev, attr);
 
 	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
@@ -1237,23 +1307,15 @@ static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_data_available.attr,
 };
 
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
+static int __iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev,
+					     struct iio_buffer *buffer,
+					     enum iio_buffer_dir dir)
 {
 	struct iio_dev_attr *p;
 	struct attribute **attr;
-	struct iio_buffer *buffer = indio_dev->buffer;
 	int ret, i, attrn, attrcount, attrcount_orig = 0;
 	const struct iio_chan_spec *channels;
 
-	channels = indio_dev->channels;
-	if (channels) {
-		int ml = indio_dev->masklength;
-
-		for (i = 0; i < indio_dev->num_channels; i++)
-			ml = max(ml, channels[i].scan_index + 1);
-		indio_dev->masklength = ml;
-	}
-
 	if (!buffer)
 		return 0;
 
@@ -1281,8 +1343,12 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 
 	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
 
-	buffer->buffer_group.name = "buffer";
+	if (dir == IIO_BUFFER_DIRECTION_OUT)
+		buffer->buffer_group.name = "buffer_out";
+	else
+		buffer->buffer_group.name = "buffer_in";
 	buffer->buffer_group.attrs = attr;
+	/* FIXME: add buffer/ symlink for backwards compat */
 
 	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
 
@@ -1319,7 +1385,11 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 		}
 	}
 
-	buffer->scan_el_group.name = iio_scan_elements_group_name;
+	if (dir == IIO_BUFFER_DIRECTION_OUT)
+		buffer->scan_el_group.name = "scan_elements_out";
+	else
+		buffer->scan_el_group.name = "scan_elements_in";
+	/* FIXME: add scan_elements/ symlink for backwards compat */
 
 	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
 					      sizeof(buffer->scan_el_group.attrs[0]),
@@ -1348,10 +1418,33 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	return ret;
 }
 
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
-	struct iio_buffer *buffer = indio_dev->buffer;
+	const struct iio_chan_spec *channels;
+	int i, ret;
+
+	channels = indio_dev->channels;
+	if (channels) {
+		int ml = indio_dev->masklength;
+
+		for (i = 0; i < indio_dev->num_channels; i++)
+			ml = max(ml, channels[i].scan_index + 1);
+		indio_dev->masklength = ml;
+	}
+
+	ret = __iio_buffer_alloc_sysfs_and_mask(indio_dev,
+						indio_dev->buffer_in,
+						IIO_BUFFER_DIRECTION_IN);
+	if (ret)
+		return ret;
+
+	return __iio_buffer_alloc_sysfs_and_mask(indio_dev,
+						 indio_dev->buffer_out,
+						 IIO_BUFFER_DIRECTION_OUT);
+}
 
+static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+{
 	if (!buffer)
 		return;
 
@@ -1361,6 +1454,12 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
 }
 
+void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+{
+	__iio_buffer_free_sysfs_and_mask(indio_dev->buffer_in);
+	__iio_buffer_free_sysfs_and_mask(indio_dev->buffer_out);
+}
+
 /**
  * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
  * @indio_dev: the iio device
@@ -1470,6 +1569,26 @@ void iio_buffer_put(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL_GPL(iio_buffer_put);
 
+/**
+ * iio_device_attach_buffer - Attach a buffer to a IIO device for reading or writing
+ * @indio_dev: The device the buffer should be attached to
+ * @buffer: The buffer to attach to the device
+ * @dir: The direction to which to use this buffer (reading or writing)
+ *
+ * This function attaches a buffer to a IIO device. The buffer stays attached to
+ * the device until the device is freed. The function should only be called at
+ * most once per device.
+ */
+void iio_device_attach_buffer_dir(struct iio_dev *indio_dev,
+				  struct iio_buffer *buffer,
+				  enum iio_buffer_dir dir)
+{
+	if (dir == IIO_BUFFER_DIRECTION_OUT)
+		indio_dev->buffer_out = iio_buffer_get(buffer);
+	else
+		indio_dev->buffer_in = iio_buffer_get(buffer);
+}
+
 /**
  * iio_device_attach_buffer - Attach a buffer to a IIO device
  * @indio_dev: The device the buffer should be attached to
@@ -1482,6 +1601,7 @@ EXPORT_SYMBOL_GPL(iio_buffer_put);
 void iio_device_attach_buffer(struct iio_dev *indio_dev,
 			      struct iio_buffer *buffer)
 {
-	indio_dev->buffer = iio_buffer_get(buffer);
+	iio_device_attach_buffer_dir(indio_dev, buffer,
+				     IIO_BUFFER_DIRECTION_IN);
 }
 EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a13957644c1d..45add6a1087a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1475,7 +1475,8 @@ static void iio_dev_release(struct device *device)
 	iio_device_unregister_eventset(indio_dev);
 	iio_device_unregister_sysfs(indio_dev);
 
-	iio_buffer_put(indio_dev->buffer);
+	iio_buffer_put(indio_dev->buffer_in);
+	iio_buffer_put(indio_dev->buffer_out);
 
 	ida_simple_remove(&iio_ida, indio_dev->id);
 	kfree(indio_dev);
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index fbba4093f06c..3e1717383f27 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,6 +11,11 @@
 
 struct iio_buffer;
 
+enum iio_buffer_dir {
+	IIO_BUFFER_DIRECTION_IN,
+	IIO_BUFFER_DIRECTION_OUT,
+};
+
 void iio_buffer_set_attrs(struct iio_buffer *buffer,
 			 const struct attribute **attrs);
 
@@ -44,6 +49,10 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
 				   const unsigned long *mask);
 
+void iio_device_attach_buffer_dir(struct iio_dev *indio_dev,
+				  struct iio_buffer *buffer,
+				  enum iio_buffer_dir dir);
+
 void iio_device_attach_buffer(struct iio_dev *indio_dev,
 			      struct iio_buffer *buffer);
 
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 1e7edf6bed96..e4728a7a2ad8 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -21,6 +21,9 @@ struct iio_buffer;
  * @read:		try to get a specified number of bytes (must exist)
  * @data_available:	indicates how much data is available for reading from
  *			the buffer.
+ * @write:		try to write a specified number of bytes
+ * @space_available:	indicates how much data is available for writing to
+ *			the buffer.
  * @request_update:	if a parameter change has been marked, update underlying
  *			storage.
  * @set_bytes_per_datum:set number of bytes per datum
@@ -48,6 +51,10 @@ struct iio_buffer_access_funcs {
 	int (*read)(struct iio_buffer *buffer, size_t n, char __user *buf);
 	size_t (*data_available)(struct iio_buffer *buffer);
 
+	int (*write)(struct iio_buffer *buffer, size_t n,
+		const char __user *buf);
+	size_t (*space_available)(struct iio_buffer *buffer);
+
 	int (*request_update)(struct iio_buffer *buffer);
 
 	int (*set_bytes_per_datum)(struct iio_buffer *buffer, size_t bpd);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index a123f8acb192..9460970d02af 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -489,7 +489,9 @@ struct iio_buffer_setup_ops {
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
  * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
- * @buffer:		[DRIVER] any buffer present
+ * @buffer:		[DRIVER] same as @buffer_in (for backwards compat)
+ * @buffer_in:		[DRIVER] any buffer present for reading from a device
+ * @buffer_out:		[DRIVER] any buffer present for writing to a device
  * @active_buffers:	[INTERN] list of all buffers currently enabled/active
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
  * @mlock:		[INTERN] lock used to prevent simultaneous device state
@@ -533,7 +535,9 @@ struct iio_dev {
 
 	struct iio_event_interface	*event_interface;
 
-	struct iio_buffer		*buffer;
+	//struct iio_buffer		*buffer;
+	struct iio_buffer		*buffer_in;
+	struct iio_buffer		*buffer_out;
 	struct list_head		active_buffers;
 	int				scan_bytes;
 	struct mutex			mlock;
-- 
2.20.1

Re: [RFC PATCH 0/3] iio: buffer: add output buffer support for chrdev

[Ardelean, Alexandru]

On Mon, 2020-03-30 at 17:57 +0300, Alexandru Ardelean wrote:

I goofed Michael's email on the first run.
Sorry for the spam.

> [This description is also present on patch 3/3, there have been some
> cosmetic stuff since that one that I did not remove. These patches
> probably won't make it in a final version, but they're there because
> I changed my mind a few times and got to this RFC]
> 
> There have been some offline discussions about how to go about this.
> Since I wasn't involved in any of those discussions, this kind of tries to
> re-build things from various bits.
> It's incomplete work, since I don't have a clear idea of what the
> accepted/acceptable
> approach would be.
> 
> 1. First approach is: we keep 1 buffer per device, and we make it either
> in/out, which means that for devices that for devices that have both in/out
> 2 iio_dev instances are required, or an ADC needs to be connected on the in
> path and a DAC on out-path. This is predominantly done in the ADI tree.
> 
> 2. One discussion/proposal was to have multiple buffers per-device. But the
> details are vague since they were relayed to me.
> One detail was, to have indexes for devices that have more than 1
> buffer:
> 
> Iio_deviceX:
>         buffer
>         scan_elements
> 
> Iio_deviceX:
>         BufferY
>         scan_elementsY
>         BufferZ
>         scan_elementsZ
> 
> I am not sure how feasible this is for a single chrdev, as when you look at
> the fileops that get assigned to a chrdev, it looks like it can have at
> most two buffers (one for input, out for output).
> 
> Multiplexing input buffers can work (from ADCs), but demultiplexing output
> buffers into a DAC, not so well. Especially on a single chrdev.
> 
> Question 1: do we want to support more than 2 buffers per chrdev?
> 
> This is what ADI currently has in it's tree (and works).
> 
> Example, ADC:
>  # ls iio\:device3/buffer/
>  data_available  enable  length  length_align_bytes  watermark
>  #  ls iio\:device3/scan_elements/
>  in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_volta
> ge1_index  in_voltage1_type
> 
> Example, DAC:
>  #  ls iio\:device4/buffer/
>  data_available  enable  length  length_align_bytes  watermark
>  # ls iio\:device4/scan_elements/
>  out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en   
>   out_voltage2_type  out_voltage3_index
>  out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_index
>   out_voltage3_en    out_voltage3_type
> 
> The direction of each element is encoded into the filename of each channel.
> 
> Another question is:
>  Does it make sense to have more than 1 'scan_elements' folder?
>  That is, for devices that would have both in & out channels.
> 
> For 'buffer' folders I was thinking that it may make sense to have,
> 'buffer_in' && 'buffer_out'.
> 
> So, one idea is:
> 
> Iio_deviceX:
>         buffer_in
>         buffer_out
>         scan_elements
> 
> Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
> But the format is:
> 
> Iio_deviceX:
>         buffer_in
>         buffer_out
>         scan_elements_in
>         scan_elements_out
> 
> Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
> some glimpse of where this could go.
> 
> 3. A side question is about the 'iio_buffer -> pollq' field. I was
> wondering if it would make sense to move that on to 'iio_dev  pollq' if
> adding multiple buffers are added per-device. It almost makes sense to
> unify the 'pollq' on indio_dev.
> But, it looks a bit difficult, and would require some more change [which is
> doable] if it makes sense for whatever reason.
> The only reason to do it, is because the iio_buffer_fileops has a .poll =
> iio_buffer_poll() function attached to it. Adding multiple buffers for an
> IIO device may require some consideration on the iio_buffer_poll() function
> as well.
> 
> Alexandru Ardelean (3):
>   iio: core: rename 'indio_dev->buffer_list' ->
>     'indio_dev->active_buffers'
>   iio: buffer: extend short-hand use for 'indio_dev->buffer'
>   iio: buffer: add output buffer support for chrdev
> 
>  .../buffer/industrialio-buffer-dmaengine.c    |   3 +-
>  drivers/iio/industrialio-buffer.c             | 267 +++++++++++++-----
>  drivers/iio/industrialio-core.c               |   5 +-
>  include/linux/iio/buffer.h                    |   9 +
>  include/linux/iio/buffer_impl.h               |   7 +
>  include/linux/iio/iio.h                       |  12 +-
>  6 files changed, 225 insertions(+), 78 deletions(-)
> 

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Lars-Peter Clausen]

On 3/30/20 4:57 PM, Alexandru Ardelean wrote:
> This is WIP.
> It hasn't been tested yet. Mostly serves as base for some discussion.
>
> There have been some offline discussions about how to go about this.
> Since I wasn't involved in any of those discussions, this kind of tries to
> re-build things from various bits.
>
> 1. First approach is: we keep 1 buffer per device, and we make it either
> in/out, which means that for devices that for devices that have both in/out
> 2 iio_dev instances are required, or an ADC needs to be connected on the in
> path and a DAC on out-path. This is predominantly done in the ADI tree.
>
> 2. One discussion/proposal was to have multiple buffers per-device. But the
> details are vague since they were relayed to me.
> One detail was, to have indexes for devices that have more than 1
> buffer:
>
> Iio_deviceX:
>          buffer
>          scan_elements
>
> Iio_deviceX:
>          BufferY
>          scan_elementsY
>          BufferZ
>          scan_elementsZ
>
> I am not sure how feasible this is for a single chrdev, as when you look at
> the fileops that get assigned to a chrdev, it looks like it can have at
> most two buffers (one for input, out for output).
>
> Multiplexing input buffers can work (from ADCs), but demultiplexing output
> buffers into a DAC, not so well. Especially on a single chrdev.
>
> Question 1: do we want to support more than 2 buffers per chrdev?
>
> This is what ADI currently has in it's tree (and works).
>
> Example, ADC:
>   # ls iio\:device3/buffer/
>   data_available  enable  length  length_align_bytes  watermark
>   #  ls iio\:device3/scan_elements/
>   in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_voltage1_index  in_voltage1_type
>
> Example, DAC:
>   #  ls iio\:device4/buffer/
>   data_available  enable  length  length_align_bytes  watermark
>   # ls iio\:device4/scan_elements/
>   out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en     out_voltage2_type  out_voltage3_index
>   out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_index  out_voltage3_en    out_voltage3_type
>
> The direction of each element is encoded into the filename of each channel.
>
> Another question is:
>   Does it make sense to have more than 1 'scan_elements' folder?
>   That is, for devices that would have both in & out channels.
>
> For 'buffer' folders I was thinking that it may make sense to have,
> 'buffer_in' && 'buffer_out'.
>
> So, one idea is:
>
> Iio_deviceX:
>          buffer_in
>          buffer_out
>          scan_elements
>
> Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
> But the format is:
>
> Iio_deviceX:
>          buffer_in
>          buffer_out
>          scan_elements_in
>          scan_elements_out

I'd make scan_elements as a sub-folder of the buffer folder. And have 
symlink for the legacy case

>
> Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
> some glimpse of where this could go.

I believe the basic idea behind the multiple buffers per device was, 
that if we do it, we should do it in a way that you can have an 
arbitrary number of buffers. E.g. not just one input and output but also 
multiple input buffers.

>
> 3. A side question is about the 'iio_buffer -> pollq' field. I was
> wondering if it would make sense to move that on to 'iio_dev  pollq' if
> adding multiple buffers are added per-device. It almost makes sense to
> unify the 'pollq' on indio_dev.
> But, it looks a bit difficult, and would require some more change [which is
> doable] if it makes sense for whatever reason.
> The only reason to do it, is because the iio_buffer_fileops has a .poll =
> iio_buffer_poll() function attached to it. Adding multiple buffers for an
> IIO device may require some consideration on the iio_buffer_poll() function
> as well.

I think we need one chardev per buffer. Conceptually that is the right 
approach in my option since the two buffers are independent streams. But 
also from a practical point of view we want to have the ability to have 
the buffers opened by different applications. E.g. iio_readdev on the 
input buffer and iio_writedev on the output buffer. And there might be 
some other operations that wont multiplex as nicely as read/write. The 
high speed interface for example would not work as it is right now.

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Ardelean, Alexandru]

On Mon, 2020-03-30 at 17:58 +0200, Lars-Peter Clausen wrote:
> [External]
> 
> On 3/30/20 4:57 PM, Alexandru Ardelean wrote:
> > This is WIP.
> > It hasn't been tested yet. Mostly serves as base for some discussion.
> > 
> > There have been some offline discussions about how to go about this.
> > Since I wasn't involved in any of those discussions, this kind of tries to
> > re-build things from various bits.
> > 
> > 1. First approach is: we keep 1 buffer per device, and we make it either
> > in/out, which means that for devices that for devices that have both in/out
> > 2 iio_dev instances are required, or an ADC needs to be connected on the in
> > path and a DAC on out-path. This is predominantly done in the ADI tree.
> > 
> > 2. One discussion/proposal was to have multiple buffers per-device. But the
> > details are vague since they were relayed to me.
> > One detail was, to have indexes for devices that have more than 1
> > buffer:
> > 
> > Iio_deviceX:
> >          buffer
> >          scan_elements
> > 
> > Iio_deviceX:
> >          BufferY
> >          scan_elementsY
> >          BufferZ
> >          scan_elementsZ
> > 
> > I am not sure how feasible this is for a single chrdev, as when you look at
> > the fileops that get assigned to a chrdev, it looks like it can have at
> > most two buffers (one for input, out for output).
> > 
> > Multiplexing input buffers can work (from ADCs), but demultiplexing output
> > buffers into a DAC, not so well. Especially on a single chrdev.
> > 
> > Question 1: do we want to support more than 2 buffers per chrdev?
> > 
> > This is what ADI currently has in it's tree (and works).
> > 
> > Example, ADC:
> >   # ls iio\:device3/buffer/
> >   data_available  enable  length  length_align_bytes  watermark
> >   #  ls iio\:device3/scan_elements/
> >  
> > in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_volt
> > age1_index  in_voltage1_type
> > 
> > Example, DAC:
> >   #  ls iio\:device4/buffer/
> >   data_available  enable  length  length_align_bytes  watermark
> >   # ls iio\:device4/scan_elements/
> >  
> > out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en  
> >    out_voltage2_type  out_voltage3_index
> >  
> > out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_inde
> > x  out_voltage3_en    out_voltage3_type
> > 
> > The direction of each element is encoded into the filename of each channel.
> > 
> > Another question is:
> >   Does it make sense to have more than 1 'scan_elements' folder?
> >   That is, for devices that would have both in & out channels.
> > 
> > For 'buffer' folders I was thinking that it may make sense to have,
> > 'buffer_in' && 'buffer_out'.
> > 
> > So, one idea is:
> > 
> > Iio_deviceX:
> >          buffer_in
> >          buffer_out
> >          scan_elements
> > 
> > Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
> > But the format is:
> > 
> > Iio_deviceX:
> >          buffer_in
> >          buffer_out
> >          scan_elements_in
> >          scan_elements_out
> 
> I'd make scan_elements as a sub-folder of the buffer folder. And have 
> symlink for the legacy case
> 
> > Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
> > some glimpse of where this could go.
> 
> I believe the basic idea behind the multiple buffers per device was, 
> that if we do it, we should do it in a way that you can have an 
> arbitrary number of buffers. E.g. not just one input and output but also 
> multiple input buffers.
> 
> > 3. A side question is about the 'iio_buffer -> pollq' field. I was
> > wondering if it would make sense to move that on to 'iio_dev  pollq' if
> > adding multiple buffers are added per-device. It almost makes sense to
> > unify the 'pollq' on indio_dev.
> > But, it looks a bit difficult, and would require some more change [which is
> > doable] if it makes sense for whatever reason.
> > The only reason to do it, is because the iio_buffer_fileops has a .poll =
> > iio_buffer_poll() function attached to it. Adding multiple buffers for an
> > IIO device may require some consideration on the iio_buffer_poll() function
> > as well.
> 
> I think we need one chardev per buffer. Conceptually that is the right 
> approach in my option since the two buffers are independent streams. But 
> also from a practical point of view we want to have the ability to have 
> the buffers opened by different applications. E.g. iio_readdev on the 
> input buffer and iio_writedev on the output buffer. And there might be 
> some other operations that wont multiplex as nicely as read/write. The 
> high speed interface for example would not work as it is right now.

For some reason, the idea of more than 1 chrdev per IIO device escaped me.
It makes things easier.
I'll take a look.
I probably need to think about the naming, or how to identify chrdevs per IIO
device.

Maybe something like:
/dev/iio:device0:0
/dev/iio:device0:1

where the 2nd index is 
/sys/bus/iio/devices/iio:device0/buffer0
/sys/bus/iio/devices/iio:device0/buffer1

or something like that

not sure yet how possible it is yet; need to check

> 

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Lars-Peter Clausen]

On 3/30/20 7:27 PM, Ardelean, Alexandru wrote:
> On Mon, 2020-03-30 at 17:58 +0200, Lars-Peter Clausen wrote:
>> [External]
>>
>> On 3/30/20 4:57 PM, Alexandru Ardelean wrote:
>>> This is WIP.
>>> It hasn't been tested yet. Mostly serves as base for some discussion.
>>>
>>> There have been some offline discussions about how to go about this.
>>> Since I wasn't involved in any of those discussions, this kind of tries to
>>> re-build things from various bits.
>>>
>>> 1. First approach is: we keep 1 buffer per device, and we make it either
>>> in/out, which means that for devices that for devices that have both in/out
>>> 2 iio_dev instances are required, or an ADC needs to be connected on the in
>>> path and a DAC on out-path. This is predominantly done in the ADI tree.
>>>
>>> 2. One discussion/proposal was to have multiple buffers per-device. But the
>>> details are vague since they were relayed to me.
>>> One detail was, to have indexes for devices that have more than 1
>>> buffer:
>>>
>>> Iio_deviceX:
>>>           buffer
>>>           scan_elements
>>>
>>> Iio_deviceX:
>>>           BufferY
>>>           scan_elementsY
>>>           BufferZ
>>>           scan_elementsZ
>>>
>>> I am not sure how feasible this is for a single chrdev, as when you look at
>>> the fileops that get assigned to a chrdev, it looks like it can have at
>>> most two buffers (one for input, out for output).
>>>
>>> Multiplexing input buffers can work (from ADCs), but demultiplexing output
>>> buffers into a DAC, not so well. Especially on a single chrdev.
>>>
>>> Question 1: do we want to support more than 2 buffers per chrdev?
>>>
>>> This is what ADI currently has in it's tree (and works).
>>>
>>> Example, ADC:
>>>    # ls iio\:device3/buffer/
>>>    data_available  enable  length  length_align_bytes  watermark
>>>    #  ls iio\:device3/scan_elements/
>>>   
>>> in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_volt
>>> age1_index  in_voltage1_type
>>>
>>> Example, DAC:
>>>    #  ls iio\:device4/buffer/
>>>    data_available  enable  length  length_align_bytes  watermark
>>>    # ls iio\:device4/scan_elements/
>>>   
>>> out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en
>>>     out_voltage2_type  out_voltage3_index
>>>   
>>> out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_inde
>>> x  out_voltage3_en    out_voltage3_type
>>>
>>> The direction of each element is encoded into the filename of each channel.
>>>
>>> Another question is:
>>>    Does it make sense to have more than 1 'scan_elements' folder?
>>>    That is, for devices that would have both in & out channels.
>>>
>>> For 'buffer' folders I was thinking that it may make sense to have,
>>> 'buffer_in' && 'buffer_out'.
>>>
>>> So, one idea is:
>>>
>>> Iio_deviceX:
>>>           buffer_in
>>>           buffer_out
>>>           scan_elements
>>>
>>> Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
>>> But the format is:
>>>
>>> Iio_deviceX:
>>>           buffer_in
>>>           buffer_out
>>>           scan_elements_in
>>>           scan_elements_out
>> I'd make scan_elements as a sub-folder of the buffer folder. And have
>> symlink for the legacy case
>>
>>> Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
>>> some glimpse of where this could go.
>> I believe the basic idea behind the multiple buffers per device was,
>> that if we do it, we should do it in a way that you can have an
>> arbitrary number of buffers. E.g. not just one input and output but also
>> multiple input buffers.
>>
>>> 3. A side question is about the 'iio_buffer -> pollq' field. I was
>>> wondering if it would make sense to move that on to 'iio_dev  pollq' if
>>> adding multiple buffers are added per-device. It almost makes sense to
>>> unify the 'pollq' on indio_dev.
>>> But, it looks a bit difficult, and would require some more change [which is
>>> doable] if it makes sense for whatever reason.
>>> The only reason to do it, is because the iio_buffer_fileops has a .poll =
>>> iio_buffer_poll() function attached to it. Adding multiple buffers for an
>>> IIO device may require some consideration on the iio_buffer_poll() function
>>> as well.
>> I think we need one chardev per buffer. Conceptually that is the right
>> approach in my option since the two buffers are independent streams. But
>> also from a practical point of view we want to have the ability to have
>> the buffers opened by different applications. E.g. iio_readdev on the
>> input buffer and iio_writedev on the output buffer. And there might be
>> some other operations that wont multiplex as nicely as read/write. The
>> high speed interface for example would not work as it is right now.
> For some reason, the idea of more than 1 chrdev per IIO device escaped me.
> It makes things easier.
> I'll take a look.
> I probably need to think about the naming, or how to identify chrdevs per IIO
> device.
>
> Maybe something like:
> /dev/iio:device0:0
> /dev/iio:device0:1
>
> where the 2nd index is
> /sys/bus/iio/devices/iio:device0/buffer0
> /sys/bus/iio/devices/iio:device0/buffer1
>
> or something like that
>
> not sure yet how possible it is yet; need to check

Yep. With symlink of buffer to buffer0 to keep backwards compatibility.

There is maybe one tricky part. We use an ioctl on the buffer file 
descriptor to get access to the event file descriptor. I don't remember 
quite exactly why it was done like this I remember that I believe Arned 
Bergman suggested it. But it feels like it was more of a novelty thing, 
like look what cool stuff we can doe with anonymous inodes.

Maybe going forward it makes sense it introduce a event chardev, which 
would bring us full circle: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8e7d967244a8eebcdadb048efc5e66849ec0a6b6

- Lars

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Jonathan Cameron]

On Mon, 30 Mar 2020 19:43:09 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 3/30/20 7:27 PM, Ardelean, Alexandru wrote:
> > On Mon, 2020-03-30 at 17:58 +0200, Lars-Peter Clausen wrote:  
> >> [External]
> >>
> >> On 3/30/20 4:57 PM, Alexandru Ardelean wrote:  
> >>> This is WIP.
> >>> It hasn't been tested yet. Mostly serves as base for some discussion.
> >>>
> >>> There have been some offline discussions about how to go about this.
> >>> Since I wasn't involved in any of those discussions, this kind of tries to
> >>> re-build things from various bits.
> >>>
> >>> 1. First approach is: we keep 1 buffer per device, and we make it either
> >>> in/out, which means that for devices that for devices that have both in/out
> >>> 2 iio_dev instances are required, or an ADC needs to be connected on the in
> >>> path and a DAC on out-path. This is predominantly done in the ADI tree.
> >>>
> >>> 2. One discussion/proposal was to have multiple buffers per-device. But the
> >>> details are vague since they were relayed to me.
> >>> One detail was, to have indexes for devices that have more than 1
> >>> buffer:
> >>>
> >>> Iio_deviceX:
> >>>           buffer
> >>>           scan_elements
> >>>
> >>> Iio_deviceX:
> >>>           BufferY
> >>>           scan_elementsY
> >>>           BufferZ
> >>>           scan_elementsZ
> >>>
> >>> I am not sure how feasible this is for a single chrdev, as when you look at
> >>> the fileops that get assigned to a chrdev, it looks like it can have at
> >>> most two buffers (one for input, out for output).
> >>>
> >>> Multiplexing input buffers can work (from ADCs), but demultiplexing output
> >>> buffers into a DAC, not so well. Especially on a single chrdev.
> >>>
> >>> Question 1: do we want to support more than 2 buffers per chrdev?
> >>>
> >>> This is what ADI currently has in it's tree (and works).
> >>>
> >>> Example, ADC:
> >>>    # ls iio\:device3/buffer/
> >>>    data_available  enable  length  length_align_bytes  watermark
> >>>    #  ls iio\:device3/scan_elements/
> >>>   
> >>> in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_volt
> >>> age1_index  in_voltage1_type
> >>>
> >>> Example, DAC:
> >>>    #  ls iio\:device4/buffer/
> >>>    data_available  enable  length  length_align_bytes  watermark
> >>>    # ls iio\:device4/scan_elements/
> >>>   
> >>> out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en
> >>>     out_voltage2_type  out_voltage3_index
> >>>   
> >>> out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_inde
> >>> x  out_voltage3_en    out_voltage3_type
> >>>
> >>> The direction of each element is encoded into the filename of each channel.
> >>>
> >>> Another question is:
> >>>    Does it make sense to have more than 1 'scan_elements' folder?
> >>>    That is, for devices that would have both in & out channels.
> >>>
> >>> For 'buffer' folders I was thinking that it may make sense to have,
> >>> 'buffer_in' && 'buffer_out'.
> >>>
> >>> So, one idea is:
> >>>
> >>> Iio_deviceX:
> >>>           buffer_in
> >>>           buffer_out
> >>>           scan_elements
> >>>
> >>> Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
> >>> But the format is:
> >>>
> >>> Iio_deviceX:
> >>>           buffer_in
> >>>           buffer_out
> >>>           scan_elements_in
> >>>           scan_elements_out  
> >> I'd make scan_elements as a sub-folder of the buffer folder. And have
> >> symlink for the legacy case
> >>  
> >>> Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
> >>> some glimpse of where this could go.  
> >> I believe the basic idea behind the multiple buffers per device was,
> >> that if we do it, we should do it in a way that you can have an
> >> arbitrary number of buffers. E.g. not just one input and output but also
> >> multiple input buffers.
> >>  
> >>> 3. A side question is about the 'iio_buffer -> pollq' field. I was
> >>> wondering if it would make sense to move that on to 'iio_dev  pollq' if
> >>> adding multiple buffers are added per-device. It almost makes sense to
> >>> unify the 'pollq' on indio_dev.
> >>> But, it looks a bit difficult, and would require some more change [which is
> >>> doable] if it makes sense for whatever reason.
> >>> The only reason to do it, is because the iio_buffer_fileops has a .poll =
> >>> iio_buffer_poll() function attached to it. Adding multiple buffers for an
> >>> IIO device may require some consideration on the iio_buffer_poll() function
> >>> as well.  
> >> I think we need one chardev per buffer. Conceptually that is the right
> >> approach in my option since the two buffers are independent streams. But
> >> also from a practical point of view we want to have the ability to have
> >> the buffers opened by different applications. E.g. iio_readdev on the
> >> input buffer and iio_writedev on the output buffer. And there might be
> >> some other operations that wont multiplex as nicely as read/write. The
> >> high speed interface for example would not work as it is right now.  
> > For some reason, the idea of more than 1 chrdev per IIO device escaped me.
> > It makes things easier.
> > I'll take a look.
> > I probably need to think about the naming, or how to identify chrdevs per IIO
> > device.
> >
> > Maybe something like:
> > /dev/iio:device0:0
> > /dev/iio:device0:1
> >
> > where the 2nd index is
> > /sys/bus/iio/devices/iio:device0/buffer0
> > /sys/bus/iio/devices/iio:device0/buffer1
> >
> > or something like that
> >
> > not sure yet how possible it is yet; need to check  
> 
> Yep. With symlink of buffer to buffer0 to keep backwards compatibility.
> 
> There is maybe one tricky part. We use an ioctl on the buffer file 
> descriptor to get access to the event file descriptor. I don't remember 
> quite exactly why it was done like this I remember that I believe Arned 
> Bergman suggested it. But it feels like it was more of a novelty thing, 
> like look what cool stuff we can doe with anonymous inodes.

Specifically it was the concern about huge numbers of character devices
that weren't obviously associated with each other.  I'm not sure we want
to change that.

Events are still going to be associated with the channels on a particular
buffer so I'd leave this alone.

I think the number of multiple buffer devices is going to be fairly small
so I'm not so worried about using additional character devices.

Jonathan

> 
> Maybe going forward it makes sense it introduce a event chardev, which 
> would bring us full circle: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8e7d967244a8eebcdadb048efc5e66849ec0a6b6
> 
> - Lars
> 

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Jonathan Cameron]

On Mon, 30 Mar 2020 17:58:16 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 3/30/20 4:57 PM, Alexandru Ardelean wrote:
> > This is WIP.
> > It hasn't been tested yet. Mostly serves as base for some discussion.
> >
> > There have been some offline discussions about how to go about this.
> > Since I wasn't involved in any of those discussions, this kind of tries to
> > re-build things from various bits.
> >
> > 1. First approach is: we keep 1 buffer per device, and we make it either
> > in/out, which means that for devices that for devices that have both in/out
> > 2 iio_dev instances are required, or an ADC needs to be connected on the in
> > path and a DAC on out-path. This is predominantly done in the ADI tree.
> >
> > 2. One discussion/proposal was to have multiple buffers per-device. But the
> > details are vague since they were relayed to me.
> > One detail was, to have indexes for devices that have more than 1
> > buffer:
> >
> > Iio_deviceX:
> >          buffer
> >          scan_elements
> >
> > Iio_deviceX:
> >          BufferY
> >          scan_elementsY
> >          BufferZ
> >          scan_elementsZ
> >
> > I am not sure how feasible this is for a single chrdev, as when you look at
> > the fileops that get assigned to a chrdev, it looks like it can have at
> > most two buffers (one for input, out for output).
> >
> > Multiplexing input buffers can work (from ADCs), but demultiplexing output
> > buffers into a DAC, not so well. Especially on a single chrdev.
> >
> > Question 1: do we want to support more than 2 buffers per chrdev?
> >
> > This is what ADI currently has in it's tree (and works).
> >
> > Example, ADC:
> >   # ls iio\:device3/buffer/
> >   data_available  enable  length  length_align_bytes  watermark
> >   #  ls iio\:device3/scan_elements/
> >   in_voltage0_en  in_voltage0_index  in_voltage0_type  in_voltage1_en  in_voltage1_index  in_voltage1_type
> >
> > Example, DAC:
> >   #  ls iio\:device4/buffer/
> >   data_available  enable  length  length_align_bytes  watermark
> >   # ls iio\:device4/scan_elements/
> >   out_voltage0_en     out_voltage0_type  out_voltage1_index  out_voltage2_en     out_voltage2_type  out_voltage3_index
> >   out_voltage0_index  out_voltage1_en    out_voltage1_type   out_voltage2_index  out_voltage3_en    out_voltage3_type
> >
> > The direction of each element is encoded into the filename of each channel.
> >
> > Another question is:
> >   Does it make sense to have more than 1 'scan_elements' folder?
> >   That is, for devices that would have both in & out channels.
> >
> > For 'buffer' folders I was thinking that it may make sense to have,
> > 'buffer_in' && 'buffer_out'.
> >
> > So, one idea is:
> >
> > Iio_deviceX:
> >          buffer_in
> >          buffer_out
> >          scan_elements
> >
> > Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
> > But the format is:
> >
> > Iio_deviceX:
> >          buffer_in
> >          buffer_out
> >          scan_elements_in
> >          scan_elements_out  
> 
> I'd make scan_elements as a sub-folder of the buffer folder. And have 
> symlink for the legacy case

Now this is a bit of legacy.  The original concept was that you could have
different types of buffer registered for a device (though only one at a time)
and hence scan_elements reflected what was being pushed 'into' the buffer
vs the buffer folder that reflected characteristics of the buffers (how long
it was for instance).

Reality was we never really supported multiple buffers like this, having figured
out that hardware fifos are almost always better fronted by a software fifo
rather than directly exposed.

So I want to think about it a bit more, but right now I can't see a reason not
to move the scan_elements directories under the buffer. (with symlink!)

> 
> >
> > Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
> > some glimpse of where this could go.  
> 
> I believe the basic idea behind the multiple buffers per device was, 
> that if we do it, we should do it in a way that you can have an 
> arbitrary number of buffers. E.g. not just one input and output but also 
> multiple input buffers.

We have other devices that might be better supported this way, but get fiddly
because you can move the channels between different buffers.

N channels, M triggers, P hardware fifos (or tagged fifos).
Some combinations go into different places.

A classic is an accelerometer / gyro device where the sampling rates can be
entirely different and you get accel/accel/gyro/accel/accel/gyro in the 
fifo.  Currently we split that into two devices.

However, the issue in transistioning to this model is there isn't an
obvious way of making it backwards compatible for existing userspace.
I guess we could do some magic by hooking in an optional consumer driver
that just wraps a particular buffer. 

For output buffers at least this isn't a problem.

There are other corners to deal with such as breaking the current assumption
that triggers are global for the device (probably move trigger under the buffer
much like scan_elements and symlink for legacy) and sampling_frequency
- though we already have devices where that is either a buffer or trigger
characteristic.

> 
> >
> > 3. A side question is about the 'iio_buffer -> pollq' field. I was
> > wondering if it would make sense to move that on to 'iio_dev  pollq' if
> > adding multiple buffers are added per-device. It almost makes sense to
> > unify the 'pollq' on indio_dev.
> > But, it looks a bit difficult, and would require some more change [which is
> > doable] if it makes sense for whatever reason.
> > The only reason to do it, is because the iio_buffer_fileops has a .poll =
> > iio_buffer_poll() function attached to it. Adding multiple buffers for an
> > IIO device may require some consideration on the iio_buffer_poll() function
> > as well.  
> 
> I think we need one chardev per buffer. Conceptually that is the right 
> approach in my option since the two buffers are independent streams. But 
> also from a practical point of view we want to have the ability to have 
> the buffers opened by different applications. E.g. iio_readdev on the 
> input buffer and iio_writedev on the output buffer. And there might be 
> some other operations that wont multiplex as nicely as read/write. The 
> high speed interface for example would not work as it is right now.
> 

Yup. Separate chardev is pretty much the only option given the poll infrastructure.
In theory could do the anon file trick again but I'm not sure it's worth it
- the vast majority of drivers are still going to be single buffer (I think!)

Jonathan

Re: [RFC PATCH 1/3] iio: core: rename 'indio_dev->buffer_list' -> 'indio_dev->active_buffers'

[Jonathan Cameron]

On Mon, 30 Mar 2020 17:57:03 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> Since we want to add support for attaching multiple buffers, and we want to
> add a new list to 'struct iio_dev', it's a good idea to rename the current
> 'indio->buffer_list' to 'indio_dev->active_buffers'.
> 
> Fortunately, this is a private field, which is used in
> 'drivers/iio/industrial-buffer.c', so this is simple to rename.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
This highlights that we are going to need to come up with a clearly concept than
'buffers' for both the stream of data coming in or out + the interfaces
within the kernel (consumer buffer etc).

I'm not sure what naming will work though..  For input buffers we could
use "buffers" for the incoming streams and "consumers" for where that is demuxed
to (including the kfifos etc).

For output I 'think' we are unlikely to have to deal with interleaving
inputs from multiple devices all wanting to drive their own channels?
(I hope so as that sounds horrible to deal with give frequency missmatch
etc)  So in output case it'll kind of be one block.

Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 28 ++++++++++++++--------------
>  drivers/iio/industrialio-core.c   |  2 +-
>  include/linux/iio/iio.h           |  4 ++--
>  3 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e6fa1a4e135d..a585c304cad4 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -591,7 +591,7 @@ static void iio_buffer_activate(struct iio_dev *indio_dev,
>  	struct iio_buffer *buffer)
>  {
>  	iio_buffer_get(buffer);
> -	list_add(&buffer->buffer_list, &indio_dev->buffer_list);
> +	list_add(&buffer->buffer_list, &indio_dev->active_buffers);
>  }
>  
>  static void iio_buffer_deactivate(struct iio_buffer *buffer)
> @@ -606,7 +606,7 @@ static void iio_buffer_deactivate_all(struct iio_dev *indio_dev)
>  	struct iio_buffer *buffer, *_buffer;
>  
>  	list_for_each_entry_safe(buffer, _buffer,
> -			&indio_dev->buffer_list, buffer_list)
> +			&indio_dev->active_buffers, buffer_list)
>  		iio_buffer_deactivate(buffer);
>  }
>  
> @@ -701,12 +701,12 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	 * to verify.
>  	 */
>  	if (remove_buffer && !insert_buffer &&
> -		list_is_singular(&indio_dev->buffer_list))
> +		list_is_singular(&indio_dev->active_buffers))
>  			return 0;
>  
>  	modes = indio_dev->modes;
>  
> -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
>  		if (buffer == remove_buffer)
>  			continue;
>  		modes &= buffer->access->modes;
> @@ -727,7 +727,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  		 * Keep things simple for now and only allow a single buffer to
>  		 * be connected in hardware mode.
>  		 */
> -		if (insert_buffer && !list_empty(&indio_dev->buffer_list))
> +		if (insert_buffer && !list_empty(&indio_dev->active_buffers))
>  			return -EINVAL;
>  		config->mode = INDIO_BUFFER_HARDWARE;
>  		strict_scanmask = true;
> @@ -747,7 +747,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  
>  	scan_timestamp = false;
>  
> -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
>  		if (buffer == remove_buffer)
>  			continue;
>  		bitmap_or(compound_mask, compound_mask, buffer->scan_mask,
> @@ -896,7 +896,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
>  	struct iio_buffer *buffer;
>  	int ret;
>  
> -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
>  		ret = iio_buffer_update_demux(indio_dev, buffer);
>  		if (ret < 0)
>  			goto error_clear_mux_table;
> @@ -904,7 +904,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
>  	return 0;
>  
>  error_clear_mux_table:
> -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list)
> +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list)
>  		iio_buffer_demux_free(buffer);
>  
>  	return ret;
> @@ -948,7 +948,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  		indio_dev->info->hwfifo_set_watermark(indio_dev,
>  			config->watermark);
>  
> -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
>  		ret = iio_buffer_enable(buffer, indio_dev);
>  		if (ret)
>  			goto err_disable_buffers;
> @@ -968,7 +968,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	return 0;
>  
>  err_disable_buffers:
> -	list_for_each_entry_continue_reverse(buffer, &indio_dev->buffer_list,
> +	list_for_each_entry_continue_reverse(buffer, &indio_dev->active_buffers,
>  					     buffer_list)
>  		iio_buffer_disable(buffer, indio_dev);
>  err_run_postdisable:
> @@ -988,7 +988,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>  	int ret2;
>  
>  	/* Wind down existing buffers - iff there are any */
> -	if (list_empty(&indio_dev->buffer_list))
> +	if (list_empty(&indio_dev->active_buffers))
>  		return 0;
>  
>  	/*
> @@ -1004,7 +1004,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>  			ret = ret2;
>  	}
>  
> -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
>  		ret2 = iio_buffer_disable(buffer, indio_dev);
>  		if (ret2 && !ret)
>  			ret = ret2;
> @@ -1052,7 +1052,7 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
>  		iio_buffer_activate(indio_dev, insert_buffer);
>  
>  	/* If no buffers in list, we are done */
> -	if (list_empty(&indio_dev->buffer_list))
> +	if (list_empty(&indio_dev->active_buffers))
>  		return 0;
>  
>  	ret = iio_enable_buffers(indio_dev, &new_config);
> @@ -1413,7 +1413,7 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
>  	int ret;
>  	struct iio_buffer *buf;
>  
> -	list_for_each_entry(buf, &indio_dev->buffer_list, buffer_list) {
> +	list_for_each_entry(buf, &indio_dev->active_buffers, buffer_list) {
>  		ret = iio_push_to_buffer(buf, data);
>  		if (ret < 0)
>  			return ret;
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 157d95a24faa..a13957644c1d 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1523,7 +1523,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
>  			return NULL;
>  		}
>  		dev_set_name(&dev->dev, "iio:device%d", dev->id);
> -		INIT_LIST_HEAD(&dev->buffer_list);
> +		INIT_LIST_HEAD(&dev->active_buffers);
>  	}
>  
>  	return dev;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index e975020abaa6..a123f8acb192 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -490,7 +490,7 @@ struct iio_buffer_setup_ops {
>   *			and owner
>   * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
>   * @buffer:		[DRIVER] any buffer present
> - * @buffer_list:	[INTERN] list of all buffers currently attached
> + * @active_buffers:	[INTERN] list of all buffers currently enabled/active
>   * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
>   * @mlock:		[INTERN] lock used to prevent simultaneous device state
>   *			changes
> @@ -534,7 +534,7 @@ struct iio_dev {
>  	struct iio_event_interface	*event_interface;
>  
>  	struct iio_buffer		*buffer;
> -	struct list_head		buffer_list;
> +	struct list_head		active_buffers;
>  	int				scan_bytes;
>  	struct mutex			mlock;
>  

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Lars-Peter Clausen]

On 4/5/20 11:49 AM, Jonathan Cameron wrote:
>>> 3. A side question is about the 'iio_buffer -> pollq' field. I was
>>> wondering if it would make sense to move that on to 'iio_dev  pollq' if
>>> adding multiple buffers are added per-device. It almost makes sense to
>>> unify the 'pollq' on indio_dev.
>>> But, it looks a bit difficult, and would require some more change [which is
>>> doable] if it makes sense for whatever reason.
>>> The only reason to do it, is because the iio_buffer_fileops has a .poll =
>>> iio_buffer_poll() function attached to it. Adding multiple buffers for an
>>> IIO device may require some consideration on the iio_buffer_poll() function
>>> as well.
>> I think we need one chardev per buffer. Conceptually that is the right
>> approach in my option since the two buffers are independent streams. But
>> also from a practical point of view we want to have the ability to have
>> the buffers opened by different applications. E.g. iio_readdev on the
>> input buffer and iio_writedev on the output buffer. And there might be
>> some other operations that wont multiplex as nicely as read/write. The
>> high speed interface for example would not work as it is right now.
>>
> Yup. Separate chardev is pretty much the only option given the poll infrastructure.
> In theory could do the anon file trick again but I'm not sure it's worth it
> - the vast majority of drivers are still going to be single buffer (I think!)
The main problem I see with the anon file trick is that we use the open 
file also as mutual exclusion so only one application can access a 
buffer at a time. This means if one application has the main chardev 
open no other application will be able to access the buffers (or events).

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Lars-Peter Clausen]

On 4/5/20 11:57 AM, Lars-Peter Clausen wrote:
> On 4/5/20 11:49 AM, Jonathan Cameron wrote:
>>>> 3. A side question is about the 'iio_buffer -> pollq' field. I was
>>>> wondering if it would make sense to move that on to 'iio_dev  
>>>> pollq' if
>>>> adding multiple buffers are added per-device. It almost makes sense to
>>>> unify the 'pollq' on indio_dev.
>>>> But, it looks a bit difficult, and would require some more change 
>>>> [which is
>>>> doable] if it makes sense for whatever reason.
>>>> The only reason to do it, is because the iio_buffer_fileops has a 
>>>> .poll =
>>>> iio_buffer_poll() function attached to it. Adding multiple buffers 
>>>> for an
>>>> IIO device may require some consideration on the iio_buffer_poll() 
>>>> function
>>>> as well.
>>> I think we need one chardev per buffer. Conceptually that is the right
>>> approach in my option since the two buffers are independent streams. 
>>> But
>>> also from a practical point of view we want to have the ability to have
>>> the buffers opened by different applications. E.g. iio_readdev on the
>>> input buffer and iio_writedev on the output buffer. And there might be
>>> some other operations that wont multiplex as nicely as read/write. The
>>> high speed interface for example would not work as it is right now.
>>>
>> Yup. Separate chardev is pretty much the only option given the poll 
>> infrastructure.
>> In theory could do the anon file trick again but I'm not sure it's 
>> worth it
>> - the vast majority of drivers are still going to be single buffer (I 
>> think!)
> The main problem I see with the anon file trick is that we use the 
> open file also as mutual exclusion so only one application can access 
> a buffer at a time. This means if one application has the main chardev 
> open no other application will be able to access the buffers (or events).

And of course also that you can use `cat`, `echo`, `dd` and so on.

Re: [RFC PATCH 3/3] iio: buffer: add output buffer support for chrdev

[Jonathan Cameron]

On Sun, 5 Apr 2020 11:58:38 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 4/5/20 11:57 AM, Lars-Peter Clausen wrote:
> > On 4/5/20 11:49 AM, Jonathan Cameron wrote:  
> >>>> 3. A side question is about the 'iio_buffer -> pollq' field. I was
> >>>> wondering if it would make sense to move that on to 'iio_dev  
> >>>> pollq' if
> >>>> adding multiple buffers are added per-device. It almost makes sense to
> >>>> unify the 'pollq' on indio_dev.
> >>>> But, it looks a bit difficult, and would require some more change 
> >>>> [which is
> >>>> doable] if it makes sense for whatever reason.
> >>>> The only reason to do it, is because the iio_buffer_fileops has a 
> >>>> .poll =
> >>>> iio_buffer_poll() function attached to it. Adding multiple buffers 
> >>>> for an
> >>>> IIO device may require some consideration on the iio_buffer_poll() 
> >>>> function
> >>>> as well.  
> >>> I think we need one chardev per buffer. Conceptually that is the right
> >>> approach in my option since the two buffers are independent streams. 
> >>> But
> >>> also from a practical point of view we want to have the ability to have
> >>> the buffers opened by different applications. E.g. iio_readdev on the
> >>> input buffer and iio_writedev on the output buffer. And there might be
> >>> some other operations that wont multiplex as nicely as read/write. The
> >>> high speed interface for example would not work as it is right now.
> >>>  
> >> Yup. Separate chardev is pretty much the only option given the poll 
> >> infrastructure.
> >> In theory could do the anon file trick again but I'm not sure it's 
> >> worth it
> >> - the vast majority of drivers are still going to be single buffer (I 
> >> think!)  
> > The main problem I see with the anon file trick is that we use the 
> > open file also as mutual exclusion so only one application can access 
> > a buffer at a time. This means if one application has the main chardev 
> > open no other application will be able to access the buffers (or events).  
> 
> And of course also that you can use `cat`, `echo`, `dd` and so on.

True on both counts. For events I'm don't think this restriction really matters
because they are normally fairly tightly coupled to the data stream coming in
etc (though we should do an in kernel consumer at somepoint).  I did have a plan
a long time ago to allow additional kfifos to be attached as consumer devices
to allow more complex multi user cases if needed but never implemented it.

Lets not go down the anon route for input vs output buffers as seems entirely
reasonable that they might be used by separate programs.

Jonathan

> 

Re: [RFC PATCH 1/3] iio: core: rename 'indio_dev->buffer_list' -> 'indio_dev->active_buffers'

[Ardelean, Alexandru]

On Sun, 2020-04-05 at 10:56 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Mon, 30 Mar 2020 17:57:03 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > Since we want to add support for attaching multiple buffers, and we want to
> > add a new list to 'struct iio_dev', it's a good idea to rename the current
> > 'indio->buffer_list' to 'indio_dev->active_buffers'.
> > 
> > Fortunately, this is a private field, which is used in
> > 'drivers/iio/industrial-buffer.c', so this is simple to rename.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> This highlights that we are going to need to come up with a clearly concept
> than
> 'buffers' for both the stream of data coming in or out + the interfaces
> within the kernel (consumer buffer etc).
> 
> I'm not sure what naming will work though..  For input buffers we could
> use "buffers" for the incoming streams and "consumers" for where that is
> demuxed
> to (including the kfifos etc).
> 
> For output I 'think' we are unlikely to have to deal with interleaving
> inputs from multiple devices all wanting to drive their own channels?
> (I hope so as that sounds horrible to deal with give frequency missmatch
> etc)  So in output case it'll kind of be one block.

Hmm, I'm still vague about this.
And trying to workout how things already work, to be able to find a route to the
multiple-buffers stuff.

This patch was an initial thought [I had] about how to go directly for multiple
buffers, and one idea was to have 'indio_dev->attached_buffers' which would
replace 'indio_dev->buffer'.
Then a new field called 'iio_buffer->direction' would be added to determine
direction.

In any case, it won't be at the start of the series, since I can't really
motivate this better than "it helps me grep easier for 'indio_dev->buffer' &
'indio_dev->buffer_list'".


> 
> Jonathan
> 
> > ---
> >  drivers/iio/industrialio-buffer.c | 28 ++++++++++++++--------------
> >  drivers/iio/industrialio-core.c   |  2 +-
> >  include/linux/iio/iio.h           |  4 ++--
> >  3 files changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index e6fa1a4e135d..a585c304cad4 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -591,7 +591,7 @@ static void iio_buffer_activate(struct iio_dev
> > *indio_dev,
> >  	struct iio_buffer *buffer)
> >  {
> >  	iio_buffer_get(buffer);
> > -	list_add(&buffer->buffer_list, &indio_dev->buffer_list);
> > +	list_add(&buffer->buffer_list, &indio_dev->active_buffers);
> >  }
> >  
> >  static void iio_buffer_deactivate(struct iio_buffer *buffer)
> > @@ -606,7 +606,7 @@ static void iio_buffer_deactivate_all(struct iio_dev
> > *indio_dev)
> >  	struct iio_buffer *buffer, *_buffer;
> >  
> >  	list_for_each_entry_safe(buffer, _buffer,
> > -			&indio_dev->buffer_list, buffer_list)
> > +			&indio_dev->active_buffers, buffer_list)
> >  		iio_buffer_deactivate(buffer);
> >  }
> >  
> > @@ -701,12 +701,12 @@ static int iio_verify_update(struct iio_dev
> > *indio_dev,
> >  	 * to verify.
> >  	 */
> >  	if (remove_buffer && !insert_buffer &&
> > -		list_is_singular(&indio_dev->buffer_list))
> > +		list_is_singular(&indio_dev->active_buffers))
> >  			return 0;
> >  
> >  	modes = indio_dev->modes;
> >  
> > -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> > +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
> >  		if (buffer == remove_buffer)
> >  			continue;
> >  		modes &= buffer->access->modes;
> > @@ -727,7 +727,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
> >  		 * Keep things simple for now and only allow a single buffer to
> >  		 * be connected in hardware mode.
> >  		 */
> > -		if (insert_buffer && !list_empty(&indio_dev->buffer_list))
> > +		if (insert_buffer && !list_empty(&indio_dev->active_buffers))
> >  			return -EINVAL;
> >  		config->mode = INDIO_BUFFER_HARDWARE;
> >  		strict_scanmask = true;
> > @@ -747,7 +747,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
> >  
> >  	scan_timestamp = false;
> >  
> > -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> > +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
> >  		if (buffer == remove_buffer)
> >  			continue;
> >  		bitmap_or(compound_mask, compound_mask, buffer->scan_mask,
> > @@ -896,7 +896,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
> >  	struct iio_buffer *buffer;
> >  	int ret;
> >  
> > -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> > +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
> >  		ret = iio_buffer_update_demux(indio_dev, buffer);
> >  		if (ret < 0)
> >  			goto error_clear_mux_table;
> > @@ -904,7 +904,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
> >  	return 0;
> >  
> >  error_clear_mux_table:
> > -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list)
> > +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list)
> >  		iio_buffer_demux_free(buffer);
> >  
> >  	return ret;
> > @@ -948,7 +948,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >  		indio_dev->info->hwfifo_set_watermark(indio_dev,
> >  			config->watermark);
> >  
> > -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> > +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
> >  		ret = iio_buffer_enable(buffer, indio_dev);
> >  		if (ret)
> >  			goto err_disable_buffers;
> > @@ -968,7 +968,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >  	return 0;
> >  
> >  err_disable_buffers:
> > -	list_for_each_entry_continue_reverse(buffer, &indio_dev->buffer_list,
> > +	list_for_each_entry_continue_reverse(buffer, &indio_dev->active_buffers,
> >  					     buffer_list)
> >  		iio_buffer_disable(buffer, indio_dev);
> >  err_run_postdisable:
> > @@ -988,7 +988,7 @@ static int iio_disable_buffers(struct iio_dev
> > *indio_dev)
> >  	int ret2;
> >  
> >  	/* Wind down existing buffers - iff there are any */
> > -	if (list_empty(&indio_dev->buffer_list))
> > +	if (list_empty(&indio_dev->active_buffers))
> >  		return 0;
> >  
> >  	/*
> > @@ -1004,7 +1004,7 @@ static int iio_disable_buffers(struct iio_dev
> > *indio_dev)
> >  			ret = ret2;
> >  	}
> >  
> > -	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> > +	list_for_each_entry(buffer, &indio_dev->active_buffers, buffer_list) {
> >  		ret2 = iio_buffer_disable(buffer, indio_dev);
> >  		if (ret2 && !ret)
> >  			ret = ret2;
> > @@ -1052,7 +1052,7 @@ static int __iio_update_buffers(struct iio_dev
> > *indio_dev,
> >  		iio_buffer_activate(indio_dev, insert_buffer);
> >  
> >  	/* If no buffers in list, we are done */
> > -	if (list_empty(&indio_dev->buffer_list))
> > +	if (list_empty(&indio_dev->active_buffers))
> >  		return 0;
> >  
> >  	ret = iio_enable_buffers(indio_dev, &new_config);
> > @@ -1413,7 +1413,7 @@ int iio_push_to_buffers(struct iio_dev *indio_dev,
> > const void *data)
> >  	int ret;
> >  	struct iio_buffer *buf;
> >  
> > -	list_for_each_entry(buf, &indio_dev->buffer_list, buffer_list) {
> > +	list_for_each_entry(buf, &indio_dev->active_buffers, buffer_list) {
> >  		ret = iio_push_to_buffer(buf, data);
> >  		if (ret < 0)
> >  			return ret;
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index 157d95a24faa..a13957644c1d 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1523,7 +1523,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> >  			return NULL;
> >  		}
> >  		dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > -		INIT_LIST_HEAD(&dev->buffer_list);
> > +		INIT_LIST_HEAD(&dev->active_buffers);
> >  	}
> >  
> >  	return dev;
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index e975020abaa6..a123f8acb192 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -490,7 +490,7 @@ struct iio_buffer_setup_ops {
> >   *			and owner
> >   * @event_interface:	[INTERN] event chrdevs associated with interrupt
> > lines
> >   * @buffer:		[DRIVER] any buffer present
> > - * @buffer_list:	[INTERN] list of all buffers currently attached
> > + * @active_buffers:	[INTERN] list of all buffers currently
> > enabled/active
> >   * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer
> > demux
> >   * @mlock:		[INTERN] lock used to prevent simultaneous device state
> >   *			changes
> > @@ -534,7 +534,7 @@ struct iio_dev {
> >  	struct iio_event_interface	*event_interface;
> >  
> >  	struct iio_buffer		*buffer;
> > -	struct list_head		buffer_list;
> > +	struct list_head		active_buffers;
> >  	int				scan_bytes;
> >  	struct mutex			mlock;
> >  

[RFC PATCH 2/3] iio: buffer: extend short-hand use for 'indio_dev->buffer'

[Alexandru Ardelean]

This change is both cosmetic and a prequel to adding support for attaching
multiple buffers per IIO device.

The IIO buffer sysfs attrs are mostly designed to support only one attached
buffer, and in order to support more, we need to centralize [in each attr
function] the buffer which is being accessed.

This also makes it a bit more uniform, as in some functions there is a
short-hand 'buffer' variable and at the same time the 'indio_dev->buffer'
is still access directly.

In the 'iio_buffer_add_channel_sysfs()' the 'buffer' is passed as a
parameter. This gives control to 'iio_buffer_alloc_sysfs_and_mask()' over
which buffer gets accessed.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 55 +++++++++++++++++--------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a585c304cad4..c6af18448dd5 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -262,10 +262,11 @@ static ssize_t iio_scan_el_show(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
 
 	/* Ensure ret is 0 or 1. */
 	ret = !!test_bit(to_iio_dev_attr(attr)->address,
-		       indio_dev->buffer->scan_mask);
+		       buffer->scan_mask);
 
 	return sprintf(buf, "%d\n", ret);
 }
@@ -381,7 +382,7 @@ static ssize_t iio_scan_el_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
 	}
@@ -410,7 +411,9 @@ static ssize_t iio_scan_el_ts_show(struct device *dev,
 				   char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	return sprintf(buf, "%d\n", indio_dev->buffer->scan_timestamp);
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	return sprintf(buf, "%d\n", buffer->scan_timestamp);
 }
 
 static ssize_t iio_scan_el_ts_store(struct device *dev,
@@ -420,6 +423,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
 	bool state;
 
 	ret = strtobool(buf, &state);
@@ -427,11 +431,11 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 		return ret;
 
 	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
 	}
-	indio_dev->buffer->scan_timestamp = state;
+	buffer->scan_timestamp = state;
 error_ret:
 	mutex_unlock(&indio_dev->mlock);
 
@@ -439,10 +443,10 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 }
 
 static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
+					struct iio_buffer *buffer,
 					const struct iio_chan_spec *chan)
 {
 	int ret, attrcount = 0;
-	struct iio_buffer *buffer = indio_dev->buffer;
 
 	ret = __iio_add_chan_devattr("index",
 				     chan,
@@ -518,7 +522,7 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 		return len;
 
 	mutex_lock(&indio_dev->mlock);
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 	} else {
 		buffer->access->set_length(buffer, val);
@@ -539,7 +543,9 @@ static ssize_t iio_buffer_show_enable(struct device *dev,
 				      char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	return sprintf(buf, "%d\n", iio_buffer_is_active(indio_dev->buffer));
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
 }
 
 static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev,
@@ -1129,6 +1135,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	int ret;
 	bool requested_state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
 	bool inlist;
 
 	ret = strtobool(buf, &requested_state);
@@ -1138,17 +1145,15 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	mutex_lock(&indio_dev->mlock);
 
 	/* Find out if it is in the list */
-	inlist = iio_buffer_is_active(indio_dev->buffer);
+	inlist = iio_buffer_is_active(buffer);
 	/* Already in desired state */
 	if (inlist == requested_state)
 		goto done;
 
 	if (requested_state)
-		ret = __iio_update_buffers(indio_dev,
-					 indio_dev->buffer, NULL);
+		ret = __iio_update_buffers(indio_dev, buffer, NULL);
 	else
-		ret = __iio_update_buffers(indio_dev,
-					 NULL, indio_dev->buffer);
+		ret = __iio_update_buffers(indio_dev, NULL, buffer);
 
 done:
 	mutex_unlock(&indio_dev->mlock);
@@ -1190,7 +1195,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 		goto out;
 	}
 
-	if (iio_buffer_is_active(indio_dev->buffer)) {
+	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -1207,11 +1212,9 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
 						char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	size_t bytes;
-
-	bytes = iio_buffer_data_available(indio_dev->buffer);
+	struct iio_buffer *buffer = indio_dev->buffer;
 
-	return sprintf(buf, "%zu\n", bytes);
+	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
@@ -1297,7 +1300,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 			if (channels[i].scan_index < 0)
 				continue;
 
-			ret = iio_buffer_add_channel_sysfs(indio_dev,
+			ret = iio_buffer_add_channel_sysfs(indio_dev, buffer,
 							 &channels[i]);
 			if (ret < 0)
 				goto error_cleanup_dynamic;
@@ -1340,20 +1343,22 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	bitmap_free(buffer->scan_mask);
 error_cleanup_dynamic:
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
-	kfree(indio_dev->buffer->buffer_group.attrs);
+	kfree(buffer->buffer_group.attrs);
 
 	return ret;
 }
 
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 {
-	if (!indio_dev->buffer)
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	if (!buffer)
 		return;
 
-	bitmap_free(indio_dev->buffer->scan_mask);
-	kfree(indio_dev->buffer->buffer_group.attrs);
-	kfree(indio_dev->buffer->scan_el_group.attrs);
-	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
+	bitmap_free(buffer->scan_mask);
+	kfree(buffer->buffer_group.attrs);
+	kfree(buffer->scan_el_group.attrs);
+	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
 }
 
 /**
-- 
2.20.1