[PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Lars-Peter Clausen]

The documentation for the iio_event_interface does not match the actual struct
anymore. This patch removes the documentation for non-existing fields and adds
documentation for missing fields.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/industrialio-core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index 2eef85f..23dd624 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -112,13 +112,14 @@ struct iio_detected_event_list {
 
 /**
  * struct iio_event_interface - chrdev interface for an event line
- * @dev:		device assocated with event interface
  * @wait:		wait queue to allow blocking reads of events
  * @event_list_lock:	mutex to protect the list of detected events
  * @det_events:		list of detected events
  * @max_events:		maximum number of events before new ones are dropped
  * @current_events:	number of events in detected list
+ * @dev_attr_list:	list of event interface sysfs attribute
  * @flags:		file operations related flags including busy flag.
+ * @group:		event interface sysfs attribute group
  */
 struct iio_event_interface {
 	wait_queue_head_t			wait;
-- 
1.7.7.3

[PATCH v2 6/6] staging:iio:events: Use non-atmoic bitops

[Lars-Peter Clausen]

We always hold the waitqueue lock when modifying the flags field. So it is safe
to use the non-atomic bitops here instead of the atomic versions.

The lock has to be held, because we need to clear the busy flag and flush the
event fifo in one atomic operation when closing the event file descriptor.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/industrialio-event.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
index 58243d9..5e461e1 100644
--- a/drivers/staging/iio/industrialio-event.c
+++ b/drivers/staging/iio/industrialio-event.c
@@ -124,7 +124,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
 	struct iio_event_interface *ev_int = filep->private_data;
 
 	spin_lock(&ev_int->wait.lock);
-	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
+	__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
 	/*
 	 * In order to maintain a clean state for reopening,
 	 * clear out any awaiting events. The mask will prevent
@@ -153,7 +153,7 @@ int iio_event_getfd(struct iio_dev *indio_dev)
 		return -ENODEV;
 
 	spin_lock(&ev_int->wait.lock);
-	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
+	if (__test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
 		spin_unlock(&ev_int->wait.lock);
 		return -EBUSY;
 	}
@@ -162,7 +162,7 @@ int iio_event_getfd(struct iio_dev *indio_dev)
 				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
 	if (fd < 0) {
 		spin_lock(&ev_int->wait.lock);
-		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
+		__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
 		spin_unlock(&ev_int->wait.lock);
 	}
 	return fd;
-- 
1.7.7.3

Re: [PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Jonathan Cameron]

On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
> The documentation for the iio_event_interface does not match the actual struct
> anymore. This patch removes the documentation for non-existing fields and adds
> documentation for missing fields.
> 
Thanks for cleaning that up.  We could add that the dev_attr_list is for
internal management only but I guess that easily established from
how it is use...
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/industrialio-core.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
> index 2eef85f..23dd624 100644
> --- a/drivers/staging/iio/industrialio-core.c
> +++ b/drivers/staging/iio/industrialio-core.c
> @@ -112,13 +112,14 @@ struct iio_detected_event_list {
>  
>  /**
>   * struct iio_event_interface - chrdev interface for an event line
> - * @dev:		device assocated with event interface
>   * @wait:		wait queue to allow blocking reads of events
>   * @event_list_lock:	mutex to protect the list of detected events
>   * @det_events:		list of detected events
>   * @max_events:		maximum number of events before new ones are dropped
>   * @current_events:	number of events in detected list
> + * @dev_attr_list:	list of event interface sysfs attribute
>   * @flags:		file operations related flags including busy flag.
> + * @group:		event interface sysfs attribute group
>   */
>  struct iio_event_interface {
>  	wait_queue_head_t			wait;

Re: [PATCH v2 2/6] staging:iio: Factor out event handling into its own file

[Jonathan Cameron]

On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
> The core iio file has gotten quite cluttered over time. This patch moves
> the event handling code into its own file. Since the event handling code is
> largely independent from the core code the only code changes necessary for
> this are to make the moved iio_device_register_eventset,
> iio_device_unregister_eventset and iio_event_getfd functions non static.
> 
> This has also the advantage that industrialio-core.c is now closer again to
> its counterpart in the outofstaging branch.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> 
> ---
> Changes since v1:
> 	* Add missing include to sched.h
> ---
>  drivers/staging/iio/Makefile             |    2 +-
>  drivers/staging/iio/iio_core.h           |    4 +
>  drivers/staging/iio/industrialio-core.c  |  459 ----------------------------
>  drivers/staging/iio/industrialio-event.c |  483 ++++++++++++++++++++++++++++++
>  4 files changed, 488 insertions(+), 460 deletions(-)
>  create mode 100644 drivers/staging/iio/industrialio-event.c
> 
> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
> index 1340aea..657710b 100644
> --- a/drivers/staging/iio/Makefile
> +++ b/drivers/staging/iio/Makefile
> @@ -3,7 +3,7 @@
>  #
>  
>  obj-$(CONFIG_IIO) += industrialio.o
> -industrialio-y := industrialio-core.o
> +industrialio-y := industrialio-core.o industrialio-event.o
>  industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>  
> diff --git a/drivers/staging/iio/iio_core.h b/drivers/staging/iio/iio_core.h
> index ff27f13..3b20de3 100644
> --- a/drivers/staging/iio/iio_core.h
> +++ b/drivers/staging/iio/iio_core.h
> @@ -60,4 +60,8 @@ static inline void iio_chrdev_buffer_release(struct iio_dev *indio_dev)
>  
>  #endif
>  
> +int iio_device_register_eventset(struct iio_dev *indio_dev);
> +void iio_device_unregister_eventset(struct iio_dev *indio_dev);
> +int iio_event_getfd(struct iio_dev *indio_dev);
> +
>  #endif
> diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
> index 23dd624..3089307 100644
> --- a/drivers/staging/iio/industrialio-core.c
> +++ b/drivers/staging/iio/industrialio-core.c
> @@ -100,72 +100,6 @@ const struct iio_chan_spec
>  	return NULL;
>  }
>  
> -/**
> - * struct iio_detected_event_list - list element for events that have occurred
> - * @list:		linked list header
> - * @ev:			the event itself
> - */
> -struct iio_detected_event_list {
> -	struct list_head		list;
> -	struct iio_event_data		ev;
> -};
> -
> -/**
> - * struct iio_event_interface - chrdev interface for an event line
> - * @wait:		wait queue to allow blocking reads of events
> - * @event_list_lock:	mutex to protect the list of detected events
> - * @det_events:		list of detected events
> - * @max_events:		maximum number of events before new ones are dropped
> - * @current_events:	number of events in detected list
> - * @dev_attr_list:	list of event interface sysfs attribute
> - * @flags:		file operations related flags including busy flag.
> - * @group:		event interface sysfs attribute group
> - */
> -struct iio_event_interface {
> -	wait_queue_head_t			wait;
> -	struct mutex				event_list_lock;
> -	struct list_head			det_events;
> -	int					max_events;
> -	int					current_events;
> -	struct list_head dev_attr_list;
> -	unsigned long flags;
> -	struct attribute_group			group;
> -};
> -
> -int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
> -{
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> -	struct iio_detected_event_list *ev;
> -	int ret = 0;
> -
> -	/* Does anyone care? */
> -	mutex_lock(&ev_int->event_list_lock);
> -	if (test_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> -		if (ev_int->current_events == ev_int->max_events) {
> -			mutex_unlock(&ev_int->event_list_lock);
> -			return 0;
> -		}
> -		ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> -		if (ev == NULL) {
> -			ret = -ENOMEM;
> -			mutex_unlock(&ev_int->event_list_lock);
> -			goto error_ret;
> -		}
> -		ev->ev.id = ev_code;
> -		ev->ev.timestamp = timestamp;
> -
> -		list_add_tail(&ev->list, &ev_int->det_events);
> -		ev_int->current_events++;
> -		mutex_unlock(&ev_int->event_list_lock);
> -		wake_up_interruptible(&ev_int->wait);
> -	} else
> -		mutex_unlock(&ev_int->event_list_lock);
> -
> -error_ret:
> -	return ret;
> -}
> -EXPORT_SYMBOL(iio_push_event);
> -
>  /* This turns up an awful lot */
>  ssize_t iio_read_const_attr(struct device *dev,
>  			    struct device_attribute *attr,
> @@ -175,110 +109,6 @@ ssize_t iio_read_const_attr(struct device *dev,
>  }
>  EXPORT_SYMBOL(iio_read_const_attr);
>  
> -static ssize_t iio_event_chrdev_read(struct file *filep,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *f_ps)
> -{
> -	struct iio_event_interface *ev_int = filep->private_data;
> -	struct iio_detected_event_list *el;
> -	size_t len = sizeof(el->ev);
> -	int ret;
> -
> -	if (count < len)
> -		return -EINVAL;
> -
> -	mutex_lock(&ev_int->event_list_lock);
> -	if (list_empty(&ev_int->det_events)) {
> -		if (filep->f_flags & O_NONBLOCK) {
> -			ret = -EAGAIN;
> -			goto error_mutex_unlock;
> -		}
> -		mutex_unlock(&ev_int->event_list_lock);
> -		/* Blocking on device; waiting for something to be there */
> -		ret = wait_event_interruptible(ev_int->wait,
> -					       !list_empty(&ev_int
> -							   ->det_events));
> -		if (ret)
> -			goto error_ret;
> -		/* Single access device so no one else can get the data */
> -		mutex_lock(&ev_int->event_list_lock);
> -	}
> -
> -	el = list_first_entry(&ev_int->det_events,
> -			      struct iio_detected_event_list,
> -			      list);
> -	if (copy_to_user(buf, &(el->ev), len)) {
> -		ret = -EFAULT;
> -		goto error_mutex_unlock;
> -	}
> -	list_del(&el->list);
> -	ev_int->current_events--;
> -	mutex_unlock(&ev_int->event_list_lock);
> -	kfree(el);
> -
> -	return len;
> -
> -error_mutex_unlock:
> -	mutex_unlock(&ev_int->event_list_lock);
> -error_ret:
> -
> -	return ret;
> -}
> -
> -static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
> -{
> -	struct iio_event_interface *ev_int = filep->private_data;
> -	struct iio_detected_event_list *el, *t;
> -
> -	mutex_lock(&ev_int->event_list_lock);
> -	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> -	/*
> -	 * In order to maintain a clean state for reopening,
> -	 * clear out any awaiting events. The mask will prevent
> -	 * any new __iio_push_event calls running.
> -	 */
> -	list_for_each_entry_safe(el, t, &ev_int->det_events, list) {
> -		list_del(&el->list);
> -		kfree(el);
> -	}
> -	ev_int->current_events = 0;
> -	mutex_unlock(&ev_int->event_list_lock);
> -
> -	return 0;
> -}
> -
> -static const struct file_operations iio_event_chrdev_fileops = {
> -	.read =  iio_event_chrdev_read,
> -	.release = iio_event_chrdev_release,
> -	.owner = THIS_MODULE,
> -	.llseek = noop_llseek,
> -};
> -
> -static int iio_event_getfd(struct iio_dev *indio_dev)
> -{
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> -	int fd;
> -
> -	if (ev_int == NULL)
> -		return -ENODEV;
> -
> -	mutex_lock(&ev_int->event_list_lock);
> -	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> -		mutex_unlock(&ev_int->event_list_lock);
> -		return -EBUSY;
> -	}
> -	mutex_unlock(&ev_int->event_list_lock);
> -	fd = anon_inode_getfd("iio:event",
> -				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
> -	if (fd < 0) {
> -		mutex_lock(&ev_int->event_list_lock);
> -		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> -		mutex_unlock(&ev_int->event_list_lock);
> -	}
> -	return fd;
> -}
> -
>  static int __init iio_init(void)
>  {
>  	int ret;
> @@ -727,295 +557,6 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
>  	kfree(indio_dev->chan_attr_group.attrs);
>  }
>  
> -static const char * const iio_ev_type_text[] = {
> -	[IIO_EV_TYPE_THRESH] = "thresh",
> -	[IIO_EV_TYPE_MAG] = "mag",
> -	[IIO_EV_TYPE_ROC] = "roc",
> -	[IIO_EV_TYPE_THRESH_ADAPTIVE] = "thresh_adaptive",
> -	[IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
> -};
> -
> -static const char * const iio_ev_dir_text[] = {
> -	[IIO_EV_DIR_EITHER] = "either",
> -	[IIO_EV_DIR_RISING] = "rising",
> -	[IIO_EV_DIR_FALLING] = "falling"
> -};
> -
> -static ssize_t iio_ev_state_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf,
> -				  size_t len)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	int ret;
> -	bool val;
> -
> -	ret = strtobool(buf, &val);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = indio_dev->info->write_event_config(indio_dev,
> -						  this_attr->address,
> -						  val);
> -	return (ret < 0) ? ret : len;
> -}
> -
> -static ssize_t iio_ev_state_show(struct device *dev,
> -				 struct device_attribute *attr,
> -				 char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	int val = indio_dev->info->read_event_config(indio_dev,
> -						     this_attr->address);
> -
> -	if (val < 0)
> -		return val;
> -	else
> -		return sprintf(buf, "%d\n", val);
> -}
> -
> -static ssize_t iio_ev_value_show(struct device *dev,
> -				 struct device_attribute *attr,
> -				 char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	int val, ret;
> -
> -	ret = indio_dev->info->read_event_value(indio_dev,
> -						this_attr->address, &val);
> -	if (ret < 0)
> -		return ret;
> -
> -	return sprintf(buf, "%d\n", val);
> -}
> -
> -static ssize_t iio_ev_value_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf,
> -				  size_t len)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	unsigned long val;
> -	int ret;
> -
> -	if (!indio_dev->info->write_event_value)
> -		return -EINVAL;
> -
> -	ret = strict_strtoul(buf, 10, &val);
> -	if (ret)
> -		return ret;
> -
> -	ret = indio_dev->info->write_event_value(indio_dev, this_attr->address,
> -						 val);
> -	if (ret < 0)
> -		return ret;
> -
> -	return len;
> -}
> -
> -static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
> -				      struct iio_chan_spec const *chan)
> -{
> -	int ret = 0, i, attrcount = 0;
> -	u64 mask = 0;
> -	char *postfix;
> -	if (!chan->event_mask)
> -		return 0;
> -
> -	for_each_set_bit(i, &chan->event_mask, sizeof(chan->event_mask)*8) {
> -		postfix = kasprintf(GFP_KERNEL, "%s_%s_en",
> -				    iio_ev_type_text[i/IIO_EV_DIR_MAX],
> -				    iio_ev_dir_text[i%IIO_EV_DIR_MAX]);
> -		if (postfix == NULL) {
> -			ret = -ENOMEM;
> -			goto error_ret;
> -		}
> -		if (chan->modified)
> -			mask = IIO_MOD_EVENT_CODE(chan->type, 0, chan->channel,
> -						  i/IIO_EV_DIR_MAX,
> -						  i%IIO_EV_DIR_MAX);
> -		else if (chan->differential)
> -			mask = IIO_EVENT_CODE(chan->type,
> -					      0, 0,
> -					      i%IIO_EV_DIR_MAX,
> -					      i/IIO_EV_DIR_MAX,
> -					      0,
> -					      chan->channel,
> -					      chan->channel2);
> -		else
> -			mask = IIO_UNMOD_EVENT_CODE(chan->type,
> -						    chan->channel,
> -						    i/IIO_EV_DIR_MAX,
> -						    i%IIO_EV_DIR_MAX);
> -
> -		ret = __iio_add_chan_devattr(postfix,
> -					     chan,
> -					     &iio_ev_state_show,
> -					     iio_ev_state_store,
> -					     mask,
> -					     0,
> -					     &indio_dev->dev,
> -					     &indio_dev->event_interface->
> -					     dev_attr_list);
> -		kfree(postfix);
> -		if (ret)
> -			goto error_ret;
> -		attrcount++;
> -		postfix = kasprintf(GFP_KERNEL, "%s_%s_value",
> -				    iio_ev_type_text[i/IIO_EV_DIR_MAX],
> -				    iio_ev_dir_text[i%IIO_EV_DIR_MAX]);
> -		if (postfix == NULL) {
> -			ret = -ENOMEM;
> -			goto error_ret;
> -		}
> -		ret = __iio_add_chan_devattr(postfix, chan,
> -					     iio_ev_value_show,
> -					     iio_ev_value_store,
> -					     mask,
> -					     0,
> -					     &indio_dev->dev,
> -					     &indio_dev->event_interface->
> -					     dev_attr_list);
> -		kfree(postfix);
> -		if (ret)
> -			goto error_ret;
> -		attrcount++;
> -	}
> -	ret = attrcount;
> -error_ret:
> -	return ret;
> -}
> -
> -static inline void __iio_remove_event_config_attrs(struct iio_dev *indio_dev)
> -{
> -	struct iio_dev_attr *p, *n;
> -	list_for_each_entry_safe(p, n,
> -				 &indio_dev->event_interface->
> -				 dev_attr_list, l) {
> -		kfree(p->dev_attr.attr.name);
> -		kfree(p);
> -	}
> -}
> -
> -static inline int __iio_add_event_config_attrs(struct iio_dev *indio_dev)
> -{
> -	int j, ret, attrcount = 0;
> -
> -	INIT_LIST_HEAD(&indio_dev->event_interface->dev_attr_list);
> -	/* Dynically created from the channels array */
> -	for (j = 0; j < indio_dev->num_channels; j++) {
> -		ret = iio_device_add_event_sysfs(indio_dev,
> -						 &indio_dev->channels[j]);
> -		if (ret < 0)
> -			goto error_clear_attrs;
> -		attrcount += ret;
> -	}
> -	return attrcount;
> -
> -error_clear_attrs:
> -	__iio_remove_event_config_attrs(indio_dev);
> -
> -	return ret;
> -}
> -
> -static bool iio_check_for_dynamic_events(struct iio_dev *indio_dev)
> -{
> -	int j;
> -
> -	for (j = 0; j < indio_dev->num_channels; j++)
> -		if (indio_dev->channels[j].event_mask != 0)
> -			return true;
> -	return false;
> -}
> -
> -static void iio_setup_ev_int(struct iio_event_interface *ev_int)
> -{
> -	mutex_init(&ev_int->event_list_lock);
> -	/* discussion point - make this variable? */
> -	ev_int->max_events = 10;
> -	ev_int->current_events = 0;
> -	INIT_LIST_HEAD(&ev_int->det_events);
> -	init_waitqueue_head(&ev_int->wait);
> -}
> -
> -static const char *iio_event_group_name = "events";
> -static int iio_device_register_eventset(struct iio_dev *indio_dev)
> -{
> -	struct iio_dev_attr *p;
> -	int ret = 0, attrcount_orig = 0, attrcount, attrn;
> -	struct attribute **attr;
> -
> -	if (!(indio_dev->info->event_attrs ||
> -	      iio_check_for_dynamic_events(indio_dev)))
> -		return 0;
> -
> -	indio_dev->event_interface =
> -		kzalloc(sizeof(struct iio_event_interface), GFP_KERNEL);
> -	if (indio_dev->event_interface == NULL) {
> -		ret = -ENOMEM;
> -		goto error_ret;
> -	}
> -
> -	iio_setup_ev_int(indio_dev->event_interface);
> -	if (indio_dev->info->event_attrs != NULL) {
> -		attr = indio_dev->info->event_attrs->attrs;
> -		while (*attr++ != NULL)
> -			attrcount_orig++;
> -	}
> -	attrcount = attrcount_orig;
> -	if (indio_dev->channels) {
> -		ret = __iio_add_event_config_attrs(indio_dev);
> -		if (ret < 0)
> -			goto error_free_setup_event_lines;
> -		attrcount += ret;
> -	}
> -
> -	indio_dev->event_interface->group.name = iio_event_group_name;
> -	indio_dev->event_interface->group.attrs = kcalloc(attrcount + 1,
> -							  sizeof(indio_dev->event_interface->group.attrs[0]),
> -							  GFP_KERNEL);
> -	if (indio_dev->event_interface->group.attrs == NULL) {
> -		ret = -ENOMEM;
> -		goto error_free_setup_event_lines;
> -	}
> -	if (indio_dev->info->event_attrs)
> -		memcpy(indio_dev->event_interface->group.attrs,
> -		       indio_dev->info->event_attrs->attrs,
> -		       sizeof(indio_dev->event_interface->group.attrs[0])
> -		       *attrcount_orig);
> -	attrn = attrcount_orig;
> -	/* Add all elements from the list. */
> -	list_for_each_entry(p,
> -			    &indio_dev->event_interface->dev_attr_list,
> -			    l)
> -		indio_dev->event_interface->group.attrs[attrn++] =
> -			&p->dev_attr.attr;
> -	indio_dev->groups[indio_dev->groupcounter++] =
> -		&indio_dev->event_interface->group;
> -
> -	return 0;
> -
> -error_free_setup_event_lines:
> -	__iio_remove_event_config_attrs(indio_dev);
> -	kfree(indio_dev->event_interface);
> -error_ret:
> -
> -	return ret;
> -}
> -
> -static void iio_device_unregister_eventset(struct iio_dev *indio_dev)
> -{
> -	if (indio_dev->event_interface == NULL)
> -		return;
> -	__iio_remove_event_config_attrs(indio_dev);
> -	kfree(indio_dev->event_interface->group.attrs);
> -	kfree(indio_dev->event_interface);
> -}
> -
>  static void iio_dev_release(struct device *device)
>  {
>  	struct iio_dev *indio_dev = container_of(device, struct iio_dev, dev);
> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
> new file mode 100644
> index 0000000..a7b345d
> --- /dev/null
> +++ b/drivers/staging/iio/industrialio-event.c
> @@ -0,0 +1,483 @@
> +/* Industrial I/O event handling
> + *
> + * Copyright (c) 2008 Jonathan Cameron
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Based on elements of hwmon and input subsystems.
> + */
> +
> +#include <linux/anon_inodes.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/wait.h>
> +#include "iio.h"
> +#include "iio_core.h"
> +#include "sysfs.h"
> +#include "events.h"
> +
> +/**
> + * struct iio_detected_event_list - list element for events that have occurred
> + * @list:		linked list header
> + * @ev:			the event itself
> + */
> +struct iio_detected_event_list {
> +	struct list_head		list;
> +	struct iio_event_data		ev;
> +};
> +
> +/**
> + * struct iio_event_interface - chrdev interface for an event line
> + * @wait:		wait queue to allow blocking reads of events
> + * @event_list_lock:	mutex to protect the list of detected events
> + * @det_events:		list of detected events
> + * @max_events:		maximum number of events before new ones are dropped
> + * @current_events:	number of events in detected list
> + * @dev_attr_list:	list of event interface sysfs attribute
> + * @flags:		file operations related flags including busy flag.
> + * @group:		event interface sysfs attribute group
> + */
> +struct iio_event_interface {
> +	wait_queue_head_t	wait;
> +	struct mutex		event_list_lock;
> +	struct list_head	det_events;
> +	int			max_events;
> +	int			current_events;
> +	struct list_head	dev_attr_list;
> +	unsigned long		flags;
> +	struct attribute_group	group;
> +};
> +
> +int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
> +{
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_detected_event_list *ev;
> +	int ret = 0;
> +
> +	/* Does anyone care? */
> +	mutex_lock(&ev_int->event_list_lock);
> +	if (test_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> +		if (ev_int->current_events == ev_int->max_events) {
> +			mutex_unlock(&ev_int->event_list_lock);
> +			return 0;
> +		}
> +		ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> +		if (ev == NULL) {
> +			ret = -ENOMEM;
> +			mutex_unlock(&ev_int->event_list_lock);
> +			goto error_ret;
> +		}
> +		ev->ev.id = ev_code;
> +		ev->ev.timestamp = timestamp;
> +
> +		list_add_tail(&ev->list, &ev_int->det_events);
> +		ev_int->current_events++;
> +		mutex_unlock(&ev_int->event_list_lock);
> +		wake_up_interruptible(&ev_int->wait);
> +	} else
> +		mutex_unlock(&ev_int->event_list_lock);
> +
> +error_ret:
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_push_event);
> +
> +static ssize_t iio_event_chrdev_read(struct file *filep,
> +				     char __user *buf,
> +				     size_t count,
> +				     loff_t *f_ps)
> +{
> +	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_detected_event_list *el;
> +	size_t len = sizeof(el->ev);
> +	int ret;
> +
> +	if (count < len)
> +		return -EINVAL;
> +
> +	mutex_lock(&ev_int->event_list_lock);
> +	if (list_empty(&ev_int->det_events)) {
> +		if (filep->f_flags & O_NONBLOCK) {
> +			ret = -EAGAIN;
> +			goto error_mutex_unlock;
> +		}
> +		mutex_unlock(&ev_int->event_list_lock);
> +		/* Blocking on device; waiting for something to be there */
> +		ret = wait_event_interruptible(ev_int->wait,
> +					       !list_empty(&ev_int
> +							   ->det_events));
> +		if (ret)
> +			goto error_ret;
> +		/* Single access device so no one else can get the data */
> +		mutex_lock(&ev_int->event_list_lock);
> +	}
> +
> +	el = list_first_entry(&ev_int->det_events,
> +			      struct iio_detected_event_list,
> +			      list);
> +	if (copy_to_user(buf, &(el->ev), len)) {
> +		ret = -EFAULT;
> +		goto error_mutex_unlock;
> +	}
> +	list_del(&el->list);
> +	ev_int->current_events--;
> +	mutex_unlock(&ev_int->event_list_lock);
> +	kfree(el);
> +
> +	return len;
> +
> +error_mutex_unlock:
> +	mutex_unlock(&ev_int->event_list_lock);
> +error_ret:
> +
> +	return ret;
> +}
> +
> +static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
> +{
> +	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_detected_event_list *el, *t;
> +
> +	mutex_lock(&ev_int->event_list_lock);
> +	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> +	/*
> +	 * In order to maintain a clean state for reopening,
> +	 * clear out any awaiting events. The mask will prevent
> +	 * any new __iio_push_event calls running.
> +	 */
> +	list_for_each_entry_safe(el, t, &ev_int->det_events, list) {
> +		list_del(&el->list);
> +		kfree(el);
> +	}
> +	ev_int->current_events = 0;
> +	mutex_unlock(&ev_int->event_list_lock);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations iio_event_chrdev_fileops = {
> +	.read =  iio_event_chrdev_read,
> +	.release = iio_event_chrdev_release,
> +	.owner = THIS_MODULE,
> +	.llseek = noop_llseek,
> +};
> +
> +int iio_event_getfd(struct iio_dev *indio_dev)
> +{
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	int fd;
> +
> +	if (ev_int == NULL)
> +		return -ENODEV;
> +
> +	mutex_lock(&ev_int->event_list_lock);
> +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> +		mutex_unlock(&ev_int->event_list_lock);
> +		return -EBUSY;
> +	}
> +	mutex_unlock(&ev_int->event_list_lock);
> +	fd = anon_inode_getfd("iio:event",
> +				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
> +	if (fd < 0) {
> +		mutex_lock(&ev_int->event_list_lock);
> +		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> +		mutex_unlock(&ev_int->event_list_lock);
> +	}
> +	return fd;
> +}
> +
> +static const char * const iio_ev_type_text[] = {
> +	[IIO_EV_TYPE_THRESH] = "thresh",
> +	[IIO_EV_TYPE_MAG] = "mag",
> +	[IIO_EV_TYPE_ROC] = "roc",
> +	[IIO_EV_TYPE_THRESH_ADAPTIVE] = "thresh_adaptive",
> +	[IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
> +};
> +
> +static const char * const iio_ev_dir_text[] = {
> +	[IIO_EV_DIR_EITHER] = "either",
> +	[IIO_EV_DIR_RISING] = "rising",
> +	[IIO_EV_DIR_FALLING] = "falling"
> +};
> +
> +static ssize_t iio_ev_state_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf,
> +				  size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int ret;
> +	bool val;
> +
> +	ret = strtobool(buf, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = indio_dev->info->write_event_config(indio_dev,
> +						  this_attr->address,
> +						  val);
> +	return (ret < 0) ? ret : len;
> +}
> +
> +static ssize_t iio_ev_state_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int val = indio_dev->info->read_event_config(indio_dev,
> +						     this_attr->address);
> +
> +	if (val < 0)
> +		return val;
> +	else
> +		return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t iio_ev_value_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int val, ret;
> +
> +	ret = indio_dev->info->read_event_value(indio_dev,
> +						this_attr->address, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t iio_ev_value_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf,
> +				  size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	unsigned long val;
> +	int ret;
> +
> +	if (!indio_dev->info->write_event_value)
> +		return -EINVAL;
> +
> +	ret = strict_strtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = indio_dev->info->write_event_value(indio_dev, this_attr->address,
> +						 val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan)
> +{
> +	int ret = 0, i, attrcount = 0;
> +	u64 mask = 0;
> +	char *postfix;
> +	if (!chan->event_mask)
> +		return 0;
> +
> +	for_each_set_bit(i, &chan->event_mask, sizeof(chan->event_mask)*8) {
> +		postfix = kasprintf(GFP_KERNEL, "%s_%s_en",
> +				    iio_ev_type_text[i/IIO_EV_DIR_MAX],
> +				    iio_ev_dir_text[i%IIO_EV_DIR_MAX]);
> +		if (postfix == NULL) {
> +			ret = -ENOMEM;
> +			goto error_ret;
> +		}
> +		if (chan->modified)
> +			mask = IIO_MOD_EVENT_CODE(chan->type, 0, chan->channel,
> +						  i/IIO_EV_DIR_MAX,
> +						  i%IIO_EV_DIR_MAX);
> +		else if (chan->differential)
> +			mask = IIO_EVENT_CODE(chan->type,
> +					      0, 0,
> +					      i%IIO_EV_DIR_MAX,
> +					      i/IIO_EV_DIR_MAX,
> +					      0,
> +					      chan->channel,
> +					      chan->channel2);
> +		else
> +			mask = IIO_UNMOD_EVENT_CODE(chan->type,
> +						    chan->channel,
> +						    i/IIO_EV_DIR_MAX,
> +						    i%IIO_EV_DIR_MAX);
> +
> +		ret = __iio_add_chan_devattr(postfix,
> +					     chan,
> +					     &iio_ev_state_show,
> +					     iio_ev_state_store,
> +					     mask,
> +					     0,
> +					     &indio_dev->dev,
> +					     &indio_dev->event_interface->
> +					     dev_attr_list);
> +		kfree(postfix);
> +		if (ret)
> +			goto error_ret;
> +		attrcount++;
> +		postfix = kasprintf(GFP_KERNEL, "%s_%s_value",
> +				    iio_ev_type_text[i/IIO_EV_DIR_MAX],
> +				    iio_ev_dir_text[i%IIO_EV_DIR_MAX]);
> +		if (postfix == NULL) {
> +			ret = -ENOMEM;
> +			goto error_ret;
> +		}
> +		ret = __iio_add_chan_devattr(postfix, chan,
> +					     iio_ev_value_show,
> +					     iio_ev_value_store,
> +					     mask,
> +					     0,
> +					     &indio_dev->dev,
> +					     &indio_dev->event_interface->
> +					     dev_attr_list);
> +		kfree(postfix);
> +		if (ret)
> +			goto error_ret;
> +		attrcount++;
> +	}
> +	ret = attrcount;
> +error_ret:
> +	return ret;
> +}
> +
> +static inline void __iio_remove_event_config_attrs(struct iio_dev *indio_dev)
> +{
> +	struct iio_dev_attr *p, *n;
> +	list_for_each_entry_safe(p, n,
> +				 &indio_dev->event_interface->
> +				 dev_attr_list, l) {
> +		kfree(p->dev_attr.attr.name);
> +		kfree(p);
> +	}
> +}
> +
> +static inline int __iio_add_event_config_attrs(struct iio_dev *indio_dev)
> +{
> +	int j, ret, attrcount = 0;
> +
> +	INIT_LIST_HEAD(&indio_dev->event_interface->dev_attr_list);
> +	/* Dynically created from the channels array */
> +	for (j = 0; j < indio_dev->num_channels; j++) {
> +		ret = iio_device_add_event_sysfs(indio_dev,
> +						 &indio_dev->channels[j]);
> +		if (ret < 0)
> +			goto error_clear_attrs;
> +		attrcount += ret;
> +	}
> +	return attrcount;
> +
> +error_clear_attrs:
> +	__iio_remove_event_config_attrs(indio_dev);
> +
> +	return ret;
> +}
> +
> +static bool iio_check_for_dynamic_events(struct iio_dev *indio_dev)
> +{
> +	int j;
> +
> +	for (j = 0; j < indio_dev->num_channels; j++)
> +		if (indio_dev->channels[j].event_mask != 0)
> +			return true;
> +	return false;
> +}
> +
> +static void iio_setup_ev_int(struct iio_event_interface *ev_int)
> +{
> +	mutex_init(&ev_int->event_list_lock);
> +	/* discussion point - make this variable? */
> +	ev_int->max_events = 10;
> +	ev_int->current_events = 0;
> +	INIT_LIST_HEAD(&ev_int->det_events);
> +	init_waitqueue_head(&ev_int->wait);
> +}
> +
> +static const char *iio_event_group_name = "events";
> +int iio_device_register_eventset(struct iio_dev *indio_dev)
> +{
> +	struct iio_dev_attr *p;
> +	int ret = 0, attrcount_orig = 0, attrcount, attrn;
> +	struct attribute **attr;
> +
> +	if (!(indio_dev->info->event_attrs ||
> +	      iio_check_for_dynamic_events(indio_dev)))
> +		return 0;
> +
> +	indio_dev->event_interface =
> +		kzalloc(sizeof(struct iio_event_interface), GFP_KERNEL);
> +	if (indio_dev->event_interface == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	iio_setup_ev_int(indio_dev->event_interface);
> +	if (indio_dev->info->event_attrs != NULL) {
> +		attr = indio_dev->info->event_attrs->attrs;
> +		while (*attr++ != NULL)
> +			attrcount_orig++;
> +	}
> +	attrcount = attrcount_orig;
> +	if (indio_dev->channels) {
> +		ret = __iio_add_event_config_attrs(indio_dev);
> +		if (ret < 0)
> +			goto error_free_setup_event_lines;
> +		attrcount += ret;
> +	}
> +
> +	indio_dev->event_interface->group.name = iio_event_group_name;
> +	indio_dev->event_interface->group.attrs = kcalloc(attrcount + 1,
> +							  sizeof(indio_dev->event_interface->group.attrs[0]),
> +							  GFP_KERNEL);
> +	if (indio_dev->event_interface->group.attrs == NULL) {
> +		ret = -ENOMEM;
> +		goto error_free_setup_event_lines;
> +	}
> +	if (indio_dev->info->event_attrs)
> +		memcpy(indio_dev->event_interface->group.attrs,
> +		       indio_dev->info->event_attrs->attrs,
> +		       sizeof(indio_dev->event_interface->group.attrs[0])
> +		       *attrcount_orig);
> +	attrn = attrcount_orig;
> +	/* Add all elements from the list. */
> +	list_for_each_entry(p,
> +			    &indio_dev->event_interface->dev_attr_list,
> +			    l)
> +		indio_dev->event_interface->group.attrs[attrn++] =
> +			&p->dev_attr.attr;
> +	indio_dev->groups[indio_dev->groupcounter++] =
> +		&indio_dev->event_interface->group;
> +
> +	return 0;
> +
> +error_free_setup_event_lines:
> +	__iio_remove_event_config_attrs(indio_dev);
> +	kfree(indio_dev->event_interface);
> +error_ret:
> +
> +	return ret;
> +}
> +
> +void iio_device_unregister_eventset(struct iio_dev *indio_dev)
> +{
> +	if (indio_dev->event_interface == NULL)
> +		return;
> +	__iio_remove_event_config_attrs(indio_dev);
> +	kfree(indio_dev->event_interface->group.attrs);
> +	kfree(indio_dev->event_interface);
> +}

Re: [PATCH v2 3/6] staging:iio:events: Use kfifo for event queue

[Jonathan Cameron]

On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
> The current IIO event code uses a list to emulate FIFO like behaviour.
> Just use a kfifo directly instead to implement the event queue. As part of this
> patch the maximum of events in the queue is increased from 10 to 16 since kfifo
> requires a power of two for the number of fifo elements.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/industrialio-event.c |   88 +++++++-----------------------
>  1 files changed, 21 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
> index a7b345d..d63aa0b 100644
> --- a/drivers/staging/iio/industrialio-event.c
> +++ b/drivers/staging/iio/industrialio-event.c
> @@ -13,6 +13,7 @@
>  #include <linux/device.h>
>  #include <linux/fs.h>
>  #include <linux/kernel.h>
> +#include <linux/kfifo.h>
>  #include <linux/module.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -24,22 +25,10 @@
>  #include "events.h"
>  
>  /**
> - * struct iio_detected_event_list - list element for events that have occurred
> - * @list:		linked list header
> - * @ev:			the event itself
> - */
> -struct iio_detected_event_list {
> -	struct list_head		list;
> -	struct iio_event_data		ev;
> -};
> -
> -/**
>   * struct iio_event_interface - chrdev interface for an event line
>   * @wait:		wait queue to allow blocking reads of events
>   * @event_list_lock:	mutex to protect the list of detected events
>   * @det_events:		list of detected events
> - * @max_events:		maximum number of events before new ones are dropped
> - * @current_events:	number of events in detected list
>   * @dev_attr_list:	list of event interface sysfs attribute
>   * @flags:		file operations related flags including busy flag.
>   * @group:		event interface sysfs attribute group
> @@ -47,9 +36,8 @@ struct iio_detected_event_list {
>  struct iio_event_interface {
>  	wait_queue_head_t	wait;
>  	struct mutex		event_list_lock;
> -	struct list_head	det_events;
> -	int			max_events;
> -	int			current_events;
> +	DECLARE_KFIFO(det_events, struct iio_event_data, 16);
> +
>  	struct list_head	dev_attr_list;
>  	unsigned long		flags;
>  	struct attribute_group	group;
> @@ -58,34 +46,25 @@ struct iio_event_interface {
>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
>  {
>  	struct iio_event_interface *ev_int = indio_dev->event_interface;
> -	struct iio_detected_event_list *ev;
> -	int ret = 0;
> +	struct iio_event_data ev;
> +	int copied;
>  
>  	/* Does anyone care? */
>  	mutex_lock(&ev_int->event_list_lock);
>  	if (test_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> -		if (ev_int->current_events == ev_int->max_events) {
> -			mutex_unlock(&ev_int->event_list_lock);
> -			return 0;
> -		}
> -		ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> -		if (ev == NULL) {
> -			ret = -ENOMEM;
> -			mutex_unlock(&ev_int->event_list_lock);
> -			goto error_ret;
> -		}
> -		ev->ev.id = ev_code;
> -		ev->ev.timestamp = timestamp;
>  
> -		list_add_tail(&ev->list, &ev_int->det_events);
> -		ev_int->current_events++;
> +		ev.id = ev_code;
> +		ev.timestamp = timestamp;
> +
> +		copied = kfifo_put(&ev_int->det_events, &ev);
> +
>  		mutex_unlock(&ev_int->event_list_lock);
> -		wake_up_interruptible(&ev_int->wait);
> +		if (copied != 0)
> +			wake_up_interruptible(&ev_int->wait);
>  	} else
>  		mutex_unlock(&ev_int->event_list_lock);
>  
> -error_ret:
> -	return ret;
> +	return 0;
>  }
>  EXPORT_SYMBOL(iio_push_event);
>  
> @@ -95,15 +74,14 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  				     loff_t *f_ps)
>  {
>  	struct iio_event_interface *ev_int = filep->private_data;
> -	struct iio_detected_event_list *el;
> -	size_t len = sizeof(el->ev);
> +	unsigned int copied;
>  	int ret;
>  
> -	if (count < len)
> +	if (count < sizeof(struct iio_event_data))
>  		return -EINVAL;
>  
>  	mutex_lock(&ev_int->event_list_lock);
> -	if (list_empty(&ev_int->det_events)) {
> +	if (kfifo_is_empty(&ev_int->det_events)) {
>  		if (filep->f_flags & O_NONBLOCK) {
>  			ret = -EAGAIN;
>  			goto error_mutex_unlock;
> @@ -111,39 +89,25 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  		mutex_unlock(&ev_int->event_list_lock);
>  		/* Blocking on device; waiting for something to be there */
>  		ret = wait_event_interruptible(ev_int->wait,
> -					       !list_empty(&ev_int
> -							   ->det_events));
> +					!kfifo_is_empty(&ev_int->det_events));
>  		if (ret)
>  			goto error_ret;
>  		/* Single access device so no one else can get the data */
>  		mutex_lock(&ev_int->event_list_lock);
>  	}
>  
> -	el = list_first_entry(&ev_int->det_events,
> -			      struct iio_detected_event_list,
> -			      list);
> -	if (copy_to_user(buf, &(el->ev), len)) {
> -		ret = -EFAULT;
> -		goto error_mutex_unlock;
> -	}
> -	list_del(&el->list);
> -	ev_int->current_events--;
>  	mutex_unlock(&ev_int->event_list_lock);
> -	kfree(el);
> -
> -	return len;
> +	ret = kfifo_to_user(&ev_int->det_events, buf, count, &copied);
>  
>  error_mutex_unlock:
>  	mutex_unlock(&ev_int->event_list_lock);
>  error_ret:
> -
> -	return ret;
> +	return ret ? ret : copied;
>  }
>  
>  static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  {
>  	struct iio_event_interface *ev_int = filep->private_data;
> -	struct iio_detected_event_list *el, *t;
>  
>  	mutex_lock(&ev_int->event_list_lock);
>  	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> @@ -152,11 +116,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  	 * clear out any awaiting events. The mask will prevent
>  	 * any new __iio_push_event calls running.
>  	 */
> -	list_for_each_entry_safe(el, t, &ev_int->det_events, list) {
> -		list_del(&el->list);
> -		kfree(el);
> -	}
> -	ev_int->current_events = 0;
> +	kfifo_reset_out(&ev_int->det_events);
>  	mutex_unlock(&ev_int->event_list_lock);
>  
>  	return 0;
> @@ -268,9 +228,6 @@ static ssize_t iio_ev_value_store(struct device *dev,
>  	unsigned long val;
>  	int ret;
>  
> -	if (!indio_dev->info->write_event_value)
> -		return -EINVAL;
> -
>  	ret = strict_strtoul(buf, 10, &val);
>  	if (ret)
>  		return ret;
> @@ -401,10 +358,7 @@ static bool iio_check_for_dynamic_events(struct iio_dev *indio_dev)
>  static void iio_setup_ev_int(struct iio_event_interface *ev_int)
>  {
>  	mutex_init(&ev_int->event_list_lock);
> -	/* discussion point - make this variable? */
> -	ev_int->max_events = 10;
> -	ev_int->current_events = 0;
> -	INIT_LIST_HEAD(&ev_int->det_events);
> +	INIT_KFIFO(ev_int->det_events);
>  	init_waitqueue_head(&ev_int->wait);
>  }
>  

Re: [PATCH v2 4/6] staging:iio:events: Use waitqueue lock to protect event queue

[Jonathan Cameron]

On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
> Use the waitqueue lock to protect the event queue instead of a custom mutex.
> This has the advantage that we can call the waitqueue operations with the lock
> held, which simplifies the code flow a bit.
> 
Unusual ordering of this.  usually your sign off first then everyone
elses whatever below.
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/staging/iio/industrialio-event.c |   45 ++++++++++++-----------------
>  1 files changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
> index d63aa0b..b5f7a2c 100644
> --- a/drivers/staging/iio/industrialio-event.c
> +++ b/drivers/staging/iio/industrialio-event.c
> @@ -34,8 +34,7 @@
>   * @group:		event interface sysfs attribute group
>   */
>  struct iio_event_interface {
> -	wait_queue_head_t	wait;
> -	struct mutex		event_list_lock;
> +	wait_queue_head_t			wait;
>  	DECLARE_KFIFO(det_events, struct iio_event_data, 16);
>  
>  	struct list_head	dev_attr_list;
> @@ -50,19 +49,17 @@ int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
>  	int copied;
>  
>  	/* Does anyone care? */
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	if (test_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>  
>  		ev.id = ev_code;
>  		ev.timestamp = timestamp;
>  
>  		copied = kfifo_put(&ev_int->det_events, &ev);
> -
> -		mutex_unlock(&ev_int->event_list_lock);
>  		if (copied != 0)
> -			wake_up_interruptible(&ev_int->wait);
> -	} else
> -		mutex_unlock(&ev_int->event_list_lock);
> +			wake_up_locked(&ev_int->wait);
> +	}
> +	spin_unlock(&ev_int->wait.lock);
>  
>  	return 0;
>  }
> @@ -80,28 +77,25 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  	if (count < sizeof(struct iio_event_data))
>  		return -EINVAL;
>  
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	if (kfifo_is_empty(&ev_int->det_events)) {
>  		if (filep->f_flags & O_NONBLOCK) {
>  			ret = -EAGAIN;
> -			goto error_mutex_unlock;
> +			goto error_unlock;
>  		}
> -		mutex_unlock(&ev_int->event_list_lock);
>  		/* Blocking on device; waiting for something to be there */
> -		ret = wait_event_interruptible(ev_int->wait,
> +		ret = wait_event_interruptible_locked(ev_int->wait,
>  					!kfifo_is_empty(&ev_int->det_events));
>  		if (ret)
> -			goto error_ret;
> +			goto error_unlock;
>  		/* Single access device so no one else can get the data */
> -		mutex_lock(&ev_int->event_list_lock);
>  	}
>  
> -	mutex_unlock(&ev_int->event_list_lock);
>  	ret = kfifo_to_user(&ev_int->det_events, buf, count, &copied);
>  
> -error_mutex_unlock:
> -	mutex_unlock(&ev_int->event_list_lock);
> -error_ret:
> +error_unlock:
> +	spin_unlock(&ev_int->wait.lock);
> +
>  	return ret ? ret : copied;
>  }
>  
> @@ -109,7 +103,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  {
>  	struct iio_event_interface *ev_int = filep->private_data;
>  
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  	/*
>  	 * In order to maintain a clean state for reopening,
> @@ -117,7 +111,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  	 * any new __iio_push_event calls running.
>  	 */
>  	kfifo_reset_out(&ev_int->det_events);
> -	mutex_unlock(&ev_int->event_list_lock);
> +	spin_unlock(&ev_int->wait.lock);
>  
>  	return 0;
>  }
> @@ -137,18 +131,18 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>  	if (ev_int == NULL)
>  		return -ENODEV;
>  
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> -		mutex_unlock(&ev_int->event_list_lock);
> +		spin_unlock(&ev_int->wait.lock);
>  		return -EBUSY;
>  	}
> -	mutex_unlock(&ev_int->event_list_lock);
> +	spin_unlock(&ev_int->wait.lock);
>  	fd = anon_inode_getfd("iio:event",
>  				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
>  	if (fd < 0) {
> -		mutex_lock(&ev_int->event_list_lock);
> +		spin_lock(&ev_int->wait.lock);
>  		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> -		mutex_unlock(&ev_int->event_list_lock);
> +		spin_unlock(&ev_int->wait.lock);
>  	}
>  	return fd;
>  }
> @@ -357,7 +351,6 @@ static bool iio_check_for_dynamic_events(struct iio_dev *indio_dev)
>  
>  static void iio_setup_ev_int(struct iio_event_interface *ev_int)
>  {
> -	mutex_init(&ev_int->event_list_lock);
>  	INIT_KFIFO(ev_int->det_events);
>  	init_waitqueue_head(&ev_int->wait);
>  }

Re: [PATCH v2 6/6] staging:iio:events: Use non-atmoic bitops

[Jonathan Cameron]

On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
> We always hold the waitqueue lock when modifying the flags field. So it is safe
> to use the non-atomic bitops here instead of the atomic versions.
> 
> The lock has to be held, because we need to clear the busy flag and flush the
> event fifo in one atomic operation when closing the event file descriptor.
Good spot.  Glad to be rid of these.  Thanks for doing this series, all
excellent tidying up and general reworking!
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/industrialio-event.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
> index 58243d9..5e461e1 100644
> --- a/drivers/staging/iio/industrialio-event.c
> +++ b/drivers/staging/iio/industrialio-event.c
> @@ -124,7 +124,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  	struct iio_event_interface *ev_int = filep->private_data;
>  
>  	spin_lock(&ev_int->wait.lock);
> -	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> +	__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  	/*
>  	 * In order to maintain a clean state for reopening,
>  	 * clear out any awaiting events. The mask will prevent
> @@ -153,7 +153,7 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>  		return -ENODEV;
>  
>  	spin_lock(&ev_int->wait.lock);
> -	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> +	if (__test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>  		spin_unlock(&ev_int->wait.lock);
>  		return -EBUSY;
>  	}
> @@ -162,7 +162,7 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>  				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
>  	if (fd < 0) {
>  		spin_lock(&ev_int->wait.lock);
> -		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> +		__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  		spin_unlock(&ev_int->wait.lock);
>  	}
>  	return fd;

Re: [PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Lars-Peter Clausen]

On 12/19/2011 09:55 PM, Jonathan Cameron wrote:
> On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
>> The documentation for the iio_event_interface does not match the actual struct
>> anymore. This patch removes the documentation for non-existing fields and adds
>> documentation for missing fields.
>>
> Thanks for cleaning that up.  We could add that the dev_attr_list is for
> internal management only but I guess that easily established from
> how it is use...

All of the fields are more or less internal, since the struct is only defined
in this c file, so don't know if it makes sense to explicitly state it for
dev_attr_list.

>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>>  drivers/staging/iio/industrialio-core.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
>> index 2eef85f..23dd624 100644
>> --- a/drivers/staging/iio/industrialio-core.c
>> +++ b/drivers/staging/iio/industrialio-core.c
>> @@ -112,13 +112,14 @@ struct iio_detected_event_list {
>>  
>>  /**
>>   * struct iio_event_interface - chrdev interface for an event line
>> - * @dev:		device assocated with event interface
>>   * @wait:		wait queue to allow blocking reads of events
>>   * @event_list_lock:	mutex to protect the list of detected events
>>   * @det_events:		list of detected events
>>   * @max_events:		maximum number of events before new ones are dropped
>>   * @current_events:	number of events in detected list
>> + * @dev_attr_list:	list of event interface sysfs attribute
>>   * @flags:		file operations related flags including busy flag.
>> + * @group:		event interface sysfs attribute group
>>   */
>>  struct iio_event_interface {
>>  	wait_queue_head_t			wait;
> 

Re: [PATCH v2 4/6] staging:iio:events: Use waitqueue lock to protect event queue

[Lars-Peter Clausen]

On 12/19/2011 09:57 PM, Jonathan Cameron wrote:
> On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
>> Use the waitqueue lock to protect the event queue instead of a custom mutex.
>> This has the advantage that we can call the waitqueue operations with the lock
>> held, which simplifies the code flow a bit.
>>
> Unusual ordering of this.  usually your sign off first then everyone
> elses whatever below.

This is mainly due to 'git commit --amend -s' adding another Signed-off-by if
your Signed-off-by is not the last line in the commit message and me getting
annoyed at it ;)

>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  drivers/staging/iio/industrialio-event.c |   45 ++++++++++++-----------------
>>  1 files changed, 19 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
>> index d63aa0b..b5f7a2c 100644
>> --- a/drivers/staging/iio/industrialio-event.c
>> +++ b/drivers/staging/iio/industrialio-event.c
>> @@ -34,8 +34,7 @@
>>   * @group:		event interface sysfs attribute group
>>   */
>>  struct iio_event_interface {
>> -	wait_queue_head_t	wait;
>> -	struct mutex		event_list_lock;
>> +	wait_queue_head_t			wait;
>>  	DECLARE_KFIFO(det_events, struct iio_event_data, 16);
>>  
>>  	struct list_head	dev_attr_list;
>> @@ -50,19 +49,17 @@ int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
>>  	int copied;
>>  
>>  	/* Does anyone care? */
>> -	mutex_lock(&ev_int->event_list_lock);
>> +	spin_lock(&ev_int->wait.lock);
>>  	if (test_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>>  
>>  		ev.id = ev_code;
>>  		ev.timestamp = timestamp;
>>  
>>  		copied = kfifo_put(&ev_int->det_events, &ev);
>> -
>> -		mutex_unlock(&ev_int->event_list_lock);
>>  		if (copied != 0)
>> -			wake_up_interruptible(&ev_int->wait);
>> -	} else
>> -		mutex_unlock(&ev_int->event_list_lock);
>> +			wake_up_locked(&ev_int->wait);
>> +	}
>> +	spin_unlock(&ev_int->wait.lock);
>>  
>>  	return 0;
>>  }
>> @@ -80,28 +77,25 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>>  	if (count < sizeof(struct iio_event_data))
>>  		return -EINVAL;
>>  
>> -	mutex_lock(&ev_int->event_list_lock);
>> +	spin_lock(&ev_int->wait.lock);
>>  	if (kfifo_is_empty(&ev_int->det_events)) {
>>  		if (filep->f_flags & O_NONBLOCK) {
>>  			ret = -EAGAIN;
>> -			goto error_mutex_unlock;
>> +			goto error_unlock;
>>  		}
>> -		mutex_unlock(&ev_int->event_list_lock);
>>  		/* Blocking on device; waiting for something to be there */
>> -		ret = wait_event_interruptible(ev_int->wait,
>> +		ret = wait_event_interruptible_locked(ev_int->wait,
>>  					!kfifo_is_empty(&ev_int->det_events));
>>  		if (ret)
>> -			goto error_ret;
>> +			goto error_unlock;
>>  		/* Single access device so no one else can get the data */
>> -		mutex_lock(&ev_int->event_list_lock);
>>  	}
>>  
>> -	mutex_unlock(&ev_int->event_list_lock);
>>  	ret = kfifo_to_user(&ev_int->det_events, buf, count, &copied);
>>  
>> -error_mutex_unlock:
>> -	mutex_unlock(&ev_int->event_list_lock);
>> -error_ret:
>> +error_unlock:
>> +	spin_unlock(&ev_int->wait.lock);
>> +
>>  	return ret ? ret : copied;
>>  }
>>  
>> @@ -109,7 +103,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>>  {
>>  	struct iio_event_interface *ev_int = filep->private_data;
>>  
>> -	mutex_lock(&ev_int->event_list_lock);
>> +	spin_lock(&ev_int->wait.lock);
>>  	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>>  	/*
>>  	 * In order to maintain a clean state for reopening,
>> @@ -117,7 +111,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>>  	 * any new __iio_push_event calls running.
>>  	 */
>>  	kfifo_reset_out(&ev_int->det_events);
>> -	mutex_unlock(&ev_int->event_list_lock);
>> +	spin_unlock(&ev_int->wait.lock);
>>  
>>  	return 0;
>>  }
>> @@ -137,18 +131,18 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>>  	if (ev_int == NULL)
>>  		return -ENODEV;
>>  
>> -	mutex_lock(&ev_int->event_list_lock);
>> +	spin_lock(&ev_int->wait.lock);
>>  	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>> -		mutex_unlock(&ev_int->event_list_lock);
>> +		spin_unlock(&ev_int->wait.lock);
>>  		return -EBUSY;
>>  	}
>> -	mutex_unlock(&ev_int->event_list_lock);
>> +	spin_unlock(&ev_int->wait.lock);
>>  	fd = anon_inode_getfd("iio:event",
>>  				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
>>  	if (fd < 0) {
>> -		mutex_lock(&ev_int->event_list_lock);
>> +		spin_lock(&ev_int->wait.lock);
>>  		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>> -		mutex_unlock(&ev_int->event_list_lock);
>> +		spin_unlock(&ev_int->wait.lock);
>>  	}
>>  	return fd;
>>  }
>> @@ -357,7 +351,6 @@ static bool iio_check_for_dynamic_events(struct iio_dev *indio_dev)
>>  
>>  static void iio_setup_ev_int(struct iio_event_interface *ev_int)
>>  {
>> -	mutex_init(&ev_int->event_list_lock);
>>  	INIT_KFIFO(ev_int->det_events);
>>  	init_waitqueue_head(&ev_int->wait);
>>  }
> 

Re: [PATCH v2 6/6] staging:iio:events: Use non-atmoic bitops

[Jonathan Cameron]

On 12/19/2011 08:59 PM, Jonathan Cameron wrote:
> On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
>> We always hold the waitqueue lock when modifying the flags field. So it is safe
>> to use the non-atomic bitops here instead of the atomic versions.
>>
>> The lock has to be held, because we need to clear the busy flag and flush the
>> event fifo in one atomic operation when closing the event file descriptor.
> Good spot.  Glad to be rid of these.  Thanks for doing this series, all
> excellent tidying up and general reworking!
Have these gone to Greg?  Sorry, have lost track.

If not, atmoic -> atomic in the patch title!

>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>>  drivers/staging/iio/industrialio-event.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
>> index 58243d9..5e461e1 100644
>> --- a/drivers/staging/iio/industrialio-event.c
>> +++ b/drivers/staging/iio/industrialio-event.c
>> @@ -124,7 +124,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>>  	struct iio_event_interface *ev_int = filep->private_data;
>>  
>>  	spin_lock(&ev_int->wait.lock);
>> -	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>> +	__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>>  	/*
>>  	 * In order to maintain a clean state for reopening,
>>  	 * clear out any awaiting events. The mask will prevent
>> @@ -153,7 +153,7 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>>  		return -ENODEV;
>>  
>>  	spin_lock(&ev_int->wait.lock);
>> -	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>> +	if (__test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>>  		spin_unlock(&ev_int->wait.lock);
>>  		return -EBUSY;
>>  	}
>> @@ -162,7 +162,7 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>>  				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
>>  	if (fd < 0) {
>>  		spin_lock(&ev_int->wait.lock);
>> -		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>> +		__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>>  		spin_unlock(&ev_int->wait.lock);
>>  	}
>>  	return fd;
> 

Re: [PATCH v2 4/6] staging:iio:events: Use waitqueue lock to protect event queue

[Jonathan Cameron]

On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
> Use the waitqueue lock to protect the event queue instead of a custom mutex.
> This has the advantage that we can call the waitqueue operations with the lock
> held, which simplifies the code flow a bit.
> 
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Hmm.. I really should have noticed this before.. If this hasn't gone to
Greg already, can you also clear out the documentation for
event_list_lock within struct iio_event_interface.  Cleaner to do it
here than in a follow up patch.

diff --git a/drivers/staging/iio/industrialio-event.c
b/drivers/staging/iio/industrialio-event.c
index 5e461e1..0703d6d 100644
--- a/drivers/staging/iio/industrialio-event.c
+++ b/drivers/staging/iio/industrialio-event.c
@@ -28,7 +28,6 @@
 /**
  * struct iio_event_interface - chrdev interface for an event line
  * @wait:              wait queue to allow blocking reads of events
- * @event_list_lock:   mutex to protect the list of detected events
  * @det_events:                list of detected events
  * @dev_attr_list:     list of event interface sysfs attribute
  * @flags:             file operations related flags including busy flag.


> ---
>  drivers/staging/iio/industrialio-event.c |   45 ++++++++++++-----------------
>  1 files changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-event.c b/drivers/staging/iio/industrialio-event.c
> index d63aa0b..b5f7a2c 100644
> --- a/drivers/staging/iio/industrialio-event.c
> +++ b/drivers/staging/iio/industrialio-event.c
> @@ -34,8 +34,7 @@
>   * @group:		event interface sysfs attribute group
>   */
>  struct iio_event_interface {
> -	wait_queue_head_t	wait;
> -	struct mutex		event_list_lock;
> +	wait_queue_head_t			wait;
>  	DECLARE_KFIFO(det_events, struct iio_event_data, 16);
>  
>  	struct list_head	dev_attr_list;
> @@ -50,19 +49,17 @@ int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
>  	int copied;
>  
>  	/* Does anyone care? */
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	if (test_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
>  
>  		ev.id = ev_code;
>  		ev.timestamp = timestamp;
>  
>  		copied = kfifo_put(&ev_int->det_events, &ev);
> -
> -		mutex_unlock(&ev_int->event_list_lock);
>  		if (copied != 0)
> -			wake_up_interruptible(&ev_int->wait);
> -	} else
> -		mutex_unlock(&ev_int->event_list_lock);
> +			wake_up_locked(&ev_int->wait);
> +	}
> +	spin_unlock(&ev_int->wait.lock);
>  
>  	return 0;
>  }
> @@ -80,28 +77,25 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  	if (count < sizeof(struct iio_event_data))
>  		return -EINVAL;
>  
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	if (kfifo_is_empty(&ev_int->det_events)) {
>  		if (filep->f_flags & O_NONBLOCK) {
>  			ret = -EAGAIN;
> -			goto error_mutex_unlock;
> +			goto error_unlock;
>  		}
> -		mutex_unlock(&ev_int->event_list_lock);
>  		/* Blocking on device; waiting for something to be there */
> -		ret = wait_event_interruptible(ev_int->wait,
> +		ret = wait_event_interruptible_locked(ev_int->wait,
>  					!kfifo_is_empty(&ev_int->det_events));
>  		if (ret)
> -			goto error_ret;
> +			goto error_unlock;
>  		/* Single access device so no one else can get the data */
> -		mutex_lock(&ev_int->event_list_lock);
>  	}
>  
> -	mutex_unlock(&ev_int->event_list_lock);
>  	ret = kfifo_to_user(&ev_int->det_events, buf, count, &copied);
>  
> -error_mutex_unlock:
> -	mutex_unlock(&ev_int->event_list_lock);
> -error_ret:
> +error_unlock:
> +	spin_unlock(&ev_int->wait.lock);
> +
>  	return ret ? ret : copied;
>  }
>  
> @@ -109,7 +103,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  {
>  	struct iio_event_interface *ev_int = filep->private_data;
>  
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  	/*
>  	 * In order to maintain a clean state for reopening,
> @@ -117,7 +111,7 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  	 * any new __iio_push_event calls running.
>  	 */
>  	kfifo_reset_out(&ev_int->det_events);
> -	mutex_unlock(&ev_int->event_list_lock);
> +	spin_unlock(&ev_int->wait.lock);
>  
>  	return 0;
>  }
> @@ -137,18 +131,18 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>  	if (ev_int == NULL)
>  		return -ENODEV;
>  
> -	mutex_lock(&ev_int->event_list_lock);
> +	spin_lock(&ev_int->wait.lock);
>  	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> -		mutex_unlock(&ev_int->event_list_lock);
> +		spin_unlock(&ev_int->wait.lock);
>  		return -EBUSY;
>  	}
> -	mutex_unlock(&ev_int->event_list_lock);
> +	spin_unlock(&ev_int->wait.lock);
>  	fd = anon_inode_getfd("iio:event",
>  				&iio_event_chrdev_fileops, ev_int, O_RDONLY);
>  	if (fd < 0) {
> -		mutex_lock(&ev_int->event_list_lock);
> +		spin_lock(&ev_int->wait.lock);
>  		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> -		mutex_unlock(&ev_int->event_list_lock);
> +		spin_unlock(&ev_int->wait.lock);
>  	}
>  	return fd;
>  }
> @@ -357,7 +351,6 @@ static bool iio_check_for_dynamic_events(struct iio_dev *indio_dev)
>  
>  static void iio_setup_ev_int(struct iio_event_interface *ev_int)
>  {
> -	mutex_init(&ev_int->event_list_lock);
>  	INIT_KFIFO(ev_int->det_events);
>  	init_waitqueue_head(&ev_int->wait);
>  }

Re: [PATCH v2 4/6] staging:iio:events: Use waitqueue lock to protect event queue

[Lars-Peter Clausen]

On 01/29/2012 07:18 PM, Jonathan Cameron wrote:
> On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
>> Use the waitqueue lock to protect the event queue instead of a custom mutex.
>> This has the advantage that we can call the waitqueue operations with the lock
>> held, which simplifies the code flow a bit.
>>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Hmm.. I really should have noticed this before.. If this hasn't gone to
> Greg already, can you also clear out the documentation for
> event_list_lock within struct iio_event_interface.  Cleaner to do it
> here than in a follow up patch.

Yes, the whole series has already been send to Greg, but since he hasn't
applied it yet, I suppose I could resend it with your fixes.

Thanks,
- Lars

Re: [PATCH v2 4/6] staging:iio:events: Use waitqueue lock to protect event queue

[Jonathan Cameron]

On 01/31/2012 05:26 PM, Lars-Peter Clausen wrote:
> On 01/29/2012 07:18 PM, Jonathan Cameron wrote:
>> On 12/19/2011 10:29 AM, Lars-Peter Clausen wrote:
>>> Use the waitqueue lock to protect the event queue instead of a custom mutex.
>>> This has the advantage that we can call the waitqueue operations with the lock
>>> held, which simplifies the code flow a bit.
>>>
>>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> Hmm.. I really should have noticed this before.. If this hasn't gone to
>> Greg already, can you also clear out the documentation for
>> event_list_lock within struct iio_event_interface.  Cleaner to do it
>> here than in a follow up patch.
> 
> Yes, the whole series has already been send to Greg, but since he hasn't
> applied it yet, I suppose I could resend it with your fixes.
Either that or just send Greg a small follow up (or I'll get to it at
somepoint) None of it is remotely critical!

Re: [PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Lars-Peter Clausen]

On 02/09/2012 07:12 PM, Greg Kroah-Hartman wrote:
> On Wed, Feb 01, 2012 at 10:17:49PM +0100, Lars-Peter Clausen wrote:
>> The documentation for the iio_event_interface does not match the actual struct
>> anymore. This patch removes the documentation for non-existing fields and adds
>> documentation for missing fields.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>>  drivers/staging/iio/industrialio-core.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> This patch fails to apply to my tree, possibly because of your other
> patches I accepted.
> 
> Care to redo this series against the next linux-next release and resend?
> 

You've applied v1 of the series, but the diffwas only a minor documentation fix
anyway, so I'll sent it as an individual patch.

Thanks,
- Lars

Re: [PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Greg Kroah-Hartman]

On Wed, Feb 01, 2012 at 10:17:49PM +0100, Lars-Peter Clausen wrote:
> The documentation for the iio_event_interface does not match the actual struct
> anymore. This patch removes the documentation for non-existing fields and adds
> documentation for missing fields.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/industrialio-core.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

This patch fails to apply to my tree, possibly because of your other
patches I accepted.

Care to redo this series against the next linux-next release and resend?

thanks,

greg k-h

[PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Lars-Peter Clausen]

The documentation for the iio_event_interface does not match the actual struct
anymore. This patch removes the documentation for non-existing fields and adds
documentation for missing fields.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/industrialio-core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index 19f897f..580f373 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -112,13 +112,14 @@ struct iio_detected_event_list {
 
 /**
  * struct iio_event_interface - chrdev interface for an event line
- * @dev:		device assocated with event interface
  * @wait:		wait queue to allow blocking reads of events
  * @event_list_lock:	mutex to protect the list of detected events
  * @det_events:		list of detected events
  * @max_events:		maximum number of events before new ones are dropped
  * @current_events:	number of events in detected list
+ * @dev_attr_list:	list of event interface sysfs attribute
  * @flags:		file operations related flags including busy flag.
+ * @group:		event interface sysfs attribute group
  */
 struct iio_event_interface {
 	wait_queue_head_t			wait;
-- 
1.7.8.3

[PATCH v2 1/6] staging:iio: Update iio_event_interface documentation

[Lars-Peter Clausen]

The documentation for the iio_event_interface does not match the actual struct
anymore. This patch removes the documentation for non-existing fields and adds
documentation for missing fields.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/industrialio-core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index 19f897f..580f373 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -112,13 +112,14 @@ struct iio_detected_event_list {
 
 /**
  * struct iio_event_interface - chrdev interface for an event line
- * @dev:		device assocated with event interface
  * @wait:		wait queue to allow blocking reads of events
  * @event_list_lock:	mutex to protect the list of detected events
  * @det_events:		list of detected events
  * @max_events:		maximum number of events before new ones are dropped
  * @current_events:	number of events in detected list
+ * @dev_attr_list:	list of event interface sysfs attribute
  * @flags:		file operations related flags including busy flag.
+ * @group:		event interface sysfs attribute group
  */
 struct iio_event_interface {
 	wait_queue_head_t			wait;
-- 
1.7.8.3