IIO hrtimer trigger

IIO hrtimer trigger

[Denis CIOCCA]

Hi Lars,

Thanks for your review.
I reviewed the code in accordance with your comments, for the other point
can you explain me better please?
You intend to use one driver to manage all triggers added by sysfs?

Thanks,
Denis

[PATCH] iio:trigger: Added iio-hrtimer-trigger

[Denis CIOCCA]

This patch adds high resolution timer trigger support for iio framework.

Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 drivers/iio/trigger/Kconfig            |    9 +
 drivers/iio/trigger/Makefile           |    1 +
 drivers/iio/trigger/iio-trig-hrtimer.c |  292 ++++++++++++++++++++++++++++++++
 3 files changed, 302 insertions(+)
 create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c

diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
index 7999612..f0da684 100644
--- a/drivers/iio/trigger/Kconfig
+++ b/drivers/iio/trigger/Kconfig
@@ -5,6 +5,15 @@
 
 menu "Triggers - standalone"
 
+config IIO_HRTIMER_TRIGGER
+	tristate "HRTIMER trigger"
+	help
+	  Provides support for using HRTIMER entries as IIO triggers.
+	  If unsure, say N (but it's safe to say "Y").
+
+	  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..424032c 100644
--- a/drivers/iio/trigger/Makefile
+++ b/drivers/iio/trigger/Makefile
@@ -3,5 +3,6 @@
 #
 
 # 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..245c51d
--- /dev/null
+++ b/drivers/iio/trigger/iio-trig-hrtimer.c
@@ -0,0 +1,292 @@
+/*
+ * Industrial I/O - hrtimer trigger support
+ *
+ * Copyright 2013 STMicroelectronics Inc.
+ * Denis Ciocca <denis.ciocca@st.com>
+ *
+ * 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/module.h>
+#include <linux/hrtimer.h>
+#include <linux/ktime.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+
+struct iio_hrtimer_trigger_data {
+	struct iio_trigger *trig;
+	struct hrtimer timer;
+	struct list_head l;
+	ktime_t period;
+	u16  freq;
+	int id;
+};
+
+static LIST_HEAD(iio_hrtimer_trigger_list);
+static DEFINE_MUTEX(iio_hrtimer_trigger_list_mut);
+
+static int iio_hrtimer_trigger_probe(int id);
+static int iio_hrtimer_trigger_remove(int id);
+
+static ssize_t iio_sysfs_hrtimer_trig_add(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	int ret;
+	unsigned int input;
+
+	ret = kstrtouint(buf, 10, &input);
+	if (ret)
+		return ret;
+
+	ret = iio_hrtimer_trigger_probe(input);
+	if (ret)
+		return ret;
+
+	return len;
+}
+static DEVICE_ATTR(add_trigger, S_IWUSR, NULL, &iio_sysfs_hrtimer_trig_add);
+
+static ssize_t iio_sysfs_hrtimer_trig_remove(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	int ret;
+	unsigned int input;
+
+	ret = kstrtouint(buf, 10, &input);
+	if (ret)
+		return ret;
+
+	ret = iio_hrtimer_trigger_remove(input);
+	if (ret)
+		return ret;
+
+	return len;
+}
+static DEVICE_ATTR(remove_trigger, S_IWUSR,
+					NULL, &iio_sysfs_hrtimer_trig_remove);
+
+static struct attribute *iio_hrtimer_trig_attrs[] = {
+	&dev_attr_add_trigger.attr,
+	&dev_attr_remove_trigger.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_hrtimer_trig_group = {
+	.attrs = iio_hrtimer_trig_attrs,
+};
+
+static const struct attribute_group *iio_hrtimer_trig_groups[] = {
+	&iio_hrtimer_trig_group,
+	NULL,
+};
+
+static struct device iio_hrtimer_trig_dev = {
+	.bus = &iio_bus_type,
+	.groups = iio_hrtimer_trig_groups,
+};
+
+static int iio_hrtimer_trig_set_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_hrtimer_trigger_data *trig_data =
+						iio_trigger_get_drvdata(trig);
+
+	if (trig_data->freq == 0)
+		return -EINVAL;
+
+	if (state)
+		hrtimer_start(&trig_data->timer,
+					trig_data->period, HRTIMER_MODE_REL);
+	else
+		hrtimer_cancel(&trig_data->timer);
+
+	return 0;
+}
+
+static ssize_t iio_hrtimer_trigger_set_freq_value(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	int ret;
+	u16 frequency;
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct iio_hrtimer_trigger_data *trig_data =
+						iio_trigger_get_drvdata(trig);
+
+	ret = kstrtou16(buf, 10, &frequency);
+	if (ret < 0)
+		return ret;
+
+	trig_data->freq = frequency;
+
+	if (frequency)
+		trig_data->period =
+				ktime_set(0, NSEC_PER_SEC / trig_data->freq);
+
+	return len;
+}
+
+static ssize_t iio_hrtimer_trigger_get_freq_value(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct iio_hrtimer_trigger_data *trig_data =
+						iio_trigger_get_drvdata(trig);
+
+	return sprintf(buf, "%hu\n", trig_data->freq);
+}
+
+static DEVICE_ATTR(frequency, S_IWUSR | S_IRUGO,
+					iio_hrtimer_trigger_get_freq_value,
+					iio_hrtimer_trigger_set_freq_value);
+
+static ssize_t iio_hrtimer_trigger_get_id(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct iio_hrtimer_trigger_data *trig_data =
+						iio_trigger_get_drvdata(trig);
+
+	return sprintf(buf, "%d\n", trig_data->id);
+}
+
+static DEVICE_ATTR(id, S_IRUGO, iio_hrtimer_trigger_get_id, NULL);
+
+static struct attribute *iio_hrtimer_trigger_attrs[] = {
+	&dev_attr_frequency.attr,
+	&dev_attr_id.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_hrtimer_trigger_attr_group = {
+	.attrs = iio_hrtimer_trigger_attrs,
+};
+
+static const struct attribute_group *iio_hrtimer_trigger_attr_groups[] = {
+	&iio_hrtimer_trigger_attr_group,
+	NULL,
+};
+
+static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = &iio_hrtimer_trig_set_state,
+};
+
+enum hrtimer_restart iio_hrtimer_trigger_func(struct hrtimer *timer)
+{
+	struct iio_hrtimer_trigger_data *trig_data;
+
+	trig_data = container_of(timer, struct iio_hrtimer_trigger_data, timer);
+
+	hrtimer_forward_now(timer, trig_data->period);
+	iio_trigger_poll(trig_data->trig, iio_get_time_ns());
+
+	return HRTIMER_RESTART;
+}
+
+static int iio_hrtimer_trigger_probe(int id)
+{
+	int err;
+	bool foundit = false;
+	struct iio_hrtimer_trigger_data *trig_data;
+
+	mutex_lock(&iio_hrtimer_trigger_list_mut);
+	list_for_each_entry(trig_data, &iio_hrtimer_trigger_list, l) {
+		if (id == trig_data->id) {
+			foundit = true;
+			break;
+		}
+	}
+	if (foundit) {
+		err = -EINVAL;
+		goto iio_hrtimer_mutex_unlock;
+	}
+
+	trig_data = kmalloc(sizeof(*trig_data), GFP_KERNEL);
+	if (trig_data == NULL) {
+		err = -ENOMEM;
+		goto iio_hrtimer_mutex_unlock;
+	}
+
+	trig_data->id = id;
+	trig_data->trig = iio_trigger_alloc("hrtimer_trig%d", id);
+	if (!trig_data->trig) {
+		err = -ENOMEM;
+		goto iio_hrtimer_free_trig_data;
+	}
+
+	trig_data->trig->dev.groups = iio_hrtimer_trigger_attr_groups;
+	trig_data->trig->ops = &iio_hrtimer_trigger_ops;
+	trig_data->trig->dev.parent = &iio_hrtimer_trig_dev;
+	iio_trigger_set_drvdata(trig_data->trig, trig_data);
+
+	trig_data->freq = 0;
+	hrtimer_init(&trig_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	trig_data->timer.function = &iio_hrtimer_trigger_func;
+
+	err = iio_trigger_register(trig_data->trig);
+	if (err)
+		goto iio_hrtimer_free_trig_data;
+
+	list_add(&trig_data->l, &iio_hrtimer_trigger_list);
+	__module_get(THIS_MODULE);
+	mutex_unlock(&iio_hrtimer_trigger_list_mut);
+
+	return 0;
+
+iio_hrtimer_free_trig_data:
+	kfree(trig_data);
+iio_hrtimer_mutex_unlock:
+	mutex_unlock(&iio_hrtimer_trigger_list_mut);
+	return err;
+}
+
+static int iio_hrtimer_trigger_remove(int id)
+{
+	bool foundit = false;
+	struct iio_hrtimer_trigger_data *trig_data;
+
+	mutex_lock(&iio_hrtimer_trigger_list_mut);
+	list_for_each_entry(trig_data, &iio_hrtimer_trigger_list, l) {
+		if (id == trig_data->id) {
+			foundit = true;
+			break;
+		}
+	}
+	if (!foundit) {
+		mutex_unlock(&iio_hrtimer_trigger_list_mut);
+		return -EINVAL;
+	}
+
+	iio_trigger_unregister(trig_data->trig);
+	iio_trigger_free(trig_data->trig);
+
+	list_del(&trig_data->l);
+	kfree(trig_data);
+	module_put(THIS_MODULE);
+	mutex_unlock(&iio_hrtimer_trigger_list_mut);
+
+	return 0;
+}
+
+static int __init iio_hrtimer_trig_init(void)
+{
+	device_initialize(&iio_hrtimer_trig_dev);
+	dev_set_name(&iio_hrtimer_trig_dev, "iio_hrtimer_trigger");
+	return device_add(&iio_hrtimer_trig_dev);
+}
+module_init(iio_hrtimer_trig_init);
+
+static void __exit iio_hrtimer_trig_exit(void)
+{
+	device_unregister(&iio_hrtimer_trig_dev);
+}
+module_exit(iio_hrtimer_trig_exit);
+
+MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
+MODULE_DESCRIPTION("Hrtimer trigger for the iio subsystem");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: IIO hrtimer trigger

[Lars-Peter Clausen]

On 09/29/2013 09:36 PM, Denis CIOCCA wrote:
> Hi Lars,
> 
> Thanks for your review.
> I reviewed the code in accordance with your comments, for the other point
> can you explain me better please?
> You intend to use one driver to manage all triggers added by sysfs?

Not necessarily, but I think we should have some common code that manages
the software triggers. But what I'm most concerned about is the userspace
ABI, since once we have added it, we have to maintain it forever. So the big
question do we think that the current ABI implemented by that patch is good
enough. Some thoughts:

* Should it maybe be called timer instead of hrtimer.
* Do we only want to allow names which follow "hrtimer-%d" or do we want to
allow arbitrary names.
* Do we want to have a top-level folder for each sw trigger type
* Is sysfs actually the right place for this, or should it go into configfs?
  Quote from Documentation/filesystems/configs:
  "configfs is a ram-based filesystem that provides the converse of
   sysfs's functionality.  Where sysfs is a filesystem-based view of
   kernel objects, configfs is a filesystem-based manager of kernel
   objects, or config_items. [...] Unlike sysfs, the
   lifetime of the representation is completely driven by userspace.  The
   kernel modules backing the items must respond to this."

I think especially the last one deserves some though.

- Lars

Re: IIO hrtimer trigger

[Jonathan Cameron]

On 10/03/13 18:10, Lars-Peter Clausen wrote:
> On 09/29/2013 09:36 PM, Denis CIOCCA wrote:
>> Hi Lars,
>>
>> Thanks for your review.
>> I reviewed the code in accordance with your comments, for the other point
>> can you explain me better please?
>> You intend to use one driver to manage all triggers added by sysfs?
> 
> Not necessarily, but I think we should have some common code that manages
> the software triggers.
That is fair enough.

> But what I'm most concerned about is the userspace
> ABI, since once we have added it, we have to maintain it forever. So the big
> question do we think that the current ABI implemented by that patch is good
> enough.
We are pretty much stuck with that for the sysfs trigger already...

> Some thoughts:
> 
> * Should it maybe be called timer instead of hrtimer.
Agreed.
> * Do we only want to allow names which follow "hrtimer-%d" or do we want to
> allow arbitrary names.
Arbitary would be fine.
> * Do we want to have a top-level folder for each sw trigger type
I'm not that bothered about this we are hardly talking a huge number of such
folders.
> * Is sysfs actually the right place for this, or should it go into configfs?
>   Quote from Documentation/filesystems/configs:
>   "configfs is a ram-based filesystem that provides the converse of
>    sysfs's functionality.  Where sysfs is a filesystem-based view of
>    kernel objects, configfs is a filesystem-based manager of kernel
>    objects, or config_items. [...] Unlike sysfs, the
>    lifetime of the representation is completely driven by userspace.  The
>    kernel modules backing the items must respond to this."
Hmm. maybe, I'm not sure how cleanly this would work and it adds an additional
dependency for all these types of drivers.  I'll take the lazy option:
Go on Lars, put together a full proposal on the actual interface ;)

Another vague thought was the on demand creation of timer based triggers
that I think zio provides.  Basically if a non existent trigger is requested
the subsystem figures out what is requested and creates it.  Not terribly
nice to implement, and to my mind unnecessary and possibly confusing...

Jonathan
> 
> I think especially the last one deserves some though.
> 
> - Lars
> 
> 
> 

Re: IIO hrtimer trigger

[Lars-Peter Clausen]

On 10/06/2013 08:15 PM, Jonathan Cameron wrote:
> On 10/03/13 18:10, Lars-Peter Clausen wrote:
>> On 09/29/2013 09:36 PM, Denis CIOCCA wrote:
>>> Hi Lars,
>>>
>>> Thanks for your review.
>>> I reviewed the code in accordance with your comments, for the other point
>>> can you explain me better please?
>>> You intend to use one driver to manage all triggers added by sysfs?
>>
>> Not necessarily, but I think we should have some common code that manages
>> the software triggers.
> That is fair enough.
> 
>> But what I'm most concerned about is the userspace
>> ABI, since once we have added it, we have to maintain it forever. So the big
>> question do we think that the current ABI implemented by that patch is good
>> enough.
> We are pretty much stuck with that for the sysfs trigger already...

Unfortunately yes. I never liked its API and I still don't like it and we
have to live with it. But this doesn't mean we have to add more of the same.

> 
>> Some thoughts:
>>
>> * Should it maybe be called timer instead of hrtimer.
> Agreed.
>> * Do we only want to allow names which follow "hrtimer-%d" or do we want to
>> allow arbitrary names.
> Arbitary would be fine.
>> * Do we want to have a top-level folder for each sw trigger type
> I'm not that bothered about this we are hardly talking a huge number of such
> folders.
>> * Is sysfs actually the right place for this, or should it go into configfs?
>>   Quote from Documentation/filesystems/configs:
>>   "configfs is a ram-based filesystem that provides the converse of
>>    sysfs's functionality.  Where sysfs is a filesystem-based view of
>>    kernel objects, configfs is a filesystem-based manager of kernel
>>    objects, or config_items. [...] Unlike sysfs, the
>>    lifetime of the representation is completely driven by userspace.  The
>>    kernel modules backing the items must respond to this."
> Hmm. maybe, I'm not sure how cleanly this would work and it adds an additional
> dependency for all these types of drivers.  I'll take the lazy option:
> Go on Lars, put together a full proposal on the actual interface ;)

I'll do that but that might take a few weeks until I get to it.

> 
> Another vague thought was the on demand creation of timer based triggers
> that I think zio provides.  Basically if a non existent trigger is requested
> the subsystem figures out what is requested and creates it.  Not terribly
> nice to implement, and to my mind unnecessary and possibly confusing...
> 

I don't think that would work to nicely in our case.

> Jonathan
>>
>> I think especially the last one deserves some though.
>>
>> - Lars

Re: IIO hrtimer trigger

[Jonathan Cameron]

On 07/10/13 15:18, Lars-Peter Clausen wrote:
> On 10/06/2013 08:15 PM, Jonathan Cameron wrote:
>> On 10/03/13 18:10, Lars-Peter Clausen wrote:
>>> On 09/29/2013 09:36 PM, Denis CIOCCA wrote:
>>>> Hi Lars,
>>>>
>>>> Thanks for your review.
>>>> I reviewed the code in accordance with your comments, for the other point
>>>> can you explain me better please?
>>>> You intend to use one driver to manage all triggers added by sysfs?
>>>
>>> Not necessarily, but I think we should have some common code that manages
>>> the software triggers.
>> That is fair enough.
>>
>>> But what I'm most concerned about is the userspace
>>> ABI, since once we have added it, we have to maintain it forever. So the big
>>> question do we think that the current ABI implemented by that patch is good
>>> enough.
>> We are pretty much stuck with that for the sysfs trigger already...
>
> Unfortunately yes. I never liked its API and I still don't like it and we
> have to live with it. But this doesn't mean we have to add more of the same.
>
>>
>>> Some thoughts:
>>>
>>> * Should it maybe be called timer instead of hrtimer.
>> Agreed.
>>> * Do we only want to allow names which follow "hrtimer-%d" or do we want to
>>> allow arbitrary names.
>> Arbitary would be fine.
>>> * Do we want to have a top-level folder for each sw trigger type
>> I'm not that bothered about this we are hardly talking a huge number of such
>> folders.
>>> * Is sysfs actually the right place for this, or should it go into configfs?
>>>    Quote from Documentation/filesystems/configs:
>>>    "configfs is a ram-based filesystem that provides the converse of
>>>     sysfs's functionality.  Where sysfs is a filesystem-based view of
>>>     kernel objects, configfs is a filesystem-based manager of kernel
>>>     objects, or config_items. [...] Unlike sysfs, the
>>>     lifetime of the representation is completely driven by userspace.  The
>>>     kernel modules backing the items must respond to this."
>> Hmm. maybe, I'm not sure how cleanly this would work and it adds an additional
>> dependency for all these types of drivers.  I'll take the lazy option:
>> Go on Lars, put together a full proposal on the actual interface ;)
>
> I'll do that but that might take a few weeks until I get to it.
Bump.  Do we want to still wait for this, or should we just go ahead with the
hrtimer as is.  It may not be ideal, but it's useful and lets us kill off
some much worse options..
>
>>
>> Another vague thought was the on demand creation of timer based triggers
>> that I think zio provides.  Basically if a non existent trigger is requested
>> the subsystem figures out what is requested and creates it.  Not terribly
>> nice to implement, and to my mind unnecessary and possibly confusing...
>>
>
> I don't think that would work to nicely in our case.
>
>> Jonathan
>>>
>>> I think especially the last one deserves some though.
>>>
>>> - Lars

Re: IIO hrtimer trigger

[Lars-Peter Clausen]

On 06/18/2014 09:47 PM, Jonathan Cameron wrote:
> On 07/10/13 15:18, Lars-Peter Clausen wrote:
>> On 10/06/2013 08:15 PM, Jonathan Cameron wrote:
>>> On 10/03/13 18:10, Lars-Peter Clausen wrote:
>>>> On 09/29/2013 09:36 PM, Denis CIOCCA wrote:
>>>>> Hi Lars,
>>>>>
>>>>> Thanks for your review.
>>>>> I reviewed the code in accordance with your comments, for the other point
>>>>> can you explain me better please?
>>>>> You intend to use one driver to manage all triggers added by sysfs?
>>>>
>>>> Not necessarily, but I think we should have some common code that manages
>>>> the software triggers.
>>> That is fair enough.
>>>
>>>> But what I'm most concerned about is the userspace
>>>> ABI, since once we have added it, we have to maintain it forever. So the
>>>> big
>>>> question do we think that the current ABI implemented by that patch is good
>>>> enough.
>>> We are pretty much stuck with that for the sysfs trigger already...
>>
>> Unfortunately yes. I never liked its API and I still don't like it and we
>> have to live with it. But this doesn't mean we have to add more of the same.
>>
>>>
>>>> Some thoughts:
>>>>
>>>> * Should it maybe be called timer instead of hrtimer.
>>> Agreed.
>>>> * Do we only want to allow names which follow "hrtimer-%d" or do we want to
>>>> allow arbitrary names.
>>> Arbitary would be fine.
>>>> * Do we want to have a top-level folder for each sw trigger type
>>> I'm not that bothered about this we are hardly talking a huge number of such
>>> folders.
>>>> * Is sysfs actually the right place for this, or should it go into
>>>> configfs?
>>>>    Quote from Documentation/filesystems/configs:
>>>>    "configfs is a ram-based filesystem that provides the converse of
>>>>     sysfs's functionality.  Where sysfs is a filesystem-based view of
>>>>     kernel objects, configfs is a filesystem-based manager of kernel
>>>>     objects, or config_items. [...] Unlike sysfs, the
>>>>     lifetime of the representation is completely driven by userspace.  The
>>>>     kernel modules backing the items must respond to this."
>>> Hmm. maybe, I'm not sure how cleanly this would work and it adds an
>>> additional
>>> dependency for all these types of drivers.  I'll take the lazy option:
>>> Go on Lars, put together a full proposal on the actual interface ;)
>>
>> I'll do that but that might take a few weeks until I get to it.
> Bump.  Do we want to still wait for this, or should we just go ahead with the
> hrtimer as is.  It may not be ideal, but it's useful and lets us kill off
> some much worse options..

I'd really prefer to not compromise on user-space ABI. Quick hacks are 
sometimes fine for kernel internal things since we can clean them up later. 
But for userspace ABI we'll be stuck with it forever.

Re: IIO hrtimer trigger

[Jonathan Cameron]

On June 19, 2014 11:57:50 AM GMT+01:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 06/18/2014 09:47 PM, Jonathan Cameron wrote:
>> On 07/10/13 15:18, Lars-Peter Clausen wrote:
>>> On 10/06/2013 08:15 PM, Jonathan Cameron wrote:
>>>> On 10/03/13 18:10, Lars-Peter Clausen wrote:
>>>>> On 09/29/2013 09:36 PM, Denis CIOCCA wrote:
>>>>>> Hi Lars,
>>>>>>
>>>>>> Thanks for your review.
>>>>>> I reviewed the code in accordance with your comments, for the
>other point
>>>>>> can you explain me better please?
>>>>>> You intend to use one driver to manage all triggers added by
>sysfs?
>>>>>
>>>>> Not necessarily, but I think we should have some common code that
>manages
>>>>> the software triggers.
>>>> That is fair enough.
>>>>
>>>>> But what I'm most concerned about is the userspace
>>>>> ABI, since once we have added it, we have to maintain it forever.
>So the
>>>>> big
>>>>> question do we think that the current ABI implemented by that
>patch is good
>>>>> enough.
>>>> We are pretty much stuck with that for the sysfs trigger already...
>>>
>>> Unfortunately yes. I never liked its API and I still don't like it
>and we
>>> have to live with it. But this doesn't mean we have to add more of
>the same.
>>>
>>>>
>>>>> Some thoughts:
>>>>>
>>>>> * Should it maybe be called timer instead of hrtimer.
>>>> Agreed.
>>>>> * Do we only want to allow names which follow "hrtimer-%d" or do
>we want to
>>>>> allow arbitrary names.
>>>> Arbitary would be fine.
>>>>> * Do we want to have a top-level folder for each sw trigger type
>>>> I'm not that bothered about this we are hardly talking a huge
>number of such
>>>> folders.
>>>>> * Is sysfs actually the right place for this, or should it go into
>>>>> configfs?
>>>>>    Quote from Documentation/filesystems/configs:
>>>>>    "configfs is a ram-based filesystem that provides the converse
>of
>>>>>     sysfs's functionality.  Where sysfs is a filesystem-based view
>of
>>>>>     kernel objects, configfs is a filesystem-based manager of
>kernel
>>>>>     objects, or config_items. [...] Unlike sysfs, the
>>>>>     lifetime of the representation is completely driven by
>userspace.  The
>>>>>     kernel modules backing the items must respond to this."
>>>> Hmm. maybe, I'm not sure how cleanly this would work and it adds an
>>>> additional
>>>> dependency for all these types of drivers.  I'll take the lazy
>option:
>>>> Go on Lars, put together a full proposal on the actual interface ;)
>>>
>>> I'll do that but that might take a few weeks until I get to it.
>> Bump.  Do we want to still wait for this, or should we just go ahead
>with the
>> hrtimer as is.  It may not be ideal, but it's useful and lets us kill
>off
>> some much worse options..
>
>I'd really prefer to not compromise on user-space ABI. Quick hacks are 
>sometimes fine for kernel internal things since we can clean them up
>later. 
>But for userspace ABI we'll be stuck with it forever.
I am slightly less bothered by this than normal because we obliged to keep the sysfs
 trigger interface around indefinitely anyway and this would be much the same.

 I am yet to be convinced that dragging in configfs makes sense...

Still, other purpose of restarting this thread was to hope to move it forward!

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

Re: IIO hrtimer trigger

[Ezequiel Garcia]

Hello Lars,

I'm picking this discussion, since we're also interested in using an hrtimer
trigger.

> On 10/06/2013 08:15 PM, Jonathan Cameron wrote:
>> We are pretty much stuck with that for the sysfs trigger already...
>
> Unfortunately yes. I never liked its API and I still don't like it and we
> have to live with it. But this doesn't mean we have to add more of the same.

So the reason why the hrtimer trigger hasn't been merged in its current form is
because we're still discussing the ABI, right?

Can you elaborate on why you don't like the sysfs trigger API? If you can give
me some hints I can try to cook a patch for the configfs hrtimer trigger.

Thanks!
Ezequiel

Re: IIO hrtimer trigger

[Lars-Peter Clausen]

On 09/05/2014 08:22 PM, Ezequiel Garcia wrote:
> Hello Lars,
>
> I'm picking this discussion, since we're also interested in using an hrtimer
> trigger.
>
>> On 10/06/2013 08:15 PM, Jonathan Cameron wrote:
>>> We are pretty much stuck with that for the sysfs trigger already...
>>
>> Unfortunately yes. I never liked its API and I still don't like it and we
>> have to live with it. But this doesn't mean we have to add more of the same.
>
> So the reason why the hrtimer trigger hasn't been merged in its current form is
> because we're still discussing the ABI, right?

I think the configfs documentation explains this quite well:

"Where sysfs is a filesystem-based view of kernel objects, configfs is a 
filesystem-based manager of kernel objects, or config_items." [1]

>
> Can you elaborate on why you don't like the sysfs trigger API? If you can give
> me some hints I can try to cook a patch for the configfs hrtimer trigger.

That would be great. I unfortunately only have a very foggy view of how the 
ABI and API should look like.

But I think the basic interface is have a toplevel iio configfs folder, 
maybe a subfolder called "triggers" and than a subfolder for each software 
trigger. E.g. "timer" (We shouldn't call it hrtimer cause that is just a 
implementation detail). Then in those trigger folders you can do mkdir to 
create a new instance of that trigger. In the folder there might be trigger 
specific config settings exposed as files.

- Lars

[1] https://www.kernel.org/doc/Documentation/filesystems/configfs/configfs.txt