[PATCH v3 0/9] iio: add support for hardware fifo

[PATCH v3 0/9] iio: add support for hardware fifo

[Octavian Purdila]

Here is the 3rd version of the patch that adds support for hardware
buffering.

I though a bit more about the watermark trigger approach and I still
think it is the right approach because:

 * it allows the application to enable or disable the FIFO

 * it avoids potential race conditions during configuration of the FIFO

 * an interrupt usually maps with a trigger, and since the FIFO
   watermark generates an interrupt it is natural to have a trigger for it

 * it matches well with the current trigger design, where only one
   trigger/interrupt can be active for one device

 * data is written to the FIFO based on the sampling rate and not
   based on a specific trigger; for example, it is at best confusing
   to have the any-motion trigger active while the FIFO is active

After the discussion with Jonathan I have decided to add a
hwfifo_watermark parameter to allow the application to change both the
device buffer watermark and the hardware fifo watermark as we want to
avoid dictating policy from kernel. I also think that it is important
for debugging and for allowing the application to use the right
settings depending on its goal (latency, power, etc.).

Other small changes since v2:

 * add a parameter to flush for the maximum number of samples to flush

 * fix a few comments

 * use indio_dev->active_scan_mask instead of buffer->scan_mask in the
   flush function

 * constify bmc150_accel_interrupts

 * use an anonymouse struct instead of struct
   bmc150_accel_interrupt_info and move it together with the
   initialization code

 * rewrote the slope code refactoring so that we update the registers
   when we enable the trigger

 * fix a potential division by zero spotted by Harmut

 * dropped the bmc150_accel_event patch


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

Octavian Purdila (8):
  iio: buffer: refactor buffer attributes setup
  iio: add support for hardware fifo
  iio: bmc150: refactor slope duration and threshold update
  iio: bmc150: refactor interrupt enabling
  iio: bmc150: exit early if event / trigger state is not changed
  iio: bmc150: introduce bmc150_accel_interrupt
  iio: bmc150: introduce bmc150_accel_trigger
  iio: bmc150: add support for hardware fifo

 Documentation/ABI/testing/sysfs-bus-iio  |  40 ++
 drivers/iio/accel/bmc150-accel.c         | 752 +++++++++++++++++++------------
 drivers/iio/industrialio-buffer.c        | 225 +++++++--
 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                  |  18 +
 7 files changed, 735 insertions(+), 323 deletions(-)

-- 
1.9.1

[PATCH v3 1/9] iio: buffer: refactor buffer attributes setup

[Octavian Purdila]

Move all core (non-custom) buffer attributes to a vector to make it
easier to add more of them in the future.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/industrialio-buffer.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 7133314..c2d5440 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -761,6 +761,11 @@ static struct device_attribute dev_attr_length_ro = __ATTR(length,
 static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_enable, iio_buffer_store_enable);
 
+static struct attribute *iio_buffer_attrs[] = {
+	&dev_attr_length.attr,
+	&dev_attr_enable.attr,
+};
+
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
 	struct iio_dev_attr *p;
@@ -778,21 +783,23 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 			attrcount++;
 	}
 
-	buffer->buffer_group.name = "buffer";
-	buffer->buffer_group.attrs = kcalloc(attrcount + 3,
-			sizeof(*buffer->buffer_group.attrs), GFP_KERNEL);
-	if (!buffer->buffer_group.attrs)
+	attr = kcalloc(attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
+		       sizeof(struct attribute *), GFP_KERNEL);
+	if (!attr)
 		return -ENOMEM;
 
-	if (buffer->access->set_length)
-		buffer->buffer_group.attrs[0] = &dev_attr_length.attr;
-	else
-		buffer->buffer_group.attrs[0] = &dev_attr_length_ro.attr;
-	buffer->buffer_group.attrs[1] = &dev_attr_enable.attr;
+	memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
+	if (!buffer->access->set_length)
+		attr[0] = &dev_attr_length_ro.attr;
+
 	if (buffer->attrs)
-		memcpy(&buffer->buffer_group.attrs[2], buffer->attrs,
-			sizeof(*&buffer->buffer_group.attrs) * attrcount);
-	buffer->buffer_group.attrs[attrcount+2] = NULL;
+		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
+		       sizeof(struct attribute *) * attrcount);
+
+	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
+
+	buffer->buffer_group.name = "buffer";
+	buffer->buffer_group.attrs = attr;
 
 	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
 
-- 
1.9.1

Re: [PATCH v3 1/9] iio: buffer: refactor buffer attributes setup

[Jonathan Cameron]

On 31/01/15 00:00, Octavian Purdila wrote:
> Move all core (non-custom) buffer attributes to a vector to make it
> easier to add more of them in the future.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Applied to the togreg branch of iio.git - will get pushed out as
testing sometime around Sunday/Monday (back home).
> ---
>  drivers/iio/industrialio-buffer.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 7133314..c2d5440 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -761,6 +761,11 @@ static struct device_attribute dev_attr_length_ro = __ATTR(length,
>  static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_enable, iio_buffer_store_enable);
>  
> +static struct attribute *iio_buffer_attrs[] = {
> +	&dev_attr_length.attr,
> +	&dev_attr_enable.attr,
> +};
> +
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_attr *p;
> @@ -778,21 +783,23 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  			attrcount++;
>  	}
>  
> -	buffer->buffer_group.name = "buffer";
> -	buffer->buffer_group.attrs = kcalloc(attrcount + 3,
> -			sizeof(*buffer->buffer_group.attrs), GFP_KERNEL);
> -	if (!buffer->buffer_group.attrs)
> +	attr = kcalloc(attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
> +		       sizeof(struct attribute *), GFP_KERNEL);
> +	if (!attr)
>  		return -ENOMEM;
>  
> -	if (buffer->access->set_length)
> -		buffer->buffer_group.attrs[0] = &dev_attr_length.attr;
> -	else
> -		buffer->buffer_group.attrs[0] = &dev_attr_length_ro.attr;
> -	buffer->buffer_group.attrs[1] = &dev_attr_enable.attr;
> +	memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
> +	if (!buffer->access->set_length)
> +		attr[0] = &dev_attr_length_ro.attr;
> +
>  	if (buffer->attrs)
> -		memcpy(&buffer->buffer_group.attrs[2], buffer->attrs,
> -			sizeof(*&buffer->buffer_group.attrs) * attrcount);
> -	buffer->buffer_group.attrs[attrcount+2] = NULL;
> +		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
> +		       sizeof(struct attribute *) * attrcount);
> +
> +	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> +
> +	buffer->buffer_group.name = "buffer";
> +	buffer->buffer_group.attrs = attr;
>  
>  	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
>  
> 

[PATCH v3 2/9] 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  |  10 +++
 drivers/iio/industrialio-buffer.c        | 124 ++++++++++++++++++++++++++-----
 drivers/iio/kfifo_buf.c                  |  11 +--
 drivers/staging/iio/accel/sca3000_ring.c |   4 +-
 include/linux/iio/buffer.h               |   8 +-
 5 files changed, 127 insertions(+), 30 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index c03a140..2a3dc12 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1193,3 +1193,13 @@ Description:
 		This attribute is used to read the current speed value of the
 		user (which is the norm or magnitude of the velocity vector).
 		Units after application of scale are m/s.
+
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/low_watermark
+KernelVersion:	3.20
+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.
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index c2d5440..e008ce03 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -37,7 +37,7 @@ 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);
 }
@@ -53,6 +53,9 @@ 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_read;
+	size_t count = 0;
 	int ret;
 
 	if (!indio_dev->info)
@@ -61,26 +64,48 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 	if (!rb || !rb->access->read_first_n)
 		return -EINVAL;
 
-	do {
-		if (!iio_buffer_data_available(rb)) {
-			if (filp->f_flags & O_NONBLOCK)
-				return -EAGAIN;
+	/*
+	 * 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;
 
+	to_read = min_t(size_t, n / datum_size, rb->low_watermark);
+
+	do {
+		if (filp->f_flags & O_NONBLOCK) {
+			if (!iio_buffer_data_available(rb)) {
+				ret = -EAGAIN;
+				break;
+			}
+		} else {
 			ret = wait_event_interruptible(rb->pollq,
-					iio_buffer_data_available(rb) ||
-					indio_dev->info == NULL);
+			       iio_buffer_data_available(rb) >= to_read ||
+						       indio_dev->info == NULL);
 			if (ret)
 				return ret;
-			if (indio_dev->info == NULL)
-				return -ENODEV;
+			if (indio_dev->info == NULL) {
+				ret = -ENODEV;
+				break;
+			}
 		}
 
-		ret = rb->access->read_first_n(rb, n, buf);
-		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
-			ret = -EAGAIN;
-	 } while (ret == 0);
+		ret = rb->access->read_first_n(rb, n, buf + count);
+		if (ret < 0)
+			break;
 
-	return ret;
+		count += ret;
+		n -= ret;
+		to_read -= ret / datum_size;
+	 } while (to_read > 0);
+
+	if (count)
+		return count;
+	if (ret < 0)
+		return ret;
+
+	return -EAGAIN;
 }
 
 /**
@@ -96,9 +121,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_data_available(rb) >= rb->low_watermark)
 		return POLLIN | POLLRDNORM;
-	/* need a way of knowing if there may be enough data... */
 	return 0;
 }
 
@@ -123,6 +147,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->low_watermark = 1;
 }
 EXPORT_SYMBOL(iio_buffer_init);
 
@@ -418,7 +443,16 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 	}
 	mutex_unlock(&indio_dev->mlock);
 
-	return ret ? ret : len;
+	if (ret)
+		return ret;
+
+	if (buffer->length)
+		val = buffer->length;
+
+	if (val < buffer->low_watermark)
+		buffer->low_watermark = val;
+
+	return len;
 }
 
 static ssize_t iio_buffer_show_enable(struct device *dev,
@@ -472,6 +506,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 +789,59 @@ 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->low_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 > buffer->length)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+	if (iio_buffer_is_active(indio_dev->buffer)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	buffer->low_watermark = val;
+	ret = 0;
+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(low_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_low_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(&buffer->pollq);
+	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..1e65dea 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->low_watermark : 0;
 }
 
 /**
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index b65850a..4459e01 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.
+ * @low_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				low_watermark;
 };
 
 /**
-- 
1.9.1

Re: [PATCH v3 2/9] iio: add watermark logic to iio read and poll

[Jonathan Cameron]

On 31/01/15 00:00, 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>
One question on the ABI below... Otherwise looks good to me.

> ---
>  Documentation/ABI/testing/sysfs-bus-iio  |  10 +++
>  drivers/iio/industrialio-buffer.c        | 124 ++++++++++++++++++++++++++-----
>  drivers/iio/kfifo_buf.c                  |  11 +--
>  drivers/staging/iio/accel/sca3000_ring.c |   4 +-
>  include/linux/iio/buffer.h               |   8 +-
>  5 files changed, 127 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index c03a140..2a3dc12 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1193,3 +1193,13 @@ Description:
>  		This attribute is used to read the current speed value of the
>  		user (which is the norm or magnitude of the velocity vector).
>  		Units after application of scale are m/s.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/low_watermark
Just thinking about this again.  Why low?
> +KernelVersion:	3.20
> +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.
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index c2d5440..e008ce03 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -37,7 +37,7 @@ 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);
>  }
> @@ -53,6 +53,9 @@ 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_read;
> +	size_t count = 0;
>  	int ret;
>  
>  	if (!indio_dev->info)
> @@ -61,26 +64,48 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  	if (!rb || !rb->access->read_first_n)
>  		return -EINVAL;
>  
> -	do {
> -		if (!iio_buffer_data_available(rb)) {
> -			if (filp->f_flags & O_NONBLOCK)
> -				return -EAGAIN;
> +	/*
> +	 * 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;
>  
> +	to_read = min_t(size_t, n / datum_size, rb->low_watermark);
> +
> +	do {
> +		if (filp->f_flags & O_NONBLOCK) {
> +			if (!iio_buffer_data_available(rb)) {
> +				ret = -EAGAIN;
> +				break;
> +			}
> +		} else {
>  			ret = wait_event_interruptible(rb->pollq,
> -					iio_buffer_data_available(rb) ||
> -					indio_dev->info == NULL);
> +			       iio_buffer_data_available(rb) >= to_read ||
> +						       indio_dev->info == NULL);
>  			if (ret)
>  				return ret;
> -			if (indio_dev->info == NULL)
> -				return -ENODEV;
> +			if (indio_dev->info == NULL) {
> +				ret = -ENODEV;
> +				break;
> +			}
>  		}
>  
> -		ret = rb->access->read_first_n(rb, n, buf);
> -		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> -			ret = -EAGAIN;
> -	 } while (ret == 0);
> +		ret = rb->access->read_first_n(rb, n, buf + count);
> +		if (ret < 0)
> +			break;
>  
> -	return ret;
> +		count += ret;
> +		n -= ret;
> +		to_read -= ret / datum_size;
> +	 } while (to_read > 0);
> +
> +	if (count)
> +		return count;
> +	if (ret < 0)
> +		return ret;
> +
> +	return -EAGAIN;
>  }
>  
>  /**
> @@ -96,9 +121,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_data_available(rb) >= rb->low_watermark)
>  		return POLLIN | POLLRDNORM;
> -	/* need a way of knowing if there may be enough data... */
>  	return 0;
>  }
>  
> @@ -123,6 +147,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->low_watermark = 1;
>  }
>  EXPORT_SYMBOL(iio_buffer_init);
>  
> @@ -418,7 +443,16 @@ static ssize_t iio_buffer_write_length(struct device *dev,
>  	}
>  	mutex_unlock(&indio_dev->mlock);
>  
> -	return ret ? ret : len;
> +	if (ret)
> +		return ret;
> +
> +	if (buffer->length)
> +		val = buffer->length;
> +
> +	if (val < buffer->low_watermark)
> +		buffer->low_watermark = val;
> +
> +	return len;
>  }
>  
>  static ssize_t iio_buffer_show_enable(struct device *dev,
> @@ -472,6 +506,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 +789,59 @@ 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->low_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 > buffer->length)
> +		return -EINVAL;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (iio_buffer_is_active(indio_dev->buffer)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	buffer->low_watermark = val;
> +	ret = 0;
> +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(low_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_low_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(&buffer->pollq);
> +	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..1e65dea 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->low_watermark : 0;
>  }
>  
>  /**
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index b65850a..4459e01 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.
> + * @low_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				low_watermark;
>  };
>  
>  /**
> 

Re: [PATCH v3 2/9] iio: add watermark logic to iio read and poll

[Octavian Purdila]

On Wed, Feb 4, 2015 at 8:49 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 31/01/15 00:00, 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>
> One question on the ABI below... Otherwise looks good to me.
>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio  |  10 +++
>>  drivers/iio/industrialio-buffer.c        | 124 ++++++++++++++++++++++++++-----
>>  drivers/iio/kfifo_buf.c                  |  11 +--
>>  drivers/staging/iio/accel/sca3000_ring.c |   4 +-
>>  include/linux/iio/buffer.h               |   8 +-
>>  5 files changed, 127 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index c03a140..2a3dc12 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -1193,3 +1193,13 @@ Description:
>>               This attribute is used to read the current speed value of the
>>               user (which is the norm or magnitude of the velocity vector).
>>               Units after application of scale are m/s.
>> +
>> +What:                /sys/bus/iio/devices/iio:deviceX/buffer/low_watermark
> Just thinking about this again.  Why low?

I just kept the original name. A simple watermark sounds better to me,
I will change it in the next version.

[PATCH v3 3/9] 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 allowing the
drivers to register operations for flushing the hadware fifo and
setting the watermark level.

A driver implementing hardware fifo support must also provide a
watermark trigger which must contain "watermark" in its name.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 30 +++++++++++++
 drivers/iio/industrialio-buffer.c       | 78 ++++++++++++++++++++++++++++++---
 include/linux/iio/iio.h                 | 18 ++++++++
 3 files changed, 121 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 2a3dc12..34b4f99 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1203,3 +1203,33 @@ Description:
 		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.
+
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_length
+KernelVersion:	3.20
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A single positive integer specifying the maximum number of
+		samples that the hardware fifo has. If the device does not
+		support hardware fifo this is zero.
+		When a device supports hardware fifo it will expose a trigger
+		with the name that contains "watermark"
+		(e.g. i2c-BMC150A:00-watermark-dev0).
+		To use the hardware fifo the user must set an appropriate value
+		in the buffer/length, buffer/low_watermark and
+		buffer/hwfifo_watermark entries and select the watermark
+		trigger. At that point the hardware fifo will be enabled and the
+		samples will be collected in the hardware buffer. When the
+		number of samples in the hardware fifo reaches the watermark
+		level the watermark trigger is issued and data is flushed to the
+		devices buffer.
+		A hardware buffer flush will also be triggered when reading from
+		the device buffer and there is not enough data available.
+
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
+KernelVersion:	3.20
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A single positive integer specifying the watermark level for the
+		hardware fifo. When the number of samples in the hardware fifo
+		reaches the hardware fifo watermark the device watermark
+		trigger will be asserted.
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e008ce03..624a9dd 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -37,9 +37,17 @@ static bool iio_buffer_is_active(struct iio_buffer *buf)
 	return !list_empty(&buf->buffer_list);
 }
 
-static size_t iio_buffer_data_available(struct iio_buffer *buf)
+static bool iio_check_and_flush_data(struct iio_dev *indio_dev,
+				     struct iio_buffer *buf, size_t required)
 {
-	return buf->access->data_available(buf);
+	size_t avail = buf->access->data_available(buf);
+
+	if (avail < required && indio_dev->hwfifo) {
+		indio_dev->hwfifo->flush(indio_dev, required);
+		avail = buf->access->data_available(buf);
+	}
+
+	return avail >= required;
 }
 
 /**
@@ -75,13 +83,13 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 
 	do {
 		if (filp->f_flags & O_NONBLOCK) {
-			if (!iio_buffer_data_available(rb)) {
+			if (!iio_check_and_flush_data(indio_dev, rb, 1)) {
 				ret = -EAGAIN;
 				break;
 			}
 		} else {
 			ret = wait_event_interruptible(rb->pollq,
-			       iio_buffer_data_available(rb) >= to_read ||
+			     iio_check_and_flush_data(indio_dev, rb, to_read) ||
 						       indio_dev->info == NULL);
 			if (ret)
 				return ret;
@@ -121,7 +129,7 @@ unsigned int iio_buffer_poll(struct file *filp,
 		return -ENODEV;
 
 	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_data_available(rb) >= rb->low_watermark)
+	if (iio_check_and_flush_data(indio_dev, rb, rb->low_watermark))
 		return POLLIN | POLLRDNORM;
 	return 0;
 }
@@ -829,6 +837,60 @@ out:
 	return ret ? ret : len;
 }
 
+ssize_t iio_buffer_hwfifo_read_length(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_hwfifo *hwfifo = indio_dev->hwfifo;
+
+	return sprintf(buf, "%u\n", hwfifo ? hwfifo->length : 0);
+}
+
+static ssize_t iio_buffer_hwfifo_show_watermark(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_hwfifo *hwfifo = indio_dev->hwfifo;
+
+	return sprintf(buf, "%u\n", hwfifo ? hwfifo->watermark : 0);
+}
+
+static ssize_t iio_buffer_hwfifo_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_hwfifo *hwfifo = indio_dev->hwfifo;
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	if (!hwfifo || val > hwfifo->length)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+	if (iio_buffer_is_active(indio_dev->buffer)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = indio_dev->hwfifo->set_watermark(indio_dev, val);
+	if (ret)
+		goto out;
+
+	hwfifo->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,
@@ -837,11 +899,17 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_enable, iio_buffer_store_enable);
 static DEVICE_ATTR(low_watermark, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_watermark, iio_buffer_store_watermark);
+static DEVICE_ATTR(hwfifo_length, S_IRUGO, iio_buffer_hwfifo_read_length, NULL);
+static DEVICE_ATTR(hwfifo_watermark, S_IRUGO | S_IWUSR,
+		   iio_buffer_hwfifo_show_watermark,
+		   iio_buffer_hwfifo_store_watermark);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_low_watermark.attr,
+	&dev_attr_hwfifo_length.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 51f1643..9cde2f9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -419,6 +419,22 @@ struct iio_buffer_setup_ops {
 };
 
 /**
+ * struct iio_hwfifo - hardware fifo ops and info
+ *
+ * @length:		[DRIVER] the hardware fifo length
+ * @watermark:		[INTERN] the current hardware fifo watermark level
+ * @set_watermark:	[DRIVER] function to set the watermark level
+ * @flush:		[DRIVER] copies data from the hardware buffer to the
+ *			device buffer
+ */
+struct iio_hwfifo {
+	int length;
+	int watermark;
+	int (*set_watermark)(struct iio_dev *, unsigned int);
+	int (*flush)(struct iio_dev *, int samples);
+};
+
+/**
  * struct iio_dev - industrial I/O device
  * @id:			[INTERN] used to identify device internally
  * @modes:		[DRIVER] operating modes supported by device
@@ -453,6 +469,7 @@ struct iio_buffer_setup_ops {
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
  * @flags:		[INTERN] file ops related flags including busy flag.
+ * @hwfifo:		[INTERN] hardware fifo ops and info
  * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
  * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
  */
@@ -493,6 +510,7 @@ struct iio_dev {
 	int				groupcounter;
 
 	unsigned long			flags;
+	struct iio_hwfifo		*hwfifo;
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry			*debugfs_dentry;
 	unsigned			cached_reg_addr;
-- 
1.9.1

Re: [PATCH v3 3/9] iio: add support for hardware fifo

[Jonathan Cameron]

On 31/01/15 00:00, 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 allowing the
> drivers to register operations for flushing the hadware fifo and
> setting the watermark level.
> 
> A driver implementing hardware fifo support must also provide a
> watermark trigger which must contain "watermark" in its name.
As commented below, this is the one bit I don't like.  I 'might' be
convinced but it is an abuse of the current meaning of triggers and
will just confuse matters.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 30 +++++++++++++
>  drivers/iio/industrialio-buffer.c       | 78 ++++++++++++++++++++++++++++++---
>  include/linux/iio/iio.h                 | 18 ++++++++
>  3 files changed, 121 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 2a3dc12..34b4f99 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1203,3 +1203,33 @@ Description:
>  		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.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_length
> +KernelVersion:	3.20
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		A single positive integer specifying the maximum number of
> +		samples that the hardware fifo has. If the device does not
> +		support hardware fifo this is zero.
> +		When a device supports hardware fifo it will expose a trigger
> +		with the name that contains "watermark"
> +		(e.g. i2c-BMC150A:00-watermark-dev0).
I'm still unsure on the necessity of having the trigger, as mentioned elsewhere.
I think we can avoid it and that we'd be better off without.

> +		To use the hardware fifo the user must set an appropriate value
> +		in the buffer/length, buffer/low_watermark and
> +		buffer/hwfifo_watermark entries and select the watermark
> +		trigger. At that point the hardware fifo will be enabled and the
> +		samples will be collected in the hardware buffer. When the
> +		number of samples in the hardware fifo reaches the watermark
> +		level the watermark trigger is issued and data is flushed to the
> +		devices buffer.
> +		A hardware buffer flush will also be triggered when reading from
> +		the device buffer and there is not enough data available.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
> +KernelVersion:	3.20
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		A single positive integer specifying the watermark level for the
> +		hardware fifo. When the number of samples in the hardware fifo
> +		reaches the hardware fifo watermark the device watermark
> +		trigger will be asserted.
I'm still worried about the complexity of the interaction of the software
watermark and the hardware one if we explicitly expose them both and allow
them both to be written.

One thought would be to have an explicit attribute such as
hwfifo_watermark_auto that would default to true and would control whether the
hwfifo_watermark is writable or not (it would always be readable to give the
current actual value).

Would that give us the best of both worlds?  Fine grained control but only
if the user really needs it and complexity (of user interface) reduced if
they don't?

Just a side point. Note that at some point we need to figure out how to
do watermark setting when we have multiple clients for the data stream.
I guess it will be a shortest watermark always wins approach.

This is actually similar to the problem of multiple consumers wanting different
sampling frequencies.  I have thought about a temporal equivalent of the
demux but that would only work well with integer multiples of frequency.
It's certainly possible in that case to end up a consumer getting fed data at
a rate only loosely matched to what it asked for.

Note with this hardware fifo, we can do the same as with sampling frequency
and park this question for now.

hmm. Sorry for heading off into the blue yonder.  Fun morning waiting for a
late flight in Dublin with limited sleep :(

At some point we should start bouncing ideas for where we are heading anyway.
I'm sure I'm not the only one who has ideas on this! (as evidenced by
your work amongst others)
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e008ce03..624a9dd 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -37,9 +37,17 @@ static bool iio_buffer_is_active(struct iio_buffer *buf)
>  	return !list_empty(&buf->buffer_list);
>  }
>  
> -static size_t iio_buffer_data_available(struct iio_buffer *buf)
> +static bool iio_check_and_flush_data(struct iio_dev *indio_dev,
> +				     struct iio_buffer *buf, size_t required)
>  {
> -	return buf->access->data_available(buf);
> +	size_t avail = buf->access->data_available(buf);
> +
> +	if (avail < required && indio_dev->hwfifo) {
> +		indio_dev->hwfifo->flush(indio_dev, required);
> +		avail = buf->access->data_available(buf);
> +	}
> +
> +	return avail >= required;
>  }
>  
>  /**
> @@ -75,13 +83,13 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  
>  	do {
>  		if (filp->f_flags & O_NONBLOCK) {
> -			if (!iio_buffer_data_available(rb)) {
> +			if (!iio_check_and_flush_data(indio_dev, rb, 1)) {
>  				ret = -EAGAIN;
>  				break;
>  			}
>  		} else {
>  			ret = wait_event_interruptible(rb->pollq,
> -			       iio_buffer_data_available(rb) >= to_read ||
> +			     iio_check_and_flush_data(indio_dev, rb, to_read) ||
>  						       indio_dev->info == NULL);
>  			if (ret)
>  				return ret;
> @@ -121,7 +129,7 @@ unsigned int iio_buffer_poll(struct file *filp,
>  		return -ENODEV;
>  
>  	poll_wait(filp, &rb->pollq, wait);
> -	if (iio_buffer_data_available(rb) >= rb->low_watermark)
> +	if (iio_check_and_flush_data(indio_dev, rb, rb->low_watermark))
>  		return POLLIN | POLLRDNORM;
>  	return 0;
>  }
> @@ -829,6 +837,60 @@ out:
>  	return ret ? ret : len;
>  }
>  
> +ssize_t iio_buffer_hwfifo_read_length(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_hwfifo *hwfifo = indio_dev->hwfifo;
> +
> +	return sprintf(buf, "%u\n", hwfifo ? hwfifo->length : 0);
> +}
> +
> +static ssize_t iio_buffer_hwfifo_show_watermark(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_hwfifo *hwfifo = indio_dev->hwfifo;
> +
> +	return sprintf(buf, "%u\n", hwfifo ? hwfifo->watermark : 0);
> +}
> +
> +static ssize_t iio_buffer_hwfifo_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_hwfifo *hwfifo = indio_dev->hwfifo;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (!hwfifo || val > hwfifo->length)
> +		return -EINVAL;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (iio_buffer_is_active(indio_dev->buffer)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = indio_dev->hwfifo->set_watermark(indio_dev, val);
> +	if (ret)
> +		goto out;
> +
> +	hwfifo->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,
> @@ -837,11 +899,17 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_enable, iio_buffer_store_enable);
>  static DEVICE_ATTR(low_watermark, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_watermark, iio_buffer_store_watermark);
> +static DEVICE_ATTR(hwfifo_length, S_IRUGO, iio_buffer_hwfifo_read_length, NULL);
> +static DEVICE_ATTR(hwfifo_watermark, S_IRUGO | S_IWUSR,
> +		   iio_buffer_hwfifo_show_watermark,
> +		   iio_buffer_hwfifo_store_watermark);
>  
>  static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_length.attr,
>  	&dev_attr_enable.attr,
>  	&dev_attr_low_watermark.attr,
> +	&dev_attr_hwfifo_length.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 51f1643..9cde2f9 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -419,6 +419,22 @@ struct iio_buffer_setup_ops {
>  };
>  
>  /**
> + * struct iio_hwfifo - hardware fifo ops and info
> + *
> + * @length:		[DRIVER] the hardware fifo length
> + * @watermark:		[INTERN] the current hardware fifo watermark level
> + * @set_watermark:	[DRIVER] function to set the watermark level
> + * @flush:		[DRIVER] copies data from the hardware buffer to the
> + *			device buffer
> + */
> +struct iio_hwfifo {
> +	int length;
> +	int watermark;
> +	int (*set_watermark)(struct iio_dev *, unsigned int);
> +	int (*flush)(struct iio_dev *, int samples);
> +};
> +
> +/**
>   * struct iio_dev - industrial I/O device
>   * @id:			[INTERN] used to identify device internally
>   * @modes:		[DRIVER] operating modes supported by device
> @@ -453,6 +469,7 @@ struct iio_buffer_setup_ops {
>   * @groups:		[INTERN] attribute groups
>   * @groupcounter:	[INTERN] index of next attribute group
>   * @flags:		[INTERN] file ops related flags including busy flag.
> + * @hwfifo:		[INTERN] hardware fifo ops and info
>   * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
>   * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
>   */
> @@ -493,6 +510,7 @@ struct iio_dev {
>  	int				groupcounter;
>  
>  	unsigned long			flags;
> +	struct iio_hwfifo		*hwfifo;
>  #if defined(CONFIG_DEBUG_FS)
>  	struct dentry			*debugfs_dentry;
>  	unsigned			cached_reg_addr;
> 

[PATCH v3 4/9] iio: bmc150: refactor slope duration and threshold update

[Octavian Purdila]

Move the slope duration and threshold update in a separate function to
reduce code duplicate between chip init and motion interrupt setup.

Also move the slope update code from the interrupt setup function to
the trigger set state function so that we can later refactor the
interrupt code.

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

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 066d0c0..2b6b80d 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -269,6 +269,37 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
 	return -EINVAL;
 }
 
+static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
+{
+	int ret, val;
+
+	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
+					data->slope_thres);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_int_6\n");
+		return ret;
+	}
+
+	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_int_5\n");
+		return ret;
+	}
+
+	val = (ret & ~BMC150_ACCEL_SLOPE_DUR_MASK) | data->slope_dur;
+	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
+					val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error write reg_int_5\n");
+		return ret;
+	}
+
+	dev_dbg(&data->client->dev, "%s: %x %x\n", __func__, data->slope_thres,
+		data->slope_dur);
+
+	return ret;
+}
+
 static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 {
 	int ret;
@@ -307,32 +338,12 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 
 	data->range = BMC150_ACCEL_DEF_RANGE_4G;
 
-	/* Set default slope duration */
-	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_int_5\n");
-		return ret;
-	}
-	data->slope_dur |= BMC150_ACCEL_DEF_SLOPE_DURATION;
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_5,
-					data->slope_dur);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_5\n");
-		return ret;
-	}
-	dev_dbg(&data->client->dev, "slope_dur %x\n", data->slope_dur);
-
-	/* Set default slope thresholds */
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_6,
-					BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_6\n");
-		return ret;
-	}
+	/* Set default slope duration and thresholds */
 	data->slope_thres = BMC150_ACCEL_DEF_SLOPE_THRESHOLD;
-	dev_dbg(&data->client->dev, "slope_thres %x\n", data->slope_thres);
+	data->slope_dur = BMC150_ACCEL_DEF_SLOPE_DURATION;
+	ret = bmc150_accel_update_slope(data);
+	if (ret < 0)
+		return ret;
 
 	/* Set default as latched interrupts */
 	ret = i2c_smbus_write_byte_data(data->client,
@@ -375,24 +386,6 @@ static int bmc150_accel_setup_any_motion_interrupt(
 	}
 
 	if (status) {
-		/* Set slope duration (no of samples) */
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_5,
-						data->slope_dur);
-		if (ret < 0) {
-			dev_err(&data->client->dev, "Error write reg_int_5\n");
-			return ret;
-		}
-
-		/* Set slope thresholds */
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_6,
-						data->slope_thres);
-		if (ret < 0) {
-			dev_err(&data->client->dev, "Error write reg_int_6\n");
-			return ret;
-		}
-
 		/*
 		 * New data interrupt is always non-latched,
 		 * which will have higher priority, so no need
@@ -732,7 +725,7 @@ static int bmc150_accel_read_event(struct iio_dev *indio_dev,
 		*val = data->slope_thres;
 		break;
 	case IIO_EV_INFO_PERIOD:
-		*val = data->slope_dur & BMC150_ACCEL_SLOPE_DUR_MASK;
+		*val = data->slope_dur;
 		break;
 	default:
 		return -EINVAL;
@@ -755,11 +748,10 @@ static int bmc150_accel_write_event(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
-		data->slope_thres = val;
+		data->slope_thres = val & 0xFF;
 		break;
 	case IIO_EV_INFO_PERIOD:
-		data->slope_dur &= ~BMC150_ACCEL_SLOPE_DUR_MASK;
-		data->slope_dur |= val & BMC150_ACCEL_SLOPE_DUR_MASK;
+		data->slope_dur = val & BMC150_ACCEL_SLOPE_DUR_MASK;
 		break;
 	default:
 		return -EINVAL;
@@ -1056,10 +1048,15 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 		mutex_unlock(&data->mutex);
 		return ret;
 	}
-	if (data->motion_trig == trig)
-		ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
-	else
+
+	if (data->motion_trig == trig) {
+		ret = bmc150_accel_update_slope(data);
+		if (!ret)
+			ret = bmc150_accel_setup_any_motion_interrupt(data,
+								      state);
+	} else {
 		ret = bmc150_accel_setup_new_data_interrupt(data, state);
+	}
 	if (ret < 0) {
 		bmc150_accel_set_power_state(data, false);
 		mutex_unlock(&data->mutex);
-- 
1.9.1

Re: [PATCH v3 4/9] iio: bmc150: refactor slope duration and threshold update

[Srinivas Pandruvada]

On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote: 
> Move the slope duration and threshold update in a separate function to
> reduce code duplicate between chip init and motion interrupt setup.
> 
> Also move the slope update code from the interrupt setup function to
> the trigger set state function so that we can later refactor the
> interrupt code.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
> ---
>  drivers/iio/accel/bmc150-accel.c | 97 +++++++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 066d0c0..2b6b80d 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -269,6 +269,37 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
>  	return -EINVAL;
>  }
>  
> +static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
> +{
> +	int ret, val;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
> +					data->slope_thres);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_int_6\n");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_int_5\n");
> +		return ret;
> +	}
> +
> +	val = (ret & ~BMC150_ACCEL_SLOPE_DUR_MASK) | data->slope_dur;
> +	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
> +					val);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error write reg_int_5\n");
> +		return ret;
> +	}
> +
> +	dev_dbg(&data->client->dev, "%s: %x %x\n", __func__, data->slope_thres,
> +		data->slope_dur);
> +
> +	return ret;
> +}
> +
>  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  {
>  	int ret;
> @@ -307,32 +338,12 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  
>  	data->range = BMC150_ACCEL_DEF_RANGE_4G;
>  
> -	/* Set default slope duration */
> -	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error reading reg_int_5\n");
> -		return ret;
> -	}
> -	data->slope_dur |= BMC150_ACCEL_DEF_SLOPE_DURATION;
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_5,
> -					data->slope_dur);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_5\n");
> -		return ret;
> -	}
> -	dev_dbg(&data->client->dev, "slope_dur %x\n", data->slope_dur);
> -
> -	/* Set default slope thresholds */
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_6,
> -					BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_6\n");
> -		return ret;
> -	}
> +	/* Set default slope duration and thresholds */
>  	data->slope_thres = BMC150_ACCEL_DEF_SLOPE_THRESHOLD;
> -	dev_dbg(&data->client->dev, "slope_thres %x\n", data->slope_thres);
> +	data->slope_dur = BMC150_ACCEL_DEF_SLOPE_DURATION;
> +	ret = bmc150_accel_update_slope(data);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* Set default as latched interrupts */
>  	ret = i2c_smbus_write_byte_data(data->client,
> @@ -375,24 +386,6 @@ static int bmc150_accel_setup_any_motion_interrupt(
>  	}
>  
>  	if (status) {
> -		/* Set slope duration (no of samples) */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_5,
> -						data->slope_dur);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev, "Error write reg_int_5\n");
> -			return ret;
> -		}
> -
> -		/* Set slope thresholds */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_6,
> -						data->slope_thres);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev, "Error write reg_int_6\n");
> -			return ret;
> -		}
> -
>  		/*
>  		 * New data interrupt is always non-latched,
>  		 * which will have higher priority, so no need
> @@ -732,7 +725,7 @@ static int bmc150_accel_read_event(struct iio_dev *indio_dev,
>  		*val = data->slope_thres;
>  		break;
>  	case IIO_EV_INFO_PERIOD:
> -		*val = data->slope_dur & BMC150_ACCEL_SLOPE_DUR_MASK;
> +		*val = data->slope_dur;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -755,11 +748,10 @@ static int bmc150_accel_write_event(struct iio_dev *indio_dev,
>  
>  	switch (info) {
>  	case IIO_EV_INFO_VALUE:
> -		data->slope_thres = val;
> +		data->slope_thres = val & 0xFF;
>  		break;
>  	case IIO_EV_INFO_PERIOD:
> -		data->slope_dur &= ~BMC150_ACCEL_SLOPE_DUR_MASK;
> -		data->slope_dur |= val & BMC150_ACCEL_SLOPE_DUR_MASK;
> +		data->slope_dur = val & BMC150_ACCEL_SLOPE_DUR_MASK;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1056,10 +1048,15 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
> -	if (data->motion_trig == trig)
> -		ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
> -	else
> +
> +	if (data->motion_trig == trig) {
> +		ret = bmc150_accel_update_slope(data);
> +		if (!ret)
> +			ret = bmc150_accel_setup_any_motion_interrupt(data,
> +								      state);
> +	} else {
>  		ret = bmc150_accel_setup_new_data_interrupt(data, state);
> +	}
>  	if (ret < 0) {
>  		bmc150_accel_set_power_state(data, false);
>  		mutex_unlock(&data->mutex);

Re: [PATCH v3 4/9] iio: bmc150: refactor slope duration and threshold update

[Jonathan Cameron]

On 05/02/15 17:02, Srinivas Pandruvada wrote:
> On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote: 
>> Move the slope duration and threshold update in a separate function to
>> reduce code duplicate between chip init and motion interrupt setup.
>>
>> Also move the slope update code from the interrupt setup function to
>> the trigger set state function so that we can later refactor the
>> interrupt code.
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
My only comment here is that I'd have preferred this being in a separate precursor
series as it's not really connected to the hardware buffer stuff!
Keep your series focused and if there is refactoring to be done, please
do it first.

Anyhow, applied this one to the togreg branch of iio.git

Will push out as testing sometime in next few days...

Thanks,

Jonathan
>> ---
>>  drivers/iio/accel/bmc150-accel.c | 97 +++++++++++++++++++---------------------
>>  1 file changed, 47 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
>> index 066d0c0..2b6b80d 100644
>> --- a/drivers/iio/accel/bmc150-accel.c
>> +++ b/drivers/iio/accel/bmc150-accel.c
>> @@ -269,6 +269,37 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
>>  	return -EINVAL;
>>  }
>>  
>> +static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
>> +{
>> +	int ret, val;
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
>> +					data->slope_thres);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error writing reg_int_6\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error reading reg_int_5\n");
>> +		return ret;
>> +	}
>> +
>> +	val = (ret & ~BMC150_ACCEL_SLOPE_DUR_MASK) | data->slope_dur;
>> +	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
>> +					val);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error write reg_int_5\n");
>> +		return ret;
>> +	}
>> +
>> +	dev_dbg(&data->client->dev, "%s: %x %x\n", __func__, data->slope_thres,
>> +		data->slope_dur);
>> +
>> +	return ret;
>> +}
>> +
>>  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>>  {
>>  	int ret;
>> @@ -307,32 +338,12 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>>  
>>  	data->range = BMC150_ACCEL_DEF_RANGE_4G;
>>  
>> -	/* Set default slope duration */
>> -	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error reading reg_int_5\n");
>> -		return ret;
>> -	}
>> -	data->slope_dur |= BMC150_ACCEL_DEF_SLOPE_DURATION;
>> -	ret = i2c_smbus_write_byte_data(data->client,
>> -					BMC150_ACCEL_REG_INT_5,
>> -					data->slope_dur);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error writing reg_int_5\n");
>> -		return ret;
>> -	}
>> -	dev_dbg(&data->client->dev, "slope_dur %x\n", data->slope_dur);
>> -
>> -	/* Set default slope thresholds */
>> -	ret = i2c_smbus_write_byte_data(data->client,
>> -					BMC150_ACCEL_REG_INT_6,
>> -					BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error writing reg_int_6\n");
>> -		return ret;
>> -	}
>> +	/* Set default slope duration and thresholds */
>>  	data->slope_thres = BMC150_ACCEL_DEF_SLOPE_THRESHOLD;
>> -	dev_dbg(&data->client->dev, "slope_thres %x\n", data->slope_thres);
>> +	data->slope_dur = BMC150_ACCEL_DEF_SLOPE_DURATION;
>> +	ret = bmc150_accel_update_slope(data);
>> +	if (ret < 0)
>> +		return ret;
>>  
>>  	/* Set default as latched interrupts */
>>  	ret = i2c_smbus_write_byte_data(data->client,
>> @@ -375,24 +386,6 @@ static int bmc150_accel_setup_any_motion_interrupt(
>>  	}
>>  
>>  	if (status) {
>> -		/* Set slope duration (no of samples) */
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -						BMC150_ACCEL_REG_INT_5,
>> -						data->slope_dur);
>> -		if (ret < 0) {
>> -			dev_err(&data->client->dev, "Error write reg_int_5\n");
>> -			return ret;
>> -		}
>> -
>> -		/* Set slope thresholds */
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -						BMC150_ACCEL_REG_INT_6,
>> -						data->slope_thres);
>> -		if (ret < 0) {
>> -			dev_err(&data->client->dev, "Error write reg_int_6\n");
>> -			return ret;
>> -		}
>> -
>>  		/*
>>  		 * New data interrupt is always non-latched,
>>  		 * which will have higher priority, so no need
>> @@ -732,7 +725,7 @@ static int bmc150_accel_read_event(struct iio_dev *indio_dev,
>>  		*val = data->slope_thres;
>>  		break;
>>  	case IIO_EV_INFO_PERIOD:
>> -		*val = data->slope_dur & BMC150_ACCEL_SLOPE_DUR_MASK;
>> +		*val = data->slope_dur;
>>  		break;
>>  	default:
>>  		return -EINVAL;
>> @@ -755,11 +748,10 @@ static int bmc150_accel_write_event(struct iio_dev *indio_dev,
>>  
>>  	switch (info) {
>>  	case IIO_EV_INFO_VALUE:
>> -		data->slope_thres = val;
>> +		data->slope_thres = val & 0xFF;
>>  		break;
>>  	case IIO_EV_INFO_PERIOD:
>> -		data->slope_dur &= ~BMC150_ACCEL_SLOPE_DUR_MASK;
>> -		data->slope_dur |= val & BMC150_ACCEL_SLOPE_DUR_MASK;
>> +		data->slope_dur = val & BMC150_ACCEL_SLOPE_DUR_MASK;
>>  		break;
>>  	default:
>>  		return -EINVAL;
>> @@ -1056,10 +1048,15 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>  		mutex_unlock(&data->mutex);
>>  		return ret;
>>  	}
>> -	if (data->motion_trig == trig)
>> -		ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
>> -	else
>> +
>> +	if (data->motion_trig == trig) {
>> +		ret = bmc150_accel_update_slope(data);
>> +		if (!ret)
>> +			ret = bmc150_accel_setup_any_motion_interrupt(data,
>> +								      state);
>> +	} else {
>>  		ret = bmc150_accel_setup_new_data_interrupt(data, state);
>> +	}
>>  	if (ret < 0) {
>>  		bmc150_accel_set_power_state(data, false);
>>  		mutex_unlock(&data->mutex);
> 
> 
> --
> 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 v3 4/9] iio: bmc150: refactor slope duration and threshold update

[Octavian Purdila]

On Sun, Feb 8, 2015 at 12:37 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 05/02/15 17:02, Srinivas Pandruvada wrote:
>> On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote:
>>> Move the slope duration and threshold update in a separate function to
>>> reduce code duplicate between chip init and motion interrupt setup.
>>>
>>> Also move the slope update code from the interrupt setup function to
>>> the trigger set state function so that we can later refactor the
>>> interrupt code.
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> My only comment here is that I'd have preferred this being in a separate precursor
> series as it's not really connected to the hardware buffer stuff!
> Keep your series focused and if there is refactoring to be done, please
> do it first.
>

I will keep that in mind for the future and sorry for the trouble.

[PATCH v3 5/9] iio: bmc150: refactor interrupt enabling

[Octavian Purdila]

This patch combines the any motion and new data interrupts function
into a single, generic, interrupt enable function. On top of this, we
can later refactor triggers to make it easier to add new triggers.

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

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 2b6b80d..0873925 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -359,137 +359,6 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 	return 0;
 }
 
-static int bmc150_accel_setup_any_motion_interrupt(
-					struct bmc150_accel_data *data,
-					bool status)
-{
-	int ret;
-
-	/* Enable/Disable INT1 mapping */
-	ret = i2c_smbus_read_byte_data(data->client,
-				       BMC150_ACCEL_REG_INT_MAP_0);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_int_map_0\n");
-		return ret;
-	}
-	if (status)
-		ret |= BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
-	else
-		ret &= ~BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
-
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_MAP_0,
-					ret);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_map_0\n");
-		return ret;
-	}
-
-	if (status) {
-		/*
-		 * New data interrupt is always non-latched,
-		 * which will have higher priority, so no need
-		 * to set latched mode, we will be flooded anyway with INTR
-		 */
-		if (!data->dready_trigger_on) {
-			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 < 0) {
-				dev_err(&data->client->dev,
-					"Error writing reg_int_rst_latch\n");
-				return ret;
-			}
-		}
-
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_EN_0,
-						BMC150_ACCEL_INT_EN_BIT_SLP_X |
-						BMC150_ACCEL_INT_EN_BIT_SLP_Y |
-						BMC150_ACCEL_INT_EN_BIT_SLP_Z);
-	} else
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_EN_0,
-						0);
-
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_en_0\n");
-		return ret;
-	}
-
-	return 0;
-}
-
-static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
-					   bool status)
-{
-	int ret;
-
-	/* Enable/Disable INT1 mapping */
-	ret = i2c_smbus_read_byte_data(data->client,
-				       BMC150_ACCEL_REG_INT_MAP_1);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_int_map_1\n");
-		return ret;
-	}
-	if (status)
-		ret |= BMC150_ACCEL_INT_MAP_1_BIT_DATA;
-	else
-		ret &= ~BMC150_ACCEL_INT_MAP_1_BIT_DATA;
-
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_MAP_1,
-					ret);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_map_1\n");
-		return ret;
-	}
-
-	if (status) {
-		/*
-		 * Set non latched mode interrupt and clear any latched
-		 * interrupt
-		 */
-		ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_RST_LATCH,
-					BMC150_ACCEL_INT_MODE_NON_LATCH_INT |
-					BMC150_ACCEL_INT_MODE_LATCH_RESET);
-		if (ret < 0) {
-			dev_err(&data->client->dev,
-				"Error writing reg_int_rst_latch\n");
-			return ret;
-		}
-
-		ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_EN_1,
-					BMC150_ACCEL_INT_EN_BIT_DATA_EN);
-
-	} else {
-		/* Restore default interrupt mode */
-		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 < 0) {
-			dev_err(&data->client->dev,
-				"Error writing reg_int_rst_latch\n");
-			return ret;
-		}
-
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_EN_1,
-						0);
-	}
-
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_en_1\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int bmc150_accel_get_bw(struct bmc150_accel_data *data, int *val,
 			       int *val2)
 {
@@ -547,6 +416,105 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 }
 #endif
 
+static const struct bmc150_accel_interrupt_info {
+	u8 map_reg;
+	u8 map_bitmask;
+	u8 en_reg;
+	u8 en_bitmask;
+} bmc150_accel_interrupts[] = {
+	{ /* data ready interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
+		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN,
+	},
+	{  /* motion interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_0,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_SLOPE,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_0,
+		.en_bitmask =  BMC150_ACCEL_INT_EN_BIT_SLP_X |
+			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
+			BMC150_ACCEL_INT_EN_BIT_SLP_Z
+	},
+};
+
+static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
+				const struct bmc150_accel_interrupt_info *info,
+				      bool state)
+{
+	int ret;
+
+	/*
+	 * We will expect the enable and disable to do operation in
+	 * in reverse order. This will happen here anyway as our
+	 * resume operation uses sync mode runtime pm calls, the
+	 * suspend operation will be delayed by autosuspend delay
+	 * So the disable operation will still happen in reverse of
+	 * enable operation. When runtime pm is disabled the mode
+	 * is always on so sequence doesn't matter
+	 */
+	ret = bmc150_accel_set_power_state(data, state);
+	if (ret < 0)
+		return ret;
+
+	/* map the interrupt to the appropriate pins */
+	ret = i2c_smbus_read_byte_data(data->client, info->map_reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_int_map\n");
+		goto out_fix_power_state;
+	}
+	if (state)
+		ret |= info->map_bitmask;
+	else
+		ret &= ~info->map_bitmask;
+
+	ret = i2c_smbus_write_byte_data(data->client, info->map_reg,
+					ret);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_int_map\n");
+		goto out_fix_power_state;
+	}
+
+	/* enable/disable the interrupt */
+	ret = i2c_smbus_read_byte_data(data->client, info->en_reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_int_en\n");
+		goto out_fix_power_state;
+	}
+
+	if (state)
+		ret |= info->en_bitmask;
+	else
+		ret &= ~info->en_bitmask;
+
+	ret = i2c_smbus_write_byte_data(data->client, info->en_reg, ret);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_int_en\n");
+		goto out_fix_power_state;
+	}
+
+	return 0;
+
+out_fix_power_state:
+	bmc150_accel_set_power_state(data, false);
+	return ret;
+}
+
+static int bmc150_accel_setup_any_motion_interrupt(
+					struct bmc150_accel_data *data,
+					bool status)
+{
+	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
+					  status);
+}
+
+static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
+						 bool status)
+{
+	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
+					  status);
+}
+
 static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
 {
 	int ret, i;
@@ -791,25 +759,8 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 		return 0;
 	}
 
-	/*
-	 * We will expect the enable and disable to do operation in
-	 * in reverse order. This will happen here anyway as our
-	 * resume operation uses sync mode runtime pm calls, the
-	 * suspend operation will be delayed by autosuspend delay
-	 * So the disable operation will still happen in reverse of
-	 * enable operation. When runtime pm is disabled the mode
-	 * is always on so sequence doesn't matter
-	 */
-
-	ret = bmc150_accel_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
-
 	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
 	if (ret < 0) {
-		bmc150_accel_set_power_state(data, false);
 		mutex_unlock(&data->mutex);
 		return ret;
 	}
@@ -1039,16 +990,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 		return 0;
 	}
 
-	/*
-	 * Refer to comment in bmc150_accel_write_event_config for
-	 * enable/disable operation order
-	 */
-	ret = bmc150_accel_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
-
 	if (data->motion_trig == trig) {
 		ret = bmc150_accel_update_slope(data);
 		if (!ret)
@@ -1058,7 +999,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 		ret = bmc150_accel_setup_new_data_interrupt(data, state);
 	}
 	if (ret < 0) {
-		bmc150_accel_set_power_state(data, false);
 		mutex_unlock(&data->mutex);
 		return ret;
 	}
@@ -1244,6 +1184,20 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		if (ret)
 			return ret;
 
+		/*
+		 * Set latched mode interrupt. While certain interrupts are
+		 * non-latched regardless of this settings (e.g. new data) we
+		 * want to use latch mode when we can to prevent interrupt
+		 * flooding.
+		 */
+		ret = i2c_smbus_write_byte_data(data->client,
+						BMC150_ACCEL_REG_INT_RST_LATCH,
+					     BMC150_ACCEL_INT_MODE_LATCH_RESET);
+		if (ret < 0) {
+			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
+			return ret;
+		}
+
 		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
 							   "%s-dev%d",
 							   indio_dev->name,
-- 
1.9.1

Re: [PATCH v3 5/9] iio: bmc150: refactor interrupt enabling

[Srinivas Pandruvada]

On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote: 
> This patch combines the any motion and new data interrupts function
> into a single, generic, interrupt enable function. On top of this, we
> can later refactor triggers to make it easier to add new triggers.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
> ---
>  drivers/iio/accel/bmc150-accel.c | 272 ++++++++++++++++-----------------------
>  1 file changed, 113 insertions(+), 159 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 2b6b80d..0873925 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -359,137 +359,6 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  	return 0;
>  }
>  
> -static int bmc150_accel_setup_any_motion_interrupt(
> -					struct bmc150_accel_data *data,
> -					bool status)
> -{
> -	int ret;
> -
> -	/* Enable/Disable INT1 mapping */
> -	ret = i2c_smbus_read_byte_data(data->client,
> -				       BMC150_ACCEL_REG_INT_MAP_0);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error reading reg_int_map_0\n");
> -		return ret;
> -	}
> -	if (status)
> -		ret |= BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
> -	else
> -		ret &= ~BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
> -
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_MAP_0,
> -					ret);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_map_0\n");
> -		return ret;
> -	}
> -
> -	if (status) {
> -		/*
> -		 * New data interrupt is always non-latched,
> -		 * which will have higher priority, so no need
> -		 * to set latched mode, we will be flooded anyway with INTR
> -		 */
> -		if (!data->dready_trigger_on) {
> -			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 < 0) {
> -				dev_err(&data->client->dev,
> -					"Error writing reg_int_rst_latch\n");
> -				return ret;
> -			}
> -		}
> -
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_EN_0,
> -						BMC150_ACCEL_INT_EN_BIT_SLP_X |
> -						BMC150_ACCEL_INT_EN_BIT_SLP_Y |
> -						BMC150_ACCEL_INT_EN_BIT_SLP_Z);
> -	} else
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_EN_0,
> -						0);
> -
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_en_0\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
> -					   bool status)
> -{
> -	int ret;
> -
> -	/* Enable/Disable INT1 mapping */
> -	ret = i2c_smbus_read_byte_data(data->client,
> -				       BMC150_ACCEL_REG_INT_MAP_1);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error reading reg_int_map_1\n");
> -		return ret;
> -	}
> -	if (status)
> -		ret |= BMC150_ACCEL_INT_MAP_1_BIT_DATA;
> -	else
> -		ret &= ~BMC150_ACCEL_INT_MAP_1_BIT_DATA;
> -
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_MAP_1,
> -					ret);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_map_1\n");
> -		return ret;
> -	}
> -
> -	if (status) {
> -		/*
> -		 * Set non latched mode interrupt and clear any latched
> -		 * interrupt
> -		 */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_RST_LATCH,
> -					BMC150_ACCEL_INT_MODE_NON_LATCH_INT |
> -					BMC150_ACCEL_INT_MODE_LATCH_RESET);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev,
> -				"Error writing reg_int_rst_latch\n");
> -			return ret;
> -		}
> -
> -		ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_EN_1,
> -					BMC150_ACCEL_INT_EN_BIT_DATA_EN);
> -
> -	} else {
> -		/* Restore default interrupt mode */
> -		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 < 0) {
> -			dev_err(&data->client->dev,
> -				"Error writing reg_int_rst_latch\n");
> -			return ret;
> -		}
> -
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_EN_1,
> -						0);
> -	}
> -
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_en_1\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int bmc150_accel_get_bw(struct bmc150_accel_data *data, int *val,
>  			       int *val2)
>  {
> @@ -547,6 +416,105 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>  }
>  #endif
>  
> +static const struct bmc150_accel_interrupt_info {
> +	u8 map_reg;
> +	u8 map_bitmask;
> +	u8 en_reg;
> +	u8 en_bitmask;
> +} bmc150_accel_interrupts[] = {
> +	{ /* data ready interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN,
> +	},
> +	{  /* motion interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_0,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_SLOPE,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_0,
> +		.en_bitmask =  BMC150_ACCEL_INT_EN_BIT_SLP_X |
> +			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
> +			BMC150_ACCEL_INT_EN_BIT_SLP_Z
> +	},
> +};
> +
> +static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
> +				const struct bmc150_accel_interrupt_info *info,
> +				      bool state)
> +{
> +	int ret;
> +
> +	/*
> +	 * We will expect the enable and disable to do operation in
> +	 * in reverse order. This will happen here anyway as our
> +	 * resume operation uses sync mode runtime pm calls, the
> +	 * suspend operation will be delayed by autosuspend delay
> +	 * So the disable operation will still happen in reverse of
> +	 * enable operation. When runtime pm is disabled the mode
> +	 * is always on so sequence doesn't matter
> +	 */
> +	ret = bmc150_accel_set_power_state(data, state);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* map the interrupt to the appropriate pins */
> +	ret = i2c_smbus_read_byte_data(data->client, info->map_reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_int_map\n");
> +		goto out_fix_power_state;
> +	}
> +	if (state)
> +		ret |= info->map_bitmask;
> +	else
> +		ret &= ~info->map_bitmask;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, info->map_reg,
> +					ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_int_map\n");
> +		goto out_fix_power_state;
> +	}
> +
> +	/* enable/disable the interrupt */
> +	ret = i2c_smbus_read_byte_data(data->client, info->en_reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_int_en\n");
> +		goto out_fix_power_state;
> +	}
> +
> +	if (state)
> +		ret |= info->en_bitmask;
> +	else
> +		ret &= ~info->en_bitmask;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, info->en_reg, ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_int_en\n");
> +		goto out_fix_power_state;
> +	}
> +
> +	return 0;
> +
> +out_fix_power_state:
> +	bmc150_accel_set_power_state(data, false);
> +	return ret;
> +}
> +
> +static int bmc150_accel_setup_any_motion_interrupt(
> +					struct bmc150_accel_data *data,
> +					bool status)
> +{
> +	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
> +					  status);
> +}
> +
> +static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
> +						 bool status)
> +{
> +	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
> +					  status);
> +}
> +
>  static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
>  {
>  	int ret, i;
> @@ -791,25 +759,8 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
>  		return 0;
>  	}
>  
> -	/*
> -	 * We will expect the enable and disable to do operation in
> -	 * in reverse order. This will happen here anyway as our
> -	 * resume operation uses sync mode runtime pm calls, the
> -	 * suspend operation will be delayed by autosuspend delay
> -	 * So the disable operation will still happen in reverse of
> -	 * enable operation. When runtime pm is disabled the mode
> -	 * is always on so sequence doesn't matter
> -	 */
> -
> -	ret = bmc150_accel_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> -
>  	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
>  	if (ret < 0) {
> -		bmc150_accel_set_power_state(data, false);
>  		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
> @@ -1039,16 +990,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  		return 0;
>  	}
>  
> -	/*
> -	 * Refer to comment in bmc150_accel_write_event_config for
> -	 * enable/disable operation order
> -	 */
> -	ret = bmc150_accel_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> -
>  	if (data->motion_trig == trig) {
>  		ret = bmc150_accel_update_slope(data);
>  		if (!ret)
> @@ -1058,7 +999,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  		ret = bmc150_accel_setup_new_data_interrupt(data, state);
>  	}
>  	if (ret < 0) {
> -		bmc150_accel_set_power_state(data, false);
>  		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
> @@ -1244,6 +1184,20 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  		if (ret)
>  			return ret;
>  
> +		/*
> +		 * Set latched mode interrupt. While certain interrupts are
> +		 * non-latched regardless of this settings (e.g. new data) we
> +		 * want to use latch mode when we can to prevent interrupt
> +		 * flooding.
> +		 */
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						BMC150_ACCEL_REG_INT_RST_LATCH,
> +					     BMC150_ACCEL_INT_MODE_LATCH_RESET);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> +			return ret;
> +		}
> +
>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
>  							   "%s-dev%d",
>  							   indio_dev->name,

Re: [PATCH v3 5/9] iio: bmc150: refactor interrupt enabling

[Jonathan Cameron]

On 05/02/15 17:06, Srinivas Pandruvada wrote:
> On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote: 
>> This patch combines the any motion and new data interrupts function
>> into a single, generic, interrupt enable function. On top of this, we
>> can later refactor triggers to make it easier to add new triggers.
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
Worthwhile little refactor on it's own.  Again would ideally have been
in a precursor series, but anyhow, applied to the togreg branch of iio.git.

Thanks,

>> ---
>>  drivers/iio/accel/bmc150-accel.c | 272 ++++++++++++++++-----------------------
>>  1 file changed, 113 insertions(+), 159 deletions(-)
>>
>> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
>> index 2b6b80d..0873925 100644
>> --- a/drivers/iio/accel/bmc150-accel.c
>> +++ b/drivers/iio/accel/bmc150-accel.c
>> @@ -359,137 +359,6 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>>  	return 0;
>>  }
>>  
>> -static int bmc150_accel_setup_any_motion_interrupt(
>> -					struct bmc150_accel_data *data,
>> -					bool status)
>> -{
>> -	int ret;
>> -
>> -	/* Enable/Disable INT1 mapping */
>> -	ret = i2c_smbus_read_byte_data(data->client,
>> -				       BMC150_ACCEL_REG_INT_MAP_0);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error reading reg_int_map_0\n");
>> -		return ret;
>> -	}
>> -	if (status)
>> -		ret |= BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
>> -	else
>> -		ret &= ~BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
>> -
>> -	ret = i2c_smbus_write_byte_data(data->client,
>> -					BMC150_ACCEL_REG_INT_MAP_0,
>> -					ret);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error writing reg_int_map_0\n");
>> -		return ret;
>> -	}
>> -
>> -	if (status) {
>> -		/*
>> -		 * New data interrupt is always non-latched,
>> -		 * which will have higher priority, so no need
>> -		 * to set latched mode, we will be flooded anyway with INTR
>> -		 */
>> -		if (!data->dready_trigger_on) {
>> -			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 < 0) {
>> -				dev_err(&data->client->dev,
>> -					"Error writing reg_int_rst_latch\n");
>> -				return ret;
>> -			}
>> -		}
>> -
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -						BMC150_ACCEL_REG_INT_EN_0,
>> -						BMC150_ACCEL_INT_EN_BIT_SLP_X |
>> -						BMC150_ACCEL_INT_EN_BIT_SLP_Y |
>> -						BMC150_ACCEL_INT_EN_BIT_SLP_Z);
>> -	} else
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -						BMC150_ACCEL_REG_INT_EN_0,
>> -						0);
>> -
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error writing reg_int_en_0\n");
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
>> -					   bool status)
>> -{
>> -	int ret;
>> -
>> -	/* Enable/Disable INT1 mapping */
>> -	ret = i2c_smbus_read_byte_data(data->client,
>> -				       BMC150_ACCEL_REG_INT_MAP_1);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error reading reg_int_map_1\n");
>> -		return ret;
>> -	}
>> -	if (status)
>> -		ret |= BMC150_ACCEL_INT_MAP_1_BIT_DATA;
>> -	else
>> -		ret &= ~BMC150_ACCEL_INT_MAP_1_BIT_DATA;
>> -
>> -	ret = i2c_smbus_write_byte_data(data->client,
>> -					BMC150_ACCEL_REG_INT_MAP_1,
>> -					ret);
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error writing reg_int_map_1\n");
>> -		return ret;
>> -	}
>> -
>> -	if (status) {
>> -		/*
>> -		 * Set non latched mode interrupt and clear any latched
>> -		 * interrupt
>> -		 */
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -					BMC150_ACCEL_REG_INT_RST_LATCH,
>> -					BMC150_ACCEL_INT_MODE_NON_LATCH_INT |
>> -					BMC150_ACCEL_INT_MODE_LATCH_RESET);
>> -		if (ret < 0) {
>> -			dev_err(&data->client->dev,
>> -				"Error writing reg_int_rst_latch\n");
>> -			return ret;
>> -		}
>> -
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -					BMC150_ACCEL_REG_INT_EN_1,
>> -					BMC150_ACCEL_INT_EN_BIT_DATA_EN);
>> -
>> -	} else {
>> -		/* Restore default interrupt mode */
>> -		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 < 0) {
>> -			dev_err(&data->client->dev,
>> -				"Error writing reg_int_rst_latch\n");
>> -			return ret;
>> -		}
>> -
>> -		ret = i2c_smbus_write_byte_data(data->client,
>> -						BMC150_ACCEL_REG_INT_EN_1,
>> -						0);
>> -	}
>> -
>> -	if (ret < 0) {
>> -		dev_err(&data->client->dev, "Error writing reg_int_en_1\n");
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  static int bmc150_accel_get_bw(struct bmc150_accel_data *data, int *val,
>>  			       int *val2)
>>  {
>> @@ -547,6 +416,105 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>>  }
>>  #endif
>>  
>> +static const struct bmc150_accel_interrupt_info {
>> +	u8 map_reg;
>> +	u8 map_bitmask;
>> +	u8 en_reg;
>> +	u8 en_bitmask;
>> +} bmc150_accel_interrupts[] = {
>> +	{ /* data ready interrupt */
>> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
>> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
>> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
>> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN,
>> +	},
>> +	{  /* motion interrupt */
>> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_0,
>> +		.map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_SLOPE,
>> +		.en_reg = BMC150_ACCEL_REG_INT_EN_0,
>> +		.en_bitmask =  BMC150_ACCEL_INT_EN_BIT_SLP_X |
>> +			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
>> +			BMC150_ACCEL_INT_EN_BIT_SLP_Z
>> +	},
>> +};
>> +
>> +static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
>> +				const struct bmc150_accel_interrupt_info *info,
>> +				      bool state)
>> +{
>> +	int ret;
>> +
>> +	/*
>> +	 * We will expect the enable and disable to do operation in
>> +	 * in reverse order. This will happen here anyway as our
>> +	 * resume operation uses sync mode runtime pm calls, the
>> +	 * suspend operation will be delayed by autosuspend delay
>> +	 * So the disable operation will still happen in reverse of
>> +	 * enable operation. When runtime pm is disabled the mode
>> +	 * is always on so sequence doesn't matter
>> +	 */
>> +	ret = bmc150_accel_set_power_state(data, state);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* map the interrupt to the appropriate pins */
>> +	ret = i2c_smbus_read_byte_data(data->client, info->map_reg);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error reading reg_int_map\n");
>> +		goto out_fix_power_state;
>> +	}
>> +	if (state)
>> +		ret |= info->map_bitmask;
>> +	else
>> +		ret &= ~info->map_bitmask;
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, info->map_reg,
>> +					ret);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error writing reg_int_map\n");
>> +		goto out_fix_power_state;
>> +	}
>> +
>> +	/* enable/disable the interrupt */
>> +	ret = i2c_smbus_read_byte_data(data->client, info->en_reg);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error reading reg_int_en\n");
>> +		goto out_fix_power_state;
>> +	}
>> +
>> +	if (state)
>> +		ret |= info->en_bitmask;
>> +	else
>> +		ret &= ~info->en_bitmask;
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, info->en_reg, ret);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Error writing reg_int_en\n");
>> +		goto out_fix_power_state;
>> +	}
>> +
>> +	return 0;
>> +
>> +out_fix_power_state:
>> +	bmc150_accel_set_power_state(data, false);
>> +	return ret;
>> +}
>> +
>> +static int bmc150_accel_setup_any_motion_interrupt(
>> +					struct bmc150_accel_data *data,
>> +					bool status)
>> +{
>> +	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
>> +					  status);
>> +}
>> +
>> +static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
>> +						 bool status)
>> +{
>> +	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
>> +					  status);
>> +}
>> +
>>  static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
>>  {
>>  	int ret, i;
>> @@ -791,25 +759,8 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
>>  		return 0;
>>  	}
>>  
>> -	/*
>> -	 * We will expect the enable and disable to do operation in
>> -	 * in reverse order. This will happen here anyway as our
>> -	 * resume operation uses sync mode runtime pm calls, the
>> -	 * suspend operation will be delayed by autosuspend delay
>> -	 * So the disable operation will still happen in reverse of
>> -	 * enable operation. When runtime pm is disabled the mode
>> -	 * is always on so sequence doesn't matter
>> -	 */
>> -
>> -	ret = bmc150_accel_set_power_state(data, state);
>> -	if (ret < 0) {
>> -		mutex_unlock(&data->mutex);
>> -		return ret;
>> -	}
>> -
>>  	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
>>  	if (ret < 0) {
>> -		bmc150_accel_set_power_state(data, false);
>>  		mutex_unlock(&data->mutex);
>>  		return ret;
>>  	}
>> @@ -1039,16 +990,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>  		return 0;
>>  	}
>>  
>> -	/*
>> -	 * Refer to comment in bmc150_accel_write_event_config for
>> -	 * enable/disable operation order
>> -	 */
>> -	ret = bmc150_accel_set_power_state(data, state);
>> -	if (ret < 0) {
>> -		mutex_unlock(&data->mutex);
>> -		return ret;
>> -	}
>> -
>>  	if (data->motion_trig == trig) {
>>  		ret = bmc150_accel_update_slope(data);
>>  		if (!ret)
>> @@ -1058,7 +999,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>  		ret = bmc150_accel_setup_new_data_interrupt(data, state);
>>  	}
>>  	if (ret < 0) {
>> -		bmc150_accel_set_power_state(data, false);
>>  		mutex_unlock(&data->mutex);
>>  		return ret;
>>  	}
>> @@ -1244,6 +1184,20 @@ static int bmc150_accel_probe(struct i2c_client *client,
>>  		if (ret)
>>  			return ret;
>>  
>> +		/*
>> +		 * Set latched mode interrupt. While certain interrupts are
>> +		 * non-latched regardless of this settings (e.g. new data) we
>> +		 * want to use latch mode when we can to prevent interrupt
>> +		 * flooding.
>> +		 */
>> +		ret = i2c_smbus_write_byte_data(data->client,
>> +						BMC150_ACCEL_REG_INT_RST_LATCH,
>> +					     BMC150_ACCEL_INT_MODE_LATCH_RESET);
>> +		if (ret < 0) {
>> +			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
>> +			return ret;
>> +		}
>> +
>>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
>>  							   "%s-dev%d",
>>  							   indio_dev->name,
> 
> 
> --
> 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
> 

[PATCH v3 6/9] iio: bmc150: exit early if event / trigger state is not changed

[Octavian Purdila]

Previous of this patch the check was only done if we enabled the event
and it was already enabled. We can do the same if the event is
disabled and we want to disable it.

The patch also adds the same check on the trigger code.

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

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 0873925..f040f40 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -748,7 +748,7 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret;
 
-	if (state && data->ev_enable_state)
+	if (state == data->ev_enable_state)
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -984,6 +984,18 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 
 	mutex_lock(&data->mutex);
 
+	if (data->motion_trig == trig) {
+		if (data->motion_trigger_on == state) {
+			mutex_unlock(&data->mutex);
+			return 0;
+		}
+	} else {
+		if (data->dready_trigger_on == state) {
+			mutex_unlock(&data->mutex);
+			return 0;
+		}
+	}
+
 	if (!state && data->ev_enable_state && data->motion_trigger_on) {
 		data->motion_trigger_on = false;
 		mutex_unlock(&data->mutex);
-- 
1.9.1

Re: [PATCH v3 6/9] iio: bmc150: exit early if event / trigger state is not changed

[Srinivas Pandruvada]

On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote: 
> Previous of this patch the check was only done if we enabled the event
> and it was already enabled. We can do the same if the event is
> disabled and we want to disable it.
> 
> The patch also adds the same check on the trigger code.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
> ---
>  drivers/iio/accel/bmc150-accel.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 0873925..f040f40 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -748,7 +748,7 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	if (state && data->ev_enable_state)
> +	if (state == data->ev_enable_state)
>  		return 0;
>  
>  	mutex_lock(&data->mutex);
> @@ -984,6 +984,18 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  
>  	mutex_lock(&data->mutex);
>  
> +	if (data->motion_trig == trig) {
> +		if (data->motion_trigger_on == state) {
> +			mutex_unlock(&data->mutex);
> +			return 0;
> +		}
> +	} else {
> +		if (data->dready_trigger_on == state) {
> +			mutex_unlock(&data->mutex);
> +			return 0;
> +		}
> +	}
> +
>  	if (!state && data->ev_enable_state && data->motion_trigger_on) {
>  		data->motion_trigger_on = false;
>  		mutex_unlock(&data->mutex);

Re: [PATCH v3 6/9] iio: bmc150: exit early if event / trigger state is not changed

[Jonathan Cameron]

On 05/02/15 17:09, Srinivas Pandruvada wrote:
> On Sat, 2015-01-31 at 02:00 +0200, Octavian Purdila wrote: 
>> Previous of this patch the check was only done if we enabled the event
>> and it was already enabled. We can do the same if the event is
>> disabled and we want to disable it.
>>
>> The patch also adds the same check on the trigger code.
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
Another good standalone improvement.

Applied to the togreg branch of iio.git

Thanks,

Jonathan
>> ---
>>  drivers/iio/accel/bmc150-accel.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
>> index 0873925..f040f40 100644
>> --- a/drivers/iio/accel/bmc150-accel.c
>> +++ b/drivers/iio/accel/bmc150-accel.c
>> @@ -748,7 +748,7 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
>>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>>  	int ret;
>>  
>> -	if (state && data->ev_enable_state)
>> +	if (state == data->ev_enable_state)
>>  		return 0;
>>  
>>  	mutex_lock(&data->mutex);
>> @@ -984,6 +984,18 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>  
>>  	mutex_lock(&data->mutex);
>>  
>> +	if (data->motion_trig == trig) {
>> +		if (data->motion_trigger_on == state) {
>> +			mutex_unlock(&data->mutex);
>> +			return 0;
>> +		}
>> +	} else {
>> +		if (data->dready_trigger_on == state) {
>> +			mutex_unlock(&data->mutex);
>> +			return 0;
>> +		}
>> +	}
>> +
>>  	if (!state && data->ev_enable_state && data->motion_trigger_on) {
>>  		data->motion_trigger_on = false;
>>  		mutex_unlock(&data->mutex);
> 
> 
> --
> 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
> 

[PATCH v3 7/9] iio: bmc150: introduce bmc150_accel_interrupt

[Octavian Purdila]

Since both triggers and events can share an interrupt, add a data
structure that tracks the users of an interrupt so that it enables or
disables it only for the first users and respectively last user.

This will allows us to easily add more events or triggers.

The patch also adds an interrupt enabled counter, so that we can
easily know if we need to put the device in normal mode when the
resume callback is issued.

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

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index f040f40..b23ad1b 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -147,10 +147,19 @@ struct bmc150_accel_chip_info {
 	const struct bmc150_scale_info scale_table[4];
 };
 
+struct bmc150_accel_interrupt {
+	const struct bmc150_accel_interrupt_info *info;
+	atomic_t users;
+};
+
+#define BMC150_ACCEL_INTERRUPTS		2
+
 struct bmc150_accel_data {
 	struct i2c_client *client;
+	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
+	atomic_t active_intr;
 	struct mutex mutex;
 	s16 buffer[8];
 	u8 bw_bits;
@@ -421,7 +430,7 @@ static const struct bmc150_accel_interrupt_info {
 	u8 map_bitmask;
 	u8 en_reg;
 	u8 en_bitmask;
-} bmc150_accel_interrupts[] = {
+} bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
 	{ /* data ready interrupt */
 		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
 		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
@@ -438,12 +447,30 @@ static const struct bmc150_accel_interrupt_info {
 	},
 };
 
+static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
+					  struct bmc150_accel_data *data)
+{
+	int i;
+
+	for (i = 0; i < BMC150_ACCEL_INTERRUPTS; i++)
+		data->interrupts[i].info = &bmc150_accel_interrupts[i];
+}
+
 static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
-				const struct bmc150_accel_interrupt_info *info,
+				      struct bmc150_accel_interrupt *intr,
 				      bool state)
 {
+	const struct bmc150_accel_interrupt_info *info = intr->info;
 	int ret;
 
+	if (state) {
+		if (atomic_inc_return(&intr->users) > 1)
+			return 0;
+	} else {
+		if (atomic_dec_return(&intr->users) > 0)
+			return 0;
+	}
+
 	/*
 	 * We will expect the enable and disable to do operation in
 	 * in reverse order. This will happen here anyway as our
@@ -493,6 +520,11 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
 		goto out_fix_power_state;
 	}
 
+	if (state)
+		atomic_inc(&data->active_intr);
+	else
+		atomic_dec(&data->active_intr);
+
 	return 0;
 
 out_fix_power_state:
@@ -500,20 +532,6 @@ out_fix_power_state:
 	return ret;
 }
 
-static int bmc150_accel_setup_any_motion_interrupt(
-					struct bmc150_accel_data *data,
-					bool status)
-{
-	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
-					  status);
-}
-
-static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
-						 bool status)
-{
-	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
-					  status);
-}
 
 static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
 {
@@ -753,13 +771,7 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 
 	mutex_lock(&data->mutex);
 
-	if (!state && data->motion_trigger_on) {
-		data->ev_enable_state = 0;
-		mutex_unlock(&data->mutex);
-		return 0;
-	}
-
-	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
+	ret = bmc150_accel_set_interrupt(data, &data->interrupts[1], state);
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
 		return ret;
@@ -996,19 +1008,15 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 		}
 	}
 
-	if (!state && data->ev_enable_state && data->motion_trigger_on) {
-		data->motion_trigger_on = false;
-		mutex_unlock(&data->mutex);
-		return 0;
-	}
-
 	if (data->motion_trig == trig) {
 		ret = bmc150_accel_update_slope(data);
 		if (!ret)
-			ret = bmc150_accel_setup_any_motion_interrupt(data,
-								      state);
+			ret = bmc150_accel_set_interrupt(data,
+							 &data->interrupts[1],
+							 state);
 	} else {
-		ret = bmc150_accel_setup_new_data_interrupt(data, state);
+		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
+						 state);
 	}
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
@@ -1210,6 +1218,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
 			return ret;
 		}
 
+		bmc150_accel_interrupts_setup(indio_dev, data);
+
 		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
 							   "%s-dev%d",
 							   indio_dev->name,
@@ -1325,8 +1335,7 @@ static int bmc150_accel_resume(struct device *dev)
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 
 	mutex_lock(&data->mutex);
-	if (data->dready_trigger_on || data->motion_trigger_on ||
-							data->ev_enable_state)
+	if (atomic_read(&data->active_intr))
 		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
 	mutex_unlock(&data->mutex);
 
-- 
1.9.1

Re: [PATCH v3 7/9] iio: bmc150: introduce bmc150_accel_interrupt

[Jonathan Cameron]

On 31/01/15 00:00, Octavian Purdila wrote:
> Since both triggers and events can share an interrupt, add a data
> structure that tracks the users of an interrupt so that it enables or
> disables it only for the first users and respectively last user.
> 
> This will allows us to easily add more events or triggers.
> 
> The patch also adds an interrupt enabled counter, so that we can
> easily know if we need to put the device in normal mode when the
> resume callback is issued.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
I'm still undecided on whether this should be using an irq_chip to
avoid reinventing the wheel.  The few drivers that doing things
similarly to what you have here all predate at least my knowledge
of that infrastructure.

Unless others (e.g. Srinivas have stron opinions on this),
lets go with what you have here for now, but very much keep in mind
that we might want to refactor it in future. Otherwise, I'll keep you
working on this patch set for another few months!
(In my mind this isn't the important bit at all!)

Having said that I'd like an Ack from Srinivas if possible.

Jonathan
> ---
>  drivers/iio/accel/bmc150-accel.c | 77 ++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index f040f40..b23ad1b 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -147,10 +147,19 @@ struct bmc150_accel_chip_info {
>  	const struct bmc150_scale_info scale_table[4];
>  };
>  
> +struct bmc150_accel_interrupt {
> +	const struct bmc150_accel_interrupt_info *info;
> +	atomic_t users;
> +};
> +
> +#define BMC150_ACCEL_INTERRUPTS		2
> +
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
> +	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
>  	struct iio_trigger *dready_trig;
>  	struct iio_trigger *motion_trig;
> +	atomic_t active_intr;
>  	struct mutex mutex;
>  	s16 buffer[8];
>  	u8 bw_bits;
> @@ -421,7 +430,7 @@ static const struct bmc150_accel_interrupt_info {
>  	u8 map_bitmask;
>  	u8 en_reg;
>  	u8 en_bitmask;
> -} bmc150_accel_interrupts[] = {
> +} bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
>  	{ /* data ready interrupt */
>  		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
>  		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
> @@ -438,12 +447,30 @@ static const struct bmc150_accel_interrupt_info {
>  	},
>  };
>  
> +static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
> +					  struct bmc150_accel_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < BMC150_ACCEL_INTERRUPTS; i++)
> +		data->interrupts[i].info = &bmc150_accel_interrupts[i];
> +}
> +
>  static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
> -				const struct bmc150_accel_interrupt_info *info,
> +				      struct bmc150_accel_interrupt *intr,
>  				      bool state)
>  {
> +	const struct bmc150_accel_interrupt_info *info = intr->info;
>  	int ret;
>  
> +	if (state) {
> +		if (atomic_inc_return(&intr->users) > 1)
> +			return 0;
> +	} else {
> +		if (atomic_dec_return(&intr->users) > 0)
> +			return 0;
> +	}
> +
>  	/*
>  	 * We will expect the enable and disable to do operation in
>  	 * in reverse order. This will happen here anyway as our
> @@ -493,6 +520,11 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
>  		goto out_fix_power_state;
>  	}
>  
> +	if (state)
> +		atomic_inc(&data->active_intr);
> +	else
> +		atomic_dec(&data->active_intr);
> +
>  	return 0;
>  
>  out_fix_power_state:
> @@ -500,20 +532,6 @@ out_fix_power_state:
>  	return ret;
>  }
>  
> -static int bmc150_accel_setup_any_motion_interrupt(
> -					struct bmc150_accel_data *data,
> -					bool status)
> -{
> -	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
> -					  status);
> -}
> -
> -static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
> -						 bool status)
> -{
> -	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
> -					  status);
> -}
>  
>  static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
>  {
> @@ -753,13 +771,7 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&data->mutex);
>  
> -	if (!state && data->motion_trigger_on) {
> -		data->ev_enable_state = 0;
> -		mutex_unlock(&data->mutex);
> -		return 0;
> -	}
> -
> -	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
> +	ret = bmc150_accel_set_interrupt(data, &data->interrupts[1], state);
>  	if (ret < 0) {
>  		mutex_unlock(&data->mutex);
>  		return ret;
> @@ -996,19 +1008,15 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  		}
>  	}
>  
> -	if (!state && data->ev_enable_state && data->motion_trigger_on) {
> -		data->motion_trigger_on = false;
> -		mutex_unlock(&data->mutex);
> -		return 0;
> -	}
> -
>  	if (data->motion_trig == trig) {
>  		ret = bmc150_accel_update_slope(data);
>  		if (!ret)
> -			ret = bmc150_accel_setup_any_motion_interrupt(data,
> -								      state);
> +			ret = bmc150_accel_set_interrupt(data,
> +							 &data->interrupts[1],
> +							 state);
>  	} else {
> -		ret = bmc150_accel_setup_new_data_interrupt(data, state);
> +		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
> +						 state);
>  	}
>  	if (ret < 0) {
>  		mutex_unlock(&data->mutex);
> @@ -1210,6 +1218,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  			return ret;
>  		}
>  
> +		bmc150_accel_interrupts_setup(indio_dev, data);
> +
>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
>  							   "%s-dev%d",
>  							   indio_dev->name,
> @@ -1325,8 +1335,7 @@ static int bmc150_accel_resume(struct device *dev)
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>  
>  	mutex_lock(&data->mutex);
> -	if (data->dready_trigger_on || data->motion_trigger_on ||
> -							data->ev_enable_state)
> +	if (atomic_read(&data->active_intr))
>  		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
>  	mutex_unlock(&data->mutex);
>  
> 

[PATCH v3 8/9] iio: bmc150: introduce bmc150_accel_trigger

[Octavian Purdila]

Add a separate structure for triggers and add the infrastructure to
support an arbitrary number of triggers. Each trigger is associated
with an interrupt and has an enabled/disabled state.

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

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index b23ad1b..c05aa43 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -152,14 +152,22 @@ struct bmc150_accel_interrupt {
 	atomic_t users;
 };
 
+struct bmc150_accel_trigger {
+	struct bmc150_accel_interrupt *intr;
+	struct bmc150_accel_data *data;
+	struct iio_trigger *indio_trig;
+	bool enabled;
+	int (*setup)(struct bmc150_accel_trigger *t, bool state);
+};
+
 #define BMC150_ACCEL_INTERRUPTS		2
+#define BMC150_ACCEL_TRIGGERS		2
 
 struct bmc150_accel_data {
 	struct i2c_client *client;
 	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
-	struct iio_trigger *dready_trig;
-	struct iio_trigger *motion_trig;
 	atomic_t active_intr;
+	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
 	struct mutex mutex;
 	s16 buffer[8];
 	u8 bw_bits;
@@ -167,8 +175,6 @@ struct bmc150_accel_data {
 	u32 slope_thres;
 	u32 range;
 	int ev_enable_state;
-	bool dready_trigger_on;
-	bool motion_trigger_on;
 	int64_t timestamp;
 	const struct bmc150_accel_chip_info *chip_info;
 };
@@ -309,6 +315,15 @@ static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
 	return ret;
 }
 
+static int bmc150_accel_any_motion_setup(struct bmc150_accel_trigger *t,
+					 bool state)
+{
+	if (state)
+		return bmc150_accel_update_slope(t->data);
+
+	return 0;
+}
+
 static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 {
 	int ret;
@@ -787,11 +802,14 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
 				   struct iio_trigger *trig)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int i;
 
-	if (data->dready_trig != trig && data->motion_trig != trig)
-		return -EINVAL;
+	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
+		if (data->triggers[i].indio_trig == trig)
+			return 0;
+	}
 
-	return 0;
+	return -EINVAL;
 }
 
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
@@ -963,12 +981,12 @@ err_read:
 
 static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
 {
-	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
-	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
+	struct bmc150_accel_data *data = t->data;
 	int ret;
 
 	/* new data interrupts don't need ack */
-	if (data->dready_trigger_on)
+	if (t == &t->data->triggers[0])
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -990,42 +1008,32 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
 static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 						   bool state)
 {
-	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
-	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
+	struct bmc150_accel_data *data = t->data;
 	int ret;
 
 	mutex_lock(&data->mutex);
 
-	if (data->motion_trig == trig) {
-		if (data->motion_trigger_on == state) {
-			mutex_unlock(&data->mutex);
-			return 0;
-		}
-	} else {
-		if (data->dready_trigger_on == state) {
+	if (t->enabled == state) {
+		mutex_unlock(&data->mutex);
+		return 0;
+	}
+
+	if (t->setup) {
+		ret = t->setup(t, state);
+		if (ret < 0) {
 			mutex_unlock(&data->mutex);
-			return 0;
+			return ret;
 		}
 	}
 
-	if (data->motion_trig == trig) {
-		ret = bmc150_accel_update_slope(data);
-		if (!ret)
-			ret = bmc150_accel_set_interrupt(data,
-							 &data->interrupts[1],
-							 state);
-	} else {
-		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
-						 state);
-	}
+	ret = bmc150_accel_set_interrupt(data, t->intr, state);
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
 		return ret;
 	}
-	if (data->motion_trig == trig)
-		data->motion_trigger_on = state;
-	else
-		data->dready_trigger_on = state;
+
+	t->enabled = state;
 
 	mutex_unlock(&data->mutex);
 
@@ -1079,7 +1087,7 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
 							dir),
 							data->timestamp);
 ack_intr_status:
-	if (!data->dready_trigger_on)
+	if (!data->triggers[0].enabled)
 		ret = i2c_smbus_write_byte_data(data->client,
 					BMC150_ACCEL_REG_INT_RST_LATCH,
 					BMC150_ACCEL_INT_MODE_LATCH_INT |
@@ -1092,13 +1100,16 @@ 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);
+	int i;
 
 	data->timestamp = iio_get_time_ns();
 
-	if (data->dready_trigger_on)
-		iio_trigger_poll(data->dready_trig);
-	else if (data->motion_trigger_on)
-		iio_trigger_poll(data->motion_trig);
+	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
+		if (data->triggers[i].enabled) {
+			iio_trigger_poll(data->triggers[i].indio_trig);
+			break;
+		}
+	}
 
 	if (data->ev_enable_state)
 		return IRQ_WAKE_THREAD;
@@ -1150,6 +1161,71 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
 	return ret;
 }
 
+static const struct {
+	int intr;
+	const char *name;
+	int (*setup)(struct bmc150_accel_trigger *t, bool state);
+} bmc150_accel_triggers[BMC150_ACCEL_TRIGGERS] = {
+	{
+		.intr = 0,
+		.name = "%s-dev%d",
+	},
+	{
+		.intr = 1,
+		.name = "%s-any-motion-dev%d",
+		.setup = bmc150_accel_any_motion_setup,
+	},
+};
+
+static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
+					     int from)
+{
+	int i;
+
+	for (i = from; i >= 0; i++) {
+		if (data->triggers[i].indio_trig) {
+			iio_trigger_unregister(data->triggers[i].indio_trig);
+			data->triggers[i].indio_trig = NULL;
+		}
+	}
+}
+
+static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
+				       struct bmc150_accel_data *data)
+{
+	int i, ret;
+
+	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
+		struct bmc150_accel_trigger *t = &data->triggers[i];
+		const char *name = bmc150_accel_triggers[i].name;
+		int intr = bmc150_accel_triggers[i].intr;
+
+		t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
+						       indio_dev->name,
+						       indio_dev->id);
+		if (!t->indio_trig) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		t->indio_trig->dev.parent = &data->client->dev;
+		t->indio_trig->ops = &bmc150_accel_trigger_ops;
+		t->intr = &data->interrupts[intr];
+		t->data = data;
+		t->setup = bmc150_accel_triggers[i].setup;
+		iio_trigger_set_drvdata(t->indio_trig, t);
+
+		ret = iio_trigger_register(t->indio_trig);
+		if (ret)
+			break;
+	}
+
+	if (ret)
+		bmc150_accel_unregister_triggers(data, i - 1);
+
+	return ret;
+}
+
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1220,36 +1296,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
 
 		bmc150_accel_interrupts_setup(indio_dev, data);
 
-		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
-							   "%s-dev%d",
-							   indio_dev->name,
-							   indio_dev->id);
-		if (!data->dready_trig)
-			return -ENOMEM;
-
-		data->motion_trig = devm_iio_trigger_alloc(&client->dev,
-							  "%s-any-motion-dev%d",
-							  indio_dev->name,
-							  indio_dev->id);
-		if (!data->motion_trig)
-			return -ENOMEM;
-
-		data->dready_trig->dev.parent = &client->dev;
-		data->dready_trig->ops = &bmc150_accel_trigger_ops;
-		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
-		ret = iio_trigger_register(data->dready_trig);
+		ret = bmc150_accel_triggers_setup(indio_dev, data);
 		if (ret)
 			return ret;
 
-		data->motion_trig->dev.parent = &client->dev;
-		data->motion_trig->ops = &bmc150_accel_trigger_ops;
-		iio_trigger_set_drvdata(data->motion_trig, indio_dev);
-		ret = iio_trigger_register(data->motion_trig);
-		if (ret) {
-			data->motion_trig = NULL;
-			goto err_trigger_unregister;
-		}
-
 		ret = iio_triggered_buffer_setup(indio_dev,
 						 &iio_pollfunc_store_time,
 						 bmc150_accel_trigger_handler,
@@ -1281,13 +1331,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
 err_iio_unregister:
 	iio_device_unregister(indio_dev);
 err_buffer_cleanup:
-	if (data->dready_trig)
+	if (indio_dev->pollfunc)
 		iio_triggered_buffer_cleanup(indio_dev);
 err_trigger_unregister:
-	if (data->dready_trig)
-		iio_trigger_unregister(data->dready_trig);
-	if (data->motion_trig)
-		iio_trigger_unregister(data->motion_trig);
+	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
 
 	return ret;
 }
@@ -1303,11 +1350,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 
-	if (data->dready_trig) {
-		iio_triggered_buffer_cleanup(indio_dev);
-		iio_trigger_unregister(data->dready_trig);
-		iio_trigger_unregister(data->motion_trig);
-	}
+	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
 
 	mutex_lock(&data->mutex);
 	bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_DEEP_SUSPEND, 0);
-- 
1.9.1

Re: [PATCH v3 8/9] iio: bmc150: introduce bmc150_accel_trigger

[Jonathan Cameron]

On 31/01/15 00:00, Octavian Purdila wrote:
> Add a separate structure for triggers and add the infrastructure to
> support an arbitrary number of triggers. Each trigger is associated
> with an interrupt and has an enabled/disabled state.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Another sensible patch. One trivial point inline.  Otherwise, I'd
like an Ack from Srinivas on this as it's a non trivial bit of refactoring
and despite seeming like a good idea does add a fair bit of code!

J
> ---
>  drivers/iio/accel/bmc150-accel.c | 195 ++++++++++++++++++++++++---------------
>  1 file changed, 119 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index b23ad1b..c05aa43 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -152,14 +152,22 @@ struct bmc150_accel_interrupt {
>  	atomic_t users;
>  };
>  
> +struct bmc150_accel_trigger {
> +	struct bmc150_accel_interrupt *intr;
> +	struct bmc150_accel_data *data;
> +	struct iio_trigger *indio_trig;
> +	bool enabled;
> +	int (*setup)(struct bmc150_accel_trigger *t, bool state);
> +};
> +
>  #define BMC150_ACCEL_INTERRUPTS		2
> +#define BMC150_ACCEL_TRIGGERS		2
>  
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
>  	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
> -	struct iio_trigger *dready_trig;
> -	struct iio_trigger *motion_trig;
>  	atomic_t active_intr;
> +	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
>  	struct mutex mutex;
>  	s16 buffer[8];
>  	u8 bw_bits;
> @@ -167,8 +175,6 @@ struct bmc150_accel_data {
>  	u32 slope_thres;
>  	u32 range;
>  	int ev_enable_state;
> -	bool dready_trigger_on;
> -	bool motion_trigger_on;
>  	int64_t timestamp;
>  	const struct bmc150_accel_chip_info *chip_info;
>  };
> @@ -309,6 +315,15 @@ static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
>  	return ret;
>  }
>  
> +static int bmc150_accel_any_motion_setup(struct bmc150_accel_trigger *t,
> +					 bool state)
> +{
> +	if (state)
> +		return bmc150_accel_update_slope(t->data);
> +
> +	return 0;
> +}
> +
>  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  {
>  	int ret;
> @@ -787,11 +802,14 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
>  				   struct iio_trigger *trig)
>  {
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int i;
>  
> -	if (data->dready_trig != trig && data->motion_trig != trig)
> -		return -EINVAL;
> +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> +		if (data->triggers[i].indio_trig == trig)
> +			return 0;
> +	}
>  
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> @@ -963,12 +981,12 @@ err_read:
>  
>  static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
>  {
> -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> -	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
> +	struct bmc150_accel_data *data = t->data;
>  	int ret;
>  
>  	/* new data interrupts don't need ack */
> -	if (data->dready_trigger_on)
> +	if (t == &t->data->triggers[0])
>  		return 0;
>  
>  	mutex_lock(&data->mutex);
> @@ -990,42 +1008,32 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
>  static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  						   bool state)
>  {
> -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> -	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
> +	struct bmc150_accel_data *data = t->data;
>  	int ret;
>  
>  	mutex_lock(&data->mutex);
>  
> -	if (data->motion_trig == trig) {
> -		if (data->motion_trigger_on == state) {
> -			mutex_unlock(&data->mutex);
> -			return 0;
> -		}
> -	} else {
> -		if (data->dready_trigger_on == state) {
> +	if (t->enabled == state) {
> +		mutex_unlock(&data->mutex);
> +		return 0;
> +	}
> +
> +	if (t->setup) {
> +		ret = t->setup(t, state);
> +		if (ret < 0) {
>  			mutex_unlock(&data->mutex);
> -			return 0;
> +			return ret;
>  		}
>  	}
>  
> -	if (data->motion_trig == trig) {
> -		ret = bmc150_accel_update_slope(data);
> -		if (!ret)
> -			ret = bmc150_accel_set_interrupt(data,
> -							 &data->interrupts[1],
> -							 state);
> -	} else {
> -		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
> -						 state);
> -	}
> +	ret = bmc150_accel_set_interrupt(data, t->intr, state);
>  	if (ret < 0) {
>  		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
> -	if (data->motion_trig == trig)
> -		data->motion_trigger_on = state;
> -	else
> -		data->dready_trigger_on = state;
> +
> +	t->enabled = state;
>  
>  	mutex_unlock(&data->mutex);
>  
> @@ -1079,7 +1087,7 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
>  							dir),
>  							data->timestamp);
>  ack_intr_status:
> -	if (!data->dready_trigger_on)
> +	if (!data->triggers[0].enabled)
>  		ret = i2c_smbus_write_byte_data(data->client,
>  					BMC150_ACCEL_REG_INT_RST_LATCH,
>  					BMC150_ACCEL_INT_MODE_LATCH_INT |
> @@ -1092,13 +1100,16 @@ 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);
> +	int i;
>  
>  	data->timestamp = iio_get_time_ns();
>  
> -	if (data->dready_trigger_on)
> -		iio_trigger_poll(data->dready_trig);
> -	else if (data->motion_trigger_on)
> -		iio_trigger_poll(data->motion_trig);
> +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> +		if (data->triggers[i].enabled) {
> +			iio_trigger_poll(data->triggers[i].indio_trig);
> +			break;
> +		}
> +	}
>  
>  	if (data->ev_enable_state)
>  		return IRQ_WAKE_THREAD;
> @@ -1150,6 +1161,71 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
>  	return ret;
>  }
>  
> +static const struct {
> +	int intr;
> +	const char *name;
> +	int (*setup)(struct bmc150_accel_trigger *t, bool state);
> +} bmc150_accel_triggers[BMC150_ACCEL_TRIGGERS] = {
> +	{
> +		.intr = 0,
> +		.name = "%s-dev%d",
> +	},
> +	{
> +		.intr = 1,
> +		.name = "%s-any-motion-dev%d",
> +		.setup = bmc150_accel_any_motion_setup,
> +	},
> +};
> +
> +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
> +					     int from)
> +{
> +	int i;
> +
> +	for (i = from; i >= 0; i++) {
> +		if (data->triggers[i].indio_trig) {
> +			iio_trigger_unregister(data->triggers[i].indio_trig);
> +			data->triggers[i].indio_trig = NULL;
> +		}
> +	}
> +}
> +
> +static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
> +				       struct bmc150_accel_data *data)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> +		struct bmc150_accel_trigger *t = &data->triggers[i];
> +		const char *name = bmc150_accel_triggers[i].name;
Why bother with this local variable?  Just pass it straight in.

> +		int intr = bmc150_accel_triggers[i].intr;
> +
> +		t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
> +						       indio_dev->name,
> +						       indio_dev->id);
> +		if (!t->indio_trig) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		t->indio_trig->dev.parent = &data->client->dev;
> +		t->indio_trig->ops = &bmc150_accel_trigger_ops;
> +		t->intr = &data->interrupts[intr];
> +		t->data = data;
> +		t->setup = bmc150_accel_triggers[i].setup;
> +		iio_trigger_set_drvdata(t->indio_trig, t);
> +
> +		ret = iio_trigger_register(t->indio_trig);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (ret)
> +		bmc150_accel_unregister_triggers(data, i - 1);
> +
> +	return ret;
> +}
> +
>  static int bmc150_accel_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
> @@ -1220,36 +1296,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  
>  		bmc150_accel_interrupts_setup(indio_dev, data);
>  
> -		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> -							   "%s-dev%d",
> -							   indio_dev->name,
> -							   indio_dev->id);
> -		if (!data->dready_trig)
> -			return -ENOMEM;
> -
> -		data->motion_trig = devm_iio_trigger_alloc(&client->dev,
> -							  "%s-any-motion-dev%d",
> -							  indio_dev->name,
> -							  indio_dev->id);
> -		if (!data->motion_trig)
> -			return -ENOMEM;
> -
> -		data->dready_trig->dev.parent = &client->dev;
> -		data->dready_trig->ops = &bmc150_accel_trigger_ops;
> -		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> -		ret = iio_trigger_register(data->dready_trig);
> +		ret = bmc150_accel_triggers_setup(indio_dev, data);
>  		if (ret)
>  			return ret;
>  
> -		data->motion_trig->dev.parent = &client->dev;
> -		data->motion_trig->ops = &bmc150_accel_trigger_ops;
> -		iio_trigger_set_drvdata(data->motion_trig, indio_dev);
> -		ret = iio_trigger_register(data->motion_trig);
> -		if (ret) {
> -			data->motion_trig = NULL;
> -			goto err_trigger_unregister;
> -		}
> -
>  		ret = iio_triggered_buffer_setup(indio_dev,
>  						 &iio_pollfunc_store_time,
>  						 bmc150_accel_trigger_handler,
> @@ -1281,13 +1331,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  err_iio_unregister:
>  	iio_device_unregister(indio_dev);
>  err_buffer_cleanup:
> -	if (data->dready_trig)
> +	if (indio_dev->pollfunc)
>  		iio_triggered_buffer_cleanup(indio_dev);
>  err_trigger_unregister:
> -	if (data->dready_trig)
> -		iio_trigger_unregister(data->dready_trig);
> -	if (data->motion_trig)
> -		iio_trigger_unregister(data->motion_trig);
> +	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
>  
>  	return ret;
>  }
> @@ -1303,11 +1350,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(indio_dev);
>  
> -	if (data->dready_trig) {
> -		iio_triggered_buffer_cleanup(indio_dev);
> -		iio_trigger_unregister(data->dready_trig);
> -		iio_trigger_unregister(data->motion_trig);
> -	}
> +	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
>  
>  	mutex_lock(&data->mutex);
>  	bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_DEEP_SUSPEND, 0);
> 

Re: [PATCH v3 8/9] iio: bmc150: introduce bmc150_accel_trigger

[Srinivas Pandruvada]

On Sun, 2015-02-08 at 11:07 +0000, Jonathan Cameron wrote: 
> On 31/01/15 00:00, Octavian Purdila wrote:
> > Add a separate structure for triggers and add the infrastructure to
> > support an arbitrary number of triggers. Each trigger is associated
> > with an interrupt and has an enabled/disabled state.
> > 
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Another sensible patch. One trivial point inline.  Otherwise, I'd
> like an Ack from Srinivas on this as it's a non trivial bit of refactoring
> and despite seeming like a good idea does add a fair bit of code!
> 
Some minor comment (But not a big deal at this time) in addition to
Jonathan's. Once addressed you can add:
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 
> J
> > ---
> >  drivers/iio/accel/bmc150-accel.c | 195 ++++++++++++++++++++++++---------------
> >  1 file changed, 119 insertions(+), 76 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> > index b23ad1b..c05aa43 100644
> > --- a/drivers/iio/accel/bmc150-accel.c
> > +++ b/drivers/iio/accel/bmc150-accel.c
> > @@ -152,14 +152,22 @@ struct bmc150_accel_interrupt {
> >  	atomic_t users;
> >  };
> >  
> > +struct bmc150_accel_trigger {
> > +	struct bmc150_accel_interrupt *intr;
> > +	struct bmc150_accel_data *data;
> > +	struct iio_trigger *indio_trig;
> > +	bool enabled;
> > +	int (*setup)(struct bmc150_accel_trigger *t, bool state);
> > +};
> > +
> >  #define BMC150_ACCEL_INTERRUPTS		2
> > +#define BMC150_ACCEL_TRIGGERS		2
> >  
> >  struct bmc150_accel_data {
> >  	struct i2c_client *client;
> >  	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
> > -	struct iio_trigger *dready_trig;
> > -	struct iio_trigger *motion_trig;
> >  	atomic_t active_intr;
> > +	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
> >  	struct mutex mutex;
> >  	s16 buffer[8];
> >  	u8 bw_bits;
> > @@ -167,8 +175,6 @@ struct bmc150_accel_data {
> >  	u32 slope_thres;
> >  	u32 range;
> >  	int ev_enable_state;
> > -	bool dready_trigger_on;
> > -	bool motion_trigger_on;
> >  	int64_t timestamp;
> >  	const struct bmc150_accel_chip_info *chip_info;
> >  };
> > @@ -309,6 +315,15 @@ static int bmc150_accel_update_slope(struct bmc150_accel_data *data)
> >  	return ret;
> >  }
> >  
> > +static int bmc150_accel_any_motion_setup(struct bmc150_accel_trigger *t,
> > +					 bool state)
> > +{
> > +	if (state)
> > +		return bmc150_accel_update_slope(t->data);
> > +
> > +	return 0;
> > +}
> > +
> >  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> >  {
> >  	int ret;
> > @@ -787,11 +802,14 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
> >  				   struct iio_trigger *trig)
> >  {
> >  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> > +	int i;
> >  
> > -	if (data->dready_trig != trig && data->motion_trig != trig)
> > -		return -EINVAL;
> > +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> > +		if (data->triggers[i].indio_trig == trig)
> > +			return 0;
> > +	}
> >  
> > -	return 0;
> > +	return -EINVAL;
> >  }
> >  
> >  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> > @@ -963,12 +981,12 @@ err_read:
> >  
> >  static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
> >  {
> > -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > -	struct bmc150_accel_data *data = iio_priv(indio_dev);
> > +	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
> > +	struct bmc150_accel_data *data = t->data;
> >  	int ret;
> >  
> >  	/* new data interrupts don't need ack */
> > -	if (data->dready_trigger_on)
> > +	if (t == &t->data->triggers[0])
> >  		return 0;
> >  
> >  	mutex_lock(&data->mutex);
> > @@ -990,42 +1008,32 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
> >  static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
> >  						   bool state)
> >  {
> > -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > -	struct bmc150_accel_data *data = iio_priv(indio_dev);
> > +	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
> > +	struct bmc150_accel_data *data = t->data;
> >  	int ret;
> >  
> >  	mutex_lock(&data->mutex);
> >  
> > -	if (data->motion_trig == trig) {
> > -		if (data->motion_trigger_on == state) {
> > -			mutex_unlock(&data->mutex);
> > -			return 0;
> > -		}
> > -	} else {
> > -		if (data->dready_trigger_on == state) {
> > +	if (t->enabled == state) {
> > +		mutex_unlock(&data->mutex);
> > +		return 0;
> > +	}
> > +
> > +	if (t->setup) {
> > +		ret = t->setup(t, state);
> > +		if (ret < 0) {
> >  			mutex_unlock(&data->mutex);
> > -			return 0;
> > +			return ret;
> >  		}
> >  	}
> >  
> > -	if (data->motion_trig == trig) {
> > -		ret = bmc150_accel_update_slope(data);
> > -		if (!ret)
> > -			ret = bmc150_accel_set_interrupt(data,
> > -							 &data->interrupts[1],
> > -							 state);
> > -	} else {
> > -		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
> > -						 state);
> > -	}
> > +	ret = bmc150_accel_set_interrupt(data, t->intr, state);
> >  	if (ret < 0) {
> >  		mutex_unlock(&data->mutex);
> >  		return ret;
> >  	}
> > -	if (data->motion_trig == trig)
> > -		data->motion_trigger_on = state;
> > -	else
> > -		data->dready_trigger_on = state;
> > +
> > +	t->enabled = state;
> >  
> >  	mutex_unlock(&data->mutex);
> >  
> > @@ -1079,7 +1087,7 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
> >  							dir),
> >  							data->timestamp);
> >  ack_intr_status:
> > -	if (!data->dready_trigger_on)
> > +	if (!data->triggers[0].enabled)
I think it is better to use some enum in place of 0, this way it will be
less confusing when we have many triggers. 
> >  		ret = i2c_smbus_write_byte_data(data->client,
> >  					BMC150_ACCEL_REG_INT_RST_LATCH,
> >  					BMC150_ACCEL_INT_MODE_LATCH_INT |
> > @@ -1092,13 +1100,16 @@ 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);
> > +	int i;
> >  
> >  	data->timestamp = iio_get_time_ns();
> >  
> > -	if (data->dready_trigger_on)
> > -		iio_trigger_poll(data->dready_trig);
> > -	else if (data->motion_trigger_on)
> > -		iio_trigger_poll(data->motion_trig);
> > +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> > +		if (data->triggers[i].enabled) {
> > +			iio_trigger_poll(data->triggers[i].indio_trig);
> > +			break;
> > +		}
> > +	}
> >  
> >  	if (data->ev_enable_state)
> >  		return IRQ_WAKE_THREAD;
> > @@ -1150,6 +1161,71 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
> >  	return ret;
> >  }
> >  
> > +static const struct {
> > +	int intr;
> > +	const char *name;
> > +	int (*setup)(struct bmc150_accel_trigger *t, bool state);
> > +} bmc150_accel_triggers[BMC150_ACCEL_TRIGGERS] = {
> > +	{
> > +		.intr = 0,
> > +		.name = "%s-dev%d",
> > +	},
> > +	{
> > +		.intr = 1,
> > +		.name = "%s-any-motion-dev%d",
> > +		.setup = bmc150_accel_any_motion_setup,
> > +	},
> > +};
> > +
> > +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
> > +					     int from)
> > +{
> > +	int i;
> > +
> > +	for (i = from; i >= 0; i++) {
> > +		if (data->triggers[i].indio_trig) {
> > +			iio_trigger_unregister(data->triggers[i].indio_trig);
> > +			data->triggers[i].indio_trig = NULL;
> > +		}
> > +	}
> > +}
> > +
> > +static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
> > +				       struct bmc150_accel_data *data)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> > +		struct bmc150_accel_trigger *t = &data->triggers[i];
> > +		const char *name = bmc150_accel_triggers[i].name;
> Why bother with this local variable?  Just pass it straight in.
> 
> > +		int intr = bmc150_accel_triggers[i].intr;
> > +
> > +		t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
> > +						       indio_dev->name,
> > +						       indio_dev->id);
> > +		if (!t->indio_trig) {
> > +			ret = -ENOMEM;
> > +			break;
> > +		}
> > +
> > +		t->indio_trig->dev.parent = &data->client->dev;
> > +		t->indio_trig->ops = &bmc150_accel_trigger_ops;
> > +		t->intr = &data->interrupts[intr];
> > +		t->data = data;
> > +		t->setup = bmc150_accel_triggers[i].setup;
> > +		iio_trigger_set_drvdata(t->indio_trig, t);
> > +
> > +		ret = iio_trigger_register(t->indio_trig);
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	if (ret)
> > +		bmc150_accel_unregister_triggers(data, i - 1);
> > +
> > +	return ret;
> > +}
> > +
> >  static int bmc150_accel_probe(struct i2c_client *client,
> >  			      const struct i2c_device_id *id)
> >  {
> > @@ -1220,36 +1296,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
> >  
> >  		bmc150_accel_interrupts_setup(indio_dev, data);
> >  
> > -		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> > -							   "%s-dev%d",
> > -							   indio_dev->name,
> > -							   indio_dev->id);
> > -		if (!data->dready_trig)
> > -			return -ENOMEM;
> > -
> > -		data->motion_trig = devm_iio_trigger_alloc(&client->dev,
> > -							  "%s-any-motion-dev%d",
> > -							  indio_dev->name,
> > -							  indio_dev->id);
> > -		if (!data->motion_trig)
> > -			return -ENOMEM;
> > -
> > -		data->dready_trig->dev.parent = &client->dev;
> > -		data->dready_trig->ops = &bmc150_accel_trigger_ops;
> > -		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> > -		ret = iio_trigger_register(data->dready_trig);
> > +		ret = bmc150_accel_triggers_setup(indio_dev, data);
> >  		if (ret)
> >  			return ret;
> >  
> > -		data->motion_trig->dev.parent = &client->dev;
> > -		data->motion_trig->ops = &bmc150_accel_trigger_ops;
> > -		iio_trigger_set_drvdata(data->motion_trig, indio_dev);
> > -		ret = iio_trigger_register(data->motion_trig);
> > -		if (ret) {
> > -			data->motion_trig = NULL;
> > -			goto err_trigger_unregister;
> > -		}
> > -
> >  		ret = iio_triggered_buffer_setup(indio_dev,
> >  						 &iio_pollfunc_store_time,
> >  						 bmc150_accel_trigger_handler,
> > @@ -1281,13 +1331,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
> >  err_iio_unregister:
> >  	iio_device_unregister(indio_dev);
> >  err_buffer_cleanup:
> > -	if (data->dready_trig)
> > +	if (indio_dev->pollfunc)
> >  		iio_triggered_buffer_cleanup(indio_dev);
> >  err_trigger_unregister:
> > -	if (data->dready_trig)
> > -		iio_trigger_unregister(data->dready_trig);
> > -	if (data->motion_trig)
> > -		iio_trigger_unregister(data->motion_trig);
> > +	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
> >  
> >  	return ret;
> >  }
> > @@ -1303,11 +1350,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
> >  
> >  	iio_device_unregister(indio_dev);
> >  
> > -	if (data->dready_trig) {
> > -		iio_triggered_buffer_cleanup(indio_dev);
> > -		iio_trigger_unregister(data->dready_trig);
> > -		iio_trigger_unregister(data->motion_trig);
> > -	}
> > +	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
> >  
> >  	mutex_lock(&data->mutex);
> >  	bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_DEEP_SUSPEND, 0);
> > 
> 

[PATCH v3 9/9] iio: bmc150: add support for hardware fifo

[Octavian Purdila]

Add a new watermark trigger and hardware fifo operations. When the
watermark trigger is activated the watermark level is set and the
hardware FIFO is activated.

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

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index c05aa43..e4535ba 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,
@@ -160,8 +170,8 @@ struct bmc150_accel_trigger {
 	int (*setup)(struct bmc150_accel_trigger *t, bool state);
 };
 
-#define BMC150_ACCEL_INTERRUPTS		2
-#define BMC150_ACCEL_TRIGGERS		2
+#define BMC150_ACCEL_INTERRUPTS		3
+#define BMC150_ACCEL_TRIGGERS		3
 
 struct bmc150_accel_data {
 	struct i2c_client *client;
@@ -169,6 +179,7 @@ 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;
@@ -460,6 +471,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,
@@ -951,6 +968,8 @@ static const struct iio_info bmc150_accel_info = {
 	.driver_module		= THIS_MODULE,
 };
 
+static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, int samples);
+
 static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -958,6 +977,12 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int bit, ret, i = 0;
 
+	if (data->fifo_mode) {
+		bmc150_accel_fifo_flush(indio_dev, BMC150_ACCEL_FIFO_LENGTH);
+		iio_trigger_notify_done(indio_dev->trig);
+		return IRQ_HANDLED;
+	}
+
 	mutex_lock(&data->mutex);
 	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
 			 indio_dev->masklength) {
@@ -1161,6 +1186,8 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
 	return ret;
 }
 
+static int bmc150_accel_fifo_setup(struct bmc150_accel_trigger *t, bool state);
+
 static const struct {
 	int intr;
 	const char *name;
@@ -1175,6 +1202,11 @@ static const struct {
 		.name = "%s-any-motion-dev%d",
 		.setup = bmc150_accel_any_motion_setup,
 	},
+	{
+		.intr = 2,
+		.name = "%s-watermark-dev%d",
+		.setup = bmc150_accel_fifo_setup,
+	},
 };
 
 static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
@@ -1226,6 +1258,145 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
+
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG0;
+	int ret;
+
+	if (val > BMC150_ACCEL_FIFO_LENGTH)
+		return -EINVAL;
+
+	ret = i2c_smbus_write_byte_data(data->client, reg, val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
+		return ret;
+	}
+
+	data->watermark = val;
+
+	return 0;
+}
+
+static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, int samples)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret, i;
+	u8 count;
+	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
+	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
+	struct i2c_msg msg[2];
+	int64_t tstamp;
+	int sample_freq = 0, sec, ms;
+
+	ret = bmc150_accel_get_bw(data, &sec, &ms);
+	if (ret == IIO_VAL_INT_PLUS_MICRO)
+		sample_freq = sec * 1000000000 + ms * 1000;
+
+	ret = i2c_smbus_read_byte_data(data->client,
+				       BMC150_ACCEL_REG_FIFO_STATUS);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Error reading reg_fifo_status\n");
+		return ret;
+	}
+
+	count = ret & 0x7F;
+
+	if (!count)
+		return 0;
+
+	if (count > samples)
+		count = samples;
+
+	msg[0].addr = data->client->addr;
+	msg[0].flags = 0;
+	msg[0].buf = &reg_fifo_data;
+	msg[0].len = sizeof(reg_fifo_data);
+
+	msg[1].addr = data->client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].buf = (u8 *)buffer;
+	msg[1].len = count * 3 * 2;
+
+	ret = i2c_transfer(data->client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&indio_dev->dev, "Error reading reg_fifo_data\n");
+		return ret;
+	}
+
+	if (!data->timestamp)
+		data->timestamp = iio_get_time_ns();
+
+	tstamp = data->timestamp - count * sample_freq;
+
+	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_freq;
+	}
+
+	data->timestamp = 0;
+
+	return 0;
+}
+
+static int bmc150_accel_fifo_mode_set(struct bmc150_accel_data *data)
+{
+	u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG1;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, reg);
+
+	/* writing the fifo config discards FIFO data - avoid it if possible */
+	if (ret == data->fifo_mode)
+		return 0;
+
+	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;
+
+	/* we can set the the watermark value only after FIFO is enabled */
+	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_fifo_setup(struct bmc150_accel_trigger *t, bool state)
+{
+	if (state)
+		t->data->fifo_mode = 0x40;
+	else
+		t->data->fifo_mode = 0;
+
+	return bmc150_accel_fifo_mode_set(t->data);
+}
+
+static struct iio_hwfifo bmc150_accel_hwfifo = {
+	.length = BMC150_ACCEL_FIFO_LENGTH,
+	.set_watermark = bmc150_accel_set_watermark,
+	.flush = bmc150_accel_fifo_flush,
+};
+
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1309,6 +1480,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
 				"Failed: iio triggered buffer setup\n");
 			goto err_trigger_unregister;
 		}
+
+		indio_dev->hwfifo = &bmc150_accel_hwfifo;
 	}
 
 	ret = iio_device_register(indio_dev);
@@ -1380,6 +1553,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_mode_set(data);
 	mutex_unlock(&data->mutex);
 
 	return 0;
@@ -1413,6 +1587,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_mode_set(data);
+	if (ret < 0)
+		return ret;
 
 	sleep_val = bmc150_accel_get_startup_times(data);
 	if (sleep_val < 20)
-- 
1.9.1

Re: [PATCH v3 9/9] iio: bmc150: add support for hardware fifo

[Jonathan Cameron]

On 31/01/15 00:00, Octavian Purdila wrote:
> Add a new watermark trigger and hardware fifo operations. When the
> watermark trigger is activated the watermark level is set and the
> hardware FIFO is activated.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Clearly the trigger question is outstanding - hence I've just addressed
the code itself here rather than the approach.  Mostly fine, but a few
little suggestions inline (and moans about hardware designers ;)
> ---
>  drivers/iio/accel/bmc150-accel.c | 185 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 181 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index c05aa43..e4535ba 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
Is it just me who instinctively thinks that if you are going to bother
with a fifo when designing a chip, it ought to be longer than this?
I guess it consumes a lot of real estate on the chip.
> +
>  enum bmc150_accel_axis {
>  	AXIS_X,
>  	AXIS_Y,
> @@ -160,8 +170,8 @@ struct bmc150_accel_trigger {
>  	int (*setup)(struct bmc150_accel_trigger *t, bool state);
>  };
>  
> -#define BMC150_ACCEL_INTERRUPTS		2
> -#define BMC150_ACCEL_TRIGGERS		2
> +#define BMC150_ACCEL_INTERRUPTS		3
> +#define BMC150_ACCEL_TRIGGERS		3
Funilly enough, might be cleaner to not have these defined, but rather
use a variable sized array [] and then use ARRAY_SIZE() to always
get the number.  Cuts down on the churn when you add more interrupts / triggers.
(Note I haven't checked closely if there is any reason this won't work here!)
>  
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
> @@ -169,6 +179,7 @@ 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;
> @@ -460,6 +471,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,
> @@ -951,6 +968,8 @@ static const struct iio_info bmc150_accel_info = {
>  	.driver_module		= THIS_MODULE,
>  };
>  
> +static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, int samples);
> +
Why not just reorganise the code to avoid needing this forward definition?
>  static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -958,6 +977,12 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>  	int bit, ret, i = 0;
>  
> +	if (data->fifo_mode) {
> +		bmc150_accel_fifo_flush(indio_dev, BMC150_ACCEL_FIFO_LENGTH);
> +		iio_trigger_notify_done(indio_dev->trig);
> +		return IRQ_HANDLED;
> +	}
> +
>  	mutex_lock(&data->mutex);
>  	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
>  			 indio_dev->masklength) {
> @@ -1161,6 +1186,8 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
>  	return ret;
>  }
>  
> +static int bmc150_accel_fifo_setup(struct bmc150_accel_trigger *t, bool state);
> +
>  static const struct {
>  	int intr;
>  	const char *name;
> @@ -1175,6 +1202,11 @@ static const struct {
>  		.name = "%s-any-motion-dev%d",
>  		.setup = bmc150_accel_any_motion_setup,
>  	},
> +	{
> +		.intr = 2,
> +		.name = "%s-watermark-dev%d",
> +		.setup = bmc150_accel_fifo_setup,
> +	},
>  };
>  
>  static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
> @@ -1226,6 +1258,145 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
> +
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG0;
> +	int ret;
> +
> +	if (val > BMC150_ACCEL_FIFO_LENGTH)
> +		return -EINVAL;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, reg, val);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
> +		return ret;
> +	}
> +
> +	data->watermark = val;
> +
> +	return 0;
> +}
> +
> +static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, int samples)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret, i;
> +	u8 count;
> +	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
> +	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
> +	struct i2c_msg msg[2];
> +	int64_t tstamp;
> +	int sample_freq = 0, sec, ms;
> +
> +	ret = bmc150_accel_get_bw(data, &sec, &ms);
> +	if (ret == IIO_VAL_INT_PLUS_MICRO)
> +		sample_freq = sec * 1000000000 + ms * 1000;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       BMC150_ACCEL_REG_FIFO_STATUS);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Error reading reg_fifo_status\n");
> +		return ret;
> +	}
> +
> +	count = ret & 0x7F;
> +
> +	if (!count)
> +		return 0;
> +
> +	if (count > samples)
> +		count = samples;
> +
> +	msg[0].addr = data->client->addr;
> +	msg[0].flags = 0;
> +	msg[0].buf = &reg_fifo_data;
> +	msg[0].len = sizeof(reg_fifo_data);
> +
> +	msg[1].addr = data->client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].buf = (u8 *)buffer;
> +	msg[1].len = count * 3 * 2;
Might be cleaner to do most of this using c99 structure initialization.
Something like.

struct i2c_msg msg[2] = {
       {
		.addr = data->client->addr,
		.buf = &reg_fifo_data,
		.len = sizeof(reg_fifo_data)
	}, {
		.addr = data->client->addr,
		.flags = I2C_M_RD,
		.buf = buffer,
	};
Then you just have to set the length here.  This makes the code slightly
easier to follow as it is obvious what is static information and what
is not.

> +
> +	ret = i2c_transfer(data->client->adapter, msg, 2);
> +	if (ret != 2) {
> +		dev_err(&indio_dev->dev, "Error reading reg_fifo_data\n");
> +		return ret;
> +	}
> +
> +	if (!data->timestamp)
> +		data->timestamp = iio_get_time_ns();
> +
> +	tstamp = data->timestamp - count * sample_freq;
> +
> +	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);
> +		}
Coding style would not put brackets around a single statement like this one...
Also this should be reworked to push everything onwards (given we've read
it) and let the core demux do it's work.  Obviously you'll need to set
available_scan_masks though.

Done like this, if we have multiple clients for the stream, we end up ripping
it up more than one time - definitely wasted effort - also that code
does amalgamated copies when possible.

At some point we should look at allowing a bulk buffer push as well.
Certainly the kfifo underlying the software buffer seems to be able to
take advantage of this.

For now one record at a time will do though!
> +
> +		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
> +
> +		tstamp += sample_freq;
> +	}
> +
> +	data->timestamp = 0;
> +
> +	return 0;
> +}
> +
> +static int bmc150_accel_fifo_mode_set(struct bmc150_accel_data *data)
> +{
> +	u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG1;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, reg);
> +
> +	/* writing the fifo config discards FIFO data - avoid it if possible */
> +	if (ret == data->fifo_mode)
> +		return 0;
> +
> +	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;
> +
> +	/* we can set the the watermark value only after FIFO is enabled */
That's wonderfully bonkers from a consistency point of view.  Oh well
hardware designers...  I'm guessing it defaults to some value such as buffer
full?  Otherwise this is nasty and racy.
> +	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_fifo_setup(struct bmc150_accel_trigger *t, bool state)
> +{
> +	if (state)
> +		t->data->fifo_mode = 0x40;
> +	else
> +		t->data->fifo_mode = 0;
> +
> +	return bmc150_accel_fifo_mode_set(t->data);
> +}
> +
> +static struct iio_hwfifo bmc150_accel_hwfifo = {
> +	.length = BMC150_ACCEL_FIFO_LENGTH,
> +	.set_watermark = bmc150_accel_set_watermark,
> +	.flush = bmc150_accel_fifo_flush,
> +};
> +
>  static int bmc150_accel_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
> @@ -1309,6 +1480,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  				"Failed: iio triggered buffer setup\n");
>  			goto err_trigger_unregister;
>  		}
> +
> +		indio_dev->hwfifo = &bmc150_accel_hwfifo;
>  	}
>  
>  	ret = iio_device_register(indio_dev);
> @@ -1380,6 +1553,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_mode_set(data);
>  	mutex_unlock(&data->mutex);
>  
>  	return 0;
> @@ -1413,6 +1587,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_mode_set(data);
> +	if (ret < 0)
> +		return ret;
>  
>  	sleep_val = bmc150_accel_get_startup_times(data);
>  	if (sleep_val < 20)
>