Add initial configfs support for IIO

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 <[email protected]>

---
  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

drivers/iio/Kconfig +8 -0

index 4011eff..de7f1d9 100644
@@ -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"

drivers/iio/Makefile +1 -0

index 698afc2..df87975 100644
@@ -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

drivers/iio/industrialio-sw-trigger.c new file +13 -0

new file mode 100644
index 0000000..f22aa63
@@ -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>

drivers/iio/industrialio-sw-trigger.c new file +7 -0

+{
+	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)

drivers/iio/industrialio-sw-trigger.c new file +44 -0

+	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);

drivers/iio/industrialio-sw-trigger.c new file +23 -0

+};
+
+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 */

Quoted 82 lines · 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 <[email protected]>

---
  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

drivers/iio/Kconfig +8 -0

index 4011eff..de7f1d9 100644
@@ -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"

drivers/iio/Makefile +1 -0

index 698afc2..df87975 100644
@@ -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

drivers/iio/industrialio-sw-trigger.c new file +6 -0

new file mode 100644
index 0000000..f22aa63
@@ -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.
+ */

drivers/iio/industrialio-sw-trigger.c new file +11 -0

+
+#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);

Quoted 4 lines

drivers/iio/industrialio-sw-trigger.c new file +3 -0

+
+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.

Quoted 4 lines

drivers/iio/industrialio-sw-trigger.c new file +3 -0

+{
+       struct iio_sw_trigger_type *t = NULL, *iter;
+

drivers/iio/industrialio-sw-trigger.c line 36

Quoted 10 lines

drivers/iio/industrialio-sw-trigger.c new file +10 -0

+       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

Quoted 15 lines

drivers/iio/industrialio-sw-trigger.c new file +14 -0

+{
+       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;
+}

drivers/iio/industrialio-sw-trigger.c line 53

Quoted 3 lines

drivers/iio/industrialio-sw-trigger.c new file +3 -0

+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.

drivers/iio/industrialio-sw-trigger.c line 42

Quoted 7 lines

drivers/iio/industrialio-sw-trigger.c new file +6 -0

+{
+       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.

Quoted 45 lines

drivers/iio/industrialio-sw-trigger.c new file +44 -0

+       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);

[...]

drivers/iio/industrialio-sw-trigger.c

Quoted 3 lines

drivers/iio/industrialio-sw-trigger.c new file +2 -0

+struct iio_sw_trigger_type {
+       char *name;

const

drivers/iio/industrialio-sw-trigger.c

Quoted 2 lines

drivers/iio/industrialio-sw-trigger.c new file +2 -0

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

const

drivers/iio/industrialio-sw-trigger.c

Quoted 9 lines

drivers/iio/industrialio-sw-trigger.c new file +9 -0

+       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.

Quoted 19 lines

drivers/iio/industrialio-sw-trigger.c new file +18 -0

+};
+
+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,

drivers/iio/industrialio-sw-trigger.c

Quoted 6 lines

drivers/iio/industrialio-sw-trigger.c new file +5 -0

+                           group);
+}
+#endif
+
+#endif /* __IIO_SW_TRIGGER */

--

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

drivers/iio/industrialio-configfs.c new file +5 -0

+static struct config_group *trigger_make_group(struct config_group *group,
+					       const char *name)
+{
+	char *type_name;
+	char *trigger_name;

drivers/iio/industrialio-configfs.c new file +12 -0

+		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);

drivers/iio/industrialio-configfs.c new file +3 -0

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

Quoted 1 line · Daniel Baluta

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

"triggers"

Quoted 3 lines

group.

Triggers group is used for handling software triggers. To create a

new software

Quoted 9 lines

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.

Quoted 2 lines

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

software

Quoted 2 lines

trigger core:

Quoted 2 lines

$ 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.

[...]

Quoted 1 line

+static struct config_group *trigger_make_group(struct config_group

*group,

Quoted 4 lines

drivers/iio/industrialio-configfs.c new file +4 -0

+					       const char *name)
+{
+	char *type_name;
+	char *trigger_name;

drivers/iio/industrialio-configfs.c line 36

Quoted 10 lines

drivers/iio/industrialio-configfs.c new file +10 -0

+	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.

Quoted 12 lines

drivers/iio/industrialio-configfs.c new file +12 -0

+		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);

drivers/iio/industrialio-configfs.c line 58

Quoted 10 lines

drivers/iio/industrialio-configfs.c new file +10 -0

+
+	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.

Quoted 3 lines

drivers/iio/industrialio-configfs.c new file +3 -0

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

Quoted 14 lines · 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

Quoted 8 lines

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.

[...]

Quoted 11 lines

+static struct config_group *trigger_make_group(struct config_group *group,

drivers/iio/industrialio-configfs.c new file +9 -0

+                           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);
+

drivers/iio/industrialio-configfs.c line 36

Quoted 5 lines

drivers/iio/industrialio-configfs.c new file +5 -0

+    /* 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.

Quoted 17 lines

drivers/iio/industrialio-configfs.c new file +16 -0

+        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;
+}
+

drivers/iio/industrialio-configfs.c line 58

Quoted 6 lines

drivers/iio/industrialio-configfs.c new file +6 -0

+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.

Quoted 4 lines

drivers/iio/industrialio-configfs.c new file +3 -0

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

(quoted diff) +2 -0

+#endif /* CONFIGFS_FS */
+

(quoted diff) +2 -0

+static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name)
+{

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+	.probe		= iio_trig_hrtimer_probe,
+	.remove		= iio_trig_hrtimer_remove,
+};

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

drivers/iio/trigger/iio-trig-hrtimer.c line 76

Quoted 3 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+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.

[...]

Quoted 2 lines

(quoted diff) +2 -0

+#endif /* CONFIGFS_FS */
+

[...]

Quoted 1 line

+static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char

*name)

Quoted 1 line

+{

[...]

drivers/iio/trigger/iio-trig-hrtimer.c line 160

Quoted 4 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +4 -0

+#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.
[...]

drivers/iio/trigger/iio-trig-hrtimer.c line 179

Quoted 12 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +12 -0

+}
+
+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.

drivers/iio/trigger/iio-trig-hrtimer.c line 185

Quoted 6 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +6 -0

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

const

drivers/iio/trigger/iio-trig-hrtimer.c line 188

Quoted 3 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+	.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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Lars-Peter Clausen

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

drivers/iio/trigger/iio-trig-hrtimer.c line 76

Quoted 3 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+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.

Quoted 9 lines

[...]

Quoted 2 lines

(quoted diff) +2 -0

+#endif /* CONFIGFS_FS */
+

[...]

Quoted 1 line

+static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char

*name)

Quoted 1 line

+{

[...]

Quoted 11 lines

drivers/iio/trigger/iio-trig-hrtimer.c line 160

Quoted 4 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +4 -0

+#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.
[...]

Quoted 10 lines

Quoted 10 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +10 -0

+}
+
+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);
+

Quoted 8 lines

drivers/iio/trigger/iio-trig-hrtimer.c line 179

Quoted 2 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +2 -0

+	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.

Quoted 9 lines

drivers/iio/trigger/iio-trig-hrtimer.c line 185

Quoted 7 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +6 -0

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

const

Quoted 5 lines

Quoted 4 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+	.probe		= iio_trig_hrtimer_probe,
+	.remove		= iio_trig_hrtimer_remove,
+};

[...]

Quoted 9 lines · Jonathan Cameron

Quoted 6 lines · Lars-Peter Clausen

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

Quoted 3 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
+              iio_hrtimer_info_show_sampling_frequency,
+              iio_hrtimer_info_store_sampling_frequency);

Quoted 6 lines

Quoted 4 lines

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.

Quoted 10 lines

Quoted 10 lines

[...]

Quoted 2 lines

(quoted diff) +2 -0

+#endif /* CONFIGFS_FS */
+

[...]

Quoted 1 line

+static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char

*name)

Quoted 1 line

+{

[...]

Quoted 11 lines

Quoted 11 lines

drivers/iio/trigger/iio-trig-hrtimer.c line 160

Quoted 4 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +4 -0

+#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.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +10 -0

+}
+
+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);
+

Quoted 8 lines

Quoted 8 lines

drivers/iio/trigger/iio-trig-hrtimer.c line 179

Quoted 2 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +2 -0

+    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.

Quoted 9 lines

Quoted 9 lines

drivers/iio/trigger/iio-trig-hrtimer.c line 185

Quoted 7 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +6 -0

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

const

Agree.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+    .probe        = iio_trig_hrtimer_probe,
+    .remove        = iio_trig_hrtimer_remove,
+};

[...]

Quoted 16 lines · Daniel Baluta

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · Lars-Peter Clausen

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

drivers/iio/trigger/iio-trig-hrtimer.c line 76

Quoted 3 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+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.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 17 lines · Jonathan Cameron

Quoted 16 lines · Daniel Baluta

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · Lars-Peter Clausen

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

drivers/iio/trigger/iio-trig-hrtimer.c line 76

Quoted 3 lines

drivers/iio/trigger/iio-trig-hrtimer.c new file +3 -0

+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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 39 lines · Daniel Baluta

Quoted 17 lines · Jonathan Cameron

Quoted 16 lines · Daniel Baluta

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · Lars-Peter Clausen

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

Review

Quoted 3 lines

(quoted diff) +3 -0

+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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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:

Quoted 7 lines

$ 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?

Quoted 42 lines · Jonathan Cameron

Quoted 41 lines · Daniel Baluta

Quoted 17 lines · Jonathan Cameron

Quoted 16 lines · Daniel Baluta

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · Lars-Peter Clausen

On 05/04/2015 12:50 PM, Daniel Baluta wrote:
[...]

Review

Quoted 3 lines

(quoted diff) +3 -0

+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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 5 lines

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.