[PATCH v5 0/4] Add initial configfs support for IIO

[PATCH v5 0/4] Add initial configfs support for IIO

[Daniel Baluta]

This patchset introduces IIO software triggers, offers a way of configuring
them via configfs and adds the IIO hrtimer based interrupt source to be used
with software triggers.

The arhitecture is now split in 3 parts, to remove all IIO trigger specific
parts from IIO configfs core:

(1) IIO software triggers - are independent of configfs.
(2) IIO configfs - offers a generic way of creating IIO objects. So far we can
	create software triggers.
(3) IIO hrtimer trigger - is the first interrupt source for software triggers
	(with syfs to follow). Each trigger type can implement its own set of
	attributes.

Changes since v4:
	* patch 1/4
		- fixed "new line" nit in industrialio-sw-trigger.c
		- added license header in sw_trigger.h
	* patch 2/4
		- none
	* patch 3/4 
		- none
	* patch 4/4
		- removed "Further work" chapter in iio_configfs.txt
		- added configfs-iio file in Documentation/ABI/testing

Changes since v3:
	* addressed comments from Jonathan for previous version
	* https://lkml.org/lkml/2015/4/6/111

Daniel Baluta (4):
  iio: core: Introduce IIO software triggers
  iio: core: Introduce IIO configfs support
  iio: trigger: Introduce IIO hrtimer based trigger
  iio: Documentation: Add IIO configfs documentation

 Documentation/ABI/testing/configfs-iio |  26 +++++
 Documentation/iio/iio_configfs.txt     |  63 +++++++++++
 drivers/iio/Kconfig                    |  16 +++
 drivers/iio/Makefile                   |   2 +
 drivers/iio/industrialio-configfs.c    | 117 +++++++++++++++++++
 drivers/iio/industrialio-sw-trigger.c  | 112 ++++++++++++++++++
 drivers/iio/trigger/Kconfig            |   9 ++
 drivers/iio/trigger/Makefile           |   2 +
 drivers/iio/trigger/iio-trig-hrtimer.c | 201 +++++++++++++++++++++++++++++++++
 include/linux/iio/sw_trigger.h         |  60 ++++++++++
 10 files changed, 608 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-iio
 create mode 100644 Documentation/iio/iio_configfs.txt
 create mode 100644 drivers/iio/industrialio-configfs.c
 create mode 100644 drivers/iio/industrialio-sw-trigger.c
 create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c
 create mode 100644 include/linux/iio/sw_trigger.h

-- 
1.9.1

[PATCH v5 1/4] iio: core: Introduce IIO software triggers

[Daniel Baluta]

A software trigger associates an IIO device trigger with a software
interrupt source (e.g: timer, sysfs). This patch adds the generic
infrastructure for handling software triggers.

Software interrupts sources are kept in a iio_trigger_types_list and
registered separately when the associated kernel module is loaded.

Software triggers can be created directly from drivers or from user
space via configfs interface.

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/Kconfig                   |   8 +++
 drivers/iio/Makefile                  |   1 +
 drivers/iio/industrialio-sw-trigger.c | 112 ++++++++++++++++++++++++++++++++++
 include/linux/iio/sw_trigger.h        |  60 ++++++++++++++++++
 4 files changed, 181 insertions(+)
 create mode 100644 drivers/iio/industrialio-sw-trigger.c
 create mode 100644 include/linux/iio/sw_trigger.h

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 4011eff..de7f1d9 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -58,6 +58,14 @@ config IIO_CONSUMERS_PER_TRIGGER
 	This value controls the maximum number of consumers that a
 	given trigger may handle. Default is 2.
 
+config IIO_SW_TRIGGER
+	bool "Enable software triggers support"
+	depends on IIO_TRIGGER
+	help
+	 Provides IIO core support for software triggers. A software
+	 trigger can be created via configfs or directly by a driver
+	 using the API provided.
+
 source "drivers/iio/accel/Kconfig"
 source "drivers/iio/adc/Kconfig"
 source "drivers/iio/amplifiers/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 698afc2..df87975 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_IIO) += industrialio.o
 industrialio-y := industrialio-core.o industrialio-event.o inkern.o
 industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
 industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
+industrialio-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
 industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
 
 obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
diff --git a/drivers/iio/industrialio-sw-trigger.c b/drivers/iio/industrialio-sw-trigger.c
new file mode 100644
index 0000000..f22aa63
--- /dev/null
+++ b/drivers/iio/industrialio-sw-trigger.c
@@ -0,0 +1,112 @@
+/*
+ * The Industrial I/O core, software trigger functions
+ *
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kmod.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+
+#include <linux/iio/sw_trigger.h>
+
+static LIST_HEAD(iio_trigger_types_list);
+static DEFINE_RWLOCK(iio_trigger_types_lock);
+
+static
+struct iio_sw_trigger_type *iio_find_sw_trigger_type(char *name, unsigned len)
+{
+	struct iio_sw_trigger_type *t = NULL, *iter;
+
+	list_for_each_entry(iter, &iio_trigger_types_list, list)
+		if (!strncmp(iter->name, name, len)) {
+			t = iter;
+			break;
+		}
+
+	return t;
+}
+
+int iio_register_sw_trigger_type(struct iio_sw_trigger_type *t)
+{
+	struct iio_sw_trigger_type *iter;
+	int ret = 0;
+
+	write_lock(&iio_trigger_types_lock);
+	iter = iio_find_sw_trigger_type(t->name, strlen(t->name));
+	if (iter)
+		ret = -EBUSY;
+	else
+		list_add_tail(&t->list, &iio_trigger_types_list);
+	write_unlock(&iio_trigger_types_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(iio_register_sw_trigger_type);
+
+int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *t)
+{
+	struct iio_sw_trigger_type *iter;
+	int ret = 0;
+
+	write_lock(&iio_trigger_types_lock);
+	iter = iio_find_sw_trigger_type(t->name, strlen(t->name));
+	if (!iter)
+		ret = -EINVAL;
+	else
+		list_del(&t->list);
+	write_unlock(&iio_trigger_types_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(iio_unregister_sw_trigger_type);
+
+static
+struct iio_sw_trigger_type *iio_get_sw_trigger_type(char *name)
+{
+	struct iio_sw_trigger_type *t;
+
+	read_lock(&iio_trigger_types_lock);
+	t = iio_find_sw_trigger_type(name, strlen(name));
+	if (t && !try_module_get(t->owner))
+		t = NULL;
+	read_unlock(&iio_trigger_types_lock);
+
+	return t;
+}
+
+struct iio_sw_trigger *iio_sw_trigger_create(char *type, char *name)
+{
+	struct iio_sw_trigger *t;
+	struct iio_sw_trigger_type *tt;
+
+	tt = iio_get_sw_trigger_type(type);
+	if (!tt) {
+		pr_err("Invalid trigger type: %s\n", type);
+		return ERR_PTR(-EINVAL);
+	}
+	t = tt->ops->probe(name);
+	if (IS_ERR(t))
+		goto out_module_put;
+
+	return t;
+out_module_put:
+	module_put(tt->owner);
+	return t;
+}
+EXPORT_SYMBOL(iio_sw_trigger_create);
+
+void iio_sw_trigger_destroy(struct iio_sw_trigger *t)
+{
+	struct iio_sw_trigger_type *tt = t->trigger_type;
+
+	tt->ops->remove(t);
+	module_put(tt->owner);
+}
+EXPORT_SYMBOL(iio_sw_trigger_destroy);
diff --git a/include/linux/iio/sw_trigger.h b/include/linux/iio/sw_trigger.h
new file mode 100644
index 0000000..4b41e3e
--- /dev/null
+++ b/include/linux/iio/sw_trigger.h
@@ -0,0 +1,60 @@
+/*
+ * Industrial I/O software trigger interface
+ *
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * 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.
+ */
+
+#ifndef __IIO_SW_TRIGGER
+#define __IIO_SW_TRIGGER
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/configfs.h>
+
+#define module_iio_sw_trigger_driver(__iio_sw_trigger_type) \
+	module_driver(__iio_sw_trigger_type, iio_register_sw_trigger_type, \
+		      iio_unregister_sw_trigger_type)
+
+struct iio_sw_trigger_ops;
+
+struct iio_sw_trigger_type {
+	char *name;
+	struct module *owner;
+	struct iio_sw_trigger_ops *ops;
+	struct list_head list;
+};
+
+struct iio_sw_trigger {
+	struct iio_trigger *trigger;
+	struct iio_sw_trigger_type *trigger_type;
+#ifdef CONFIG_CONFIGFS_FS
+	struct config_group group;
+#endif
+};
+
+struct iio_sw_trigger_ops {
+	struct iio_sw_trigger* (*probe)(const char *);
+	int (*remove)(struct iio_sw_trigger *);
+};
+
+int iio_register_sw_trigger_type(struct iio_sw_trigger_type *);
+int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *);
+
+struct iio_sw_trigger *iio_sw_trigger_create(char *, char *);
+void iio_sw_trigger_destroy(struct iio_sw_trigger *);
+
+#ifdef CONFIG_CONFIGFS_FS
+static inline
+struct iio_sw_trigger *to_iio_sw_trigger(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct iio_sw_trigger,
+			    group);
+}
+#endif
+
+#endif /* __IIO_SW_TRIGGER */
-- 
1.9.1

[PATCH v5 2/4] iio: core: Introduce IIO configfs support

[Daniel Baluta]

This creates an IIO configfs subystem named "iio", with a default "triggers"
group.

Triggers group is used for handling software triggers. To create a new software
trigger one must create a directory inside the trigger directory.

Software trigger name MUST follow the following convention:
	* <trigger-type>-<trigger-name>
Where:
	* <trigger_type>, specifies the interrupt source (e.g: hrtimer)
	* <trigger-name>, specifies the IIO device trigger name

Failing to follow this convention will result in an directory creation error.

E.g, assuming that hrtimer trigger type is registered with IIO software
trigger core:

$ mkdir /config/iio/triggers/hrtimer-instance1

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/Kconfig                 |   8 +++
 drivers/iio/Makefile                |   1 +
 drivers/iio/industrialio-configfs.c | 117 ++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 drivers/iio/industrialio-configfs.c

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index de7f1d9..c310156 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -18,6 +18,14 @@ config IIO_BUFFER
 	  Provide core support for various buffer based data
 	  acquisition methods.
 
+config IIO_CONFIGFS
+	tristate "Enable IIO configuration via configfs"
+	select CONFIGFS_FS
+	help
+	  This allows configuring various IIO bits through configfs
+	  (e.g. software triggers). For more info see
+	  Documentation/iio/iio_configfs.txt.
+
 if IIO_BUFFER
 
 config IIO_BUFFER_CB
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index df87975..31aead3 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -10,6 +10,7 @@ industrialio-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
 industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
 
 obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
+obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
 obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
 
 obj-y += accel/
diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
new file mode 100644
index 0000000..0361434
--- /dev/null
+++ b/drivers/iio/industrialio-configfs.c
@@ -0,0 +1,117 @@
+/*
+ * Industrial I/O configfs bits
+ *
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * 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.
+ */
+
+#include <linux/configfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kmod.h>
+#include <linux/slab.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sw_trigger.h>
+
+#define MAX_NAME_LEN 32
+
+static struct config_group *trigger_make_group(struct config_group *group,
+					       const char *name)
+{
+	char *type_name;
+	char *trigger_name;
+	char buf[MAX_NAME_LEN];
+	struct iio_sw_trigger *t;
+
+	snprintf(buf, MAX_NAME_LEN, "%s", name);
+
+	/* group name should have the form <trigger-type>-<trigger-name> */
+	type_name = buf;
+	trigger_name = strchr(buf, '-');
+	if (!trigger_name) {
+		pr_err("Unable to locate '-' in %s. Use <type>-<name>.\n", buf);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* replace - with \0, this nicely separates the two strings */
+	*trigger_name = '\0';
+	trigger_name++;
+
+	t = iio_sw_trigger_create(type_name, trigger_name);
+	if (IS_ERR(t))
+		return ERR_CAST(t);
+
+	config_item_set_name(&t->group.cg_item, name);
+
+	return &t->group;
+}
+
+static void trigger_drop_group(struct config_group *group,
+			       struct config_item *item)
+{
+	struct iio_sw_trigger *t = to_iio_sw_trigger(item);
+
+	if (t)
+		iio_sw_trigger_destroy(t);
+	config_item_put(item);
+}
+
+static struct configfs_group_operations triggers_ops = {
+	.make_group	= &trigger_make_group,
+	.drop_item	= &trigger_drop_group,
+};
+
+static struct config_item_type iio_triggers_group_type = {
+	.ct_group_ops = &triggers_ops,
+	.ct_owner       = THIS_MODULE,
+};
+
+static struct config_group iio_triggers_group = {
+	.cg_item = {
+		.ci_namebuf = "triggers",
+		.ci_type = &iio_triggers_group_type,
+	},
+};
+
+static struct config_group *iio_root_default_groups[] = {
+	&iio_triggers_group,
+	NULL
+};
+
+static struct config_item_type iio_root_group_type = {
+	.ct_owner       = THIS_MODULE,
+};
+
+static struct configfs_subsystem iio_configfs_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "iio",
+			.ci_type = &iio_root_group_type,
+		},
+		.default_groups = iio_root_default_groups,
+	},
+	.su_mutex = __MUTEX_INITIALIZER(iio_configfs_subsys.su_mutex),
+};
+
+static int __init iio_configfs_init(void)
+{
+	config_group_init(&iio_triggers_group);
+	config_group_init(&iio_configfs_subsys.su_group);
+
+	return configfs_register_subsystem(&iio_configfs_subsys);
+}
+module_init(iio_configfs_init);
+
+static void __exit iio_configfs_exit(void)
+{
+	configfs_unregister_subsystem(&iio_configfs_subsys);
+}
+module_exit(iio_configfs_exit);
+
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
+MODULE_DESCRIPTION("Industrial I/O configfs support");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Daniel Baluta]

This patch registers a new IIO software trigger interrupt source
based on high resolution timers.

Notice that if configfs is enabled we create sampling_frequency
attribute allowing users to change hrtimer period (1/sampling_frequency).

The IIO hrtimer trigger has a long history, this patch is based on
an older version from Marten and Lars-Peter.

Signed-off-by: Marten Svanfeldt <marten@intuitiveaerial.com>
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/trigger/Kconfig            |   9 ++
 drivers/iio/trigger/Makefile           |   2 +
 drivers/iio/trigger/iio-trig-hrtimer.c | 201 +++++++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c

diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
index 7999612..454665a 100644
--- a/drivers/iio/trigger/Kconfig
+++ b/drivers/iio/trigger/Kconfig
@@ -5,6 +5,15 @@
 
 menu "Triggers - standalone"
 
+config IIO_HRTIMER_TRIGGER
+	tristate "High resolution timer trigger"
+	select IIO_SW_TRIGGER
+	help
+	  Provides a frequency based IIO trigger using hrtimers.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called iio-trig-hrtimer.
+
 config IIO_INTERRUPT_TRIGGER
 	tristate "Generic interrupt trigger"
 	help
diff --git a/drivers/iio/trigger/Makefile b/drivers/iio/trigger/Makefile
index 0694dae..fe06eb5 100644
--- a/drivers/iio/trigger/Makefile
+++ b/drivers/iio/trigger/Makefile
@@ -3,5 +3,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+
+obj-$(CONFIG_IIO_HRTIMER_TRIGGER) += iio-trig-hrtimer.o
 obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o
 obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
diff --git a/drivers/iio/trigger/iio-trig-hrtimer.c b/drivers/iio/trigger/iio-trig-hrtimer.c
new file mode 100644
index 0000000..5077fad
--- /dev/null
+++ b/drivers/iio/trigger/iio-trig-hrtimer.c
@@ -0,0 +1,201 @@
+/**
+ * The industrial I/O periodic hrtimer trigger driver
+ *
+ * Copyright (C) Intuitive Aerial AB
+ * Written by Marten Svanfeldt, marten@intuitiveaerial.com
+ * Copyright (C) 2012, Analog Device Inc.
+ *	Author: Lars-Peter Clausen <lars@metafoo.de>
+ * Copyright (C) 2015, Intel Corporation
+ *
+ * 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.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/hrtimer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/sw_trigger.h>
+
+/* default sampling frequency - 100Hz */
+#define HRTIMER_DEFAULT_SAMPLING_FREQUENCY 100
+
+struct iio_hrtimer_info {
+	struct iio_sw_trigger swt;
+	struct hrtimer timer;
+	unsigned long sampling_frequency;
+	ktime_t period;
+};
+
+#ifdef CONFIG_CONFIGFS_FS
+struct iio_hrtimer_info *to_iio_hrtimer_info(struct config_item *item)
+{
+	return item ? container_of(to_iio_sw_trigger(item),
+				   struct iio_hrtimer_info, swt) : NULL;
+}
+
+CONFIGFS_ATTR_STRUCT(iio_hrtimer_info);
+
+#define IIO_HRTIMER_INFO_ATTR(_name, _mode, _show, _store) \
+	struct iio_hrtimer_info_attribute iio_hrtimer_attr_##_name = \
+	__CONFIGFS_ATTR(_name, _mode, _show, _store)
+
+static
+ssize_t iio_hrtimer_info_show_sampling_frequency(struct iio_hrtimer_info *info,
+						 char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%lu\n", info->sampling_frequency);
+}
+
+static
+ssize_t iio_hrtimer_info_store_sampling_frequency(struct iio_hrtimer_info *info,
+						  const char *page,
+						  size_t count)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(page, 10, &val);
+	if (ret)
+		return ret;
+
+	if (!val || val > NSEC_PER_SEC)
+		return -EINVAL;
+
+	info->sampling_frequency = val;
+	info->period = ktime_set(0, NSEC_PER_SEC / val);
+
+	return count;
+}
+
+IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
+		      iio_hrtimer_info_show_sampling_frequency,
+		      iio_hrtimer_info_store_sampling_frequency);
+
+static struct configfs_attribute *iio_hrtimer_attrs[] = {
+	&iio_hrtimer_attr_sampling_frequency.attr,
+	NULL
+};
+
+CONFIGFS_ATTR_OPS(iio_hrtimer_info);
+static struct configfs_item_operations iio_hrtimer_ops = {
+	.show_attribute		= iio_hrtimer_info_attr_show,
+	.store_attribute	= iio_hrtimer_info_attr_store,
+};
+
+static struct config_item_type iio_hrtimer_type = {
+	.ct_item_ops	= &iio_hrtimer_ops,
+	.ct_attrs	= iio_hrtimer_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+#endif /* CONFIGFS_FS */
+
+static enum hrtimer_restart iio_hrtimer_trig_handler(struct hrtimer *timer)
+{
+	struct iio_hrtimer_info *info;
+
+	info = container_of(timer, struct iio_hrtimer_info, timer);
+
+	hrtimer_forward_now(timer, info->period);
+	iio_trigger_poll(info->swt.trigger);
+
+	return HRTIMER_RESTART;
+}
+
+static int iio_trig_hrtimer_set_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_hrtimer_info *trig_info;
+
+	trig_info = iio_trigger_get_drvdata(trig);
+
+	if (state)
+		hrtimer_start(&trig_info->timer, trig_info->period,
+			      HRTIMER_MODE_REL);
+	else
+		hrtimer_cancel(&trig_info->timer);
+
+	return 0;
+}
+
+static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = iio_trig_hrtimer_set_state,
+};
+
+static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name)
+{
+	struct iio_hrtimer_info *trig_info;
+	int ret;
+
+	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
+	if (!trig_info)
+		return ERR_PTR(-ENOMEM);
+
+	trig_info->swt.trigger = iio_trigger_alloc("%s", name);
+	if (!trig_info->swt.trigger) {
+		ret = -ENOMEM;
+		goto err_free_trig_info;
+	}
+
+	iio_trigger_set_drvdata(trig_info->swt.trigger, trig_info);
+	trig_info->swt.trigger->ops = &iio_hrtimer_trigger_ops;
+
+	hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	trig_info->timer.function = iio_hrtimer_trig_handler;
+
+	trig_info->sampling_frequency = HRTIMER_DEFAULT_SAMPLING_FREQUENCY;
+	trig_info->period = ktime_set(0, NSEC_PER_SEC /
+				      trig_info->sampling_frequency);
+
+	ret = iio_trigger_register(trig_info->swt.trigger);
+	if (ret)
+		goto err_free_trigger;
+#ifdef CONFIG_CONFIGFS_FS
+	config_group_init_type_name(&trig_info->swt.group, name,
+				    &iio_hrtimer_type);
+#endif
+	return &trig_info->swt;
+err_free_trigger:
+	iio_trigger_free(trig_info->swt.trigger);
+err_free_trig_info:
+	kfree(trig_info);
+
+	return ERR_PTR(ret);
+}
+
+static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
+{
+	struct iio_hrtimer_info *trig_info;
+
+	trig_info = iio_trigger_get_drvdata(swt->trigger);
+
+	hrtimer_cancel(&trig_info->timer);
+
+	iio_trigger_unregister(swt->trigger);
+	iio_trigger_free(swt->trigger);
+	kfree(trig_info);
+
+	return 0;
+}
+
+struct iio_sw_trigger_ops iio_trig_hrtimer_ops = {
+	.probe		= iio_trig_hrtimer_probe,
+	.remove		= iio_trig_hrtimer_remove,
+};
+
+struct iio_sw_trigger_type iio_trig_hrtimer = {
+	.name = "hrtimer",
+	.owner = THIS_MODULE,
+	.ops = &iio_trig_hrtimer_ops,
+};
+
+module_iio_sw_trigger_driver(iio_trig_hrtimer);
+
+MODULE_AUTHOR("Marten Svanfeldt <marten@intuitiveaerial.com>");
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
+MODULE_DESCRIPTION("Periodic hrtimer trigger for the IIO subsystem");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v5 4/4] iio: Documentation: Add IIO configfs documentation

[Daniel Baluta]

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 Documentation/ABI/testing/configfs-iio | 26 ++++++++++++++
 Documentation/iio/iio_configfs.txt     | 63 ++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-iio
 create mode 100644 Documentation/iio/iio_configfs.txt

diff --git a/Documentation/ABI/testing/configfs-iio b/Documentation/ABI/testing/configfs-iio
new file mode 100644
index 0000000..a143135
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-iio
@@ -0,0 +1,26 @@
+What:		/config/iio
+Date:		May 2015
+KernelVersion:	4.2
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This represents Industrial IO configuration entry point
+		directory. It contains sub-groups corresponding to IIO
+		objects.
+
+What:		/config/iio/triggers
+Date:		May 2015
+KernelVersion:	4.2
+Description:
+		Industrial IO software triggers directory.
+
+What:		/config/iio/triggers/hrtimer-name
+Date:		May 2015
+KernelVersion:	4.2
+Description:
+		High resolution timer instance.
+
+What:		/config/iio/triggers/hrtimer-name/sampling_frequency
+Date:		May 2015
+KernelVersion:	4.2
+Description:
+		Sampling frequency for this hrtimer instance.
diff --git a/Documentation/iio/iio_configfs.txt b/Documentation/iio/iio_configfs.txt
new file mode 100644
index 0000000..456f7a5
--- /dev/null
+++ b/Documentation/iio/iio_configfs.txt
@@ -0,0 +1,63 @@
+Industrial IIO configfs support
+
+1. Overview
+
+Configfs is a filesystem-based manager of kernel objects. IIO uses some
+objects that could be easily configured using configfs (e.g.: devices,
+triggers).
+
+See Documentation/filesystems/configfs/configfs.txt for more information
+about how configfs works.
+
+2. Usage
+
+In order to use configfs support in IIO we need to select it at compile
+time via CONFIG_IIO_CONFIGFS config option.
+
+Then, mount the configfs filesystem (usually under /config directory):
+
+$ mkdir /config
+$ mount -t configfs none /config
+
+At this point, all default IIO groups will be created and can be accessed
+under /config/iio. Next chapters will describe available IIO configuration
+objects.
+
+3. Software triggers
+
+One of the IIO default configfs groups is the "triggers" groups. It is
+automagically accessible when the configfs is mounted and can be found
+under /config/iio/triggers.
+
+Software triggers are created under /config/iio/triggers directory. A sofware
+trigger name MUST be of the following form:
+	* <trigger-type>-<trigger-name>:
+Where:
+	* <trigger-type>, specifies the interrupt source (e.g: hrtimer)
+	* <trigger-name>, spefcifies the IIO device trigger name
+
+We support now to following interrupt sources (trigger types):
+	* hrtimer, uses high resolution timers as interrupt source
+
+3.1 Software triggers creation and destruction
+
+As simply as:
+
+$ mkdir /config/triggers/<trigger-type>-<trigger-name>
+$ rmdir /config/triggers/<trigger-type>-<trigger-name>
+e.g:
+
+$ mkdir /config/triggers/hrtimer-instance1
+$ rmdir /config/triggers/hrtimer-instance1
+
+Each trigger can have one or more attributes specific to the trigger type.
+
+3.2 "hrtimer" trigger types attributes
+
+"hrtimer" trigger type has only one attribute:
+
+$ ls /config/triggers/hrtimer-instance1
+sampling_frequency
+
+sampling_frequency - represents the period in Hz between two consecutive
+iio_trigger_poll calls. By default it is set to 100Hz.
-- 
1.9.1

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Lars-Peter Clausen]

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]
> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
> +		      iio_hrtimer_info_show_sampling_frequency,
> +		      iio_hrtimer_info_store_sampling_frequency);

I wonder if the sampling frequency should be configurable the regular IIO 
API, just like any other IIO device. But things like min/max sampling 
frequency should be configured in configfs.

[...]
> +#endif /* CONFIGFS_FS */
> +
[...]
> +static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name)
> +{
[...]
> +#ifdef CONFIG_CONFIGFS_FS
> +	config_group_init_type_name(&trig_info->swt.group, name,
> +				    &iio_hrtimer_type);
> +#endif

This should probably have a helper function in the sw trigger core, that 
gets stubbed out when CONFIG_FS is disabled. Otherwise we'll see the same 
#ifdef in every software trigger driver.
[...]
> +}
> +
> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
> +{
> +	struct iio_hrtimer_info *trig_info;
> +
> +	trig_info = iio_trigger_get_drvdata(swt->trigger);
> +
> +	hrtimer_cancel(&trig_info->timer);
> +
> +	iio_trigger_unregister(swt->trigger);
> +	iio_trigger_free(swt->trigger);

There is a bit of a race condition here. hrtimer_cancel() should be called 
between unregister and free, otherwise it might be re-armed before it is 
unregistered.

> +	kfree(trig_info);
> +
> +	return 0;
> +}
> +
> +struct iio_sw_trigger_ops iio_trig_hrtimer_ops = {

const

> +	.probe		= iio_trig_hrtimer_probe,
> +	.remove		= iio_trig_hrtimer_remove,
> +};
[...]

Re: [PATCH v5 2/4] iio: core: Introduce IIO configfs support

[Lars-Peter Clausen]

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
> This creates an IIO configfs subystem named "iio", with a default "triggers"
> group.
>
> Triggers group is used for handling software triggers. To create a new software
> trigger one must create a directory inside the trigger directory.
>
> Software trigger name MUST follow the following convention:
> 	* <trigger-type>-<trigger-name>
> Where:
> 	* <trigger_type>, specifies the interrupt source (e.g: hrtimer)
> 	* <trigger-name>, specifies the IIO device trigger name
>
> Failing to follow this convention will result in an directory creation error.
>
> E.g, assuming that hrtimer trigger type is registered with IIO software
> trigger core:
>
> $ mkdir /config/iio/triggers/hrtimer-instance1
>

Nice, short and clean. Looks pretty good. It's a bit of a shame that we 
can't have a per type directory, but if that's how configfs works I guess 
there is not much choice.

[...]
> +static struct config_group *trigger_make_group(struct config_group *group,
> +					       const char *name)
> +{
> +	char *type_name;
> +	char *trigger_name;
> +	char buf[MAX_NAME_LEN];
> +	struct iio_sw_trigger *t;
> +
> +	snprintf(buf, MAX_NAME_LEN, "%s", name);
> +
> +	/* group name should have the form <trigger-type>-<trigger-name> */
> +	type_name = buf;
> +	trigger_name = strchr(buf, '-');
> +	if (!trigger_name) {
> +		pr_err("Unable to locate '-' in %s. Use <type>-<name>.\n", buf);

Do we want to print this side channel message? Makes it pretty easy to spam 
the kernel log with a rouge application.

> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/* replace - with \0, this nicely separates the two strings */
> +	*trigger_name = '\0';
> +	trigger_name++;
> +
> +	t = iio_sw_trigger_create(type_name, trigger_name);
> +	if (IS_ERR(t))
> +		return ERR_CAST(t);
> +
> +	config_item_set_name(&t->group.cg_item, name);
> +
> +	return &t->group;
> +}
> +
> +static void trigger_drop_group(struct config_group *group,
> +			       struct config_item *item)
> +{
> +	struct iio_sw_trigger *t = to_iio_sw_trigger(item);
> +
> +	if (t)

t will never be NULL.

> +		iio_sw_trigger_destroy(t);
> +	config_item_put(item);
> +}

Re: [PATCH v5 1/4] iio: core: Introduce IIO software triggers

[Lars-Peter Clausen]

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
> A software trigger associates an IIO device trigger with a software
> interrupt source (e.g: timer, sysfs). This patch adds the generic
> infrastructure for handling software triggers.
>
> Software interrupts sources are kept in a iio_trigger_types_list and
> registered separately when the associated kernel module is loaded.
>
> Software triggers can be created directly from drivers or from user
> space via configfs interface.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
>   drivers/iio/Kconfig                   |   8 +++
>   drivers/iio/Makefile                  |   1 +
>   drivers/iio/industrialio-sw-trigger.c | 112 ++++++++++++++++++++++++++++++++++
>   include/linux/iio/sw_trigger.h        |  60 ++++++++++++++++++
>   4 files changed, 181 insertions(+)
>   create mode 100644 drivers/iio/industrialio-sw-trigger.c
>   create mode 100644 include/linux/iio/sw_trigger.h
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 4011eff..de7f1d9 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -58,6 +58,14 @@ config IIO_CONSUMERS_PER_TRIGGER
>   	This value controls the maximum number of consumers that a
>   	given trigger may handle. Default is 2.
>
> +config IIO_SW_TRIGGER
> +	bool "Enable software triggers support"
> +	depends on IIO_TRIGGER
> +	help
> +	 Provides IIO core support for software triggers. A software
> +	 trigger can be created via configfs or directly by a driver
> +	 using the API provided.
> +
>   source "drivers/iio/accel/Kconfig"
>   source "drivers/iio/adc/Kconfig"
>   source "drivers/iio/amplifiers/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 698afc2..df87975 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_IIO) += industrialio.o
>   industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>   industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>   industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> +industrialio-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
>   industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>
>   obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
> diff --git a/drivers/iio/industrialio-sw-trigger.c b/drivers/iio/industrialio-sw-trigger.c
> new file mode 100644
> index 0000000..f22aa63
> --- /dev/null
> +++ b/drivers/iio/industrialio-sw-trigger.c
> @@ -0,0 +1,112 @@
> +/*
> + * The Industrial I/O core, software trigger functions
> + *
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kmod.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/sw_trigger.h>
> +
> +static LIST_HEAD(iio_trigger_types_list);
> +static DEFINE_RWLOCK(iio_trigger_types_lock);
> +
> +static
> +struct iio_sw_trigger_type *iio_find_sw_trigger_type(char *name, unsigned len)

const char *name, there are a couple of other places where char * should be 
const char * below as well.

> +{
> +	struct iio_sw_trigger_type *t = NULL, *iter;
> +
> +	list_for_each_entry(iter, &iio_trigger_types_list, list)
> +		if (!strncmp(iter->name, name, len)) {
> +			t = iter;
> +			break;
> +		}
> +
> +	return t;
> +}
> +
> +int iio_register_sw_trigger_type(struct iio_sw_trigger_type *t)

Kernel doc would be nice for the public API

> +{
> +	struct iio_sw_trigger_type *iter;
> +	int ret = 0;
> +
> +	write_lock(&iio_trigger_types_lock);
> +	iter = iio_find_sw_trigger_type(t->name, strlen(t->name));
> +	if (iter)
> +		ret = -EBUSY;
> +	else
> +		list_add_tail(&t->list, &iio_trigger_types_list);
> +	write_unlock(&iio_trigger_types_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_register_sw_trigger_type);
> +
> +int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *t)

I'd make the return type void. Either it is not registered and unregister is 
a noop, or it is registered and it will be successfully unregistered. Either 
way the operation won't fail.

> +{
> +	struct iio_sw_trigger_type *iter;
> +	int ret = 0;
> +
> +	write_lock(&iio_trigger_types_lock);
> +	iter = iio_find_sw_trigger_type(t->name, strlen(t->name));

Not sure if we need this. unregister should never be called without register 
succeeding before.

> +	if (!iter)
> +		ret = -EINVAL;
> +	else
> +		list_del(&t->list);
> +	write_unlock(&iio_trigger_types_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_unregister_sw_trigger_type);
> +
> +static
> +struct iio_sw_trigger_type *iio_get_sw_trigger_type(char *name)
> +{
> +	struct iio_sw_trigger_type *t;
> +
> +	read_lock(&iio_trigger_types_lock);
> +	t = iio_find_sw_trigger_type(name, strlen(name));
> +	if (t && !try_module_get(t->owner))
> +		t = NULL;
> +	read_unlock(&iio_trigger_types_lock);
> +
> +	return t;
> +}
> +
> +struct iio_sw_trigger *iio_sw_trigger_create(char *type, char *name)
> +{
> +	struct iio_sw_trigger *t;
> +	struct iio_sw_trigger_type *tt;
> +
> +	tt = iio_get_sw_trigger_type(type);
> +	if (!tt) {
> +		pr_err("Invalid trigger type: %s\n", type);
> +		return ERR_PTR(-EINVAL);
> +	}
> +	t = tt->ops->probe(name);
> +	if (IS_ERR(t))
> +		goto out_module_put;
> +
> +	return t;
> +out_module_put:
> +	module_put(tt->owner);
> +	return t;
> +}
> +EXPORT_SYMBOL(iio_sw_trigger_create);
[...]
> +struct iio_sw_trigger_type {
> +	char *name;

const

> +	struct module *owner;
> +	struct iio_sw_trigger_ops *ops;

const

> +	struct list_head list;
> +};
> +
> +struct iio_sw_trigger {
> +	struct iio_trigger *trigger;
> +	struct iio_sw_trigger_type *trigger_type;
> +#ifdef CONFIG_CONFIGFS_FS
> +	struct config_group group;
> +#endif

The configfs specific bits should go into patch 2.

> +};
> +
> +struct iio_sw_trigger_ops {
> +	struct iio_sw_trigger* (*probe)(const char *);
> +	int (*remove)(struct iio_sw_trigger *);
> +};
> +
> +int iio_register_sw_trigger_type(struct iio_sw_trigger_type *);
> +int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *);
> +
> +struct iio_sw_trigger *iio_sw_trigger_create(char *, char *);
> +void iio_sw_trigger_destroy(struct iio_sw_trigger *);
> +
> +#ifdef CONFIG_CONFIGFS_FS
> +static inline
> +struct iio_sw_trigger *to_iio_sw_trigger(struct config_item *item)
> +{
> +	return container_of(to_config_group(item), struct iio_sw_trigger,
> +			    group);
> +}
> +#endif
> +
> +#endif /* __IIO_SW_TRIGGER */
>

Re: [PATCH v5 2/4] iio: core: Introduce IIO configfs support

[Jonathan Cameron]

On 4 May 2015 20:59:06 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>> This creates an IIO configfs subystem named "iio", with a default
>"triggers"
>> group.
>>
>> Triggers group is used for handling software triggers. To create a
>new software
>> trigger one must create a directory inside the trigger directory.
>>
>> Software trigger name MUST follow the following convention:
>> 	* <trigger-type>-<trigger-name>
>> Where:
>> 	* <trigger_type>, specifies the interrupt source (e.g: hrtimer)
>> 	* <trigger-name>, specifies the IIO device trigger name
>>
>> Failing to follow this convention will result in an directory
>creation error.
>>
>> E.g, assuming that hrtimer trigger type is registered with IIO
>software
>> trigger core:
>>
>> $ mkdir /config/iio/triggers/hrtimer-instance1
>>
>
>Nice, short and clean. Looks pretty good. It's a bit of a shame that we
>
>can't have a per type directory, but if that's how configfs works I
>guess 
>there is not much choice.
>
We could but an intermediate mkdir would be needed from user space which
would have odd semantics!
>[...]
>> +static struct config_group *trigger_make_group(struct config_group
>*group,
>> +					       const char *name)
>> +{
>> +	char *type_name;
>> +	char *trigger_name;
>> +	char buf[MAX_NAME_LEN];
>> +	struct iio_sw_trigger *t;
>> +
>> +	snprintf(buf, MAX_NAME_LEN, "%s", name);
>> +
>> +	/* group name should have the form <trigger-type>-<trigger-name> */
>> +	type_name = buf;
>> +	trigger_name = strchr(buf, '-');
>> +	if (!trigger_name) {
>> +		pr_err("Unable to locate '-' in %s. Use <type>-<name>.\n", buf);
>
>Do we want to print this side channel message? Makes it pretty easy to
>spam 
>the kernel log with a rouge application.
>
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	/* replace - with \0, this nicely separates the two strings */
>> +	*trigger_name = '\0';
>> +	trigger_name++;
>> +
>> +	t = iio_sw_trigger_create(type_name, trigger_name);
>> +	if (IS_ERR(t))
>> +		return ERR_CAST(t);
>> +
>> +	config_item_set_name(&t->group.cg_item, name);
>> +
>> +	return &t->group;
>> +}
>> +
>> +static void trigger_drop_group(struct config_group *group,
>> +			       struct config_item *item)
>> +{
>> +	struct iio_sw_trigger *t = to_iio_sw_trigger(item);
>> +
>> +	if (t)
>
>t will never be NULL.
>
>> +		iio_sw_trigger_destroy(t);
>> +	config_item_put(item);
>> +}

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Jonathan Cameron]

On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>[...]
>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>> +		      iio_hrtimer_info_show_sampling_frequency,
>> +		      iio_hrtimer_info_store_sampling_frequency);
>
>I wonder if the sampling frequency should be configurable the regular
>IIO 
>API, just like any other IIO device. But things like min/max sampling 
>frequency should be configured in configfs.
Would have to be in the trigger dir rather than device... Makes sense to put it there.
Limits on it here seem like a sensible idea.
>
>[...]
>> +#endif /* CONFIGFS_FS */
>> +
>[...]
>> +static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char
>*name)
>> +{
>[...]
>> +#ifdef CONFIG_CONFIGFS_FS
>> +	config_group_init_type_name(&trig_info->swt.group, name,
>> +				    &iio_hrtimer_type);
>> +#endif
>
>This should probably have a helper function in the sw trigger core,
>that 
>gets stubbed out when CONFIG_FS is disabled. Otherwise we'll see the
>same 
>#ifdef in every software trigger driver.
>[...]
>> +}
>> +
>> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
>> +{
>> +	struct iio_hrtimer_info *trig_info;
>> +
>> +	trig_info = iio_trigger_get_drvdata(swt->trigger);
>> +
>> +	hrtimer_cancel(&trig_info->timer);
>> +
>> +	iio_trigger_unregister(swt->trigger);
>> +	iio_trigger_free(swt->trigger);
>
>There is a bit of a race condition here. hrtimer_cancel() should be
>called 
>between unregister and free, otherwise it might be re-armed before it
>is 
>unregistered.
>
>> +	kfree(trig_info);
>> +
>> +	return 0;
>> +}
>> +
>> +struct iio_sw_trigger_ops iio_trig_hrtimer_ops = {
>
>const
>
>> +	.probe		= iio_trig_hrtimer_probe,
>> +	.remove		= iio_trig_hrtimer_remove,
>> +};
>[...]
>
>--
>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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v5 1/4] iio: core: Introduce IIO software triggers

[Daniel Baluta]

On Mon, May 4, 2015 at 11:11 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>>
>> A software trigger associates an IIO device trigger with a software
>> interrupt source (e.g: timer, sysfs). This patch adds the generic
>> infrastructure for handling software triggers.
>>
>> Software interrupts sources are kept in a iio_trigger_types_list and
>> registered separately when the associated kernel module is loaded.
>>
>> Software triggers can be created directly from drivers or from user
>> space via configfs interface.
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>>   drivers/iio/Kconfig                   |   8 +++
>>   drivers/iio/Makefile                  |   1 +
>>   drivers/iio/industrialio-sw-trigger.c | 112
>> ++++++++++++++++++++++++++++++++++
>>   include/linux/iio/sw_trigger.h        |  60 ++++++++++++++++++
>>   4 files changed, 181 insertions(+)
>>   create mode 100644 drivers/iio/industrialio-sw-trigger.c
>>   create mode 100644 include/linux/iio/sw_trigger.h
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 4011eff..de7f1d9 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -58,6 +58,14 @@ config IIO_CONSUMERS_PER_TRIGGER
>>         This value controls the maximum number of consumers that a
>>         given trigger may handle. Default is 2.
>>
>> +config IIO_SW_TRIGGER
>> +       bool "Enable software triggers support"
>> +       depends on IIO_TRIGGER
>> +       help
>> +        Provides IIO core support for software triggers. A software
>> +        trigger can be created via configfs or directly by a driver
>> +        using the API provided.
>> +
>>   source "drivers/iio/accel/Kconfig"
>>   source "drivers/iio/adc/Kconfig"
>>   source "drivers/iio/amplifiers/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 698afc2..df87975 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_IIO) += industrialio.o
>>   industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>>   industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>   industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>> +industrialio-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
>>   industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>>
>>   obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>> diff --git a/drivers/iio/industrialio-sw-trigger.c
>> b/drivers/iio/industrialio-sw-trigger.c
>> new file mode 100644
>> index 0000000..f22aa63
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-sw-trigger.c
>> @@ -0,0 +1,112 @@
>> +/*
>> + * The Industrial I/O core, software trigger functions
>> + *
>> + * Copyright (c) 2015 Intel Corporation
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/kmod.h>
>> +#include <linux/list.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/iio/sw_trigger.h>
>> +
>> +static LIST_HEAD(iio_trigger_types_list);
>> +static DEFINE_RWLOCK(iio_trigger_types_lock);
>> +
>> +static
>> +struct iio_sw_trigger_type *iio_find_sw_trigger_type(char *name, unsigned
>> len)
>
>
> const char *name, there are a couple of other places where char * should be
> const char * below as well.

Agree. I will fix this.

>
>> +{
>> +       struct iio_sw_trigger_type *t = NULL, *iter;
>> +
>> +       list_for_each_entry(iter, &iio_trigger_types_list, list)
>> +               if (!strncmp(iter->name, name, len)) {
>> +                       t = iter;
>> +                       break;
>> +               }
>> +
>> +       return t;
>> +}
>> +
>> +int iio_register_sw_trigger_type(struct iio_sw_trigger_type *t)
>
>
> Kernel doc would be nice for the public API

ok.

>
>> +{
>> +       struct iio_sw_trigger_type *iter;
>> +       int ret = 0;
>> +
>> +       write_lock(&iio_trigger_types_lock);
>> +       iter = iio_find_sw_trigger_type(t->name, strlen(t->name));
>> +       if (iter)
>> +               ret = -EBUSY;
>> +       else
>> +               list_add_tail(&t->list, &iio_trigger_types_list);
>> +       write_unlock(&iio_trigger_types_lock);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(iio_register_sw_trigger_type);
>> +
>> +int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *t)
>
>
> I'd make the return type void. Either it is not registered and unregister is
> a noop, or it is registered and it will be successfully unregistered. Either
> way the operation won't fail.

I don't have a strong preference for this. Likely the drivers will not check for
the return code of the function, but would be nice to let them the possibility
to do so.

I'm looking at unregister_filesystem for example:

http://lxr.free-electrons.com/source/fs/filesystems.c#L101

>
>> +{
>> +       struct iio_sw_trigger_type *iter;
>> +       int ret = 0;
>> +
>> +       write_lock(&iio_trigger_types_lock);
>> +       iter = iio_find_sw_trigger_type(t->name, strlen(t->name));
>
>
> Not sure if we need this. unregister should never be called without register
> succeeding before.
>

Famous last words :). I can change it if you really have a preference for this.
I don't see any drawbacks as it is now. Also the Linux kernel code seems to
use both patterns.

>
>> +       if (!iter)
>> +               ret = -EINVAL;
>> +       else
>> +               list_del(&t->list);
>> +       write_unlock(&iio_trigger_types_lock);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(iio_unregister_sw_trigger_type);
>> +
>> +static
>> +struct iio_sw_trigger_type *iio_get_sw_trigger_type(char *name)
>> +{
>> +       struct iio_sw_trigger_type *t;
>> +
>> +       read_lock(&iio_trigger_types_lock);
>> +       t = iio_find_sw_trigger_type(name, strlen(name));
>> +       if (t && !try_module_get(t->owner))
>> +               t = NULL;
>> +       read_unlock(&iio_trigger_types_lock);
>> +
>> +       return t;
>> +}
>> +
>> +struct iio_sw_trigger *iio_sw_trigger_create(char *type, char *name)
>> +{
>> +       struct iio_sw_trigger *t;
>> +       struct iio_sw_trigger_type *tt;
>> +
>> +       tt = iio_get_sw_trigger_type(type);
>> +       if (!tt) {
>> +               pr_err("Invalid trigger type: %s\n", type);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>> +       t = tt->ops->probe(name);
>> +       if (IS_ERR(t))
>> +               goto out_module_put;
>> +
>> +       return t;
>> +out_module_put:
>> +       module_put(tt->owner);
>> +       return t;
>> +}
>> +EXPORT_SYMBOL(iio_sw_trigger_create);
>
> [...]
>>
>> +struct iio_sw_trigger_type {
>> +       char *name;
>
>
> const
>
>> +       struct module *owner;
>> +       struct iio_sw_trigger_ops *ops;
>
>
> const
>
>> +       struct list_head list;
>> +};
>> +
>> +struct iio_sw_trigger {
>> +       struct iio_trigger *trigger;
>> +       struct iio_sw_trigger_type *trigger_type;
>> +#ifdef CONFIG_CONFIGFS_FS
>> +       struct config_group group;
>> +#endif
>
>
> The configfs specific bits should go into patch 2.

Agree.

>
>> +};
>> +
>> +struct iio_sw_trigger_ops {
>> +       struct iio_sw_trigger* (*probe)(const char *);
>> +       int (*remove)(struct iio_sw_trigger *);
>> +};
>> +
>> +int iio_register_sw_trigger_type(struct iio_sw_trigger_type *);
>> +int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *);
>> +
>> +struct iio_sw_trigger *iio_sw_trigger_create(char *, char *);
>> +void iio_sw_trigger_destroy(struct iio_sw_trigger *);
>> +
>> +#ifdef CONFIG_CONFIGFS_FS
>> +static inline
>> +struct iio_sw_trigger *to_iio_sw_trigger(struct config_item *item)
>> +{
>> +       return container_of(to_config_group(item), struct iio_sw_trigger,
>> +                           group);
>> +}
>> +#endif
>> +
>> +#endif /* __IIO_SW_TRIGGER */
>>
>
> --

Thanks a lot Lars!

Daniel.

Re: [PATCH v5 2/4] iio: core: Introduce IIO configfs support

[Daniel Baluta]

On 05/04/2015 10:59 PM, Lars-Peter Clausen wrote:
> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>> This creates an IIO configfs subystem named "iio", with a default
>> "triggers"
>> group.
>>
>> Triggers group is used for handling software triggers. To create a new
>> software
>> trigger one must create a directory inside the trigger directory.
>>
>> Software trigger name MUST follow the following convention:
>>     * <trigger-type>-<trigger-name>
>> Where:
>>     * <trigger_type>, specifies the interrupt source (e.g: hrtimer)
>>     * <trigger-name>, specifies the IIO device trigger name
>>
>> Failing to follow this convention will result in an directory creation
>> error.
>>
>> E.g, assuming that hrtimer trigger type is registered with IIO software
>> trigger core:
>>
>> $ mkdir /config/iio/triggers/hrtimer-instance1
>>
>
> Nice, short and clean. Looks pretty good. It's a bit of a shame that we
> can't have a per type directory, but if that's how configfs works I
> guess there is not much choice.
>
> [...]
>> +static struct config_group *trigger_make_group(struct config_group
>> *group,
>> +                           const char *name)
>> +{
>> +    char *type_name;
>> +    char *trigger_name;
>> +    char buf[MAX_NAME_LEN];
>> +    struct iio_sw_trigger *t;
>> +
>> +    snprintf(buf, MAX_NAME_LEN, "%s", name);
>> +
>> +    /* group name should have the form <trigger-type>-<trigger-name> */
>> +    type_name = buf;
>> +    trigger_name = strchr(buf, '-');
>> +    if (!trigger_name) {
>> +        pr_err("Unable to locate '-' in %s. Use <type>-<name>.\n", buf);
>
> Do we want to print this side channel message? Makes it pretty easy to
> spam the kernel log with a rouge application.

I think this is useful, people can get crazy for random -EINVAL errors 
:). Also the same is done in drivers/usb/gadget/configfs.c.


>
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    /* replace - with \0, this nicely separates the two strings */
>> +    *trigger_name = '\0';
>> +    trigger_name++;
>> +
>> +    t = iio_sw_trigger_create(type_name, trigger_name);
>> +    if (IS_ERR(t))
>> +        return ERR_CAST(t);
>> +
>> +    config_item_set_name(&t->group.cg_item, name);
>> +
>> +    return &t->group;
>> +}
>> +
>> +static void trigger_drop_group(struct config_group *group,
>> +                   struct config_item *item)
>> +{
>> +    struct iio_sw_trigger *t = to_iio_sw_trigger(item);
>> +
>> +    if (t)
>
> t will never be NULL.

Agree, will fix in v6.
>
>> +        iio_sw_trigger_destroy(t);
>> +    config_item_put(item);
>> +}
>

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Daniel Baluta]

On 05/05/2015 04:51 PM, Jonathan Cameron wrote:
>
>
> On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>> [...]
>>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>>> +		      iio_hrtimer_info_show_sampling_frequency,
>>> +		      iio_hrtimer_info_store_sampling_frequency);
>>
>> I wonder if the sampling frequency should be configurable the regular
>> IIO
>> API, just like any other IIO device. But things like min/max sampling
>> frequency should be configured in configfs.
> Would have to be in the trigger dir rather than device... Makes sense to put it there.
> Limits on it here seem like a sensible idea.

But then each trigger will have sampling_frequency right? This is not 
what we want.

>>
>> [...]
>>> +#endif /* CONFIGFS_FS */
>>> +
>> [...]
>>> +static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char
>> *name)
>>> +{
>> [...]
>>> +#ifdef CONFIG_CONFIGFS_FS
>>> +	config_group_init_type_name(&trig_info->swt.group, name,
>>> +				    &iio_hrtimer_type);
>>> +#endif
>>
>> This should probably have a helper function in the sw trigger core,
>> that
>> gets stubbed out when CONFIG_FS is disabled. Otherwise we'll see the
>> same
>> #ifdef in every software trigger driver.
>> [...]

Agree with this. Will fix.

>>> +}
>>> +
>>> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
>>> +{
>>> +	struct iio_hrtimer_info *trig_info;
>>> +
>>> +	trig_info = iio_trigger_get_drvdata(swt->trigger);
>>> +
>>> +	hrtimer_cancel(&trig_info->timer);
>>> +
>>> +	iio_trigger_unregister(swt->trigger);
>>> +	iio_trigger_free(swt->trigger);
>>
>> There is a bit of a race condition here. hrtimer_cancel() should be
>> called
>> between unregister and free, otherwise it might be re-armed before it
>> is
>> unregistered.

So this can be re-armed only if the buffer is re-enabled between 
hrtimer_cancel and iio_trigger_unregister :). I'm trying to understand 
how the race can happen.


>>
>>> +	kfree(trig_info);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +struct iio_sw_trigger_ops iio_trig_hrtimer_ops = {
>>
>> const

Agree.
>>
>>> +	.probe		= iio_trig_hrtimer_probe,
>>> +	.remove		= iio_trig_hrtimer_remove,
>>> +};
>> [...]

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Jonathan Cameron]

On 06/05/15 17:25, Daniel Baluta wrote:
> 
> 
> On 05/05/2015 04:51 PM, Jonathan Cameron wrote:
>>
>>
>> On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>>> [...]
>>>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>>>> +              iio_hrtimer_info_show_sampling_frequency,
>>>> +              iio_hrtimer_info_store_sampling_frequency);
>>>
>>> I wonder if the sampling frequency should be configurable the regular
>>> IIO
>>> API, just like any other IIO device. But things like min/max sampling
>>> frequency should be configured in configfs.
>> Would have to be in the trigger dir rather than device... Makes sense to put it there.
>> Limits on it here seem like a sensible idea.
> 
> But then each trigger will have sampling_frequency right? This is not what we want.
I'm confused now.  Why not?  Each hrtimer trigger created in configfs should have
it's own sampling frequency should it not? 
> 
>>>
>>> [...]
>>>> +#endif /* CONFIGFS_FS */
>>>> +
>>> [...]
>>>> +static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char
>>> *name)
>>>> +{
>>> [...]
>>>> +#ifdef CONFIG_CONFIGFS_FS
>>>> +    config_group_init_type_name(&trig_info->swt.group, name,
>>>> +                    &iio_hrtimer_type);
>>>> +#endif
>>>
>>> This should probably have a helper function in the sw trigger core,
>>> that
>>> gets stubbed out when CONFIG_FS is disabled. Otherwise we'll see the
>>> same
>>> #ifdef in every software trigger driver.
>>> [...]
> 
> Agree with this. Will fix.
> 
>>>> +}
>>>> +
>>>> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
>>>> +{
>>>> +    struct iio_hrtimer_info *trig_info;
>>>> +
>>>> +    trig_info = iio_trigger_get_drvdata(swt->trigger);
>>>> +
>>>> +    hrtimer_cancel(&trig_info->timer);
>>>> +
>>>> +    iio_trigger_unregister(swt->trigger);
>>>> +    iio_trigger_free(swt->trigger);
>>>
>>> There is a bit of a race condition here. hrtimer_cancel() should be
>>> called
>>> between unregister and free, otherwise it might be re-armed before it
>>> is
>>> unregistered.
> 
> So this can be re-armed only if the buffer is re-enabled between hrtimer_cancel and iio_trigger_unregister :). I'm trying to understand how the race can happen.
> 
> 
>>>
>>>> +    kfree(trig_info);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +struct iio_sw_trigger_ops iio_trig_hrtimer_ops = {
>>>
>>> const
> 
> Agree.
>>>
>>>> +    .probe        = iio_trig_hrtimer_probe,
>>>> +    .remove        = iio_trig_hrtimer_remove,
>>>> +};
>>> [...]

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Daniel Baluta]

On Wed, May 6, 2015 at 8:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 06/05/15 17:25, Daniel Baluta wrote:
>>
>>
>> On 05/05/2015 04:51 PM, Jonathan Cameron wrote:
>>>
>>>
>>> On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>>>> [...]
>>>>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>>>>> +              iio_hrtimer_info_show_sampling_frequency,
>>>>> +              iio_hrtimer_info_store_sampling_frequency);
>>>>
>>>> I wonder if the sampling frequency should be configurable the regular
>>>> IIO
>>>> API, just like any other IIO device. But things like min/max sampling
>>>> frequency should be configured in configfs.
>>> Would have to be in the trigger dir rather than device... Makes sense to put it there.
>>> Limits on it here seem like a sensible idea.
>>
>> But then each trigger will have sampling_frequency right? This is not what we want.
> I'm confused now.  Why not?  Each hrtimer trigger created in configfs should have
> it's own sampling frequency should it not?

I was referring to triggers in general, not just hrtimer triggers.

But I see now that we can set trig->dev.groups to point to our
specific attributes. This
should work.

Anyhow, I'm not convinced that sampling_frequency should be configured
from sysfs.
We create the trigger from configfs:

$ mkdir /config/triggers/hrtimer-instance0

Then, likely we have to do something like this:

$ echo 100 > /sys/bus/iio/trigger7/sampling_frequency

How is the user application going to know which is the exact directory
for hrtimer-instance0 ?

Daniel.

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Jonathan Cameron]

On 06/05/15 18:37, Daniel Baluta wrote:
> On Wed, May 6, 2015 at 8:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 06/05/15 17:25, Daniel Baluta wrote:
>>>
>>>
>>> On 05/05/2015 04:51 PM, Jonathan Cameron wrote:
>>>>
>>>>
>>>> On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>>>>> [...]
>>>>>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>>>>>> +              iio_hrtimer_info_show_sampling_frequency,
>>>>>> +              iio_hrtimer_info_store_sampling_frequency);
>>>>>
>>>>> I wonder if the sampling frequency should be configurable the regular
>>>>> IIO
>>>>> API, just like any other IIO device. But things like min/max sampling
>>>>> frequency should be configured in configfs.
>>>> Would have to be in the trigger dir rather than device... Makes sense to put it there.
>>>> Limits on it here seem like a sensible idea.
>>>
>>> But then each trigger will have sampling_frequency right? This is not what we want.
>> I'm confused now.  Why not?  Each hrtimer trigger created in configfs should have
>> it's own sampling frequency should it not?
> 
> I was referring to triggers in general, not just hrtimer triggers.
> 
> But I see now that we can set trig->dev.groups to point to our
> specific attributes. This
> should work.
> 
> Anyhow, I'm not convinced that sampling_frequency should be configured
> from sysfs.
> We create the trigger from configfs:
> 
> $ mkdir /config/triggers/hrtimer-instance0
> 
> Then, likely we have to do something like this:
> 
> $ echo 100 > /sys/bus/iio/trigger7/sampling_frequency
> 
> How is the user application going to know which is the exact directory
> for hrtimer-instance0 ?
> 
> Daniel.
> 
Find it by name like we normally do?

J

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Daniel Baluta]

On Thu, May 7, 2015 at 12:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 06/05/15 18:37, Daniel Baluta wrote:
>> On Wed, May 6, 2015 at 8:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 06/05/15 17:25, Daniel Baluta wrote:
>>>>
>>>>
>>>> On 05/05/2015 04:51 PM, Jonathan Cameron wrote:
>>>>>
>>>>>
>>>>> On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>>>>>> [...]
>>>>>>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>>>>>>> +              iio_hrtimer_info_show_sampling_frequency,
>>>>>>> +              iio_hrtimer_info_store_sampling_frequency);
>>>>>>
>>>>>> I wonder if the sampling frequency should be configurable the regular
>>>>>> IIO
>>>>>> API, just like any other IIO device. But things like min/max sampling
>>>>>> frequency should be configured in configfs.
>>>>> Would have to be in the trigger dir rather than device... Makes sense to put it there.
>>>>> Limits on it here seem like a sensible idea.
>>>>
>>>> But then each trigger will have sampling_frequency right? This is not what we want.
>>> I'm confused now.  Why not?  Each hrtimer trigger created in configfs should have
>>> it's own sampling frequency should it not?
>>
>> I was referring to triggers in general, not just hrtimer triggers.
>>
>> But I see now that we can set trig->dev.groups to point to our
>> specific attributes. This
>> should work.
>>
>> Anyhow, I'm not convinced that sampling_frequency should be configured
>> from sysfs.
>> We create the trigger from configfs:
>>
>> $ mkdir /config/triggers/hrtimer-instance0
>>
>> Then, likely we have to do something like this:
>>
>> $ echo 100 > /sys/bus/iio/trigger7/sampling_frequency
>>
>> How is the user application going to know which is the exact directory
>> for hrtimer-instance0 ?
>>
>> Daniel.
>>
> Find it by name like we normally do?

Ok :). I will move this to sysfs and send v6.

thanks,
Daniel.

Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger

[Lars-Peter Clausen]

On 05/07/2015 12:26 PM, Daniel Baluta wrote:
> On Thu, May 7, 2015 at 12:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 06/05/15 18:37, Daniel Baluta wrote:
>>> On Wed, May 6, 2015 at 8:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 06/05/15 17:25, Daniel Baluta wrote:
>>>>>
>>>>>
>>>>> On 05/05/2015 04:51 PM, Jonathan Cameron wrote:
>>>>>>
>>>>>>
>>>>>> On 4 May 2015 20:54:08 GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>>> On 05/04/2015 12:50 PM, Daniel Baluta wrote:
>>>>>>> [...]
>>>>>>>> +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
>>>>>>>> +              iio_hrtimer_info_show_sampling_frequency,
>>>>>>>> +              iio_hrtimer_info_store_sampling_frequency);
>>>>>>>
>>>>>>> I wonder if the sampling frequency should be configurable the regular
>>>>>>> IIO
>>>>>>> API, just like any other IIO device. But things like min/max sampling
>>>>>>> frequency should be configured in configfs.
>>>>>> Would have to be in the trigger dir rather than device... Makes sense to put it there.
>>>>>> Limits on it here seem like a sensible idea.
>>>>>
>>>>> But then each trigger will have sampling_frequency right? This is not what we want.
>>>> I'm confused now.  Why not?  Each hrtimer trigger created in configfs should have
>>>> it's own sampling frequency should it not?
>>>
>>> I was referring to triggers in general, not just hrtimer triggers.
>>>
>>> But I see now that we can set trig->dev.groups to point to our
>>> specific attributes. This
>>> should work.
>>>
>>> Anyhow, I'm not convinced that sampling_frequency should be configured
>>> from sysfs.
>>> We create the trigger from configfs:
>>>
>>> $ mkdir /config/triggers/hrtimer-instance0
>>>
>>> Then, likely we have to do something like this:
>>>
>>> $ echo 100 > /sys/bus/iio/trigger7/sampling_frequency
>>>
>>> How is the user application going to know which is the exact directory
>>> for hrtimer-instance0 ?
>>>
>>> Daniel.
>>>
>> Find it by name like we normally do?
>
> Ok :). I will move this to sysfs and send v6.

If you add the min/max properties you could still control the frequency via 
configfs by setting min and max to the same. In that case the sysfs property 
would merely inform applications about the currently selected frequency.

- Lars