[PATCH v3 0/3] Add initial configfs support for IIO

[PATCH v3 0/3] Add initial configfs support for IIO

[Daniel Baluta]

This patchset adds initial support for configfs in IIO.

Patch 1:
	* adds configfs infrastructure creating an "iio" configfs
        subystem + an initial "triggers" group.
Patch 2:
	* adds the first "real" trigger type - hrtimer.
Patch 3:
	* adds Documentation

Changes since last version are listed in each individual patch.

Daniel Baluta (3):
  iio: configfs: Add configfs support to IIO
  iio: trigger: Add support for highres timer trigger type
  iio: Documentation: Add documentation for IIO configfs

 Documentation/iio/iio_configfs.txt       |  83 +++++++++
 drivers/iio/Kconfig                      |   8 +
 drivers/iio/Makefile                     |   1 +
 drivers/iio/industrialio-configfs.c      | 291 +++++++++++++++++++++++++++++++
 drivers/iio/trigger/Kconfig              |   9 +
 drivers/iio/trigger/Makefile             |   2 +
 drivers/iio/trigger/iio-trig-hrtimer.c   | 154 ++++++++++++++++
 include/linux/iio/iio_configfs_trigger.h |  41 +++++
 8 files changed, 589 insertions(+)
 create mode 100644 Documentation/iio/iio_configfs.txt
 create mode 100644 drivers/iio/industrialio-configfs.c
 create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c
 create mode 100644 include/linux/iio/iio_configfs_trigger.h

-- 
1.9.1

[PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Daniel Baluta]

This module is the core of IIO configfs. It creates the "iio" subsystem under
configfs mount point root, with one default group for "triggers".

It offers basic interface for registering software trigger types. Next patches
will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
create a new kernel module which implements iio_configfs_trigger.h interface.

See Documentation/iio/iio_configfs.txt for more details on how configfs
support for IIO works.

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---

Changes since v2:
	* Fix build errors by compiling industrialio-configfs.ko 
	as a separate module, as reported by Paul.

Changes since v1:
	* addressed feedback from v1:
		* https://lkml.org/lkml/2015/3/25/647
	* create a directory per trigger type
	* removed activate and type attributes
	
 drivers/iio/Kconfig                      |   8 ++
 drivers/iio/Makefile                     |   1 +
 drivers/iio/industrialio-configfs.c      | 127 +++++++++++++++++++++++++++++++
 include/linux/iio/iio_configfs_trigger.h |  40 ++++++++++
 4 files changed, 176 insertions(+)
 create mode 100644 drivers/iio/industrialio-configfs.c
 create mode 100644 include/linux/iio/iio_configfs_trigger.h

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 4011eff..41d5863 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 698afc2..72e85c42 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -8,6 +8,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
 industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
 industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
 
+obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
 obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
 obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
 
diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
new file mode 100644
index 0000000..d8ffd76
--- /dev/null
+++ b/drivers/iio/industrialio-configfs.c
@@ -0,0 +1,127 @@
+/*
+ * 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/iio_configfs_trigger.h>
+
+static struct iio_configfs_trigger_type *trigger_types[IIO_TRIGGER_TYPE_MAX];
+static DEFINE_RWLOCK(trigger_types_lock);
+
+int register_configfs_trigger(struct iio_configfs_trigger_type *t)
+{
+	int index = t->type;
+	int ret = 0;
+
+	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
+		return -EINVAL;
+
+	write_lock(&trigger_types_lock);
+	if (trigger_types[index])
+		ret = -EBUSY;
+	else
+		trigger_types[index] = t;
+	write_unlock(&trigger_types_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(register_configfs_trigger);
+
+int unregister_configfs_trigger(struct iio_configfs_trigger_type *t)
+{
+	int index = t->type;
+
+	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
+		return -EINVAL;
+
+	write_lock(&trigger_types_lock);
+	trigger_types[index] = NULL;
+	write_unlock(&trigger_types_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(unregister_configfs_trigger);
+
+static
+struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
+{
+	struct iio_configfs_trigger_type *t;
+
+	if (type < 0 || type >= IIO_TRIGGER_TYPE_MAX)
+		return ERR_PTR(-EINVAL);
+
+	read_lock(&trigger_types_lock);
+	t = trigger_types[type];
+	if (t && !try_module_get(t->owner))
+		t = NULL;
+	read_unlock(&trigger_types_lock);
+
+	return t;
+}
+
+static struct config_group *iio_triggers_default_groups[] = {
+	NULL
+};
+
+static struct config_item_type iio_triggers_group_type = {
+	.ct_owner       = THIS_MODULE,
+};
+
+static struct config_group iio_triggers_group = {
+	.cg_item = {
+		.ci_namebuf = "triggers",
+		.ci_type = &iio_triggers_group_type,
+	},
+	.default_groups = iio_triggers_default_groups,
+};
+
+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");
diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
new file mode 100644
index 0000000..75e76f2
--- /dev/null
+++ b/include/linux/iio/iio_configfs_trigger.h
@@ -0,0 +1,40 @@
+#ifndef __IIO_CONFIGFS_TRIGGER
+#define __IIO_CONFIGFS_TRIGGER
+
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+
+#define module_iio_configfs_trigger_driver(__iio_configfs_trigger) \
+	module_driver(__iio_configfs_trigger, register_configfs_trigger, \
+		      unregister_configfs_trigger)
+
+enum {
+	IIO_TRIGGER_TYPE_MAX,
+};
+
+struct iio_configfs_trigger_ops;
+
+struct iio_configfs_trigger_type {
+	int type;
+	struct module *owner;
+	struct iio_configfs_trigger_ops *trigger_ops;
+};
+
+struct iio_configfs_trigger_info {
+	char *name;
+	struct iio_trigger *trigger;
+	struct iio_configfs_trigger_type *trigger_type;
+};
+
+struct iio_configfs_trigger_ops {
+	int (*get_delay)(struct iio_configfs_trigger_info *, unsigned long *);
+	int (*set_delay)(struct iio_configfs_trigger_info *, unsigned long);
+	int (*probe)(struct iio_configfs_trigger_info *);
+	int (*remove)(struct iio_configfs_trigger_info *);
+};
+
+int register_configfs_trigger(struct iio_configfs_trigger_type *);
+int unregister_configfs_trigger(struct iio_configfs_trigger_type *);
+
+#endif /* __IIO_CONFIGFS_TRIGGER */
-- 
1.9.1

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Daniel Baluta]

On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
> This module is the core of IIO configfs. It creates the "iio" subsystem under
> configfs mount point root, with one default group for "triggers".
>
> It offers basic interface for registering software trigger types. Next patches
> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
> create a new kernel module which implements iio_configfs_trigger.h interface.
>
> See Documentation/iio/iio_configfs.txt for more details on how configfs
> support for IIO works.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>

Hi all,

I also need your feedback on the following problem.

We would like to be able to create hrtimer triggers from within
a kernel module. There are cases where we don't have an interrupt
pin or the interrupt pin is not connected, and we would like
that applications to run unmodified with these types of sensors too.

We will split the current design into 3 parts:

(1) IIO trigger handling generic part, which offers an API
     to register/unregister/get a reference to a trigger type

(2) IIO configfs part that gets a reference to a trigger type and
handles user requests to create/destroy a trigger.

(3) IIO hrtimer driver that use the API at (1) for registering
/ deregistering a trigger type.

Then, each driver in the case that it doesn't have a "real" trigger,
will get a reference to a "hrtimer" trigger type and create
a new "hrtimer" trigger.

Does this look good to you? This could be easily done from
userspace, but we will need to modify our userspace applications.

Also, handling sampling_frequency/delay would be transparent to
applications if we provide this in kernel API.

Daniel.

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 08/04/15 14:30, Daniel Baluta wrote:
> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>> configfs mount point root, with one default group for "triggers".
>>
>> It offers basic interface for registering software trigger types. Next patches
>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>
>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>> support for IIO works.
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> 
> Hi all,
> 
> I also need your feedback on the following problem.
> 
> We would like to be able to create hrtimer triggers from within
> a kernel module. There are cases where we don't have an interrupt
> pin or the interrupt pin is not connected, and we would like
> that applications to run unmodified with these types of sensors too.
A reasonable aim perhaps, as opposed to locally implemented polling
all over the place (which should definitely not happen).

An alternative the zio guys used was to create timer
based triggers on the fly purely based on them being requested
(with an appropriate name IIRC)...  Doesn't quite solve your
problem though as still needs userspace changes.
> 
> We will split the current design into 3 parts:
> 
> (1) IIO trigger handling generic part, which offers an API
>      to register/unregister/get a reference to a trigger type
> 
> (2) IIO configfs part that gets a reference to a trigger type and
> handles user requests to create/destroy a trigger.
> 
> (3) IIO hrtimer driver that use the API at (1) for registering
> / deregistering a trigger type.
> 
> Then, each driver in the case that it doesn't have a "real" trigger,
> will get a reference to a "hrtimer" trigger type and create
> a new "hrtimer" trigger.
> 
> Does this look good to you? This could be easily done from
> userspace, but we will need to modify our userspace applications.
My initial thought is this really is a job for userspace, as should
be in most cases connecting up the device specific trigger as well
(as it's not always the right thing to use).

In the general case it is far from obvious that an hrtimer is the
right option.  Many usecases would be better off with a sysfs trigger
or even running off a different interrupt based trigger entirely.
> 
> Also, handling sampling_frequency/delay would be transparent to
> applications if we provide this in kernel API.
Not really as sampling frequency in this case should only be an
attribute of the trigger, not the device.  We only really allow
it for the device rather than always the triggers on the basis
that it has impacts on other device elements (events etc).

Now sensible userspace ought to search for the trigger sysfs
directory and look there as well.

I suppose if there is an awful lot of demand for this function
we could add a control knob somewhere that would set a 'default
trigger type' to be created for buffered devices that haven't
provided their own...  I'm not overly keen though.
> 
> Daniel.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Daniel Baluta]

On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 08/04/15 14:30, Daniel Baluta wrote:
>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>> configfs mount point root, with one default group for "triggers".
>>>
>>> It offers basic interface for registering software trigger types. Next patches
>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>
>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>> support for IIO works.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>
>> Hi all,
>>
>> I also need your feedback on the following problem.
>>
>> We would like to be able to create hrtimer triggers from within
>> a kernel module. There are cases where we don't have an interrupt
>> pin or the interrupt pin is not connected, and we would like
>> that applications to run unmodified with these types of sensors too.
> A reasonable aim perhaps, as opposed to locally implemented polling
> all over the place (which should definitely not happen).

Yes, exactly! :)

>
> An alternative the zio guys used was to create timer
> based triggers on the fly purely based on them being requested
> (with an appropriate name IIRC)...  Doesn't quite solve your
> problem though as still needs userspace changes.
>>
>> We will split the current design into 3 parts:
>>
>> (1) IIO trigger handling generic part, which offers an API
>>      to register/unregister/get a reference to a trigger type
>>
>> (2) IIO configfs part that gets a reference to a trigger type and
>> handles user requests to create/destroy a trigger.
>>
>> (3) IIO hrtimer driver that use the API at (1) for registering
>> / deregistering a trigger type.
>>
>> Then, each driver in the case that it doesn't have a "real" trigger,
>> will get a reference to a "hrtimer" trigger type and create
>> a new "hrtimer" trigger.
>>
>> Does this look good to you? This could be easily done from
>> userspace, but we will need to modify our userspace applications.
> My initial thought is this really is a job for userspace, as should
> be in most cases connecting up the device specific trigger as well
> (as it's not always the right thing to use).
>
> In the general case it is far from obvious that an hrtimer is the
> right option.  Many usecases would be better off with a sysfs trigger
> or even running off a different interrupt based trigger entirely.
>>
>> Also, handling sampling_frequency/delay would be transparent to
>> applications if we provide this in kernel API.
> Not really as sampling frequency in this case should only be an
> attribute of the trigger, not the device.  We only really allow
> it for the device rather than always the triggers on the basis
> that it has impacts on other device elements (events etc).

Well, if the trigger is directly created from the driver then we will
have a reference to a function that sets its delay.

Each time the user sets the sampling frequency for the device
with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
case we also call set_delay(). Thus we always have have device
sampling frequency in sync with trigger delay.

I know this sounds crazy :) and I don't like it. I am not sure how
to guarantee that device frequency is always in sync with trigger
delay.

>
> Now sensible userspace ought to search for the trigger sysfs
> directory and look there as well.
>
> I suppose if there is an awful lot of demand for this function
> we could add a control knob somewhere that would set a 'default
> trigger type' to be created for buffered devices that haven't
> provided their own...  I'm not overly keen though.
>>

This functionality would be very nice and useful, lets see if we can
find some elegant way to do it.

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 10/04/15 14:43, Daniel Baluta wrote:
> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 08/04/15 14:30, Daniel Baluta wrote:
>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>> configfs mount point root, with one default group for "triggers".
>>>>
>>>> It offers basic interface for registering software trigger types. Next patches
>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>
>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>> support for IIO works.
>>>>
>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>
>>> Hi all,
>>>
>>> I also need your feedback on the following problem.
>>>
>>> We would like to be able to create hrtimer triggers from within
>>> a kernel module. There are cases where we don't have an interrupt
>>> pin or the interrupt pin is not connected, and we would like
>>> that applications to run unmodified with these types of sensors too.
>> A reasonable aim perhaps, as opposed to locally implemented polling
>> all over the place (which should definitely not happen).
> 
> Yes, exactly! :)
I'm actually beginning to have my doubts about whether my initial
response here was right.  Given your use cases and the complexity
of matching frequencies, I think you are almost always better
off implementing it in the drivers themselves (which can decide
if there is new data or not).
> 
>>
>> An alternative the zio guys used was to create timer
>> based triggers on the fly purely based on them being requested
>> (with an appropriate name IIRC)...  Doesn't quite solve your
>> problem though as still needs userspace changes.
>>>
>>> We will split the current design into 3 parts:
>>>
>>> (1) IIO trigger handling generic part, which offers an API
>>>      to register/unregister/get a reference to a trigger type
>>>
>>> (2) IIO configfs part that gets a reference to a trigger type and
>>> handles user requests to create/destroy a trigger.
>>>
>>> (3) IIO hrtimer driver that use the API at (1) for registering
>>> / deregistering a trigger type.
>>>
>>> Then, each driver in the case that it doesn't have a "real" trigger,
>>> will get a reference to a "hrtimer" trigger type and create
>>> a new "hrtimer" trigger.
>>>
>>> Does this look good to you? This could be easily done from
>>> userspace, but we will need to modify our userspace applications.
>> My initial thought is this really is a job for userspace, as should
>> be in most cases connecting up the device specific trigger as well
>> (as it's not always the right thing to use).
>>
>> In the general case it is far from obvious that an hrtimer is the
>> right option.  Many usecases would be better off with a sysfs trigger
>> or even running off a different interrupt based trigger entirely.
>>>
>>> Also, handling sampling_frequency/delay would be transparent to
>>> applications if we provide this in kernel API.
>> Not really as sampling frequency in this case should only be an
>> attribute of the trigger, not the device.  We only really allow
>> it for the device rather than always the triggers on the basis
>> that it has impacts on other device elements (events etc).
> 
> Well, if the trigger is directly created from the driver then we will
> have a reference to a function that sets its delay.
> 
> Each time the user sets the sampling frequency for the device
> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
> case we also call set_delay(). Thus we always have have device
> sampling frequency in sync with trigger delay.
> 
> I know this sounds crazy :) and I don't like it. I am not sure how
> to guarantee that device frequency is always in sync with trigger
> delay.
You can't that I can see.  Hence you've convinced me this is a bad
idea.
> 
>>
>> Now sensible userspace ought to search for the trigger sysfs
>> directory and look there as well.
>>
>> I suppose if there is an awful lot of demand for this function
>> we could add a control knob somewhere that would set a 'default
>> trigger type' to be created for buffered devices that haven't
>> provided their own...  I'm not overly keen though.
>>>
> 
> This functionality would be very nice and useful, lets see if we can
> find some elegant way to do it.
I think this may well still be worth having, just not for your
particular case.  It would be useful for the devices that have
a 'sample now' signal of some type that allows the trigger to
control when they are sampling (and hence always match perfectly!)

J
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Octavian Purdila]

On Sun, Apr 12, 2015 at 8:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 10/04/15 14:43, Daniel Baluta wrote:
>> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 08/04/15 14:30, Daniel Baluta wrote:
>>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>>> configfs mount point root, with one default group for "triggers".
>>>>>
>>>>> It offers basic interface for registering software trigger types. Next patches
>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>>
>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>>> support for IIO works.
>>>>>
>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>>
>>>> Hi all,
>>>>
>>>> I also need your feedback on the following problem.
>>>>
>>>> We would like to be able to create hrtimer triggers from within
>>>> a kernel module. There are cases where we don't have an interrupt
>>>> pin or the interrupt pin is not connected, and we would like
>>>> that applications to run unmodified with these types of sensors too.
>>> A reasonable aim perhaps, as opposed to locally implemented polling
>>> all over the place (which should definitely not happen).
>>
>> Yes, exactly! :)
> I'm actually beginning to have my doubts about whether my initial
> response here was right.  Given your use cases and the complexity
> of matching frequencies, I think you are almost always better
> off implementing it in the drivers themselves (which can decide
> if there is new data or not).

I agree. Although I think having some helper functions in IIO core
should help keep the driver modifications to a minimum.

>>
>>>
>>> An alternative the zio guys used was to create timer
>>> based triggers on the fly purely based on them being requested
>>> (with an appropriate name IIRC)...  Doesn't quite solve your
>>> problem though as still needs userspace changes.
>>>>
>>>> We will split the current design into 3 parts:
>>>>
>>>> (1) IIO trigger handling generic part, which offers an API
>>>>      to register/unregister/get a reference to a trigger type
>>>>
>>>> (2) IIO configfs part that gets a reference to a trigger type and
>>>> handles user requests to create/destroy a trigger.
>>>>
>>>> (3) IIO hrtimer driver that use the API at (1) for registering
>>>> / deregistering a trigger type.
>>>>
>>>> Then, each driver in the case that it doesn't have a "real" trigger,
>>>> will get a reference to a "hrtimer" trigger type and create
>>>> a new "hrtimer" trigger.
>>>>
>>>> Does this look good to you? This could be easily done from
>>>> userspace, but we will need to modify our userspace applications.
>>> My initial thought is this really is a job for userspace, as should
>>> be in most cases connecting up the device specific trigger as well
>>> (as it's not always the right thing to use).
>>>
>>> In the general case it is far from obvious that an hrtimer is the
>>> right option.  Many usecases would be better off with a sysfs trigger
>>> or even running off a different interrupt based trigger entirely.
>>>>
>>>> Also, handling sampling_frequency/delay would be transparent to
>>>> applications if we provide this in kernel API.
>>> Not really as sampling frequency in this case should only be an
>>> attribute of the trigger, not the device.  We only really allow
>>> it for the device rather than always the triggers on the basis
>>> that it has impacts on other device elements (events etc).
>>
>> Well, if the trigger is directly created from the driver then we will
>> have a reference to a function that sets its delay.
>>
>> Each time the user sets the sampling frequency for the device
>> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
>> case we also call set_delay(). Thus we always have have device
>> sampling frequency in sync with trigger delay.
>>
>> I know this sounds crazy :) and I don't like it. I am not sure how
>> to guarantee that device frequency is always in sync with trigger
>> delay.
> You can't that I can see.  Hence you've convinced me this is a bad
> idea.

I agree we can't accurately synchronize the device frequency with the
trigger frequency, but I think we can get it do a good enough level. I
also see no difference in accuracy in doing it from userspace or from
the driver itself.

Why do you think it is a bad idea to synchronize the trigger sample
frequency from the device sample frequency handler? I think that if we
want to provide the hrtimer trigger from the driver itself, we should
do this as well, otherwise there is no real gain to userspace.

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 15/04/15 21:58, Octavian Purdila wrote:
> On Sun, Apr 12, 2015 at 8:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 10/04/15 14:43, Daniel Baluta wrote:
>>> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 08/04/15 14:30, Daniel Baluta wrote:
>>>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>>>> configfs mount point root, with one default group for "triggers".
>>>>>>
>>>>>> It offers basic interface for registering software trigger types. Next patches
>>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>>>
>>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>>>> support for IIO works.
>>>>>>
>>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> I also need your feedback on the following problem.
>>>>>
>>>>> We would like to be able to create hrtimer triggers from within
>>>>> a kernel module. There are cases where we don't have an interrupt
>>>>> pin or the interrupt pin is not connected, and we would like
>>>>> that applications to run unmodified with these types of sensors too.
>>>> A reasonable aim perhaps, as opposed to locally implemented polling
>>>> all over the place (which should definitely not happen).
>>>
>>> Yes, exactly! :)
>> I'm actually beginning to have my doubts about whether my initial
>> response here was right.  Given your use cases and the complexity
>> of matching frequencies, I think you are almost always better
>> off implementing it in the drivers themselves (which can decide
>> if there is new data or not).
> 
> I agree. Although I think having some helper functions in IIO core
> should help keep the driver modifications to a minimum.
> 
>>>
>>>>
>>>> An alternative the zio guys used was to create timer
>>>> based triggers on the fly purely based on them being requested
>>>> (with an appropriate name IIRC)...  Doesn't quite solve your
>>>> problem though as still needs userspace changes.
>>>>>
>>>>> We will split the current design into 3 parts:
>>>>>
>>>>> (1) IIO trigger handling generic part, which offers an API
>>>>>      to register/unregister/get a reference to a trigger type
>>>>>
>>>>> (2) IIO configfs part that gets a reference to a trigger type and
>>>>> handles user requests to create/destroy a trigger.
>>>>>
>>>>> (3) IIO hrtimer driver that use the API at (1) for registering
>>>>> / deregistering a trigger type.
>>>>>
>>>>> Then, each driver in the case that it doesn't have a "real" trigger,
>>>>> will get a reference to a "hrtimer" trigger type and create
>>>>> a new "hrtimer" trigger.
>>>>>
>>>>> Does this look good to you? This could be easily done from
>>>>> userspace, but we will need to modify our userspace applications.
>>>> My initial thought is this really is a job for userspace, as should
>>>> be in most cases connecting up the device specific trigger as well
>>>> (as it's not always the right thing to use).
>>>>
>>>> In the general case it is far from obvious that an hrtimer is the
>>>> right option.  Many usecases would be better off with a sysfs trigger
>>>> or even running off a different interrupt based trigger entirely.
>>>>>
>>>>> Also, handling sampling_frequency/delay would be transparent to
>>>>> applications if we provide this in kernel API.
>>>> Not really as sampling frequency in this case should only be an
>>>> attribute of the trigger, not the device.  We only really allow
>>>> it for the device rather than always the triggers on the basis
>>>> that it has impacts on other device elements (events etc).
>>>
>>> Well, if the trigger is directly created from the driver then we will
>>> have a reference to a function that sets its delay.
>>>
>>> Each time the user sets the sampling frequency for the device
>>> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
>>> case we also call set_delay(). Thus we always have have device
>>> sampling frequency in sync with trigger delay.
>>>
>>> I know this sounds crazy :) and I don't like it. I am not sure how
>>> to guarantee that device frequency is always in sync with trigger
>>> delay.
>> You can't that I can see.  Hence you've convinced me this is a bad
>> idea.
> 
> I agree we can't accurately synchronize the device frequency with the
> trigger frequency, but I think we can get it do a good enough level. I
> also see no difference in accuracy in doing it from userspace or from
> the driver itself.
> 
> Why do you think it is a bad idea to synchronize the trigger sample
> frequency from the device sample frequency handler? I think that if we
> want to provide the hrtimer trigger from the driver itself, we should
> do this as well, otherwise there is no real gain to userspace.
> 
You will get beating problems.  So once in a while you'll either miss
a reading or you'll get a bonus one.

The point about doing it in the driver is that you actually poll at a
higher frequency, but only fire a trigger if there is something there
(by reading the dataready register or equivalent).

Thus your trigger acts more or less the same as a wired interrupt.
You can't do this with an external hrtimer trigger.

J

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Octavian Purdila]

On Wed, Apr 15, 2015 at 2:34 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 15/04/15 21:58, Octavian Purdila wrote:
>> On Sun, Apr 12, 2015 at 8:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 10/04/15 14:43, Daniel Baluta wrote:
>>>> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 08/04/15 14:30, Daniel Baluta wrote:
>>>>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>>>>> configfs mount point root, with one default group for "triggers".
>>>>>>>
>>>>>>> It offers basic interface for registering software trigger types. Next patches
>>>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>>>>
>>>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>>>>> support for IIO works.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I also need your feedback on the following problem.
>>>>>>
>>>>>> We would like to be able to create hrtimer triggers from within
>>>>>> a kernel module. There are cases where we don't have an interrupt
>>>>>> pin or the interrupt pin is not connected, and we would like
>>>>>> that applications to run unmodified with these types of sensors too.
>>>>> A reasonable aim perhaps, as opposed to locally implemented polling
>>>>> all over the place (which should definitely not happen).
>>>>
>>>> Yes, exactly! :)
>>> I'm actually beginning to have my doubts about whether my initial
>>> response here was right.  Given your use cases and the complexity
>>> of matching frequencies, I think you are almost always better
>>> off implementing it in the drivers themselves (which can decide
>>> if there is new data or not).
>>
>> I agree. Although I think having some helper functions in IIO core
>> should help keep the driver modifications to a minimum.
>>
>>>>
>>>>>
>>>>> An alternative the zio guys used was to create timer
>>>>> based triggers on the fly purely based on them being requested
>>>>> (with an appropriate name IIRC)...  Doesn't quite solve your
>>>>> problem though as still needs userspace changes.
>>>>>>
>>>>>> We will split the current design into 3 parts:
>>>>>>
>>>>>> (1) IIO trigger handling generic part, which offers an API
>>>>>>      to register/unregister/get a reference to a trigger type
>>>>>>
>>>>>> (2) IIO configfs part that gets a reference to a trigger type and
>>>>>> handles user requests to create/destroy a trigger.
>>>>>>
>>>>>> (3) IIO hrtimer driver that use the API at (1) for registering
>>>>>> / deregistering a trigger type.
>>>>>>
>>>>>> Then, each driver in the case that it doesn't have a "real" trigger,
>>>>>> will get a reference to a "hrtimer" trigger type and create
>>>>>> a new "hrtimer" trigger.
>>>>>>
>>>>>> Does this look good to you? This could be easily done from
>>>>>> userspace, but we will need to modify our userspace applications.
>>>>> My initial thought is this really is a job for userspace, as should
>>>>> be in most cases connecting up the device specific trigger as well
>>>>> (as it's not always the right thing to use).
>>>>>
>>>>> In the general case it is far from obvious that an hrtimer is the
>>>>> right option.  Many usecases would be better off with a sysfs trigger
>>>>> or even running off a different interrupt based trigger entirely.
>>>>>>
>>>>>> Also, handling sampling_frequency/delay would be transparent to
>>>>>> applications if we provide this in kernel API.
>>>>> Not really as sampling frequency in this case should only be an
>>>>> attribute of the trigger, not the device.  We only really allow
>>>>> it for the device rather than always the triggers on the basis
>>>>> that it has impacts on other device elements (events etc).
>>>>
>>>> Well, if the trigger is directly created from the driver then we will
>>>> have a reference to a function that sets its delay.
>>>>
>>>> Each time the user sets the sampling frequency for the device
>>>> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
>>>> case we also call set_delay(). Thus we always have have device
>>>> sampling frequency in sync with trigger delay.
>>>>
>>>> I know this sounds crazy :) and I don't like it. I am not sure how
>>>> to guarantee that device frequency is always in sync with trigger
>>>> delay.
>>> You can't that I can see.  Hence you've convinced me this is a bad
>>> idea.
>>
>> I agree we can't accurately synchronize the device frequency with the
>> trigger frequency, but I think we can get it do a good enough level. I
>> also see no difference in accuracy in doing it from userspace or from
>> the driver itself.
>>
>> Why do you think it is a bad idea to synchronize the trigger sample
>> frequency from the device sample frequency handler? I think that if we
>> want to provide the hrtimer trigger from the driver itself, we should
>> do this as well, otherwise there is no real gain to userspace.
>>
> You will get beating problems.  So once in a while you'll either miss
> a reading or you'll get a bonus one.
>
> The point about doing it in the driver is that you actually poll at a
> higher frequency, but only fire a trigger if there is something there
> (by reading the dataready register or equivalent).
>
> Thus your trigger acts more or less the same as a wired interrupt.
> You can't do this with an external hrtimer trigger.

Polling will have a large impact on power. I know that some people
will care more about saving power and that is why I think its worth
having the external hrtimer trigger option.

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 15/04/15 23:15, Octavian Purdila wrote:
> On Wed, Apr 15, 2015 at 2:34 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 15/04/15 21:58, Octavian Purdila wrote:
>>> On Sun, Apr 12, 2015 at 8:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 10/04/15 14:43, Daniel Baluta wrote:
>>>>> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>> On 08/04/15 14:30, Daniel Baluta wrote:
>>>>>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>>>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>>>>>> configfs mount point root, with one default group for "triggers".
>>>>>>>>
>>>>>>>> It offers basic interface for registering software trigger types. Next patches
>>>>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>>>>>
>>>>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>>>>>> support for IIO works.
>>>>>>>>
>>>>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I also need your feedback on the following problem.
>>>>>>>
>>>>>>> We would like to be able to create hrtimer triggers from within
>>>>>>> a kernel module. There are cases where we don't have an interrupt
>>>>>>> pin or the interrupt pin is not connected, and we would like
>>>>>>> that applications to run unmodified with these types of sensors too.
>>>>>> A reasonable aim perhaps, as opposed to locally implemented polling
>>>>>> all over the place (which should definitely not happen).
>>>>>
>>>>> Yes, exactly! :)
>>>> I'm actually beginning to have my doubts about whether my initial
>>>> response here was right.  Given your use cases and the complexity
>>>> of matching frequencies, I think you are almost always better
>>>> off implementing it in the drivers themselves (which can decide
>>>> if there is new data or not).
>>>
>>> I agree. Although I think having some helper functions in IIO core
>>> should help keep the driver modifications to a minimum.
>>>
>>>>>
>>>>>>
>>>>>> An alternative the zio guys used was to create timer
>>>>>> based triggers on the fly purely based on them being requested
>>>>>> (with an appropriate name IIRC)...  Doesn't quite solve your
>>>>>> problem though as still needs userspace changes.
>>>>>>>
>>>>>>> We will split the current design into 3 parts:
>>>>>>>
>>>>>>> (1) IIO trigger handling generic part, which offers an API
>>>>>>>      to register/unregister/get a reference to a trigger type
>>>>>>>
>>>>>>> (2) IIO configfs part that gets a reference to a trigger type and
>>>>>>> handles user requests to create/destroy a trigger.
>>>>>>>
>>>>>>> (3) IIO hrtimer driver that use the API at (1) for registering
>>>>>>> / deregistering a trigger type.
>>>>>>>
>>>>>>> Then, each driver in the case that it doesn't have a "real" trigger,
>>>>>>> will get a reference to a "hrtimer" trigger type and create
>>>>>>> a new "hrtimer" trigger.
>>>>>>>
>>>>>>> Does this look good to you? This could be easily done from
>>>>>>> userspace, but we will need to modify our userspace applications.
>>>>>> My initial thought is this really is a job for userspace, as should
>>>>>> be in most cases connecting up the device specific trigger as well
>>>>>> (as it's not always the right thing to use).
>>>>>>
>>>>>> In the general case it is far from obvious that an hrtimer is the
>>>>>> right option.  Many usecases would be better off with a sysfs trigger
>>>>>> or even running off a different interrupt based trigger entirely.
>>>>>>>
>>>>>>> Also, handling sampling_frequency/delay would be transparent to
>>>>>>> applications if we provide this in kernel API.
>>>>>> Not really as sampling frequency in this case should only be an
>>>>>> attribute of the trigger, not the device.  We only really allow
>>>>>> it for the device rather than always the triggers on the basis
>>>>>> that it has impacts on other device elements (events etc).
>>>>>
>>>>> Well, if the trigger is directly created from the driver then we will
>>>>> have a reference to a function that sets its delay.
>>>>>
>>>>> Each time the user sets the sampling frequency for the device
>>>>> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
>>>>> case we also call set_delay(). Thus we always have have device
>>>>> sampling frequency in sync with trigger delay.
>>>>>
>>>>> I know this sounds crazy :) and I don't like it. I am not sure how
>>>>> to guarantee that device frequency is always in sync with trigger
>>>>> delay.
>>>> You can't that I can see.  Hence you've convinced me this is a bad
>>>> idea.
>>>
>>> I agree we can't accurately synchronize the device frequency with the
>>> trigger frequency, but I think we can get it do a good enough level. I
>>> also see no difference in accuracy in doing it from userspace or from
>>> the driver itself.
>>>
>>> Why do you think it is a bad idea to synchronize the trigger sample
>>> frequency from the device sample frequency handler? I think that if we
>>> want to provide the hrtimer trigger from the driver itself, we should
>>> do this as well, otherwise there is no real gain to userspace.
>>>
>> You will get beating problems.  So once in a while you'll either miss
>> a reading or you'll get a bonus one.
>>
>> The point about doing it in the driver is that you actually poll at a
>> higher frequency, but only fire a trigger if there is something there
>> (by reading the dataready register or equivalent).
>>
>> Thus your trigger acts more or less the same as a wired interrupt.
>> You can't do this with an external hrtimer trigger.
> 
> Polling will have a large impact on power. I know that some people
> will care more about saving power and that is why I think its worth
> having the external hrtimer trigger option.
> 
Two different use cases...

Either you are interested in 'faking' the data ready signal, in which case polling
is the way to go as nothing else will get you there.  Note that ranged sleeps can be
used to limit the power usage pain.

or you are interested in much lower frequency sampling in which case you use
an hrtimer trigger, but run it at way below the sample rate.  You will always
get new data, but there is no control of when exactly that data was grabbed
and you will loose samples.

The first was what I thought we were discussing... 

Any attempt to synchronize timing is a non starter as it will beat as any two
similar frequencies will so you will loose data.

J

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Octavian Purdila]

On Sat, Apr 18, 2015 at 12:30 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On 15/04/15 23:15, Octavian Purdila wrote:
> > On Wed, Apr 15, 2015 at 2:34 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> >> On 15/04/15 21:58, Octavian Purdila wrote:
> >>> On Sun, Apr 12, 2015 at 8:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> >>>> On 10/04/15 14:43, Daniel Baluta wrote:
> >>>>> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> >>>>>> On 08/04/15 14:30, Daniel Baluta wrote:
> >>>>>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
> >>>>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
> >>>>>>>> configfs mount point root, with one default group for "triggers".
> >>>>>>>>
> >>>>>>>> It offers basic interface for registering software trigger types. Next patches
> >>>>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
> >>>>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
> >>>>>>>>
> >>>>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
> >>>>>>>> support for IIO works.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> >>>>>>>
> >>>>>>> Hi all,
> >>>>>>>
> >>>>>>> I also need your feedback on the following problem.
> >>>>>>>
> >>>>>>> We would like to be able to create hrtimer triggers from within
> >>>>>>> a kernel module. There are cases where we don't have an interrupt
> >>>>>>> pin or the interrupt pin is not connected, and we would like
> >>>>>>> that applications to run unmodified with these types of sensors too.
> >>>>>> A reasonable aim perhaps, as opposed to locally implemented polling
> >>>>>> all over the place (which should definitely not happen).
> >>>>>
> >>>>> Yes, exactly! :)
> >>>> I'm actually beginning to have my doubts about whether my initial
> >>>> response here was right.  Given your use cases and the complexity
> >>>> of matching frequencies, I think you are almost always better
> >>>> off implementing it in the drivers themselves (which can decide
> >>>> if there is new data or not).
> >>>
> >>> I agree. Although I think having some helper functions in IIO core
> >>> should help keep the driver modifications to a minimum.
> >>>
> >>>>>
> >>>>>>
> >>>>>> An alternative the zio guys used was to create timer
> >>>>>> based triggers on the fly purely based on them being requested
> >>>>>> (with an appropriate name IIRC)...  Doesn't quite solve your
> >>>>>> problem though as still needs userspace changes.
> >>>>>>>
> >>>>>>> We will split the current design into 3 parts:
> >>>>>>>
> >>>>>>> (1) IIO trigger handling generic part, which offers an API
> >>>>>>>      to register/unregister/get a reference to a trigger type
> >>>>>>>
> >>>>>>> (2) IIO configfs part that gets a reference to a trigger type and
> >>>>>>> handles user requests to create/destroy a trigger.
> >>>>>>>
> >>>>>>> (3) IIO hrtimer driver that use the API at (1) for registering
> >>>>>>> / deregistering a trigger type.
> >>>>>>>
> >>>>>>> Then, each driver in the case that it doesn't have a "real" trigger,
> >>>>>>> will get a reference to a "hrtimer" trigger type and create
> >>>>>>> a new "hrtimer" trigger.
> >>>>>>>
> >>>>>>> Does this look good to you? This could be easily done from
> >>>>>>> userspace, but we will need to modify our userspace applications.
> >>>>>> My initial thought is this really is a job for userspace, as should
> >>>>>> be in most cases connecting up the device specific trigger as well
> >>>>>> (as it's not always the right thing to use).
> >>>>>>
> >>>>>> In the general case it is far from obvious that an hrtimer is the
> >>>>>> right option.  Many usecases would be better off with a sysfs trigger
> >>>>>> or even running off a different interrupt based trigger entirely.
> >>>>>>>
> >>>>>>> Also, handling sampling_frequency/delay would be transparent to
> >>>>>>> applications if we provide this in kernel API.
> >>>>>> Not really as sampling frequency in this case should only be an
> >>>>>> attribute of the trigger, not the device.  We only really allow
> >>>>>> it for the device rather than always the triggers on the basis
> >>>>>> that it has impacts on other device elements (events etc).
> >>>>>
> >>>>> Well, if the trigger is directly created from the driver then we will
> >>>>> have a reference to a function that sets its delay.
> >>>>>
> >>>>> Each time the user sets the sampling frequency for the device
> >>>>> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
> >>>>> case we also call set_delay(). Thus we always have have device
> >>>>> sampling frequency in sync with trigger delay.
> >>>>>
> >>>>> I know this sounds crazy :) and I don't like it. I am not sure how
> >>>>> to guarantee that device frequency is always in sync with trigger
> >>>>> delay.
> >>>> You can't that I can see.  Hence you've convinced me this is a bad
> >>>> idea.
> >>>
> >>> I agree we can't accurately synchronize the device frequency with the
> >>> trigger frequency, but I think we can get it do a good enough level. I
> >>> also see no difference in accuracy in doing it from userspace or from
> >>> the driver itself.
> >>>
> >>> Why do you think it is a bad idea to synchronize the trigger sample
> >>> frequency from the device sample frequency handler? I think that if we
> >>> want to provide the hrtimer trigger from the driver itself, we should
> >>> do this as well, otherwise there is no real gain to userspace.
> >>>
> >> You will get beating problems.  So once in a while you'll either miss
> >> a reading or you'll get a bonus one.
> >>
> >> The point about doing it in the driver is that you actually poll at a
> >> higher frequency, but only fire a trigger if there is something there
> >> (by reading the dataready register or equivalent).
> >>
> >> Thus your trigger acts more or less the same as a wired interrupt.
> >> You can't do this with an external hrtimer trigger.
> >
> > Polling will have a large impact on power. I know that some people
> > will care more about saving power and that is why I think its worth
> > having the external hrtimer trigger option.
> >
> Two different use cases...
>
> Either you are interested in 'faking' the data ready signal, in which case polling
> is the way to go as nothing else will get you there.  Note that ranged sleeps can be
> used to limit the power usage pain.
>

Sure, but ranged sleeps will use a hrtimer anyway :) And because of
that we have no guarantee that we will not miss data.

> or you are interested in much lower frequency sampling in which case you use
> an hrtimer trigger, but run it at way below the sample rate.  You will always
> get new data, but there is no control of when exactly that data was grabbed
> and you will loose samples.
>
> The first was what I thought we were discussing...
>
> Any attempt to synchronize timing is a non starter as it will beat as any two
> similar frequencies will so you will loose data.
>

Maybe synchronizing is a too stronger word here. What I meant was that
when the device sampling frequency is changed to also change the
hrtimer interval too (basically to 1/freq).

Yes, we will occasionally loose data but it is the best we can do if
we don't have the interrupt available. Fully polling will waste too
much power and ranged sleeps can still lead to missing samples.

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 22/04/15 21:34, Octavian Purdila wrote:
> On Sat, Apr 18, 2015 at 12:30 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> On 15/04/15 23:15, Octavian Purdila wrote:
>>> On Wed, Apr 15, 2015 at 2:34 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 15/04/15 21:58, Octavian Purdila wrote:
>>>>> On Sun, Apr 12, 2015 at 8:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>> On 10/04/15 14:43, Daniel Baluta wrote:
>>>>>>> On Thu, Apr 9, 2015 at 8:18 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>>>> On 08/04/15 14:30, Daniel Baluta wrote:
>>>>>>>>> On Mon, Apr 6, 2015 at 5:18 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>>>>>>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>>>>>>>> configfs mount point root, with one default group for "triggers".
>>>>>>>>>>
>>>>>>>>>> It offers basic interface for registering software trigger types. Next patches
>>>>>>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>>>>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>>>>>>>
>>>>>>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>>>>>>>> support for IIO works.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> I also need your feedback on the following problem.
>>>>>>>>>
>>>>>>>>> We would like to be able to create hrtimer triggers from within
>>>>>>>>> a kernel module. There are cases where we don't have an interrupt
>>>>>>>>> pin or the interrupt pin is not connected, and we would like
>>>>>>>>> that applications to run unmodified with these types of sensors too.
>>>>>>>> A reasonable aim perhaps, as opposed to locally implemented polling
>>>>>>>> all over the place (which should definitely not happen).
>>>>>>>
>>>>>>> Yes, exactly! :)
>>>>>> I'm actually beginning to have my doubts about whether my initial
>>>>>> response here was right.  Given your use cases and the complexity
>>>>>> of matching frequencies, I think you are almost always better
>>>>>> off implementing it in the drivers themselves (which can decide
>>>>>> if there is new data or not).
>>>>>
>>>>> I agree. Although I think having some helper functions in IIO core
>>>>> should help keep the driver modifications to a minimum.
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> An alternative the zio guys used was to create timer
>>>>>>>> based triggers on the fly purely based on them being requested
>>>>>>>> (with an appropriate name IIRC)...  Doesn't quite solve your
>>>>>>>> problem though as still needs userspace changes.
>>>>>>>>>
>>>>>>>>> We will split the current design into 3 parts:
>>>>>>>>>
>>>>>>>>> (1) IIO trigger handling generic part, which offers an API
>>>>>>>>>      to register/unregister/get a reference to a trigger type
>>>>>>>>>
>>>>>>>>> (2) IIO configfs part that gets a reference to a trigger type and
>>>>>>>>> handles user requests to create/destroy a trigger.
>>>>>>>>>
>>>>>>>>> (3) IIO hrtimer driver that use the API at (1) for registering
>>>>>>>>> / deregistering a trigger type.
>>>>>>>>>
>>>>>>>>> Then, each driver in the case that it doesn't have a "real" trigger,
>>>>>>>>> will get a reference to a "hrtimer" trigger type and create
>>>>>>>>> a new "hrtimer" trigger.
>>>>>>>>>
>>>>>>>>> Does this look good to you? This could be easily done from
>>>>>>>>> userspace, but we will need to modify our userspace applications.
>>>>>>>> My initial thought is this really is a job for userspace, as should
>>>>>>>> be in most cases connecting up the device specific trigger as well
>>>>>>>> (as it's not always the right thing to use).
>>>>>>>>
>>>>>>>> In the general case it is far from obvious that an hrtimer is the
>>>>>>>> right option.  Many usecases would be better off with a sysfs trigger
>>>>>>>> or even running off a different interrupt based trigger entirely.
>>>>>>>>>
>>>>>>>>> Also, handling sampling_frequency/delay would be transparent to
>>>>>>>>> applications if we provide this in kernel API.
>>>>>>>> Not really as sampling frequency in this case should only be an
>>>>>>>> attribute of the trigger, not the device.  We only really allow
>>>>>>>> it for the device rather than always the triggers on the basis
>>>>>>>> that it has impacts on other device elements (events etc).
>>>>>>>
>>>>>>> Well, if the trigger is directly created from the driver then we will
>>>>>>> have a reference to a function that sets its delay.
>>>>>>>
>>>>>>> Each time the user sets the sampling frequency for the device
>>>>>>> with hit write_raw and there on IIO_CHAN_INFO_SAMP_FREQ
>>>>>>> case we also call set_delay(). Thus we always have have device
>>>>>>> sampling frequency in sync with trigger delay.
>>>>>>>
>>>>>>> I know this sounds crazy :) and I don't like it. I am not sure how
>>>>>>> to guarantee that device frequency is always in sync with trigger
>>>>>>> delay.
>>>>>> You can't that I can see.  Hence you've convinced me this is a bad
>>>>>> idea.
>>>>>
>>>>> I agree we can't accurately synchronize the device frequency with the
>>>>> trigger frequency, but I think we can get it do a good enough level. I
>>>>> also see no difference in accuracy in doing it from userspace or from
>>>>> the driver itself.
>>>>>
>>>>> Why do you think it is a bad idea to synchronize the trigger sample
>>>>> frequency from the device sample frequency handler? I think that if we
>>>>> want to provide the hrtimer trigger from the driver itself, we should
>>>>> do this as well, otherwise there is no real gain to userspace.
>>>>>
>>>> You will get beating problems.  So once in a while you'll either miss
>>>> a reading or you'll get a bonus one.
>>>>
>>>> The point about doing it in the driver is that you actually poll at a
>>>> higher frequency, but only fire a trigger if there is something there
>>>> (by reading the dataready register or equivalent).
>>>>
>>>> Thus your trigger acts more or less the same as a wired interrupt.
>>>> You can't do this with an external hrtimer trigger.
>>>
>>> Polling will have a large impact on power. I know that some people
>>> will care more about saving power and that is why I think its worth
>>> having the external hrtimer trigger option.
>>>
>> Two different use cases...
>>
>> Either you are interested in 'faking' the data ready signal, in which case polling
>> is the way to go as nothing else will get you there.  Note that ranged sleeps can be
>> used to limit the power usage pain.
>>
> 
> Sure, but ranged sleeps will use a hrtimer anyway :) And because of
> that we have no guarantee that we will not miss data.
Should be fine if you poll at double the sampling frequency.. or at least
it will loose samples very rarely.
> 
>> or you are interested in much lower frequency sampling in which case you use
>> an hrtimer trigger, but run it at way below the sample rate.  You will always
>> get new data, but there is no control of when exactly that data was grabbed
>> and you will loose samples.
>>
>> The first was what I thought we were discussing...
>>
>> Any attempt to synchronize timing is a non starter as it will beat as any two
>> similar frequencies will so you will loose data.
>>
> 
> Maybe synchronizing is a too stronger word here. What I meant was that
> when the device sampling frequency is changed to also change the
> hrtimer interval too (basically to 1/freq).
> 
> Yes, we will occasionally loose data but it is the best we can do if
> we don't have the interrupt available. Fully polling will waste too
> much power and ranged sleeps can still lead to missing samples.
Would polling at double the sampling frequency prove too power consuming?
That we can't do with a straight hrtimer trigger, but can be easily done
if we do it in driver (with out ever pushing missing data or doubled
data to userspace - assuming the device has a new data element that is
poll-able).

If you try to match you either get double samples (which might
be detectable, but we have no facility to indicate it in the buffer),
or half samples.

If anyone is trying to do integration either case could leave them
with a comically incorrect answer if undetected. It's bad enough even
with clean well timed data!

J

> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 06/04/15 15:18, Daniel Baluta wrote:
> This module is the core of IIO configfs. It creates the "iio" subsystem under
> configfs mount point root, with one default group for "triggers".
> 
> It offers basic interface for registering software trigger types. Next patches
> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
> create a new kernel module which implements iio_configfs_trigger.h interface.
> 
> See Documentation/iio/iio_configfs.txt for more details on how configfs
> support for IIO works.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Looks good and is actually a fair bit simpler than I expected which is nice!
As an ideal aim, I'd like there to be no need for the core to have any
idea what triggers exist and can be registered (beyond preventing naming
clashes etc). Actually, not much would be needed to implement that I think, just
using a list and looking up in it (we aren't on a particularly fast path so
don't need anything clever) instead of a simple array.  A touch of locking
probably to avoid two many simultaneous users of that list...

Hmm. having read through the patches, are we fundamentally limited to having to
specify the entire directory structure before bringing up configfs?

> ---
> 
> Changes since v2:
> 	* Fix build errors by compiling industrialio-configfs.ko 
> 	as a separate module, as reported by Paul.
> 
> Changes since v1:
> 	* addressed feedback from v1:
> 		* https://lkml.org/lkml/2015/3/25/647
> 	* create a directory per trigger type
> 	* removed activate and type attributes
> 	
>  drivers/iio/Kconfig                      |   8 ++
>  drivers/iio/Makefile                     |   1 +
>  drivers/iio/industrialio-configfs.c      | 127 +++++++++++++++++++++++++++++++
>  include/linux/iio/iio_configfs_trigger.h |  40 ++++++++++
>  4 files changed, 176 insertions(+)
>  create mode 100644 drivers/iio/industrialio-configfs.c
>  create mode 100644 include/linux/iio/iio_configfs_trigger.h
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 4011eff..41d5863 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 698afc2..72e85c42 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -8,6 +8,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>  industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>  
> +obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
>  
> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
> new file mode 100644
> index 0000000..d8ffd76
> --- /dev/null
> +++ b/drivers/iio/industrialio-configfs.c
> @@ -0,0 +1,127 @@
> +/*
> + * 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/iio_configfs_trigger.h>
> +
> +static struct iio_configfs_trigger_type *trigger_types[IIO_TRIGGER_TYPE_MAX];
> +static DEFINE_RWLOCK(trigger_types_lock);
> +
> +int register_configfs_trigger(struct iio_configfs_trigger_type *t)
> +{
> +	int index = t->type;
> +	int ret = 0;
> +
> +	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
> +		return -EINVAL;
> +
> +	write_lock(&trigger_types_lock);
> +	if (trigger_types[index])
> +		ret = -EBUSY;
> +	else
> +		trigger_types[index] = t;
> +	write_unlock(&trigger_types_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(register_configfs_trigger);
> +
> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *t)
> +{
> +	int index = t->type;
> +
> +	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
> +		return -EINVAL;
> +
> +	write_lock(&trigger_types_lock);
> +	trigger_types[index] = NULL;
> +	write_unlock(&trigger_types_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(unregister_configfs_trigger);
> +
> +static
> +struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
> +{
> +	struct iio_configfs_trigger_type *t;
> +
> +	if (type < 0 || type >= IIO_TRIGGER_TYPE_MAX)
> +		return ERR_PTR(-EINVAL);
> +
> +	read_lock(&trigger_types_lock);
> +	t = trigger_types[type];
> +	if (t && !try_module_get(t->owner))
> +		t = NULL;
> +	read_unlock(&trigger_types_lock);
> +
> +	return t;
> +}
> +
> +static struct config_group *iio_triggers_default_groups[] = {
> +	NULL
> +};
> +
> +static struct config_item_type iio_triggers_group_type = {
> +	.ct_owner       = THIS_MODULE,
> +};
> +
> +static struct config_group iio_triggers_group = {
> +	.cg_item = {
> +		.ci_namebuf = "triggers",
> +		.ci_type = &iio_triggers_group_type,
> +	},
> +	.default_groups = iio_triggers_default_groups,
> +};
> +
> +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");
> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
> new file mode 100644
> index 0000000..75e76f2
> --- /dev/null
> +++ b/include/linux/iio/iio_configfs_trigger.h
> @@ -0,0 +1,40 @@
> +#ifndef __IIO_CONFIGFS_TRIGGER
> +#define __IIO_CONFIGFS_TRIGGER
> +
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +
> +#define module_iio_configfs_trigger_driver(__iio_configfs_trigger) \
> +	module_driver(__iio_configfs_trigger, register_configfs_trigger, \
> +		      unregister_configfs_trigger)
> +
> +enum {
Is this enum actually needed as such? If we can avoid explicit need
to list triggers in one place it would be nice.

As I understand it, the point of this is to allow allocating of various
arrays for easy lookup etc.  How about we switch over to a list and
lookup based on magic provided by the trigger? (string or address most likely).
> +	IIO_TRIGGER_TYPE_MAX,
> +};
> +
> +struct iio_configfs_trigger_ops;
> +
> +struct iio_configfs_trigger_type {
> +	int type;
> +	struct module *owner;
> +	struct iio_configfs_trigger_ops *trigger_ops;
> +};
> +
> +struct iio_configfs_trigger_info {
> +	char *name;
> +	struct iio_trigger *trigger;
> +	struct iio_configfs_trigger_type *trigger_type;
> +};
> +
> +struct iio_configfs_trigger_ops {
I wonder if we can make these more flexible from the start because
avoiding churn in callbacks is always nice as us ensuring we have
a manageable number in the long run!  Also we might want to play
the 'fixed point' games we play elsewhere in IIO to allow us to have
a very wide range of precisions.
enum iio_configfs_trigger_paramtype = {
     delay,
};

As a final thought, we have standardised on sampling_frequency elsewhere in
IIO (largely based on that was what the first few device used) so perhaps
we can use that here as well instead of delay?

int (*get_parameter)(struct iio_configfs_trigger_info *, enum iio_configfs_triger_paramtype param, unsigned long *);

> +	int (*get_delay)(struct iio_configfs_trigger_info *, unsigned long *);
> +	int (*set_delay)(struct iio_configfs_trigger_info *, unsigned long);
> +	int (*probe)(struct iio_configfs_trigger_info *);
> +	int (*remove)(struct iio_configfs_trigger_info *);
> +};
> +
> +int register_configfs_trigger(struct iio_configfs_trigger_type *);
> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *);
> +
> +#endif /* __IIO_CONFIGFS_TRIGGER */
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 09/04/15 18:15, Jonathan Cameron wrote:
> On 06/04/15 15:18, Daniel Baluta wrote:
>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>> configfs mount point root, with one default group for "triggers".
>>
>> It offers basic interface for registering software trigger types. Next patches
>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>
>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>> support for IIO works.
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> Looks good and is actually a fair bit simpler than I expected which is nice!
> As an ideal aim, I'd like there to be no need for the core to have any
> idea what triggers exist and can be registered (beyond preventing naming
> clashes etc). Actually, not much would be needed to implement that I think, just
> using a list and looking up in it (we aren't on a particularly fast path so
> don't need anything clever) instead of a simple array.  A touch of locking
> probably to avoid two many simultaneous users of that list...
> 
> Hmm. having read through the patches, are we fundamentally limited to having to
> specify the entire directory structure before bringing up configfs?
As I read more on this, I think the definition of a 'subsystem' in configfs is
very restricted. It means a tight group of devices sharing a prior
known interface with no ability to add extras later.
It's sort of like the difference between a class and a bus in sysfs (but more limited
actually that's a rubbish analogy).  Thus either we have to do as here
and have the core know all about everything it might want to setup in advance
of registering the iio configfs subsystem or we need to make a whole load of
different configfs subsystems. Basically boils down to one per module.  We lose
the grouping convenience of the current structure but gain in separation.

So option 1 we have effectively to predefine anything that might turn up
in a module...

/config/iio/triggers/hrtimer/instance1/..
/config/iio/triggers/sysfs/instance2/..
/config/iio/virtualdevs/hwmon/iiohwmoninstance1/..
/config/iio/virtualdevs/input/iioinputinstance1/..
/config/iio/maps/channelmapisntance1/.. 
etc (actually fine for the maps as they are coming from the core anyway
and will be known in advance)

Option 2 we have complete flexibility as its down to the relevant drivers

/config/iio-trigger-hrtimer/instance/..
/config/iio-trigger-sysfs/instance/..
/config/iio-virtual-hwmon/instance/..
/config/iio-virtual-input/instance/..

but we lose the grouping by directory which is a conceptual shame.

Am I missing something important here?  (pretty new to configfs!)

Just for others not familiar with IIO who might read this. We have a number
of userspace created elements but each type is provided by a different module,
hence it's a real pain having to have them all defined at configfs subsystem
initialization as it adds tight coupling we don't otherwise have!

J
> 
>> ---
>>
>> Changes since v2:
>> 	* Fix build errors by compiling industrialio-configfs.ko 
>> 	as a separate module, as reported by Paul.
>>
>> Changes since v1:
>> 	* addressed feedback from v1:
>> 		* https://lkml.org/lkml/2015/3/25/647
>> 	* create a directory per trigger type
>> 	* removed activate and type attributes
>> 	
>>  drivers/iio/Kconfig                      |   8 ++
>>  drivers/iio/Makefile                     |   1 +
>>  drivers/iio/industrialio-configfs.c      | 127 +++++++++++++++++++++++++++++++
>>  include/linux/iio/iio_configfs_trigger.h |  40 ++++++++++
>>  4 files changed, 176 insertions(+)
>>  create mode 100644 drivers/iio/industrialio-configfs.c
>>  create mode 100644 include/linux/iio/iio_configfs_trigger.h
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 4011eff..41d5863 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 698afc2..72e85c42 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -8,6 +8,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>  industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>>  
>> +obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
>>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
>>  
>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>> new file mode 100644
>> index 0000000..d8ffd76
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-configfs.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * 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/iio_configfs_trigger.h>
>> +
>> +static struct iio_configfs_trigger_type *trigger_types[IIO_TRIGGER_TYPE_MAX];
>> +static DEFINE_RWLOCK(trigger_types_lock);
>> +
>> +int register_configfs_trigger(struct iio_configfs_trigger_type *t)
>> +{
>> +	int index = t->type;
>> +	int ret = 0;
>> +
>> +	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>> +		return -EINVAL;
>> +
>> +	write_lock(&trigger_types_lock);
>> +	if (trigger_types[index])
>> +		ret = -EBUSY;
>> +	else
>> +		trigger_types[index] = t;
>> +	write_unlock(&trigger_types_lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(register_configfs_trigger);
>> +
>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *t)
>> +{
>> +	int index = t->type;
>> +
>> +	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>> +		return -EINVAL;
>> +
>> +	write_lock(&trigger_types_lock);
>> +	trigger_types[index] = NULL;
>> +	write_unlock(&trigger_types_lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(unregister_configfs_trigger);
>> +
>> +static
>> +struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>> +{
>> +	struct iio_configfs_trigger_type *t;
>> +
>> +	if (type < 0 || type >= IIO_TRIGGER_TYPE_MAX)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	read_lock(&trigger_types_lock);
>> +	t = trigger_types[type];
>> +	if (t && !try_module_get(t->owner))
>> +		t = NULL;
>> +	read_unlock(&trigger_types_lock);
>> +
>> +	return t;
>> +}
>> +
>> +static struct config_group *iio_triggers_default_groups[] = {
>> +	NULL
>> +};
>> +
>> +static struct config_item_type iio_triggers_group_type = {
>> +	.ct_owner       = THIS_MODULE,
>> +};
>> +
>> +static struct config_group iio_triggers_group = {
>> +	.cg_item = {
>> +		.ci_namebuf = "triggers",
>> +		.ci_type = &iio_triggers_group_type,
>> +	},
>> +	.default_groups = iio_triggers_default_groups,
>> +};
>> +
>> +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");
>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>> new file mode 100644
>> index 0000000..75e76f2
>> --- /dev/null
>> +++ b/include/linux/iio/iio_configfs_trigger.h
>> @@ -0,0 +1,40 @@
>> +#ifndef __IIO_CONFIGFS_TRIGGER
>> +#define __IIO_CONFIGFS_TRIGGER
>> +
>> +#include <linux/device.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +
>> +#define module_iio_configfs_trigger_driver(__iio_configfs_trigger) \
>> +	module_driver(__iio_configfs_trigger, register_configfs_trigger, \
>> +		      unregister_configfs_trigger)
>> +
>> +enum {
> Is this enum actually needed as such? If we can avoid explicit need
> to list triggers in one place it would be nice.
> 
> As I understand it, the point of this is to allow allocating of various
> arrays for easy lookup etc.  How about we switch over to a list and
> lookup based on magic provided by the trigger? (string or address most likely).
>> +	IIO_TRIGGER_TYPE_MAX,
>> +};
>> +
>> +struct iio_configfs_trigger_ops;
>> +
>> +struct iio_configfs_trigger_type {
>> +	int type;
>> +	struct module *owner;
>> +	struct iio_configfs_trigger_ops *trigger_ops;
>> +};
>> +
>> +struct iio_configfs_trigger_info {
>> +	char *name;
>> +	struct iio_trigger *trigger;
>> +	struct iio_configfs_trigger_type *trigger_type;
>> +};
>> +
>> +struct iio_configfs_trigger_ops {
> I wonder if we can make these more flexible from the start because
> avoiding churn in callbacks is always nice as us ensuring we have
> a manageable number in the long run!  Also we might want to play
> the 'fixed point' games we play elsewhere in IIO to allow us to have
> a very wide range of precisions.
> enum iio_configfs_trigger_paramtype = {
>      delay,
> };
> 
> As a final thought, we have standardised on sampling_frequency elsewhere in
> IIO (largely based on that was what the first few device used) so perhaps
> we can use that here as well instead of delay?
> 
> int (*get_parameter)(struct iio_configfs_trigger_info *, enum iio_configfs_triger_paramtype param, unsigned long *);
> 
>> +	int (*get_delay)(struct iio_configfs_trigger_info *, unsigned long *);
>> +	int (*set_delay)(struct iio_configfs_trigger_info *, unsigned long);
>> +	int (*probe)(struct iio_configfs_trigger_info *);
>> +	int (*remove)(struct iio_configfs_trigger_info *);
>> +};
>> +
>> +int register_configfs_trigger(struct iio_configfs_trigger_type *);
>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *);
>> +
>> +#endif /* __IIO_CONFIGFS_TRIGGER */
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 09/04/15 18:40, Jonathan Cameron wrote:
> On 09/04/15 18:15, Jonathan Cameron wrote:
>> On 06/04/15 15:18, Daniel Baluta wrote:
>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>> configfs mount point root, with one default group for "triggers".
>>>
>>> It offers basic interface for registering software trigger types. Next patches
>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>
>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>> support for IIO works.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> Looks good and is actually a fair bit simpler than I expected which is nice!
>> As an ideal aim, I'd like there to be no need for the core to have any
>> idea what triggers exist and can be registered (beyond preventing naming
>> clashes etc). Actually, not much would be needed to implement that I think, just
>> using a list and looking up in it (we aren't on a particularly fast path so
>> don't need anything clever) instead of a simple array.  A touch of locking
>> probably to avoid two many simultaneous users of that list...
>>
>> Hmm. having read through the patches, are we fundamentally limited to having to
>> specify the entire directory structure before bringing up configfs?
> As I read more on this, I think the definition of a 'subsystem' in configfs is
> very restricted. It means a tight group of devices sharing a prior
> known interface with no ability to add extras later.
> It's sort of like the difference between a class and a bus in sysfs (but more limited
> actually that's a rubbish analogy).  Thus either we have to do as here
> and have the core know all about everything it might want to setup in advance
> of registering the iio configfs subsystem or we need to make a whole load of
> different configfs subsystems. Basically boils down to one per module.  We lose
> the grouping convenience of the current structure but gain in separation.
> 
> So option 1 we have effectively to predefine anything that might turn up
> in a module...
> 
> /config/iio/triggers/hrtimer/instance1/..
> /config/iio/triggers/sysfs/instance2/..
> /config/iio/virtualdevs/hwmon/iiohwmoninstance1/..
> /config/iio/virtualdevs/input/iioinputinstance1/..
> /config/iio/maps/channelmapisntance1/.. 
> etc (actually fine for the maps as they are coming from the core anyway
> and will be known in advance)
> 
> Option 2 we have complete flexibility as its down to the relevant drivers
> 
> /config/iio-trigger-hrtimer/instance/..
> /config/iio-trigger-sysfs/instance/..
> /config/iio-virtual-hwmon/instance/..
> /config/iio-virtual-input/instance/..
> 
> but we lose the grouping by directory which is a conceptual shame.
> 
> Am I missing something important here?  (pretty new to configfs!)
> 
> Just for others not familiar with IIO who might read this. We have a number
> of userspace created elements but each type is provided by a different module,
> hence it's a real pain having to have them all defined at configfs subsystem
> initialization as it adds tight coupling we don't otherwise have!

For reference of others the usb gadget code is a good example of how to handle
these sort of cross dependencies..

the approach used for function drivers there would give us the cleaner option of

/config/iio/triggers/hrtimer-instance1
/config/iio/triggers/hrtimer-instance2
/config/iio/triggers/sysfs-instance1
/config/iio/virtualdevs/hwmon-instance1
etc


> 
> J
>>
>>> ---
>>>
>>> Changes since v2:
>>> 	* Fix build errors by compiling industrialio-configfs.ko 
>>> 	as a separate module, as reported by Paul.
>>>
>>> Changes since v1:
>>> 	* addressed feedback from v1:
>>> 		* https://lkml.org/lkml/2015/3/25/647
>>> 	* create a directory per trigger type
>>> 	* removed activate and type attributes
>>> 	
>>>  drivers/iio/Kconfig                      |   8 ++
>>>  drivers/iio/Makefile                     |   1 +
>>>  drivers/iio/industrialio-configfs.c      | 127 +++++++++++++++++++++++++++++++
>>>  include/linux/iio/iio_configfs_trigger.h |  40 ++++++++++
>>>  4 files changed, 176 insertions(+)
>>>  create mode 100644 drivers/iio/industrialio-configfs.c
>>>  create mode 100644 include/linux/iio/iio_configfs_trigger.h
>>>
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 4011eff..41d5863 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 698afc2..72e85c42 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -8,6 +8,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>>  industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>>>  
>>> +obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
>>>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>>>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
>>>  
>>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>>> new file mode 100644
>>> index 0000000..d8ffd76
>>> --- /dev/null
>>> +++ b/drivers/iio/industrialio-configfs.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * 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/iio_configfs_trigger.h>
>>> +
>>> +static struct iio_configfs_trigger_type *trigger_types[IIO_TRIGGER_TYPE_MAX];
>>> +static DEFINE_RWLOCK(trigger_types_lock);
>>> +
>>> +int register_configfs_trigger(struct iio_configfs_trigger_type *t)
>>> +{
>>> +	int index = t->type;
>>> +	int ret = 0;
>>> +
>>> +	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	write_lock(&trigger_types_lock);
>>> +	if (trigger_types[index])
>>> +		ret = -EBUSY;
>>> +	else
>>> +		trigger_types[index] = t;
>>> +	write_unlock(&trigger_types_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL(register_configfs_trigger);
>>> +
>>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *t)
>>> +{
>>> +	int index = t->type;
>>> +
>>> +	if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	write_lock(&trigger_types_lock);
>>> +	trigger_types[index] = NULL;
>>> +	write_unlock(&trigger_types_lock);
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(unregister_configfs_trigger);
>>> +
>>> +static
>>> +struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>>> +{
>>> +	struct iio_configfs_trigger_type *t;
>>> +
>>> +	if (type < 0 || type >= IIO_TRIGGER_TYPE_MAX)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	read_lock(&trigger_types_lock);
>>> +	t = trigger_types[type];
>>> +	if (t && !try_module_get(t->owner))
>>> +		t = NULL;
>>> +	read_unlock(&trigger_types_lock);
>>> +
>>> +	return t;
>>> +}
>>> +
>>> +static struct config_group *iio_triggers_default_groups[] = {
>>> +	NULL
>>> +};
>>> +
>>> +static struct config_item_type iio_triggers_group_type = {
>>> +	.ct_owner       = THIS_MODULE,
>>> +};
>>> +
>>> +static struct config_group iio_triggers_group = {
>>> +	.cg_item = {
>>> +		.ci_namebuf = "triggers",
>>> +		.ci_type = &iio_triggers_group_type,
>>> +	},
>>> +	.default_groups = iio_triggers_default_groups,
>>> +};
>>> +
>>> +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");
>>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>>> new file mode 100644
>>> index 0000000..75e76f2
>>> --- /dev/null
>>> +++ b/include/linux/iio/iio_configfs_trigger.h
>>> @@ -0,0 +1,40 @@
>>> +#ifndef __IIO_CONFIGFS_TRIGGER
>>> +#define __IIO_CONFIGFS_TRIGGER
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/module.h>
>>> +
>>> +#define module_iio_configfs_trigger_driver(__iio_configfs_trigger) \
>>> +	module_driver(__iio_configfs_trigger, register_configfs_trigger, \
>>> +		      unregister_configfs_trigger)
>>> +
>>> +enum {
>> Is this enum actually needed as such? If we can avoid explicit need
>> to list triggers in one place it would be nice.
>>
>> As I understand it, the point of this is to allow allocating of various
>> arrays for easy lookup etc.  How about we switch over to a list and
>> lookup based on magic provided by the trigger? (string or address most likely).
>>> +	IIO_TRIGGER_TYPE_MAX,
>>> +};
>>> +
>>> +struct iio_configfs_trigger_ops;
>>> +
>>> +struct iio_configfs_trigger_type {
>>> +	int type;
>>> +	struct module *owner;
>>> +	struct iio_configfs_trigger_ops *trigger_ops;
>>> +};
>>> +
>>> +struct iio_configfs_trigger_info {
>>> +	char *name;
>>> +	struct iio_trigger *trigger;
>>> +	struct iio_configfs_trigger_type *trigger_type;
>>> +};
>>> +
>>> +struct iio_configfs_trigger_ops {
>> I wonder if we can make these more flexible from the start because
>> avoiding churn in callbacks is always nice as us ensuring we have
>> a manageable number in the long run!  Also we might want to play
>> the 'fixed point' games we play elsewhere in IIO to allow us to have
>> a very wide range of precisions.
>> enum iio_configfs_trigger_paramtype = {
>>      delay,
>> };
>>
>> As a final thought, we have standardised on sampling_frequency elsewhere in
>> IIO (largely based on that was what the first few device used) so perhaps
>> we can use that here as well instead of delay?
>>
>> int (*get_parameter)(struct iio_configfs_trigger_info *, enum iio_configfs_triger_paramtype param, unsigned long *);
>>
>>> +	int (*get_delay)(struct iio_configfs_trigger_info *, unsigned long *);
>>> +	int (*set_delay)(struct iio_configfs_trigger_info *, unsigned long);
>>> +	int (*probe)(struct iio_configfs_trigger_info *);
>>> +	int (*remove)(struct iio_configfs_trigger_info *);
>>> +};
>>> +
>>> +int register_configfs_trigger(struct iio_configfs_trigger_type *);
>>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *);
>>> +
>>> +#endif /* __IIO_CONFIGFS_TRIGGER */
>>>
>>
>> --
>> 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
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Daniel Baluta]

On Thu, Apr 9, 2015 at 9:12 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 09/04/15 18:40, Jonathan Cameron wrote:
>> On 09/04/15 18:15, Jonathan Cameron wrote:
>>> On 06/04/15 15:18, Daniel Baluta wrote:
>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>> configfs mount point root, with one default group for "triggers".
>>>>
>>>> It offers basic interface for registering software trigger types. Next patches
>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>
>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>> support for IIO works.
>>>>
>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>> Looks good and is actually a fair bit simpler than I expected which is nice!
>>> As an ideal aim, I'd like there to be no need for the core to have any
>>> idea what triggers exist and can be registered (beyond preventing naming
>>> clashes etc). Actually, not much would be needed to implement that I think, just
>>> using a list and looking up in it (we aren't on a particularly fast path so
>>> don't need anything clever) instead of a simple array.  A touch of locking
>>> probably to avoid two many simultaneous users of that list...
>>>
>>> Hmm. having read through the patches, are we fundamentally limited to having to
>>> specify the entire directory structure before bringing up configfs?
>> As I read more on this, I think the definition of a 'subsystem' in configfs is
>> very restricted. It means a tight group of devices sharing a prior
>> known interface with no ability to add extras later.

It looked fairly easy for me to add new attributes to an existing
interface. But this
is also my first experience with configfs.

>> It's sort of like the difference between a class and a bus in sysfs (but more limited
>> actually that's a rubbish analogy).  Thus either we have to do as here
>> and have the core know all about everything it might want to setup in advance
>> of registering the iio configfs subsystem or we need to make a whole load of
>> different configfs subsystems. Basically boils down to one per module.  We lose
>> the grouping convenience of the current structure but gain in separation.
>>
>> So option 1 we have effectively to predefine anything that might turn up
>> in a module...
>>
>> /config/iio/triggers/hrtimer/instance1/..
>> /config/iio/triggers/sysfs/instance2/..
>> /config/iio/virtualdevs/hwmon/iiohwmoninstance1/..
>> /config/iio/virtualdevs/input/iioinputinstance1/..
>> /config/iio/maps/channelmapisntance1/..
>> etc (actually fine for the maps as they are coming from the core anyway
>> and will be known in advance)

We predefine the files that would appear inside instace1/instance2. But we
do not predefine the instances. Also, we can easaily add new attributes for
a given type of trigger type ("hrtimer", "sysfs", etc).

These instances directories would be created with mkdir.

>>
>> Option 2 we have complete flexibility as its down to the relevant drivers
>>
>> /config/iio-trigger-hrtimer/instance/..
>> /config/iio-trigger-sysfs/instance/..
>> /config/iio-virtual-hwmon/instance/..
>> /config/iio-virtual-input/instance/..
>>
>> but we lose the grouping by directory which is a conceptual shame.
>>
>> Am I missing something important here?  (pretty new to configfs!)

Not sure exactly :), also my first experience with configs. But I woul prefer
option 1.

>>
>> Just for others not familiar with IIO who might read this. We have a number
>> of userspace created elements but each type is provided by a different module,
>> hence it's a real pain having to have them all defined at configfs subsystem
>> initialization as it adds tight coupling we don't otherwise have!
>
> For reference of others the usb gadget code is a good example of how to handle
> these sort of cross dependencies..
>
> the approach used for function drivers there would give us the cleaner option of
>
> /config/iio/triggers/hrtimer-instance1
> /config/iio/triggers/hrtimer-instance2
> /config/iio/triggers/sysfs-instance1
> /config/iio/virtualdevs/hwmon-instance1
> etc

I will look again at the gadget code, this was also my source of inspiration
since there are not so many users of configfs.

Daniel.

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 10/04/15 14:50, Daniel Baluta wrote:
> On Thu, Apr 9, 2015 at 9:12 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 09/04/15 18:40, Jonathan Cameron wrote:
>>> On 09/04/15 18:15, Jonathan Cameron wrote:
>>>> On 06/04/15 15:18, Daniel Baluta wrote:
>>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>>> configfs mount point root, with one default group for "triggers".
>>>>>
>>>>> It offers basic interface for registering software trigger types. Next patches
>>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>>
>>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>>> support for IIO works.
>>>>>
>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>> Looks good and is actually a fair bit simpler than I expected which is nice!
>>>> As an ideal aim, I'd like there to be no need for the core to have any
>>>> idea what triggers exist and can be registered (beyond preventing naming
>>>> clashes etc). Actually, not much would be needed to implement that I think, just
>>>> using a list and looking up in it (we aren't on a particularly fast path so
>>>> don't need anything clever) instead of a simple array.  A touch of locking
>>>> probably to avoid two many simultaneous users of that list...
>>>>
>>>> Hmm. having read through the patches, are we fundamentally limited to having to
>>>> specify the entire directory structure before bringing up configfs?
>>> As I read more on this, I think the definition of a 'subsystem' in configfs is
>>> very restricted. It means a tight group of devices sharing a prior
>>> known interface with no ability to add extras later.
> 
> It looked fairly easy for me to add new attributes to an existing
> interface. But this
> is also my first experience with configfs.
> 
>>> It's sort of like the difference between a class and a bus in sysfs (but more limited
>>> actually that's a rubbish analogy).  Thus either we have to do as here
>>> and have the core know all about everything it might want to setup in advance
>>> of registering the iio configfs subsystem or we need to make a whole load of
>>> different configfs subsystems. Basically boils down to one per module.  We lose
>>> the grouping convenience of the current structure but gain in separation.
>>>
>>> So option 1 we have effectively to predefine anything that might turn up
>>> in a module...
>>>
>>> /config/iio/triggers/hrtimer/instance1/..
>>> /config/iio/triggers/sysfs/instance2/..
>>> /config/iio/virtualdevs/hwmon/iiohwmoninstance1/..
>>> /config/iio/virtualdevs/input/iioinputinstance1/..
>>> /config/iio/maps/channelmapisntance1/..
>>> etc (actually fine for the maps as they are coming from the core anyway
>>> and will be known in advance)
> 
> We predefine the files that would appear inside instace1/instance2. But we
> do not predefine the instances. Also, we can easaily add new attributes for
> a given type of trigger type ("hrtimer", "sysfs", etc).
The issue is that what we can get away with not predefining because
we don't want the core to have any idea what a given trigger implementation
is going to want to have in the way of attrs.
> 
> These instances directories would be created with mkdir.
> 
>>>
>>> Option 2 we have complete flexibility as its down to the relevant drivers
>>>
>>> /config/iio-trigger-hrtimer/instance/..
>>> /config/iio-trigger-sysfs/instance/..
>>> /config/iio-virtual-hwmon/instance/..
>>> /config/iio-virtual-input/instance/..
>>>
>>> but we lose the grouping by directory which is a conceptual shame.
>>>
>>> Am I missing something important here?  (pretty new to configfs!)
> 
> Not sure exactly :), also my first experience with configs. But I woul prefer
> option 1.
> 
>>>
>>> Just for others not familiar with IIO who might read this. We have a number
>>> of userspace created elements but each type is provided by a different module,
>>> hence it's a real pain having to have them all defined at configfs subsystem
>>> initialization as it adds tight coupling we don't otherwise have!
>>
>> For reference of others the usb gadget code is a good example of how to handle
>> these sort of cross dependencies..
>>
>> the approach used for function drivers there would give us the cleaner option of
>>
>> /config/iio/triggers/hrtimer-instance1
>> /config/iio/triggers/hrtimer-instance2
>> /config/iio/triggers/sysfs-instance1
>> /config/iio/virtualdevs/hwmon-instance1
>> etc
> 
> I will look again at the gadget code, this was also my source of inspiration
> since there are not so many users of configfs.
Quite. Been around a while, but never picked up wide usage...

What I don't think it allows is sub directories of sub directories of a directory that has
not been defined at startup.  There is only one layer of dynamic creation possible.

So we can only do 
/config/iio/triggers/hrtimer/instance1 if we have hrtimer defined in the core.
That's pretty much a non starter and why (I think) the gadget driver uses the
<type>-instance name syntax to avoid the same problem. Otherwise, function/type/instance
would make more sense (to my mind) for them as well.
> 
> Daniel.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Daniel Baluta]

On Thu, Apr 9, 2015 at 8:15 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 06/04/15 15:18, Daniel Baluta wrote:
>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>> configfs mount point root, with one default group for "triggers".
>>
>> It offers basic interface for registering software trigger types. Next patches
>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>
>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>> support for IIO works.
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> Looks good and is actually a fair bit simpler than I expected which is nice!
> As an ideal aim, I'd like there to be no need for the core to have any
> idea what triggers exist and can be registered (beyond preventing naming
> clashes etc). Actually, not much would be needed to implement that I think, just
> using a list and looking up in it (we aren't on a particularly fast path so
> don't need anything clever) instead of a simple array.  A touch of locking
> probably to avoid two many simultaneous users of that list...

Good point. Will do.

I will create a list holding the list with the registered trigger types using
the trigger type name as the searching key (e.g. : "hrtimer", "sysfs").

>
> Hmm. having read through the patches, are we fundamentally limited to having to
> specify the entire directory structure before bringing up configfs?

AFAIK, directories can be created in two ways:

*  as default groups registered at iio trigger configfs init time.
*  when userspace applications call mkdir.

So, unless we agree that userspace applications should be aware of IIO
trigger types and manually call mkdir /config/iio/triggers/hrtimer (for example)
we are limited to specifying the entire directory structure in the IIO
configs module.

>
>> ---
>>
>> Changes since v2:
>>       * Fix build errors by compiling industrialio-configfs.ko
>>       as a separate module, as reported by Paul.
>>
>> Changes since v1:
>>       * addressed feedback from v1:
>>               * https://lkml.org/lkml/2015/3/25/647
>>       * create a directory per trigger type
>>       * removed activate and type attributes
>>
>>  drivers/iio/Kconfig                      |   8 ++
>>  drivers/iio/Makefile                     |   1 +
>>  drivers/iio/industrialio-configfs.c      | 127 +++++++++++++++++++++++++++++++
>>  include/linux/iio/iio_configfs_trigger.h |  40 ++++++++++
>>  4 files changed, 176 insertions(+)
>>  create mode 100644 drivers/iio/industrialio-configfs.c
>>  create mode 100644 include/linux/iio/iio_configfs_trigger.h
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 4011eff..41d5863 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 698afc2..72e85c42 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -8,6 +8,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>  industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>>
>> +obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
>>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
>>
>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>> new file mode 100644
>> index 0000000..d8ffd76
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-configfs.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * 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/iio_configfs_trigger.h>
>> +
>> +static struct iio_configfs_trigger_type *trigger_types[IIO_TRIGGER_TYPE_MAX];
>> +static DEFINE_RWLOCK(trigger_types_lock);
>> +
>> +int register_configfs_trigger(struct iio_configfs_trigger_type *t)
>> +{
>> +     int index = t->type;
>> +     int ret = 0;
>> +
>> +     if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>> +             return -EINVAL;
>> +
>> +     write_lock(&trigger_types_lock);
>> +     if (trigger_types[index])
>> +             ret = -EBUSY;
>> +     else
>> +             trigger_types[index] = t;
>> +     write_unlock(&trigger_types_lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(register_configfs_trigger);
>> +
>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *t)
>> +{
>> +     int index = t->type;
>> +
>> +     if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>> +             return -EINVAL;
>> +
>> +     write_lock(&trigger_types_lock);
>> +     trigger_types[index] = NULL;
>> +     write_unlock(&trigger_types_lock);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(unregister_configfs_trigger);
>> +
>> +static
>> +struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>> +{
>> +     struct iio_configfs_trigger_type *t;
>> +
>> +     if (type < 0 || type >= IIO_TRIGGER_TYPE_MAX)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     read_lock(&trigger_types_lock);
>> +     t = trigger_types[type];
>> +     if (t && !try_module_get(t->owner))
>> +             t = NULL;
>> +     read_unlock(&trigger_types_lock);
>> +
>> +     return t;
>> +}
>> +
>> +static struct config_group *iio_triggers_default_groups[] = {
>> +     NULL
>> +};
>> +
>> +static struct config_item_type iio_triggers_group_type = {
>> +     .ct_owner       = THIS_MODULE,
>> +};
>> +
>> +static struct config_group iio_triggers_group = {
>> +     .cg_item = {
>> +             .ci_namebuf = "triggers",
>> +             .ci_type = &iio_triggers_group_type,
>> +     },
>> +     .default_groups = iio_triggers_default_groups,
>> +};
>> +
>> +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");
>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>> new file mode 100644
>> index 0000000..75e76f2
>> --- /dev/null
>> +++ b/include/linux/iio/iio_configfs_trigger.h
>> @@ -0,0 +1,40 @@
>> +#ifndef __IIO_CONFIGFS_TRIGGER
>> +#define __IIO_CONFIGFS_TRIGGER
>> +
>> +#include <linux/device.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +
>> +#define module_iio_configfs_trigger_driver(__iio_configfs_trigger) \
>> +     module_driver(__iio_configfs_trigger, register_configfs_trigger, \
>> +                   unregister_configfs_trigger)
>> +
>> +enum {
> Is this enum actually needed as such? If we can avoid explicit need
> to list triggers in one place it would be nice.
>
> As I understand it, the point of this is to allow allocating of various
> arrays for easy lookup etc.  How about we switch over to a list and
> lookup based on magic provided by the trigger? (string or address most likely).

Not really needed, I was trying to avoid a list traversal. You have good point
here, I will use a list with the trigger type name (string) as a search key.

>> +     IIO_TRIGGER_TYPE_MAX,
>> +};
>> +
>> +struct iio_configfs_trigger_ops;
>> +
>> +struct iio_configfs_trigger_type {
>> +     int type;
>> +     struct module *owner;
>> +     struct iio_configfs_trigger_ops *trigger_ops;
>> +};
>> +
>> +struct iio_configfs_trigger_info {
>> +     char *name;
>> +     struct iio_trigger *trigger;
>> +     struct iio_configfs_trigger_type *trigger_type;
>> +};
>> +
>> +struct iio_configfs_trigger_ops {
> I wonder if we can make these more flexible from the start because
> avoiding churn in callbacks is always nice as us ensuring we have
> a manageable number in the long run!  Also we might want to play
> the 'fixed point' games we play elsewhere in IIO to allow us to have
> a very wide range of precisions.

Will try, this configfs_trigger_paramtype looks promising.

> enum iio_configfs_trigger_paramtype = {
>      delay,
> };
>
> As a final thought, we have standardised on sampling_frequency elsewhere in
> IIO (largely based on that was what the first few device used) so perhaps
> we can use that here as well instead of delay?

I am not sure I understand what is the "fixed point" game but I tried to
avoid conversions from sampling_frequency integer.fractional part
to timer period by using delay.

As already said sampling frequency comes natural for devices and delay
for timers.

>
> int (*get_parameter)(struct iio_configfs_trigger_info *, enum iio_configfs_triger_paramtype param, unsigned long *);
>
>> +     int (*get_delay)(struct iio_configfs_trigger_info *, unsigned long *);
>> +     int (*set_delay)(struct iio_configfs_trigger_info *, unsigned long);
>> +     int (*probe)(struct iio_configfs_trigger_info *);
>> +     int (*remove)(struct iio_configfs_trigger_info *);
>> +};
>> +
>> +int register_configfs_trigger(struct iio_configfs_trigger_type *);
>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *);
>> +
>> +#endif /* __IIO_CONFIGFS_TRIGGER */
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Jonathan Cameron]

On 10/04/15 12:08, Daniel Baluta wrote:
> On Thu, Apr 9, 2015 at 8:15 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 06/04/15 15:18, Daniel Baluta wrote:
>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>> configfs mount point root, with one default group for "triggers".
>>>
>>> It offers basic interface for registering software trigger types. Next patches
>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>
>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>> support for IIO works.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> Looks good and is actually a fair bit simpler than I expected which is nice!
>> As an ideal aim, I'd like there to be no need for the core to have any
>> idea what triggers exist and can be registered (beyond preventing naming
>> clashes etc). Actually, not much would be needed to implement that I think, just
>> using a list and looking up in it (we aren't on a particularly fast path so
>> don't need anything clever) instead of a simple array.  A touch of locking
>> probably to avoid two many simultaneous users of that list...
> 
> Good point. Will do.
> 
> I will create a list holding the list with the registered trigger types using
> the trigger type name as the searching key (e.g. : "hrtimer", "sysfs").
> 
>>
>> Hmm. having read through the patches, are we fundamentally limited to having to
>> specify the entire directory structure before bringing up configfs?
> 
> AFAIK, directories can be created in two ways:
> 
> *  as default groups registered at iio trigger configfs init time.
> *  when userspace applications call mkdir.
> 
> So, unless we agree that userspace applications should be aware of IIO
> trigger types and manually call mkdir /config/iio/triggers/hrtimer (for example)
> we are limited to specifying the entire directory structure in the IIO
> configs module.
That's my understanding as well and the reason the usb gadget stuff
uses <type>-instancename instead of type/instancename

At the moment I think we will need to go the same way.
Of course from the point of view of what userspace needs to know
it's actually the same as

mkdir hrtimer
cd hrtime
mkdir instancename
though so perhaps we should just do that.

> 
>>
>>> ---
>>>
>>> Changes since v2:
>>>       * Fix build errors by compiling industrialio-configfs.ko
>>>       as a separate module, as reported by Paul.
>>>
>>> Changes since v1:
>>>       * addressed feedback from v1:
>>>               * https://lkml.org/lkml/2015/3/25/647
>>>       * create a directory per trigger type
>>>       * removed activate and type attributes
>>>
>>>  drivers/iio/Kconfig                      |   8 ++
>>>  drivers/iio/Makefile                     |   1 +
>>>  drivers/iio/industrialio-configfs.c      | 127 +++++++++++++++++++++++++++++++
>>>  include/linux/iio/iio_configfs_trigger.h |  40 ++++++++++
>>>  4 files changed, 176 insertions(+)
>>>  create mode 100644 drivers/iio/industrialio-configfs.c
>>>  create mode 100644 include/linux/iio/iio_configfs_trigger.h
>>>
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 4011eff..41d5863 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 698afc2..72e85c42 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -8,6 +8,7 @@ industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>>  industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>>>
>>> +obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
>>>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>>>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
>>>
>>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>>> new file mode 100644
>>> index 0000000..d8ffd76
>>> --- /dev/null
>>> +++ b/drivers/iio/industrialio-configfs.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * 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/iio_configfs_trigger.h>
>>> +
>>> +static struct iio_configfs_trigger_type *trigger_types[IIO_TRIGGER_TYPE_MAX];
>>> +static DEFINE_RWLOCK(trigger_types_lock);
>>> +
>>> +int register_configfs_trigger(struct iio_configfs_trigger_type *t)
>>> +{
>>> +     int index = t->type;
>>> +     int ret = 0;
>>> +
>>> +     if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>>> +             return -EINVAL;
>>> +
>>> +     write_lock(&trigger_types_lock);
>>> +     if (trigger_types[index])
>>> +             ret = -EBUSY;
>>> +     else
>>> +             trigger_types[index] = t;
>>> +     write_unlock(&trigger_types_lock);
>>> +
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL(register_configfs_trigger);
>>> +
>>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *t)
>>> +{
>>> +     int index = t->type;
>>> +
>>> +     if (index < 0 || index >= IIO_TRIGGER_TYPE_MAX)
>>> +             return -EINVAL;
>>> +
>>> +     write_lock(&trigger_types_lock);
>>> +     trigger_types[index] = NULL;
>>> +     write_unlock(&trigger_types_lock);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL(unregister_configfs_trigger);
>>> +
>>> +static
>>> +struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>>> +{
>>> +     struct iio_configfs_trigger_type *t;
>>> +
>>> +     if (type < 0 || type >= IIO_TRIGGER_TYPE_MAX)
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     read_lock(&trigger_types_lock);
>>> +     t = trigger_types[type];
>>> +     if (t && !try_module_get(t->owner))
>>> +             t = NULL;
>>> +     read_unlock(&trigger_types_lock);
>>> +
>>> +     return t;
>>> +}
>>> +
>>> +static struct config_group *iio_triggers_default_groups[] = {
>>> +     NULL
>>> +};
>>> +
>>> +static struct config_item_type iio_triggers_group_type = {
>>> +     .ct_owner       = THIS_MODULE,
>>> +};
>>> +
>>> +static struct config_group iio_triggers_group = {
>>> +     .cg_item = {
>>> +             .ci_namebuf = "triggers",
>>> +             .ci_type = &iio_triggers_group_type,
>>> +     },
>>> +     .default_groups = iio_triggers_default_groups,
>>> +};
>>> +
>>> +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");
>>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>>> new file mode 100644
>>> index 0000000..75e76f2
>>> --- /dev/null
>>> +++ b/include/linux/iio/iio_configfs_trigger.h
>>> @@ -0,0 +1,40 @@
>>> +#ifndef __IIO_CONFIGFS_TRIGGER
>>> +#define __IIO_CONFIGFS_TRIGGER
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/module.h>
>>> +
>>> +#define module_iio_configfs_trigger_driver(__iio_configfs_trigger) \
>>> +     module_driver(__iio_configfs_trigger, register_configfs_trigger, \
>>> +                   unregister_configfs_trigger)
>>> +
>>> +enum {
>> Is this enum actually needed as such? If we can avoid explicit need
>> to list triggers in one place it would be nice.
>>
>> As I understand it, the point of this is to allow allocating of various
>> arrays for easy lookup etc.  How about we switch over to a list and
>> lookup based on magic provided by the trigger? (string or address most likely).
> 
> Not really needed, I was trying to avoid a list traversal. You have good point
> here, I will use a list with the trigger type name (string) as a search key.
> 
>>> +     IIO_TRIGGER_TYPE_MAX,
>>> +};
>>> +
>>> +struct iio_configfs_trigger_ops;
>>> +
>>> +struct iio_configfs_trigger_type {
>>> +     int type;
>>> +     struct module *owner;
>>> +     struct iio_configfs_trigger_ops *trigger_ops;
>>> +};
>>> +
>>> +struct iio_configfs_trigger_info {
>>> +     char *name;
>>> +     struct iio_trigger *trigger;
>>> +     struct iio_configfs_trigger_type *trigger_type;
>>> +};
>>> +
>>> +struct iio_configfs_trigger_ops {
>> I wonder if we can make these more flexible from the start because
>> avoiding churn in callbacks is always nice as us ensuring we have
>> a manageable number in the long run!  Also we might want to play
>> the 'fixed point' games we play elsewhere in IIO to allow us to have
>> a very wide range of precisions.
> 
> Will try, this configfs_trigger_paramtype looks promising.
> 
>> enum iio_configfs_trigger_paramtype = {
>>      delay,
>> };
>>
>> As a final thought, we have standardised on sampling_frequency elsewhere in
>> IIO (largely based on that was what the first few device used) so perhaps
>> we can use that here as well instead of delay?
> 
> I am not sure I understand what is the "fixed point" game but I tried to
> avoid conversions from sampling_frequency integer.fractional part
> to timer period by using delay.
The fixed point game was precisely because lots of the parameters we
are dealing with look like floating point to userspace (though they
almost never actually are) and we can't do that in the kernel.

Hence we needed a representation for writing floating point (ish)
values to sysfs attributes or reading from them without actually
having floating point numbers,

In numerous individual drivers that hit this in the past they rolled
their own fixed point implementation.  All we did was decide on some
base options that fit nicely in to a pair of integers and expanded that
set whenever something crazy came along that went off one end or the other.
> 
> As already said sampling frequency comes natural for devices and delay
> for timers.
Agreed, it's natural from a hardware point of view in this case but it does
result in a mixture in the ABI which I don't like.  If we can
reasonably easily go back to a frequency based control I'd prefer to do
so.

We insist on sampling frequency on all the other triggers based on
timers (the device specific ones), though some of these are period
based as well.  Hence we really need to match this here.
> 
>>
>> int (*get_parameter)(struct iio_configfs_trigger_info *, enum iio_configfs_triger_paramtype param, unsigned long *);
>>
>>> +     int (*get_delay)(struct iio_configfs_trigger_info *, unsigned long *);
>>> +     int (*set_delay)(struct iio_configfs_trigger_info *, unsigned long);
>>> +     int (*probe)(struct iio_configfs_trigger_info *);
>>> +     int (*remove)(struct iio_configfs_trigger_info *);
>>> +};
>>> +
>>> +int register_configfs_trigger(struct iio_configfs_trigger_type *);
>>> +int unregister_configfs_trigger(struct iio_configfs_trigger_type *);
>>> +
>>> +#endif /* __IIO_CONFIGFS_TRIGGER */
>>>
>>
>> --
>> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/3] iio: configfs: Add configfs support to IIO

[Daniel Baluta]

On Sun, Apr 12, 2015 at 6:55 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 10/04/15 12:08, Daniel Baluta wrote:
>> On Thu, Apr 9, 2015 at 8:15 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 06/04/15 15:18, Daniel Baluta wrote:
>>>> This module is the core of IIO configfs. It creates the "iio" subsystem under
>>>> configfs mount point root, with one default group for "triggers".
>>>>
>>>> It offers basic interface for registering software trigger types. Next patches
>>>> will add "hrtimer" and "sysfs" trigger types. To add a new trigger type we must
>>>> create a new kernel module which implements iio_configfs_trigger.h interface.
>>>>
>>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>>> support for IIO works.
>>>>
>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>> Looks good and is actually a fair bit simpler than I expected which is nice!
>>> As an ideal aim, I'd like there to be no need for the core to have any
>>> idea what triggers exist and can be registered (beyond preventing naming
>>> clashes etc). Actually, not much would be needed to implement that I think, just
>>> using a list and looking up in it (we aren't on a particularly fast path so
>>> don't need anything clever) instead of a simple array.  A touch of locking
>>> probably to avoid two many simultaneous users of that list...
>>
>> Good point. Will do.
>>
>> I will create a list holding the list with the registered trigger types using
>> the trigger type name as the searching key (e.g. : "hrtimer", "sysfs").
>>
>>>
>>> Hmm. having read through the patches, are we fundamentally limited to having to
>>> specify the entire directory structure before bringing up configfs?
>>
>> AFAIK, directories can be created in two ways:
>>
>> *  as default groups registered at iio trigger configfs init time.
>> *  when userspace applications call mkdir.
>>
>> So, unless we agree that userspace applications should be aware of IIO
>> trigger types and manually call mkdir /config/iio/triggers/hrtimer (for example)
>> we are limited to specifying the entire directory structure in the IIO
>> configs module.
> That's my understanding as well and the reason the usb gadget stuff
> uses <type>-instancename instead of type/instancename
>
> At the moment I think we will need to go the same way.
> Of course from the point of view of what userspace needs to know
> it's actually the same as
>
> mkdir hrtimer
> cd hrtime
> mkdir instancename
> though so perhaps we should just do that.

Ok, I see your point here. I will try to send a new version with this in mind.

[PATCH v3 2/3] iio: trigger: Add support for highres timer trigger type

[Daniel Baluta]

This patch adds a new trigger type - the high resoluton timer ("hrtimer")
under /config/iio/triggers/ and also creates a module that implements hrtimer
trigger type functionality.

A new IIO trigger is allocated when a new directory is created under
/config/iio/triggers/. The name of the trigger is the same as the dir
name. Each trigger will have a "delay" attribute representing the delay
between two successive IIO trigger_poll calls.

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>
---
Changes since v2: 
	* none
Changes since v1:
	* replaced sampling_frequency with delay to make the in kernel
	processing easier
	* implemented hrtimer driver using the new API proposed in patch 1

 drivers/iio/industrialio-configfs.c      | 164 +++++++++++++++++++++++++++++++
 drivers/iio/trigger/Kconfig              |   9 ++
 drivers/iio/trigger/Makefile             |   2 +
 drivers/iio/trigger/iio-trig-hrtimer.c   | 154 +++++++++++++++++++++++++++++
 include/linux/iio/iio_configfs_trigger.h |   1 +
 5 files changed, 330 insertions(+)
 create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c

diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
index d8ffd76..a0e5d24 100644
--- a/drivers/iio/industrialio-configfs.c
+++ b/drivers/iio/industrialio-configfs.c
@@ -71,7 +71,170 @@ struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
 	return t;
 }
 
+struct iio_trigger_item {
+	struct config_item item;
+	struct iio_configfs_trigger_info *trigger_info;
+};
+
+static
+inline struct iio_trigger_item *to_iio_trigger_item(struct config_item *item)
+{
+	if (!item)
+		return NULL;
+	return container_of(item, struct iio_trigger_item, item);
+}
+
+CONFIGFS_ATTR_STRUCT(iio_trigger_item);
+
+#define IIO_TRIGGER_ITEM_ATTR(_name, _mode, _show, _store) \
+struct iio_trigger_item_attribute iio_trigger_item_attr_##_name = \
+	__CONFIGFS_ATTR(_name, _mode, _show, _store)
+
+static
+ssize_t iio_trigger_item_get_delay(struct iio_trigger_item *item, char *page)
+{
+	unsigned long delay;
+	int ret;
+
+	struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
+
+	ret = t->trigger_ops->get_delay(item->trigger_info, &delay);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(page, PAGE_SIZE, "%lu\n", delay);
+}
+
+static
+ssize_t iio_trigger_item_set_delay(struct iio_trigger_item *item,
+				   const char *page,
+				   size_t count)
+{
+	int ret;
+	unsigned long delay;
+	struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
+
+	ret = kstrtoul(page, 10, &delay);
+	if (ret < 0)
+		return ret;
+
+	ret = t->trigger_ops->set_delay(item->trigger_info, delay);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+IIO_TRIGGER_ITEM_ATTR(delay, S_IRUGO | S_IWUSR,
+		      iio_trigger_item_get_delay,
+		      iio_trigger_item_set_delay);
+
+static struct configfs_attribute *iio_trigger_item_attrs[] = {
+	&iio_trigger_item_attr_delay.attr,
+	NULL
+};
+
+CONFIGFS_ATTR_OPS(iio_trigger_item);
+
+static struct configfs_item_operations iio_trigger_item_ops = {
+	.show_attribute = iio_trigger_item_attr_show,
+	.store_attribute = iio_trigger_item_attr_store,
+};
+
+static struct config_item_type iio_trigger_item_type;
+
+static struct config_item *iio_hrtimer_make_item(struct config_group *group,
+						 const char *name)
+{
+	struct iio_trigger_item *trigger_item;
+	struct iio_configfs_trigger_info *trigger_info;
+	struct iio_configfs_trigger_type *trigger_type;
+	int ret;
+
+	trigger_item = kzalloc(sizeof(*trigger_item), GFP_KERNEL);
+	if (!trigger_item)
+		return ERR_PTR(-ENOMEM);
+	trigger_info = kzalloc(sizeof(*trigger_info), GFP_KERNEL);
+	if (!trigger_info) {
+		ret = -ENOMEM;
+		goto out_free_trigger_item;
+	}
+
+	trigger_info->name = kstrdup(name, GFP_KERNEL);
+	if (!trigger_info->name) {
+		ret = -ENOMEM;
+		goto out_free_trigger_info;
+	}
+
+	trigger_type = iio_get_configfs_trigger_type(IIO_TRIGGER_TYPE_HRTIMER);
+	if (!trigger_type) {
+		pr_err("Unable to get hrtimer trigger!\n");
+		ret = -ENODEV;
+		goto out_free_trigger_name;
+	}
+	ret = trigger_type->trigger_ops->probe(trigger_info);
+	if (ret < 0)
+		goto out_error_module_put;
+
+	trigger_info->trigger_type = trigger_type;
+	trigger_item->trigger_info = trigger_info;
+
+	config_item_init_type_name(&trigger_item->item, name,
+				   &iio_trigger_item_type);
+
+	return &trigger_item->item;
+out_error_module_put:
+	module_put(trigger_type->owner);
+out_free_trigger_name:
+	kfree(trigger_info->name);
+out_free_trigger_info:
+	kfree(trigger_info);
+out_free_trigger_item:
+	kfree(trigger_item);
+	return ERR_PTR(ret);
+}
+
+static void iio_hrtimer_drop_item(struct config_group *group,
+				  struct config_item *item)
+{
+	struct iio_trigger_item *trigger_item;
+	struct iio_configfs_trigger_type *trigger_type;
+
+	trigger_item = container_of(item, struct iio_trigger_item, item);
+	trigger_type = trigger_item->trigger_info->trigger_type;
+
+	trigger_type->trigger_ops->remove(trigger_item->trigger_info);
+	module_put(trigger_type->owner);
+	kfree(trigger_item->trigger_info->name);
+	kfree(trigger_item->trigger_info);
+	kfree(trigger_item);
+}
+
+static struct configfs_group_operations iio_triggers_hrtimer_group_ops = {
+	.make_item = iio_hrtimer_make_item,
+	.drop_item = iio_hrtimer_drop_item,
+};
+
+static struct config_item_type iio_trigger_item_type = {
+	.ct_owner	= THIS_MODULE,
+	.ct_item_ops	= &iio_trigger_item_ops,
+	.ct_attrs	= iio_trigger_item_attrs,
+};
+
+static struct config_item_type iio_triggers_hrtimer_type = {
+	.ct_owner	= THIS_MODULE,
+	.ct_group_ops	= &iio_triggers_hrtimer_group_ops,
+};
+
+static struct config_group iio_triggers_hrtimer_group = {
+	.cg_item = {
+		.ci_namebuf = "hrtimer",
+		.ci_type = &iio_triggers_hrtimer_type,
+	},
+};
+
 static struct config_group *iio_triggers_default_groups[] = {
+	&iio_triggers_hrtimer_group,
 	NULL
 };
 
@@ -109,6 +272,7 @@ static struct configfs_subsystem iio_configfs_subsys = {
 
 static int __init iio_configfs_init(void)
 {
+	config_group_init(&iio_triggers_hrtimer_group);
 	config_group_init(&iio_triggers_group);
 	config_group_init(&iio_configfs_subsys.su_group);
 
diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
index 7999612..e21688e 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"
+	depends on IIO_CONFIGFS
+	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..0acde8d
--- /dev/null
+++ b/drivers/iio/trigger/iio-trig-hrtimer.c
@@ -0,0 +1,154 @@
+/**
+ * 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>
+ *
+ * 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/iio_configfs_trigger.h>
+
+/* default delay in ns (100Hz) */
+#define HRTIMER_TRIGGER_DEFAULT_DELAY 100000000
+
+struct iio_hrtimer_trig_info {
+	struct iio_configfs_trigger_info *trigger_info;
+	struct hrtimer timer;
+	unsigned long delay;
+};
+
+static enum hrtimer_restart iio_trig_hrtimer_trig(struct hrtimer *timer)
+{
+	struct iio_hrtimer_trig_info *trig_info;
+
+	trig_info = container_of(timer, struct iio_hrtimer_trig_info, timer);
+
+	hrtimer_forward_now(timer, ns_to_ktime(trig_info->delay));
+	iio_trigger_poll(trig_info->trigger_info->trigger);
+
+	return HRTIMER_RESTART;
+}
+
+static int iio_trig_hrtimer_set_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_hrtimer_trig_info *trig_info;
+
+	trig_info = iio_trigger_get_drvdata(trig);
+
+	if (state)
+		hrtimer_start(&trig_info->timer, ns_to_ktime(trig_info->delay),
+			      HRTIMER_MODE_REL);
+	else
+		hrtimer_cancel(&trig_info->timer);
+
+	return 0;
+}
+
+int iio_trig_hrtimer_get_delay(struct iio_configfs_trigger_info *t,
+			       unsigned long *delay)
+{
+	struct iio_hrtimer_trig_info *trig_info;
+
+	trig_info = iio_trigger_get_drvdata(t->trigger);
+	*delay = trig_info->delay;
+
+	return 0;
+}
+
+int iio_trig_hrtimer_set_delay(struct iio_configfs_trigger_info *t,
+			       unsigned long delay)
+{
+	struct iio_hrtimer_trig_info *trig_info;
+
+	trig_info = iio_trigger_get_drvdata(t->trigger);
+
+	if (!delay)
+		return -EINVAL;
+
+	trig_info->delay = delay;
+
+	return 0;
+}
+
+static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = iio_trig_hrtimer_set_state,
+};
+
+static int iio_trig_hrtimer_probe(struct iio_configfs_trigger_info *t)
+{
+	struct iio_hrtimer_trig_info *trig_info;
+	int ret;
+
+	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
+	if (!trig_info)
+		return -ENOMEM;
+
+	trig_info->trigger_info = t;
+
+	t->trigger = iio_trigger_alloc("%s", t->name);
+	if (!t->trigger)
+		return -ENOMEM;
+
+	iio_trigger_set_drvdata(t->trigger, trig_info);
+	t->trigger->ops = &iio_hrtimer_trigger_ops;
+
+	hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	trig_info->timer.function = iio_trig_hrtimer_trig;
+
+	trig_info->delay = HRTIMER_TRIGGER_DEFAULT_DELAY;
+
+	ret = iio_trigger_register(t->trigger);
+	if (ret)
+		goto err_free_trigger;
+
+	return 0;
+err_free_trigger:
+	iio_trigger_free(t->trigger);
+
+	return ret;
+}
+
+static int iio_trig_hrtimer_remove(struct iio_configfs_trigger_info *t)
+{
+	struct iio_hrtimer_trig_info *trig_info;
+
+	trig_info = iio_trigger_get_drvdata(t->trigger);
+
+	iio_trigger_unregister(t->trigger);
+	hrtimer_cancel(&trig_info->timer);
+	iio_trigger_free(t->trigger);
+
+	return 0;
+}
+
+struct iio_configfs_trigger_ops hrtimer_trigger_ops = {
+	.get_delay	= iio_trig_hrtimer_get_delay,
+	.set_delay	= iio_trig_hrtimer_set_delay,
+	.probe		= iio_trig_hrtimer_probe,
+	.remove		= iio_trig_hrtimer_remove,
+};
+
+struct iio_configfs_trigger_type iio_configfs_hrtimer = {
+	.type = IIO_TRIGGER_TYPE_HRTIMER,
+	.owner = THIS_MODULE,
+	.trigger_ops = &hrtimer_trigger_ops,
+};
+
+module_iio_configfs_trigger_driver(iio_configfs_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");
diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
index 75e76f2..6233733 100644
--- a/include/linux/iio/iio_configfs_trigger.h
+++ b/include/linux/iio/iio_configfs_trigger.h
@@ -10,6 +10,7 @@
 		      unregister_configfs_trigger)
 
 enum {
+	IIO_TRIGGER_TYPE_HRTIMER = 0,
 	IIO_TRIGGER_TYPE_MAX,
 };
 
-- 
1.9.1

Re: [PATCH v3 2/3] iio: trigger: Add support for highres timer trigger type

[Jonathan Cameron]

On 06/04/15 15:18, Daniel Baluta wrote:
> This patch adds a new trigger type - the high resoluton timer ("hrtimer")
> under /config/iio/triggers/ and also creates a module that implements hrtimer
> trigger type functionality.
> 
> A new IIO trigger is allocated when a new directory is created under
> /config/iio/triggers/. The name of the trigger is the same as the dir
> name. Each trigger will have a "delay" attribute representing the delay
> between two successive IIO trigger_poll calls.
Cool ;)
> 
> 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>
I'd ideally have liked the break up of the patches to be slightly different.
There is stuff in here that would have made more sense in the first patch
as it isn't specific to this particular trigger.
> ---
> Changes since v2: 
> 	* none
> Changes since v1:
> 	* replaced sampling_frequency with delay to make the in kernel
> 	processing easier
Then along comes me an suggests going back again ;)  Make your case...
> 	* implemented hrtimer driver using the new API proposed in patch 1
> 
>  drivers/iio/industrialio-configfs.c      | 164 +++++++++++++++++++++++++++++++
>  drivers/iio/trigger/Kconfig              |   9 ++
>  drivers/iio/trigger/Makefile             |   2 +
>  drivers/iio/trigger/iio-trig-hrtimer.c   | 154 +++++++++++++++++++++++++++++
>  include/linux/iio/iio_configfs_trigger.h |   1 +
>  5 files changed, 330 insertions(+)
>  create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c
> 
> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
> index d8ffd76..a0e5d24 100644
> --- a/drivers/iio/industrialio-configfs.c
> +++ b/drivers/iio/industrialio-configfs.c
> @@ -71,7 +71,170 @@ struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>  	return t;
>  }
>  
> +struct iio_trigger_item {
> +	struct config_item item;
> +	struct iio_configfs_trigger_info *trigger_info;
> +};
> +
> +static
> +inline struct iio_trigger_item *to_iio_trigger_item(struct config_item *item)
> +{
> +	if (!item)
> +		return NULL;
> +	return container_of(item, struct iio_trigger_item, item);
> +}
> +
> +CONFIGFS_ATTR_STRUCT(iio_trigger_item);
> +
> +#define IIO_TRIGGER_ITEM_ATTR(_name, _mode, _show, _store) \
> +struct iio_trigger_item_attribute iio_trigger_item_attr_##_name = \
> +	__CONFIGFS_ATTR(_name, _mode, _show, _store)
> +
> +static
> +ssize_t iio_trigger_item_get_delay(struct iio_trigger_item *item, char *page)
> +{
> +	unsigned long delay;
> +	int ret;
> +
> +	struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
> +
> +	ret = t->trigger_ops->get_delay(item->trigger_info, &delay);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(page, PAGE_SIZE, "%lu\n", delay);
> +}
> +
> +static
> +ssize_t iio_trigger_item_set_delay(struct iio_trigger_item *item,
> +				   const char *page,
> +				   size_t count)
> +{
> +	int ret;
> +	unsigned long delay;
> +	struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
> +
> +	ret = kstrtoul(page, 10, &delay);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = t->trigger_ops->set_delay(item->trigger_info, delay);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +IIO_TRIGGER_ITEM_ATTR(delay, S_IRUGO | S_IWUSR,
> +		      iio_trigger_item_get_delay,
> +		      iio_trigger_item_set_delay);
> +
> +static struct configfs_attribute *iio_trigger_item_attrs[] = {
> +	&iio_trigger_item_attr_delay.attr,
> +	NULL
> +};
> +
> +CONFIGFS_ATTR_OPS(iio_trigger_item);
> +
> +static struct configfs_item_operations iio_trigger_item_ops = {
> +	.show_attribute = iio_trigger_item_attr_show,
> +	.store_attribute = iio_trigger_item_attr_store,
> +};
So everything down to here would have fitted a little better in the previous
patch to my mind.

> +
> +static struct config_item_type iio_trigger_item_type;
> +

Why is this hrtimer specific?  Only one real place that I can see...
I think we need to get that element out into the hrtimer module rather than
here.  A utility function doing most of this makes sense in the core
but not the hrtimer bit.

> +static struct config_item *iio_hrtimer_make_item(struct config_group *group,
> +						 const char *name)
> +{
> +	struct iio_trigger_item *trigger_item;
> +	struct iio_configfs_trigger_info *trigger_info;
> +	struct iio_configfs_trigger_type *trigger_type;
> +	int ret;
> +
> +	trigger_item = kzalloc(sizeof(*trigger_item), GFP_KERNEL);
> +	if (!trigger_item)
> +		return ERR_PTR(-ENOMEM);
> +	trigger_info = kzalloc(sizeof(*trigger_info), GFP_KERNEL);
> +	if (!trigger_info) {
> +		ret = -ENOMEM;
> +		goto out_free_trigger_item;
> +	}
> +
> +	trigger_info->name = kstrdup(name, GFP_KERNEL);
> +	if (!trigger_info->name) {
> +		ret = -ENOMEM;
> +		goto out_free_trigger_info;
> +	}
> +
> +	trigger_type = iio_get_configfs_trigger_type(IIO_TRIGGER_TYPE_HRTIMER);
> +	if (!trigger_type) {
> +		pr_err("Unable to get hrtimer trigger!\n");
> +		ret = -ENODEV;
> +		goto out_free_trigger_name;
> +	}
> +	ret = trigger_type->trigger_ops->probe(trigger_info);
> +	if (ret < 0)
> +		goto out_error_module_put;
> +
> +	trigger_info->trigger_type = trigger_type;
> +	trigger_item->trigger_info = trigger_info;
> +
> +	config_item_init_type_name(&trigger_item->item, name,
> +				   &iio_trigger_item_type);
> +
> +	return &trigger_item->item;
> +out_error_module_put:
> +	module_put(trigger_type->owner);
> +out_free_trigger_name:
> +	kfree(trigger_info->name);
> +out_free_trigger_info:
> +	kfree(trigger_info);
> +out_free_trigger_item:
> +	kfree(trigger_item);
> +	return ERR_PTR(ret);
> +}
> +
This one is no way at all hrtimer specific that I can see!
> +static void iio_hrtimer_drop_item(struct config_group *group,
> +				  struct config_item *item)
> +{
> +	struct iio_trigger_item *trigger_item;
> +	struct iio_configfs_trigger_type *trigger_type;
> +
> +	trigger_item = container_of(item, struct iio_trigger_item, item);
> +	trigger_type = trigger_item->trigger_info->trigger_type;
> +
> +	trigger_type->trigger_ops->remove(trigger_item->trigger_info);
> +	module_put(trigger_type->owner);
> +	kfree(trigger_item->trigger_info->name);
> +	kfree(trigger_item->trigger_info);
> +	kfree(trigger_item);
> +}
> +
> +static struct configfs_group_operations iio_triggers_hrtimer_group_ops = {
So to make the separation clean we need to create this object dynamically or pass
it through from the hrtimer trigger module.
> +	.make_item = iio_hrtimer_make_item,
> +	.drop_item = iio_hrtimer_drop_item,
> +};
> +
> +static struct config_item_type iio_trigger_item_type = {
> +	.ct_owner	= THIS_MODULE,
> +	.ct_item_ops	= &iio_trigger_item_ops,
> +	.ct_attrs	= iio_trigger_item_attrs,
> +};
> +
> +static struct config_item_type iio_triggers_hrtimer_type = {
> +	.ct_owner	= THIS_MODULE,
> +	.ct_group_ops	= &iio_triggers_hrtimer_group_ops,
> +};
So this one also needs to come from the hrtimer module.
> +
> +static struct config_group iio_triggers_hrtimer_group = {
> +	.cg_item = {
> +		.ci_namebuf = "hrtimer",
> +		.ci_type = &iio_triggers_hrtimer_type,
> +	},
> +};
> +
>  static struct config_group *iio_triggers_default_groups[] = {
> +	&iio_triggers_hrtimer_group,
>  	NULL
>  };
>  
> @@ -109,6 +272,7 @@ static struct configfs_subsystem iio_configfs_subsys = {
>  
>  static int __init iio_configfs_init(void)
>  {
> +	config_group_init(&iio_triggers_hrtimer_group);
ahh... We have to do the init in this order? No dynamic creation later?
>  	config_group_init(&iio_triggers_group);
>  	config_group_init(&iio_configfs_subsys.su_group);
>  
> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
> index 7999612..e21688e 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"
> +	depends on IIO_CONFIGFS
> +	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..0acde8d
> --- /dev/null
> +++ b/drivers/iio/trigger/iio-trig-hrtimer.c
> @@ -0,0 +1,154 @@
> +/**
> + * 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>
> + *
> + * 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/iio_configfs_trigger.h>
> +
> +/* default delay in ns (100Hz) */
> +#define HRTIMER_TRIGGER_DEFAULT_DELAY 100000000
> +
> +struct iio_hrtimer_trig_info {
> +	struct iio_configfs_trigger_info *trigger_info;
> +	struct hrtimer timer;
> +	unsigned long delay;
> +};
> +
> +static enum hrtimer_restart iio_trig_hrtimer_trig(struct hrtimer *timer)
> +{
> +	struct iio_hrtimer_trig_info *trig_info;
> +
> +	trig_info = container_of(timer, struct iio_hrtimer_trig_info, timer);
> +
> +	hrtimer_forward_now(timer, ns_to_ktime(trig_info->delay));
> +	iio_trigger_poll(trig_info->trigger_info->trigger);
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static int iio_trig_hrtimer_set_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_hrtimer_trig_info *trig_info;
> +
> +	trig_info = iio_trigger_get_drvdata(trig);
> +
> +	if (state)
> +		hrtimer_start(&trig_info->timer, ns_to_ktime(trig_info->delay),
> +			      HRTIMER_MODE_REL);
> +	else
> +		hrtimer_cancel(&trig_info->timer);
> +
> +	return 0;
> +}
> +
> +int iio_trig_hrtimer_get_delay(struct iio_configfs_trigger_info *t,
> +			       unsigned long *delay)
> +{
> +	struct iio_hrtimer_trig_info *trig_info;
> +
> +	trig_info = iio_trigger_get_drvdata(t->trigger);
> +	*delay = trig_info->delay;
> +
> +	return 0;
> +}
> +
> +int iio_trig_hrtimer_set_delay(struct iio_configfs_trigger_info *t,
> +			       unsigned long delay)
> +{
> +	struct iio_hrtimer_trig_info *trig_info;
> +
> +	trig_info = iio_trigger_get_drvdata(t->trigger);
> +
> +	if (!delay)
> +		return -EINVAL;
> +
> +	trig_info->delay = delay;
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
> +	.owner = THIS_MODULE,
> +	.set_trigger_state = iio_trig_hrtimer_set_state,
> +};
> +
> +static int iio_trig_hrtimer_probe(struct iio_configfs_trigger_info *t)
> +{
> +	struct iio_hrtimer_trig_info *trig_info;
> +	int ret;
> +
> +	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
> +	if (!trig_info)
> +		return -ENOMEM;
> +
> +	trig_info->trigger_info = t;
> +
> +	t->trigger = iio_trigger_alloc("%s", t->name);
> +	if (!t->trigger)
> +		return -ENOMEM;
> +
> +	iio_trigger_set_drvdata(t->trigger, trig_info);
> +	t->trigger->ops = &iio_hrtimer_trigger_ops;
> +
> +	hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	trig_info->timer.function = iio_trig_hrtimer_trig;
> +
> +	trig_info->delay = HRTIMER_TRIGGER_DEFAULT_DELAY;
> +
> +	ret = iio_trigger_register(t->trigger);
> +	if (ret)
> +		goto err_free_trigger;
> +
> +	return 0;
> +err_free_trigger:
> +	iio_trigger_free(t->trigger);
> +
> +	return ret;
> +}
> +
> +static int iio_trig_hrtimer_remove(struct iio_configfs_trigger_info *t)
> +{
> +	struct iio_hrtimer_trig_info *trig_info;
> +
> +	trig_info = iio_trigger_get_drvdata(t->trigger);
> +
> +	iio_trigger_unregister(t->trigger);
> +	hrtimer_cancel(&trig_info->timer);
> +	iio_trigger_free(t->trigger);
> +
> +	return 0;
> +}
> +
> +struct iio_configfs_trigger_ops hrtimer_trigger_ops = {
> +	.get_delay	= iio_trig_hrtimer_get_delay,
> +	.set_delay	= iio_trig_hrtimer_set_delay,
> +	.probe		= iio_trig_hrtimer_probe,
> +	.remove		= iio_trig_hrtimer_remove,
> +};
> +
> +struct iio_configfs_trigger_type iio_configfs_hrtimer = {
> +	.type = IIO_TRIGGER_TYPE_HRTIMER,
> +	.owner = THIS_MODULE,
> +	.trigger_ops = &hrtimer_trigger_ops,
> +};
> +
> +module_iio_configfs_trigger_driver(iio_configfs_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");
> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
> index 75e76f2..6233733 100644
> --- a/include/linux/iio/iio_configfs_trigger.h
> +++ b/include/linux/iio/iio_configfs_trigger.h
> @@ -10,6 +10,7 @@
>  		      unregister_configfs_trigger)
>  
>  enum {
> +	IIO_TRIGGER_TYPE_HRTIMER = 0,
>  	IIO_TRIGGER_TYPE_MAX,
>  };
>  
> 

Re: [PATCH v3 2/3] iio: trigger: Add support for highres timer trigger type

[Daniel Baluta]

On Thu, Apr 9, 2015 at 8:09 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 06/04/15 15:18, Daniel Baluta wrote:
>> This patch adds a new trigger type - the high resoluton timer ("hrtimer")
>> under /config/iio/triggers/ and also creates a module that implements hrtimer
>> trigger type functionality.
>>
>> A new IIO trigger is allocated when a new directory is created under
>> /config/iio/triggers/. The name of the trigger is the same as the dir
>> name. Each trigger will have a "delay" attribute representing the delay
>> between two successive IIO trigger_poll calls.
> Cool ;)
>>
>> 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>
> I'd ideally have liked the break up of the patches to be slightly different.
> There is stuff in here that would have made more sense in the first patch
> as it isn't specific to this particular trigger.

Yes :), this was my first thought too (implemented in v1), but then I wanted
to make a complete example on how to add a new trigger type.

People implementing a new trigger type will find this patch very useful.

Anyhow, I have no objection on this. Can be fixed.
>> ---
>> Changes since v2:
>>       * none
>> Changes since v1:
>>       * replaced sampling_frequency with delay to make the in kernel
>>       processing easier
> Then along comes me an suggests going back again ;)  Make your case...

Initial hrtimer trigger implementation was limited to using integer sampling
frequencies. Adding "rational" frequencies (e.g 12.5 Hz) and converting them
back to hrtimer trigger delay  would add some complex calculation in the kernel
with the possibility of losing precision.

Besides that, for most devices sampling_frequencies (Hz) attribute is natural
because this is what the devices expose in its registers, while the timers
API uses delays (nsec).

Here is how I see the usage of device's sampling_frequency and trigger's
delay (this should be kept in sync)

User application reads device's sampling_frequency and then based on that
it sets trigger's delay to 1/sampling_frequency -- all of that happening in user
space where floating point is easy :).

>>       * implemented hrtimer driver using the new API proposed in patch 1
>>
>>  drivers/iio/industrialio-configfs.c      | 164 +++++++++++++++++++++++++++++++
>>  drivers/iio/trigger/Kconfig              |   9 ++
>>  drivers/iio/trigger/Makefile             |   2 +
>>  drivers/iio/trigger/iio-trig-hrtimer.c   | 154 +++++++++++++++++++++++++++++
>>  include/linux/iio/iio_configfs_trigger.h |   1 +
>>  5 files changed, 330 insertions(+)
>>  create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c
>>
>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>> index d8ffd76..a0e5d24 100644
>> --- a/drivers/iio/industrialio-configfs.c
>> +++ b/drivers/iio/industrialio-configfs.c
>> @@ -71,7 +71,170 @@ struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>>       return t;
>>  }
>>
>> +struct iio_trigger_item {
>> +     struct config_item item;
>> +     struct iio_configfs_trigger_info *trigger_info;
>> +};
>> +
>> +static
>> +inline struct iio_trigger_item *to_iio_trigger_item(struct config_item *item)
>> +{
>> +     if (!item)
>> +             return NULL;
>> +     return container_of(item, struct iio_trigger_item, item);
>> +}
>> +
>> +CONFIGFS_ATTR_STRUCT(iio_trigger_item);
>> +
>> +#define IIO_TRIGGER_ITEM_ATTR(_name, _mode, _show, _store) \
>> +struct iio_trigger_item_attribute iio_trigger_item_attr_##_name = \
>> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
>> +
>> +static
>> +ssize_t iio_trigger_item_get_delay(struct iio_trigger_item *item, char *page)
>> +{
>> +     unsigned long delay;
>> +     int ret;
>> +
>> +     struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
>> +
>> +     ret = t->trigger_ops->get_delay(item->trigger_info, &delay);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return snprintf(page, PAGE_SIZE, "%lu\n", delay);
>> +}
>> +
>> +static
>> +ssize_t iio_trigger_item_set_delay(struct iio_trigger_item *item,
>> +                                const char *page,
>> +                                size_t count)
>> +{
>> +     int ret;
>> +     unsigned long delay;
>> +     struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
>> +
>> +     ret = kstrtoul(page, 10, &delay);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = t->trigger_ops->set_delay(item->trigger_info, delay);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +
>> +IIO_TRIGGER_ITEM_ATTR(delay, S_IRUGO | S_IWUSR,
>> +                   iio_trigger_item_get_delay,
>> +                   iio_trigger_item_set_delay);
>> +
>> +static struct configfs_attribute *iio_trigger_item_attrs[] = {
>> +     &iio_trigger_item_attr_delay.attr,
>> +     NULL
>> +};
>> +
>> +CONFIGFS_ATTR_OPS(iio_trigger_item);
>> +
>> +static struct configfs_item_operations iio_trigger_item_ops = {
>> +     .show_attribute = iio_trigger_item_attr_show,
>> +     .store_attribute = iio_trigger_item_attr_store,
>> +};
> So everything down to here would have fitted a little better in the previous
> patch to my mind.

ok. As said above, one way of doing this would be to start with
an empty configfs support than build new trigger types on top
of it.

>
>> +
>> +static struct config_item_type iio_trigger_item_type;
>> +
>
> Why is this hrtimer specific?  Only one real place that I can see...
> I think we need to get that element out into the hrtimer module rather than
> here.  A utility function doing most of this makes sense in the core
> but not the hrtimer bit.

ok.

>
>> +static struct config_item *iio_hrtimer_make_item(struct config_group *group,
>> +                                              const char *name)
>> +{
>> +     struct iio_trigger_item *trigger_item;
>> +     struct iio_configfs_trigger_info *trigger_info;
>> +     struct iio_configfs_trigger_type *trigger_type;
>> +     int ret;
>> +
>> +     trigger_item = kzalloc(sizeof(*trigger_item), GFP_KERNEL);
>> +     if (!trigger_item)
>> +             return ERR_PTR(-ENOMEM);
>> +     trigger_info = kzalloc(sizeof(*trigger_info), GFP_KERNEL);
>> +     if (!trigger_info) {
>> +             ret = -ENOMEM;
>> +             goto out_free_trigger_item;
>> +     }
>> +
>> +     trigger_info->name = kstrdup(name, GFP_KERNEL);
>> +     if (!trigger_info->name) {
>> +             ret = -ENOMEM;
>> +             goto out_free_trigger_info;
>> +     }
>> +
>> +     trigger_type = iio_get_configfs_trigger_type(IIO_TRIGGER_TYPE_HRTIMER);
>> +     if (!trigger_type) {
>> +             pr_err("Unable to get hrtimer trigger!\n");
>> +             ret = -ENODEV;
>> +             goto out_free_trigger_name;
>> +     }
>> +     ret = trigger_type->trigger_ops->probe(trigger_info);
>> +     if (ret < 0)
>> +             goto out_error_module_put;
>> +
>> +     trigger_info->trigger_type = trigger_type;
>> +     trigger_item->trigger_info = trigger_info;
>> +
>> +     config_item_init_type_name(&trigger_item->item, name,
>> +                                &iio_trigger_item_type);
>> +
>> +     return &trigger_item->item;
>> +out_error_module_put:
>> +     module_put(trigger_type->owner);
>> +out_free_trigger_name:
>> +     kfree(trigger_info->name);
>> +out_free_trigger_info:
>> +     kfree(trigger_info);
>> +out_free_trigger_item:
>> +     kfree(trigger_item);
>> +     return ERR_PTR(ret);
>> +}
>> +
> This one is no way at all hrtimer specific that I can see!

Well, here we register the iio_trigger_item_type, which has the
"iio_trigger_item_attrs" attributes field.

If a new trigger type has a different set of attributes then
iio_trigger_item_type
would need a different instance.

But I see now how "iio_hrtimer_make_item" needs to be made generic.

>> +static void iio_hrtimer_drop_item(struct config_group *group,
>> +                               struct config_item *item)
>> +{
>> +     struct iio_trigger_item *trigger_item;
>> +     struct iio_configfs_trigger_type *trigger_type;
>> +
>> +     trigger_item = container_of(item, struct iio_trigger_item, item);
>> +     trigger_type = trigger_item->trigger_info->trigger_type;
>> +
>> +     trigger_type->trigger_ops->remove(trigger_item->trigger_info);
>> +     module_put(trigger_type->owner);
>> +     kfree(trigger_item->trigger_info->name);
>> +     kfree(trigger_item->trigger_info);
>> +     kfree(trigger_item);
>> +}
>> +
>> +static struct configfs_group_operations iio_triggers_hrtimer_group_ops = {
> So to make the separation clean we need to create this object dynamically or pass
> it through from the hrtimer trigger module.
>> +     .make_item = iio_hrtimer_make_item,
>> +     .drop_item = iio_hrtimer_drop_item,
>> +};
>> +
>> +static struct config_item_type iio_trigger_item_type = {
>> +     .ct_owner       = THIS_MODULE,
>> +     .ct_item_ops    = &iio_trigger_item_ops,
>> +     .ct_attrs       = iio_trigger_item_attrs,
>> +};
>> +
>> +static struct config_item_type iio_triggers_hrtimer_type = {
>> +     .ct_owner       = THIS_MODULE,
>> +     .ct_group_ops   = &iio_triggers_hrtimer_group_ops,
>> +};
> So this one also needs to come from the hrtimer module.

Hmm. I would also like that hrtimer module to be independent of
configfs stuff. For example, one could use hrtimer trigger from within
the kernel without even the need of iio configfs support to be compiled
in.

I will try to see if this is possible and also address your comment.

>> +
>> +static struct config_group iio_triggers_hrtimer_group = {
>> +     .cg_item = {
>> +             .ci_namebuf = "hrtimer",
>> +             .ci_type = &iio_triggers_hrtimer_type,
>> +     },
>> +};
>> +
>>  static struct config_group *iio_triggers_default_groups[] = {
>> +     &iio_triggers_hrtimer_group,
>>       NULL
>>  };
>>
>> @@ -109,6 +272,7 @@ static struct configfs_subsystem iio_configfs_subsys = {
>>
>>  static int __init iio_configfs_init(void)
>>  {
>> +     config_group_init(&iio_triggers_hrtimer_group);
> ahh... We have to do the init in this order? No dynamic creation later?

This can be done. But I'm not sure we really want this.

Now "hrtimer" is just a default group so that it automagically
appears in /config/iio/triggers/ when the configfs is mounted.

If we want this to be dynamically created later then some userspace
application should be aware of the "hrtimer" trigger type existence
and create the hrtimer directory at some point.

>>       config_group_init(&iio_triggers_group);
>>       config_group_init(&iio_configfs_subsys.su_group);
>>
>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
>> index 7999612..e21688e 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"
>> +     depends on IIO_CONFIGFS
>> +     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..0acde8d
>> --- /dev/null
>> +++ b/drivers/iio/trigger/iio-trig-hrtimer.c
>> @@ -0,0 +1,154 @@
>> +/**
>> + * 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>
>> + *
>> + * 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/iio_configfs_trigger.h>
>> +
>> +/* default delay in ns (100Hz) */
>> +#define HRTIMER_TRIGGER_DEFAULT_DELAY 100000000
>> +
>> +struct iio_hrtimer_trig_info {
>> +     struct iio_configfs_trigger_info *trigger_info;
>> +     struct hrtimer timer;
>> +     unsigned long delay;
>> +};
>> +
>> +static enum hrtimer_restart iio_trig_hrtimer_trig(struct hrtimer *timer)
>> +{
>> +     struct iio_hrtimer_trig_info *trig_info;
>> +
>> +     trig_info = container_of(timer, struct iio_hrtimer_trig_info, timer);
>> +
>> +     hrtimer_forward_now(timer, ns_to_ktime(trig_info->delay));
>> +     iio_trigger_poll(trig_info->trigger_info->trigger);
>> +
>> +     return HRTIMER_RESTART;
>> +}
>> +
>> +static int iio_trig_hrtimer_set_state(struct iio_trigger *trig, bool state)
>> +{
>> +     struct iio_hrtimer_trig_info *trig_info;
>> +
>> +     trig_info = iio_trigger_get_drvdata(trig);
>> +
>> +     if (state)
>> +             hrtimer_start(&trig_info->timer, ns_to_ktime(trig_info->delay),
>> +                           HRTIMER_MODE_REL);
>> +     else
>> +             hrtimer_cancel(&trig_info->timer);
>> +
>> +     return 0;
>> +}
>> +
>> +int iio_trig_hrtimer_get_delay(struct iio_configfs_trigger_info *t,
>> +                            unsigned long *delay)
>> +{
>> +     struct iio_hrtimer_trig_info *trig_info;
>> +
>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>> +     *delay = trig_info->delay;
>> +
>> +     return 0;
>> +}
>> +
>> +int iio_trig_hrtimer_set_delay(struct iio_configfs_trigger_info *t,
>> +                            unsigned long delay)
>> +{
>> +     struct iio_hrtimer_trig_info *trig_info;
>> +
>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>> +
>> +     if (!delay)
>> +             return -EINVAL;
>> +
>> +     trig_info->delay = delay;
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
>> +     .owner = THIS_MODULE,
>> +     .set_trigger_state = iio_trig_hrtimer_set_state,
>> +};
>> +
>> +static int iio_trig_hrtimer_probe(struct iio_configfs_trigger_info *t)
>> +{
>> +     struct iio_hrtimer_trig_info *trig_info;
>> +     int ret;
>> +
>> +     trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
>> +     if (!trig_info)
>> +             return -ENOMEM;
>> +
>> +     trig_info->trigger_info = t;
>> +
>> +     t->trigger = iio_trigger_alloc("%s", t->name);
>> +     if (!t->trigger)
>> +             return -ENOMEM;
>> +
>> +     iio_trigger_set_drvdata(t->trigger, trig_info);
>> +     t->trigger->ops = &iio_hrtimer_trigger_ops;
>> +
>> +     hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> +     trig_info->timer.function = iio_trig_hrtimer_trig;
>> +
>> +     trig_info->delay = HRTIMER_TRIGGER_DEFAULT_DELAY;
>> +
>> +     ret = iio_trigger_register(t->trigger);
>> +     if (ret)
>> +             goto err_free_trigger;
>> +
>> +     return 0;
>> +err_free_trigger:
>> +     iio_trigger_free(t->trigger);
>> +
>> +     return ret;
>> +}
>> +
>> +static int iio_trig_hrtimer_remove(struct iio_configfs_trigger_info *t)
>> +{
>> +     struct iio_hrtimer_trig_info *trig_info;
>> +
>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>> +
>> +     iio_trigger_unregister(t->trigger);
>> +     hrtimer_cancel(&trig_info->timer);
>> +     iio_trigger_free(t->trigger);
>> +
>> +     return 0;
>> +}
>> +
>> +struct iio_configfs_trigger_ops hrtimer_trigger_ops = {
>> +     .get_delay      = iio_trig_hrtimer_get_delay,
>> +     .set_delay      = iio_trig_hrtimer_set_delay,
>> +     .probe          = iio_trig_hrtimer_probe,
>> +     .remove         = iio_trig_hrtimer_remove,
>> +};
>> +
>> +struct iio_configfs_trigger_type iio_configfs_hrtimer = {
>> +     .type = IIO_TRIGGER_TYPE_HRTIMER,
>> +     .owner = THIS_MODULE,
>> +     .trigger_ops = &hrtimer_trigger_ops,
>> +};
>> +
>> +module_iio_configfs_trigger_driver(iio_configfs_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");
>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>> index 75e76f2..6233733 100644
>> --- a/include/linux/iio/iio_configfs_trigger.h
>> +++ b/include/linux/iio/iio_configfs_trigger.h
>> @@ -10,6 +10,7 @@
>>                     unregister_configfs_trigger)
>>
>>  enum {
>> +     IIO_TRIGGER_TYPE_HRTIMER = 0,
>>       IIO_TRIGGER_TYPE_MAX,
>>  };
>>
>>
>

Thanks a lot for your valuable feedback Jonathan :).

Daniel.

Re: [PATCH v3 2/3] iio: trigger: Add support for highres timer trigger type

[Jonathan Cameron]

On 10/04/15 10:02, Daniel Baluta wrote:
> On Thu, Apr 9, 2015 at 8:09 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 06/04/15 15:18, Daniel Baluta wrote:
>>> This patch adds a new trigger type - the high resoluton timer ("hrtimer")
>>> under /config/iio/triggers/ and also creates a module that implements hrtimer
>>> trigger type functionality.
>>>
>>> A new IIO trigger is allocated when a new directory is created under
>>> /config/iio/triggers/. The name of the trigger is the same as the dir
>>> name. Each trigger will have a "delay" attribute representing the delay
>>> between two successive IIO trigger_poll calls.
>> Cool ;)
>>>
>>> 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>
>> I'd ideally have liked the break up of the patches to be slightly different.
>> There is stuff in here that would have made more sense in the first patch
>> as it isn't specific to this particular trigger.
> 
> Yes :), this was my first thought too (implemented in v1), but then I wanted
> to make a complete example on how to add a new trigger type.
I can see where you are coming from, but really think we need to
make it so there is less of each trigger implementation in the core.
Preferably none!
> 
> People implementing a new trigger type will find this patch very useful.
> 
> Anyhow, I have no objection on this. Can be fixed.
>>> ---
>>> Changes since v2:
>>>       * none
>>> Changes since v1:
>>>       * replaced sampling_frequency with delay to make the in kernel
>>>       processing easier
>> Then along comes me an suggests going back again ;)  Make your case...
> 
> Initial hrtimer trigger implementation was limited to using integer sampling
> frequencies. Adding "rational" frequencies (e.g 12.5 Hz) and converting them
> back to hrtimer trigger delay  would add some complex calculation in the kernel
> with the possibility of losing precision.
> 
> Besides that, for most devices sampling_frequencies (Hz) attribute is natural
> because this is what the devices expose in its registers, while the timers
> API uses delays (nsec).
> 
> Here is how I see the usage of device's sampling_frequency and trigger's
> delay (this should be kept in sync)
> 
> User application reads device's sampling_frequency and then based on that
> it sets trigger's delay to 1/sampling_frequency -- all of that happening in user
> space where floating point is easy :).
I'm not keen on the flipping backwards and forwards going on here.  To my
mind the two should be the same.

I'm a little unclear on when we'd actually hit your example and whether
this is the right approach if we do.

The usual case for using a 'software' trigger is for devices with no
sampling clock of their own.  So devices with a 'sample now' signal
(be this an explicit one or a case where the device samples using the
bus clock to drive the adc).

So here you are dealing with devices that would actually have a dataready
type signal, but it's not exposed? Any attempt to match sampling is just
destined to go wrong.  If you want to guarantee getting all the data you'll
have to over sample, probably by a significant degree given that you'll
have some variation in any software trigger.

In most (possibly) all devices there is a software means of reading whether
their is new data available.  I'd suggest in this case we do actually need
to move the polling logic into the driver.  It simply doesn't
make sense to use the generic timer to do it as we won't sample ever time.

Hence you'll end up with a trigger that has an underlying 'hidden' poll
of the device and only fires when there is new data available - much cleaner
interface to userspace and reliable data capture without repeat samples
and all the mess that will entail.
> 
>>>       * implemented hrtimer driver using the new API proposed in patch 1
>>>
>>>  drivers/iio/industrialio-configfs.c      | 164 +++++++++++++++++++++++++++++++
>>>  drivers/iio/trigger/Kconfig              |   9 ++
>>>  drivers/iio/trigger/Makefile             |   2 +
>>>  drivers/iio/trigger/iio-trig-hrtimer.c   | 154 +++++++++++++++++++++++++++++
>>>  include/linux/iio/iio_configfs_trigger.h |   1 +
>>>  5 files changed, 330 insertions(+)
>>>  create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c
>>>
>>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>>> index d8ffd76..a0e5d24 100644
>>> --- a/drivers/iio/industrialio-configfs.c
>>> +++ b/drivers/iio/industrialio-configfs.c
>>> @@ -71,7 +71,170 @@ struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>>>       return t;
>>>  }
>>>
>>> +struct iio_trigger_item {
>>> +     struct config_item item;
>>> +     struct iio_configfs_trigger_info *trigger_info;
>>> +};
>>> +
>>> +static
>>> +inline struct iio_trigger_item *to_iio_trigger_item(struct config_item *item)
>>> +{
>>> +     if (!item)
>>> +             return NULL;
>>> +     return container_of(item, struct iio_trigger_item, item);
>>> +}
>>> +
>>> +CONFIGFS_ATTR_STRUCT(iio_trigger_item);
>>> +
>>> +#define IIO_TRIGGER_ITEM_ATTR(_name, _mode, _show, _store) \
>>> +struct iio_trigger_item_attribute iio_trigger_item_attr_##_name = \
>>> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
>>> +
>>> +static
>>> +ssize_t iio_trigger_item_get_delay(struct iio_trigger_item *item, char *page)
>>> +{
>>> +     unsigned long delay;
>>> +     int ret;
>>> +
>>> +     struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
>>> +
>>> +     ret = t->trigger_ops->get_delay(item->trigger_info, &delay);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return snprintf(page, PAGE_SIZE, "%lu\n", delay);
>>> +}
>>> +
>>> +static
>>> +ssize_t iio_trigger_item_set_delay(struct iio_trigger_item *item,
>>> +                                const char *page,
>>> +                                size_t count)
>>> +{
>>> +     int ret;
>>> +     unsigned long delay;
>>> +     struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
>>> +
>>> +     ret = kstrtoul(page, 10, &delay);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     ret = t->trigger_ops->set_delay(item->trigger_info, delay);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return count;
>>> +}
>>> +
>>> +IIO_TRIGGER_ITEM_ATTR(delay, S_IRUGO | S_IWUSR,
>>> +                   iio_trigger_item_get_delay,
>>> +                   iio_trigger_item_set_delay);
>>> +
>>> +static struct configfs_attribute *iio_trigger_item_attrs[] = {
>>> +     &iio_trigger_item_attr_delay.attr,
>>> +     NULL
>>> +};
>>> +
>>> +CONFIGFS_ATTR_OPS(iio_trigger_item);
>>> +
>>> +static struct configfs_item_operations iio_trigger_item_ops = {
>>> +     .show_attribute = iio_trigger_item_attr_show,
>>> +     .store_attribute = iio_trigger_item_attr_store,
>>> +};
>> So everything down to here would have fitted a little better in the previous
>> patch to my mind.
> 
> ok. As said above, one way of doing this would be to start with
> an empty configfs support than build new trigger types on top
> of it.
> 
>>
>>> +
>>> +static struct config_item_type iio_trigger_item_type;
>>> +
>>
>> Why is this hrtimer specific?  Only one real place that I can see...
>> I think we need to get that element out into the hrtimer module rather than
>> here.  A utility function doing most of this makes sense in the core
>> but not the hrtimer bit.
> 
> ok.
> 
>>
>>> +static struct config_item *iio_hrtimer_make_item(struct config_group *group,
>>> +                                              const char *name)
>>> +{
>>> +     struct iio_trigger_item *trigger_item;
>>> +     struct iio_configfs_trigger_info *trigger_info;
>>> +     struct iio_configfs_trigger_type *trigger_type;
>>> +     int ret;
>>> +
>>> +     trigger_item = kzalloc(sizeof(*trigger_item), GFP_KERNEL);
>>> +     if (!trigger_item)
>>> +             return ERR_PTR(-ENOMEM);
>>> +     trigger_info = kzalloc(sizeof(*trigger_info), GFP_KERNEL);
>>> +     if (!trigger_info) {
>>> +             ret = -ENOMEM;
>>> +             goto out_free_trigger_item;
>>> +     }
>>> +
>>> +     trigger_info->name = kstrdup(name, GFP_KERNEL);
>>> +     if (!trigger_info->name) {
>>> +             ret = -ENOMEM;
>>> +             goto out_free_trigger_info;
>>> +     }
>>> +
>>> +     trigger_type = iio_get_configfs_trigger_type(IIO_TRIGGER_TYPE_HRTIMER);
>>> +     if (!trigger_type) {
>>> +             pr_err("Unable to get hrtimer trigger!\n");
>>> +             ret = -ENODEV;
>>> +             goto out_free_trigger_name;
>>> +     }
>>> +     ret = trigger_type->trigger_ops->probe(trigger_info);
>>> +     if (ret < 0)
>>> +             goto out_error_module_put;
>>> +
>>> +     trigger_info->trigger_type = trigger_type;
>>> +     trigger_item->trigger_info = trigger_info;
>>> +
>>> +     config_item_init_type_name(&trigger_item->item, name,
>>> +                                &iio_trigger_item_type);
>>> +
>>> +     return &trigger_item->item;
>>> +out_error_module_put:
>>> +     module_put(trigger_type->owner);
>>> +out_free_trigger_name:
>>> +     kfree(trigger_info->name);
>>> +out_free_trigger_info:
>>> +     kfree(trigger_info);
>>> +out_free_trigger_item:
>>> +     kfree(trigger_item);
>>> +     return ERR_PTR(ret);
>>> +}
>>> +
>> This one is no way at all hrtimer specific that I can see!
> 
> Well, here we register the iio_trigger_item_type, which has the
> "iio_trigger_item_attrs" attributes field.
Yeah, a slight exageration on my part there ;)
> 
> If a new trigger type has a different set of attributes then
> iio_trigger_item_type
> would need a different instance.
> 
> But I see now how "iio_hrtimer_make_item" needs to be made generic.
cool
> 
>>> +static void iio_hrtimer_drop_item(struct config_group *group,
>>> +                               struct config_item *item)
>>> +{
>>> +     struct iio_trigger_item *trigger_item;
>>> +     struct iio_configfs_trigger_type *trigger_type;
>>> +
>>> +     trigger_item = container_of(item, struct iio_trigger_item, item);
>>> +     trigger_type = trigger_item->trigger_info->trigger_type;
>>> +
>>> +     trigger_type->trigger_ops->remove(trigger_item->trigger_info);
>>> +     module_put(trigger_type->owner);
>>> +     kfree(trigger_item->trigger_info->name);
>>> +     kfree(trigger_item->trigger_info);
>>> +     kfree(trigger_item);
>>> +}
>>> +
>>> +static struct configfs_group_operations iio_triggers_hrtimer_group_ops = {
>> So to make the separation clean we need to create this object dynamically or pass
>> it through from the hrtimer trigger module.
>>> +     .make_item = iio_hrtimer_make_item,
>>> +     .drop_item = iio_hrtimer_drop_item,
>>> +};
>>> +
>>> +static struct config_item_type iio_trigger_item_type = {
>>> +     .ct_owner       = THIS_MODULE,
>>> +     .ct_item_ops    = &iio_trigger_item_ops,
>>> +     .ct_attrs       = iio_trigger_item_attrs,
>>> +};
>>> +
>>> +static struct config_item_type iio_triggers_hrtimer_type = {
>>> +     .ct_owner       = THIS_MODULE,
>>> +     .ct_group_ops   = &iio_triggers_hrtimer_group_ops,
>>> +};
>> So this one also needs to come from the hrtimer module.
> 
> Hmm. I would also like that hrtimer module to be independent of
> configfs stuff. For example, one could use hrtimer trigger from within
> the kernel without even the need of iio configfs support to be compiled
> in.
> 
> I will try to see if this is possible and also address your comment.
> 
>>> +
>>> +static struct config_group iio_triggers_hrtimer_group = {
>>> +     .cg_item = {
>>> +             .ci_namebuf = "hrtimer",
>>> +             .ci_type = &iio_triggers_hrtimer_type,
>>> +     },
>>> +};
>>> +
>>>  static struct config_group *iio_triggers_default_groups[] = {
>>> +     &iio_triggers_hrtimer_group,
>>>       NULL
>>>  };
>>>
>>> @@ -109,6 +272,7 @@ static struct configfs_subsystem iio_configfs_subsys = {
>>>
>>>  static int __init iio_configfs_init(void)
>>>  {
>>> +     config_group_init(&iio_triggers_hrtimer_group);
>> ahh... We have to do the init in this order? No dynamic creation later?
> 
> This can be done. But I'm not sure we really want this.
> 
> Now "hrtimer" is just a default group so that it automagically
> appears in /config/iio/triggers/ when the configfs is mounted.
> 
> If we want this to be dynamically created later then some userspace
> application should be aware of the "hrtimer" trigger type existence
> and create the hrtimer directory at some point.
See the tricks used in the usb gadget driver to solve an equivalent
issue.  There the core has no knowledge at all of what the different
function drivers are called or do.  That's what we want to be aiming for.
> 
>>>       config_group_init(&iio_triggers_group);
>>>       config_group_init(&iio_configfs_subsys.su_group);
>>>
>>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
>>> index 7999612..e21688e 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"
>>> +     depends on IIO_CONFIGFS
>>> +     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..0acde8d
>>> --- /dev/null
>>> +++ b/drivers/iio/trigger/iio-trig-hrtimer.c
>>> @@ -0,0 +1,154 @@
>>> +/**
>>> + * 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>
>>> + *
>>> + * 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/iio_configfs_trigger.h>
>>> +
>>> +/* default delay in ns (100Hz) */
>>> +#define HRTIMER_TRIGGER_DEFAULT_DELAY 100000000
>>> +
>>> +struct iio_hrtimer_trig_info {
>>> +     struct iio_configfs_trigger_info *trigger_info;
>>> +     struct hrtimer timer;
>>> +     unsigned long delay;
>>> +};
>>> +
>>> +static enum hrtimer_restart iio_trig_hrtimer_trig(struct hrtimer *timer)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = container_of(timer, struct iio_hrtimer_trig_info, timer);
>>> +
>>> +     hrtimer_forward_now(timer, ns_to_ktime(trig_info->delay));
>>> +     iio_trigger_poll(trig_info->trigger_info->trigger);
>>> +
>>> +     return HRTIMER_RESTART;
>>> +}
>>> +
>>> +static int iio_trig_hrtimer_set_state(struct iio_trigger *trig, bool state)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(trig);
>>> +
>>> +     if (state)
>>> +             hrtimer_start(&trig_info->timer, ns_to_ktime(trig_info->delay),
>>> +                           HRTIMER_MODE_REL);
>>> +     else
>>> +             hrtimer_cancel(&trig_info->timer);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int iio_trig_hrtimer_get_delay(struct iio_configfs_trigger_info *t,
>>> +                            unsigned long *delay)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>>> +     *delay = trig_info->delay;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int iio_trig_hrtimer_set_delay(struct iio_configfs_trigger_info *t,
>>> +                            unsigned long delay)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>>> +
>>> +     if (!delay)
>>> +             return -EINVAL;
>>> +
>>> +     trig_info->delay = delay;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
>>> +     .owner = THIS_MODULE,
>>> +     .set_trigger_state = iio_trig_hrtimer_set_state,
>>> +};
>>> +
>>> +static int iio_trig_hrtimer_probe(struct iio_configfs_trigger_info *t)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +     int ret;
>>> +
>>> +     trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
>>> +     if (!trig_info)
>>> +             return -ENOMEM;
>>> +
>>> +     trig_info->trigger_info = t;
>>> +
>>> +     t->trigger = iio_trigger_alloc("%s", t->name);
>>> +     if (!t->trigger)
>>> +             return -ENOMEM;
>>> +
>>> +     iio_trigger_set_drvdata(t->trigger, trig_info);
>>> +     t->trigger->ops = &iio_hrtimer_trigger_ops;
>>> +
>>> +     hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>> +     trig_info->timer.function = iio_trig_hrtimer_trig;
>>> +
>>> +     trig_info->delay = HRTIMER_TRIGGER_DEFAULT_DELAY;
>>> +
>>> +     ret = iio_trigger_register(t->trigger);
>>> +     if (ret)
>>> +             goto err_free_trigger;
>>> +
>>> +     return 0;
>>> +err_free_trigger:
>>> +     iio_trigger_free(t->trigger);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int iio_trig_hrtimer_remove(struct iio_configfs_trigger_info *t)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>>> +
>>> +     iio_trigger_unregister(t->trigger);
>>> +     hrtimer_cancel(&trig_info->timer);
>>> +     iio_trigger_free(t->trigger);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +struct iio_configfs_trigger_ops hrtimer_trigger_ops = {
>>> +     .get_delay      = iio_trig_hrtimer_get_delay,
>>> +     .set_delay      = iio_trig_hrtimer_set_delay,
>>> +     .probe          = iio_trig_hrtimer_probe,
>>> +     .remove         = iio_trig_hrtimer_remove,
>>> +};
>>> +
>>> +struct iio_configfs_trigger_type iio_configfs_hrtimer = {
>>> +     .type = IIO_TRIGGER_TYPE_HRTIMER,
>>> +     .owner = THIS_MODULE,
>>> +     .trigger_ops = &hrtimer_trigger_ops,
>>> +};
>>> +
>>> +module_iio_configfs_trigger_driver(iio_configfs_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");
>>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>>> index 75e76f2..6233733 100644
>>> --- a/include/linux/iio/iio_configfs_trigger.h
>>> +++ b/include/linux/iio/iio_configfs_trigger.h
>>> @@ -10,6 +10,7 @@
>>>                     unregister_configfs_trigger)
>>>
>>>  enum {
>>> +     IIO_TRIGGER_TYPE_HRTIMER = 0,
>>>       IIO_TRIGGER_TYPE_MAX,
>>>  };
>>>
>>>
>>
> 
> Thanks a lot for your valuable feedback Jonathan :).
> 
> Daniel.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 2/3] iio: trigger: Add support for highres timer trigger type

[Daniel Baluta]

On Sun, Apr 12, 2015 at 6:40 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 10/04/15 10:02, Daniel Baluta wrote:
>> On Thu, Apr 9, 2015 at 8:09 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 06/04/15 15:18, Daniel Baluta wrote:
>>>> This patch adds a new trigger type - the high resoluton timer ("hrtimer")
>>>> under /config/iio/triggers/ and also creates a module that implements hrtimer
>>>> trigger type functionality.
>>>>
>>>> A new IIO trigger is allocated when a new directory is created under
>>>> /config/iio/triggers/. The name of the trigger is the same as the dir
>>>> name. Each trigger will have a "delay" attribute representing the delay
>>>> between two successive IIO trigger_poll calls.
>>> Cool ;)
>>>>
>>>> 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>
>>> I'd ideally have liked the break up of the patches to be slightly different.
>>> There is stuff in here that would have made more sense in the first patch
>>> as it isn't specific to this particular trigger.
>>
>> Yes :), this was my first thought too (implemented in v1), but then I wanted
>> to make a complete example on how to add a new trigger type.
> I can see where you are coming from, but really think we need to
> make it so there is less of each trigger implementation in the core.
> Preferably none!
>>
>> People implementing a new trigger type will find this patch very useful.
>>
>> Anyhow, I have no objection on this. Can be fixed.
>>>> ---
>>>> Changes since v2:
>>>>       * none
>>>> Changes since v1:
>>>>       * replaced sampling_frequency with delay to make the in kernel
>>>>       processing easier
>>> Then along comes me an suggests going back again ;)  Make your case...
>>
>> Initial hrtimer trigger implementation was limited to using integer sampling
>> frequencies. Adding "rational" frequencies (e.g 12.5 Hz) and converting them
>> back to hrtimer trigger delay  would add some complex calculation in the kernel
>> with the possibility of losing precision.
>>
>> Besides that, for most devices sampling_frequencies (Hz) attribute is natural
>> because this is what the devices expose in its registers, while the timers
>> API uses delays (nsec).
>>
>> Here is how I see the usage of device's sampling_frequency and trigger's
>> delay (this should be kept in sync)
>>
>> User application reads device's sampling_frequency and then based on that
>> it sets trigger's delay to 1/sampling_frequency -- all of that happening in user
>> space where floating point is easy :).
> I'm not keen on the flipping backwards and forwards going on here.  To my
> mind the two should be the same.

I see. So it shouldn't be any direct relation between device
sampling_frequency and
trigger sampling_frequency.

Now, I'm afraid that users will get confused about this naming and they'll
try to keep both of them in sync.

To sum up, I will also use sampling_frequency also for triggers and
make sure this is
well documented. Not sure if supporting rational frequencies will
bring any benefit
(e.g 12.5), since we won't be able to match the exact sampling
frequency of the device.

>
> I'm a little unclear on when we'd actually hit your example and whether
> this is the right approach if we do.
>
> The usual case for using a 'software' trigger is for devices with no
> sampling clock of their own.  So devices with a 'sample now' signal
> (be this an explicit one or a case where the device samples using the
> bus clock to drive the adc).
>
> So here you are dealing with devices that would actually have a dataready
> type signal, but it's not exposed? Any attempt to match sampling is just
> destined to go wrong.  If you want to guarantee getting all the data you'll
> have to over sample, probably by a significant degree given that you'll
> have some variation in any software trigger.

There are devices which do not have an interrupt pin (by design).
Also, there are
cases where is not possible to know if new data is ready. You'll just
have to read it
and compare it with previous values to really figure out if it's new.

>
> In most (possibly) all devices there is a software means of reading whether
> their is new data available.  I'd suggest in this case we do actually need
> to move the polling logic into the driver.  It simply doesn't
> make sense to use the generic timer to do it as we won't sample ever time.

It makes some sense because we want to have a generic way of support
triggered buffers
without having to modify the driver to use polling.

>
> Hence you'll end up with a trigger that has an underlying 'hidden' poll
> of the device and only fires when there is new data available - much cleaner
> interface to userspace and reliable data capture without repeat samples
> and all the mess that will entail.
>>

I agree here that the interface to userspace will be cleaner in this
case (in fact the user
doesn't have to do anything).  But the amount of work needed for the user
in case we use generic trigger described above should be negligible compared
to the amount of work needed to implement polling in every driver.

Re: [PATCH v3 2/3] iio: trigger: Add support for highres timer trigger type

[Jonathan Cameron]

On 15/04/15 16:33, Daniel Baluta wrote:
> On Sun, Apr 12, 2015 at 6:40 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 10/04/15 10:02, Daniel Baluta wrote:
>>> On Thu, Apr 9, 2015 at 8:09 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 06/04/15 15:18, Daniel Baluta wrote:
>>>>> This patch adds a new trigger type - the high resoluton timer ("hrtimer")
>>>>> under /config/iio/triggers/ and also creates a module that implements hrtimer
>>>>> trigger type functionality.
>>>>>
>>>>> A new IIO trigger is allocated when a new directory is created under
>>>>> /config/iio/triggers/. The name of the trigger is the same as the dir
>>>>> name. Each trigger will have a "delay" attribute representing the delay
>>>>> between two successive IIO trigger_poll calls.
>>>> Cool ;)
>>>>>
>>>>> 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>
>>>> I'd ideally have liked the break up of the patches to be slightly different.
>>>> There is stuff in here that would have made more sense in the first patch
>>>> as it isn't specific to this particular trigger.
>>>
>>> Yes :), this was my first thought too (implemented in v1), but then I wanted
>>> to make a complete example on how to add a new trigger type.
>> I can see where you are coming from, but really think we need to
>> make it so there is less of each trigger implementation in the core.
>> Preferably none!
>>>
>>> People implementing a new trigger type will find this patch very useful.
>>>
>>> Anyhow, I have no objection on this. Can be fixed.
>>>>> ---
>>>>> Changes since v2:
>>>>>       * none
>>>>> Changes since v1:
>>>>>       * replaced sampling_frequency with delay to make the in kernel
>>>>>       processing easier
>>>> Then along comes me an suggests going back again ;)  Make your case...
>>>
>>> Initial hrtimer trigger implementation was limited to using integer sampling
>>> frequencies. Adding "rational" frequencies (e.g 12.5 Hz) and converting them
>>> back to hrtimer trigger delay  would add some complex calculation in the kernel
>>> with the possibility of losing precision.
>>>
>>> Besides that, for most devices sampling_frequencies (Hz) attribute is natural
>>> because this is what the devices expose in its registers, while the timers
>>> API uses delays (nsec).
>>>
>>> Here is how I see the usage of device's sampling_frequency and trigger's
>>> delay (this should be kept in sync)
>>>
>>> User application reads device's sampling_frequency and then based on that
>>> it sets trigger's delay to 1/sampling_frequency -- all of that happening in user
>>> space where floating point is easy :).
>> I'm not keen on the flipping backwards and forwards going on here.  To my
>> mind the two should be the same.
> 
> I see. So it shouldn't be any direct relation between device
> sampling_frequency and
> trigger sampling_frequency.
> 
> Now, I'm afraid that users will get confused about this naming and they'll
> try to keep both of them in sync.
> 
> To sum up, I will also use sampling_frequency also for triggers and
> make sure this is
> well documented. Not sure if supporting rational frequencies will
> bring any benefit
> (e.g 12.5), since we won't be able to match the exact sampling
> frequency of the device.
This is kind of why I really don't like using a timer based trigger
for devices with their own sampling sequencers.  
They are suitable for either slow sampling of a much high sample rate
signal, or for devices where we have to initialize ever sample...
> 
>>
>> I'm a little unclear on when we'd actually hit your example and whether
>> this is the right approach if we do.
>>
>> The usual case for using a 'software' trigger is for devices with no
>> sampling clock of their own.  So devices with a 'sample now' signal
>> (be this an explicit one or a case where the device samples using the
>> bus clock to drive the adc).
>>
>> So here you are dealing with devices that would actually have a dataready
>> type signal, but it's not exposed? Any attempt to match sampling is just
>> destined to go wrong.  If you want to guarantee getting all the data you'll
>> have to over sample, probably by a significant degree given that you'll
>> have some variation in any software trigger.
> 
> There are devices which do not have an interrupt pin (by design).
> Also, there are
> cases where is not possible to know if new data is ready. You'll just
> have to read it
> and compare it with previous values to really figure out if it's new.
Yup, that's reasonably common, but normally only in the sort of device
where looking at changes is not of interest.  E.g. slow things
like temperature where you are just querying 'a recent ish temperature'.
> 
>>
>> In most (possibly) all devices there is a software means of reading whether
>> their is new data available.  I'd suggest in this case we do actually need
>> to move the polling logic into the driver.  It simply doesn't
>> make sense to use the generic timer to do it as we won't sample ever time.
> 
> It makes some sense because we want to have a generic way of support
> triggered buffers
> without having to modify the driver to use polling.
I think in most cases, polling a 'dataready register' is probably the
right option unfortunately...

Maybe we can come up with a bit of library code
that does the hard work of this...
> 
>>
>> Hence you'll end up with a trigger that has an underlying 'hidden' poll
>> of the device and only fires when there is new data available - much cleaner
>> interface to userspace and reliable data capture without repeat samples
>> and all the mess that will entail.
>>>
> 
> I agree here that the interface to userspace will be cleaner in this
> case (in fact the user
> doesn't have to do anything).  But the amount of work needed for the user
> in case we use generic trigger described above should be negligible compared
> to the amount of work needed to implement polling in every driver.
> 
Realistically we only need to do it in drivers where we know anyone actually cares.
(so kind of on request).  For some sensors you'd be bonkers not to wire it
(e.g. anything fast).

I doubt we are really dealing with much code when you get down to it...

I guess we need to first implement it in a few drivers and see what we end up with.
Then factor out any useful utility code.

I'm definitely thinking we are going to end up with at worst 10-20 lines of actual
code per driver.  The fiddly stuff is doing the interrupt generation but that
can definitely be a core provided function.

We may want to be a little clever there and only provide a true interrupt if
any of the client devices of the trigger have a top half registered.  Not sure
if we can actually do that though...

Jonathan

[PATCH v3 3/3] iio: Documentation: Add documentation for IIO configfs

[Daniel Baluta]

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
Changes since v2:
	* fix some whitespace issues reported by Paul

Changes since v1:
	* addressed feedback for v1:
		* https://lkml.org/lkml/2015/3/25/648
	* adapted to match the changes in patches 1 and 2
	* fixed some typos and clarified further work

 Documentation/iio/iio_configfs.txt | 83 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/iio/iio_configfs.txt

diff --git a/Documentation/iio/iio_configfs.txt b/Documentation/iio/iio_configfs.txt
new file mode 100644
index 0000000..ace7b54
--- /dev/null
+++ b/Documentation/iio/iio_configfs.txt
@@ -0,0 +1,83 @@
+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.
+
+Under /config/iio/triggers we will create trigger types. For the moment
+we have "hrtimer" trigger type and we plan to add at least one more type,
+the "sysfs" trigger type.
+
+3.1 Trigger types
+
+Represent a specific type of trigger. For now we have an implementation
+for "hrtimer" trigger type.
+
+3.2. Trigger creation and destruction
+
+As simply as:
+
+$ mkdir /config/triggers/hrtimer/my_trigger
+
+Creating my_trigger directory will result in creation of "my_trigger"
+trigger of "hrtimer" type.  Destruction happens when my_trigger directory
+is removed.
+
+Each trigger can have one or more attributes specific to the trigger type.
+
+3.3 "hrtimer" trigger types attributes
+
+"hrtimer" trigger type has only one attribute:
+
+$ ls /config/triggers/hrtimer/my_trigger
+delay
+
+delay - represents the amount of time in nanoseconds between two
+consecutive iio_trigger_poll calls. By default it is set to 100000000,
+that is the equivalent of a 100Hz frequency.
+
+3.4. Adding a new trigger type
+
+In order to add a new trigger type, one needs to:
+
+* [iio_configfs_trigger.h]
+	** add a new IIO_TRIGGER_TYPE
+* [iio-trig-<type>.c]
+	** declare and initialize a structure of type
+	iio_configfs_trigger_type
+	** implement needed iio_configfs_trigger_ops
+	** register/unregister the new trigger
+	type with the IIO configfs core
+* [industrialiio-configfs.c]
+	** create a new group type and add it in the default group of
+	   trigger types.
+	** create new attributes and them to the IIO configfs core.
+
+4. Further work
+
+* add "sysfs" trigger type
-- 
1.9.1