[PATCH v1 0/2] iio-interrupt-trigger enhancements

[PATCH v1 0/2] iio-interrupt-trigger enhancements

[Gregor Boirie]

First patch enable device-tree support.

Second one adds poll(2) support through a new sysfs attribute file "count"
showing how many times the attached interrupt occurred since trigger
initialization. This allows a userspace process :
* to explicitly wait for the interrupt to happen ;
* and to know wether or not it missed interrupt events since last count
  retrieval.

Regards.
Grégor

Grégor Boirie (2):
  iio:iio-interrupt-trigger: device-tree support
  iio:iio-interrupt-trigger: sysfs poll support

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

-- 
2.1.4

[PATCH v1 1/2] iio:iio-interrupt-trigger: device-tree support

[Gregor Boirie]

From: Grégor Boirie <gregor.boirie@parrot.com>

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 drivers/iio/trigger/iio-trig-interrupt.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c
index 572bc6f..3c4e18f 100644
--- a/drivers/iio/trigger/iio-trig-interrupt.c
+++ b/drivers/iio/trigger/iio-trig-interrupt.c
@@ -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)
 	},
 };
 
-- 
2.1.4

Re: [PATCH v1 1/2] iio:iio-interrupt-trigger: device-tree support

[Jonathan Cameron]

On 19/02/16 19:18, Gregor Boirie wrote:
> From: Grégor Boirie <gregor.boirie@parrot.com>
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
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).

Jonathan
> ---
>  drivers/iio/trigger/iio-trig-interrupt.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c
> index 572bc6f..3c4e18f 100644
> --- a/drivers/iio/trigger/iio-trig-interrupt.c
> +++ b/drivers/iio/trigger/iio-trig-interrupt.c
> @@ -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)
>  	},
>  };
>  
> 

Re: [PATCH v1 1/2] iio:iio-interrupt-trigger: device-tree support

[Rob Herring]

On Sun, Feb 21, 2016 at 07:55:24PM +0000, Jonathan Cameron wrote:
> On 19/02/16 19:18, Gregor Boirie wrote:
> > From: Grégor Boirie <gregor.boirie@parrot.com>
> > 
> > Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
> 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.

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

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

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

Rob

Re: [PATCH v1 1/2] iio:iio-interrupt-trigger: device-tree support

[Gregor Boirie]

On 02/22/2016 08:05 PM, Rob Herring wrote:
> On Sun, Feb 21, 2016 at 07:55:24PM +0000, Jonathan Cameron wrote:
>> On 19/02/16 19:18, Gregor Boirie wrote:
>>> From: Grégor Boirie <gregor.boirie@parrot.com>
>>>
>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>> 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.
> If linux has no control of the setup, then do we care? It's just some
> blackbox driving a signal.
>
>> 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?
Our setup uses an ARM software generated interrupt coming from an 
alternate core
running a custom OS. Usage is however quite similar to GPIO irqs.

[snip]

Grégor.

Re: [PATCH v1 1/2] iio:iio-interrupt-trigger: device-tree support

[Jonathan Cameron]

On 22/02/16 19:05, Rob Herring wrote:
> On Sun, Feb 21, 2016 at 07:55:24PM +0000, Jonathan Cameron wrote:
>> On 19/02/16 19:18, Gregor Boirie wrote:
>>> From: Grégor Boirie <gregor.boirie@parrot.com>
>>>
>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>> 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.
> 
> If linux has no control of the setup, then do we care? It's just some 
> blackbox driving a signal.
> 
>> 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?

The challenge is that we need to be able to capture it's use.
> 
> 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.
Right now we don't have a device - the interrupt trigger is only soft
associated (via sysfs) with it's users at a later date.

I think we just have 'make up a device' for this to represent the use case
so we can probe the interrupt trigger driver in iio.

sensor sampling trigger perhaps?

> 
>> 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.
Absolutely. Short of allowing completely generic grabbing from a configfs call
of an interrupt, definitely need to specify which interrupts are suitable for
use like this. 

I think a 'fictional' device is going to have to exist to allow this..

Jonathan

[PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Gregor Boirie]

From: Grégor Boirie <gregor.boirie@parrot.com>

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 <gregor.boirie@parrot.com>
---
 .../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

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt b/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt
new file mode 100644
index 0000000..cb246d2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt
@@ -0,0 +1,22 @@
+What:		/sys/bus/iio/devices/triggerX/name
+KernelVersion:	3.11
+Contact:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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.
diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c
index 3c4e18f..1bf3986 100644
--- a/drivers/iio/trigger/iio-trig-interrupt.c
+++ b/drivers/iio/trigger/iio-trig-interrupt.c
@@ -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);
 
-- 
2.1.4

Re: [PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Jonathan Cameron]

On 19/02/16 19:18, Gregor Boirie wrote:
> From: Grégor Boirie <gregor.boirie@parrot.com>
> 
Hi Gregor.

You certainly have some unusual requirements  - or perhaps you are
simply the first person to show up with them here!
> Add a sysfs file entry for each interrupt trigger instance allowing
> userspace to :
> * poll for interrupt events ;
Why? 

> * 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.
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
> ---
>  .../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
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt b/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt
> new file mode 100644
> index 0000000..cb246d2
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt
> @@ -0,0 +1,22 @@
> +What:		/sys/bus/iio/devices/triggerX/name
> +KernelVersion:	3.11
> +Contact:	linux-iio@vger.kernel.org
> +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:	linux-iio@vger.kernel.org
> +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.
> diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c
> index 3c4e18f..1bf3986 100644
> --- a/drivers/iio/trigger/iio-trig-interrupt.c
> +++ b/drivers/iio/trigger/iio-trig-interrupt.c
> @@ -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);
>  
> 

Re: [PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Gregor Boirie]

On 02/21/2016 09:08 PM, Jonathan Cameron wrote:
> On 19/02/16 19:18, Gregor Boirie wrote:
>> From: Grégor Boirie <gregor.boirie@parrot.com>
>>
> 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 :)
>> Add a sysfs file entry for each interrupt trigger instance allowing
>> userspace to :
>> * poll for interrupt events ;
> 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.

>
>> * retrieve number of interrupts that occurred since trigger was
>> * initialized
> Again why?
Some critical process scheduled at higher priority might in rare cases defer
the execution of sensor data polling. To recover from such situations, our
sensor fusion algorithm needs to know if interrupt events were missed since
last polling iteration and how many.
>
> 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.
>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>> ---
>>   .../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
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt b/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt
>> new file mode 100644
>> index 0000000..cb246d2
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-trig-interrupt
>> @@ -0,0 +1,22 @@
>> +What:		/sys/bus/iio/devices/triggerX/name
>> +KernelVersion:	3.11
>> +Contact:	linux-iio@vger.kernel.org
>> +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:	linux-iio@vger.kernel.org
>> +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.
>> diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c
>> index 3c4e18f..1bf3986 100644
>> --- a/drivers/iio/trigger/iio-trig-interrupt.c
>> +++ b/drivers/iio/trigger/iio-trig-interrupt.c
>> @@ -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);
>>   
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Lars-Peter Clausen]

On 02/22/2016 12:32 PM, Gregor Boirie wrote:
> On 02/21/2016 09:08 PM, Jonathan Cameron wrote:
>> On 19/02/16 19:18, Gregor Boirie wrote:
>>> From: Grégor Boirie <gregor.boirie@parrot.com>
>>>
>> 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 :)
>>> Add a sysfs file entry for each interrupt trigger instance allowing
>>> userspace to :
>>> * poll for interrupt events ;
>> 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

Re: [PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Gregor Boirie]

On 02/22/2016 12:37 PM, Lars-Peter Clausen wrote:
> Add a sysfs file entry for each interrupt trigger instance allowing
> userspace to :
> * poll for interrupt events ;
>>> 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
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.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Lars-Peter Clausen]

On 02/22/2016 02:07 PM, Gregor Boirie wrote:
> On 02/22/2016 12:37 PM, Lars-Peter Clausen wrote:
>> Add a sysfs file entry for each interrupt trigger instance allowing
>> userspace to :
>> * poll for interrupt events ;
>>>> 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
> 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

Re: [PATCH v1 2/2] iio:iio-interrupt-trigger: sysfs poll support

[Gregor Boirie]

On 02/22/2016 02:57 PM, Lars-Peter Clausen wrote:
> On 02/22/2016 02:07 PM, Gregor Boirie wrote:
>> On 02/22/2016 12:37 PM, Lars-Peter Clausen wrote:
>>> Add a sysfs file entry for each interrupt trigger instance allowing
>>> userspace to :
>>> * poll for interrupt events ;
>>>>> 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
>> 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.
I did not think this UIO framework was as flexible as it is.
It makes the iio-trigger-interrupt patch serie quite useless once UIO is 
considered as
a real alternative.
Many thanks for the proposal.

Grégor.
>
> - Lars
>