[PATCH v5 0/3] iio: add support for hardware fifos

[PATCH v5 0/3] iio: add support for hardware fifos

[Octavian Purdila]

Sorry for spawning a new patch set so quickly, but based on the
comments from Lars I rewrote the read watermark logic and the patches
are now hopefully easier to review than the previous patch set.

Changes since v4:

 * rewrite the watermark read logic to fix an undeflow issue and allow
   draining when the buffer is disabled

 * manipulate the watermark field under the device lock

 * use wake_up_interruptible_poll

 * bmc150: flush the fifo before disabling the buffer and take the
   device mutex when emabling/disabling the fifo


Changes since v3:

* remove hwfifo_length and make hwfifo_watermark read-only

* remove trigger for hardware fifo

* use the buffer watermark as a hint for the hardware fifo

* hwfifo_watermark is negative if the device does not support hardware
  fifo, 0 if hardware fifo is supported but currently disabled and a
  strictly positive value if hardware fifo is enabled in which case
  this is the watermark for the hardware fifo

* the hardware fifo is activated by the device driver when it makes
  sense (e.g. at buffer enable time if there is no conflicting
  trigger)

* move the hwfifo operations to struct iio_info

* remove the flush from the poll operation - it causes unnecessary
  flush operations

* move the wait condition logic in a separate function to make it more
  readable

* bmc150: make sure to check the I2C bus supports either full i2c or
  at least SMBUS i2c block read as fifo reads must do a burst read of
  the whole frame (all 3 axis)

* bmc150: rework the way we timestamp the samples stored in the fifo
  to account for sampling frequency variations from device to device



Josselin Costanzi (1):
  iio: add watermark logic to iio read and poll

Octavian Purdila (2):
  iio: add support for hardware fifo
  iio: bmc150_accel: add support for hardware fifo

 Documentation/ABI/testing/sysfs-bus-iio  |  40 ++++
 drivers/iio/accel/bmc150-accel.c         | 363 +++++++++++++++++++++++++++++--
 drivers/iio/industrialio-buffer.c        | 177 +++++++++++++--
 drivers/iio/kfifo_buf.c                  |  11 +-
 drivers/staging/iio/accel/sca3000_ring.c |   4 +-
 include/linux/iio/buffer.h               |   8 +-
 include/linux/iio/iio.h                  |  26 +++
 7 files changed, 581 insertions(+), 48 deletions(-)

-- 
1.9.1

[PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Octavian Purdila]

From: Josselin Costanzi <josselin.costanzi@mobile-devices.fr>

Currently the IIO buffer blocking read only wait until at least one
data element is available.
This patch makes the reader sleep until enough data is collected before
returning to userspace. This should limit the read() calls count when
trying to get data in batches.

Co-author: Yannick Bedhomme <yannick.bedhomme@mobile-devices.fr>
Signed-off-by: Josselin Costanzi <josselin.costanzi@mobile-devices.fr>
[rebased and remove buffer timeout]
Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio  |  15 ++++
 drivers/iio/industrialio-buffer.c        | 118 +++++++++++++++++++++++++++----
 drivers/iio/kfifo_buf.c                  |  11 ++-
 drivers/staging/iio/accel/sca3000_ring.c |   4 +-
 include/linux/iio/buffer.h               |   8 ++-
 5 files changed, 129 insertions(+), 27 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 9a70c31..1283ca7 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1249,3 +1249,18 @@ Contact:	linux-iio@vger.kernel.org
 Description:
 		Specifies number of seconds in which we compute the steps
 		that occur in order to decide if the consumer is making steps.
+
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/watermark
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A single positive integer specifying the maximum number of scan
+		elements to wait for.
+		Poll will block until the watermark is reached.
+		Blocking read will wait until the minimum between the requested
+		read amount or the low water mark is available.
+		Non-blocking read will retrieve the available samples from the
+		buffer even if there are less samples then watermark level. This
+		allows the application to block on poll with a timeout and read
+		the available samples after the timeout expires and thus have a
+		maximum delay guarantee.
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index c2d5440..a4f4f07 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -37,11 +37,28 @@ static bool iio_buffer_is_active(struct iio_buffer *buf)
 	return !list_empty(&buf->buffer_list);
 }
 
-static bool iio_buffer_data_available(struct iio_buffer *buf)
+static size_t iio_buffer_data_available(struct iio_buffer *buf)
 {
 	return buf->access->data_available(buf);
 }
 
+static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
+			     size_t to_wait)
+{
+	/* wakeup if the device was unregistered */
+	if (!indio_dev->info)
+		return true;
+
+	/* drain the buffer if it was disabled */
+	if (!iio_buffer_is_active(buf))
+		to_wait = min_t(size_t, to_wait, 1);
+
+	if (iio_buffer_data_available(buf) >= to_wait)
+		return true;
+
+	return false;
+}
+
 /**
  * iio_buffer_read_first_n_outer() - chrdev read for buffer access
  *
@@ -53,6 +70,8 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 {
 	struct iio_dev *indio_dev = filp->private_data;
 	struct iio_buffer *rb = indio_dev->buffer;
+	size_t datum_size = rb->bytes_per_datum;
+	size_t to_wait = 0;
 	int ret;
 
 	if (!indio_dev->info)
@@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 	if (!rb || !rb->access->read_first_n)
 		return -EINVAL;
 
+	/*
+	 * If datum_size is 0 there will never be anything to read from the
+	 * buffer, so signal end of file now.
+	 */
+	if (!datum_size)
+		return 0;
+
+	if (!(filp->f_flags & O_NONBLOCK))
+		to_wait = min_t(size_t, n / datum_size, rb->watermark);
+
 	do {
-		if (!iio_buffer_data_available(rb)) {
-			if (filp->f_flags & O_NONBLOCK)
-				return -EAGAIN;
+		ret = wait_event_interruptible(rb->pollq,
+				      iio_buffer_ready(indio_dev, rb, to_wait));
+		if (ret)
+			return ret;
 
-			ret = wait_event_interruptible(rb->pollq,
-					iio_buffer_data_available(rb) ||
-					indio_dev->info == NULL);
-			if (ret)
-				return ret;
-			if (indio_dev->info == NULL)
-				return -ENODEV;
-		}
+		if (!indio_dev->info)
+			return -ENODEV;
 
 		ret = rb->access->read_first_n(rb, n, buf);
 		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
@@ -96,9 +120,8 @@ unsigned int iio_buffer_poll(struct file *filp,
 		return -ENODEV;
 
 	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_data_available(rb))
+	if (iio_buffer_ready(indio_dev, rb, rb->watermark))
 		return POLLIN | POLLRDNORM;
-	/* need a way of knowing if there may be enough data... */
 	return 0;
 }
 
@@ -123,6 +146,7 @@ void iio_buffer_init(struct iio_buffer *buffer)
 	INIT_LIST_HEAD(&buffer->buffer_list);
 	init_waitqueue_head(&buffer->pollq);
 	kref_init(&buffer->ref);
+	buffer->watermark = 1;
 }
 EXPORT_SYMBOL(iio_buffer_init);
 
@@ -416,6 +440,11 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 		buffer->access->set_length(buffer, val);
 		ret = 0;
 	}
+	if (ret)
+		goto out;
+	if (buffer->length && buffer->length < buffer->watermark)
+		buffer->watermark = buffer->length;
+out:
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret ? ret : len;
@@ -472,6 +501,7 @@ static void iio_buffer_activate(struct iio_dev *indio_dev,
 static void iio_buffer_deactivate(struct iio_buffer *buffer)
 {
 	list_del_init(&buffer->buffer_list);
+	wake_up_interruptible(&buffer->pollq);
 	iio_buffer_put(buffer);
 }
 
@@ -754,16 +784,64 @@ done:
 
 static const char * const iio_scan_elements_group_name = "scan_elements";
 
+static ssize_t iio_buffer_show_watermark(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	return sprintf(buf, "%u\n", buffer->watermark);
+}
+
+static ssize_t iio_buffer_store_watermark(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret)
+		return ret;
+	if (!val)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+
+	if (val > buffer->length) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (iio_buffer_is_active(indio_dev->buffer)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	buffer->watermark = val;
+out:
+	mutex_unlock(&indio_dev->mlock);
+
+	return ret ? ret : len;
+}
+
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
 		   iio_buffer_write_length);
 static struct device_attribute dev_attr_length_ro = __ATTR(length,
 	S_IRUGO, iio_buffer_read_length, NULL);
 static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_enable, iio_buffer_store_enable);
+static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
+		   iio_buffer_show_watermark, iio_buffer_store_watermark);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
 	&dev_attr_enable.attr,
+	&dev_attr_watermark.attr,
 };
 
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
@@ -944,8 +1022,18 @@ static const void *iio_demux(struct iio_buffer *buffer,
 static int iio_push_to_buffer(struct iio_buffer *buffer, const void *data)
 {
 	const void *dataout = iio_demux(buffer, data);
+	int ret;
+
+	ret = buffer->access->store_to(buffer, dataout);
+	if (ret)
+		return ret;
 
-	return buffer->access->store_to(buffer, dataout);
+	/*
+	 * We can't just test for watermark to decide if we wake the poll queue
+	 * because read may request less samples than the watermark.
+	 */
+	wake_up_interruptible_poll(&buffer->pollq, POLLIN | POLLRDNORM);
+	return 0;
 }
 
 static void iio_buffer_demux_free(struct iio_buffer *buffer)
diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
index b2beea0..847ca56 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -83,9 +83,6 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
 	ret = kfifo_in(&kf->kf, data, 1);
 	if (ret != 1)
 		return -EBUSY;
-
-	wake_up_interruptible_poll(&r->pollq, POLLIN | POLLRDNORM);
-
 	return 0;
 }
 
@@ -109,16 +106,16 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
 	return copied;
 }
 
-static bool iio_kfifo_buf_data_available(struct iio_buffer *r)
+static size_t iio_kfifo_buf_data_available(struct iio_buffer *r)
 {
 	struct iio_kfifo *kf = iio_to_kfifo(r);
-	bool empty;
+	size_t samples;
 
 	mutex_lock(&kf->user_lock);
-	empty = kfifo_is_empty(&kf->kf);
+	samples = kfifo_len(&kf->kf);
 	mutex_unlock(&kf->user_lock);
 
-	return !empty;
+	return samples;
 }
 
 static void iio_kfifo_buffer_release(struct iio_buffer *buffer)
diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
index f76a268..8589ead 100644
--- a/drivers/staging/iio/accel/sca3000_ring.c
+++ b/drivers/staging/iio/accel/sca3000_ring.c
@@ -129,9 +129,9 @@ error_ret:
 	return ret ? ret : num_read;
 }
 
-static bool sca3000_ring_buf_data_available(struct iio_buffer *r)
+static size_t sca3000_ring_buf_data_available(struct iio_buffer *r)
 {
-	return r->stufftoread;
+	return r->stufftoread ? r->watermark : 0;
 }
 
 /**
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index b65850a..eb8622b 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -21,8 +21,8 @@ struct iio_buffer;
  * struct iio_buffer_access_funcs - access functions for buffers.
  * @store_to:		actually store stuff to the buffer
  * @read_first_n:	try to get a specified number of bytes (must exist)
- * @data_available:	indicates whether data for reading from the buffer is
- *			available.
+ * @data_available:	indicates how much data is available for reading from
+ *			the buffer.
  * @request_update:	if a parameter change has been marked, update underlying
  *			storage.
  * @set_bytes_per_datum:set number of bytes per datum
@@ -43,7 +43,7 @@ struct iio_buffer_access_funcs {
 	int (*read_first_n)(struct iio_buffer *buffer,
 			    size_t n,
 			    char __user *buf);
-	bool (*data_available)(struct iio_buffer *buffer);
+	size_t (*data_available)(struct iio_buffer *buffer);
 
 	int (*request_update)(struct iio_buffer *buffer);
 
@@ -72,6 +72,7 @@ struct iio_buffer_access_funcs {
  * @demux_bounce:	[INTERN] buffer for doing gather from incoming scan.
  * @buffer_list:	[INTERN] entry in the devices list of current buffers.
  * @ref:		[INTERN] reference count of the buffer.
+ * @watermark:		[INTERN] number of datums to wait for poll/read.
  */
 struct iio_buffer {
 	int					length;
@@ -90,6 +91,7 @@ struct iio_buffer {
 	void					*demux_bounce;
 	struct list_head			buffer_list;
 	struct kref				ref;
+	unsigned int				watermark;
 };
 
 /**
-- 
1.9.1

[PATCH v5 2/3] iio: add support for hardware fifo

[Octavian Purdila]

Some devices have hardware buffers that can store a number of samples
for later consumption. Hardware usually provides interrupts to notify
the processor when the fifo is full or when it has reached a certain
threshold. This helps with reducing the number of interrupts to the
host processor and thus it helps decreasing the power consumption.

This patch adds support for hardware fifo to IIO by adding drivers
operations for flushing the hadware fifo and setting and getting the
watermark level.

Since a driver may support hardware fifo only when not in triggered
buffer mode (due to different samantics of hardware fifo sampling and
triggered sampling) this patch changes the IIO core code to allow
falling back to non-triggered buffered mode if no trigger is enabled.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
 drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
 include/linux/iio/iio.h                 | 26 +++++++++++++
 3 files changed, 108 insertions(+), 12 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 1283ca7..143ddf2d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1264,3 +1264,28 @@ Description:
 		allows the application to block on poll with a timeout and read
 		the available samples after the timeout expires and thus have a
 		maximum delay guarantee.
+
+What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
+KernelVersion: 3.21
+Contact:       linux-iio@vger.kernel.org
+Description:
+	       Read-only entry that contains a single integer specifying the
+	       current level for the hardware fifo watermark level. If this
+	       value is negative it means that the device does not support a
+	       hardware fifo. If this value is 0 it means that the hardware fifo
+	       is currently disabled.
+	       If this value is strictly positive it signals that the hardware
+	       fifo of the device is active and that samples are stored in an
+	       internal hardware buffer. When the level of the hardware fifo
+	       reaches the watermark level the device will flush its internal
+	       buffer to the device buffer. Because of this a trigger is not
+	       needed to use the device in buffer mode.
+	       The hardware watermark level is set by the driver based on the
+	       value set by the user in buffer/watermark but taking into account
+	       the limitations of the hardware (e.g. most hardware buffers are
+	       limited to 32-64 samples).
+	       Because the sematics of triggers and hardware fifo may be
+	       different (e.g. the hadware fifo may record samples according to
+	       the sample rate while an any-motion trigger generates samples
+	       based on the set rate of change) setting a trigger may disable
+	       the hardware fifo.
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a4f4f07..b6f1bd4 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -42,18 +42,47 @@ static size_t iio_buffer_data_available(struct iio_buffer *buf)
 	return buf->access->data_available(buf);
 }
 
+static int iio_buffer_flush_hwfifo(struct iio_dev *indio_dev,
+				   struct iio_buffer *buf, size_t required)
+{
+	if (!indio_dev->info->hwfifo_flush)
+		return -ENODEV;
+
+	return indio_dev->info->hwfifo_flush(indio_dev, required);
+}
+
 static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
-			     size_t to_wait)
+			     size_t to_wait, bool try_flush)
 {
+	size_t avail;
+	int flushed = 0;
+
 	/* wakeup if the device was unregistered */
 	if (!indio_dev->info)
 		return true;
 
 	/* drain the buffer if it was disabled */
-	if (!iio_buffer_is_active(buf))
+	if (!iio_buffer_is_active(buf)) {
 		to_wait = min_t(size_t, to_wait, 1);
+		try_flush = false;
+	}
+
+	avail = iio_buffer_data_available(buf);
 
-	if (iio_buffer_data_available(buf) >= to_wait)
+	if (avail >= to_wait) {
+		/* force a flush for non-blocking reads */
+		if (!to_wait && !avail && try_flush)
+			iio_buffer_flush_hwfifo(indio_dev, buf, 1);
+		return true;
+	}
+
+	if (try_flush)
+		flushed = iio_buffer_flush_hwfifo(indio_dev, buf,
+						  to_wait - avail);
+	if (flushed <= 0)
+		return false;
+
+	if (avail + flushed >= to_wait)
 		return true;
 
 	return false;
@@ -92,7 +121,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 
 	do {
 		ret = wait_event_interruptible(rb->pollq,
-				      iio_buffer_ready(indio_dev, rb, to_wait));
+			       iio_buffer_ready(indio_dev, rb, to_wait, true));
 		if (ret)
 			return ret;
 
@@ -120,7 +149,7 @@ unsigned int iio_buffer_poll(struct file *filp,
 		return -ENODEV;
 
 	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_ready(indio_dev, rb, rb->watermark))
+	if (iio_buffer_ready(indio_dev, rb, rb->watermark, false))
 		return POLLIN | POLLRDNORM;
 	return 0;
 }
@@ -659,19 +688,16 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
 		}
 	}
 	/* Definitely possible for devices to support both of these. */
-	if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) {
-		if (!indio_dev->trig) {
-			printk(KERN_INFO "Buffer not started: no trigger\n");
-			ret = -EINVAL;
-			/* Can only occur on first buffer */
-			goto error_run_postdisable;
-		}
+	if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
 		indio_dev->currentmode = INDIO_BUFFER_TRIGGERED;
 	} else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
 		indio_dev->currentmode = INDIO_BUFFER_HARDWARE;
 	} else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
 		indio_dev->currentmode = INDIO_BUFFER_SOFTWARE;
 	} else { /* Should never be reached */
+		/* Can only occur on first buffer */
+		if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
+			pr_info("Buffer not started: no trigger\n");
 		ret = -EINVAL;
 		goto error_run_postdisable;
 	}
@@ -823,12 +849,28 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 	}
 
 	buffer->watermark = val;
+
+	if (indio_dev->info->hwfifo_set_watermark)
+		indio_dev->info->hwfifo_set_watermark(indio_dev, val);
 out:
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret ? ret : len;
 }
 
+static ssize_t iio_buffer_show_hwfifo_watermark(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	int ret = -1;
+
+	if (indio_dev->info->hwfifo_get_watermark)
+		ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
+
+	return sprintf(buf, "%d\n", ret < -1 ? -1 : ret);
+}
+
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
 		   iio_buffer_write_length);
 static struct device_attribute dev_attr_length_ro = __ATTR(length,
@@ -837,11 +879,14 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_enable, iio_buffer_store_enable);
 static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_watermark, iio_buffer_store_watermark);
+static DEVICE_ATTR(hwfifo_watermark, S_IRUGO, iio_buffer_show_hwfifo_watermark,
+		   NULL);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_watermark.attr,
+	&dev_attr_hwfifo_watermark.attr,
 };
 
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 80d8550..1b1cd7d 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -338,6 +338,29 @@ struct iio_dev;
  *			provide a custom of_xlate function that reads the
  *			*args* and returns the appropriate index in registered
  *			IIO channels array.
+ * @hwfifo_set_watermark: function pointer to set the current hardware fifo
+ *			watermark level. It receives the desired watermark as a
+ *			hint and the device driver may adjust it to take into
+ *			account hardware limitations. Setting the watermark to a
+ *			strictly positive value should enable the hardware fifo
+ *			if not already enabled. When the hardware fifo is
+ *			enabled and its level reaches the watermark level the
+ *			device must flush the samples stored in the hardware
+ *			fifo to the device buffer. Setting the watermark to 0
+ *			should disable the hardware fifo. The device driver must
+ *			disable the hardware fifo when a trigger with different
+ *			sampling semantic (then the hardware fifo) is set
+ *			(e.g. when setting an any-motion trigger to a device
+ *			that has its hardware fifo sample based on the set
+ *			sample frequency).
+ * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
+ *			watermark level
+ * @hwfifo_flush:	function pointer to flush the samples stored in the
+ *			hardware fifo to the device buffer. The driver should
+ *			not flush more then count samples. The function must
+ *			return the number of samples flushed, 0 if no samples
+ *			were flushed or a negative integer if no samples were
+ *			flushed and there was an error.
  **/
 struct iio_info {
 	struct module			*driver_module;
@@ -399,6 +422,9 @@ struct iio_info {
 				  unsigned *readval);
 	int (*of_xlate)(struct iio_dev *indio_dev,
 			const struct of_phandle_args *iiospec);
+	int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val);
+	int (*hwfifo_get_watermark)(struct iio_dev *indio_dev);
+	int (*hwfifo_flush)(struct iio_dev *indio_dev, unsigned count);
 };
 
 /**
-- 
1.9.1

[PATCH v5 3/3] iio: bmc150_accel: add support for hardware fifo

[Octavian Purdila]

We only advertise hardware fifo support if the I2C bus supports full
I2C or smbus I2C block data reads since it is mandatory to read the
full frame in one read (otherwise the rest of the frame is discarded).

The hardware fifo is enabled only when triggers are not active because:

(a) when using the any-motion trigger the user expects to see samples
based on ROC events, but the fifo stores samples based on the sample
frequency

(b) the data-ready trigger is waking the CPU for for every sample, so
using the hardware fifo does not have any benefit

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 363 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 349 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index d826394..984689a 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -70,7 +70,9 @@
 #define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE	BIT(2)
 
 #define BMC150_ACCEL_REG_INT_MAP_1		0x1A
-#define BMC150_ACCEL_INT_MAP_1_BIT_DATA	BIT(0)
+#define BMC150_ACCEL_INT_MAP_1_BIT_DATA		BIT(0)
+#define BMC150_ACCEL_INT_MAP_1_BIT_FWM		BIT(1)
+#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL	BIT(2)
 
 #define BMC150_ACCEL_REG_INT_RST_LATCH		0x21
 #define BMC150_ACCEL_INT_MODE_LATCH_RESET	0x80
@@ -83,7 +85,9 @@
 #define BMC150_ACCEL_INT_EN_BIT_SLP_Z		BIT(2)
 
 #define BMC150_ACCEL_REG_INT_EN_1		0x17
-#define BMC150_ACCEL_INT_EN_BIT_DATA_EN	BIT(4)
+#define BMC150_ACCEL_INT_EN_BIT_DATA_EN		BIT(4)
+#define BMC150_ACCEL_INT_EN_BIT_FFULL_EN	BIT(5)
+#define BMC150_ACCEL_INT_EN_BIT_FWM_EN		BIT(6)
 
 #define BMC150_ACCEL_REG_INT_OUT_CTRL		0x20
 #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL	BIT(0)
@@ -122,6 +126,12 @@
 #define BMC150_ACCEL_AXIS_TO_REG(axis)	(BMC150_ACCEL_REG_XOUT_L + (axis * 2))
 #define BMC150_AUTO_SUSPEND_DELAY_MS		2000
 
+#define BMC150_ACCEL_REG_FIFO_STATUS		0x0E
+#define BMC150_ACCEL_REG_FIFO_CONFIG0		0x30
+#define BMC150_ACCEL_REG_FIFO_CONFIG1		0x3E
+#define BMC150_ACCEL_REG_FIFO_DATA		0x3F
+#define BMC150_ACCEL_FIFO_LENGTH		32
+
 enum bmc150_accel_axis {
 	AXIS_X,
 	AXIS_Y,
@@ -179,13 +189,14 @@ struct bmc150_accel_data {
 	atomic_t active_intr;
 	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
 	struct mutex mutex;
+	u8 fifo_mode, watermark;
 	s16 buffer[8];
 	u8 bw_bits;
 	u32 slope_dur;
 	u32 slope_thres;
 	u32 range;
 	int ev_enable_state;
-	int64_t timestamp;
+	int64_t timestamp, old_timestamp;
 	const struct bmc150_accel_chip_info *chip_info;
 };
 
@@ -470,6 +481,12 @@ static const struct bmc150_accel_interrupt_info {
 			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
 			BMC150_ACCEL_INT_EN_BIT_SLP_Z
 	},
+	{ /* fifo watermark interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
+		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN,
+	},
 };
 
 static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
@@ -823,6 +840,183 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
+
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+
+	if (val > BMC150_ACCEL_FIFO_LENGTH)
+		val = BMC150_ACCEL_FIFO_LENGTH * 3 / 4;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static int bmc150_accel_get_watermark(struct iio_dev *indio_dev)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	if (!data->fifo_mode)
+		ret = 0;
+	else
+		ret = data->watermark;
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+/*
+ * We must read at least one full frame in one burst, otherwise the rest of the
+ * frame data is discarded.
+ */
+static int bmc150_accel_fifo_transfer(const struct i2c_client *client,
+				      char *buffer, int samples)
+{
+	int sample_length = 3 * 2;
+	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
+	int ret = -EIO;
+
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		struct i2c_msg msg[2] = {
+			{
+				.addr = client->addr,
+				.flags = 0,
+				.buf = &reg_fifo_data,
+				.len = sizeof(reg_fifo_data),
+			},
+			{
+				.addr = client->addr,
+				.flags = I2C_M_RD,
+				.buf = (u8 *)buffer,
+				.len = samples * sample_length,
+			}
+		};
+
+		ret = i2c_transfer(client->adapter, msg, 2);
+		if (ret != 2)
+			ret = -EIO;
+		else
+			ret = 0;
+	} else {
+		int i, step = I2C_SMBUS_BLOCK_MAX / sample_length;
+
+		for (i = 0; i < samples * sample_length; i += step) {
+			ret = i2c_smbus_read_i2c_block_data(client,
+							    reg_fifo_data, step,
+							    &buffer[i]);
+			if (ret != step) {
+				ret = -EIO;
+				break;
+			}
+
+			ret = 0;
+		}
+	}
+
+	if (ret)
+		dev_err(&client->dev, "Error transferring data from fifo\n");
+
+	return ret;
+}
+
+static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
+				     unsigned samples, bool irq)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret, i;
+	u8 count;
+	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
+	int64_t tstamp, sample_period;
+
+	ret = i2c_smbus_read_byte_data(data->client,
+				       BMC150_ACCEL_REG_FIFO_STATUS);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_fifo_status\n");
+		return ret;
+	}
+
+	count = ret & 0x7F;
+
+	if (!count)
+		return 0;
+
+	/*
+	 * If we getting called from IRQ handler we know the stored timestamp is
+	 * fairly accurate for the last stored sample. Otherwise, if we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is +/- one sample period.
+	 */
+	if (!irq) {
+		data->old_timestamp = data->timestamp;
+		data->timestamp = iio_get_time_ns();
+	}
+
+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 *
+	 * Note that we can't use the current bandwidth settings to compute the
+	 * sample period because the sample rate varies with the device
+	 * (e.g. between 31.70ms to 32.20ms for a bandwidth of 15.63HZ). That
+	 * small variation adds when we store a large number of samples and
+	 * creates significant jitter between the last and first samples in
+	 * different batches (e.g. 32ms vs 21ms).
+	 *
+	 * To avoid this issue we compute the actual sample period ourselves
+	 * based on the timestamp delta between the last two flush operations.
+	 */
+	sample_period = (data->timestamp - data->old_timestamp) / count;
+	tstamp = data->timestamp - (count - 1) * sample_period;
+
+	if (samples && count > samples)
+		count = samples;
+
+	ret = bmc150_accel_fifo_transfer(data->client, (u8 *)buffer, count);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally we want the IIO core to handle the demux when running in fifo
+	 * mode but not when running in triggerde buffer mode. Unfortunately
+	 * this does not seem to be possible, so stick with driver demux for
+	 * now.
+	 */
+	for (i = 0; i < count; i++) {
+		u16 sample[8];
+		int j, bit;
+
+		j = 0;
+		for_each_set_bit(bit, indio_dev->active_scan_mask,
+				 indio_dev->masklength)
+			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
+
+		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	return count;
+}
+
+static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, unsigned samples)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = __bmc150_accel_fifo_flush(indio_dev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
 		"15.620000 31.260000 62.50000 125 250 500 1000 2000");
 
@@ -950,7 +1144,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 	},
 };
 
-static const struct iio_info bmc150_accel_info = {
+static struct iio_info bmc150_accel_info = {
 	.attrs			= &bmc150_accel_attrs_group,
 	.read_raw		= bmc150_accel_read_raw,
 	.write_raw		= bmc150_accel_write_raw,
@@ -959,6 +1153,9 @@ static const struct iio_info bmc150_accel_info = {
 	.write_event_config	= bmc150_accel_write_event_config,
 	.read_event_config	= bmc150_accel_read_event_config,
 	.validate_trigger	= bmc150_accel_validate_trigger,
+	.hwfifo_set_watermark	= bmc150_accel_set_watermark,
+	.hwfifo_get_watermark	= bmc150_accel_get_watermark,
+	.hwfifo_flush		= bmc150_accel_fifo_flush,
 	.driver_module		= THIS_MODULE,
 };
 
@@ -1057,18 +1254,17 @@ static const struct iio_trigger_ops bmc150_accel_trigger_ops = {
 	.owner = THIS_MODULE,
 };
 
-static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
+static int bmc150_accel_handle_roc_event(struct iio_dev *indio_dev)
 {
-	struct iio_dev *indio_dev = private;
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
-	int ret;
 	int dir;
+	int ret;
 
 	ret = i2c_smbus_read_byte_data(data->client,
 				       BMC150_ACCEL_REG_INT_STATUS_2);
 	if (ret < 0) {
 		dev_err(&data->client->dev, "Error reading reg_int_status_2\n");
-		goto ack_intr_status;
+		return ret;
 	}
 
 	if (ret & BMC150_ACCEL_ANY_MOTION_BIT_SIGN)
@@ -1097,35 +1293,73 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
 							IIO_EV_TYPE_ROC,
 							dir),
 							data->timestamp);
-ack_intr_status:
-	if (!data->triggers[BMC150_ACCEL_TRIGGER_DATA_READY].enabled)
+	return ret;
+}
+
+static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	bool ack = false;
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (data->fifo_mode) {
+		ret = __bmc150_accel_fifo_flush(indio_dev,
+						BMC150_ACCEL_FIFO_LENGTH, true);
+		if (ret > 0)
+			ack = true;
+	}
+
+	if (data->ev_enable_state) {
+		ret = bmc150_accel_handle_roc_event(indio_dev);
+		if (ret > 0)
+			ack = true;
+	}
+
+	if (ack) {
 		ret = i2c_smbus_write_byte_data(data->client,
 					BMC150_ACCEL_REG_INT_RST_LATCH,
 					BMC150_ACCEL_INT_MODE_LATCH_INT |
 					BMC150_ACCEL_INT_MODE_LATCH_RESET);
+		if (ret)
+			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
+		ret = IRQ_HANDLED;
+	} else {
+		ret = IRQ_NONE;
+	}
 
-	return IRQ_HANDLED;
+	mutex_unlock(&data->mutex);
+
+	return ret;
 }
 
 static irqreturn_t bmc150_accel_data_rdy_trig_poll(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	bool ack = false;
 	int i;
 
+	data->old_timestamp = data->timestamp;
 	data->timestamp = iio_get_time_ns();
 
 	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
 		if (data->triggers[i].enabled) {
 			iio_trigger_poll(data->triggers[i].indio_trig);
+			ack = true;
 			break;
 		}
 	}
 
-	if (data->ev_enable_state)
+	if (data->ev_enable_state || data->fifo_mode)
 		return IRQ_WAKE_THREAD;
-	else
+
+	if (ack)
 		return IRQ_HANDLED;
+
+	return IRQ_NONE;
 }
 
 static const char *bmc150_accel_match_acpi_device(struct device *dev, int *data)
@@ -1232,6 +1466,94 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
 	return ret;
 }
 
+#define BMC150_ACCEL_FIFO_MODE_STREAM          0x80
+#define BMC150_ACCEL_FIFO_MODE_FIFO            0x40
+#define BMC150_ACCEL_FIFO_MODE_BYPASS          0x00
+
+static int bmc150_accel_fifo_set_mode(struct bmc150_accel_data *data)
+{
+	u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG1;
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, reg, data->fifo_mode);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_fifo_config1\n");
+		return ret;
+	}
+
+	if (!data->fifo_mode)
+		return 0;
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					BMC150_ACCEL_REG_FIFO_CONFIG0,
+					data->watermark);
+	if (ret < 0)
+		dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
+
+	return ret;
+}
+
+static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+		return iio_triggered_buffer_postenable(indio_dev);
+
+	mutex_lock(&data->mutex);
+
+	if (!data->watermark)
+		goto out;
+
+	ret = bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK,
+					 true);
+	if (ret)
+		goto out;
+
+	data->fifo_mode = BMC150_ACCEL_FIFO_MODE_FIFO;
+
+	ret = bmc150_accel_fifo_set_mode(data);
+	if (ret) {
+		data->fifo_mode = 0;
+		bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK,
+					   false);
+	}
+
+out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+
+	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+		return iio_triggered_buffer_predisable(indio_dev);
+
+	mutex_lock(&data->mutex);
+
+	if (!data->fifo_mode)
+		goto out;
+
+	bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK, false);
+	__bmc150_accel_fifo_flush(indio_dev, BMC150_ACCEL_FIFO_LENGTH, false);
+	data->fifo_mode = 0;
+	bmc150_accel_fifo_set_mode(data);
+
+out:
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops bmc150_accel_buffer_ops = {
+	.postenable = bmc150_accel_buffer_postenable,
+	.predisable = bmc150_accel_buffer_predisable,
+};
+
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1270,6 +1592,15 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	indio_dev->num_channels = data->chip_info->num_channels;
 	indio_dev->name = name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
+	    i2c_check_functionality(client->adapter,
+				    I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+		indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
+	} else {
+		bmc150_accel_info.hwfifo_set_watermark = NULL;
+		bmc150_accel_info.hwfifo_get_watermark = NULL;
+		bmc150_accel_info.hwfifo_flush = NULL;
+	}
 	indio_dev->info = &bmc150_accel_info;
 
 	if (client->irq < 0)
@@ -1309,7 +1640,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		ret = iio_triggered_buffer_setup(indio_dev,
 						 &iio_pollfunc_store_time,
 						 bmc150_accel_trigger_handler,
-						 NULL);
+						 &bmc150_accel_buffer_ops);
 		if (ret < 0) {
 			dev_err(&client->dev,
 				"Failed: iio triggered buffer setup\n");
@@ -1386,6 +1717,7 @@ static int bmc150_accel_resume(struct device *dev)
 	mutex_lock(&data->mutex);
 	if (atomic_read(&data->active_intr))
 		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
+	bmc150_accel_fifo_set_mode(data);
 	mutex_unlock(&data->mutex);
 
 	return 0;
@@ -1419,6 +1751,9 @@ static int bmc150_accel_runtime_resume(struct device *dev)
 	ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
 	if (ret < 0)
 		return ret;
+	ret = bmc150_accel_fifo_set_mode(data);
+	if (ret < 0)
+		return ret;
 
 	sleep_val = bmc150_accel_get_startup_times(data);
 	if (sleep_val < 20)
-- 
1.9.1

Re: [PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Jonathan Cameron]

On 04/03/15 17:56, Octavian Purdila wrote:
> From: Josselin Costanzi <josselin.costanzi@mobile-devices.fr>
> 
> Currently the IIO buffer blocking read only wait until at least one
> data element is available.
> This patch makes the reader sleep until enough data is collected before
> returning to userspace. This should limit the read() calls count when
> trying to get data in batches.
> 
> Co-author: Yannick Bedhomme <yannick.bedhomme@mobile-devices.fr>
> Signed-off-by: Josselin Costanzi <josselin.costanzi@mobile-devices.fr>
> [rebased and remove buffer timeout]
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>

This looks good to me.  I'd like a reviewed-by from Lars though if possible
as he's thought more about this than I have ;)

On that note, with previous reviews from Lars existing, I'd definitely have
added him to the CC list. I've added all the other IIO reviewers listed in
MAINTAINERS as well.  This is an important addition to the subsystem so 
whilst they all avidly read linux-iio (who doesn't?) I'd like to bring
it to their special attention!


Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio  |  15 ++++
>  drivers/iio/industrialio-buffer.c        | 118 +++++++++++++++++++++++++++----
>  drivers/iio/kfifo_buf.c                  |  11 ++-
>  drivers/staging/iio/accel/sca3000_ring.c |   4 +-
>  include/linux/iio/buffer.h               |   8 ++-
>  5 files changed, 129 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 9a70c31..1283ca7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1249,3 +1249,18 @@ Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Specifies number of seconds in which we compute the steps
>  		that occur in order to decide if the consumer is making steps.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/watermark
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		A single positive integer specifying the maximum number of scan
> +		elements to wait for.
> +		Poll will block until the watermark is reached.
> +		Blocking read will wait until the minimum between the requested
> +		read amount or the low water mark is available.
> +		Non-blocking read will retrieve the available samples from the
> +		buffer even if there are less samples then watermark level. This
> +		allows the application to block on poll with a timeout and read
> +		the available samples after the timeout expires and thus have a
> +		maximum delay guarantee.
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index c2d5440..a4f4f07 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -37,11 +37,28 @@ static bool iio_buffer_is_active(struct iio_buffer *buf)
>  	return !list_empty(&buf->buffer_list);
>  }
>  
> -static bool iio_buffer_data_available(struct iio_buffer *buf)
> +static size_t iio_buffer_data_available(struct iio_buffer *buf)
>  {
>  	return buf->access->data_available(buf);
>  }
>  
> +static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
> +			     size_t to_wait)
> +{
> +	/* wakeup if the device was unregistered */
> +	if (!indio_dev->info)
> +		return true;
> +
> +	/* drain the buffer if it was disabled */
> +	if (!iio_buffer_is_active(buf))
> +		to_wait = min_t(size_t, to_wait, 1);
> +
> +	if (iio_buffer_data_available(buf) >= to_wait)
> +		return true;
> +
> +	return false;
> +}
> +
>  /**
>   * iio_buffer_read_first_n_outer() - chrdev read for buffer access
>   *
> @@ -53,6 +70,8 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  {
>  	struct iio_dev *indio_dev = filp->private_data;
>  	struct iio_buffer *rb = indio_dev->buffer;
> +	size_t datum_size = rb->bytes_per_datum;
> +	size_t to_wait = 0;
>  	int ret;
>  
>  	if (!indio_dev->info)
> @@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  	if (!rb || !rb->access->read_first_n)
>  		return -EINVAL;
>  
> +	/*
> +	 * If datum_size is 0 there will never be anything to read from the
> +	 * buffer, so signal end of file now.
> +	 */
> +	if (!datum_size)
> +		return 0;
> +
> +	if (!(filp->f_flags & O_NONBLOCK))
> +		to_wait = min_t(size_t, n / datum_size, rb->watermark);
> +
>  	do {
> -		if (!iio_buffer_data_available(rb)) {
> -			if (filp->f_flags & O_NONBLOCK)
> -				return -EAGAIN;
> +		ret = wait_event_interruptible(rb->pollq,
> +				      iio_buffer_ready(indio_dev, rb, to_wait));
> +		if (ret)
> +			return ret;
>  
> -			ret = wait_event_interruptible(rb->pollq,
> -					iio_buffer_data_available(rb) ||
> -					indio_dev->info == NULL);
> -			if (ret)
> -				return ret;
> -			if (indio_dev->info == NULL)
> -				return -ENODEV;
> -		}
> +		if (!indio_dev->info)
> +			return -ENODEV;
>  
>  		ret = rb->access->read_first_n(rb, n, buf);
>  		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> @@ -96,9 +120,8 @@ unsigned int iio_buffer_poll(struct file *filp,
>  		return -ENODEV;
>  
>  	poll_wait(filp, &rb->pollq, wait);
> -	if (iio_buffer_data_available(rb))
> +	if (iio_buffer_ready(indio_dev, rb, rb->watermark))
>  		return POLLIN | POLLRDNORM;
> -	/* need a way of knowing if there may be enough data... */
>  	return 0;
>  }
>  
> @@ -123,6 +146,7 @@ void iio_buffer_init(struct iio_buffer *buffer)
>  	INIT_LIST_HEAD(&buffer->buffer_list);
>  	init_waitqueue_head(&buffer->pollq);
>  	kref_init(&buffer->ref);
> +	buffer->watermark = 1;
>  }
>  EXPORT_SYMBOL(iio_buffer_init);
>  
> @@ -416,6 +440,11 @@ static ssize_t iio_buffer_write_length(struct device *dev,
>  		buffer->access->set_length(buffer, val);
>  		ret = 0;
>  	}
> +	if (ret)
> +		goto out;
> +	if (buffer->length && buffer->length < buffer->watermark)
> +		buffer->watermark = buffer->length;
> +out:
>  	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret ? ret : len;
> @@ -472,6 +501,7 @@ static void iio_buffer_activate(struct iio_dev *indio_dev,
>  static void iio_buffer_deactivate(struct iio_buffer *buffer)
>  {
>  	list_del_init(&buffer->buffer_list);
> +	wake_up_interruptible(&buffer->pollq);
>  	iio_buffer_put(buffer);
>  }
>  
> @@ -754,16 +784,64 @@ done:
>  
>  static const char * const iio_scan_elements_group_name = "scan_elements";
>  
> +static ssize_t iio_buffer_show_watermark(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +
> +	return sprintf(buf, "%u\n", buffer->watermark);
> +}
> +
> +static ssize_t iio_buffer_store_watermark(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf,
> +					  size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +	if (!val)
> +		return -EINVAL;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	if (val > buffer->length) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (iio_buffer_is_active(indio_dev->buffer)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	buffer->watermark = val;
> +out:
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret ? ret : len;
> +}
> +
>  static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
>  		   iio_buffer_write_length);
>  static struct device_attribute dev_attr_length_ro = __ATTR(length,
>  	S_IRUGO, iio_buffer_read_length, NULL);
>  static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_enable, iio_buffer_store_enable);
> +static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
> +		   iio_buffer_show_watermark, iio_buffer_store_watermark);
>  
>  static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_length.attr,
>  	&dev_attr_enable.attr,
> +	&dev_attr_watermark.attr,
>  };
>  
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> @@ -944,8 +1022,18 @@ static const void *iio_demux(struct iio_buffer *buffer,
>  static int iio_push_to_buffer(struct iio_buffer *buffer, const void *data)
>  {
>  	const void *dataout = iio_demux(buffer, data);
> +	int ret;
> +
> +	ret = buffer->access->store_to(buffer, dataout);
> +	if (ret)
> +		return ret;
>  
> -	return buffer->access->store_to(buffer, dataout);
> +	/*
> +	 * We can't just test for watermark to decide if we wake the poll queue
> +	 * because read may request less samples than the watermark.
> +	 */
> +	wake_up_interruptible_poll(&buffer->pollq, POLLIN | POLLRDNORM);
> +	return 0;
>  }
>  
>  static void iio_buffer_demux_free(struct iio_buffer *buffer)
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
> index b2beea0..847ca56 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -83,9 +83,6 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>  	ret = kfifo_in(&kf->kf, data, 1);
>  	if (ret != 1)
>  		return -EBUSY;
> -
> -	wake_up_interruptible_poll(&r->pollq, POLLIN | POLLRDNORM);
> -
>  	return 0;
>  }
>  
> @@ -109,16 +106,16 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>  	return copied;
>  }
>  
> -static bool iio_kfifo_buf_data_available(struct iio_buffer *r)
> +static size_t iio_kfifo_buf_data_available(struct iio_buffer *r)
>  {
>  	struct iio_kfifo *kf = iio_to_kfifo(r);
> -	bool empty;
> +	size_t samples;
>  
>  	mutex_lock(&kf->user_lock);
> -	empty = kfifo_is_empty(&kf->kf);
> +	samples = kfifo_len(&kf->kf);
>  	mutex_unlock(&kf->user_lock);
>  
> -	return !empty;
> +	return samples;
>  }
>  
>  static void iio_kfifo_buffer_release(struct iio_buffer *buffer)
> diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
> index f76a268..8589ead 100644
> --- a/drivers/staging/iio/accel/sca3000_ring.c
> +++ b/drivers/staging/iio/accel/sca3000_ring.c
> @@ -129,9 +129,9 @@ error_ret:
>  	return ret ? ret : num_read;
>  }
>  
> -static bool sca3000_ring_buf_data_available(struct iio_buffer *r)
> +static size_t sca3000_ring_buf_data_available(struct iio_buffer *r)
>  {
> -	return r->stufftoread;
> +	return r->stufftoread ? r->watermark : 0;
>  }
>  
>  /**
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index b65850a..eb8622b 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -21,8 +21,8 @@ struct iio_buffer;
>   * struct iio_buffer_access_funcs - access functions for buffers.
>   * @store_to:		actually store stuff to the buffer
>   * @read_first_n:	try to get a specified number of bytes (must exist)
> - * @data_available:	indicates whether data for reading from the buffer is
> - *			available.
> + * @data_available:	indicates how much data is available for reading from
> + *			the buffer.
>   * @request_update:	if a parameter change has been marked, update underlying
>   *			storage.
>   * @set_bytes_per_datum:set number of bytes per datum
> @@ -43,7 +43,7 @@ struct iio_buffer_access_funcs {
>  	int (*read_first_n)(struct iio_buffer *buffer,
>  			    size_t n,
>  			    char __user *buf);
> -	bool (*data_available)(struct iio_buffer *buffer);
> +	size_t (*data_available)(struct iio_buffer *buffer);
>  
>  	int (*request_update)(struct iio_buffer *buffer);
>  
> @@ -72,6 +72,7 @@ struct iio_buffer_access_funcs {
>   * @demux_bounce:	[INTERN] buffer for doing gather from incoming scan.
>   * @buffer_list:	[INTERN] entry in the devices list of current buffers.
>   * @ref:		[INTERN] reference count of the buffer.
> + * @watermark:		[INTERN] number of datums to wait for poll/read.
>   */
>  struct iio_buffer {
>  	int					length;
> @@ -90,6 +91,7 @@ struct iio_buffer {
>  	void					*demux_bounce;
>  	struct list_head			buffer_list;
>  	struct kref				ref;
> +	unsigned int				watermark;
>  };
>  
>  /**
> 

Re: [PATCH v5 2/3] iio: add support for hardware fifo

[Jonathan Cameron]

On 04/03/15 17:56, Octavian Purdila wrote:
> Some devices have hardware buffers that can store a number of samples
> for later consumption. Hardware usually provides interrupts to notify
> the processor when the fifo is full or when it has reached a certain
> threshold. This helps with reducing the number of interrupts to the
> host processor and thus it helps decreasing the power consumption.
> 
> This patch adds support for hardware fifo to IIO by adding drivers
> operations for flushing the hadware fifo and setting and getting the
> watermark level.
> 
> Since a driver may support hardware fifo only when not in triggered
> buffer mode (due to different samantics of hardware fifo sampling and
> triggered sampling) this patch changes the IIO core code to allow
> falling back to non-triggered buffered mode if no trigger is enabled.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Bits and bobs inline

Typo's and a comment on weird hardware possibilities..

Also I'm not keen on some of the error handling.

Sorry it too me so very long to look at this.  I keep thinking
it'll be fiddly and hence doing the easier stuff first only
to come to this and see that actually its very straight forward!.

Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
>  drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
>  include/linux/iio/iio.h                 | 26 +++++++++++++
>  3 files changed, 108 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 1283ca7..143ddf2d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1264,3 +1264,28 @@ Description:
>  		allows the application to block on poll with a timeout and read
>  		the available samples after the timeout expires and thus have a
>  		maximum delay guarantee.
> +
> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
> +KernelVersion: 3.21
> +Contact:       linux-iio@vger.kernel.org
> +Description:
> +	       Read-only entry that contains a single integer specifying the
> +	       current level for the hardware fifo watermark level. If this
> +	       value is negative it means that the device does not support a
> +	       hardware fifo. If this value is 0 it means that the hardware fifo
> +	       is currently disabled.
I wonder if we should indicate lack of hardware support by just not exporting
this attribute in the first place?
> +	       If this value is strictly positive it signals that the hardware
> +	       fifo of the device is active and that samples are stored in an
> +	       internal hardware buffer. When the level of the hardware fifo
> +	       reaches the watermark level the device will flush its internal
> +	       buffer to the device buffer. Because of this a trigger is not
> +	       needed to use the device in buffer mode.
> +	       The hardware watermark level is set by the driver based on the
> +	       value set by the user in buffer/watermark but taking into account
> +	       the limitations of the hardware (e.g. most hardware buffers are
> +	       limited to 32-64 samples).
> +	       Because the sematics of triggers and hardware fifo may be
> +	       different (e.g. the hadware fifo may record samples according to
hardware fifo
> +	       the sample rate while an any-motion trigger generates samples
> +	       based on the set rate of change) setting a trigger may disable
> +	       the hardware fifo.
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a4f4f07..b6f1bd4 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -42,18 +42,47 @@ static size_t iio_buffer_data_available(struct iio_buffer *buf)
>  	return buf->access->data_available(buf);
>  }
>  
> +static int iio_buffer_flush_hwfifo(struct iio_dev *indio_dev,
> +				   struct iio_buffer *buf, size_t required)
> +{
> +	if (!indio_dev->info->hwfifo_flush)
> +		return -ENODEV;
> +
> +	return indio_dev->info->hwfifo_flush(indio_dev, required);
> +}
> +
>  static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
> -			     size_t to_wait)
> +			     size_t to_wait, bool try_flush)
>  {
> +	size_t avail;
> +	int flushed = 0;
> +
>  	/* wakeup if the device was unregistered */
>  	if (!indio_dev->info)
>  		return true;
>  
>  	/* drain the buffer if it was disabled */
> -	if (!iio_buffer_is_active(buf))
> +	if (!iio_buffer_is_active(buf)) {
>  		to_wait = min_t(size_t, to_wait, 1);
> +		try_flush = false;
> +	}
> +
> +	avail = iio_buffer_data_available(buf);
>  
> -	if (iio_buffer_data_available(buf) >= to_wait)
> +	if (avail >= to_wait) {
I'm not really following what this is for.
If to_wait = 0 and avail = 0 and we have requested a flush
then try to read 1 element?

I think this only occurs on a non blocking read when nothing is
available?  What I don't understand is why we'd only try to read 1
sample.  Surely asking for as many as requested is more intuitive?

> +		/* force a flush for non-blocking reads */
> +		if (!to_wait && !avail && try_flush)
> +			iio_buffer_flush_hwfifo(indio_dev, buf, 1);
> +		return true;
> +	}
> +
> +	if (try_flush)
> +		flushed = iio_buffer_flush_hwfifo(indio_dev, buf,
> +						  to_wait - avail);
> +	if (flushed <= 0)
> +		return false;
> +
> +	if (avail + flushed >= to_wait)
>  		return true;
>  
>  	return false;
> @@ -92,7 +121,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  
>  	do {
>  		ret = wait_event_interruptible(rb->pollq,
> -				      iio_buffer_ready(indio_dev, rb, to_wait));
> +			       iio_buffer_ready(indio_dev, rb, to_wait, true));
>  		if (ret)
>  			return ret;
>  
> @@ -120,7 +149,7 @@ unsigned int iio_buffer_poll(struct file *filp,
>  		return -ENODEV;
>  
>  	poll_wait(filp, &rb->pollq, wait);
> -	if (iio_buffer_ready(indio_dev, rb, rb->watermark))
> +	if (iio_buffer_ready(indio_dev, rb, rb->watermark, false))
>  		return POLLIN | POLLRDNORM;
>  	return 0;
>  }
> @@ -659,19 +688,16 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
>  		}
>  	}
>  	/* Definitely possible for devices to support both of these. */
> -	if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) {
> -		if (!indio_dev->trig) {
> -			printk(KERN_INFO "Buffer not started: no trigger\n");
> -			ret = -EINVAL;
> -			/* Can only occur on first buffer */
> -			goto error_run_postdisable;
> -		}
> +	if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
>  		indio_dev->currentmode = INDIO_BUFFER_TRIGGERED;
>  	} else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
>  		indio_dev->currentmode = INDIO_BUFFER_HARDWARE;
>  	} else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
>  		indio_dev->currentmode = INDIO_BUFFER_SOFTWARE;
>  	} else { /* Should never be reached */
> +		/* Can only occur on first buffer */
> +		if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
> +			pr_info("Buffer not started: no trigger\n");
>  		ret = -EINVAL;
>  		goto error_run_postdisable;
>  	}
> @@ -823,12 +849,28 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>  	}
>  
>  	buffer->watermark = val;
> +
> +	if (indio_dev->info->hwfifo_set_watermark)
> +		indio_dev->info->hwfifo_set_watermark(indio_dev, val);
>  out:
>  	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret ? ret : len;
>  }
>  
> +static ssize_t iio_buffer_show_hwfifo_watermark(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	int ret = -1;
> +
> +	if (indio_dev->info->hwfifo_get_watermark)
> +		ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
Any error code returned should be returned by this function to userspace.

Also, ret = -1 would be EPERM, a possible error code.  Hence you need
to separate your handling of not supported from possible return values.

if (indio_dev->info->hwfifo_get_watermark) {
	ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
	if (ret < 0)
	   return ret;
	return sprintf(buf, "%d\n", ret);
} else {
        return sprintf(buf, "-1\n");

seems the simplest way to me.
> +
> +	return sprintf(buf, "%d\n", ret < -1 ? -1 : ret);
> +}
> +
>  static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
>  		   iio_buffer_write_length);
>  static struct device_attribute dev_attr_length_ro = __ATTR(length,
> @@ -837,11 +879,14 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_enable, iio_buffer_store_enable);
>  static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_watermark, iio_buffer_store_watermark);
> +static DEVICE_ATTR(hwfifo_watermark, S_IRUGO, iio_buffer_show_hwfifo_watermark,
> +		   NULL);
>  
>  static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_length.attr,
>  	&dev_attr_enable.attr,
>  	&dev_attr_watermark.attr,
> +	&dev_attr_hwfifo_watermark.attr,
>  };
>  
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 80d8550..1b1cd7d 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -338,6 +338,29 @@ struct iio_dev;
>   *			provide a custom of_xlate function that reads the
>   *			*args* and returns the appropriate index in registered
>   *			IIO channels array.
> + * @hwfifo_set_watermark: function pointer to set the current hardware fifo
> + *			watermark level. It receives the desired watermark as a
> + *			hint and the device driver may adjust it to take into
> + *			account hardware limitations. Setting the watermark to a
> + *			strictly positive value should enable the hardware fifo
> + *			if not already enabled. When the hardware fifo is
> + *			enabled and its level reaches the watermark level the
> + *			device must flush the samples stored in the hardware
> + *			fifo to the device buffer.
Hmm. This one is interesting.  I wonder if we have devices with minimum
watermark levels.  I know that devices with fixed levels exist (sca3000 IIRC)
but in that case if we are below the level we can fallback to the data ready
signal (watermark of 1 effectively).  If the device provides no notification
that data capture is occuring until we reach a particular level (say 50% full)
then its not entirely clear whether we should turn the buffer on when a
watermark below that is set.
> Setting the watermark to 0
> + *			should disable the hardware fifo. The device driver must
> + *			disable the hardware fifo when a trigger with different
> + *			sampling semantic (then the hardware fifo) is set
(than the...
> + *			(e.g. when setting an any-motion trigger to a device
> + *			that has its hardware fifo sample based on the set
> + *			sample frequency).
> + * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
> + *			watermark level
> + * @hwfifo_flush:	function pointer to flush the samples stored in the
> + *			hardware fifo to the device buffer. The driver should
> + *			not flush more then count samples. The function must
more than count
> + *			return the number of samples flushed, 0 if no samples
> + *			were flushed or a negative integer if no samples were
> + *			flushed and there was an error.
>   **/
>  struct iio_info {
>  	struct module			*driver_module;
> @@ -399,6 +422,9 @@ struct iio_info {
>  				  unsigned *readval);
>  	int (*of_xlate)(struct iio_dev *indio_dev,
>  			const struct of_phandle_args *iiospec);
> +	int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val);
> +	int (*hwfifo_get_watermark)(struct iio_dev *indio_dev);
> +	int (*hwfifo_flush)(struct iio_dev *indio_dev, unsigned count);
>  };
>  
>  /**
> 

Re: [PATCH v5 3/3] iio: bmc150_accel: add support for hardware fifo

[Jonathan Cameron]

On 04/03/15 17:56, Octavian Purdila wrote:
> We only advertise hardware fifo support if the I2C bus supports full
> I2C or smbus I2C block data reads since it is mandatory to read the
> full frame in one read (otherwise the rest of the frame is discarded).
> 
> The hardware fifo is enabled only when triggers are not active because:
> 
> (a) when using the any-motion trigger the user expects to see samples
> based on ROC events, but the fifo stores samples based on the sample
> frequency
> 
> (b) the data-ready trigger is waking the CPU for for every sample, so
> using the hardware fifo does not have any benefit
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Mostly looks very nice.

A few bits and pieces inline.  You definitely want to avoid
the non constant global structure you introduced however.
Just have two constant ones and choose between then.

Jonathan
> ---
>  drivers/iio/accel/bmc150-accel.c | 363 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 349 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index d826394..984689a 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -70,7 +70,9 @@
>  #define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE	BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_MAP_1		0x1A
> -#define BMC150_ACCEL_INT_MAP_1_BIT_DATA	BIT(0)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_DATA		BIT(0)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_FWM		BIT(1)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL	BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_RST_LATCH		0x21
>  #define BMC150_ACCEL_INT_MODE_LATCH_RESET	0x80
> @@ -83,7 +85,9 @@
>  #define BMC150_ACCEL_INT_EN_BIT_SLP_Z		BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_EN_1		0x17
> -#define BMC150_ACCEL_INT_EN_BIT_DATA_EN	BIT(4)
> +#define BMC150_ACCEL_INT_EN_BIT_DATA_EN		BIT(4)
> +#define BMC150_ACCEL_INT_EN_BIT_FFULL_EN	BIT(5)
> +#define BMC150_ACCEL_INT_EN_BIT_FWM_EN		BIT(6)
>  
>  #define BMC150_ACCEL_REG_INT_OUT_CTRL		0x20
>  #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL	BIT(0)
> @@ -122,6 +126,12 @@
>  #define BMC150_ACCEL_AXIS_TO_REG(axis)	(BMC150_ACCEL_REG_XOUT_L + (axis * 2))
>  #define BMC150_AUTO_SUSPEND_DELAY_MS		2000
>  
> +#define BMC150_ACCEL_REG_FIFO_STATUS		0x0E
> +#define BMC150_ACCEL_REG_FIFO_CONFIG0		0x30
> +#define BMC150_ACCEL_REG_FIFO_CONFIG1		0x3E
> +#define BMC150_ACCEL_REG_FIFO_DATA		0x3F
> +#define BMC150_ACCEL_FIFO_LENGTH		32
> +
>  enum bmc150_accel_axis {
>  	AXIS_X,
>  	AXIS_Y,
> @@ -179,13 +189,14 @@ struct bmc150_accel_data {
>  	atomic_t active_intr;
>  	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
>  	struct mutex mutex;
> +	u8 fifo_mode, watermark;
>  	s16 buffer[8];
>  	u8 bw_bits;
>  	u32 slope_dur;
>  	u32 slope_thres;
>  	u32 range;
>  	int ev_enable_state;
> -	int64_t timestamp;
> +	int64_t timestamp, old_timestamp;
>  	const struct bmc150_accel_chip_info *chip_info;
>  };
>  
> @@ -470,6 +481,12 @@ static const struct bmc150_accel_interrupt_info {
>  			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
>  			BMC150_ACCEL_INT_EN_BIT_SLP_Z
>  	},
> +	{ /* fifo watermark interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN,
> +	},
>  };
>  
>  static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
> @@ -823,6 +840,183 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
> +
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +
> +	if (val > BMC150_ACCEL_FIFO_LENGTH)
> +		val = BMC150_ACCEL_FIFO_LENGTH * 3 / 4;
> +
> +	mutex_lock(&data->mutex);
> +	data->watermark = val;
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +
> +static int bmc150_accel_get_watermark(struct iio_dev *indio_dev)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	if (!data->fifo_mode)
> +		ret = 0;
> +	else
> +		ret = data->watermark;
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +/*
> + * We must read at least one full frame in one burst, otherwise the rest of the
> + * frame data is discarded.
> + */
> +static int bmc150_accel_fifo_transfer(const struct i2c_client *client,
> +				      char *buffer, int samples)
> +{
> +	int sample_length = 3 * 2;
> +	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
> +	int ret = -EIO;
> +
> +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		struct i2c_msg msg[2] = {
> +			{
> +				.addr = client->addr,
> +				.flags = 0,
> +				.buf = &reg_fifo_data,
> +				.len = sizeof(reg_fifo_data),
> +			},
> +			{
> +				.addr = client->addr,
> +				.flags = I2C_M_RD,
> +				.buf = (u8 *)buffer,
> +				.len = samples * sample_length,
> +			}
> +		};
> +
> +		ret = i2c_transfer(client->adapter, msg, 2);
> +		if (ret != 2)
> +			ret = -EIO;
> +		else
> +			ret = 0;
> +	} else {
> +		int i, step = I2C_SMBUS_BLOCK_MAX / sample_length;
> +
> +		for (i = 0; i < samples * sample_length; i += step) {
> +			ret = i2c_smbus_read_i2c_block_data(client,
> +							    reg_fifo_data, step,
> +							    &buffer[i]);
> +			if (ret != step) {
> +				ret = -EIO;
> +				break;
> +			}
> +
> +			ret = 0;
> +		}
> +	}
> +
> +	if (ret)
> +		dev_err(&client->dev, "Error transferring data from fifo\n");
> +
> +	return ret;
> +}
> +
> +static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
> +				     unsigned samples, bool irq)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret, i;
> +	u8 count;
> +	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
> +	int64_t tstamp, sample_period;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       BMC150_ACCEL_REG_FIFO_STATUS);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_fifo_status\n");
> +		return ret;
> +	}
> +
> +	count = ret & 0x7F;
> +
> +	if (!count)
> +		return 0;
> +
> +	/*
> +	 * If we getting called from IRQ handler we know the stored timestamp is
> +	 * fairly accurate for the last stored sample. Otherwise, if we are
> +	 * called as a result of a read operation from userspace and hence
> +	 * before the watermark interrupt was triggered, take a timestamp
> +	 * now. We can fall anywhere in between two samples so the error in this
> +	 * case is +/- one sample period.
> +	 */
> +	if (!irq) {
> +		data->old_timestamp = data->timestamp;
> +		data->timestamp = iio_get_time_ns();
> +	}
> +
> +	/*
> +	 * Approximate timestamps for each of the sample based on the sampling
> +	 * frequency, timestamp for last sample and number of samples.
> +	 *
> +	 * Note that we can't use the current bandwidth settings to compute the
> +	 * sample period because the sample rate varies with the device
> +	 * (e.g. between 31.70ms to 32.20ms for a bandwidth of 15.63HZ). That
> +	 * small variation adds when we store a large number of samples and
> +	 * creates significant jitter between the last and first samples in
> +	 * different batches (e.g. 32ms vs 21ms).
> +	 *
> +	 * To avoid this issue we compute the actual sample period ourselves
> +	 * based on the timestamp delta between the last two flush operations.
> +	 */
> +	sample_period = (data->timestamp - data->old_timestamp) / count;
> +	tstamp = data->timestamp - (count - 1) * sample_period;
> +
> +	if (samples && count > samples)
> +		count = samples;
> +
> +	ret = bmc150_accel_fifo_transfer(data->client, (u8 *)buffer, count);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Ideally we want the IIO core to handle the demux when running in fifo
> +	 * mode but not when running in triggerde buffer mode. Unfortunately
triggered.
> +	 * this does not seem to be possible, so stick with driver demux for
> +	 * now.
> +	 */
hmm. That's a new requirement.. Fiddly to do unfortunately to say the least given
the presetup nature of the demux.  Perhaps a better option would be to factor
that support out and make it available to drivers that need to do their own
demux for reasons such as this.  For now though this simple reimplementation
is fine.
> +	for (i = 0; i < count; i++) {
> +		u16 sample[8];
> +		int j, bit;
> +
> +		j = 0;
> +		for_each_set_bit(bit, indio_dev->active_scan_mask,
> +				 indio_dev->masklength)
> +			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
> +
> +		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
> +
> +		tstamp += sample_period;
> +	}
> +
> +	return count;
> +}
> +
> +static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, unsigned samples)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	ret = __bmc150_accel_fifo_flush(indio_dev, samples, false);
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>  		"15.620000 31.260000 62.50000 125 250 500 1000 2000");
>  
> @@ -950,7 +1144,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  	},
>  };
>  
> -static const struct iio_info bmc150_accel_info = {
> +static struct iio_info bmc150_accel_info = {
>  	.attrs			= &bmc150_accel_attrs_group,
>  	.read_raw		= bmc150_accel_read_raw,
>  	.write_raw		= bmc150_accel_write_raw,
> @@ -959,6 +1153,9 @@ static const struct iio_info bmc150_accel_info = {
>  	.write_event_config	= bmc150_accel_write_event_config,
>  	.read_event_config	= bmc150_accel_read_event_config,
>  	.validate_trigger	= bmc150_accel_validate_trigger,
> +	.hwfifo_set_watermark	= bmc150_accel_set_watermark,
> +	.hwfifo_get_watermark	= bmc150_accel_get_watermark,
> +	.hwfifo_flush		= bmc150_accel_fifo_flush,
>  	.driver_module		= THIS_MODULE,
>  };
>  
> @@ -1057,18 +1254,17 @@ static const struct iio_trigger_ops bmc150_accel_trigger_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
> +static int bmc150_accel_handle_roc_event(struct iio_dev *indio_dev)
>  {
> -	struct iio_dev *indio_dev = private;
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> -	int ret;
>  	int dir;
> +	int ret;
>  
>  	ret = i2c_smbus_read_byte_data(data->client,
>  				       BMC150_ACCEL_REG_INT_STATUS_2);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev, "Error reading reg_int_status_2\n");
> -		goto ack_intr_status;
> +		return ret;
>  	}
>  
>  	if (ret & BMC150_ACCEL_ANY_MOTION_BIT_SIGN)
> @@ -1097,35 +1293,73 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
>  							IIO_EV_TYPE_ROC,
>  							dir),
>  							data->timestamp);
> -ack_intr_status:
> -	if (!data->triggers[BMC150_ACCEL_TRIGGER_DATA_READY].enabled)
> +	return ret;
> +}
> +
> +static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
This wants renaming given it's no longer just handling what we
call 'events' in IIO, but also the fifo.
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	bool ack = false;
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +
> +	if (data->fifo_mode) {
> +		ret = __bmc150_accel_fifo_flush(indio_dev,
> +						BMC150_ACCEL_FIFO_LENGTH, true);
> +		if (ret > 0)
> +			ack = true;
> +	}
> +
> +	if (data->ev_enable_state) {
> +		ret = bmc150_accel_handle_roc_event(indio_dev);
> +		if (ret > 0)
> +			ack = true;
> +	}
> +
> +	if (ack) {
>  		ret = i2c_smbus_write_byte_data(data->client,
>  					BMC150_ACCEL_REG_INT_RST_LATCH,
>  					BMC150_ACCEL_INT_MODE_LATCH_INT |
>  					BMC150_ACCEL_INT_MODE_LATCH_RESET);
> +		if (ret)
> +			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> +		ret = IRQ_HANDLED;
> +	} else {
> +		ret = IRQ_NONE;
> +	}
>  
> -	return IRQ_HANDLED;
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
>  }
>  
>  static irqreturn_t bmc150_accel_data_rdy_trig_poll(int irq, void *private)
>  {
>  	struct iio_dev *indio_dev = private;
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	bool ack = false;
>  	int i;
>  
> +	data->old_timestamp = data->timestamp;
>  	data->timestamp = iio_get_time_ns();
>  
>  	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
>  		if (data->triggers[i].enabled) {
>  			iio_trigger_poll(data->triggers[i].indio_trig);
> +			ack = true;
>  			break;
>  		}
>  	}
>  
> -	if (data->ev_enable_state)
> +	if (data->ev_enable_state || data->fifo_mode)
>  		return IRQ_WAKE_THREAD;
> -	else
> +
> +	if (ack)
>  		return IRQ_HANDLED;
> +
> +	return IRQ_NONE;
>  }
>  
>  static const char *bmc150_accel_match_acpi_device(struct device *dev, int *data)
> @@ -1232,6 +1466,94 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +#define BMC150_ACCEL_FIFO_MODE_STREAM          0x80
> +#define BMC150_ACCEL_FIFO_MODE_FIFO            0x40
> +#define BMC150_ACCEL_FIFO_MODE_BYPASS          0x00
> +
> +static int bmc150_accel_fifo_set_mode(struct bmc150_accel_data *data)
> +{
> +	u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG1;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, reg, data->fifo_mode);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_fifo_config1\n");
> +		return ret;
> +	}
> +
> +	if (!data->fifo_mode)
> +		return 0;
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					BMC150_ACCEL_REG_FIFO_CONFIG0,
> +					data->watermark);
> +	if (ret < 0)
> +		dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
> +
> +	return ret;
> +}
> +
> +static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +		return iio_triggered_buffer_postenable(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +
> +	if (!data->watermark)
> +		goto out;
> +
> +	ret = bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK,
> +					 true);
> +	if (ret)
> +		goto out;
> +
> +	data->fifo_mode = BMC150_ACCEL_FIFO_MODE_FIFO;
> +
> +	ret = bmc150_accel_fifo_set_mode(data);
> +	if (ret) {
> +		data->fifo_mode = 0;
> +		bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK,
> +					   false);
> +	}
> +
> +out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +
> +	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +		return iio_triggered_buffer_predisable(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +
> +	if (!data->fifo_mode)
> +		goto out;
> +
> +	bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_WATERMARK, false);
> +	__bmc150_accel_fifo_flush(indio_dev, BMC150_ACCEL_FIFO_LENGTH, false);
> +	data->fifo_mode = 0;
> +	bmc150_accel_fifo_set_mode(data);
> +
> +out:
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops bmc150_accel_buffer_ops = {
> +	.postenable = bmc150_accel_buffer_postenable,
> +	.predisable = bmc150_accel_buffer_predisable,
> +};
> +
>  static int bmc150_accel_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
> @@ -1270,6 +1592,15 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  	indio_dev->num_channels = data->chip_info->num_channels;
>  	indio_dev->name = name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
> +	    i2c_check_functionality(client->adapter,
> +				    I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> +	} else {
> +		bmc150_accel_info.hwfifo_set_watermark = NULL;
> +		bmc150_accel_info.hwfifo_get_watermark = NULL;
> +		bmc150_accel_info.hwfifo_flush = NULL;
Have two instances of bmc150_accel_info (one with support and one without
and pick from them here). That avoids having to make that global structure
non constant and the issues that can cause for multiple instances of this
sensor on the same machine.
> +	}
>  	indio_dev->info = &bmc150_accel_info;
>  
>  	if (client->irq < 0)
> @@ -1309,7 +1640,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  		ret = iio_triggered_buffer_setup(indio_dev,
>  						 &iio_pollfunc_store_time,
>  						 bmc150_accel_trigger_handler,
> -						 NULL);
> +						 &bmc150_accel_buffer_ops);
>  		if (ret < 0) {
>  			dev_err(&client->dev,
>  				"Failed: iio triggered buffer setup\n");
> @@ -1386,6 +1717,7 @@ static int bmc150_accel_resume(struct device *dev)
>  	mutex_lock(&data->mutex);
>  	if (atomic_read(&data->active_intr))
>  		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
> +	bmc150_accel_fifo_set_mode(data);
>  	mutex_unlock(&data->mutex);
>  
>  	return 0;
> @@ -1419,6 +1751,9 @@ static int bmc150_accel_runtime_resume(struct device *dev)
>  	ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
>  	if (ret < 0)
>  		return ret;
> +	ret = bmc150_accel_fifo_set_mode(data);
> +	if (ret < 0)
> +		return ret;
>  
>  	sleep_val = bmc150_accel_get_startup_times(data);
>  	if (sleep_val < 20)
> 

Re: [PATCH v5 2/3] iio: add support for hardware fifo

[Octavian Purdila]

On Sat, Mar 14, 2015 at 8:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On 04/03/15 17:56, Octavian Purdila wrote:
> > Some devices have hardware buffers that can store a number of samples
> > for later consumption. Hardware usually provides interrupts to notify
> > the processor when the fifo is full or when it has reached a certain
> > threshold. This helps with reducing the number of interrupts to the
> > host processor and thus it helps decreasing the power consumption.
> >
> > This patch adds support for hardware fifo to IIO by adding drivers
> > operations for flushing the hadware fifo and setting and getting the
> > watermark level.
> >
> > Since a driver may support hardware fifo only when not in triggered
> > buffer mode (due to different samantics of hardware fifo sampling and
> > triggered sampling) this patch changes the IIO core code to allow
> > falling back to non-triggered buffered mode if no trigger is enabled.
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Bits and bobs inline
>
> Typo's and a comment on weird hardware possibilities..
>
> Also I'm not keen on some of the error handling.
>
> Sorry it too me so very long to look at this.  I keep thinking
> it'll be fiddly and hence doing the easier stuff first only
> to come to this and see that actually its very straight forward!.

Thank you for the review Jonathan ! I will try to be more responsive
to help with the review process, sorry for the long absence.

>
> Jonathan
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
> >  drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
> >  include/linux/iio/iio.h                 | 26 +++++++++++++
> >  3 files changed, 108 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 1283ca7..143ddf2d 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1264,3 +1264,28 @@ Description:
> >               allows the application to block on poll with a timeout and read
> >               the available samples after the timeout expires and thus have a
> >               maximum delay guarantee.
> > +
> > +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
> > +KernelVersion: 3.21
> > +Contact:       linux-iio@vger.kernel.org
> > +Description:
> > +            Read-only entry that contains a single integer specifying the
> > +            current level for the hardware fifo watermark level. If this
> > +            value is negative it means that the device does not support a
> > +            hardware fifo. If this value is 0 it means that the hardware fifo
> > +            is currently disabled.
> I wonder if we should indicate lack of hardware support by just not exporting
> this attribute in the first place?

OK, I don't see any reasons for which shouldn't work, I will give it a
try in the next version.

> > +            If this value is strictly positive it signals that the hardware
> > +            fifo of the device is active and that samples are stored in an
> > +            internal hardware buffer. When the level of the hardware fifo
> > +            reaches the watermark level the device will flush its internal
> > +            buffer to the device buffer. Because of this a trigger is not
> > +            needed to use the device in buffer mode.
> > +            The hardware watermark level is set by the driver based on the
> > +            value set by the user in buffer/watermark but taking into account
> > +            the limitations of the hardware (e.g. most hardware buffers are
> > +            limited to 32-64 samples).
> > +            Because the sematics of triggers and hardware fifo may be
> > +            different (e.g. the hadware fifo may record samples according to
> hardware fifo

Ack.

> > +            the sample rate while an any-motion trigger generates samples
> > +            based on the set rate of change) setting a trigger may disable
> > +            the hardware fifo.
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index a4f4f07..b6f1bd4 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -42,18 +42,47 @@ static size_t iio_buffer_data_available(struct iio_buffer *buf)
> >       return buf->access->data_available(buf);
> >  }
> >
> > +static int iio_buffer_flush_hwfifo(struct iio_dev *indio_dev,
> > +                                struct iio_buffer *buf, size_t required)
> > +{
> > +     if (!indio_dev->info->hwfifo_flush)
> > +             return -ENODEV;
> > +
> > +     return indio_dev->info->hwfifo_flush(indio_dev, required);
> > +}
> > +
> >  static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
> > -                          size_t to_wait)
> > +                          size_t to_wait, bool try_flush)
> >  {
> > +     size_t avail;
> > +     int flushed = 0;
> > +
> >       /* wakeup if the device was unregistered */
> >       if (!indio_dev->info)
> >               return true;
> >
> >       /* drain the buffer if it was disabled */
> > -     if (!iio_buffer_is_active(buf))
> > +     if (!iio_buffer_is_active(buf)) {
> >               to_wait = min_t(size_t, to_wait, 1);
> > +             try_flush = false;
> > +     }
> > +
> > +     avail = iio_buffer_data_available(buf);
> >
> > -     if (iio_buffer_data_available(buf) >= to_wait)
> > +     if (avail >= to_wait) {
> I'm not really following what this is for.
> If to_wait = 0 and avail = 0 and we have requested a flush
> then try to read 1 element?
>
> I think this only occurs on a non blocking read when nothing is
> available?  What I don't understand is why we'd only try to read 1
> sample.  Surely asking for as many as requested is more intuitive?
>

Yes, good idea.

> > +             /* force a flush for non-blocking reads */
> > +             if (!to_wait && !avail && try_flush)
> > +                     iio_buffer_flush_hwfifo(indio_dev, buf, 1);
> > +             return true;
> > +     }
> > +
> > +     if (try_flush)
> > +             flushed = iio_buffer_flush_hwfifo(indio_dev, buf,
> > +                                               to_wait - avail);
> > +     if (flushed <= 0)
> > +             return false;
> > +
> > +     if (avail + flushed >= to_wait)
> >               return true;
> >
> >       return false;
> > @@ -92,7 +121,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
> >
> >       do {
> >               ret = wait_event_interruptible(rb->pollq,
> > -                                   iio_buffer_ready(indio_dev, rb, to_wait));
> > +                            iio_buffer_ready(indio_dev, rb, to_wait, true));
> >               if (ret)
> >                       return ret;
> >
> > @@ -120,7 +149,7 @@ unsigned int iio_buffer_poll(struct file *filp,
> >               return -ENODEV;
> >
> >       poll_wait(filp, &rb->pollq, wait);
> > -     if (iio_buffer_ready(indio_dev, rb, rb->watermark))
> > +     if (iio_buffer_ready(indio_dev, rb, rb->watermark, false))
> >               return POLLIN | POLLRDNORM;
> >       return 0;
> >  }
> > @@ -659,19 +688,16 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
> >               }
> >       }
> >       /* Definitely possible for devices to support both of these. */
> > -     if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) {
> > -             if (!indio_dev->trig) {
> > -                     printk(KERN_INFO "Buffer not started: no trigger\n");
> > -                     ret = -EINVAL;
> > -                     /* Can only occur on first buffer */
> > -                     goto error_run_postdisable;
> > -             }
> > +     if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
> >               indio_dev->currentmode = INDIO_BUFFER_TRIGGERED;
> >       } else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
> >               indio_dev->currentmode = INDIO_BUFFER_HARDWARE;
> >       } else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
> >               indio_dev->currentmode = INDIO_BUFFER_SOFTWARE;
> >       } else { /* Should never be reached */
> > +             /* Can only occur on first buffer */
> > +             if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
> > +                     pr_info("Buffer not started: no trigger\n");
> >               ret = -EINVAL;
> >               goto error_run_postdisable;
> >       }
> > @@ -823,12 +849,28 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
> >       }
> >
> >       buffer->watermark = val;
> > +
> > +     if (indio_dev->info->hwfifo_set_watermark)
> > +             indio_dev->info->hwfifo_set_watermark(indio_dev, val);
> >  out:
> >       mutex_unlock(&indio_dev->mlock);
> >
> >       return ret ? ret : len;
> >  }
> >
> > +static ssize_t iio_buffer_show_hwfifo_watermark(struct device *dev,
> > +                                             struct device_attribute *attr,
> > +                                             char *buf)
> > +{
> > +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +     int ret = -1;
> > +
> > +     if (indio_dev->info->hwfifo_get_watermark)
> > +             ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
> Any error code returned should be returned by this function to userspace.
>
> Also, ret = -1 would be EPERM, a possible error code.  Hence you need
> to separate your handling of not supported from possible return values.
>
> if (indio_dev->info->hwfifo_get_watermark) {
>         ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
>         if (ret < 0)
>            return ret;
>         return sprintf(buf, "%d\n", ret);
> } else {
>         return sprintf(buf, "-1\n");
>
> seems the simplest way to me.

If we make the entry visible only when we have the hw fifo supported
then we can remove the else branch. What do you think?

> > +
> > +     return sprintf(buf, "%d\n", ret < -1 ? -1 : ret);
> > +}
> > +
> >  static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
> >                  iio_buffer_write_length);
> >  static struct device_attribute dev_attr_length_ro = __ATTR(length,
> > @@ -837,11 +879,14 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
> >                  iio_buffer_show_enable, iio_buffer_store_enable);
> >  static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
> >                  iio_buffer_show_watermark, iio_buffer_store_watermark);
> > +static DEVICE_ATTR(hwfifo_watermark, S_IRUGO, iio_buffer_show_hwfifo_watermark,
> > +                NULL);
> >
> >  static struct attribute *iio_buffer_attrs[] = {
> >       &dev_attr_length.attr,
> >       &dev_attr_enable.attr,
> >       &dev_attr_watermark.attr,
> > +     &dev_attr_hwfifo_watermark.attr,
> >  };
> >
> >  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 80d8550..1b1cd7d 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -338,6 +338,29 @@ struct iio_dev;
> >   *                   provide a custom of_xlate function that reads the
> >   *                   *args* and returns the appropriate index in registered
> >   *                   IIO channels array.
> > + * @hwfifo_set_watermark: function pointer to set the current hardware fifo
> > + *                   watermark level. It receives the desired watermark as a
> > + *                   hint and the device driver may adjust it to take into
> > + *                   account hardware limitations. Setting the watermark to a
> > + *                   strictly positive value should enable the hardware fifo
> > + *                   if not already enabled. When the hardware fifo is
> > + *                   enabled and its level reaches the watermark level the
> > + *                   device must flush the samples stored in the hardware
> > + *                   fifo to the device buffer.
> Hmm. This one is interesting.  I wonder if we have devices with minimum
> watermark levels.  I know that devices with fixed levels exist (sca3000 IIRC)
> but in that case if we are below the level we can fallback to the data ready
> signal (watermark of 1 effectively).  If the device provides no notification
> that data capture is occuring until we reach a particular level (say 50% full)
> then its not entirely clear whether we should turn the buffer on when a
> watermark below that is set.

Interesting, in that case I think we should expose
hwfifo_min_watermark to let userspace know how it needs to set the
watermark level to enable the hardware fifo. Then we can update the
ABI to say that the driver will enable the fifo if the watermark level
is greater than hwfifo_min_watermark.

Related to this, I think we should also add hwfifo_max_watermark to
make it easier to userspace to pick a good value.

One more thing: right now I am overloading the hwfifo_get_watermark
(and hwfifio_watermark sysfs entry with information about whether the
fifo is active. Hence if hwfifo is not active we return 0, otherwise
we return the current hwfifo level. Given that some hardware may have
additional hwfifo limitations (say perhaps not all values in the min
.. max range are supported), perhaps is better to add a separate
function and sysfs entry for checking if the hwfifo is active or not.
This way userspace can set watermark then read hwfifo_watermark to see
what is the actual watermark value without needing to start the buffer
for checking that value. What do you think?

> > Setting the watermark to 0
> > + *                   should disable the hardware fifo. The device driver must
> > + *                   disable the hardware fifo when a trigger with different
> > + *                   sampling semantic (then the hardware fifo) is set
> (than the...
> > + *                   (e.g. when setting an any-motion trigger to a device
> > + *                   that has its hardware fifo sample based on the set
> > + *                   sample frequency).
> > + * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
> > + *                   watermark level
> > + * @hwfifo_flush:    function pointer to flush the samples stored in the
> > + *                   hardware fifo to the device buffer. The driver should
> > + *                   not flush more then count samples. The function must
> more than count

Oops :)

> > + *                   return the number of samples flushed, 0 if no samples
> > + *                   were flushed or a negative integer if no samples were
> > + *                   flushed and there was an error.
> >   **/
> >  struct iio_info {
> >       struct module                   *driver_module;
> > @@ -399,6 +422,9 @@ struct iio_info {
> >                                 unsigned *readval);
> >       int (*of_xlate)(struct iio_dev *indio_dev,
> >                       const struct of_phandle_args *iiospec);
> > +     int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val);
> > +     int (*hwfifo_get_watermark)(struct iio_dev *indio_dev);
> > +     int (*hwfifo_flush)(struct iio_dev *indio_dev, unsigned count);
> >  };
> >
> >  /**
> >
>
> --
> 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 v5 1/3] iio: add watermark logic to iio read and poll

[Lars-Peter Clausen]

Looks good mostly, two things.

On 03/04/2015 06:56 PM, Octavian Purdila wrote:
[...]
> @@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>   	if (!rb || !rb->access->read_first_n)
>   		return -EINVAL;
>
> +	/*
> +	 * If datum_size is 0 there will never be anything to read from the
> +	 * buffer, so signal end of file now.
> +	 */
> +	if (!datum_size)
> +		return 0;
> +
> +	if (!(filp->f_flags & O_NONBLOCK))
> +		to_wait = min_t(size_t, n / datum_size, rb->watermark);
> +
>   	do {
> -		if (!iio_buffer_data_available(rb)) {
> -			if (filp->f_flags & O_NONBLOCK)
> -				return -EAGAIN;

If I understand this correctly we no longer return -EAGAIN if no data is 
available in non blocking mode, but rather return 0. Which means 
end-of-file, so it is not correct.

> +		ret = wait_event_interruptible(rb->pollq,
> +				      iio_buffer_ready(indio_dev, rb, to_wait));
> +		if (ret)
> +			return ret;
>
> -			ret = wait_event_interruptible(rb->pollq,
> -					iio_buffer_data_available(rb) ||
> -					indio_dev->info == NULL);
> -			if (ret)
> -				return ret;
> -			if (indio_dev->info == NULL)
> -				return -ENODEV;
> -		}
> +		if (!indio_dev->info)
> +			return -ENODEV;
>   [...]
> +
> +static ssize_t iio_buffer_store_watermark(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf,
> +					  size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +	if (!val)
> +		return -EINVAL;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	if (val > buffer->length) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

This is missing the check for val == 0.

> +
> +	if (iio_buffer_is_active(indio_dev->buffer)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	buffer->watermark = val;
> +out:
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret ? ret : len;
> +}

Re: [PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Octavian Purdila]

On Wed, Mar 18, 2015 at 11:19 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> Looks good mostly, two things.
>

Thanks for the review Lars !

> On 03/04/2015 06:56 PM, Octavian Purdila wrote:
> [...]
>>
>> @@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file
>> *filp, char __user *buf,
>>         if (!rb || !rb->access->read_first_n)
>>                 return -EINVAL;
>>
>> +       /*
>> +        * If datum_size is 0 there will never be anything to read from
>> the
>> +        * buffer, so signal end of file now.
>> +        */
>> +       if (!datum_size)
>> +               return 0;
>> +
>> +       if (!(filp->f_flags & O_NONBLOCK))
>> +               to_wait = min_t(size_t, n / datum_size, rb->watermark);
>> +
>>         do {
>> -               if (!iio_buffer_data_available(rb)) {
>> -                       if (filp->f_flags & O_NONBLOCK)
>> -                               return -EAGAIN;
>
>
> If I understand this correctly we no longer return -EAGAIN if no data is
> available in non blocking mode, but rather return 0. Which means
> end-of-file, so it is not correct.
>

You are right, I'll add have to add back the if
(!iio_buffer_data_available(rb) && (filp->f_flags & O_NONBLOCK))
statement.

>> +               ret = wait_event_interruptible(rb->pollq,
>> +                                     iio_buffer_ready(indio_dev, rb,
>> to_wait));
>> +               if (ret)
>> +                       return ret;
>>
>> -                       ret = wait_event_interruptible(rb->pollq,
>> -                                       iio_buffer_data_available(rb) ||
>> -                                       indio_dev->info == NULL);
>> -                       if (ret)
>> -                               return ret;
>> -                       if (indio_dev->info == NULL)
>> -                               return -ENODEV;
>> -               }
>> +               if (!indio_dev->info)
>> +                       return -ENODEV;
>>   [...]
>> +
>> +static ssize_t iio_buffer_store_watermark(struct device *dev,
>> +                                         struct device_attribute *attr,
>> +                                         const char *buf,
>> +                                         size_t len)
>> +{
>> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +       struct iio_buffer *buffer = indio_dev->buffer;
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       ret = kstrtouint(buf, 10, &val);
>> +       if (ret)
>> +               return ret;
>> +       if (!val)
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&indio_dev->mlock);
>> +
>> +       if (val > buffer->length) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>
>
> This is missing the check for val == 0.
>

Unless I misunderstand you, the check is done right after kstrtouint()
before taking the lock.

>
>> +
>> +       if (iio_buffer_is_active(indio_dev->buffer)) {
>> +               ret = -EBUSY;
>> +               goto out;
>> +       }
>> +
>> +       buffer->watermark = val;
>> +out:
>> +       mutex_unlock(&indio_dev->mlock);
>> +
>> +       return ret ? ret : len;
>> +}
>
>

Re: [PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Lars-Peter Clausen]

On 03/18/2015 10:29 AM, Octavian Purdila wrote:
> On Wed, Mar 18, 2015 at 11:19 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:

>>> +static ssize_t iio_buffer_store_watermark(struct device *dev,
>>> +                                         struct device_attribute *attr,
>>> +                                         const char *buf,
>>> +                                         size_t len)
>>> +{
>>> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +       struct iio_buffer *buffer = indio_dev->buffer;
>>> +       unsigned int val;
>>> +       int ret;
>>> +
>>> +       ret = kstrtouint(buf, 10, &val);
>>> +       if (ret)
>>> +               return ret;
>>> +       if (!val)
>>> +               return -EINVAL;
>>> +
>>> +       mutex_lock(&indio_dev->mlock);
>>> +
>>> +       if (val > buffer->length) {
>>> +               ret = -EINVAL;
>>> +               goto out;
>>> +       }
>>
>>
>> This is missing the check for val == 0.
>>
>
> Unless I misunderstand you, the check is done right after kstrtouint()
> before taking the lock.

Yep, sorry overlooked that.

Re: [PATCH v5 2/3] iio: add support for hardware fifo

[Lars-Peter Clausen]

On 03/04/2015 06:56 PM, Octavian Purdila wrote:
>[...]
>   Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
>   drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
>   include/linux/iio/iio.h                 | 26 +++++++++++++
>   3 files changed, 108 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 1283ca7..143ddf2d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1264,3 +1264,28 @@ Description:
>   		allows the application to block on poll with a timeout and read
>   		the available samples after the timeout expires and thus have a
>   		maximum delay guarantee.
> +
> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
> +KernelVersion: 3.21
> +Contact:       linux-iio@vger.kernel.org
> +Description:
> +	       Read-only entry that contains a single integer specifying the
> +	       current level for the hardware fifo watermark level. If this
> +	       value is negative it means that the device does not support a
> +	       hardware fifo. If this value is 0 it means that the hardware fifo
> +	       is currently disabled.
> +	       If this value is strictly positive it signals that the hardware
> +	       fifo of the device is active and that samples are stored in an
> +	       internal hardware buffer. When the level of the hardware fifo
> +	       reaches the watermark level the device will flush its internal
> +	       buffer to the device buffer. Because of this a trigger is not
> +	       needed to use the device in buffer mode.
> +	       The hardware watermark level is set by the driver based on the
> +	       value set by the user in buffer/watermark but taking into account
> +	       the limitations of the hardware (e.g. most hardware buffers are
> +	       limited to 32-64 samples).
> +	       Because the sematics of triggers and hardware fifo may be

semantics

> +	       different (e.g. the hadware fifo may record samples according to
> +	       the sample rate while an any-motion trigger generates samples
> +	       based on the set rate of change) setting a trigger may disable
> +	       the hardware fifo.

I still don't understand why the hardware fifo level is something the 
userspace application needs to set. And how would the application decide 
which level it wants?

[...]
>   int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 80d8550..1b1cd7d 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -338,6 +338,29 @@ struct iio_dev;
>    *			provide a custom of_xlate function that reads the
>    *			*args* and returns the appropriate index in registered
>    *			IIO channels array.
> + * @hwfifo_set_watermark: function pointer to set the current hardware fifo
> + *			watermark level. It receives the desired watermark as a
> + *			hint and the device driver may adjust it to take into
> + *			account hardware limitations. Setting the watermark to a
> + *			strictly positive value should enable the hardware fifo
> + *			if not already enabled. When the hardware fifo is
> + *			enabled and its level reaches the watermark level the
> + *			device must flush the samples stored in the hardware
> + *			fifo to the device buffer. Setting the watermark to 0
> + *			should disable the hardware fifo. The device driver must
> + *			disable the hardware fifo when a trigger with different
> + *			sampling semantic (then the hardware fifo) is set
> + *			(e.g. when setting an any-motion trigger to a device
> + *			that has its hardware fifo sample based on the set
> + *			sample frequency).
> + * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
> + *			watermark level
> + * @hwfifo_flush:	function pointer to flush the samples stored in the

This might just be me, but I associate flushing a FIFO with discarding the 
data. Our previous discussions make a lot more sense now :)

> + *			hardware fifo to the device buffer. The driver should
> + *			not flush more then count samples. The function must
> + *			return the number of samples flushed, 0 if no samples
> + *			were flushed or a negative integer if no samples were
> + *			flushed and there was an error.
>    **/
>   struct iio_info {
>   	struct module			*driver_module;
> @@ -399,6 +422,9 @@ struct iio_info {
>   				  unsigned *readval);
>   	int (*of_xlate)(struct iio_dev *indio_dev,
>   			const struct of_phandle_args *iiospec);
> +	int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val);
> +	int (*hwfifo_get_watermark)(struct iio_dev *indio_dev);
> +	int (*hwfifo_flush)(struct iio_dev *indio_dev, unsigned count);
>   };
>
>   /**
>

Re: [PATCH v5 2/3] iio: add support for hardware fifo

[Octavian Purdila]

On Wed, Mar 18, 2015 at 1:55 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 03/04/2015 06:56 PM, Octavian Purdila wrote:
>>
>> [...]
>>
>>   Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
>>   drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
>>   include/linux/iio/iio.h                 | 26 +++++++++++++
>>   3 files changed, 108 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index 1283ca7..143ddf2d 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -1264,3 +1264,28 @@ Description:
>>                 allows the application to block on poll with a timeout and read
>>                 the available samples after the timeout expires and thus have a
>>                 maximum delay guarantee.
>> +
>> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
>> +KernelVersion: 3.21
>> +Contact:       linux-iio@vger.kernel.org
>> +Description:
>> +              Read-only entry that contains a single integer specifying the
>> +              current level for the hardware fifo watermark level. If this
>> +              value is negative it means that the device does not support a
>> +              hardware fifo. If this value is 0 it means that the hardware fifo
>> +              is currently disabled.
>> +              If this value is strictly positive it signals that the hardware
>> +              fifo of the device is active and that samples are stored in an
>> +              internal hardware buffer. When the level of the hardware fifo
>> +              reaches the watermark level the device will flush its internal
>> +              buffer to the device buffer. Because of this a trigger is not
>> +              needed to use the device in buffer mode.
>> +              The hardware watermark level is set by the driver based on the
>> +              value set by the user in buffer/watermark but taking into account
>> +              the limitations of the hardware (e.g. most hardware buffers are
>> +              limited to 32-64 samples).
>> +              Because the sematics of triggers and hardware fifo may be
>
>
> semantics

Ack.

>
>> +              different (e.g. the hadware fifo may record samples according to
>> +              the sample rate while an any-motion trigger generates samples
>> +              based on the set rate of change) setting a trigger may disable
>> +              the hardware fifo.
>
>
> I still don't understand why the hardware fifo level is something the userspace application needs to set. And how would the application decide which level it wants?

This entry is read-only, it is not set by the user.  The user sets
buffer/watermark then the driver sets the hardware fifo watermark and
that is visible here.

Please also see my last proposals in the response I've sent to Jonathan:

* add a hwfifo_watermark_min to let the application know what
watermark level it needs to set so that hardware fifo can be enabled

* add a hwfifo_watermark_max entry - required by Android batch mode
(see sensor_t.fifoMaxEventCount at [1]); it should be useful to
compute a watermark value that avoids overflowing the hardware fifo

* add a hwfifo_active entry - makes it easy to know if the hardware
fifo is active or not

[1] https://source.android.com/devices/sensors/batching.html

>
>
> [...]
>
>>   int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 80d8550..1b1cd7d 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -338,6 +338,29 @@ struct iio_dev;
>>    *                    provide a custom of_xlate function that reads the
>>    *                    *args* and returns the appropriate index in registered
>>    *                    IIO channels array.
>> + * @hwfifo_set_watermark: function pointer to set the current hardware fifo
>> + *                     watermark level. It receives the desired watermark as a
>> + *                     hint and the device driver may adjust it to take into
>> + *                     account hardware limitations. Setting the watermark to a
>> + *                     strictly positive value should enable the hardware fifo
>> + *                     if not already enabled. When the hardware fifo is
>> + *                     enabled and its level reaches the watermark level the
>> + *                     device must flush the samples stored in the hardware
>> + *                     fifo to the device buffer. Setting the watermark to 0
>> + *                     should disable the hardware fifo. The device driver must
>> + *                     disable the hardware fifo when a trigger with different
>> + *                     sampling semantic (then the hardware fifo) is set
>> + *                     (e.g. when setting an any-motion trigger to a device
>> + *                     that has its hardware fifo sample based on the set
>> + *                     sample frequency).
>> + * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
>> + *                     watermark level
>> + * @hwfifo_flush:      function pointer to flush the samples stored in the
>
>
> This might just be me, but I associate flushing a FIFO with discarding the data. Our previous discussions make a lot more sense now :)
>
>

I see :) I've used the terminology from the processor cache where
flush is used to "save" the cache line to memory and invalidate to
discard it.

Re: [PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Octavian Purdila]

On Wed, Mar 18, 2015 at 11:29 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Mar 18, 2015 at 11:19 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> Looks good mostly, two things.
>>
>
> Thanks for the review Lars !
>
>> On 03/04/2015 06:56 PM, Octavian Purdila wrote:
>> [...]
>>>
>>> @@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file
>>> *filp, char __user *buf,
>>>         if (!rb || !rb->access->read_first_n)
>>>                 return -EINVAL;
>>>
>>> +       /*
>>> +        * If datum_size is 0 there will never be anything to read from
>>> the
>>> +        * buffer, so signal end of file now.
>>> +        */
>>> +       if (!datum_size)
>>> +               return 0;
>>> +
>>> +       if (!(filp->f_flags & O_NONBLOCK))
>>> +               to_wait = min_t(size_t, n / datum_size, rb->watermark);
>>> +
>>>         do {
>>> -               if (!iio_buffer_data_available(rb)) {
>>> -                       if (filp->f_flags & O_NONBLOCK)
>>> -                               return -EAGAIN;
>>
>>
>> If I understand this correctly we no longer return -EAGAIN if no data is
>> available in non blocking mode, but rather return 0. Which means
>> end-of-file, so it is not correct.
>>
>
> You are right, I'll add have to add back the if
> (!iio_buffer_data_available(rb) && (filp->f_flags & O_NONBLOCK))
> statement.
>
>>> +               ret = wait_event_interruptible(rb->pollq,
>>> +                                     iio_buffer_ready(indio_dev, rb,
>>> to_wait));
>>> +               if (ret)
>>> +                       return ret;
>>>
>>> -                       ret = wait_event_interruptible(rb->pollq,
>>> -                                       iio_buffer_data_available(rb) ||
>>> -                                       indio_dev->info == NULL);
>>> -                       if (ret)
>>> -                               return ret;
>>> -                       if (indio_dev->info == NULL)
>>> -                               return -ENODEV;
>>> -               }
>>> +               if (!indio_dev->info)
>>> +                       return -ENODEV;
>>>   [...]
>>> +
>>> +static ssize_t iio_buffer_store_watermark(struct device *dev,
>>> +                                         struct device_attribute *attr,
>>> +                                         const char *buf,
>>> +                                         size_t len)
>>> +{
>>> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +       struct iio_buffer *buffer = indio_dev->buffer;
>>> +       unsigned int val;
>>> +       int ret;
>>> +
>>> +       ret = kstrtouint(buf, 10, &val);
>>> +       if (ret)
>>> +               return ret;
>>> +       if (!val)
>>> +               return -EINVAL;
>>> +
>>> +       mutex_lock(&indio_dev->mlock);
>>> +
>>> +       if (val > buffer->length) {
>>> +               ret = -EINVAL;
>>> +               goto out;
>>> +       }
>>
>>
>> This is missing the check for val == 0.
>>
>
> Unless I misunderstand you, the check is done right after kstrtouint()
> before taking the lock.
>
>>
>>> +
>>> +       if (iio_buffer_is_active(indio_dev->buffer)) {
>>> +               ret = -EBUSY;
>>> +               goto out;
>>> +       }
>>> +
>>> +       buffer->watermark = val;
>>> +out:
>>> +       mutex_unlock(&indio_dev->mlock);
>>> +
>>> +       return ret ? ret : len;
>>> +}
>>
>>

Re: [PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Octavian Purdila]

On Wed, Mar 18, 2015 at 11:29 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Mar 18, 2015 at 11:19 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> Looks good mostly, two things.
>>
>
> Thanks for the review Lars !
>
>> On 03/04/2015 06:56 PM, Octavian Purdila wrote:
>> [...]
>>>
>>> @@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file
>>> *filp, char __user *buf,
>>>         if (!rb || !rb->access->read_first_n)
>>>                 return -EINVAL;
>>>
>>> +       /*
>>> +        * If datum_size is 0 there will never be anything to read from
>>> the
>>> +        * buffer, so signal end of file now.
>>> +        */
>>> +       if (!datum_size)
>>> +               return 0;
>>> +
>>> +       if (!(filp->f_flags & O_NONBLOCK))
>>> +               to_wait = min_t(size_t, n / datum_size, rb->watermark);
>>> +
>>>         do {
>>> -               if (!iio_buffer_data_available(rb)) {
>>> -                       if (filp->f_flags & O_NONBLOCK)
>>> -                               return -EAGAIN;
>>
>>
>> If I understand this correctly we no longer return -EAGAIN if no data is
>> available in non blocking mode, but rather return 0. Which means
>> end-of-file, so it is not correct.
>>
>
> You are right, I'll add have to add back the if
> (!iio_buffer_data_available(rb) && (filp->f_flags & O_NONBLOCK))
> statement.
>

Actually this case is already taken care of below the patch context:

                ret = rb->access->read_first_n(rb, n, buf);
                if (ret == 0 && (filp->f_flags & O_NONBLOCK))
                        ret = -EAGAIN;


And sorry for the previous spurious email :)

>>> +               ret = wait_event_interruptible(rb->pollq,
>>> +                                     iio_buffer_ready(indio_dev, rb,
>>> to_wait));
>>> +               if (ret)
>>> +                       return ret;
>>>
>>> -                       ret = wait_event_interruptible(rb->pollq,
>>> -                                       iio_buffer_data_available(rb) ||
>>> -                                       indio_dev->info == NULL);
>>> -                       if (ret)
>>> -                               return ret;
>>> -                       if (indio_dev->info == NULL)
>>> -                               return -ENODEV;
>>> -               }
>>> +               if (!indio_dev->info)
>>> +                       return -ENODEV;
>>>   [...]
>>> +
>>> +static ssize_t iio_buffer_store_watermark(struct device *dev,
>>> +                                         struct device_attribute *attr,
>>> +                                         const char *buf,
>>> +                                         size_t len)
>>> +{
>>> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +       struct iio_buffer *buffer = indio_dev->buffer;
>>> +       unsigned int val;
>>> +       int ret;
>>> +
>>> +       ret = kstrtouint(buf, 10, &val);
>>> +       if (ret)
>>> +               return ret;
>>> +       if (!val)
>>> +               return -EINVAL;
>>> +
>>> +       mutex_lock(&indio_dev->mlock);
>>> +
>>> +       if (val > buffer->length) {
>>> +               ret = -EINVAL;
>>> +               goto out;
>>> +       }
>>
>>
>> This is missing the check for val == 0.
>>
>
> Unless I misunderstand you, the check is done right after kstrtouint()
> before taking the lock.
>
>>
>>> +
>>> +       if (iio_buffer_is_active(indio_dev->buffer)) {
>>> +               ret = -EBUSY;
>>> +               goto out;
>>> +       }
>>> +
>>> +       buffer->watermark = val;
>>> +out:
>>> +       mutex_unlock(&indio_dev->mlock);
>>> +
>>> +       return ret ? ret : len;
>>> +}
>>
>>

Re: [PATCH v5 2/3] iio: add support for hardware fifo

[Jonathan Cameron]

On 16/03/15 10:05, Octavian Purdila wrote:
> On Sat, Mar 14, 2015 at 8:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> On 04/03/15 17:56, Octavian Purdila wrote:
>>> Some devices have hardware buffers that can store a number of samples
>>> for later consumption. Hardware usually provides interrupts to notify
>>> the processor when the fifo is full or when it has reached a certain
>>> threshold. This helps with reducing the number of interrupts to the
>>> host processor and thus it helps decreasing the power consumption.
>>>
>>> This patch adds support for hardware fifo to IIO by adding drivers
>>> operations for flushing the hadware fifo and setting and getting the
>>> watermark level.
>>>
>>> Since a driver may support hardware fifo only when not in triggered
>>> buffer mode (due to different samantics of hardware fifo sampling and
>>> triggered sampling) this patch changes the IIO core code to allow
>>> falling back to non-triggered buffered mode if no trigger is enabled.
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>> Bits and bobs inline
>>
>> Typo's and a comment on weird hardware possibilities..
>>
>> Also I'm not keen on some of the error handling.
>>
>> Sorry it too me so very long to look at this.  I keep thinking
>> it'll be fiddly and hence doing the easier stuff first only
>> to come to this and see that actually its very straight forward!.
> 
> Thank you for the review Jonathan ! I will try to be more responsive
> to help with the review process, sorry for the long absence.
You are welcome.  Take any time you need with this stuff. You are working
on an area we have all wanted for a while, but no one else has taken the
time to do it. Your hard work is very much appreciated!
> 
>>
>> Jonathan
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
>>>  drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
>>>  include/linux/iio/iio.h                 | 26 +++++++++++++
>>>  3 files changed, 108 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 1283ca7..143ddf2d 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1264,3 +1264,28 @@ Description:
>>>               allows the application to block on poll with a timeout and read
>>>               the available samples after the timeout expires and thus have a
>>>               maximum delay guarantee.
>>> +
>>> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
>>> +KernelVersion: 3.21
>>> +Contact:       linux-iio@vger.kernel.org
>>> +Description:
>>> +            Read-only entry that contains a single integer specifying the
>>> +            current level for the hardware fifo watermark level. If this
>>> +            value is negative it means that the device does not support a
>>> +            hardware fifo. If this value is 0 it means that the hardware fifo
>>> +            is currently disabled.
>> I wonder if we should indicate lack of hardware support by just not exporting
>> this attribute in the first place?
> 
> OK, I don't see any reasons for which shouldn't work, I will give it a
> try in the next version.
> 
>>> +            If this value is strictly positive it signals that the hardware
>>> +            fifo of the device is active and that samples are stored in an
>>> +            internal hardware buffer. When the level of the hardware fifo
>>> +            reaches the watermark level the device will flush its internal
>>> +            buffer to the device buffer. Because of this a trigger is not
>>> +            needed to use the device in buffer mode.
>>> +            The hardware watermark level is set by the driver based on the
>>> +            value set by the user in buffer/watermark but taking into account
>>> +            the limitations of the hardware (e.g. most hardware buffers are
>>> +            limited to 32-64 samples).
>>> +            Because the sematics of triggers and hardware fifo may be
>>> +            different (e.g. the hadware fifo may record samples according to
>> hardware fifo
> 
> Ack.
> 
>>> +            the sample rate while an any-motion trigger generates samples
>>> +            based on the set rate of change) setting a trigger may disable
>>> +            the hardware fifo.
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index a4f4f07..b6f1bd4 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -42,18 +42,47 @@ static size_t iio_buffer_data_available(struct iio_buffer *buf)
>>>       return buf->access->data_available(buf);
>>>  }
>>>
>>> +static int iio_buffer_flush_hwfifo(struct iio_dev *indio_dev,
>>> +                                struct iio_buffer *buf, size_t required)
>>> +{
>>> +     if (!indio_dev->info->hwfifo_flush)
>>> +             return -ENODEV;
>>> +
>>> +     return indio_dev->info->hwfifo_flush(indio_dev, required);
>>> +}
>>> +
>>>  static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
>>> -                          size_t to_wait)
>>> +                          size_t to_wait, bool try_flush)
>>>  {
>>> +     size_t avail;
>>> +     int flushed = 0;
>>> +
>>>       /* wakeup if the device was unregistered */
>>>       if (!indio_dev->info)
>>>               return true;
>>>
>>>       /* drain the buffer if it was disabled */
>>> -     if (!iio_buffer_is_active(buf))
>>> +     if (!iio_buffer_is_active(buf)) {
>>>               to_wait = min_t(size_t, to_wait, 1);
>>> +             try_flush = false;
>>> +     }
>>> +
>>> +     avail = iio_buffer_data_available(buf);
>>>
>>> -     if (iio_buffer_data_available(buf) >= to_wait)
>>> +     if (avail >= to_wait) {
>> I'm not really following what this is for.
>> If to_wait = 0 and avail = 0 and we have requested a flush
>> then try to read 1 element?
>>
>> I think this only occurs on a non blocking read when nothing is
>> available?  What I don't understand is why we'd only try to read 1
>> sample.  Surely asking for as many as requested is more intuitive?
>>
> 
> Yes, good idea.
> 
>>> +             /* force a flush for non-blocking reads */
>>> +             if (!to_wait && !avail && try_flush)
>>> +                     iio_buffer_flush_hwfifo(indio_dev, buf, 1);
>>> +             return true;
>>> +     }
>>> +
>>> +     if (try_flush)
>>> +             flushed = iio_buffer_flush_hwfifo(indio_dev, buf,
>>> +                                               to_wait - avail);
>>> +     if (flushed <= 0)
>>> +             return false;
>>> +
>>> +     if (avail + flushed >= to_wait)
>>>               return true;
>>>
>>>       return false;
>>> @@ -92,7 +121,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>>
>>>       do {
>>>               ret = wait_event_interruptible(rb->pollq,
>>> -                                   iio_buffer_ready(indio_dev, rb, to_wait));
>>> +                            iio_buffer_ready(indio_dev, rb, to_wait, true));
>>>               if (ret)
>>>                       return ret;
>>>
>>> @@ -120,7 +149,7 @@ unsigned int iio_buffer_poll(struct file *filp,
>>>               return -ENODEV;
>>>
>>>       poll_wait(filp, &rb->pollq, wait);
>>> -     if (iio_buffer_ready(indio_dev, rb, rb->watermark))
>>> +     if (iio_buffer_ready(indio_dev, rb, rb->watermark, false))
>>>               return POLLIN | POLLRDNORM;
>>>       return 0;
>>>  }
>>> @@ -659,19 +688,16 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
>>>               }
>>>       }
>>>       /* Definitely possible for devices to support both of these. */
>>> -     if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) {
>>> -             if (!indio_dev->trig) {
>>> -                     printk(KERN_INFO "Buffer not started: no trigger\n");
>>> -                     ret = -EINVAL;
>>> -                     /* Can only occur on first buffer */
>>> -                     goto error_run_postdisable;
>>> -             }
>>> +     if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
>>>               indio_dev->currentmode = INDIO_BUFFER_TRIGGERED;
>>>       } else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
>>>               indio_dev->currentmode = INDIO_BUFFER_HARDWARE;
>>>       } else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
>>>               indio_dev->currentmode = INDIO_BUFFER_SOFTWARE;
>>>       } else { /* Should never be reached */
>>> +             /* Can only occur on first buffer */
>>> +             if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
>>> +                     pr_info("Buffer not started: no trigger\n");
>>>               ret = -EINVAL;
>>>               goto error_run_postdisable;
>>>       }
>>> @@ -823,12 +849,28 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>>>       }
>>>
>>>       buffer->watermark = val;
>>> +
>>> +     if (indio_dev->info->hwfifo_set_watermark)
>>> +             indio_dev->info->hwfifo_set_watermark(indio_dev, val);
>>>  out:
>>>       mutex_unlock(&indio_dev->mlock);
>>>
>>>       return ret ? ret : len;
>>>  }
>>>
>>> +static ssize_t iio_buffer_show_hwfifo_watermark(struct device *dev,
>>> +                                             struct device_attribute *attr,
>>> +                                             char *buf)
>>> +{
>>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +     int ret = -1;
>>> +
>>> +     if (indio_dev->info->hwfifo_get_watermark)
>>> +             ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
>> Any error code returned should be returned by this function to userspace.
>>
>> Also, ret = -1 would be EPERM, a possible error code.  Hence you need
>> to separate your handling of not supported from possible return values.
>>
>> if (indio_dev->info->hwfifo_get_watermark) {
>>         ret = indio_dev->info->hwfifo_get_watermark(indio_dev);
>>         if (ret < 0)
>>            return ret;
>>         return sprintf(buf, "%d\n", ret);
>> } else {
>>         return sprintf(buf, "-1\n");
>>
>> seems the simplest way to me.
> 
> If we make the entry visible only when we have the hw fifo supported
> then we can remove the else branch. What do you think?
exactly.
> 
>>> +
>>> +     return sprintf(buf, "%d\n", ret < -1 ? -1 : ret);
>>> +}
>>> +
>>>  static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
>>>                  iio_buffer_write_length);
>>>  static struct device_attribute dev_attr_length_ro = __ATTR(length,
>>> @@ -837,11 +879,14 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
>>>                  iio_buffer_show_enable, iio_buffer_store_enable);
>>>  static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
>>>                  iio_buffer_show_watermark, iio_buffer_store_watermark);
>>> +static DEVICE_ATTR(hwfifo_watermark, S_IRUGO, iio_buffer_show_hwfifo_watermark,
>>> +                NULL);
>>>
>>>  static struct attribute *iio_buffer_attrs[] = {
>>>       &dev_attr_length.attr,
>>>       &dev_attr_enable.attr,
>>>       &dev_attr_watermark.attr,
>>> +     &dev_attr_hwfifo_watermark.attr,
>>>  };
>>>
>>>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 80d8550..1b1cd7d 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -338,6 +338,29 @@ struct iio_dev;
>>>   *                   provide a custom of_xlate function that reads the
>>>   *                   *args* and returns the appropriate index in registered
>>>   *                   IIO channels array.
>>> + * @hwfifo_set_watermark: function pointer to set the current hardware fifo
>>> + *                   watermark level. It receives the desired watermark as a
>>> + *                   hint and the device driver may adjust it to take into
>>> + *                   account hardware limitations. Setting the watermark to a
>>> + *                   strictly positive value should enable the hardware fifo
>>> + *                   if not already enabled. When the hardware fifo is
>>> + *                   enabled and its level reaches the watermark level the
>>> + *                   device must flush the samples stored in the hardware
>>> + *                   fifo to the device buffer.
>> Hmm. This one is interesting.  I wonder if we have devices with minimum
>> watermark levels.  I know that devices with fixed levels exist (sca3000 IIRC)
>> but in that case if we are below the level we can fallback to the data ready
>> signal (watermark of 1 effectively).  If the device provides no notification
>> that data capture is occuring until we reach a particular level (say 50% full)
>> then its not entirely clear whether we should turn the buffer on when a
>> watermark below that is set.
> 
> Interesting, in that case I think we should expose
> hwfifo_min_watermark to let userspace know how it needs to set the
> watermark level to enable the hardware fifo. Then we can update the
> ABI to say that the driver will enable the fifo if the watermark level
> is greater than hwfifo_min_watermark.
That would work nicely.
> 
> Related to this, I think we should also add hwfifo_max_watermark to
> make it easier to userspace to pick a good value.
Again, as information only attributes, they will help and add no
real ABI burden
> 
> One more thing: right now I am overloading the hwfifo_get_watermark
> (and hwfifio_watermark sysfs entry with information about whether the
> fifo is active. Hence if hwfifo is not active we return 0, otherwise
> we return the current hwfifo level. Given that some hardware may have
> additional hwfifo limitations (say perhaps not all values in the min
> .. max range are supported), perhaps is better to add a separate
> function and sysfs entry for checking if the hwfifo is active or not.
> This way userspace can set watermark then read hwfifo_watermark to see
> what is the actual watermark value without needing to start the buffer
> for checking that value. What do you think?
Either that or for non standard cases, just add an _available attribute
listing what it can be set to. 

I'd like to get much tighter listing of what is possible on all the attributes
we expose.  Working very slowly towards that ;)
> 
>>> Setting the watermark to 0
>>> + *                   should disable the hardware fifo. The device driver must
>>> + *                   disable the hardware fifo when a trigger with different
>>> + *                   sampling semantic (then the hardware fifo) is set
>> (than the...
>>> + *                   (e.g. when setting an any-motion trigger to a device
>>> + *                   that has its hardware fifo sample based on the set
>>> + *                   sample frequency).
>>> + * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
>>> + *                   watermark level
>>> + * @hwfifo_flush:    function pointer to flush the samples stored in the
>>> + *                   hardware fifo to the device buffer. The driver should
>>> + *                   not flush more then count samples. The function must
>> more than count
> 
> Oops :)
> 
>>> + *                   return the number of samples flushed, 0 if no samples
>>> + *                   were flushed or a negative integer if no samples were
>>> + *                   flushed and there was an error.
>>>   **/
>>>  struct iio_info {
>>>       struct module                   *driver_module;
>>> @@ -399,6 +422,9 @@ struct iio_info {
>>>                                 unsigned *readval);
>>>       int (*of_xlate)(struct iio_dev *indio_dev,
>>>                       const struct of_phandle_args *iiospec);
>>> +     int (*hwfifo_set_watermark)(struct iio_dev *indio_dev, unsigned val);
>>> +     int (*hwfifo_get_watermark)(struct iio_dev *indio_dev);
>>> +     int (*hwfifo_flush)(struct iio_dev *indio_dev, unsigned count);
>>>  };
>>>
>>>  /**
>>>
>>
>> --
>> 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 v5 2/3] iio: add support for hardware fifo

[Jonathan Cameron]

On 18/03/15 16:47, Octavian Purdila wrote:
> On Wed, Mar 18, 2015 at 1:55 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>> On 03/04/2015 06:56 PM, Octavian Purdila wrote:
>>>
>>> [...]
>>>
>>>   Documentation/ABI/testing/sysfs-bus-iio | 25 ++++++++++++
>>>   drivers/iio/industrialio-buffer.c       | 69 +++++++++++++++++++++++++++------
>>>   include/linux/iio/iio.h                 | 26 +++++++++++++
>>>   3 files changed, 108 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 1283ca7..143ddf2d 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1264,3 +1264,28 @@ Description:
>>>                 allows the application to block on poll with a timeout and read
>>>                 the available samples after the timeout expires and thus have a
>>>                 maximum delay guarantee.
>>> +
>>> +What:          /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
>>> +KernelVersion: 3.21
>>> +Contact:       linux-iio@vger.kernel.org
>>> +Description:
>>> +              Read-only entry that contains a single integer specifying the
>>> +              current level for the hardware fifo watermark level. If this
>>> +              value is negative it means that the device does not support a
>>> +              hardware fifo. If this value is 0 it means that the hardware fifo
>>> +              is currently disabled.
>>> +              If this value is strictly positive it signals that the hardware
>>> +              fifo of the device is active and that samples are stored in an
>>> +              internal hardware buffer. When the level of the hardware fifo
>>> +              reaches the watermark level the device will flush its internal
>>> +              buffer to the device buffer. Because of this a trigger is not
>>> +              needed to use the device in buffer mode.
>>> +              The hardware watermark level is set by the driver based on the
>>> +              value set by the user in buffer/watermark but taking into account
>>> +              the limitations of the hardware (e.g. most hardware buffers are
>>> +              limited to 32-64 samples).
>>> +              Because the sematics of triggers and hardware fifo may be
>>
>>
>> semantics
> 
> Ack.
> 
>>
>>> +              different (e.g. the hadware fifo may record samples according to
>>> +              the sample rate while an any-motion trigger generates samples
>>> +              based on the set rate of change) setting a trigger may disable
>>> +              the hardware fifo.
>>
>>
>> I still don't understand why the hardware fifo level is something the userspace application needs to set. And how would the application decide which level it wants?
> 
> This entry is read-only, it is not set by the user.  The user sets
> buffer/watermark then the driver sets the hardware fifo watermark and
> that is visible here.
> 
> Please also see my last proposals in the response I've sent to Jonathan:
> 
> * add a hwfifo_watermark_min to let the application know what
> watermark level it needs to set so that hardware fifo can be enabled
> 
> * add a hwfifo_watermark_max entry - required by Android batch mode
> (see sensor_t.fifoMaxEventCount at [1]); it should be useful to
> compute a watermark value that avoids overflowing the hardware fifo
> 
> * add a hwfifo_active entry - makes it easy to know if the hardware
> fifo is active or not
> 
> [1] https://source.android.com/devices/sensors/batching.html
> 
>>
>>
>> [...]
>>
>>>   int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 80d8550..1b1cd7d 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -338,6 +338,29 @@ struct iio_dev;
>>>    *                    provide a custom of_xlate function that reads the
>>>    *                    *args* and returns the appropriate index in registered
>>>    *                    IIO channels array.
>>> + * @hwfifo_set_watermark: function pointer to set the current hardware fifo
>>> + *                     watermark level. It receives the desired watermark as a
>>> + *                     hint and the device driver may adjust it to take into
>>> + *                     account hardware limitations. Setting the watermark to a
>>> + *                     strictly positive value should enable the hardware fifo
>>> + *                     if not already enabled. When the hardware fifo is
>>> + *                     enabled and its level reaches the watermark level the
>>> + *                     device must flush the samples stored in the hardware
>>> + *                     fifo to the device buffer. Setting the watermark to 0
>>> + *                     should disable the hardware fifo. The device driver must
>>> + *                     disable the hardware fifo when a trigger with different
>>> + *                     sampling semantic (then the hardware fifo) is set
>>> + *                     (e.g. when setting an any-motion trigger to a device
>>> + *                     that has its hardware fifo sample based on the set
>>> + *                     sample frequency).
>>> + * @hwfifo_get_watermark: function pointer to obtain the current hardware fifo
>>> + *                     watermark level
>>> + * @hwfifo_flush:      function pointer to flush the samples stored in the
>>
>>
>> This might just be me, but I associate flushing a FIFO with discarding the data. Our previous discussions make a lot more sense now :)
>>
>>
> 
> I see :) I've used the terminology from the processor cache where
> flush is used to "save" the cache line to memory and invalidate to
> discard it.
Howabout avoiding future confusion and renaming as
hwfifo_flush_to_swbuffer or something that that?
> --
> 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
>