[RFC PATCH 0/4] iio: scan_mask rework to track enabled channels on per-channel basis

[RFC PATCH 0/4] iio: scan_mask rework to track enabled channels on per-channel basis

[Alexandru Ardelean]

From my side, I'll admit that the specific use-cases for these patches
are a bit outside of my scope of understanding.
I did my best to re-apply them on a newer tree, and dig-up the
information from the ADI tree [where they've been living for a while
now].

Also, I don't have any idea if there was a prior discussion about this.
I could not find anything on a quick search.

I'm hoping that the author would have some input on them/

Hence the RFC.

Lars-Peter Clausen (4):
  iio: Move scan mask management to the core
  iio: hw_consumer: use new scanmask functions
  iio: Allow channels to share storage elements
  iio: Track enabled channels on a per channel basis

 drivers/iio/buffer/industrialio-buffer-cb.c   | 17 ++--
 drivers/iio/buffer/industrialio-hw-consumer.c | 19 +++-
 drivers/iio/industrialio-buffer.c             | 98 +++++++++++++------
 drivers/iio/industrialio-core.c               | 27 +++--
 drivers/iio/inkern.c                          | 30 ++++++
 include/linux/iio/buffer_impl.h               |  3 +
 include/linux/iio/consumer.h                  | 12 +++
 7 files changed, 154 insertions(+), 52 deletions(-)

-- 
2.17.1

[RFC PATCH 1/4] iio: Move scan mask management to the core

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Let the core handle the buffer scan mask management including allocation
and channel selection. Having this handled in a central place rather than
open-coding it all over the place will make it easier to change the
implementation.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/buffer/industrialio-buffer-cb.c | 17 ++++------
 drivers/iio/industrialio-buffer.c           | 36 +++++++++++++++------
 drivers/iio/inkern.c                        | 15 +++++++++
 include/linux/iio/consumer.h                | 10 ++++++
 4 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
index 47c96f7f4976..b50f1f48cac6 100644
--- a/drivers/iio/buffer/industrialio-buffer-cb.c
+++ b/drivers/iio/buffer/industrialio-buffer-cb.c
@@ -33,8 +33,7 @@ static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
 static void iio_buffer_cb_release(struct iio_buffer *buffer)
 {
 	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
-
-	bitmap_free(cb_buff->buffer.scan_mask);
+	iio_buffer_free_scanmask(buffer);
 	kfree(cb_buff);
 }
 
@@ -72,27 +71,25 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
 	}
 
 	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
-	cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev->masklength,
-						  GFP_KERNEL);
-	if (cb_buff->buffer.scan_mask == NULL) {
-		ret = -ENOMEM;
+
+	ret = iio_buffer_alloc_scanmask(&cb_buff->buffer, cb_buff->indio_dev);
+	if (ret)
 		goto error_release_channels;
-	}
+
 	chan = &cb_buff->channels[0];
 	while (chan->indio_dev) {
 		if (chan->indio_dev != cb_buff->indio_dev) {
 			ret = -EINVAL;
 			goto error_free_scan_mask;
 		}
-		set_bit(chan->channel->scan_index,
-			cb_buff->buffer.scan_mask);
+		iio_buffer_channel_enable(&cb_buff->buffer, chan);
 		chan++;
 	}
 
 	return cb_buff;
 
 error_free_scan_mask:
-	bitmap_free(cb_buff->buffer.scan_mask);
+	iio_buffer_free_scanmask(&cb_buff->buffer);
 error_release_channels:
 	iio_channel_release_all(cb_buff->channels);
 error_free_cb_buff:
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 221157136af6..c06691281287 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -206,6 +206,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL(iio_buffer_init);
 
+int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
+			      struct iio_dev *indio_dev)
+{
+	if (!indio_dev->masklength)
+		return 0;
+
+	buffer->scan_mask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
+	if (buffer->scan_mask == NULL)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
+
+void iio_buffer_free_scanmask(struct iio_buffer *buffer)
+{
+	bitmap_free(buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
+
 /**
  * iio_buffer_set_attrs - Set buffer specific attributes
  * @buffer: The buffer for which we are setting attributes
@@ -1301,14 +1321,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 				indio_dev->scan_index_timestamp =
 					channels[i].scan_index;
 		}
-		if (indio_dev->masklength && buffer->scan_mask == NULL) {
-			buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
-							  GFP_KERNEL);
-			if (buffer->scan_mask == NULL) {
-				ret = -ENOMEM;
-				goto error_cleanup_dynamic;
-			}
-		}
+
+		ret = iio_buffer_alloc_scanmask(buffer, indio_dev);
+		if (ret)
+			goto error_cleanup_dynamic;
 	}
 
 	buffer->scan_el_group.name = iio_scan_elements_group_name;
@@ -1329,7 +1345,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	return 0;
 
 error_free_scan_mask:
-	bitmap_free(buffer->scan_mask);
+	iio_buffer_free_scanmask(buffer);
 error_cleanup_dynamic:
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
 	kfree(indio_dev->buffer->buffer_group.attrs);
@@ -1342,7 +1358,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!indio_dev->buffer)
 		return;
 
-	bitmap_free(indio_dev->buffer->scan_mask);
+	iio_buffer_free_scanmask(indio_dev->buffer);
 	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);
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index ede99e0d5371..f35cb9985edc 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -11,6 +11,7 @@
 
 #include <linux/iio/iio.h>
 #include "iio_core.h"
+#include <linux/iio/buffer_impl.h>
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
 #include <linux/iio/consumer.h>
@@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
 }
 EXPORT_SYMBOL_GPL(iio_write_channel_raw);
 
+void iio_buffer_channel_enable(struct iio_buffer *buffer,
+			       const struct iio_channel *chan)
+{
+	set_bit(chan->channel->scan_index, buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
+
+void iio_buffer_channel_disable(struct iio_buffer *buffer,
+				const struct iio_channel *chan)
+{
+	clear_bit(chan->channel->scan_index, buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
+
 unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
 {
 	const struct iio_chan_spec_ext_info *ext_info;
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index c4118dcb8e05..dbc87c26250a 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -12,6 +12,7 @@
 
 struct iio_dev;
 struct iio_chan_spec;
+struct iio_buffer;
 struct device;
 
 /**
@@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
 int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
 	int *processed, unsigned int scale);
 
+void iio_buffer_channel_enable(struct iio_buffer *buffer,
+			       const struct iio_channel *chan);
+void iio_buffer_channel_disable(struct iio_buffer *buffer,
+				const struct iio_channel *chan);
+
+int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
+			      struct iio_dev *indio_dev);
+void iio_buffer_free_scanmask(struct iio_buffer *buffer);
+
 /**
  * iio_get_channel_ext_info_count() - get number of ext_info attributes
  *				      connected to the channel.
-- 
2.17.1

[RFC PATCH 2/4] iio: hw_consumer: use new scanmask functions

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

This change moves the handling of the scanmasks to the new wrapper
functions that were added in a previous commit.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/buffer/industrialio-hw-consumer.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-hw-consumer.c b/drivers/iio/buffer/industrialio-hw-consumer.c
index f2d27788f666..f1cc72520685 100644
--- a/drivers/iio/buffer/industrialio-hw-consumer.c
+++ b/drivers/iio/buffer/industrialio-hw-consumer.c
@@ -28,7 +28,6 @@ struct hw_consumer_buffer {
 	struct list_head head;
 	struct iio_dev *indio_dev;
 	struct iio_buffer buffer;
-	long scan_mask[];
 };
 
 static struct hw_consumer_buffer *iio_buffer_to_hw_consumer_buffer(
@@ -41,6 +40,8 @@ static void iio_hw_buf_release(struct iio_buffer *buffer)
 {
 	struct hw_consumer_buffer *hw_buf =
 		iio_buffer_to_hw_consumer_buffer(buffer);
+
+	iio_buffer_free_scanmask(buffer);
 	kfree(hw_buf);
 }
 
@@ -52,26 +53,34 @@ static const struct iio_buffer_access_funcs iio_hw_buf_access = {
 static struct hw_consumer_buffer *iio_hw_consumer_get_buffer(
 	struct iio_hw_consumer *hwc, struct iio_dev *indio_dev)
 {
-	size_t mask_size = BITS_TO_LONGS(indio_dev->masklength) * sizeof(long);
 	struct hw_consumer_buffer *buf;
+	int ret;
 
 	list_for_each_entry(buf, &hwc->buffers, head) {
 		if (buf->indio_dev == indio_dev)
 			return buf;
 	}
 
-	buf = kzalloc(sizeof(*buf) + mask_size, GFP_KERNEL);
+	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return NULL;
 
 	buf->buffer.access = &iio_hw_buf_access;
 	buf->indio_dev = indio_dev;
-	buf->buffer.scan_mask = buf->scan_mask;
 
 	iio_buffer_init(&buf->buffer);
+
+	ret = iio_buffer_alloc_scanmask(&buf->buffer, indio_dev);
+	if (ret)
+		goto err_free_buf;
+
 	list_add_tail(&buf->head, &hwc->buffers);
 
 	return buf;
+
+err_free_buf:
+	kfree(buf);
+	return NULL;
 }
 
 /**
@@ -106,7 +115,7 @@ struct iio_hw_consumer *iio_hw_consumer_alloc(struct device *dev)
 			ret = -ENOMEM;
 			goto err_put_buffers;
 		}
-		set_bit(chan->channel->scan_index, buf->buffer.scan_mask);
+		iio_buffer_channel_enable(&buf->buffer, chan);
 		chan++;
 	}
 
-- 
2.17.1

[RFC PATCH 3/4] iio: Allow channels to share storage elements

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Currently each IIO channel has it's own storage element in the data stream
each with its own unique scan index. This works for a lot of use-cases,
but in some is not good enough to represent the hardware accurately. On
those devices multiple separate pieces of information are stored within the
same storage element and the storage element can't be further broken down
into multiple storage elements (e.g. because the data is not aligned).

This can for example be status bits stored in unused data bits. E.g. a
14-bit ADC that stores its data in a 16-bit word and uses the two
additional bits to convey status information like for example whether a
overrange condition has happened. Currently this kind of extra status
information is usually ignored and can only be used by applications that
have special knowledge about the connected device and its data layout.

In addition to that some might also have data channels with less than 8
bits that get packed into the same storage element.

Allow two or more channels to use the same scan index, if they their
storage element does have the same size.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f4daf19f2a3b..cdf59a51c917 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1651,6 +1651,16 @@ static const struct file_operations iio_buffer_fileops = {
 	.compat_ioctl = compat_ptr_ioctl,
 };
 
+static bool iio_chan_same_size(const struct iio_chan_spec *a,
+	const struct iio_chan_spec *b)
+{
+	if (a->scan_type.storagebits != b->scan_type.storagebits)
+		return false;
+	if (a->scan_type.repeat != b->scan_type.repeat)
+		return false;
+	return true;
+}
+
 static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
 {
 	int i, j;
@@ -1662,13 +1672,16 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
 	for (i = 0; i < indio_dev->num_channels - 1; i++) {
 		if (channels[i].scan_index < 0)
 			continue;
-		for (j = i + 1; j < indio_dev->num_channels; j++)
-			if (channels[i].scan_index == channels[j].scan_index) {
-				dev_err(&indio_dev->dev,
-					"Duplicate scan index %d\n",
-					channels[i].scan_index);
-				return -EINVAL;
-			}
+		for (j = i + 1; j < indio_dev->num_channels; j++) {
+			if (channels[i].scan_index != channels[j].scan_index)
+				continue;
+			if (iio_chan_same_size(&channels[i], &channels[j]))
+				continue;
+			dev_err(&indio_dev->dev,
+				"Duplicate scan index %d\n",
+				channels[i].scan_index);
+			return -EINVAL;
+		}
 	}
 
 	return 0;
-- 
2.17.1

[RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Now that we support multiple channels with the same scan index we can no
longer use the scan mask to track which channels have been enabled.
Otherwise it is not possible to enable channels with the same scan index
independently.

Introduce a new channel mask which is used instead of the scan mask to
track which channels are enabled. Whenever the channel mask is changed a
new scan mask is computed based on it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
 drivers/iio/inkern.c              | 19 +++++++++-
 include/linux/iio/buffer_impl.h   |  3 ++
 include/linux/iio/consumer.h      |  2 +
 4 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index c06691281287..1821a3e32fb3 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
 	if (buffer->scan_mask == NULL)
 		return -ENOMEM;
 
+	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
+					     GFP_KERNEL);
+	if (buffer->channel_mask == NULL) {
+		bitmap_free(buffer->scan_mask);
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
 
 void iio_buffer_free_scanmask(struct iio_buffer *buffer)
 {
+	bitmap_free(buffer->channel_mask);
 	bitmap_free(buffer->scan_mask);
 }
 EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
@@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
 
 	/* Ensure ret is 0 or 1. */
 	ret = !!test_bit(to_iio_dev_attr(attr)->address,
-		       indio_dev->buffer->scan_mask);
+		       indio_dev->buffer->channel_mask);
 
 	return sprintf(buf, "%d\n", ret);
 }
@@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct iio_dev *indio_dev,
  * buffers might request, hence this code only verifies that the
  * individual buffers request is plausible.
  */
-static int iio_scan_mask_set(struct iio_dev *indio_dev,
-		      struct iio_buffer *buffer, int bit)
+static int iio_channel_mask_set(struct iio_dev *indio_dev,
+				struct iio_buffer *buffer, int bit)
 {
 	const unsigned long *mask;
 	unsigned long *trialmask;
+	unsigned int ch;
 
 	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
 	if (trialmask == NULL)
@@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
 		WARN(1, "Trying to set scanmask prior to registering buffer\n");
 		goto err_invalid_mask;
 	}
-	bitmap_copy(trialmask, buffer->scan_mask, indio_dev->masklength);
-	set_bit(bit, trialmask);
+
+	set_bit(bit, buffer->channel_mask);
+
+	for_each_set_bit(ch, buffer->channel_mask, indio_dev->num_channels)
+		set_bit(indio_dev->channels[ch].scan_index, trialmask);
 
 	if (!iio_validate_scan_mask(indio_dev, trialmask))
 		goto err_invalid_mask;
@@ -363,28 +375,37 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
 	return 0;
 
 err_invalid_mask:
+	clear_bit(bit, buffer->channel_mask);
 	bitmap_free(trialmask);
 	return -EINVAL;
 }
 
-static int iio_scan_mask_clear(struct iio_buffer *buffer, int bit)
+static int iio_channel_mask_clear(struct iio_dev *indio_dev,
+				  struct iio_buffer *buffer, int bit)
 {
-	clear_bit(bit, buffer->scan_mask);
+	unsigned int ch;
+
+	clear_bit(bit, buffer->channel_mask);
+
+	bitmap_clear(buffer->scan_mask, 0, indio_dev->masklength);
+
+	for_each_set_bit(ch, buffer->channel_mask, indio_dev->num_channels)
+		set_bit(indio_dev->channels[ch].scan_index, buffer->scan_mask);
 	return 0;
 }
 
-static int iio_scan_mask_query(struct iio_dev *indio_dev,
-			       struct iio_buffer *buffer, int bit)
+static int iio_channel_mask_query(struct iio_dev *indio_dev,
+				 struct iio_buffer *buffer, int bit)
 {
-	if (bit > indio_dev->masklength)
+	if (bit > indio_dev->num_channels)
 		return -EINVAL;
 
-	if (!buffer->scan_mask)
+	if (!buffer->channel_mask)
 		return 0;
 
 	/* Ensure return value is 0 or 1. */
-	return !!test_bit(bit, buffer->scan_mask);
-};
+	return !!test_bit(bit, buffer->channel_mask);
+}
 
 static ssize_t iio_scan_el_store(struct device *dev,
 				 struct device_attribute *attr,
@@ -405,15 +426,15 @@ static ssize_t iio_scan_el_store(struct device *dev,
 		ret = -EBUSY;
 		goto error_ret;
 	}
-	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
+	ret = iio_channel_mask_query(indio_dev, buffer, this_attr->address);
 	if (ret < 0)
 		goto error_ret;
 	if (!state && ret) {
-		ret = iio_scan_mask_clear(buffer, this_attr->address);
+		ret = iio_channel_mask_clear(indio_dev, buffer, this_attr->address);
 		if (ret)
 			goto error_ret;
 	} else if (state && !ret) {
-		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
+		ret = iio_channel_mask_set(indio_dev, buffer, this_attr->address);
 		if (ret)
 			goto error_ret;
 	}
@@ -459,7 +480,8 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 }
 
 static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
-					const struct iio_chan_spec *chan)
+					const struct iio_chan_spec *chan,
+					unsigned int address)
 {
 	int ret, attrcount = 0;
 	struct iio_buffer *buffer = indio_dev->buffer;
@@ -491,7 +513,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 					     chan,
 					     &iio_scan_el_show,
 					     &iio_scan_el_store,
-					     chan->scan_index,
+					     address,
 					     0,
 					     &indio_dev->dev,
 					     &buffer->scan_el_dev_attr_list);
@@ -500,7 +522,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 					     chan,
 					     &iio_scan_el_ts_show,
 					     &iio_scan_el_ts_store,
-					     chan->scan_index,
+					     address,
 					     0,
 					     &indio_dev->dev,
 					     &buffer->scan_el_dev_attr_list);
@@ -1313,7 +1335,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 				continue;
 
 			ret = iio_buffer_add_channel_sysfs(indio_dev,
-							 &channels[i]);
+							 &channels[i], i);
 			if (ret < 0)
 				goto error_cleanup_dynamic;
 			attrcount += ret;
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index f35cb9985edc..57cf4b01c403 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -150,6 +150,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
 	if (index < 0)
 		goto err_put;
 	channel->channel = &indio_dev->channels[index];
+	channel->channel_index = index;
 
 	return 0;
 
@@ -861,14 +862,28 @@ EXPORT_SYMBOL_GPL(iio_write_channel_raw);
 void iio_buffer_channel_enable(struct iio_buffer *buffer,
 			       const struct iio_channel *chan)
 {
-	set_bit(chan->channel->scan_index, buffer->scan_mask);
+	unsigned int ch;
+
+	set_bit(chan->channel_index, buffer->channel_mask);
+
+	bitmap_clear(buffer->scan_mask, 0, chan->indio_dev->masklength);
+
+	for_each_set_bit(ch, buffer->channel_mask, chan->indio_dev->num_channels)
+		set_bit(chan->indio_dev->channels[ch].scan_index, buffer->scan_mask);
 }
 EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
 
 void iio_buffer_channel_disable(struct iio_buffer *buffer,
 				const struct iio_channel *chan)
 {
-	clear_bit(chan->channel->scan_index, buffer->scan_mask);
+	unsigned int ch;
+
+	clear_bit(chan->channel_index, buffer->channel_mask);
+
+	bitmap_clear(buffer->scan_mask, 0, chan->indio_dev->masklength);
+
+	for_each_set_bit(ch, buffer->channel_mask, chan->indio_dev->num_channels)
+		set_bit(chan->indio_dev->channels[ch].scan_index, buffer->scan_mask);
 }
 EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
 
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index a63dc07b7350..801e6ffa062c 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -84,6 +84,9 @@ struct iio_buffer {
 	/** @scan_mask: Bitmask used in masking scan mode elements. */
 	long *scan_mask;
 
+	/** @channel_mask: Bitmask used in masking scan mode elements (per channel). */
+	long *channel_mask;
+
 	/** @demux_list: List of operations required to demux the scan. */
 	struct list_head demux_list;
 
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index dbc87c26250a..6efd7091d3dd 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -19,11 +19,13 @@ struct device;
  * struct iio_channel - everything needed for a consumer to use a channel
  * @indio_dev:		Device on which the channel exists.
  * @channel:		Full description of the channel.
+ * @channel_index:	Offset of the channel into the devices channel array.
  * @data:		Data about the channel used by consumer.
  */
 struct iio_channel {
 	struct iio_dev *indio_dev;
 	const struct iio_chan_spec *channel;
+	unsigned int channel_index;
 	void *data;
 };
 
-- 
2.17.1

RE: [RFC PATCH 0/4] iio: scan_mask rework to track enabled channels on per-channel basis

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> On Behalf Of Alexandru Ardelean
> Sent: Freitag, 24. April 2020 07:18
> To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>
> Subject: [RFC PATCH 0/4] iio: scan_mask rework to track enabled channels on
> per-channel basis
> 
> 
> From my side, I'll admit that the specific use-cases for these patches
> are a bit outside of my scope of understanding.
> I did my best to re-apply them on a newer tree, and dig-up the
> information from the ADI tree [where they've been living for a while
> now].
> 
> Also, I don't have any idea if there was a prior discussion about this.
> I could not find anything on a quick search.
> 
> I'm hoping that the author would have some input on them/
> 
> Hence the RFC.
> 

Yeah, I remember I stumbled against this when looking to some of our drivers where we have (IIRC) a 16
channel ADC where 1 channel occupies 1 bit. I just gave a quick look on this and it looks good to me
(naturally it needs a deeper look :D). I just have one remark/question inline in patch 4.

- Nuno Sá

RE: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> On Behalf Of Alexandru Ardelean
> Sent: Freitag, 24. April 2020 07:18
> To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>
> Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis
> 
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Now that we support multiple channels with the same scan index we can no
> longer use the scan mask to track which channels have been enabled.
> Otherwise it is not possible to enable channels with the same scan index
> independently.
> 
> Introduce a new channel mask which is used instead of the scan mask to
> track which channels are enabled. Whenever the channel mask is changed a
> new scan mask is computed based on it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
>  drivers/iio/inkern.c              | 19 +++++++++-
>  include/linux/iio/buffer_impl.h   |  3 ++
>  include/linux/iio/consumer.h      |  2 +
>  4 files changed, 64 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index c06691281287..1821a3e32fb3 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> *buffer,
>  	if (buffer->scan_mask == NULL)
>  		return -ENOMEM;
> 
> +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> +					     GFP_KERNEL);
> +	if (buffer->channel_mask == NULL) {
> +		bitmap_free(buffer->scan_mask);
> +		return -ENOMEM;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> 
>  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
>  {
> +	bitmap_free(buffer->channel_mask);
>  	bitmap_free(buffer->scan_mask);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
> 
>  	/* Ensure ret is 0 or 1. */
>  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> -		       indio_dev->buffer->scan_mask);
> +		       indio_dev->buffer->channel_mask);
> 
>  	return sprintf(buf, "%d\n", ret);
>  }
> @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct iio_dev
> *indio_dev,
>   * buffers might request, hence this code only verifies that the
>   * individual buffers request is plausible.
>   */
> -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> -		      struct iio_buffer *buffer, int bit)
> +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> +				struct iio_buffer *buffer, int bit)
>  {
>  	const unsigned long *mask;
>  	unsigned long *trialmask;
> +	unsigned int ch;
> 
>  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
>  	if (trialmask == NULL)
> @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> *indio_dev,
>  		WARN(1, "Trying to set scanmask prior to registering
> buffer\n");
>  		goto err_invalid_mask;
>  	}
> -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-
> >masklength);
> -	set_bit(bit, trialmask);
> +
> +	set_bit(bit, buffer->channel_mask);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-
> >num_channels)
> +		set_bit(indio_dev->channels[ch].scan_index, trialmask);

So, here if the channels all have the same scan_index, we will end up with a scan_mask which is
different that channel_mask, right? I saw that in our internal driver's we then just access the
channel_mask field directly to know what pieces/channels do we need to enable prior to
buffering, which implies including buffer_impl.h.

So, for me it would make sense to compute scan_mask so that it will be the same as channel_mask
(hmm but that would be a problem when computing the buffer size...) and drivers can correctly use
` validate_scan_mask ()` cb. Alternatively, we need to expose channel_mask either on a new cb or
change the ` validate_scan_mask ()` footprint. 

- Nuno Sá

Re: [RFC PATCH 1/4] iio: Move scan mask management to the core

[Jonathan Cameron]

On Fri, 24 Apr 2020 08:18:15 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Let the core handle the buffer scan mask management including allocation
> and channel selection. Having this handled in a central place rather than
> open-coding it all over the place will make it easier to change the
> implementation.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Hi Alex,

For some reason I only have patch 1 of this series of 4.

This one looks reasonable to me as abstracts away how it is implemented
which is good. A few comments and a question inline.

Jonathan


> ---
>  drivers/iio/buffer/industrialio-buffer-cb.c | 17 ++++------
>  drivers/iio/industrialio-buffer.c           | 36 +++++++++++++++------
>  drivers/iio/inkern.c                        | 15 +++++++++
>  include/linux/iio/consumer.h                | 10 ++++++
>  4 files changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
> index 47c96f7f4976..b50f1f48cac6 100644
> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> @@ -33,8 +33,7 @@ static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
>  static void iio_buffer_cb_release(struct iio_buffer *buffer)
>  {
>  	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> -
> -	bitmap_free(cb_buff->buffer.scan_mask);
> +	iio_buffer_free_scanmask(buffer);
>  	kfree(cb_buff);
>  }
>  
> @@ -72,27 +71,25 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  	}
>  
>  	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> -	cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev->masklength,
> -						  GFP_KERNEL);
> -	if (cb_buff->buffer.scan_mask == NULL) {
> -		ret = -ENOMEM;
> +
> +	ret = iio_buffer_alloc_scanmask(&cb_buff->buffer, cb_buff->indio_dev);
> +	if (ret)
>  		goto error_release_channels;
> -	}
> +
>  	chan = &cb_buff->channels[0];
>  	while (chan->indio_dev) {
>  		if (chan->indio_dev != cb_buff->indio_dev) {
>  			ret = -EINVAL;
>  			goto error_free_scan_mask;
>  		}
> -		set_bit(chan->channel->scan_index,
> -			cb_buff->buffer.scan_mask);
> +		iio_buffer_channel_enable(&cb_buff->buffer, chan);
>  		chan++;
>  	}
>  
>  	return cb_buff;
>  
>  error_free_scan_mask:
> -	bitmap_free(cb_buff->buffer.scan_mask);
> +	iio_buffer_free_scanmask(&cb_buff->buffer);
>  error_release_channels:
>  	iio_channel_release_all(cb_buff->channels);
>  error_free_cb_buff:
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 221157136af6..c06691281287 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -206,6 +206,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
>  }
>  EXPORT_SYMBOL(iio_buffer_init);
>  
> +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> +			      struct iio_dev *indio_dev)
I'm not sure passing the indio_dev in here makes sense as it
obscures that all we are getting from it is the masklength.
May be better to pass that explicitly.

> +{
> +	if (!indio_dev->masklength)
> +		return 0;

This is a bit of an oddity of the old code.  Any idea why we
allow things to continue with a masklength of 0?  Seems to me
that it is thoroughly broken if that occurs!

> +
> +	buffer->scan_mask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> +	if (buffer->scan_mask == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> +
> +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> +{
> +	bitmap_free(buffer->scan_mask);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> +
>  /**
>   * iio_buffer_set_attrs - Set buffer specific attributes
>   * @buffer: The buffer for which we are setting attributes
> @@ -1301,14 +1321,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  				indio_dev->scan_index_timestamp =
>  					channels[i].scan_index;
>  		}
> -		if (indio_dev->masklength && buffer->scan_mask == NULL) {
> -			buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> -							  GFP_KERNEL);
> -			if (buffer->scan_mask == NULL) {
> -				ret = -ENOMEM;
> -				goto error_cleanup_dynamic;
> -			}
> -		}
> +
> +		ret = iio_buffer_alloc_scanmask(buffer, indio_dev);
> +		if (ret)
> +			goto error_cleanup_dynamic;
>  	}
>  
>  	buffer->scan_el_group.name = iio_scan_elements_group_name;
> @@ -1329,7 +1345,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	return 0;
>  
>  error_free_scan_mask:
> -	bitmap_free(buffer->scan_mask);
> +	iio_buffer_free_scanmask(buffer);
>  error_cleanup_dynamic:
>  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
>  	kfree(indio_dev->buffer->buffer_group.attrs);
> @@ -1342,7 +1358,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (!indio_dev->buffer)
>  		return;
>  
> -	bitmap_free(indio_dev->buffer->scan_mask);
> +	iio_buffer_free_scanmask(indio_dev->buffer);
>  	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);
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index ede99e0d5371..f35cb9985edc 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/iio/iio.h>
>  #include "iio_core.h"
> +#include <linux/iio/buffer_impl.h>
>  #include <linux/iio/machine.h>
>  #include <linux/iio/driver.h>
>  #include <linux/iio/consumer.h>
> @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
>  }
>  EXPORT_SYMBOL_GPL(iio_write_channel_raw);
>  
> +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> +			       const struct iio_channel *chan)
> +{
> +	set_bit(chan->channel->scan_index, buffer->scan_mask);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> +
> +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> +				const struct iio_channel *chan)
> +{
> +	clear_bit(chan->channel->scan_index, buffer->scan_mask);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> +
>  unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
>  {
>  	const struct iio_chan_spec_ext_info *ext_info;
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index c4118dcb8e05..dbc87c26250a 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -12,6 +12,7 @@
>  
>  struct iio_dev;
>  struct iio_chan_spec;
> +struct iio_buffer;
>  struct device;
>  
>  /**
> @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
>  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
>  	int *processed, unsigned int scale);
>  
> +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> +			       const struct iio_channel *chan);
> +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> +				const struct iio_channel *chan);
> +
> +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> +			      struct iio_dev *indio_dev);
> +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> +
>  /**
>   * iio_get_channel_ext_info_count() - get number of ext_info attributes
>   *				      connected to the channel.

Re: [RFC PATCH 1/4] iio: Move scan mask management to the core

[Jonathan Cameron]

On Sun, 26 Apr 2020 10:45:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 24 Apr 2020 08:18:15 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > Let the core handle the buffer scan mask management including allocation
> > and channel selection. Having this handled in a central place rather than
> > open-coding it all over the place will make it easier to change the
> > implementation.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> 
> Hi Alex,
> 
> For some reason I only have patch 1 of this series of 4.
Ah. My email went crazy. Rest of series now turned up from somewhere :)

Thanks,

J

> 
> This one looks reasonable to me as abstracts away how it is implemented
> which is good. A few comments and a question inline.
> 
> Jonathan
> 
> 
> > ---
> >  drivers/iio/buffer/industrialio-buffer-cb.c | 17 ++++------
> >  drivers/iio/industrialio-buffer.c           | 36 +++++++++++++++------
> >  drivers/iio/inkern.c                        | 15 +++++++++
> >  include/linux/iio/consumer.h                | 10 ++++++
> >  4 files changed, 58 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
> > index 47c96f7f4976..b50f1f48cac6 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> > @@ -33,8 +33,7 @@ static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
> >  static void iio_buffer_cb_release(struct iio_buffer *buffer)
> >  {
> >  	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> > -
> > -	bitmap_free(cb_buff->buffer.scan_mask);
> > +	iio_buffer_free_scanmask(buffer);
> >  	kfree(cb_buff);
> >  }
> >  
> > @@ -72,27 +71,25 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
> >  	}
> >  
> >  	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> > -	cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev->masklength,
> > -						  GFP_KERNEL);
> > -	if (cb_buff->buffer.scan_mask == NULL) {
> > -		ret = -ENOMEM;
> > +
> > +	ret = iio_buffer_alloc_scanmask(&cb_buff->buffer, cb_buff->indio_dev);
> > +	if (ret)
> >  		goto error_release_channels;
> > -	}
> > +
> >  	chan = &cb_buff->channels[0];
> >  	while (chan->indio_dev) {
> >  		if (chan->indio_dev != cb_buff->indio_dev) {
> >  			ret = -EINVAL;
> >  			goto error_free_scan_mask;
> >  		}
> > -		set_bit(chan->channel->scan_index,
> > -			cb_buff->buffer.scan_mask);
> > +		iio_buffer_channel_enable(&cb_buff->buffer, chan);
> >  		chan++;
> >  	}
> >  
> >  	return cb_buff;
> >  
> >  error_free_scan_mask:
> > -	bitmap_free(cb_buff->buffer.scan_mask);
> > +	iio_buffer_free_scanmask(&cb_buff->buffer);
> >  error_release_channels:
> >  	iio_channel_release_all(cb_buff->channels);
> >  error_free_cb_buff:
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 221157136af6..c06691281287 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -206,6 +206,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> >  }
> >  EXPORT_SYMBOL(iio_buffer_init);
> >  
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > +			      struct iio_dev *indio_dev)  
> I'm not sure passing the indio_dev in here makes sense as it
> obscures that all we are getting from it is the masklength.
> May be better to pass that explicitly.
> 
> > +{
> > +	if (!indio_dev->masklength)
> > +		return 0;  
> 
> This is a bit of an oddity of the old code.  Any idea why we
> allow things to continue with a masklength of 0?  Seems to me
> that it is thoroughly broken if that occurs!
> 
> > +
> > +	buffer->scan_mask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> > +	if (buffer->scan_mask == NULL)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > +
> > +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > +{
> > +	bitmap_free(buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > +
> >  /**
> >   * iio_buffer_set_attrs - Set buffer specific attributes
> >   * @buffer: The buffer for which we are setting attributes
> > @@ -1301,14 +1321,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >  				indio_dev->scan_index_timestamp =
> >  					channels[i].scan_index;
> >  		}
> > -		if (indio_dev->masklength && buffer->scan_mask == NULL) {
> > -			buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> > -							  GFP_KERNEL);
> > -			if (buffer->scan_mask == NULL) {
> > -				ret = -ENOMEM;
> > -				goto error_cleanup_dynamic;
> > -			}
> > -		}
> > +
> > +		ret = iio_buffer_alloc_scanmask(buffer, indio_dev);
> > +		if (ret)
> > +			goto error_cleanup_dynamic;
> >  	}
> >  
> >  	buffer->scan_el_group.name = iio_scan_elements_group_name;
> > @@ -1329,7 +1345,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >  	return 0;
> >  
> >  error_free_scan_mask:
> > -	bitmap_free(buffer->scan_mask);
> > +	iio_buffer_free_scanmask(buffer);
> >  error_cleanup_dynamic:
> >  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> >  	kfree(indio_dev->buffer->buffer_group.attrs);
> > @@ -1342,7 +1358,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >  	if (!indio_dev->buffer)
> >  		return;
> >  
> > -	bitmap_free(indio_dev->buffer->scan_mask);
> > +	iio_buffer_free_scanmask(indio_dev->buffer);
> >  	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);
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index ede99e0d5371..f35cb9985edc 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include <linux/iio/iio.h>
> >  #include "iio_core.h"
> > +#include <linux/iio/buffer_impl.h>
> >  #include <linux/iio/machine.h>
> >  #include <linux/iio/driver.h>
> >  #include <linux/iio/consumer.h>
> > @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
> >  }
> >  EXPORT_SYMBOL_GPL(iio_write_channel_raw);
> >  
> > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > +			       const struct iio_channel *chan)
> > +{
> > +	set_bit(chan->channel->scan_index, buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> > +
> > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > +				const struct iio_channel *chan)
> > +{
> > +	clear_bit(chan->channel->scan_index, buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> > +
> >  unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> >  {
> >  	const struct iio_chan_spec_ext_info *ext_info;
> > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> > index c4118dcb8e05..dbc87c26250a 100644
> > --- a/include/linux/iio/consumer.h
> > +++ b/include/linux/iio/consumer.h
> > @@ -12,6 +12,7 @@
> >  
> >  struct iio_dev;
> >  struct iio_chan_spec;
> > +struct iio_buffer;
> >  struct device;
> >  
> >  /**
> > @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
> >  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> >  	int *processed, unsigned int scale);
> >  
> > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > +			       const struct iio_channel *chan);
> > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > +				const struct iio_channel *chan);
> > +
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > +			      struct iio_dev *indio_dev);
> > +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> > +
> >  /**
> >   * iio_get_channel_ext_info_count() - get number of ext_info attributes
> >   *				      connected to the channel.  
> 

Re: [RFC PATCH 3/4] iio: Allow channels to share storage elements

[Jonathan Cameron]

On Fri, 24 Apr 2020 08:18:17 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Currently each IIO channel has it's own storage element in the data stream
> each with its own unique scan index. This works for a lot of use-cases,
> but in some is not good enough to represent the hardware accurately. On
> those devices multiple separate pieces of information are stored within the
> same storage element and the storage element can't be further broken down
> into multiple storage elements (e.g. because the data is not aligned).
> 
> This can for example be status bits stored in unused data bits. E.g. a
> 14-bit ADC that stores its data in a 16-bit word and uses the two
> additional bits to convey status information like for example whether a
> overrange condition has happened. Currently this kind of extra status
> information is usually ignored and can only be used by applications that
> have special knowledge about the connected device and its data layout.

Hmm. The problem with this (and the reason I have resisted this in the past)
is that this fundamentally breaks the existing ABI.  Whilst we have never
explicitly stated that this can't be done (I think) a lot of code may
assume it.

Are we sure this doesn't break existing userspace in weird and wonderful
ways?   I'm sure someone does stuff 'in place' on the incoming data
streams for example which this would break.

Also does the demux work with these if we have multiple consumers?  Seems
unlikely that it will do it efficiently if at all.

J
> 
> In addition to that some might also have data channels with less than 8
> bits that get packed into the same storage element.
> 
> Allow two or more channels to use the same scan index, if they their
> storage element does have the same size.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index f4daf19f2a3b..cdf59a51c917 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1651,6 +1651,16 @@ static const struct file_operations iio_buffer_fileops = {
>  	.compat_ioctl = compat_ptr_ioctl,
>  };
>  
> +static bool iio_chan_same_size(const struct iio_chan_spec *a,
> +	const struct iio_chan_spec *b)
> +{
> +	if (a->scan_type.storagebits != b->scan_type.storagebits)
> +		return false;
> +	if (a->scan_type.repeat != b->scan_type.repeat)
> +		return false;
> +	return true;
> +}
> +
>  static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
>  {
>  	int i, j;
> @@ -1662,13 +1672,16 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
>  	for (i = 0; i < indio_dev->num_channels - 1; i++) {
>  		if (channels[i].scan_index < 0)
>  			continue;
> -		for (j = i + 1; j < indio_dev->num_channels; j++)
> -			if (channels[i].scan_index == channels[j].scan_index) {
> -				dev_err(&indio_dev->dev,
> -					"Duplicate scan index %d\n",
> -					channels[i].scan_index);
> -				return -EINVAL;
> -			}
> +		for (j = i + 1; j < indio_dev->num_channels; j++) {
> +			if (channels[i].scan_index != channels[j].scan_index)
> +				continue;
> +			if (iio_chan_same_size(&channels[i], &channels[j]))
> +				continue;
> +			dev_err(&indio_dev->dev,
> +				"Duplicate scan index %d\n",
> +				channels[i].scan_index);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	return 0;

Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Jonathan Cameron]

On Fri, 24 Apr 2020 08:18:18 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Now that we support multiple channels with the same scan index we can no
> longer use the scan mask to track which channels have been enabled.
> Otherwise it is not possible to enable channels with the same scan index
> independently.
> 
> Introduce a new channel mask which is used instead of the scan mask to
> track which channels are enabled. Whenever the channel mask is changed a
> new scan mask is computed based on it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Hmm. This does seem to handle the demux so that concern is addressed.
Perhaps merge this with previous patch or reorder them.  The sanity check
should only be removed once it's is safe to do so.

This comes in the category of 'clever' but I'm worried it makes the
interface rather less obvious.   Definitely want a lot of inputs from
others on this.

At first glance I rather like it as it solves some of the special cases
we had last time we looked a single bit channels.

Note I'm still far from sold on metadata unless it maps to a 'real' channel
in some sense.  It's really hard to define ABI around the sort of meta
data these devices tend to hide in those unused bits.


Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
>  drivers/iio/inkern.c              | 19 +++++++++-
>  include/linux/iio/buffer_impl.h   |  3 ++
>  include/linux/iio/consumer.h      |  2 +
>  4 files changed, 64 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index c06691281287..1821a3e32fb3 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
>  	if (buffer->scan_mask == NULL)
>  		return -ENOMEM;
>  
> +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> +					     GFP_KERNEL);
> +	if (buffer->channel_mask == NULL) {
> +		bitmap_free(buffer->scan_mask);
> +		return -ENOMEM;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
>  
>  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
>  {
> +	bitmap_free(buffer->channel_mask);
>  	bitmap_free(buffer->scan_mask);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
>  
>  	/* Ensure ret is 0 or 1. */
>  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> -		       indio_dev->buffer->scan_mask);
> +		       indio_dev->buffer->channel_mask);
>  
>  	return sprintf(buf, "%d\n", ret);
>  }
> @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct iio_dev *indio_dev,
>   * buffers might request, hence this code only verifies that the
>   * individual buffers request is plausible.
>   */
> -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> -		      struct iio_buffer *buffer, int bit)
> +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> +				struct iio_buffer *buffer, int bit)
>  {
>  	const unsigned long *mask;
>  	unsigned long *trialmask;
> +	unsigned int ch;
>  
>  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
>  	if (trialmask == NULL)
> @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
>  		WARN(1, "Trying to set scanmask prior to registering buffer\n");
>  		goto err_invalid_mask;
>  	}
> -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev->masklength);
> -	set_bit(bit, trialmask);
> +
> +	set_bit(bit, buffer->channel_mask);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, indio_dev->num_channels)
> +		set_bit(indio_dev->channels[ch].scan_index, trialmask);
>  
>  	if (!iio_validate_scan_mask(indio_dev, trialmask))
>  		goto err_invalid_mask;
> @@ -363,28 +375,37 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
>  	return 0;
>  
>  err_invalid_mask:
> +	clear_bit(bit, buffer->channel_mask);
>  	bitmap_free(trialmask);
>  	return -EINVAL;
>  }
>  
> -static int iio_scan_mask_clear(struct iio_buffer *buffer, int bit)
> +static int iio_channel_mask_clear(struct iio_dev *indio_dev,
> +				  struct iio_buffer *buffer, int bit)
>  {
> -	clear_bit(bit, buffer->scan_mask);
> +	unsigned int ch;
> +
> +	clear_bit(bit, buffer->channel_mask);
> +
> +	bitmap_clear(buffer->scan_mask, 0, indio_dev->masklength);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, indio_dev->num_channels)
> +		set_bit(indio_dev->channels[ch].scan_index, buffer->scan_mask);
>  	return 0;
>  }
>  
> -static int iio_scan_mask_query(struct iio_dev *indio_dev,
> -			       struct iio_buffer *buffer, int bit)
> +static int iio_channel_mask_query(struct iio_dev *indio_dev,
> +				 struct iio_buffer *buffer, int bit)
>  {
> -	if (bit > indio_dev->masklength)
> +	if (bit > indio_dev->num_channels)
>  		return -EINVAL;
>  
> -	if (!buffer->scan_mask)
> +	if (!buffer->channel_mask)
>  		return 0;
>  
>  	/* Ensure return value is 0 or 1. */
> -	return !!test_bit(bit, buffer->scan_mask);
> -};
> +	return !!test_bit(bit, buffer->channel_mask);
> +}
>  
>  static ssize_t iio_scan_el_store(struct device *dev,
>  				 struct device_attribute *attr,
> @@ -405,15 +426,15 @@ static ssize_t iio_scan_el_store(struct device *dev,
>  		ret = -EBUSY;
>  		goto error_ret;
>  	}
> -	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
> +	ret = iio_channel_mask_query(indio_dev, buffer, this_attr->address);
>  	if (ret < 0)
>  		goto error_ret;
>  	if (!state && ret) {
> -		ret = iio_scan_mask_clear(buffer, this_attr->address);
> +		ret = iio_channel_mask_clear(indio_dev, buffer, this_attr->address);
>  		if (ret)
>  			goto error_ret;
>  	} else if (state && !ret) {
> -		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> +		ret = iio_channel_mask_set(indio_dev, buffer, this_attr->address);
>  		if (ret)
>  			goto error_ret;
>  	}
> @@ -459,7 +480,8 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
>  }
>  
>  static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> -					const struct iio_chan_spec *chan)
> +					const struct iio_chan_spec *chan,
> +					unsigned int address)
>  {
>  	int ret, attrcount = 0;
>  	struct iio_buffer *buffer = indio_dev->buffer;
> @@ -491,7 +513,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  					     chan,
>  					     &iio_scan_el_show,
>  					     &iio_scan_el_store,
> -					     chan->scan_index,
> +					     address,
>  					     0,
>  					     &indio_dev->dev,
>  					     &buffer->scan_el_dev_attr_list);
> @@ -500,7 +522,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  					     chan,
>  					     &iio_scan_el_ts_show,
>  					     &iio_scan_el_ts_store,
> -					     chan->scan_index,
> +					     address,
>  					     0,
>  					     &indio_dev->dev,
>  					     &buffer->scan_el_dev_attr_list);
> @@ -1313,7 +1335,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  				continue;
>  
>  			ret = iio_buffer_add_channel_sysfs(indio_dev,
> -							 &channels[i]);
> +							 &channels[i], i);
>  			if (ret < 0)
>  				goto error_cleanup_dynamic;
>  			attrcount += ret;
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index f35cb9985edc..57cf4b01c403 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -150,6 +150,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>  	if (index < 0)
>  		goto err_put;
>  	channel->channel = &indio_dev->channels[index];
> +	channel->channel_index = index;
>  
>  	return 0;
>  
> @@ -861,14 +862,28 @@ EXPORT_SYMBOL_GPL(iio_write_channel_raw);
>  void iio_buffer_channel_enable(struct iio_buffer *buffer,
>  			       const struct iio_channel *chan)
>  {
> -	set_bit(chan->channel->scan_index, buffer->scan_mask);
> +	unsigned int ch;
> +
> +	set_bit(chan->channel_index, buffer->channel_mask);
> +

This seems overkill.  Can enabling a channel ever result in a bitmap
that doesn't contain all the bits that were previously set.
Feels like we should be able to just set the one matching the channel
we added to the channel_mask.

I guess the advantage of this is it clearly matches with the disable below
where we do have to walk the channel mask to find out if we have any
overlapping bits.

> +	bitmap_clear(buffer->scan_mask, 0, chan->indio_dev->masklength);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, chan->indio_dev->num_channels)
> +		set_bit(chan->indio_dev->channels[ch].scan_index, buffer->scan_mask);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
>  
>  void iio_buffer_channel_disable(struct iio_buffer *buffer,
>  				const struct iio_channel *chan)
>  {
> -	clear_bit(chan->channel->scan_index, buffer->scan_mask);
> +	unsigned int ch;
> +
> +	clear_bit(chan->channel_index, buffer->channel_mask);
> +
> +	bitmap_clear(buffer->scan_mask, 0, chan->indio_dev->masklength);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, chan->indio_dev->num_channels)
> +		set_bit(chan->indio_dev->channels[ch].scan_index, buffer->scan_mask);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
>  
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index a63dc07b7350..801e6ffa062c 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -84,6 +84,9 @@ struct iio_buffer {
>  	/** @scan_mask: Bitmask used in masking scan mode elements. */
>  	long *scan_mask;
>  
> +	/** @channel_mask: Bitmask used in masking scan mode elements (per channel). */
> +	long *channel_mask;
> +
>  	/** @demux_list: List of operations required to demux the scan. */
>  	struct list_head demux_list;
>  
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index dbc87c26250a..6efd7091d3dd 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -19,11 +19,13 @@ struct device;
>   * struct iio_channel - everything needed for a consumer to use a channel
>   * @indio_dev:		Device on which the channel exists.
>   * @channel:		Full description of the channel.
> + * @channel_index:	Offset of the channel into the devices channel array.
>   * @data:		Data about the channel used by consumer.
>   */
>  struct iio_channel {
>  	struct iio_dev *indio_dev;
>  	const struct iio_chan_spec *channel;
> +	unsigned int channel_index;
>  	void *data;
>  };
>  

Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Jonathan Cameron]

On Fri, 24 Apr 2020 07:51:05 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> > On Behalf Of Alexandru Ardelean
> > Sent: Freitag, 24. April 2020 07:18
> > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com>
> > Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis
> > 
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > Now that we support multiple channels with the same scan index we can no
> > longer use the scan mask to track which channels have been enabled.
> > Otherwise it is not possible to enable channels with the same scan index
> > independently.
> > 
> > Introduce a new channel mask which is used instead of the scan mask to
> > track which channels are enabled. Whenever the channel mask is changed a
> > new scan mask is computed based on it.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
> >  drivers/iio/inkern.c              | 19 +++++++++-
> >  include/linux/iio/buffer_impl.h   |  3 ++
> >  include/linux/iio/consumer.h      |  2 +
> >  4 files changed, 64 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index c06691281287..1821a3e32fb3 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> > *buffer,
> >  	if (buffer->scan_mask == NULL)
> >  		return -ENOMEM;
> > 
> > +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> > +					     GFP_KERNEL);
> > +	if (buffer->channel_mask == NULL) {
> > +		bitmap_free(buffer->scan_mask);
> > +		return -ENOMEM;
> > +	}
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > 
> >  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> >  {
> > +	bitmap_free(buffer->channel_mask);
> >  	bitmap_free(buffer->scan_mask);
> >  }
> >  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
> > 
> >  	/* Ensure ret is 0 or 1. */
> >  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > -		       indio_dev->buffer->scan_mask);
> > +		       indio_dev->buffer->channel_mask);
> > 
> >  	return sprintf(buf, "%d\n", ret);
> >  }
> > @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct iio_dev
> > *indio_dev,
> >   * buffers might request, hence this code only verifies that the
> >   * individual buffers request is plausible.
> >   */
> > -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> > -		      struct iio_buffer *buffer, int bit)
> > +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> > +				struct iio_buffer *buffer, int bit)
> >  {
> >  	const unsigned long *mask;
> >  	unsigned long *trialmask;
> > +	unsigned int ch;
> > 
> >  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> >  	if (trialmask == NULL)
> > @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> > *indio_dev,
> >  		WARN(1, "Trying to set scanmask prior to registering
> > buffer\n");
> >  		goto err_invalid_mask;
> >  	}
> > -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-  
> > >masklength);  
> > -	set_bit(bit, trialmask);
> > +
> > +	set_bit(bit, buffer->channel_mask);
> > +
> > +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-  
> > >num_channels)  
> > +		set_bit(indio_dev->channels[ch].scan_index, trialmask);  
> 
> So, here if the channels all have the same scan_index, we will end up with a scan_mask which is
> different that channel_mask, right? I saw that in our internal driver's we then just access the
> channel_mask field directly to know what pieces/channels do we need to enable prior to
> buffering, which implies including buffer_impl.h.
Given that we handle the demux only at the level of scan elements that won't work in general
(even if it wasn't a horrible layering issue).

> 
> So, for me it would make sense to compute scan_mask so that it will be the same as channel_mask
> (hmm but that would be a problem when computing the buffer size...) and drivers can correctly use
> ` validate_scan_mask ()` cb. Alternatively, we need to expose channel_mask either on a new cb or
> change the ` validate_scan_mask ()` footprint. 

Excellent points. We need to address support for:

1) available_scan_mask - if we have complicated rules on mixtures of channels inside
   a given buffer element.

2) channel enabling though I'm sort of inclined to say that if you are using this approach
   you only get information on channels that make up a scan mask element.  Tough luck you
   may end up enabling more than you'd like.

It might be possible to make switch to using a channel mask but given the channel index is
implicit that is going to be at least a little bit nasty.

How much does it hurt to not have the ability to separately control channels within
a given buffer element?   Userspace can enable / disable them but reality is you'll
get data for all the channels in a buffer element if any of them are enabled.

Given the demux will copy the whole element anyway (don't want to waste time doing
masking inside an element), userspace might get these extra channels anyway if another
consumer has enabled them.

Jonathan


> 
> - Nuno Sá

Re: [RFC PATCH 1/4] iio: Move scan mask management to the core

[Ardelean, Alexandru]

On Sun, 2020-04-26 at 10:45 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Fri, 24 Apr 2020 08:18:15 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > Let the core handle the buffer scan mask management including allocation
> > and channel selection. Having this handled in a central place rather than
> > open-coding it all over the place will make it easier to change the
> > implementation.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Hi Alex,
> 
> For some reason I only have patch 1 of this series of 4.
> 
> This one looks reasonable to me as abstracts away how it is implemented
> which is good. A few comments and a question inline.
> 

Replies inline.

Since the discussion about patches 3 & 4 got a little more complicated [than I'd
like to focus on at this point in time], would it be ok to split patches 1 & 2
into a separate series? [after discussions are resolved]
Even just those 2 patches helps me work on 2 kernel trees [the ADI internal &
the IIO upstream one], as it minimizes the diff.


> Jonathan
> 
> 
> > ---
> >  drivers/iio/buffer/industrialio-buffer-cb.c | 17 ++++------
> >  drivers/iio/industrialio-buffer.c           | 36 +++++++++++++++------
> >  drivers/iio/inkern.c                        | 15 +++++++++
> >  include/linux/iio/consumer.h                | 10 ++++++
> >  4 files changed, 58 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c
> > b/drivers/iio/buffer/industrialio-buffer-cb.c
> > index 47c96f7f4976..b50f1f48cac6 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> > @@ -33,8 +33,7 @@ static int iio_buffer_cb_store_to(struct iio_buffer
> > *buffer, const void *data)
> >  static void iio_buffer_cb_release(struct iio_buffer *buffer)
> >  {
> >  	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> > -
> > -	bitmap_free(cb_buff->buffer.scan_mask);
> > +	iio_buffer_free_scanmask(buffer);
> >  	kfree(cb_buff);
> >  }
> >  
> > @@ -72,27 +71,25 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct
> > device *dev,
> >  	}
> >  
> >  	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> > -	cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev-
> > >masklength,
> > -						  GFP_KERNEL);
> > -	if (cb_buff->buffer.scan_mask == NULL) {
> > -		ret = -ENOMEM;
> > +
> > +	ret = iio_buffer_alloc_scanmask(&cb_buff->buffer, cb_buff->indio_dev);
> > +	if (ret)
> >  		goto error_release_channels;
> > -	}
> > +
> >  	chan = &cb_buff->channels[0];
> >  	while (chan->indio_dev) {
> >  		if (chan->indio_dev != cb_buff->indio_dev) {
> >  			ret = -EINVAL;
> >  			goto error_free_scan_mask;
> >  		}
> > -		set_bit(chan->channel->scan_index,
> > -			cb_buff->buffer.scan_mask);
> > +		iio_buffer_channel_enable(&cb_buff->buffer, chan);
> >  		chan++;
> >  	}
> >  
> >  	return cb_buff;
> >  
> >  error_free_scan_mask:
> > -	bitmap_free(cb_buff->buffer.scan_mask);
> > +	iio_buffer_free_scanmask(&cb_buff->buffer);
> >  error_release_channels:
> >  	iio_channel_release_all(cb_buff->channels);
> >  error_free_cb_buff:
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index 221157136af6..c06691281287 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -206,6 +206,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> >  }
> >  EXPORT_SYMBOL(iio_buffer_init);
> >  
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > +			      struct iio_dev *indio_dev)
> I'm not sure passing the indio_dev in here makes sense as it
> obscures that all we are getting from it is the masklength.
> May be better to pass that explicitly.

Passing indio_dev seems to make more sense if we are also doing the channel_mask
thing in the next patches.
If we do the buffer->indio_dev back-ref, then we can just pass the buffer.
But that may obscure things a bit more about the 'masklength'; though, that may
be one of the intentions I suppose [can't be sure]

> 
> > +{
> > +	if (!indio_dev->masklength)
> > +		return 0;
> 
> This is a bit of an oddity of the old code.  Any idea why we
> allow things to continue with a masklength of 0?  Seems to me
> that it is thoroughly broken if that occurs!

See [1]

> 
> > +
> > +	buffer->scan_mask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> > +	if (buffer->scan_mask == NULL)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > +
> > +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > +{
> > +	bitmap_free(buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > +
> >  /**
> >   * iio_buffer_set_attrs - Set buffer specific attributes
> >   * @buffer: The buffer for which we are setting attributes
> > @@ -1301,14 +1321,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> >  				indio_dev->scan_index_timestamp =
> >  					channels[i].scan_index;
> >  		}
> > -		if (indio_dev->masklength && buffer->scan_mask == NULL) {

[1] I suspect when this patch was written, it had this part in mind [with the
retun 0]
So, it preserves behavior a bit.
The 'buffer->scan_mask == NULL' check is a bit odd; I wonder if the initial
intent was to also allow to pass a 'scan_mask' from somewhere else.

So, updating '!indio_dev->masklength' to return -EINVAL in
iio_buffer_alloc_scanmask() sounds like a good idea, but I'll need to add a note
in the commit description.


> > -			buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> > -							  GFP_KERNEL);
> > -			if (buffer->scan_mask == NULL) {
> > -				ret = -ENOMEM;
> > -				goto error_cleanup_dynamic;
> > -			}
> > -		}
> > +
> > +		ret = iio_buffer_alloc_scanmask(buffer, indio_dev);
> > +		if (ret)
> > +			goto error_cleanup_dynamic;
> >  	}
> >  
> >  	buffer->scan_el_group.name = iio_scan_elements_group_name;
> > @@ -1329,7 +1345,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> >  	return 0;
> >  
> >  error_free_scan_mask:
> > -	bitmap_free(buffer->scan_mask);
> > +	iio_buffer_free_scanmask(buffer);
> >  error_cleanup_dynamic:
> >  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> >  	kfree(indio_dev->buffer->buffer_group.attrs);
> > @@ -1342,7 +1358,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> >  	if (!indio_dev->buffer)
> >  		return;
> >  
> > -	bitmap_free(indio_dev->buffer->scan_mask);
> > +	iio_buffer_free_scanmask(indio_dev->buffer);
> >  	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);
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index ede99e0d5371..f35cb9985edc 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include <linux/iio/iio.h>
> >  #include "iio_core.h"
> > +#include <linux/iio/buffer_impl.h>
> >  #include <linux/iio/machine.h>
> >  #include <linux/iio/driver.h>
> >  #include <linux/iio/consumer.h>
> > @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int
> > val)
> >  }
> >  EXPORT_SYMBOL_GPL(iio_write_channel_raw);
> >  
> > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > +			       const struct iio_channel *chan)
> > +{
> > +	set_bit(chan->channel->scan_index, buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> > +
> > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > +				const struct iio_channel *chan)
> > +{
> > +	clear_bit(chan->channel->scan_index, buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> > +
> >  unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> >  {
> >  	const struct iio_chan_spec_ext_info *ext_info;
> > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> > index c4118dcb8e05..dbc87c26250a 100644
> > --- a/include/linux/iio/consumer.h
> > +++ b/include/linux/iio/consumer.h
> > @@ -12,6 +12,7 @@
> >  
> >  struct iio_dev;
> >  struct iio_chan_spec;
> > +struct iio_buffer;
> >  struct device;
> >  
> >  /**
> > @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan,
> > int *val,
> >  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> >  	int *processed, unsigned int scale);
> >  
> > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > +			       const struct iio_channel *chan);
> > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > +				const struct iio_channel *chan);
> > +
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > +			      struct iio_dev *indio_dev);
> > +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> > +
> >  /**
> >   * iio_get_channel_ext_info_count() - get number of ext_info attributes
> >   *				      connected to the channel.

RE: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> On Behalf Of Jonathan Cameron
> Sent: Sonntag, 26. April 2020 12:51
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; lars@metafoo.de
> Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> basis
> 
> On Fri, 24 Apr 2020 07:51:05 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: linux-iio-owner@vger.kernel.org <linux-iio-
> owner@vger.kernel.org>
> > > On Behalf Of Alexandru Ardelean
> > > Sent: Freitag, 24. April 2020 07:18
> > > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> > > <alexandru.Ardelean@analog.com>
> > > Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> basis
> > >
> > > From: Lars-Peter Clausen <lars@metafoo.de>
> > >
> > > Now that we support multiple channels with the same scan index we can
> no
> > > longer use the scan mask to track which channels have been enabled.
> > > Otherwise it is not possible to enable channels with the same scan index
> > > independently.
> > >
> > > Introduce a new channel mask which is used instead of the scan mask to
> > > track which channels are enabled. Whenever the channel mask is
> changed a
> > > new scan mask is computed based on it.
> > >
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
> > >  drivers/iio/inkern.c              | 19 +++++++++-
> > >  include/linux/iio/buffer_impl.h   |  3 ++
> > >  include/linux/iio/consumer.h      |  2 +
> > >  4 files changed, 64 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> buffer.c
> > > index c06691281287..1821a3e32fb3 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> > > *buffer,
> > >  	if (buffer->scan_mask == NULL)
> > >  		return -ENOMEM;
> > >
> > > +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> > > +					     GFP_KERNEL);
> > > +	if (buffer->channel_mask == NULL) {
> > > +		bitmap_free(buffer->scan_mask);
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > >
> > >  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > >  {
> > > +	bitmap_free(buffer->channel_mask);
> > >  	bitmap_free(buffer->scan_mask);
> > >  }
> > >  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device
> *dev,
> > >
> > >  	/* Ensure ret is 0 or 1. */
> > >  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > > -		       indio_dev->buffer->scan_mask);
> > > +		       indio_dev->buffer->channel_mask);
> > >
> > >  	return sprintf(buf, "%d\n", ret);
> > >  }
> > > @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct
> iio_dev
> > > *indio_dev,
> > >   * buffers might request, hence this code only verifies that the
> > >   * individual buffers request is plausible.
> > >   */
> > > -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> > > -		      struct iio_buffer *buffer, int bit)
> > > +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> > > +				struct iio_buffer *buffer, int bit)
> > >  {
> > >  	const unsigned long *mask;
> > >  	unsigned long *trialmask;
> > > +	unsigned int ch;
> > >
> > >  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> > >  	if (trialmask == NULL)
> > > @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> > > *indio_dev,
> > >  		WARN(1, "Trying to set scanmask prior to registering
> > > buffer\n");
> > >  		goto err_invalid_mask;
> > >  	}
> > > -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-
> > > >masklength);
> > > -	set_bit(bit, trialmask);
> > > +
> > > +	set_bit(bit, buffer->channel_mask);
> > > +
> > > +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-
> > > >num_channels)
> > > +		set_bit(indio_dev->channels[ch].scan_index, trialmask);
> >
> > So, here if the channels all have the same scan_index, we will end up with a
> scan_mask which is
> > different that channel_mask, right? I saw that in our internal driver's we
> then just access the
> > channel_mask field directly to know what pieces/channels do we need to
> enable prior to
> > buffering, which implies including buffer_impl.h.
> Given that we handle the demux only at the level of scan elements that
> won't work in general
> (even if it wasn't a horrible layering issue).

Yes, and the driver just adds 16 channels and points all of them to scan_index 0. It then
sets real_bits and the shift so that userspace can get the right channel bit. So, in the end
we have just one buffer/scan element with 16bits. My problem here is more architectural...
We should not directly include "buffer_impl.h" in drivers...

> >
> > So, for me it would make sense to compute scan_mask so that it will be the
> same as channel_mask
> > (hmm but that would be a problem when computing the buffer size...) and
> drivers can correctly use
> > ` validate_scan_mask ()` cb. Alternatively, we need to expose
> channel_mask either on a new cb or
> > change the ` validate_scan_mask ()` footprint.
> 
> Excellent points. We need to address support for:
> 
> 1) available_scan_mask - if we have complicated rules on mixtures of
> channels inside
>    a given buffer element.

Maybe one solution to expose channel mask is to check if channel_mask != scan_mask
before calling the ` validate_scan_mask()`. If it is, we pass channel_mask to the callback.
Driver's should then know what to do with it...

> 2) channel enabling though I'm sort of inclined to say that if you are using this
> approach
>    you only get information on channels that make up a scan mask element.
> Tough luck you
>    may end up enabling more than you'd like.

Not sure if I'm fully understanding this point. I believe with this approach channel
enablement works as before since the core is kind of mapping channel_mask to
scan_mask. So if we have 16 channels using only 1 scan_element we can still
enable/disable all 16 channels.

In the end, if we have a traditional driver with one channel per scan_index, channel_mask
should be equal to scan_mask. As we start to have more than one channel pointing to the
same scan_index, these masks will be different.

> It might be possible to make switch to using a channel mask but given the
> channel index is
> implicit that is going to be at least a little bit nasty.
> 
> How much does it hurt to not have the ability to separately control channels
> within
> a given buffer element?   Userspace can enable / disable them but reality is
> you'll

As long as we are "ok" with the extra amount of allocated memory, I think it would work.
Though drivers will have to replicate the same data trough all the enabled scan elements...

- Nuno Sá

> get data for all the channels in a buffer element if any of them are enabled.
> 
> Given the demux will copy the whole element anyway (don't want to waste
> time doing
> masking inside an element), userspace might get these extra channels
> anyway if another
> consumer has enabled them.
> 
> Jonathan
> 
> 
> >
> > - Nuno Sá

Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Jonathan Cameron]

On Mon, 27 Apr 2020 12:09:18 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> > On Behalf Of Jonathan Cameron
> > Sent: Sonntag, 26. April 2020 12:51
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org; lars@metafoo.de
> > Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> > basis
> > 
> > On Fri, 24 Apr 2020 07:51:05 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >   
> > > > From: linux-iio-owner@vger.kernel.org <linux-iio-  
> > owner@vger.kernel.org>  
> > > > On Behalf Of Alexandru Ardelean
> > > > Sent: Freitag, 24. April 2020 07:18
> > > > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> > > > <alexandru.Ardelean@analog.com>
> > > > Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel  
> > basis  
> > > >
> > > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > >
> > > > Now that we support multiple channels with the same scan index we can  
> > no  
> > > > longer use the scan mask to track which channels have been enabled.
> > > > Otherwise it is not possible to enable channels with the same scan index
> > > > independently.
> > > >
> > > > Introduce a new channel mask which is used instead of the scan mask to
> > > > track which channels are enabled. Whenever the channel mask is  
> > changed a  
> > > > new scan mask is computed based on it.
> > > >
> > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > ---
> > > >  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
> > > >  drivers/iio/inkern.c              | 19 +++++++++-
> > > >  include/linux/iio/buffer_impl.h   |  3 ++
> > > >  include/linux/iio/consumer.h      |  2 +
> > > >  4 files changed, 64 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-  
> > buffer.c  
> > > > index c06691281287..1821a3e32fb3 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> > > > *buffer,
> > > >  	if (buffer->scan_mask == NULL)
> > > >  		return -ENOMEM;
> > > >
> > > > +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> > > > +					     GFP_KERNEL);
> > > > +	if (buffer->channel_mask == NULL) {
> > > > +		bitmap_free(buffer->scan_mask);
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > > >
> > > >  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > > >  {
> > > > +	bitmap_free(buffer->channel_mask);
> > > >  	bitmap_free(buffer->scan_mask);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > > @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device  
> > *dev,  
> > > >
> > > >  	/* Ensure ret is 0 or 1. */
> > > >  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > > > -		       indio_dev->buffer->scan_mask);
> > > > +		       indio_dev->buffer->channel_mask);
> > > >
> > > >  	return sprintf(buf, "%d\n", ret);
> > > >  }
> > > > @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct  
> > iio_dev  
> > > > *indio_dev,
> > > >   * buffers might request, hence this code only verifies that the
> > > >   * individual buffers request is plausible.
> > > >   */
> > > > -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> > > > -		      struct iio_buffer *buffer, int bit)
> > > > +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> > > > +				struct iio_buffer *buffer, int bit)
> > > >  {
> > > >  	const unsigned long *mask;
> > > >  	unsigned long *trialmask;
> > > > +	unsigned int ch;
> > > >
> > > >  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> > > >  	if (trialmask == NULL)
> > > > @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> > > > *indio_dev,
> > > >  		WARN(1, "Trying to set scanmask prior to registering
> > > > buffer\n");
> > > >  		goto err_invalid_mask;
> > > >  	}
> > > > -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-  
> > > > >masklength);  
> > > > -	set_bit(bit, trialmask);
> > > > +
> > > > +	set_bit(bit, buffer->channel_mask);
> > > > +
> > > > +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-  
> > > > >num_channels)  
> > > > +		set_bit(indio_dev->channels[ch].scan_index, trialmask);  
> > >
> > > So, here if the channels all have the same scan_index, we will end up with a  
> > scan_mask which is  
> > > different that channel_mask, right? I saw that in our internal driver's we  
> > then just access the  
> > > channel_mask field directly to know what pieces/channels do we need to  
> > enable prior to  
> > > buffering, which implies including buffer_impl.h.  
> > Given that we handle the demux only at the level of scan elements that
> > won't work in general
> > (even if it wasn't a horrible layering issue).  
> 
> Yes, and the driver just adds 16 channels and points all of them to scan_index 0. It then
> sets real_bits and the shift so that userspace can get the right channel bit. So, in the end
> we have just one buffer/scan element with 16bits. My problem here is more architectural...
> We should not directly include "buffer_impl.h" in drivers...
> 
> > >
> > > So, for me it would make sense to compute scan_mask so that it will be the  
> > same as channel_mask  
> > > (hmm but that would be a problem when computing the buffer size...) and  
> > drivers can correctly use  
> > > ` validate_scan_mask ()` cb. Alternatively, we need to expose  
> > channel_mask either on a new cb or  
> > > change the ` validate_scan_mask ()` footprint.  
> > 
> > Excellent points. We need to address support for:
> > 
> > 1) available_scan_mask - if we have complicated rules on mixtures of
> > channels inside
> >    a given buffer element.  
> 
> Maybe one solution to expose channel mask is to check if channel_mask != scan_mask
> before calling the ` validate_scan_mask()`. If it is, we pass channel_mask to the callback.
> Driver's should then know what to do with it...

That's liable to be flakey as there is no requirement for the scan_mask to
be ordered or indeed not have holes.

> 
> > 2) channel enabling though I'm sort of inclined to say that if you are using this
> > approach
> >    you only get information on channels that make up a scan mask element.
> > Tough luck you
> >    may end up enabling more than you'd like.  
> 
> Not sure if I'm fully understanding this point. I believe with this approach channel
> enablement works as before since the core is kind of mapping channel_mask to
> scan_mask. So if we have 16 channels using only 1 scan_element we can still
> enable/disable all 16 channels.

Its more subtle than that.  Because of the mux, a number of different channels can
be enabled by different consumers, but all that is exposed to the driver is
the resulting fused scan_mask across all consumers.  It has no idea what channels
have been enabled if they lie within a scan_mask element.

Hence, whilst there can be individual channel enable and disable attributes
they driver only seems enable and disable of scan mask elements. That means
it needs to turn on ALL of the channels within one scan mask element.
To do anything more complex requires us to carry all the following to the demux
calculator

1) scan_mask
2) channel_mask
3) mapping from channel mask to scan mask

It could be done, but it's potentially nasty.  Even then we don't want to
get into breaking out particular elements within a scan mask element so we'd
end up providing all enabled channels (within each scan mask element)
to the all consumers who are after any of them.

We'd also have to expose the fused channel mask as well as scan mask
to the driver which is not exactly elegant.

That's why I'd suggest initial work uses scan mask as the fundamental
unit of enable / disable, not the channel mask.

Nothing stops then improving that later to deal with the channel mask
fusion needed to work out the enables, but it's not something I'd do
for step 1.

> 
> In the end, if we have a traditional driver with one channel per scan_index, channel_mask
> should be equal to scan_mask. As we start to have more than one channel pointing to the
> same scan_index, these masks will be different.
> 
> > It might be possible to make switch to using a channel mask but given the
> > channel index is
> > implicit that is going to be at least a little bit nasty.
> > 
> > How much does it hurt to not have the ability to separately control channels
> > within
> > a given buffer element?   Userspace can enable / disable them but reality is
> > you'll  
> 
> As long as we are "ok" with the extra amount of allocated memory, I think it would work.
> Though drivers will have to replicate the same data trough all the enabled scan elements...

Hmm. I think we are talking about different things.  Let me give an example.

8 channels in scan mask element 0 size 8 bits, 8 channels in scan mask element 1

Enable a channel in scan mask element 0 on consumer 0, and a
different one on consumer 1.  If they were in different scan mask elements
we'd deliver the first element only to consumer 0 and the second element
only to consumer 1 (that's what the demux does for us)

Here, in what I would suggest for the initial implementation, channel mask
is not exposed at all to buffer setup op (update_scan_mask) - so we
don't know which channels in that scan mask element are needed.
Only answer, turn all 8 on.

In this case we would deliver one 8 bit buffer element to each of the consumers
but it would include the values for all 8 channels (but none from the 8 channels
in our second scan mask element).

This keeps the channel mask logic (for now) separate from the demux
and the buffered capture setup logic, but at the cost of sampling channels
no one cares about.  Note we often do that anyway as a lot of hardware does
not have per channel enables, or is more efficient if we grab all the channels
in a single transaction.

Jonathan

> 
> - Nuno Sá
> 
> > get data for all the channels in a buffer element if any of them are enabled.
> > 
> > Given the demux will copy the whole element anyway (don't want to waste
> > time doing
> > masking inside an element), userspace might get these extra channels
> > anyway if another
> > consumer has enabled them.
> > 
> > Jonathan
> > 
> >   
> > >
> > > - Nuno Sá  
> 

Re: [RFC PATCH 1/4] iio: Move scan mask management to the core

[Jonathan Cameron]

On Mon, 27 Apr 2020 06:25:19 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2020-04-26 at 10:45 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Fri, 24 Apr 2020 08:18:15 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > 
> > > Let the core handle the buffer scan mask management including allocation
> > > and channel selection. Having this handled in a central place rather than
> > > open-coding it all over the place will make it easier to change the
> > > implementation.
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > 
> > Hi Alex,
> > 
> > For some reason I only have patch 1 of this series of 4.
> > 
> > This one looks reasonable to me as abstracts away how it is implemented
> > which is good. A few comments and a question inline.
> >   
> 
> Replies inline.
> 
> Since the discussion about patches 3 & 4 got a little more complicated [than I'd
> like to focus on at this point in time], would it be ok to split patches 1 & 2
> into a separate series? [after discussions are resolved]
> Even just those 2 patches helps me work on 2 kernel trees [the ADI internal &
> the IIO upstream one], as it minimizes the diff.
> 

That would be fine.

> 
> > Jonathan
> > 
> >   
> > > ---
> > >  drivers/iio/buffer/industrialio-buffer-cb.c | 17 ++++------
> > >  drivers/iio/industrialio-buffer.c           | 36 +++++++++++++++------
> > >  drivers/iio/inkern.c                        | 15 +++++++++
> > >  include/linux/iio/consumer.h                | 10 ++++++
> > >  4 files changed, 58 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c
> > > b/drivers/iio/buffer/industrialio-buffer-cb.c
> > > index 47c96f7f4976..b50f1f48cac6 100644
> > > --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> > > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> > > @@ -33,8 +33,7 @@ static int iio_buffer_cb_store_to(struct iio_buffer
> > > *buffer, const void *data)
> > >  static void iio_buffer_cb_release(struct iio_buffer *buffer)
> > >  {
> > >  	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> > > -
> > > -	bitmap_free(cb_buff->buffer.scan_mask);
> > > +	iio_buffer_free_scanmask(buffer);
> > >  	kfree(cb_buff);
> > >  }
> > >  
> > > @@ -72,27 +71,25 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct
> > > device *dev,
> > >  	}
> > >  
> > >  	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> > > -	cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev-  
> > > >masklength,  
> > > -						  GFP_KERNEL);
> > > -	if (cb_buff->buffer.scan_mask == NULL) {
> > > -		ret = -ENOMEM;
> > > +
> > > +	ret = iio_buffer_alloc_scanmask(&cb_buff->buffer, cb_buff->indio_dev);
> > > +	if (ret)
> > >  		goto error_release_channels;
> > > -	}
> > > +
> > >  	chan = &cb_buff->channels[0];
> > >  	while (chan->indio_dev) {
> > >  		if (chan->indio_dev != cb_buff->indio_dev) {
> > >  			ret = -EINVAL;
> > >  			goto error_free_scan_mask;
> > >  		}
> > > -		set_bit(chan->channel->scan_index,
> > > -			cb_buff->buffer.scan_mask);
> > > +		iio_buffer_channel_enable(&cb_buff->buffer, chan);
> > >  		chan++;
> > >  	}
> > >  
> > >  	return cb_buff;
> > >  
> > >  error_free_scan_mask:
> > > -	bitmap_free(cb_buff->buffer.scan_mask);
> > > +	iio_buffer_free_scanmask(&cb_buff->buffer);
> > >  error_release_channels:
> > >  	iio_channel_release_all(cb_buff->channels);
> > >  error_free_cb_buff:
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > index 221157136af6..c06691281287 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -206,6 +206,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> > >  }
> > >  EXPORT_SYMBOL(iio_buffer_init);
> > >  
> > > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > > +			      struct iio_dev *indio_dev)  
> > I'm not sure passing the indio_dev in here makes sense as it
> > obscures that all we are getting from it is the masklength.
> > May be better to pass that explicitly.  
> 
> Passing indio_dev seems to make more sense if we are also doing the channel_mask
> thing in the next patches.
> If we do the buffer->indio_dev back-ref, then we can just pass the buffer.
> But that may obscure things a bit more about the 'masklength'; though, that may
> be one of the intentions I suppose [can't be sure]
> 
> >   
> > > +{
> > > +	if (!indio_dev->masklength)
> > > +		return 0;  
> > 
> > This is a bit of an oddity of the old code.  Any idea why we
> > allow things to continue with a masklength of 0?  Seems to me
> > that it is thoroughly broken if that occurs!  
> 
> See [1]
> 
> >   
> > > +
> > > +	buffer->scan_mask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> > > +	if (buffer->scan_mask == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > > +
> > > +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > > +{
> > > +	bitmap_free(buffer->scan_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > +
> > >  /**
> > >   * iio_buffer_set_attrs - Set buffer specific attributes
> > >   * @buffer: The buffer for which we are setting attributes
> > > @@ -1301,14 +1321,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > >  				indio_dev->scan_index_timestamp =
> > >  					channels[i].scan_index;
> > >  		}
> > > -		if (indio_dev->masklength && buffer->scan_mask == NULL) {  
> 
> [1] I suspect when this patch was written, it had this part in mind [with the
> retun 0]
> So, it preserves behavior a bit.
> The 'buffer->scan_mask == NULL' check is a bit odd; I wonder if the initial
> intent was to also allow to pass a 'scan_mask' from somewhere else.
> 
> So, updating '!indio_dev->masklength' to return -EINVAL in
> iio_buffer_alloc_scanmask() sounds like a good idea, but I'll need to add a note
> in the commit description.

Yup. Sounds good.
> 
> 
> > > -			buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> > > -							  GFP_KERNEL);
> > > -			if (buffer->scan_mask == NULL) {
> > > -				ret = -ENOMEM;
> > > -				goto error_cleanup_dynamic;
> > > -			}
> > > -		}
> > > +
> > > +		ret = iio_buffer_alloc_scanmask(buffer, indio_dev);
> > > +		if (ret)
> > > +			goto error_cleanup_dynamic;
> > >  	}
> > >  
> > >  	buffer->scan_el_group.name = iio_scan_elements_group_name;
> > > @@ -1329,7 +1345,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > >  	return 0;
> > >  
> > >  error_free_scan_mask:
> > > -	bitmap_free(buffer->scan_mask);
> > > +	iio_buffer_free_scanmask(buffer);
> > >  error_cleanup_dynamic:
> > >  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > >  	kfree(indio_dev->buffer->buffer_group.attrs);
> > > @@ -1342,7 +1358,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > >  	if (!indio_dev->buffer)
> > >  		return;
> > >  
> > > -	bitmap_free(indio_dev->buffer->scan_mask);
> > > +	iio_buffer_free_scanmask(indio_dev->buffer);
> > >  	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);
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index ede99e0d5371..f35cb9985edc 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -11,6 +11,7 @@
> > >  
> > >  #include <linux/iio/iio.h>
> > >  #include "iio_core.h"
> > > +#include <linux/iio/buffer_impl.h>
> > >  #include <linux/iio/machine.h>
> > >  #include <linux/iio/driver.h>
> > >  #include <linux/iio/consumer.h>
> > > @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int
> > > val)
> > >  }
> > >  EXPORT_SYMBOL_GPL(iio_write_channel_raw);
> > >  
> > > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > > +			       const struct iio_channel *chan)
> > > +{
> > > +	set_bit(chan->channel->scan_index, buffer->scan_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> > > +
> > > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > > +				const struct iio_channel *chan)
> > > +{
> > > +	clear_bit(chan->channel->scan_index, buffer->scan_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> > > +
> > >  unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> > >  {
> > >  	const struct iio_chan_spec_ext_info *ext_info;
> > > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> > > index c4118dcb8e05..dbc87c26250a 100644
> > > --- a/include/linux/iio/consumer.h
> > > +++ b/include/linux/iio/consumer.h
> > > @@ -12,6 +12,7 @@
> > >  
> > >  struct iio_dev;
> > >  struct iio_chan_spec;
> > > +struct iio_buffer;
> > >  struct device;
> > >  
> > >  /**
> > > @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan,
> > > int *val,
> > >  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> > >  	int *processed, unsigned int scale);
> > >  
> > > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > > +			       const struct iio_channel *chan);
> > > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > > +				const struct iio_channel *chan);
> > > +
> > > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > > +			      struct iio_dev *indio_dev);
> > > +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> > > +
> > >  /**
> > >   * iio_get_channel_ext_info_count() - get number of ext_info attributes
> > >   *				      connected to the channel.  

RE: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Samstag, 2. Mai 2020 19:19
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; lars@metafoo.de
> Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> basis
> 
> [External]
> 
> On Mon, 27 Apr 2020 12:09:18 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: linux-iio-owner@vger.kernel.org <linux-iio-
> owner@vger.kernel.org>
> > > On Behalf Of Jonathan Cameron
> > > Sent: Sonntag, 26. April 2020 12:51
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; lars@metafoo.de
> > > Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> > > basis
> > >
> > > On Fri, 24 Apr 2020 07:51:05 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > > From: linux-iio-owner@vger.kernel.org <linux-iio-
> > > owner@vger.kernel.org>
> > > > > On Behalf Of Alexandru Ardelean
> > > > > Sent: Freitag, 24. April 2020 07:18
> > > > > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> > > > > <alexandru.Ardelean@analog.com>
> > > > > Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> > > basis
> > > > >
> > > > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > > >
> > > > > Now that we support multiple channels with the same scan index we
> can
> > > no
> > > > > longer use the scan mask to track which channels have been enabled.
> > > > > Otherwise it is not possible to enable channels with the same scan
> index
> > > > > independently.
> > > > >
> > > > > Introduce a new channel mask which is used instead of the scan mask
> to
> > > > > track which channels are enabled. Whenever the channel mask is
> > > changed a
> > > > > new scan mask is computed based on it.
> > > > >
> > > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > > > Signed-off-by: Alexandru Ardelean
> <alexandru.ardelean@analog.com>
> > > > > ---
> > > > >  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++-------
> ---
> > > > >  drivers/iio/inkern.c              | 19 +++++++++-
> > > > >  include/linux/iio/buffer_impl.h   |  3 ++
> > > > >  include/linux/iio/consumer.h      |  2 +
> > > > >  4 files changed, 64 insertions(+), 22 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > > > index c06691281287..1821a3e32fb3 100644
> > > > > --- a/drivers/iio/industrialio-buffer.c
> > > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > > @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct
> iio_buffer
> > > > > *buffer,
> > > > >  	if (buffer->scan_mask == NULL)
> > > > >  		return -ENOMEM;
> > > > >
> > > > > +	buffer->channel_mask = bitmap_zalloc(indio_dev-
> >num_channels,
> > > > > +					     GFP_KERNEL);
> > > > > +	if (buffer->channel_mask == NULL) {
> > > > > +		bitmap_free(buffer->scan_mask);
> > > > > +		return -ENOMEM;
> > > > > +	}
> > > > > +
> > > > >  	return 0;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > > > >
> > > > >  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > > > >  {
> > > > > +	bitmap_free(buffer->channel_mask);
> > > > >  	bitmap_free(buffer->scan_mask);
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > > > @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device
> > > *dev,
> > > > >
> > > > >  	/* Ensure ret is 0 or 1. */
> > > > >  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > > > > -		       indio_dev->buffer->scan_mask);
> > > > > +		       indio_dev->buffer->channel_mask);
> > > > >
> > > > >  	return sprintf(buf, "%d\n", ret);
> > > > >  }
> > > > > @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct
> > > iio_dev
> > > > > *indio_dev,
> > > > >   * buffers might request, hence this code only verifies that the
> > > > >   * individual buffers request is plausible.
> > > > >   */
> > > > > -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> > > > > -		      struct iio_buffer *buffer, int bit)
> > > > > +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> > > > > +				struct iio_buffer *buffer, int bit)
> > > > >  {
> > > > >  	const unsigned long *mask;
> > > > >  	unsigned long *trialmask;
> > > > > +	unsigned int ch;
> > > > >
> > > > >  	trialmask = bitmap_zalloc(indio_dev->masklength,
> GFP_KERNEL);
> > > > >  	if (trialmask == NULL)
> > > > > @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> > > > > *indio_dev,
> > > > >  		WARN(1, "Trying to set scanmask prior to registering
> > > > > buffer\n");
> > > > >  		goto err_invalid_mask;
> > > > >  	}
> > > > > -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-
> > > > > >masklength);
> > > > > -	set_bit(bit, trialmask);
> > > > > +
> > > > > +	set_bit(bit, buffer->channel_mask);
> > > > > +
> > > > > +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-
> > > > > >num_channels)
> > > > > +		set_bit(indio_dev->channels[ch].scan_index,
> trialmask);
> > > >
> > > > So, here if the channels all have the same scan_index, we will end up
> with a
> > > scan_mask which is
> > > > different that channel_mask, right? I saw that in our internal driver's we
> > > then just access the
> > > > channel_mask field directly to know what pieces/channels do we need
> to
> > > enable prior to
> > > > buffering, which implies including buffer_impl.h.
> > > Given that we handle the demux only at the level of scan elements that
> > > won't work in general
> > > (even if it wasn't a horrible layering issue).
> >
> > Yes, and the driver just adds 16 channels and points all of them to
> scan_index 0. It then
> > sets real_bits and the shift so that userspace can get the right channel bit.
> So, in the end
> > we have just one buffer/scan element with 16bits. My problem here is
> more architectural...
> > We should not directly include "buffer_impl.h" in drivers...
> >
> > > >
> > > > So, for me it would make sense to compute scan_mask so that it will be
> the
> > > same as channel_mask
> > > > (hmm but that would be a problem when computing the buffer size...)
> and
> > > drivers can correctly use
> > > > ` validate_scan_mask ()` cb. Alternatively, we need to expose
> > > channel_mask either on a new cb or
> > > > change the ` validate_scan_mask ()` footprint.
> > >
> > > Excellent points. We need to address support for:
> > >
> > > 1) available_scan_mask - if we have complicated rules on mixtures of
> > > channels inside
> > >    a given buffer element.
> >
> > Maybe one solution to expose channel mask is to check if channel_mask !=
> scan_mask
> > before calling the ` validate_scan_mask()`. If it is, we pass channel_mask to
> the callback.
> > Driver's should then know what to do with it...
> 
> That's liable to be flakey as there is no requirement for the scan_mask to
> be ordered or indeed not have holes.
> 

Yes, but the patch is adding this code:

`
for_each_set_bit(ch, buffer->channel_mask, indio_dev-  
		num_channels)  
	set_bit(indio_dev->channels[ch].scan_index, trialmask);  
`

So, As I'm understanding we always enable the scan element on which a channel is inserted.
In the end, for a traditional driver with all different scan indexes, the resulting scan mask will
be the same as the channel mask if I'm not missing any subtlety here...

> >
> > > 2) channel enabling though I'm sort of inclined to say that if you are using
> this
> > > approach
> > >    you only get information on channels that make up a scan mask
> element.
> > > Tough luck you
> > >    may end up enabling more than you'd like.
> >
> > Not sure if I'm fully understanding this point. I believe with this approach
> channel
> > enablement works as before since the core is kind of mapping
> channel_mask to
> > scan_mask. So if we have 16 channels using only 1 scan_element we can
> still
> > enable/disable all 16 channels.
> 
> Its more subtle than that.  Because of the mux, a number of different
> channels can
> be enabled by different consumers, but all that is exposed to the driver is
> the resulting fused scan_mask across all consumers.  It has no idea what
> channels
> have been enabled if they lie within a scan_mask element.
> 
> Hence, whilst there can be individual channel enable and disable attributes
> they driver only seems enable and disable of scan mask elements. That
> means
> it needs to turn on ALL of the channels within one scan mask element.
> To do anything more complex requires us to carry all the following to the
> demux
> calculator
> 
> 1) scan_mask
> 2) channel_mask
> 3) mapping from channel mask to scan mask
> 
> It could be done, but it's potentially nasty.  Even then we don't want to
> get into breaking out particular elements within a scan mask element so we'd
> end up providing all enabled channels (within each scan mask element)
> to the all consumers who are after any of them.
> 
> We'd also have to expose the fused channel mask as well as scan mask
> to the driver which is not exactly elegant.
> 
> That's why I'd suggest initial work uses scan mask as the fundamental
> unit of enable / disable, not the channel mask.
> 
> Nothing stops then improving that later to deal with the channel mask
> fusion needed to work out the enables, but it's not something I'd do
> for step 1.
> 
> >
> > In the end, if we have a traditional driver with one channel per scan_index,
> channel_mask
> > should be equal to scan_mask. As we start to have more than one channel
> pointing to the
> > same scan_index, these masks will be different.
> >
> > > It might be possible to make switch to using a channel mask but given the
> > > channel index is
> > > implicit that is going to be at least a little bit nasty.
> > >
> > > How much does it hurt to not have the ability to separately control
> channels
> > > within
> > > a given buffer element?   Userspace can enable / disable them but reality
> is
> > > you'll
> >
> > As long as we are "ok" with the extra amount of allocated memory, I think
> it would work.
> > Though drivers will have to replicate the same data trough all the enabled
> scan elements...
> 
> Hmm. I think we are talking about different things.  Let me give an example.
> 
> 8 channels in scan mask element 0 size 8 bits, 8 channels in scan mask
> element 1
> 
> Enable a channel in scan mask element 0 on consumer 0, and a
> different one on consumer 1.  If they were in different scan mask elements
> we'd deliver the first element only to consumer 0 and the second element
> only to consumer 1 (that's what the demux does for us)
> 
> Here, in what I would suggest for the initial implementation, channel mask
> is not exposed at all to buffer setup op (update_scan_mask) - so we
> don't know which channels in that scan mask element are needed.
> Only answer, turn all 8 on.
> 
> In this case we would deliver one 8 bit buffer element to each of the
> consumers
> but it would include the values for all 8 channels (but none from the 8
> channels
> in our second scan mask element).
> 
> This keeps the channel mask logic (for now) separate from the demux
> and the buffered capture setup logic, but at the cost of sampling channels
> no one cares about.  Note we often do that anyway as a lot of hardware does
> not have per channel enables, or is more efficient if we grab all the channels
> in a single transaction.
> 

Hmm, I see your point now. Looking at the patchset, it also looks like that there's
no intention of doing the demux at the channel/bit level. I also agree on keeping the
granularity at the scan_element level otherwise it can be really unpleasant to implement
and "read" the demux code.

I was more looking for the possibility of passing the channel_mask to the drivers instead
of the scan_mask (when it makes sense to do so) so we can only sample the channels we
are interested in. In the end, we would push the complete scan_element to the iio_buffer
only with the bits we are interested in...

That being sad, I'm also not seeing a big problem in just enabling all the channels for a given
scan element but I might be missing something.

- Nuno Sá

Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Jonathan Cameron]

On Mon, 4 May 2020 08:24:35 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Samstag, 2. Mai 2020 19:19
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org; lars@metafoo.de
> > Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> > basis
> > 
> > [External]
> > 
> > On Mon, 27 Apr 2020 12:09:18 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >   
> > > > From: linux-iio-owner@vger.kernel.org <linux-iio-  
> > owner@vger.kernel.org>  
> > > > On Behalf Of Jonathan Cameron
> > > > Sent: Sonntag, 26. April 2020 12:51
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; linux-
> > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; lars@metafoo.de
> > > > Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> > > > basis
> > > >
> > > > On Fri, 24 Apr 2020 07:51:05 +0000
> > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > >  
> > > > > > From: linux-iio-owner@vger.kernel.org <linux-iio-  
> > > > owner@vger.kernel.org>  
> > > > > > On Behalf Of Alexandru Ardelean
> > > > > > Sent: Freitag, 24. April 2020 07:18
> > > > > > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > > Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> > > > > > <alexandru.Ardelean@analog.com>
> > > > > > Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel  
> > > > basis  
> > > > > >
> > > > > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > > > >
> > > > > > Now that we support multiple channels with the same scan index we  
> > can  
> > > > no  
> > > > > > longer use the scan mask to track which channels have been enabled.
> > > > > > Otherwise it is not possible to enable channels with the same scan  
> > index  
> > > > > > independently.
> > > > > >
> > > > > > Introduce a new channel mask which is used instead of the scan mask  
> > to  
> > > > > > track which channels are enabled. Whenever the channel mask is  
> > > > changed a  
> > > > > > new scan mask is computed based on it.
> > > > > >
> > > > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > > > > Signed-off-by: Alexandru Ardelean  
> > <alexandru.ardelean@analog.com>  
> > > > > > ---
> > > > > >  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++-------  
> > ---  
> > > > > >  drivers/iio/inkern.c              | 19 +++++++++-
> > > > > >  include/linux/iio/buffer_impl.h   |  3 ++
> > > > > >  include/linux/iio/consumer.h      |  2 +
> > > > > >  4 files changed, 64 insertions(+), 22 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-  
> > > > buffer.c  
> > > > > > index c06691281287..1821a3e32fb3 100644
> > > > > > --- a/drivers/iio/industrialio-buffer.c
> > > > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > > > @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct  
> > iio_buffer  
> > > > > > *buffer,
> > > > > >  	if (buffer->scan_mask == NULL)
> > > > > >  		return -ENOMEM;
> > > > > >
> > > > > > +	buffer->channel_mask = bitmap_zalloc(indio_dev-  
> > >num_channels,  
> > > > > > +					     GFP_KERNEL);
> > > > > > +	if (buffer->channel_mask == NULL) {
> > > > > > +		bitmap_free(buffer->scan_mask);
> > > > > > +		return -ENOMEM;
> > > > > > +	}
> > > > > > +
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > > > > >
> > > > > >  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > > > > >  {
> > > > > > +	bitmap_free(buffer->channel_mask);
> > > > > >  	bitmap_free(buffer->scan_mask);
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > > > > @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device  
> > > > *dev,  
> > > > > >
> > > > > >  	/* Ensure ret is 0 or 1. */
> > > > > >  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > > > > > -		       indio_dev->buffer->scan_mask);
> > > > > > +		       indio_dev->buffer->channel_mask);
> > > > > >
> > > > > >  	return sprintf(buf, "%d\n", ret);
> > > > > >  }
> > > > > > @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct  
> > > > iio_dev  
> > > > > > *indio_dev,
> > > > > >   * buffers might request, hence this code only verifies that the
> > > > > >   * individual buffers request is plausible.
> > > > > >   */
> > > > > > -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> > > > > > -		      struct iio_buffer *buffer, int bit)
> > > > > > +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> > > > > > +				struct iio_buffer *buffer, int bit)
> > > > > >  {
> > > > > >  	const unsigned long *mask;
> > > > > >  	unsigned long *trialmask;
> > > > > > +	unsigned int ch;
> > > > > >
> > > > > >  	trialmask = bitmap_zalloc(indio_dev->masklength,  
> > GFP_KERNEL);  
> > > > > >  	if (trialmask == NULL)
> > > > > > @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> > > > > > *indio_dev,
> > > > > >  		WARN(1, "Trying to set scanmask prior to registering
> > > > > > buffer\n");
> > > > > >  		goto err_invalid_mask;
> > > > > >  	}
> > > > > > -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-  
> > > > > > >masklength);  
> > > > > > -	set_bit(bit, trialmask);
> > > > > > +
> > > > > > +	set_bit(bit, buffer->channel_mask);
> > > > > > +
> > > > > > +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-  
> > > > > > >num_channels)  
> > > > > > +		set_bit(indio_dev->channels[ch].scan_index,  
> > trialmask);  
> > > > >
> > > > > So, here if the channels all have the same scan_index, we will end up  
> > with a  
> > > > scan_mask which is  
> > > > > different that channel_mask, right? I saw that in our internal driver's we  
> > > > then just access the  
> > > > > channel_mask field directly to know what pieces/channels do we need  
> > to  
> > > > enable prior to  
> > > > > buffering, which implies including buffer_impl.h.  
> > > > Given that we handle the demux only at the level of scan elements that
> > > > won't work in general
> > > > (even if it wasn't a horrible layering issue).  
> > >
> > > Yes, and the driver just adds 16 channels and points all of them to  
> > scan_index 0. It then  
> > > sets real_bits and the shift so that userspace can get the right channel bit.  
> > So, in the end  
> > > we have just one buffer/scan element with 16bits. My problem here is  
> > more architectural...  
> > > We should not directly include "buffer_impl.h" in drivers...
> > >  
> > > > >
> > > > > So, for me it would make sense to compute scan_mask so that it will be  
> > the  
> > > > same as channel_mask  
> > > > > (hmm but that would be a problem when computing the buffer size...)  
> > and  
> > > > drivers can correctly use  
> > > > > ` validate_scan_mask ()` cb. Alternatively, we need to expose  
> > > > channel_mask either on a new cb or  
> > > > > change the ` validate_scan_mask ()` footprint.  
> > > >
> > > > Excellent points. We need to address support for:
> > > >
> > > > 1) available_scan_mask - if we have complicated rules on mixtures of
> > > > channels inside
> > > >    a given buffer element.  
> > >
> > > Maybe one solution to expose channel mask is to check if channel_mask !=  
> > scan_mask  
> > > before calling the ` validate_scan_mask()`. If it is, we pass channel_mask to  
> > the callback.  
> > > Driver's should then know what to do with it...  
> > 
> > That's liable to be flakey as there is no requirement for the scan_mask to
> > be ordered or indeed not have holes.
> >   
> 
> Yes, but the patch is adding this code:
> 
> `
> for_each_set_bit(ch, buffer->channel_mask, indio_dev-  
> 		num_channels)  
> 	set_bit(indio_dev->channels[ch].scan_index, trialmask);  
> `
> 
> So, As I'm understanding we always enable the scan element on which a channel is inserted.
> In the end, for a traditional driver with all different scan indexes, the resulting scan mask will
> be the same as the channel mask if I'm not missing any subtlety here...

The bits for scan mask are provided by the driver, channel mask elements are simply done
in order of the channel array as we parse it.  Hence scan_mask can have gaps, whereas
channel_mask can't.  I think it's actually more complex than that because not all channels
are in the scan_mask at all.  Now we don't add channels that aren't to either scan_mask or
channel mask (as they have no buffer attributes) but they will change the bit locations
for the channel_mask.

Look at how iio_buffer_add_channel_sysfs is called.

> 
> > >  
> > > > 2) channel enabling though I'm sort of inclined to say that if you are using  
> > this  
> > > > approach
> > > >    you only get information on channels that make up a scan mask  
> > element.  
> > > > Tough luck you
> > > >    may end up enabling more than you'd like.  
> > >
> > > Not sure if I'm fully understanding this point. I believe with this approach  
> > channel  
> > > enablement works as before since the core is kind of mapping  
> > channel_mask to  
> > > scan_mask. So if we have 16 channels using only 1 scan_element we can  
> > still  
> > > enable/disable all 16 channels.  
> > 
> > Its more subtle than that.  Because of the mux, a number of different
> > channels can
> > be enabled by different consumers, but all that is exposed to the driver is
> > the resulting fused scan_mask across all consumers.  It has no idea what
> > channels
> > have been enabled if they lie within a scan_mask element.
> > 
> > Hence, whilst there can be individual channel enable and disable attributes
> > they driver only seems enable and disable of scan mask elements. That
> > means
> > it needs to turn on ALL of the channels within one scan mask element.
> > To do anything more complex requires us to carry all the following to the
> > demux
> > calculator
> > 
> > 1) scan_mask
> > 2) channel_mask
> > 3) mapping from channel mask to scan mask
> > 
> > It could be done, but it's potentially nasty.  Even then we don't want to
> > get into breaking out particular elements within a scan mask element so we'd
> > end up providing all enabled channels (within each scan mask element)
> > to the all consumers who are after any of them.
> > 
> > We'd also have to expose the fused channel mask as well as scan mask
> > to the driver which is not exactly elegant.
> > 
> > That's why I'd suggest initial work uses scan mask as the fundamental
> > unit of enable / disable, not the channel mask.
> > 
> > Nothing stops then improving that later to deal with the channel mask
> > fusion needed to work out the enables, but it's not something I'd do
> > for step 1.
> >   
> > >
> > > In the end, if we have a traditional driver with one channel per scan_index,  
> > channel_mask  
> > > should be equal to scan_mask. As we start to have more than one channel  
> > pointing to the  
> > > same scan_index, these masks will be different.
> > >  
> > > > It might be possible to make switch to using a channel mask but given the
> > > > channel index is
> > > > implicit that is going to be at least a little bit nasty.
> > > >
> > > > How much does it hurt to not have the ability to separately control  
> > channels  
> > > > within
> > > > a given buffer element?   Userspace can enable / disable them but reality  
> > is  
> > > > you'll  
> > >
> > > As long as we are "ok" with the extra amount of allocated memory, I think  
> > it would work.  
> > > Though drivers will have to replicate the same data trough all the enabled  
> > scan elements...
> > 
> > Hmm. I think we are talking about different things.  Let me give an example.
> > 
> > 8 channels in scan mask element 0 size 8 bits, 8 channels in scan mask
> > element 1
> > 
> > Enable a channel in scan mask element 0 on consumer 0, and a
> > different one on consumer 1.  If they were in different scan mask elements
> > we'd deliver the first element only to consumer 0 and the second element
> > only to consumer 1 (that's what the demux does for us)
> > 
> > Here, in what I would suggest for the initial implementation, channel mask
> > is not exposed at all to buffer setup op (update_scan_mask) - so we
> > don't know which channels in that scan mask element are needed.
> > Only answer, turn all 8 on.
> > 
> > In this case we would deliver one 8 bit buffer element to each of the
> > consumers
> > but it would include the values for all 8 channels (but none from the 8
> > channels
> > in our second scan mask element).
> > 
> > This keeps the channel mask logic (for now) separate from the demux
> > and the buffered capture setup logic, but at the cost of sampling channels
> > no one cares about.  Note we often do that anyway as a lot of hardware does
> > not have per channel enables, or is more efficient if we grab all the channels
> > in a single transaction.
> >   
> 
> Hmm, I see your point now. Looking at the patchset, it also looks like that there's
> no intention of doing the demux at the channel/bit level. I also agree on keeping the
> granularity at the scan_element level otherwise it can be really unpleasant to implement
> and "read" the demux code.
> 
> I was more looking for the possibility of passing the channel_mask to the drivers instead
> of the scan_mask (when it makes sense to do so) so we can only sample the channels we
> are interested in. In the end, we would push the complete scan_element to the iio_buffer
> only with the bits we are interested in...
To do that you'd have to deal with fusing the channel masks from multiple consumers and
checking that's possible each time we enable a channel (so channel_mask_available etc)
> 
> That being sad, I'm also not seeing a big problem in just enabling all the channels for a given
> scan element but I might be missing something.

Definitely easier for a 'first pass'.  We can be more clever later - the result of
adding fine grained control should have no impact on the perceived output - it's just
an efficiency improvement.
> 
> - Nuno Sá

Thanks,

Jonathan