[PATCH 0/3] iio: Hardware buffer improvements

[PATCH 0/3] iio: Hardware buffer improvements

[Lars-Peter Clausen]

Hi,

This series has a few small improvements for hardware buffer support.

The first patch makes sure that we always compute the masklength for the
scanmask, even if the driver does not register a userspace accessable
buffer. This makes it possible to support cases where the data of the
device can not be accessed by software but is directly passed to a
different device which can then register a in kernel consumer to control
the buffer of the device.

The second patch adds support for specifying the supported buffer modes for
each buffer type. This makes sure that the correct mode is chosen when a
device driver support multiple types, e.g. both software and hardware
buffers and also makes sure that we can attach buffers which are not
supported by the device (e.g. callback buffer to sca3000).

The last patch makes sure that when running in hardware mode the scan mask
matching is strict since there is no software demuxer in that case.

- Lars

Lars-Peter Clausen (3):
  iio: Always compute masklength
  iio: Specify supported modes for buffers
  iio: Require strict scan mask matching in hardware mode

 drivers/iio/buffer_cb.c                  |  2 ++
 drivers/iio/industrialio-buffer.c        | 58 ++++++++++++++++++++++++--------
 drivers/iio/kfifo_buf.c                  |  2 ++
 drivers/staging/iio/accel/sca3000_ring.c |  2 ++
 include/linux/iio/buffer.h               |  3 ++
 5 files changed, 53 insertions(+), 14 deletions(-)

-- 
2.1.4

[PATCH 1/3] iio: Always compute masklength

[Lars-Peter Clausen]

Even if no userspace consumer buffer is attached to the IIO device at
registration we still need to compute the masklength, since it is possible
that a in-kernel consumer buffer is going to get attached to the device at
a later point.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-buffer.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 1129125..a72db00 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -968,6 +968,15 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	int ret, i, attrn, attrcount, attrcount_orig = 0;
 	const struct iio_chan_spec *channels;
 
+	channels = indio_dev->channels;
+	if (channels) {
+		int ml = indio_dev->masklength;
+
+		for (i = 0; i < indio_dev->num_channels; i++)
+			ml = max(ml, channels[i].scan_index + 1);
+		indio_dev->masklength = ml;
+	}
+
 	if (!buffer)
 		return 0;
 
@@ -1011,12 +1020,6 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 			if (channels[i].scan_index < 0)
 				continue;
 
-			/* Establish necessary mask length */
-			if (channels[i].scan_index >
-			    (int)indio_dev->masklength - 1)
-				indio_dev->masklength
-					= channels[i].scan_index + 1;
-
 			ret = iio_buffer_add_channel_sysfs(indio_dev,
 							 &channels[i]);
 			if (ret < 0)
-- 
2.1.4

[PATCH 2/3] iio: Specify supported modes for buffers

[Lars-Peter Clausen]

For each buffer type specify the supported device modes for this buffer.
This allows us for devices which support multiple different operating modes
to pick the correct operating mode based on the modes supported by the
attached buffers.

It also prevents that buffers with conflicting modes are attached
to a device at the same time or that a buffer with a non-supported mode is
attached to a device (e.g. in-kernel callback buffer to a device only
supporting hardware mode).

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/buffer_cb.c                  |  2 ++
 drivers/iio/industrialio-buffer.c        | 18 +++++++++++++++---
 drivers/iio/kfifo_buf.c                  |  2 ++
 drivers/staging/iio/accel/sca3000_ring.c |  2 ++
 include/linux/iio/buffer.h               |  3 +++
 5 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
index eb46e72..1648e6e 100644
--- a/drivers/iio/buffer_cb.c
+++ b/drivers/iio/buffer_cb.c
@@ -33,6 +33,8 @@ static void iio_buffer_cb_release(struct iio_buffer *buffer)
 static const struct iio_buffer_access_funcs iio_cb_access = {
 	.store_to = &iio_buffer_cb_store_to,
 	.release = &iio_buffer_cb_release,
+
+	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
 };
 
 struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a72db00..4250e97 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -604,6 +604,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	const unsigned long *scan_mask;
 	struct iio_buffer *buffer;
 	bool scan_timestamp;
+	unsigned int modes;
 
 	memset(config, 0, sizeof(*config));
 
@@ -615,12 +616,23 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 		list_is_singular(&indio_dev->buffer_list))
 			return 0;
 
+	modes = indio_dev->modes;
+
+	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+		if (buffer == remove_buffer)
+			continue;
+		modes &= buffer->access->modes;
+	}
+
+	if (insert_buffer)
+		modes &= insert_buffer->access->modes;
+
 	/* Definitely possible for devices to support both of these. */
-	if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
+	if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
 		config->mode = INDIO_BUFFER_TRIGGERED;
-	} else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
+	} else if (modes & INDIO_BUFFER_HARDWARE) {
 		config->mode = INDIO_BUFFER_HARDWARE;
-	} else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
+	} else if (modes & INDIO_BUFFER_SOFTWARE) {
 		config->mode = INDIO_BUFFER_SOFTWARE;
 	} else {
 		/* Can only occur on first buffer */
diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
index 847ca56..db15684 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -135,6 +135,8 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
 	.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
 	.set_length = &iio_set_length_kfifo,
 	.release = &iio_kfifo_buffer_release,
+
+	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
 };
 
 struct iio_buffer *iio_kfifo_allocate(void)
diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
index 8589eade..23685e7 100644
--- a/drivers/staging/iio/accel/sca3000_ring.c
+++ b/drivers/staging/iio/accel/sca3000_ring.c
@@ -258,6 +258,8 @@ static const struct iio_buffer_access_funcs sca3000_ring_access_funcs = {
 	.read_first_n = &sca3000_read_first_n_hw_rb,
 	.data_available = sca3000_ring_buf_data_available,
 	.release = sca3000_ring_release,
+
+	.modes = INDIO_BUFFER_HARDWARE,
 };
 
 int sca3000_configure_ring(struct iio_dev *indio_dev)
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index eb8622b..1600c55 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -29,6 +29,7 @@ struct iio_buffer;
  * @set_length:		set number of datums in buffer
  * @release:		called when the last reference to the buffer is dropped,
  *			should free all resources allocated by the buffer.
+ * @modes:		Supported operating modes by this buffer type
  *
  * The purpose of this structure is to make the buffer element
  * modular as event for a given driver, different usecases may require
@@ -51,6 +52,8 @@ struct iio_buffer_access_funcs {
 	int (*set_length)(struct iio_buffer *buffer, int length);
 
 	void (*release)(struct iio_buffer *buffer);
+
+	unsigned int modes;
 };
 
 /**
-- 
2.1.4

[PATCH 3/3] iio: Require strict scan mask matching in hardware mode

[Lars-Peter Clausen]

In hardware mode we can not use the software demuxer, this means that the
selected scan mask needs to match one of the available scan masks exactly.

It also means that all attached buffers need to use the same scan mask.
Given that when operating in hardware mode there is typically only a single
buffer attached to the device this not an issue. Add a sanity check to make
sure that only a single buffer is attached in hardware mode nevertheless.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-buffer.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 4250e97..e174fcc 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -239,13 +239,19 @@ static ssize_t iio_scan_el_show(struct device *dev,
 /* Note NULL used as error indicator as it doesn't make sense. */
 static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
 					  unsigned int masklength,
-					  const unsigned long *mask)
+					  const unsigned long *mask,
+					  bool strict)
 {
 	if (bitmap_empty(mask, masklength))
 		return NULL;
 	while (*av_masks) {
-		if (bitmap_subset(mask, av_masks, masklength))
-			return av_masks;
+		if (strict) {
+			if (bitmap_equal(mask, av_masks, masklength))
+				return av_masks;
+		} else {
+			if (bitmap_subset(mask, av_masks, masklength))
+				return av_masks;
+		}
 		av_masks += BITS_TO_LONGS(masklength);
 	}
 	return NULL;
@@ -295,7 +301,7 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
 	if (indio_dev->available_scan_masks) {
 		mask = iio_scan_mask_match(indio_dev->available_scan_masks,
 					   indio_dev->masklength,
-					   trialmask);
+					   trialmask, false);
 		if (!mask)
 			goto err_invalid_mask;
 	}
@@ -602,6 +608,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 {
 	unsigned long *compound_mask;
 	const unsigned long *scan_mask;
+	bool strict_scanmask = false;
 	struct iio_buffer *buffer;
 	bool scan_timestamp;
 	unsigned int modes;
@@ -631,7 +638,14 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
 		config->mode = INDIO_BUFFER_TRIGGERED;
 	} else if (modes & INDIO_BUFFER_HARDWARE) {
+		/*
+		 * Keep things simple for now and only allow a single buffer to
+		 * be connected in hardware mode.
+		 */
+		if (insert_buffer && !list_empty(&indio_dev->buffer_list))
+			return -EINVAL;
 		config->mode = INDIO_BUFFER_HARDWARE;
+		strict_scanmask = true;
 	} else if (modes & INDIO_BUFFER_SOFTWARE) {
 		config->mode = INDIO_BUFFER_SOFTWARE;
 	} else {
@@ -666,7 +680,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	if (indio_dev->available_scan_masks) {
 		scan_mask = iio_scan_mask_match(indio_dev->available_scan_masks,
 				    indio_dev->masklength,
-				    compound_mask);
+				    compound_mask,
+				    strict_scanmask);
 		kfree(compound_mask);
 		if (scan_mask == NULL)
 			return -EINVAL;
-- 
2.1.4

Re: [PATCH 1/3] iio: Always compute masklength

[Jonathan Cameron]

On 29/05/15 17:14, Lars-Peter Clausen wrote:
> Even if no userspace consumer buffer is attached to the IIO device at
> registration we still need to compute the masklength, since it is possible
> that a in-kernel consumer buffer is going to get attached to the device at
> a later point.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Not sure we actually have this condition as of yet, but I guess it 'might'
occur.  Certainly would be good to allow for IIO devices without an
IIO 'front end' in the future.
> ---
>  drivers/iio/industrialio-buffer.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 1129125..a72db00 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -968,6 +968,15 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	int ret, i, attrn, attrcount, attrcount_orig = 0;
>  	const struct iio_chan_spec *channels;
>  
> +	channels = indio_dev->channels;
> +	if (channels) {
> +		int ml = indio_dev->masklength;
> +
> +		for (i = 0; i < indio_dev->num_channels; i++)
> +			ml = max(ml, channels[i].scan_index + 1);
> +		indio_dev->masklength = ml;
> +	}
> +
>  	if (!buffer)
>  		return 0;
>  
> @@ -1011,12 +1020,6 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  			if (channels[i].scan_index < 0)
>  				continue;
>  
> -			/* Establish necessary mask length */
> -			if (channels[i].scan_index >
> -			    (int)indio_dev->masklength - 1)
> -				indio_dev->masklength
> -					= channels[i].scan_index + 1;
> -
>  			ret = iio_buffer_add_channel_sysfs(indio_dev,
>  							 &channels[i]);
>  			if (ret < 0)
> 

Re: [PATCH 2/3] iio: Specify supported modes for buffers

[Jonathan Cameron]

On 29/05/15 17:14, Lars-Peter Clausen wrote:
> For each buffer type specify the supported device modes for this buffer.
> This allows us for devices which support multiple different operating modes
> to pick the correct operating mode based on the modes supported by the
> attached buffers.
> 
> It also prevents that buffers with conflicting modes are attached
> to a device at the same time or that a buffer with a non-supported mode is
> attached to a device (e.g. in-kernel callback buffer to a device only
> supporting hardware mode).
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied.

I really need to wire up the sca3000 I have around here somewhere
to a board and rewrite that driver.  Still even in it's current
form it is exercising corner cases that aren't touched by others.

Jonathan
> ---
>  drivers/iio/buffer_cb.c                  |  2 ++
>  drivers/iio/industrialio-buffer.c        | 18 +++++++++++++++---
>  drivers/iio/kfifo_buf.c                  |  2 ++
>  drivers/staging/iio/accel/sca3000_ring.c |  2 ++
>  include/linux/iio/buffer.h               |  3 +++
>  5 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
> index eb46e72..1648e6e 100644
> --- a/drivers/iio/buffer_cb.c
> +++ b/drivers/iio/buffer_cb.c
> @@ -33,6 +33,8 @@ static void iio_buffer_cb_release(struct iio_buffer *buffer)
>  static const struct iio_buffer_access_funcs iio_cb_access = {
>  	.store_to = &iio_buffer_cb_store_to,
>  	.release = &iio_buffer_cb_release,
> +
> +	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
>  };
>  
>  struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a72db00..4250e97 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -604,6 +604,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	const unsigned long *scan_mask;
>  	struct iio_buffer *buffer;
>  	bool scan_timestamp;
> +	unsigned int modes;
>  
>  	memset(config, 0, sizeof(*config));
>  
> @@ -615,12 +616,23 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  		list_is_singular(&indio_dev->buffer_list))
>  			return 0;
>  
> +	modes = indio_dev->modes;
> +
> +	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +		if (buffer == remove_buffer)
> +			continue;
> +		modes &= buffer->access->modes;
> +	}
> +
> +	if (insert_buffer)
> +		modes &= insert_buffer->access->modes;
> +
>  	/* Definitely possible for devices to support both of these. */
> -	if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
> +	if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
>  		config->mode = INDIO_BUFFER_TRIGGERED;
> -	} else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
> +	} else if (modes & INDIO_BUFFER_HARDWARE) {
>  		config->mode = INDIO_BUFFER_HARDWARE;
> -	} else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
> +	} else if (modes & INDIO_BUFFER_SOFTWARE) {
>  		config->mode = INDIO_BUFFER_SOFTWARE;
>  	} else {
>  		/* Can only occur on first buffer */
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
> index 847ca56..db15684 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -135,6 +135,8 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
>  	.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
>  	.set_length = &iio_set_length_kfifo,
>  	.release = &iio_kfifo_buffer_release,
> +
> +	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
>  };
>  
>  struct iio_buffer *iio_kfifo_allocate(void)
> diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
> index 8589eade..23685e7 100644
> --- a/drivers/staging/iio/accel/sca3000_ring.c
> +++ b/drivers/staging/iio/accel/sca3000_ring.c
> @@ -258,6 +258,8 @@ static const struct iio_buffer_access_funcs sca3000_ring_access_funcs = {
>  	.read_first_n = &sca3000_read_first_n_hw_rb,
>  	.data_available = sca3000_ring_buf_data_available,
>  	.release = sca3000_ring_release,
> +
> +	.modes = INDIO_BUFFER_HARDWARE,
>  };
>  
>  int sca3000_configure_ring(struct iio_dev *indio_dev)
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index eb8622b..1600c55 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -29,6 +29,7 @@ struct iio_buffer;
>   * @set_length:		set number of datums in buffer
>   * @release:		called when the last reference to the buffer is dropped,
>   *			should free all resources allocated by the buffer.
> + * @modes:		Supported operating modes by this buffer type
>   *
>   * The purpose of this structure is to make the buffer element
>   * modular as event for a given driver, different usecases may require
> @@ -51,6 +52,8 @@ struct iio_buffer_access_funcs {
>  	int (*set_length)(struct iio_buffer *buffer, int length);
>  
>  	void (*release)(struct iio_buffer *buffer);
> +
> +	unsigned int modes;
>  };
>  
>  /**
> 

Re: [PATCH 3/3] iio: Require strict scan mask matching in hardware mode

[Jonathan Cameron]

On 29/05/15 17:14, Lars-Peter Clausen wrote:
> In hardware mode we can not use the software demuxer, this means that the
> selected scan mask needs to match one of the available scan masks exactly.
> 
> It also means that all attached buffers need to use the same scan mask.
> Given that when operating in hardware mode there is typically only a single
> buffer attached to the device this not an issue. Add a sanity check to make
> sure that only a single buffer is attached in hardware mode nevertheless.
> 
That pretty much sums up why devices supporting only hardware buffers
are probably not going to be a long term feature!
Mind you for fast devices we may want to allow forced bypassing of the
demux (i.e. a hardware buffer).

Out of curiosity is this series a precursor to another hardware buffered
device or just a useful intellectual exercise?


Applied. 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>  drivers/iio/industrialio-buffer.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 4250e97..e174fcc 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -239,13 +239,19 @@ static ssize_t iio_scan_el_show(struct device *dev,
>  /* Note NULL used as error indicator as it doesn't make sense. */
>  static const unsigned long *iio_scan_mask_match(const unsigned long *av_masks,
>  					  unsigned int masklength,
> -					  const unsigned long *mask)
> +					  const unsigned long *mask,
> +					  bool strict)
>  {
>  	if (bitmap_empty(mask, masklength))
>  		return NULL;
>  	while (*av_masks) {
> -		if (bitmap_subset(mask, av_masks, masklength))
> -			return av_masks;
> +		if (strict) {
> +			if (bitmap_equal(mask, av_masks, masklength))
> +				return av_masks;
> +		} else {
> +			if (bitmap_subset(mask, av_masks, masklength))
> +				return av_masks;
> +		}
>  		av_masks += BITS_TO_LONGS(masklength);
>  	}
>  	return NULL;
> @@ -295,7 +301,7 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
>  	if (indio_dev->available_scan_masks) {
>  		mask = iio_scan_mask_match(indio_dev->available_scan_masks,
>  					   indio_dev->masklength,
> -					   trialmask);
> +					   trialmask, false);
>  		if (!mask)
>  			goto err_invalid_mask;
>  	}
> @@ -602,6 +608,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  {
>  	unsigned long *compound_mask;
>  	const unsigned long *scan_mask;
> +	bool strict_scanmask = false;
>  	struct iio_buffer *buffer;
>  	bool scan_timestamp;
>  	unsigned int modes;
> @@ -631,7 +638,14 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
>  		config->mode = INDIO_BUFFER_TRIGGERED;
>  	} else if (modes & INDIO_BUFFER_HARDWARE) {
> +		/*
> +		 * Keep things simple for now and only allow a single buffer to
> +		 * be connected in hardware mode.
> +		 */
> +		if (insert_buffer && !list_empty(&indio_dev->buffer_list))
> +			return -EINVAL;
>  		config->mode = INDIO_BUFFER_HARDWARE;
> +		strict_scanmask = true;
>  	} else if (modes & INDIO_BUFFER_SOFTWARE) {
>  		config->mode = INDIO_BUFFER_SOFTWARE;
>  	} else {
> @@ -666,7 +680,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	if (indio_dev->available_scan_masks) {
>  		scan_mask = iio_scan_mask_match(indio_dev->available_scan_masks,
>  				    indio_dev->masklength,
> -				    compound_mask);
> +				    compound_mask,
> +				    strict_scanmask);
>  		kfree(compound_mask);
>  		if (scan_mask == NULL)
>  			return -EINVAL;
> 

Re: [PATCH 3/3] iio: Require strict scan mask matching in hardware mode

[Lars-Peter Clausen]

On 06/01/2015 12:34 PM, Jonathan Cameron wrote:
> On 29/05/15 17:14, Lars-Peter Clausen wrote:
>> In hardware mode we can not use the software demuxer, this means that the
>> selected scan mask needs to match one of the available scan masks exactly.
>>
>> It also means that all attached buffers need to use the same scan mask.
>> Given that when operating in hardware mode there is typically only a single
>> buffer attached to the device this not an issue. Add a sanity check to make
>> sure that only a single buffer is attached in hardware mode nevertheless.
>>
> That pretty much sums up why devices supporting only hardware buffers
> are probably not going to be a long term feature!
> Mind you for fast devices we may want to allow forced bypassing of the
> demux (i.e. a hardware buffer).
>
> Out of curiosity is this series a precursor to another hardware buffered
> device or just a useful intellectual exercise?

It's all for real hardware and I'm already using it in some projects. E.g. 
one thing I'm working on is adding optional DMA support for some of the 
converters, if DMA is available it will use hardware mode. If no DMA is 
available or the consumer wants to run in software triggered mode fallback 
to software triggered mode.

- Lars

Re: [PATCH 3/3] iio: Require strict scan mask matching in hardware mode

[Jonathan Cameron]

On 06/03/2015 06:19 PM, Lars-Peter Clausen wrote:
> On 06/01/2015 12:34 PM, Jonathan Cameron wrote:
>> On 29/05/15 17:14, Lars-Peter Clausen wrote:
>>> In hardware mode we can not use the software demuxer, this means that
>>> the
>>> selected scan mask needs to match one of the available scan masks
>>> exactly.
>>>
>>> It also means that all attached buffers need to use the same scan mask.
>>> Given that when operating in hardware mode there is typically only a
>>> single
>>> buffer attached to the device this not an issue. Add a sanity check
>>> to make
>>> sure that only a single buffer is attached in hardware mode
>>> nevertheless.
>>>
>> That pretty much sums up why devices supporting only hardware buffers
>> are probably not going to be a long term feature!
>> Mind you for fast devices we may want to allow forced bypassing of the
>> demux (i.e. a hardware buffer).
>>
>> Out of curiosity is this series a precursor to another hardware buffered
>> device or just a useful intellectual exercise?
>
> It's all for real hardware and I'm already using it in some projects.
> E.g. one thing I'm working on is adding optional DMA support for some of
> the converters, if DMA is available it will use hardware mode. If no DMA
> is available or the consumer wants to run in software triggered mode
> fallback to software triggered mode.
Cool.  Looking forward to seeing the use cases!
>
> - Lars
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: Specify supported modes for buffers

[Jonathan Cameron]

On 29/05/15 17:14, Lars-Peter Clausen wrote:
> For each buffer type specify the supported device modes for this buffer.
> This allows us for devices which support multiple different operating modes
> to pick the correct operating mode based on the modes supported by the
> attached buffers.
> 
> It also prevents that buffers with conflicting modes are attached
> to a device at the same time or that a buffer with a non-supported mode is
> attached to a device (e.g. in-kernel callback buffer to a device only
> supporting hardware mode).
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

This email is more of a heads up than anything else.  We accidentally
broke the am335x driver doing this as it is specifying that it has
a hardware buffer whereas is should be a software buffer as the buffer
exposed to userspace is a software one (kfifo).

Hardware buffer should be reserved for those rare cases where we really
are talking directly to the hardware (and as such can't do some intermediate
stuff).  In this case we are reading from a fifo in the hardware but then
pushing it into the software buffer.

I'll send a patch shortly.

Jonathan
> ---
>  drivers/iio/buffer_cb.c                  |  2 ++
>  drivers/iio/industrialio-buffer.c        | 18 +++++++++++++++---
>  drivers/iio/kfifo_buf.c                  |  2 ++
>  drivers/staging/iio/accel/sca3000_ring.c |  2 ++
>  include/linux/iio/buffer.h               |  3 +++
>  5 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
> index eb46e72..1648e6e 100644
> --- a/drivers/iio/buffer_cb.c
> +++ b/drivers/iio/buffer_cb.c
> @@ -33,6 +33,8 @@ static void iio_buffer_cb_release(struct iio_buffer *buffer)
>  static const struct iio_buffer_access_funcs iio_cb_access = {
>  	.store_to = &iio_buffer_cb_store_to,
>  	.release = &iio_buffer_cb_release,
> +
> +	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
>  };
>  
>  struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a72db00..4250e97 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -604,6 +604,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  	const unsigned long *scan_mask;
>  	struct iio_buffer *buffer;
>  	bool scan_timestamp;
> +	unsigned int modes;
>  
>  	memset(config, 0, sizeof(*config));
>  
> @@ -615,12 +616,23 @@ static int iio_verify_update(struct iio_dev *indio_dev,
>  		list_is_singular(&indio_dev->buffer_list))
>  			return 0;
>  
> +	modes = indio_dev->modes;
> +
> +	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> +		if (buffer == remove_buffer)
> +			continue;
> +		modes &= buffer->access->modes;
> +	}
> +
> +	if (insert_buffer)
> +		modes &= insert_buffer->access->modes;
> +
>  	/* Definitely possible for devices to support both of these. */
> -	if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
> +	if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
>  		config->mode = INDIO_BUFFER_TRIGGERED;
> -	} else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
> +	} else if (modes & INDIO_BUFFER_HARDWARE) {
>  		config->mode = INDIO_BUFFER_HARDWARE;
> -	} else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
> +	} else if (modes & INDIO_BUFFER_SOFTWARE) {
>  		config->mode = INDIO_BUFFER_SOFTWARE;
>  	} else {
>  		/* Can only occur on first buffer */
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
> index 847ca56..db15684 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -135,6 +135,8 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
>  	.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
>  	.set_length = &iio_set_length_kfifo,
>  	.release = &iio_kfifo_buffer_release,
> +
> +	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
>  };
>  
>  struct iio_buffer *iio_kfifo_allocate(void)
> diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
> index 8589eade..23685e7 100644
> --- a/drivers/staging/iio/accel/sca3000_ring.c
> +++ b/drivers/staging/iio/accel/sca3000_ring.c
> @@ -258,6 +258,8 @@ static const struct iio_buffer_access_funcs sca3000_ring_access_funcs = {
>  	.read_first_n = &sca3000_read_first_n_hw_rb,
>  	.data_available = sca3000_ring_buf_data_available,
>  	.release = sca3000_ring_release,
> +
> +	.modes = INDIO_BUFFER_HARDWARE,
>  };
>  
>  int sca3000_configure_ring(struct iio_dev *indio_dev)
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index eb8622b..1600c55 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -29,6 +29,7 @@ struct iio_buffer;
>   * @set_length:		set number of datums in buffer
>   * @release:		called when the last reference to the buffer is dropped,
>   *			should free all resources allocated by the buffer.
> + * @modes:		Supported operating modes by this buffer type
>   *
>   * The purpose of this structure is to make the buffer element
>   * modular as event for a given driver, different usecases may require
> @@ -51,6 +52,8 @@ struct iio_buffer_access_funcs {
>  	int (*set_length)(struct iio_buffer *buffer, int length);
>  
>  	void (*release)(struct iio_buffer *buffer);
> +
> +	unsigned int modes;
>  };
>  
>  /**
>