iio-interrupt-trigger enhancements

From: Grégor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Signed-off-by Gregor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

---
 drivers/iio/trigger/iio-trig-interrupt.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/iio/trigger/iio-trig-interrupt.c +10 -1

index 572bc6f..3c4e18f 100644
@@ -104,11 +104,20 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id iio_interrupt_trigger_of_match[] = {
+	{ .compatible = "iio-interrupt-trigger" },
+	{}
+};
+#endif
+
 static struct platform_driver iio_interrupt_trigger_driver = {
 	.probe = iio_interrupt_trigger_probe,
 	.remove = iio_interrupt_trigger_remove,
 	.driver = {
-		.name = "iio_interrupt_trigger",
+		.name           = "iio_interrupt_trigger",
+		.owner          = THIS_MODULE,
+		.of_match_table = of_match_ptr(iio_interrupt_trigger_of_match)
 	},
 };
 

From: Grégor Boirie <[email protected]>

Add a sysfs file entry for each interrupt trigger instance allowing userspace to :
* poll for interrupt events ;

* retrieve number of interrupts that occurred since trigger was
* initialized

Signed-off-by Gregor Boirie <[email protected]>

---
 .../ABI/testing/sysfs-bus-iio-trig-interrupt       | 22 ++++++
 drivers/iio/trigger/iio-trig-interrupt.c           | 88 ++++++++++++++++------
 2 files changed, 88 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt

Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt new file +22 -0

new file mode 100644
index 0000000..cb246d2
@@ -0,0 +1,22 @@
+What:		/sys/bus/iio/devices/triggerX/name
+KernelVersion:	3.11
+Contact:	[email protected]
+Description:
+		The name attribute holds a description string for the current
+		trigger. In order to associate the trigger with an IIO device
+		one should write this name string to
+		/sys/bus/iio/devices/iio:deviceY/trigger/current_trigger.
+
+What:		/sys/bus/iio/devices/triggerX/count
+KernelVersion:	4.5
+Contact:	[email protected]
+Description:
+		The count attribute is a unsigned int counter holding the number
+		of times the attached interrupt occurred since trigger was
+		initialized.
+		You can poll(2) on that file and poll(2) will return whenever
+		the interrupt was triggered. If you use poll(2), set the events
+		POLLPRI and POLLERR. If you use select(2), set the file
+		descriptor in exceptfds. After poll(2) returns, either lseek(2)
+		to the beginning of the sysfs file and read the new value or
+		close the file and re-open it to read the value.

drivers/iio/trigger/iio-trig-interrupt.c +66 -22

index 3c4e18f..1bf3986 100644
@@ -17,14 +17,53 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>
 
-
 struct iio_interrupt_trigger_info {
+	struct kernfs_node *poll;
+	atomic_t count;
 	unsigned int irq;
 };
 
+/*
+ * If interested by counter value, userspace should read often enough since
+ * counter may wrap. Userspace will miss interrupt events when counter wraps
+ * twice or more between 2 consecutive reads.
+ */
+ssize_t iio_interrupt_trigger_show_count(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct iio_interrupt_trigger_info *info = iio_trigger_get_drvdata(trig);
+	unsigned int count = atomic_read(&info->count);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", count);
+}
+
+static DEVICE_ATTR(count, S_IRUGO, iio_interrupt_trigger_show_count, NULL);
+
+static struct attribute *iio_interrupt_trigger_attrs[] = {
+	&dev_attr_count.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_interrupt_trigger_attr_group = {
+	.attrs = iio_interrupt_trigger_attrs,
+};
+
+static const struct attribute_group *iio_interrupt_trigger_attr_groups[] = {
+	&iio_interrupt_trigger_attr_group,
+	NULL
+};
+
 static irqreturn_t iio_interrupt_trigger_poll(int irq, void *private)
 {
-	iio_trigger_poll(private);
+	struct iio_trigger *trig = private;
+	struct iio_interrupt_trigger_info *info = iio_trigger_get_drvdata(trig);
+
+	atomic_inc(&info->count);
+	sysfs_notify_dirent(info->poll);
+	iio_trigger_poll(trig);
+
 	return IRQ_HANDLED;
 }
 

@@ -36,17 +75,12 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 {
 	struct iio_interrupt_trigger_info *trig_info;
 	struct iio_trigger *trig;
-	unsigned long irqflags;
 	struct resource *irq_res;
 	int irq, ret = 0;
 
 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-
-	if (irq_res == NULL)
+	if (!irq_res)
 		return -ENODEV;
-
-	irqflags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
-
 	irq = irq_res->start;
 
 	trig = iio_trigger_alloc("irqtrig%d", irq);

@@ -60,27 +94,36 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 		ret = -ENOMEM;
 		goto error_put_trigger;
 	}
-	iio_trigger_set_drvdata(trig, trig_info);
+
+	atomic_set(&trig_info->count, 0);
 	trig_info->irq = irq;
+	iio_trigger_set_drvdata(trig, trig_info);
 	trig->ops = &iio_interrupt_trigger_ops;
-	ret = request_irq(irq, iio_interrupt_trigger_poll,
-			  irqflags, trig->name, trig);
-	if (ret) {
-		dev_err(&pdev->dev,
-			"request IRQ-%d failed", irq);
+	trig->dev.groups = iio_interrupt_trigger_attr_groups;
+	ret = iio_trigger_register(trig);
+	if (ret)
 		goto error_free_trig_info;
+
+	/* Create a sysfs entry which the userspace may poll for irq events. */
+	trig_info->poll = sysfs_get_dirent(trig->dev.kobj.sd, "count");
+	if (!trig_info->poll) {
+		ret = -ENOENT;
+		goto error_unregister_trig;
 	}
 
-	ret = iio_trigger_register(trig);
-	if (ret)
-		goto error_release_irq;
-	platform_set_drvdata(pdev, trig);
+	ret = request_irq(irq, iio_interrupt_trigger_poll,
+			  (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED,
+			  trig->name, trig);
+	if (!ret) {
+		platform_set_drvdata(pdev, trig);
+		return 0;
+	}
 
-	return 0;
+	sysfs_put(trig_info->poll);
 
 /* First clean up the partly allocated trigger */
-error_release_irq:
-	free_irq(irq, trig);
+error_unregister_trig:
+	iio_trigger_unregister(trig);
 error_free_trig_info:
 	kfree(trig_info);
 error_put_trigger:

@@ -96,8 +139,9 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev)
 
 	trig = platform_get_drvdata(pdev);
 	trig_info = iio_trigger_get_drvdata(trig);
-	iio_trigger_unregister(trig);
 	free_irq(trig_info->irq, trig);
+	sysfs_put(trig_info->poll);
+	iio_trigger_unregister(trig);
 	kfree(trig_info);
 	iio_trigger_put(trig);
 

Quoted 2 lines · Gregor Boirie

From: Grégor Boirie <[email protected]>

Hi Gregor.

You certainly have some unusual requirements - or perhaps you are simply the first person to show up with them here!

Quoted 3 lines

Add a sysfs file entry for each interrupt trigger instance allowing userspace to :
* poll for interrupt events ;

Why?

Quoted 3 lines

* retrieve number of interrupts that occurred since trigger was
* initialized

Again why?

This is interesting stuff, but I'd like to fully understand the question of what you are doing with it before we go too far into the code.

Quoted 166 lines

Signed-off-by Gregor Boirie <[email protected]>

---
  .../ABI/testing/sysfs-bus-iio-trig-interrupt       | 22 ++++++
  drivers/iio/trigger/iio-trig-interrupt.c           | 88 ++++++++++++++++------
  2 files changed, 88 insertions(+), 22 deletions(-)

create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt

Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt new file +22 -0

new file mode 100644
index 0000000..cb246d2
@@ -0,0 +1,22 @@
+What:		/sys/bus/iio/devices/triggerX/name
+KernelVersion:	3.11
+Contact:	[email protected]
+Description:
+		The name attribute holds a description string for the current
+		trigger. In order to associate the trigger with an IIO device
+		one should write this name string to
+		/sys/bus/iio/devices/iio:deviceY/trigger/current_trigger.
+
+What:		/sys/bus/iio/devices/triggerX/count
+KernelVersion:	4.5
+Contact:	[email protected]
+Description:
+		The count attribute is a unsigned int counter holding the number
+		of times the attached interrupt occurred since trigger was
+		initialized.
+		You can poll(2) on that file and poll(2) will return whenever
+		the interrupt was triggered. If you use poll(2), set the events
+		POLLPRI and POLLERR. If you use select(2), set the file
+		descriptor in exceptfds. After poll(2) returns, either lseek(2)
+		to the beginning of the sysfs file and read the new value or
+		close the file and re-open it to read the value.

drivers/iio/trigger/iio-trig-interrupt.c +64 -21

index 3c4e18f..1bf3986 100644
@@ -17,14 +17,53 @@
  #include <linux/iio/iio.h>
  #include <linux/iio/trigger.h>
 
-
  struct iio_interrupt_trigger_info {
+	struct kernfs_node *poll;
+	atomic_t count;
  	unsigned int irq;
  };
 
+/*
+ * If interested by counter value, userspace should read often enough since
+ * counter may wrap. Userspace will miss interrupt events when counter wraps
+ * twice or more between 2 consecutive reads.
+ */
+ssize_t iio_interrupt_trigger_show_count(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct iio_interrupt_trigger_info *info = iio_trigger_get_drvdata(trig);
+	unsigned int count = atomic_read(&info->count);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", count);
+}
+
+static DEVICE_ATTR(count, S_IRUGO, iio_interrupt_trigger_show_count, NULL);
+
+static struct attribute *iio_interrupt_trigger_attrs[] = {
+	&dev_attr_count.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_interrupt_trigger_attr_group = {
+	.attrs = iio_interrupt_trigger_attrs,
+};
+
+static const struct attribute_group *iio_interrupt_trigger_attr_groups[] = {
+	&iio_interrupt_trigger_attr_group,
+	NULL
+};
+
  static irqreturn_t iio_interrupt_trigger_poll(int irq, void *private)
  {
-	iio_trigger_poll(private);
+	struct iio_trigger *trig = private;
+	struct iio_interrupt_trigger_info *info = iio_trigger_get_drvdata(trig);
+
+	atomic_inc(&info->count);
+	sysfs_notify_dirent(info->poll);
+	iio_trigger_poll(trig);
+
  	return IRQ_HANDLED;
  }
 

@@ -36,17 +75,12 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
  {
  	struct iio_interrupt_trigger_info *trig_info;
  	struct iio_trigger *trig;
-	unsigned long irqflags;
  	struct resource *irq_res;
  	int irq, ret = 0;
 
  	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-
-	if (irq_res == NULL)
+	if (!irq_res)
  		return -ENODEV;
-
-	irqflags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
-
  	irq = irq_res->start;
 
  	trig = iio_trigger_alloc("irqtrig%d", irq);

@@ -60,27 +94,36 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
  		ret = -ENOMEM;
  		goto error_put_trigger;
  	}
-	iio_trigger_set_drvdata(trig, trig_info);
+
+	atomic_set(&trig_info->count, 0);
  	trig_info->irq = irq;
+	iio_trigger_set_drvdata(trig, trig_info);
  	trig->ops = &iio_interrupt_trigger_ops;
-	ret = request_irq(irq, iio_interrupt_trigger_poll,
-			  irqflags, trig->name, trig);
-	if (ret) {
-		dev_err(&pdev->dev,
-			"request IRQ-%d failed", irq);
+	trig->dev.groups = iio_interrupt_trigger_attr_groups;
+	ret = iio_trigger_register(trig);
+	if (ret)
  		goto error_free_trig_info;
+
+	/* Create a sysfs entry which the userspace may poll for irq events. */
+	trig_info->poll = sysfs_get_dirent(trig->dev.kobj.sd, "count");
+	if (!trig_info->poll) {
+		ret = -ENOENT;
+		goto error_unregister_trig;
  	}
 
-	ret = iio_trigger_register(trig);
-	if (ret)
-		goto error_release_irq;
-	platform_set_drvdata(pdev, trig);
+	ret = request_irq(irq, iio_interrupt_trigger_poll,
+			  (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED,
+			  trig->name, trig);
+	if (!ret) {
+		platform_set_drvdata(pdev, trig);
+		return 0;
+	}
 
-	return 0;
+	sysfs_put(trig_info->poll);
 
  /* First clean up the partly allocated trigger */
-error_release_irq:
-	free_irq(irq, trig);
+error_unregister_trig:
+	iio_trigger_unregister(trig);
  error_free_trig_info:
  	kfree(trig_info);
  error_put_trigger:

drivers/iio/trigger/iio-trig-interrupt.c static int iio_interrupt_trigger_remove(struct platform_device *pdev) · line 147

Quoted 10 lines

drivers/iio/trigger/iio-trig-interrupt.c +2 -1

@@ -96,8 +139,9 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev)
 
  	trig = platform_get_drvdata(pdev);
  	trig_info = iio_trigger_get_drvdata(trig);
-	iio_trigger_unregister(trig);
  	free_irq(trig_info->irq, trig);
+	sysfs_put(trig_info->poll);
+	iio_trigger_unregister(trig);
  	kfree(trig_info);
  	iio_trigger_put(trig);
 

--
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 7 lines · Jonathan Cameron

Quoted 2 lines · Gregor Boirie

From: Grégor Boirie <[email protected]>

Hi Gregor.

You certainly have some unusual requirements - or perhaps you are simply the first person to show up with them here!

I would give preference to 2nd option :)

Quoted 3 lines

Quoted 3 lines

Add a sysfs file entry for each interrupt trigger instance allowing userspace to :
* poll for interrupt events ;

Quoted 1 line

Why?

On our platform, several sensors are driven by a dedicated "real-time" CPU (bare-metal context). This CPU interrupts Linux in a periodic manner which in turn wakes a user process up to perform sensor fusion. This sysfs poll support allows the user process to explictly wait for this dedicated CPU interrupt to fetch data located in RAM then process them.

Add a sysfs file entry for each interrupt trigger instance allowing userspace to :
* poll for interrupt events ;

Quoted 7 lines

Quoted 1 line

Why?

On our platform, several sensors are driven by a dedicated "real-time" CPU (bare-metal context). This CPU interrupts Linux in a periodic manner which in turn wakes a user process up to perform sensor fusion. This sysfs poll support allows the user process to explictly wait for this dedicated CPU interrupt to fetch data located in RAM then process them.

So you are only using the IIO trigger in this setup, but no buffers or devices? If you just want to forward the interrupt to userspace UIO might be the better solution in this case:
https://www.kernel.org/doc/htmldocs/uio-howto/about.html

- Lars

--
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 15 lines · Lars-Peter Clausen

Add a sysfs file entry for each interrupt trigger instance allowing userspace to :
* poll for interrupt events ;

Quoted 7 lines

Quoted 1 line

Why?

On our platform, several sensors are driven by a dedicated "real-time" CPU (bare-metal context). This CPU interrupts Linux in a periodic manner which in turn wakes a user process up to perform sensor fusion. This sysfs poll support allows the user process to explictly wait for this dedicated CPU interrupt to fetch data located in RAM then process them.

So you are only using the IIO trigger in this setup, but no buffers or devices? If you just want to forward the interrupt to userspace UIO might be the better solution in this case:
https://www.kernel.org/doc/htmldocs/uio-howto/about.html

Quoted 1 line

- Lars

I forgot to mention I also need the trigger to initiate in-kernel sampling for other IIO devices. These samples are also retrieved by the process performing
fusion.

Quoted 16 lines · Gregor Boirie

Quoted 15 lines · Lars-Peter Clausen

Add a sysfs file entry for each interrupt trigger instance allowing userspace to :
* poll for interrupt events ;

Quoted 7 lines

Quoted 1 line

Why?

On our platform, several sensors are driven by a dedicated "real-time" CPU (bare-metal context). This CPU interrupts Linux in a periodic manner which in turn wakes a user process up to perform sensor fusion. This sysfs poll support allows the user process to explictly wait for this dedicated CPU interrupt to fetch data located in RAM then process them.

So you are only using the IIO trigger in this setup, but no buffers or devices? If you just want to forward the interrupt to userspace UIO might be the better solution in this case:
https://www.kernel.org/doc/htmldocs/uio-howto/about.html

Quoted 5 lines

Quoted 1 line

- Lars

I forgot to mention I also need the trigger to initiate in-kernel sampling for other IIO devices. These samples are also retrieved by the process performing
fusion.

You should still be able to export the interrupt using UIO by sharing the IRQ.

- Lars

Quoted 3 lines · Gregor Boirie

From: Grégor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Signed-off-by Gregor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Snag here is that iio_interrupt_trigger is a very linux specific name and device tree bindings should be just about the hardware.

Not entirely sure how we avoid this though as the use is rather hard to describe generically.

cc'd device tree list and bindings maintainers.

As a brief summary - this IIO trigger driver takes a generic
interrupt (from whatever) and uses it to drive sampling of IIO devices. The interrupt might be associated with particularly simple sensors directly but is more commonly a gpio interrupt line used cause samples to be captured from unrelated devices. Sometimes the source of that interrupt can be a convoluted external mux setup over which linux has no control for example.

Any suggestions on appropriate naming?

We aren't really describing hardware here, rather a policy decision on what a given interrupt is to be used for.

I suppose ultimately we could take the view this should be handled via another route (from userspace via an appropriate configfs interface for example).

Quoted 10 lines · Jonathan Cameron

Quoted 3 lines · Gregor Boirie

From: Grégor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Signed-off-by Gregor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Snag here is that iio_interrupt_trigger is a very linux specific name and device tree bindings should be just about the hardware.

Not entirely sure how we avoid this though as the use is rather hard to describe generically.

Quoted 8 lines

cc'd device tree list and bindings maintainers.

As a brief summary - this IIO trigger driver takes a generic
interrupt (from whatever) and uses it to drive sampling of IIO devices. The interrupt might be associated with particularly simple sensors directly but is more commonly a gpio interrupt line used cause samples to be captured from unrelated devices. Sometimes the source of that interrupt can be a convoluted external mux setup over which linux has no control for example.

If linux has no control of the setup, then do we care? It's just some blackbox driving a signal.

Quoted 1 line

Any suggestions on appropriate naming?

I would think of it outside of IIO perhaps. We already have gpio-keys which is kind of similar. Maybe just "external interrupt"? Is it always a GPIO interrupt or could be polled GPIO or some other mechanism?

Quoted 10 lines · Jonathan Cameron

Quoted 3 lines · Gregor Boirie

From: Grégor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Signed-off-by Gregor Boirie <gregor.boirie-ITF29qwbsa/[email protected]>

Snag here is that iio_interrupt_trigger is a very linux specific name and device tree bindings should be just about the hardware.

Not entirely sure how we avoid this though as the use is rather hard to describe generically.

Quoted 8 lines

cc'd device tree list and bindings maintainers.

As a brief summary - this IIO trigger driver takes a generic
interrupt (from whatever) and uses it to drive sampling of IIO devices. The interrupt might be associated with particularly simple sensors directly but is more commonly a gpio interrupt line used cause samples to be captured from unrelated devices. Sometimes the source of that interrupt can be a convoluted external mux setup over which linux has no control for example.

If linux has no control of the setup, then do we care? It's just some blackbox driving a signal.

Quoted 1 line

Any suggestions on appropriate naming?

I would think of it outside of IIO perhaps. We already have gpio-keys which is kind of similar. Maybe just "external interrupt"? Is it always a GPIO interrupt or could be polled GPIO or some other mechanism?

Could you add "trigger-gpios" to every device that uses it and allow for it to appear multiple times? It somewhat depends on how static setting the trigger source is whether that would be appropriate.

Quoted 6 lines

We aren't really describing hardware here, rather a policy decision on what a given interrupt is to be used for.

I suppose ultimately we could take the view this should be handled via another route (from userspace via an appropriate configfs interface for example).

You would still need to know which GPIOs you could use or assign, so I think we need something in DT.