Add IIO trigger symlink in iio:device0/trigger/

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

drivers/iio/industrialio-core.c +20 -0

index 69feb91..94bcd54 100644
@@ -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)

drivers/iio/industrialio-trigger.c +30 -0

index d31098e..cebdd10 100644
@@ -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;

drivers/iio/industrialio-trigger.c +10 -0

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

drivers/iio/industrialio-trigger.c +4 -0

+	return 0;
+}
+EXPORT_SYMBOL(iio_trigger_unlink);
+
 static struct iio_trigger *iio_trigger_find_by_name(const char *name,
 						    size_t len)
 {

include/linux/iio/iio.h +1 -0

index 878d861..9ff54ef 100644
@@ -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

include/linux/iio/trigger.h +24 -0

index fa76c79..4001e44 100644
@@ -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
  *

Quoted 25 lines · Robert Dolca

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.

Quoted 1 line

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.

Quoted 31 lines · Jonathan Cameron

Quoted 21 lines · Robert Dolca

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.

Quoted 5 lines

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?

Quoted 1 line

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.

Quoted 32 lines · Lars-Peter Clausen

Quoted 31 lines · Jonathan Cameron

Quoted 21 lines · Robert Dolca

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.

Quoted 5 lines

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?

Quoted 6 lines

Quoted 1 line

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.

Quoted 33 lines · Jonathan Cameron

Quoted 32 lines · Lars-Peter Clausen

Quoted 31 lines · Jonathan Cameron

Quoted 21 lines · Robert Dolca

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.

Quoted 5 lines

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?

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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

Signature · 4 lines

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

Quoted 10 lines · Lars-Peter Clausen

Quoted 9 lines · Jonathan Cameron

Quoted 8 lines · Lars-Peter Clausen

Quoted 7 lines · Jonathan Cameron

Quoted 6 lines · Robert Dolca

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.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

   # ls -l /sys/bus/iio/devices/iio:device0/trigger/
total 0
-rw-r--r--    1 root     root          4096 Apr 16 08:33

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

er0

This should be used for device specific triggers. Doing this the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

applications can figure out what if the trigger registered by a

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and what should they write in the current_trigger file. Currently

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

applications rely on the trigger name and this does not always work.

This implementation assumes that the trigger is registered before

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

registered. If the order is not this the symlink will not be

Quoted 33 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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?

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 1 line

- Lars

I'll go with Lars response on this one. Not worth the hassle. That's the nature of an RFC of course!

Jonathan

Quoted 60 lines · Lars-Peter Clausen

Quoted 33 lines · Jonathan Cameron

Quoted 32 lines · Lars-Peter Clausen

Quoted 31 lines · Jonathan Cameron

Quoted 21 lines · Robert Dolca

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.

Quoted 5 lines

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?

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 1 line

- Lars

I'll go with Lars response on this one. Not worth the hassle. That's the nature of an RFC of course!

Jonathan

Quoted 66 lines · Jonathan Cameron

Quoted 56 lines · Lars-Peter Clausen

Quoted 33 lines · Jonathan Cameron

Quoted 32 lines · Lars-Peter Clausen

Quoted 31 lines · Jonathan Cameron

Quoted 21 lines · Robert Dolca

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.

Quoted 5 lines

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?

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 5 lines

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!

Quoted 1 line

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.

Quoted 75 lines · Robert Dolca

Quoted 73 lines · Jonathan Cameron

Quoted 62 lines · Lars-Peter Clausen

Quoted 39 lines · Jonathan Cameron

Quoted 38 lines · Lars-Peter Clausen

Quoted 37 lines · Jonathan Cameron

Quoted 25 lines · Robert Dolca

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.

Quoted 7 lines

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?

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 5 lines

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

Quoted 4 lines

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.

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

drivers/iio/industrialio-core.c +20 -0

index 69feb91..94bcd54 100644
@@ -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)

drivers/iio/industrialio-trigger.c +64 -0

index d31098e..cebdd10 100644
@@ -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)
 {

include/linux/iio/iio.h +0 -0

index 878d861..9ff54ef 100644

include/linux/iio/iio.h +0 -0

 };
 
 const struct iio_chan_spec

include/linux/iio/trigger.h +0 -0

index fa76c79..4001e44 100644

include/linux/iio/trigger.h +21 -1

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

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

drivers/iio/industrialio-core.c +4 -0

index 94bcd54..04862a4 100644
@@ -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);
 

drivers/iio/industrialio-trigger.c +6 -0

index cebdd10..d4118fe 100644
@@ -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;

include/linux/iio/iio.h +1 -1

index 9ff54ef..3f704ae 100644
@@ -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/