Currently the user space applications write the trigger name in the
current_trigger file. The user space application should know what trigger to
use. The association can be manually configured or can be "detected" based
on the trigger's name if the triggers name has the device's index in the name
or any other custom and unstandard convention.
This issue is being fixed by these patches. They create a symlink in the
device's trigger folder for all triggers registered by the device.
/ # ls -l /sys/bus/iio/devices/iio:device0/trigger/
total 0
-rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
er0
This way the user space application can know what triggers were registered by
the device.
The 1st patch adds the main functionality (creating symlink if the trigger was
registered before the device was registered).
The 2nd patch improves the functionality allowing to register a trigger after
the IIO device was registered and the symlink is being created.
In the final patch there is an example on how to use this new API.
Robert Dolca (3):
iio: Add symlink to triggers in the devoce's trigger folder
iio: Improve iio_trigger_register_with_dev to register trigger after
device
iio: Use with iio_trigger_register_with_dev to register trigger
drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +-
drivers/iio/industrialio-core.c | 24 ++++++++
drivers/iio/industrialio-trigger.c | 70 ++++++++++++++++++++++
include/linux/iio/iio.h | 2 +
include/linux/iio/trigger.h | 24 ++++++++
5 files changed, 121 insertions(+), 1 deletion(-)
--
1.9.1
This patch adds a new function called iio_trigger_register_with_dev
which is a wrapper for iio_trigger_register. Besides the iio_trigger
struct this function requires iio_dev struct. It adds the trigger in
the device's trigger list and saves a reference to the device in the
trigger's struct.
When the device is registered, in the trigger folder of the device
(where current_trigger file resides) a symlink is being created for
each trigger that was registered width iio_trigger_register_with_dev.
# ls -l /sys/bus/iio/devices/iio:device0/trigger/
total 0
-rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
er0
This should be used for device specific triggers. Doing this the user space
applications can figure out what if the trigger registered by a specific device
and what should they write in the current_trigger file. Currently some
applications rely on the trigger name and this does not always work.
This implementation assumes that the trigger is registered before the device is
registered. If the order is not this the symlink will not be created but
everything else will work as before.
Signed-off-by: Robert Dolca <[email protected]>
---
drivers/iio/industrialio-core.c | 20 ++++++++++++
drivers/iio/industrialio-trigger.c | 64 ++++++++++++++++++++++++++++++++++++++
include/linux/iio/iio.h | 1 +
include/linux/iio/trigger.h | 24 ++++++++++++++
4 files changed, 109 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 69feb91..94bcd54 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -26,6 +26,7 @@
#include <linux/anon_inodes.h>
#include <linux/debugfs.h>
#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
#include "iio_core.h"
#include "iio_core_trigger.h"
#include <linux/iio/sysfs.h>
@@ -988,6 +989,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
}
dev_set_name(&dev->dev, "iio:device%d", dev->id);
INIT_LIST_HEAD(&dev->buffer_list);
+ INIT_LIST_HEAD(&dev->triggers);
}
return dev;
@@ -1166,6 +1168,8 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops;
int iio_device_register(struct iio_dev *indio_dev)
{
int ret;
+ struct iio_trigger *trig_info;
+ struct list_head *pos;
/* If the calling driver did not initialize of_node, do it here */
if (!indio_dev->dev.of_node && indio_dev->dev.parent)
@@ -1222,6 +1226,13 @@ int iio_device_register(struct iio_dev *indio_dev)
if (ret < 0)
goto error_cdev_del;
+ list_for_each(pos, &indio_dev->triggers) {
+ trig_info = list_entry(pos, struct iio_trigger, indio_dev_list);
+ ret = iio_trigger_link(trig_info);
+ if (ret < 0)
+ goto error_cdev_del;
+ }
+
return 0;
error_cdev_del:
cdev_del(&indio_dev->chrdev);
@@ -1243,8 +1254,17 @@ EXPORT_SYMBOL(iio_device_register);
**/
void iio_device_unregister(struct iio_dev *indio_dev)
{
+ struct list_head *pos, *safe;
+ struct iio_trigger *trig_info;
+
mutex_lock(&indio_dev->info_exist_lock);
+ list_for_each_safe(pos, safe, &indio_dev->triggers) {
+ trig_info = list_entry(pos, struct iio_trigger, indio_dev_list);
+ list_del(pos);
+ iio_trigger_unlink(trig_info);
+ }
+
device_del(&indio_dev->dev);
if (indio_dev->chrdev.dev)
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index d31098e..cebdd10 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -57,6 +57,24 @@ static struct attribute *iio_trig_dev_attrs[] = {
};
ATTRIBUTE_GROUPS(iio_trig_dev);
+int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
+ struct iio_trigger *trig_info)
+{
+ int ret;
+
+ ret = iio_trigger_register(trig_info);
+ if (ret < 0)
+ goto error;
+
+ trig_info->indio_dev = indio_dev;
+ list_add(&trig_info->indio_dev_list, &indio_dev->triggers);
+
+ return 0;
+error:
+ return ret;
+}
+EXPORT_SYMBOL(iio_trigger_register_with_dev);
+
int iio_trigger_register(struct iio_trigger *trig_info)
{
int ret;
@@ -88,6 +106,11 @@ EXPORT_SYMBOL(iio_trigger_register);
void iio_trigger_unregister(struct iio_trigger *trig_info)
{
+ if (trig_info->indio_dev) {
+ iio_trigger_unlink(trig_info);
+ list_del(&trig_info->indio_dev_list);
+ }
+
mutex_lock(&iio_trigger_list_lock);
list_del(&trig_info->list);
mutex_unlock(&iio_trigger_list_lock);
@@ -98,6 +121,47 @@ void iio_trigger_unregister(struct iio_trigger *trig_info)
}
EXPORT_SYMBOL(iio_trigger_unregister);
+int iio_trigger_link(struct iio_trigger *trig_info)
+{
+ int ret;
+ char *name;
+ struct iio_dev *indio_dev;
+
+ indio_dev = trig_info->indio_dev;
+ if (!indio_dev)
+ return -EINVAL;
+
+ name = kasprintf(GFP_KERNEL, "trigger%d", trig_info->id);
+ if (!name)
+ return -ENOMEM;
+
+ ret = sysfs_add_link_to_group(&indio_dev->dev.kobj, "trigger",
+ &trig_info->dev.kobj, name);
+ kfree(name);
+ return ret;
+}
+EXPORT_SYMBOL(iio_trigger_link);
+
+int iio_trigger_unlink(struct iio_trigger *trig_info)
+{
+ char *name;
+ struct iio_dev *indio_dev;
+
+ indio_dev = trig_info->indio_dev;
+ if (!trig_info->indio_dev)
+ return -EINVAL;
+
+ name = kasprintf(GFP_KERNEL, "trigger%d", trig_info->id);
+ if (!name)
+ return -ENOMEM;
+
+ sysfs_remove_link_from_group(&indio_dev->dev.kobj, "trigger", name);
+ kfree(name);
+ trig_info->indio_dev = NULL;
+ return 0;
+}
+EXPORT_SYMBOL(iio_trigger_unlink);
+
static struct iio_trigger *iio_trigger_find_by_name(const char *name,
size_t len)
{
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 878d861..9ff54ef 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -495,6 +495,7 @@ struct iio_dev {
struct dentry *debugfs_dentry;
unsigned cached_reg_addr;
#endif
+ struct list_head triggers;
};
const struct iio_chan_spec
diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index fa76c79..4001e44 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -70,6 +70,9 @@ struct iio_trigger {
struct iio_subirq subirqs[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)];
struct mutex pool_lock;
+
+ struct iio_dev *indio_dev;
+ struct list_head indio_dev_list;
};
@@ -117,6 +120,15 @@ static inline void *iio_trigger_get_drvdata(struct iio_trigger *trig)
}
/**
+ * iio_trigger_register_width_dev() - register a trigger with the IIO core
+ * and associate it with a device
+ * @iio_dev: device to associate the trigger with
+ * @trig_info: trigger to be registered
+ **/
+int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
+ struct iio_trigger *trig_info);
+
+/**
* iio_trigger_register() - register a trigger with the IIO core
* @trig_info: trigger to be registered
**/
@@ -129,6 +141,18 @@ int iio_trigger_register(struct iio_trigger *trig_info);
void iio_trigger_unregister(struct iio_trigger *trig_info);
/**
+ * Add a symlink to the trigger in the devices' trigger folder
+ * @trig_info: symlink destination trigger
+ */
+int iio_trigger_link(struct iio_trigger *trig_info);
+
+/**
+ * Remove the trigger's symlink from the device's trigger folder
+ * @trig_info: symlink destination trigger
+ */
+int iio_trigger_unlink(struct iio_trigger *trig_info);
+
+/**
* iio_trigger_poll() - called on a trigger occurring
* @trig: trigger which occurred
*
--
1.9.1
Until now calling iio_trigger_register_with_dev after registering the IIO device
added the trigger to the device's trigger list and saved a reference to the
device in the trigger's struct but it did not create the symlink.
In order to know if the device was registered or not this patch adds a flag
in the dev_iio struct and checks it when iio_trigger_register_with_dev is
called.
Signed-off-by: Robert Dolca <[email protected]>
---
drivers/iio/industrialio-core.c | 4 ++++
drivers/iio/industrialio-trigger.c | 6 ++++++
include/linux/iio/iio.h | 1 +
3 files changed, 11 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 94bcd54..04862a4 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1233,6 +1233,8 @@ int iio_device_register(struct iio_dev *indio_dev)
goto error_cdev_del;
}
+ indio_dev->registered = true;
+
return 0;
error_cdev_del:
cdev_del(&indio_dev->chrdev);
@@ -1281,6 +1283,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
mutex_unlock(&indio_dev->info_exist_lock);
iio_buffer_free_sysfs_and_mask(indio_dev);
+
+ indio_dev->registered = false;
}
EXPORT_SYMBOL(iio_device_unregister);
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index cebdd10..d4118fe 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -69,6 +69,12 @@ int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
trig_info->indio_dev = indio_dev;
list_add(&trig_info->indio_dev_list, &indio_dev->triggers);
+ if (indio_dev->registered) {
+ ret = iio_trigger_link(trig_info);
+ if (ret < 0)
+ goto error;
+ }
+
return 0;
error:
return ret;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 9ff54ef..3f704ae 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -496,6 +496,7 @@ struct iio_dev {
unsigned cached_reg_addr;
#endif
struct list_head triggers;
+ bool registered;
};
const struct iio_chan_spec
--
1.9.1
The trigger registered by the driver has the main purpose to be used
with this driver so it should be linked to the IIO device. This
way the user space applications can find the connection between them.
Signed-off-by: Robert Dolca <[email protected]>
---
drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 8d8ca6f..6681a7d 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -44,7 +44,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
sdata->trig->ops = trigger_ops;
sdata->trig->dev.parent = sdata->dev;
- err = iio_trigger_register(sdata->trig);
+ err = iio_trigger_register_with_dev(indio_dev, sdata->trig);
if (err < 0) {
dev_err(&indio_dev->dev, "failed to register iio trigger.\n");
goto iio_trigger_register_error;
--
1.9.1
On 16/04/15 05:01, Robert Dolca wrote:
> This patch adds a new function called iio_trigger_register_with_dev
> which is a wrapper for iio_trigger_register. Besides the iio_trigger
> struct this function requires iio_dev struct. It adds the trigger in
> the device's trigger list and saves a reference to the device in the
> trigger's struct.
>
> When the device is registered, in the trigger folder of the device
> (where current_trigger file resides) a symlink is being created for
> each trigger that was registered width iio_trigger_register_with_dev.
>
> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
> total 0
> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
> er0
>
> This should be used for device specific triggers. Doing this the user space
> applications can figure out what if the trigger registered by a specific device
> and what should they write in the current_trigger file. Currently some
> applications rely on the trigger name and this does not always work.
>
> This implementation assumes that the trigger is registered before the device is
> registered. If the order is not this the symlink will not be created but
> everything else will work as before.
>
> Signed-off-by: Robert Dolca <[email protected]>
I was rather hoping we'd get a few more comments on this.
In principle I like the idea, but it's new ABI and does make life
a tiny bit more complex, so what do people think?
Few trivial code comments inline.
> ---
> drivers/iio/industrialio-core.c | 20 ++++++++++++
> drivers/iio/industrialio-trigger.c | 64 ++++++++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 1 +
> include/linux/iio/trigger.h | 24 ++++++++++++++
> 4 files changed, 109 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 69feb91..94bcd54 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -26,6 +26,7 @@
> #include <linux/anon_inodes.h>
> #include <linux/debugfs.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> #include "iio_core.h"
> #include "iio_core_trigger.h"
> #include <linux/iio/sysfs.h>
> @@ -988,6 +989,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> }
> dev_set_name(&dev->dev, "iio:device%d", dev->id);
> INIT_LIST_HEAD(&dev->buffer_list);
> + INIT_LIST_HEAD(&dev->triggers);
> }
>
> return dev;
> @@ -1166,6 +1168,8 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops;
> int iio_device_register(struct iio_dev *indio_dev)
> {
> int ret;
> + struct iio_trigger *trig_info;
> + struct list_head *pos;
>
> /* If the calling driver did not initialize of_node, do it here */
> if (!indio_dev->dev.of_node && indio_dev->dev.parent)
> @@ -1222,6 +1226,13 @@ int iio_device_register(struct iio_dev *indio_dev)
> if (ret < 0)
> goto error_cdev_del;
>
> + list_for_each(pos, &indio_dev->triggers) {
> + trig_info = list_entry(pos, struct iio_trigger, indio_dev_list);
> + ret = iio_trigger_link(trig_info);
> + if (ret < 0)
> + goto error_cdev_del;
> + }
> +
> return 0;
> error_cdev_del:
> cdev_del(&indio_dev->chrdev);
> @@ -1243,8 +1254,17 @@ EXPORT_SYMBOL(iio_device_register);
> **/
> void iio_device_unregister(struct iio_dev *indio_dev)
> {
> + struct list_head *pos, *safe;
> + struct iio_trigger *trig_info;
> +
> mutex_lock(&indio_dev->info_exist_lock);
>
> + list_for_each_safe(pos, safe, &indio_dev->triggers) {
> + trig_info = list_entry(pos, struct iio_trigger, indio_dev_list);
> + list_del(pos);
> + iio_trigger_unlink(trig_info);
> + }
> +
> device_del(&indio_dev->dev);
>
> if (indio_dev->chrdev.dev)
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index d31098e..cebdd10 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -57,6 +57,24 @@ static struct attribute *iio_trig_dev_attrs[] = {
> };
> ATTRIBUTE_GROUPS(iio_trig_dev);
>
> +int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
> + struct iio_trigger *trig_info)
> +{
> + int ret;
> +
> + ret = iio_trigger_register(trig_info);
> + if (ret < 0)
> + goto error;
> +
> + trig_info->indio_dev = indio_dev;
> + list_add(&trig_info->indio_dev_list, &indio_dev->triggers);
> +
> + return 0;
> +error:
> + return ret;
> +}
> +EXPORT_SYMBOL(iio_trigger_register_with_dev);
> +
> int iio_trigger_register(struct iio_trigger *trig_info)
> {
> int ret;
> @@ -88,6 +106,11 @@ EXPORT_SYMBOL(iio_trigger_register);
>
> void iio_trigger_unregister(struct iio_trigger *trig_info)
> {
> + if (trig_info->indio_dev) {
> + iio_trigger_unlink(trig_info);
> + list_del(&trig_info->indio_dev_list);
> + }
> +
> mutex_lock(&iio_trigger_list_lock);
> list_del(&trig_info->list);
> mutex_unlock(&iio_trigger_list_lock);
> @@ -98,6 +121,47 @@ void iio_trigger_unregister(struct iio_trigger *trig_info)
> }
> EXPORT_SYMBOL(iio_trigger_unregister);
>
> +int iio_trigger_link(struct iio_trigger *trig_info)
> +{
> + int ret;
> + char *name;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = trig_info->indio_dev;
> + if (!indio_dev)
> + return -EINVAL;
> +
> + name = kasprintf(GFP_KERNEL, "trigger%d", trig_info->id);
> + if (!name)
> + return -ENOMEM;
> +
> + ret = sysfs_add_link_to_group(&indio_dev->dev.kobj, "trigger",
> + &trig_info->dev.kobj, name);
> + kfree(name);
blank line here.
> + return ret;
> +}
> +EXPORT_SYMBOL(iio_trigger_link);
> +
> +int iio_trigger_unlink(struct iio_trigger *trig_info)
> +{
> + char *name;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = trig_info->indio_dev;
> + if (!trig_info->indio_dev)
> + return -EINVAL;
> +
> + name = kasprintf(GFP_KERNEL, "trigger%d", trig_info->id);
> + if (!name)
> + return -ENOMEM;
> +
> + sysfs_remove_link_from_group(&indio_dev->dev.kobj, "trigger", name);
> + kfree(name);
> + trig_info->indio_dev = NULL;
and another blank line here.
> + return 0;
> +}
> +EXPORT_SYMBOL(iio_trigger_unlink);
> +
> static struct iio_trigger *iio_trigger_find_by_name(const char *name,
> size_t len)
> {
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 878d861..9ff54ef 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -495,6 +495,7 @@ struct iio_dev {
> struct dentry *debugfs_dentry;
> unsigned cached_reg_addr;
> #endif
> + struct list_head triggers;
> };
>
> const struct iio_chan_spec
> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
> index fa76c79..4001e44 100644
> --- a/include/linux/iio/trigger.h
> +++ b/include/linux/iio/trigger.h
> @@ -70,6 +70,9 @@ struct iio_trigger {
> struct iio_subirq subirqs[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
> unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)];
> struct mutex pool_lock;
> +
> + struct iio_dev *indio_dev;
> + struct list_head indio_dev_list;
> };
>
>
> @@ -117,6 +120,15 @@ static inline void *iio_trigger_get_drvdata(struct iio_trigger *trig)
> }
>
> /**
> + * iio_trigger_register_width_dev() - register a trigger with the IIO core
> + * and associate it with a device
> + * @iio_dev: device to associate the trigger with
> + * @trig_info: trigger to be registered
> + **/
> +int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
> + struct iio_trigger *trig_info);
> +
> +/**
> * iio_trigger_register() - register a trigger with the IIO core
> * @trig_info: trigger to be registered
> **/
> @@ -129,6 +141,18 @@ int iio_trigger_register(struct iio_trigger *trig_info);
> void iio_trigger_unregister(struct iio_trigger *trig_info);
>
> /**
> + * Add a symlink to the trigger in the devices' trigger folder
> + * @trig_info: symlink destination trigger
> + */
> +int iio_trigger_link(struct iio_trigger *trig_info);
> +
> +/**
> + * Remove the trigger's symlink from the device's trigger folder
> + * @trig_info: symlink destination trigger
> + */
> +int iio_trigger_unlink(struct iio_trigger *trig_info);
> +
> +/**
> * iio_trigger_poll() - called on a trigger occurring
> * @trig: trigger which occurred
> *
>
On Thu, Apr 16, 2015 at 12:01 PM, Robert Dolca <[email protected]> wrote:
> This patch adds a new function called iio_trigger_register_with_dev
> which is a wrapper for iio_trigger_register. Besides the iio_trigger
> struct this function requires iio_dev struct. It adds the trigger in
> the device's trigger list and saves a reference to the device in the
> trigger's struct.
>
> When the device is registered, in the trigger folder of the device
> (where current_trigger file resides) a symlink is being created for
> each trigger that was registered width iio_trigger_register_with_dev.
s/width/with
>
> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
> total 0
> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
> er0
>
> This should be used for device specific triggers. Doing this the user space
> applications can figure out what if the trigger registered by a specific device
s/what if/what is
> and what should they write in the current_trigger file. Currently some
> applications rely on the trigger name and this does not always work.
>
> This implementation assumes that the trigger is registered before the device is
> registered. If the order is not this the symlink will not be created but
> everything else will work as before.
This patch is very useful. Also I think that the userspace ABI is good. We need
to work on the in-kernel API because I think we can do better.
We should create a separate function that "attaches" a trigger with
a device.
So, the sequence at probe will be:
indio = iio_device_alloc();
trig = iio_trigger_alloc();
iio_trigger_attach(indio, trig);
iio_trigger_register();
iio_device_register();
The sequence at remove will be in reverse order, where we
would introduce iio_trigger_detach().
We might want document the ABI here:
http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio
>
> Signed-off-by: Robert Dolca <[email protected]>
> ---
> drivers/iio/industrialio-core.c | 20 ++++++++++++
> drivers/iio/industrialio-trigger.c | 64 ++++++++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 1 +
> include/linux/iio/trigger.h | 24 ++++++++++++++
> 4 files changed, 109 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 69feb91..94bcd54 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -26,6 +26,7 @@
> #include <linux/anon_inodes.h>
> #include <linux/debugfs.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> #include "iio_core.h"
> #include "iio_core_trigger.h"
> #include <linux/iio/sysfs.h>
> @@ -988,6 +989,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> }
> dev_set_name(&dev->dev, "iio:device%d", dev->id);
> INIT_LIST_HEAD(&dev->buffer_list);
> + INIT_LIST_HEAD(&dev->triggers);
> }
>
> return dev;
> @@ -1166,6 +1168,8 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops;
> int iio_device_register(struct iio_dev *indio_dev)
> {
> int ret;
> + struct iio_trigger *trig_info;
> + struct list_head *pos;
>
> /* If the calling driver did not initialize of_node, do it here */
> if (!indio_dev->dev.of_node && indio_dev->dev.parent)
> @@ -1222,6 +1226,13 @@ int iio_device_register(struct iio_dev *indio_dev)
> if (ret < 0)
> goto error_cdev_del;
>
> + list_for_each(pos, &indio_dev->triggers) {
> + trig_info = list_entry(pos, struct iio_trigger, indio_dev_list);
> + ret = iio_trigger_link(trig_info);
> + if (ret < 0)
> + goto error_cdev_del;
> + }
> +
> return 0;
> error_cdev_del:
> cdev_del(&indio_dev->chrdev);
> @@ -1243,8 +1254,17 @@ EXPORT_SYMBOL(iio_device_register);
> **/
> void iio_device_unregister(struct iio_dev *indio_dev)
> {
> + struct list_head *pos, *safe;
> + struct iio_trigger *trig_info;
> +
> mutex_lock(&indio_dev->info_exist_lock);
>
> + list_for_each_safe(pos, safe, &indio_dev->triggers) {
> + trig_info = list_entry(pos, struct iio_trigger, indio_dev_list);
> + list_del(pos);
> + iio_trigger_unlink(trig_info);
> + }
> +
> device_del(&indio_dev->dev);
>
> if (indio_dev->chrdev.dev)
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index d31098e..cebdd10 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -57,6 +57,24 @@ static struct attribute *iio_trig_dev_attrs[] = {
> };
> ATTRIBUTE_GROUPS(iio_trig_dev);
>
> +int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
> + struct iio_trigger *trig_info)
> +{
> + int ret;
> +
> + ret = iio_trigger_register(trig_info);
> + if (ret < 0)
> + goto error;
> +
> + trig_info->indio_dev = indio_dev;
> + list_add(&trig_info->indio_dev_list, &indio_dev->triggers);
> +
> + return 0;
> +error:
> + return ret;
> +}
> +EXPORT_SYMBOL(iio_trigger_register_with_dev);
> +
> int iio_trigger_register(struct iio_trigger *trig_info)
> {
> int ret;
> @@ -88,6 +106,11 @@ EXPORT_SYMBOL(iio_trigger_register);
>
> void iio_trigger_unregister(struct iio_trigger *trig_info)
> {
> + if (trig_info->indio_dev) {
> + iio_trigger_unlink(trig_info);
> + list_del(&trig_info->indio_dev_list);
> + }
> +
> mutex_lock(&iio_trigger_list_lock);
> list_del(&trig_info->list);
> mutex_unlock(&iio_trigger_list_lock);
> @@ -98,6 +121,47 @@ void iio_trigger_unregister(struct iio_trigger *trig_info)
> }
> EXPORT_SYMBOL(iio_trigger_unregister);
>
> +int iio_trigger_link(struct iio_trigger *trig_info)
> +{
> + int ret;
> + char *name;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = trig_info->indio_dev;
> + if (!indio_dev)
> + return -EINVAL;
> +
> + name = kasprintf(GFP_KERNEL, "trigger%d", trig_info->id);
> + if (!name)
> + return -ENOMEM;
> +
> + ret = sysfs_add_link_to_group(&indio_dev->dev.kobj, "trigger",
> + &trig_info->dev.kobj, name);
> + kfree(name);
> + return ret;
> +}
> +EXPORT_SYMBOL(iio_trigger_link);
> +
> +int iio_trigger_unlink(struct iio_trigger *trig_info)
> +{
> + char *name;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = trig_info->indio_dev;
> + if (!trig_info->indio_dev)
> + return -EINVAL;
> +
> + name = kasprintf(GFP_KERNEL, "trigger%d", trig_info->id);
> + if (!name)
> + return -ENOMEM;
> +
> + sysfs_remove_link_from_group(&indio_dev->dev.kobj, "trigger", name);
> + kfree(name);
> + trig_info->indio_dev = NULL;
> + return 0;
> +}
> +EXPORT_SYMBOL(iio_trigger_unlink);
> +
> static struct iio_trigger *iio_trigger_find_by_name(const char *name,
> size_t len)
> {
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 878d861..9ff54ef 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -495,6 +495,7 @@ struct iio_dev {
> struct dentry *debugfs_dentry;
> unsigned cached_reg_addr;
> #endif
> + struct list_head triggers;
Better name for this would be trigger_list (see buffer_list or chan_attr_list
in the same structure).
Also, please update the doc string above.
> };
>
> const struct iio_chan_spec
> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
> index fa76c79..4001e44 100644
> --- a/include/linux/iio/trigger.h
> +++ b/include/linux/iio/trigger.h
> @@ -70,6 +70,9 @@ struct iio_trigger {
> struct iio_subirq subirqs[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
> unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)];
> struct mutex pool_lock;
> +
> + struct iio_dev *indio_dev;
> + struct list_head indio_dev_list;
> };
The same observation about the doc string.
>
>
> @@ -117,6 +120,15 @@ static inline void *iio_trigger_get_drvdata(struct iio_trigger *trig)
> }
>
> /**
> + * iio_trigger_register_width_dev() - register a trigger with the IIO core
> + * and associate it with a device
> + * @iio_dev: device to associate the trigger with
> + * @trig_info: trigger to be registered
> + **/
> +int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
> + struct iio_trigger *trig_info);
> +
> +/**
> * iio_trigger_register() - register a trigger with the IIO core
> * @trig_info: trigger to be registered
> **/
> @@ -129,6 +141,18 @@ int iio_trigger_register(struct iio_trigger *trig_info);
> void iio_trigger_unregister(struct iio_trigger *trig_info);
>
> /**
> + * Add a symlink to the trigger in the devices' trigger folder
> + * @trig_info: symlink destination trigger
> + */
> +int iio_trigger_link(struct iio_trigger *trig_info);
> +
> +/**
> + * Remove the trigger's symlink from the device's trigger folder
> + * @trig_info: symlink destination trigger
> + */
> +int iio_trigger_unlink(struct iio_trigger *trig_info);
> +
> +/**
> * iio_trigger_poll() - called on a trigger occurring
> * @trig: trigger which occurred
> *
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 16, 2015 at 12:01 PM, Robert Dolca <[email protected]> wrote:
> Until now calling iio_trigger_register_with_dev after registering the IIO device
> added the trigger to the device's trigger list and saved a reference to the
> device in the trigger's struct but it did not create the symlink.
>
> In order to know if the device was registered or not this patch adds a flag
> in the dev_iio struct and checks it when iio_trigger_register_with_dev is
> called.
I don't think we should care about this. In order for the symlink to be created
the driver should call the right sequence of functions.
>
> Signed-off-by: Robert Dolca <[email protected]>
> ---
> drivers/iio/industrialio-core.c | 4 ++++
> drivers/iio/industrialio-trigger.c | 6 ++++++
> include/linux/iio/iio.h | 1 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 94bcd54..04862a4 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1233,6 +1233,8 @@ int iio_device_register(struct iio_dev *indio_dev)
> goto error_cdev_del;
> }
>
> + indio_dev->registered = true;
> +
> return 0;
> error_cdev_del:
> cdev_del(&indio_dev->chrdev);
> @@ -1281,6 +1283,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
> mutex_unlock(&indio_dev->info_exist_lock);
>
> iio_buffer_free_sysfs_and_mask(indio_dev);
> +
> + indio_dev->registered = false;
> }
> EXPORT_SYMBOL(iio_device_unregister);
>
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index cebdd10..d4118fe 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -69,6 +69,12 @@ int iio_trigger_register_with_dev(struct iio_dev *indio_dev,
> trig_info->indio_dev = indio_dev;
> list_add(&trig_info->indio_dev_list, &indio_dev->triggers);
>
> + if (indio_dev->registered) {
> + ret = iio_trigger_link(trig_info);
> + if (ret < 0)
> + goto error;
> + }
> +
> return 0;
> error:
> return ret;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 9ff54ef..3f704ae 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -496,6 +496,7 @@ struct iio_dev {
> unsigned cached_reg_addr;
> #endif
> struct list_head triggers;
> + bool registered;
> };
>
> const struct iio_chan_spec
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
> On 16/04/15 05:01, Robert Dolca wrote:
>> This patch adds a new function called iio_trigger_register_with_dev
>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
>> struct this function requires iio_dev struct. It adds the trigger in
>> the device's trigger list and saves a reference to the device in the
>> trigger's struct.
>>
>> When the device is registered, in the trigger folder of the device
>> (where current_trigger file resides) a symlink is being created for
>> each trigger that was registered width iio_trigger_register_with_dev.
>>
>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
>> total 0
>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
>> er0
>>
>> This should be used for device specific triggers. Doing this the user space
>> applications can figure out what if the trigger registered by a specific device
>> and what should they write in the current_trigger file. Currently some
>> applications rely on the trigger name and this does not always work.
>>
>> This implementation assumes that the trigger is registered before the device is
>> registered. If the order is not this the symlink will not be created but
>> everything else will work as before.
>>
>> Signed-off-by: Robert Dolca <[email protected]>
> I was rather hoping we'd get a few more comments on this.
> In principle I like the idea, but it's new ABI and does make life
> a tiny bit more complex, so what do people think?
>
> Few trivial code comments inline.
I don't think it adds more information. Both the trigger and the device get
registered for the same parent device, so you can already easily find the
trigger for a device by going to the parent device and taking a look at the
triggers registered by the parent device.
On 12/05/15 17:56, Lars-Peter Clausen wrote:
> On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
>> On 16/04/15 05:01, Robert Dolca wrote:
>>> This patch adds a new function called iio_trigger_register_with_dev
>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
>>> struct this function requires iio_dev struct. It adds the trigger in
>>> the device's trigger list and saves a reference to the device in the
>>> trigger's struct.
>>>
>>> When the device is registered, in the trigger folder of the device
>>> (where current_trigger file resides) a symlink is being created for
>>> each trigger that was registered width iio_trigger_register_with_dev.
>>>
>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
>>> total 0
>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
>>> er0
>>>
>>> This should be used for device specific triggers. Doing this the user space
>>> applications can figure out what if the trigger registered by a specific device
>>> and what should they write in the current_trigger file. Currently some
>>> applications rely on the trigger name and this does not always work.
>>>
>>> This implementation assumes that the trigger is registered before the device is
>>> registered. If the order is not this the symlink will not be created but
>>> everything else will work as before.
>>>
>>> Signed-off-by: Robert Dolca <[email protected]>
>> I was rather hoping we'd get a few more comments on this.
>> In principle I like the idea, but it's new ABI and does make life
>> a tiny bit more complex, so what do people think?
>>
>> Few trivial code comments inline.
>
> I don't think it adds more information. Both the trigger and the
> device get registered for the same parent device, so you can already
> easily find the trigger for a device by going to the parent device
> and taking a look at the triggers registered by the parent device.
I had the same thought. The question is whether the slightly gain in
simplicity for userspace is worth it... I'm undecided at the moment.
On 05/12/2015 09:06 PM, Jonathan Cameron wrote:
> On 12/05/15 17:56, Lars-Peter Clausen wrote:
>> On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
>>> On 16/04/15 05:01, Robert Dolca wrote:
>>>> This patch adds a new function called iio_trigger_register_with_dev
>>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
>>>> struct this function requires iio_dev struct. It adds the trigger in
>>>> the device's trigger list and saves a reference to the device in the
>>>> trigger's struct.
>>>>
>>>> When the device is registered, in the trigger folder of the device
>>>> (where current_trigger file resides) a symlink is being created for
>>>> each trigger that was registered width iio_trigger_register_with_dev.
>>>>
>>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
>>>> total 0
>>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
>>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
>>>> er0
>>>>
>>>> This should be used for device specific triggers. Doing this the user space
>>>> applications can figure out what if the trigger registered by a specific device
>>>> and what should they write in the current_trigger file. Currently some
>>>> applications rely on the trigger name and this does not always work.
>>>>
>>>> This implementation assumes that the trigger is registered before the device is
>>>> registered. If the order is not this the symlink will not be created but
>>>> everything else will work as before.
>>>>
>>>> Signed-off-by: Robert Dolca <[email protected]>
>>> I was rather hoping we'd get a few more comments on this.
>>> In principle I like the idea, but it's new ABI and does make life
>>> a tiny bit more complex, so what do people think?
>>>
>>> Few trivial code comments inline.
>>
>> I don't think it adds more information. Both the trigger and the
>> device get registered for the same parent device, so you can already
>> easily find the trigger for a device by going to the parent device
>> and taking a look at the triggers registered by the parent device.
> I had the same thought. The question is whether the slightly gain in
> simplicity for userspace is worth it... I'm undecided at the moment.
As you may have guessed by now I'm always quite conservative when it comes
to introducing new ABI. Simply because we have to maintain it forever, the
less stuff to maintain forever the better.
Hence I think all new ABI needs a compelling reason, e.g. like a improvement
in performance. And of course this patch slightly simplifies things, but in
my opinion not enough to justify a ABI extension. We can always find ways to
simplify the interface, but the metric for ABI should be whether the
simplification actually matters. In this case I don't think it does, finding
the trigger for a device is not really hot-path. The amount of time saved
will be disappear in the noise.
And in my opinion applications shouldn't directly use the low-level ABI but
rather use middle-ware libraries/frameworks, like e.g. libiio, and that's
where you'd hide the complexities of a operation.
- Lars
On 13/05/15 08:28, Lars-Peter Clausen wrote:
> On 05/12/2015 09:06 PM, Jonathan Cameron wrote:
>> On 12/05/15 17:56, Lars-Peter Clausen wrote:
>>> On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
>>>> On 16/04/15 05:01, Robert Dolca wrote:
>>>>> This patch adds a new function called iio_trigger_register_with_dev
>>>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
>>>>> struct this function requires iio_dev struct. It adds the trigger in
>>>>> the device's trigger list and saves a reference to the device in the
>>>>> trigger's struct.
>>>>>
>>>>> When the device is registered, in the trigger folder of the device
>>>>> (where current_trigger file resides) a symlink is being created for
>>>>> each trigger that was registered width iio_trigger_register_with_dev.
>>>>>
>>>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
>>>>> total 0
>>>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
>>>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
>>>>> er0
>>>>>
>>>>> This should be used for device specific triggers. Doing this the user space
>>>>> applications can figure out what if the trigger registered by a specific device
>>>>> and what should they write in the current_trigger file. Currently some
>>>>> applications rely on the trigger name and this does not always work.
>>>>>
>>>>> This implementation assumes that the trigger is registered before the device is
>>>>> registered. If the order is not this the symlink will not be created but
>>>>> everything else will work as before.
>>>>>
>>>>> Signed-off-by: Robert Dolca <[email protected]>
>>>> I was rather hoping we'd get a few more comments on this.
>>>> In principle I like the idea, but it's new ABI and does make life
>>>> a tiny bit more complex, so what do people think?
>>>>
>>>> Few trivial code comments inline.
>>>
>>> I don't think it adds more information. Both the trigger and the
>>> device get registered for the same parent device, so you can already
>>> easily find the trigger for a device by going to the parent device
>>> and taking a look at the triggers registered by the parent device.
>> I had the same thought. The question is whether the slightly gain in
>> simplicity for userspace is worth it... I'm undecided at the moment.
>
> As you may have guessed by now I'm always quite conservative when it
> comes to introducing new ABI. Simply because we have to maintain it
> forever, the less stuff to maintain forever the better.
>
> Hence I think all new ABI needs a compelling reason, e.g. like a
> improvement in performance. And of course this patch slightly
> simplifies things, but in my opinion not enough to justify a ABI
> extension. We can always find ways to simplify the interface, but the
> metric for ABI should be whether the simplification actually matters.
> In this case I don't think it does, finding the trigger for a device
> is not really hot-path. The amount of time saved will be disappear in
> the noise.
>
> And in my opinion applications shouldn't directly use the low-level
> ABI but rather use middle-ware libraries/frameworks, like e.g.
> libiio, and that's where you'd hide the complexities of a operation.
>
> - Lars
I'll go with Lars response on this one. Not worth the hassle.
That's the nature of an RFC of course!
Jonathan
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 8:42 PM, Jonathan Cameron <[email protected]> wrote:
>
> On 13/05/15 08:28, Lars-Peter Clausen wrote:
> > On 05/12/2015 09:06 PM, Jonathan Cameron wrote:
> >> On 12/05/15 17:56, Lars-Peter Clausen wrote:
> >>> On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
> >>>> On 16/04/15 05:01, Robert Dolca wrote:
> >>>>> This patch adds a new function called iio_trigger_register_with_dev
> >>>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
> >>>>> struct this function requires iio_dev struct. It adds the trigger in
> >>>>> the device's trigger list and saves a reference to the device in the
> >>>>> trigger's struct.
> >>>>>
> >>>>> When the device is registered, in the trigger folder of the device
> >>>>> (where current_trigger file resides) a symlink is being created for
> >>>>> each trigger that was registered width iio_trigger_register_with_dev.
> >>>>>
> >>>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
> >>>>> total 0
> >>>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
> >>>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
> >>>>> er0
> >>>>>
> >>>>> This should be used for device specific triggers. Doing this the user space
> >>>>> applications can figure out what if the trigger registered by a specific device
> >>>>> and what should they write in the current_trigger file. Currently some
> >>>>> applications rely on the trigger name and this does not always work.
> >>>>>
> >>>>> This implementation assumes that the trigger is registered before the device is
> >>>>> registered. If the order is not this the symlink will not be created but
> >>>>> everything else will work as before.
> >>>>>
> >>>>> Signed-off-by: Robert Dolca <[email protected]>
> >>>> I was rather hoping we'd get a few more comments on this.
> >>>> In principle I like the idea, but it's new ABI and does make life
> >>>> a tiny bit more complex, so what do people think?
> >>>>
> >>>> Few trivial code comments inline.
> >>>
> >>> I don't think it adds more information. Both the trigger and the
> >>> device get registered for the same parent device, so you can already
> >>> easily find the trigger for a device by going to the parent device
> >>> and taking a look at the triggers registered by the parent device.
> >> I had the same thought. The question is whether the slightly gain in
> >> simplicity for userspace is worth it... I'm undecided at the moment.
> >
> > As you may have guessed by now I'm always quite conservative when it
> > comes to introducing new ABI. Simply because we have to maintain it
> > forever, the less stuff to maintain forever the better.
> >
> > Hence I think all new ABI needs a compelling reason, e.g. like a
> > improvement in performance. And of course this patch slightly
> > simplifies things, but in my opinion not enough to justify a ABI
> > extension. We can always find ways to simplify the interface, but the
> > metric for ABI should be whether the simplification actually matters.
> > In this case I don't think it does, finding the trigger for a device
> > is not really hot-path. The amount of time saved will be disappear in
> > the noise.
> >
> > And in my opinion applications shouldn't directly use the low-level
> > ABI but rather use middle-ware libraries/frameworks, like e.g.
> > libiio, and that's where you'd hide the complexities of a operation.
> >
> > - Lars
> I'll go with Lars response on this one. Not worth the hassle.
> That's the nature of an RFC of course!
>
> Jonathan
Would it be acceptable to add the symlinks without adding a new API?
When a trigger is registered you could use the common parent to get a
pointer to the iio_dev and then create the symlink. This is a little
bit complicated but I think it can be done.
Robert
On 05/13/2015 08:03 PM, Robert Dolca wrote:
> On Wed, May 13, 2015 at 8:42 PM, Jonathan Cameron <[email protected]> wrote:
>>
>> On 13/05/15 08:28, Lars-Peter Clausen wrote:
>>> On 05/12/2015 09:06 PM, Jonathan Cameron wrote:
>>>> On 12/05/15 17:56, Lars-Peter Clausen wrote:
>>>>> On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
>>>>>> On 16/04/15 05:01, Robert Dolca wrote:
>>>>>>> This patch adds a new function called iio_trigger_register_with_dev
>>>>>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
>>>>>>> struct this function requires iio_dev struct. It adds the trigger in
>>>>>>> the device's trigger list and saves a reference to the device in the
>>>>>>> trigger's struct.
>>>>>>>
>>>>>>> When the device is registered, in the trigger folder of the device
>>>>>>> (where current_trigger file resides) a symlink is being created for
>>>>>>> each trigger that was registered width iio_trigger_register_with_dev.
>>>>>>>
>>>>>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
>>>>>>> total 0
>>>>>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 current_trigger
>>>>>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> ../../trigg
>>>>>>> er0
>>>>>>>
>>>>>>> This should be used for device specific triggers. Doing this the user space
>>>>>>> applications can figure out what if the trigger registered by a specific device
>>>>>>> and what should they write in the current_trigger file. Currently some
>>>>>>> applications rely on the trigger name and this does not always work.
>>>>>>>
>>>>>>> This implementation assumes that the trigger is registered before the device is
>>>>>>> registered. If the order is not this the symlink will not be created but
>>>>>>> everything else will work as before.
>>>>>>>
>>>>>>> Signed-off-by: Robert Dolca <[email protected]>
>>>>>> I was rather hoping we'd get a few more comments on this.
>>>>>> In principle I like the idea, but it's new ABI and does make life
>>>>>> a tiny bit more complex, so what do people think?
>>>>>>
>>>>>> Few trivial code comments inline.
>>>>>
>>>>> I don't think it adds more information. Both the trigger and the
>>>>> device get registered for the same parent device, so you can already
>>>>> easily find the trigger for a device by going to the parent device
>>>>> and taking a look at the triggers registered by the parent device.
>>>> I had the same thought. The question is whether the slightly gain in
>>>> simplicity for userspace is worth it... I'm undecided at the moment.
>>>
>>> As you may have guessed by now I'm always quite conservative when it
>>> comes to introducing new ABI. Simply because we have to maintain it
>>> forever, the less stuff to maintain forever the better.
>>>
>>> Hence I think all new ABI needs a compelling reason, e.g. like a
>>> improvement in performance. And of course this patch slightly
>>> simplifies things, but in my opinion not enough to justify a ABI
>>> extension. We can always find ways to simplify the interface, but the
>>> metric for ABI should be whether the simplification actually matters.
>>> In this case I don't think it does, finding the trigger for a device
>>> is not really hot-path. The amount of time saved will be disappear in
>>> the noise.
>>>
>>> And in my opinion applications shouldn't directly use the low-level
>>> ABI but rather use middle-ware libraries/frameworks, like e.g.
>>> libiio, and that's where you'd hide the complexities of a operation.
>>>
>>> - Lars
>> I'll go with Lars response on this one. Not worth the hassle.
>> That's the nature of an RFC of course!
>>
>> Jonathan
>
> Would it be acceptable to add the symlinks without adding a new API?
> When a trigger is registered you could use the common parent to get a
> pointer to the iio_dev and then create the symlink. This is a little
> bit complicated but I think it can be done.
The concerns are with the symlink and with he symlink only. Adding new API
inside the kernel is generally not as much of a problem as external ABI.
- Lars
On 05/13/2015 08:05 PM, Lars-Peter Clausen wrote:
> On 05/13/2015 08:03 PM, Robert Dolca wrote:
>> On Wed, May 13, 2015 at 8:42 PM, Jonathan Cameron <[email protected]> wrote:
>>>
>>> On 13/05/15 08:28, Lars-Peter Clausen wrote:
>>>> On 05/12/2015 09:06 PM, Jonathan Cameron wrote:
>>>>> On 12/05/15 17:56, Lars-Peter Clausen wrote:
>>>>>> On 05/08/2015 05:11 PM, Jonathan Cameron wrote:
>>>>>>> On 16/04/15 05:01, Robert Dolca wrote:
>>>>>>>> This patch adds a new function called iio_trigger_register_with_dev
>>>>>>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger
>>>>>>>> struct this function requires iio_dev struct. It adds the trigger in
>>>>>>>> the device's trigger list and saves a reference to the device in the
>>>>>>>> trigger's struct.
>>>>>>>>
>>>>>>>> When the device is registered, in the trigger folder of the device
>>>>>>>> (where current_trigger file resides) a symlink is being created for
>>>>>>>> each trigger that was registered width iio_trigger_register_with_dev.
>>>>>>>>
>>>>>>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/
>>>>>>>> total 0
>>>>>>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33
>>>>>>>> current_trigger
>>>>>>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 ->
>>>>>>>> ../../trigg
>>>>>>>> er0
>>>>>>>>
>>>>>>>> This should be used for device specific triggers. Doing this the
>>>>>>>> user space
>>>>>>>> applications can figure out what if the trigger registered by a
>>>>>>>> specific device
>>>>>>>> and what should they write in the current_trigger file. Currently some
>>>>>>>> applications rely on the trigger name and this does not always work.
>>>>>>>>
>>>>>>>> This implementation assumes that the trigger is registered before
>>>>>>>> the device is
>>>>>>>> registered. If the order is not this the symlink will not be created
>>>>>>>> but
>>>>>>>> everything else will work as before.
>>>>>>>>
>>>>>>>> Signed-off-by: Robert Dolca <[email protected]>
>>>>>>> I was rather hoping we'd get a few more comments on this.
>>>>>>> In principle I like the idea, but it's new ABI and does make life
>>>>>>> a tiny bit more complex, so what do people think?
>>>>>>>
>>>>>>> Few trivial code comments inline.
>>>>>>
>>>>>> I don't think it adds more information. Both the trigger and the
>>>>>> device get registered for the same parent device, so you can already
>>>>>> easily find the trigger for a device by going to the parent device
>>>>>> and taking a look at the triggers registered by the parent device.
>>>>> I had the same thought. The question is whether the slightly gain in
>>>>> simplicity for userspace is worth it... I'm undecided at the moment.
>>>>
>>>> As you may have guessed by now I'm always quite conservative when it
>>>> comes to introducing new ABI. Simply because we have to maintain it
>>>> forever, the less stuff to maintain forever the better.
>>>>
>>>> Hence I think all new ABI needs a compelling reason, e.g. like a
>>>> improvement in performance. And of course this patch slightly
>>>> simplifies things, but in my opinion not enough to justify a ABI
>>>> extension. We can always find ways to simplify the interface, but the
>>>> metric for ABI should be whether the simplification actually matters.
>>>> In this case I don't think it does, finding the trigger for a device
>>>> is not really hot-path. The amount of time saved will be disappear in
>>>> the noise.
>>>>
>>>> And in my opinion applications shouldn't directly use the low-level
>>>> ABI but rather use middle-ware libraries/frameworks, like e.g.
>>>> libiio, and that's where you'd hide the complexities of a operation.
>>>>
>>>> - Lars
>>> I'll go with Lars response on this one. Not worth the hassle.
>>> That's the nature of an RFC of course!
>>>
>>> Jonathan
>>
>> Would it be acceptable to add the symlinks without adding a new API?
>> When a trigger is registered you could use the common parent to get a
>> pointer to the iio_dev and then create the symlink. This is a little
>> bit complicated but I think it can be done.
>
> The concerns are with the symlink and with he symlink only. Adding new API
> inside the kernel is generally not as much of a problem as external ABI.
Sorry, I should have added you can easily find out which triggers and which
devices belong together by basically doing a dirname `readlink
/sys/bus/iio/devices/X`. Those that are at the same place in the global
hierarchy belong to the same device.