Skip to contentSkip to conversation

iio: Rework the generic trigger-gpio trigger and move out of staging

RFC PATCH Superseded Jonathan Cameron 2 participants 7 messages May 6, 2013 – May 12, 2013

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Jonathan Cameron

author maintainer
#0 lore raw

cover iio: Rework the generic trigger-gpio trigger and move out of staging

Hi all,

This is an RFC for a couple of reasons.

1) I'm not sure how people feel about the move to multiple platform devices (patch 3).
2) I don't actually have a simple test setup for this one. I can put one together but it might be a while before I get it done.

Anyhow, the purpose of this set is to respond to Lars-Peter's comments when I originally asked if anyone minded it moving out of staging as it was before this. Lars raised the point that having one driver instance register multiple triggers just made life more complicated, and instead it might be better to have multiple driver instances, with one trigger each. Lars also pointed out that the name of the driver was silly given there was nolonger a requirement that the source of the interrupts be a gpio and there hasn't been for quite some time.

All comments welcome. Particularly wrt to patch 3.

Jonathan Cameron (4):
staging:iio:trigger:gpio bug in release of gpio in error path staging:iio:triggers: rename iio-trig-gpio to iio-trig-interrupt staging:iio:triggers:interrupt trigger - one per platform device. iio:triggers:interrupt trigger - move out of staging.

 drivers/iio/trigger/Kconfig                 |   9 ++
 drivers/iio/trigger/Makefile                |   1 +
 drivers/iio/trigger/iio-trig-interrupt.c    | 123 ++++++++++++++++++++
 drivers/staging/iio/trigger/Kconfig         |   6 -
 drivers/staging/iio/trigger/Makefile        |   1 -
 drivers/staging/iio/trigger/iio-trig-gpio.c | 167 ----------------------------
 6 files changed, 133 insertions(+), 174 deletions(-)
 create mode 100644 drivers/iio/trigger/iio-trig-interrupt.c
 delete mode 100644 drivers/staging/iio/trigger/iio-trig-gpio.c
Signature · 1 line
1.8.2.1
author maintainer
#1 lore raw

PATCH 1/4 staging:iio:trigger:gpio bug in release of gpio in error path

Also dropped the unneeded gpio.h header.

---
 drivers/staging/iio/trigger/iio-trig-gpio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/staging/iio/trigger/iio-trig-gpio.c +1 -2
index 7c593d1..69dabca 100644
@@ -19,7 +19,6 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
-#include <linux/gpio.h>
 #include <linux/slab.h>
 
 #include <linux/iio/iio.h>
@@ -122,7 +121,7 @@ error_free_completed_registrations:
 				 &iio_gpio_trigger_list,
 				 alloc_list) {
 		trig_info = iio_trigger_get_drvdata(trig);
-		free_irq(gpio_to_irq(trig_info->irq), trig);
+		free_irq(trig_info->irq, trig);
 		kfree(trig_info);
 		iio_trigger_unregister(trig);
 	}
Signature · 1 line
1.8.2.1
author maintainer
#2 lore raw

PATCH 2/4 staging:iio:triggers: rename iio-trig-gpio to iio-trig-interrupt

In reply to Jonathan Cameron

Also change all internal naming appropriately.
This trigger is no longer just for gpio provided interrupts so change the naming to reflect this. Also drop some now missleading left over comments.

---
 drivers/staging/iio/trigger/Kconfig              |   8 +-
 drivers/staging/iio/trigger/Makefile             |   2 +-
 drivers/staging/iio/trigger/iio-trig-gpio.c      | 166 -----------------------
 drivers/staging/iio/trigger/iio-trig-interrupt.c | 152 +++++++++++++++++++++
 4 files changed, 157 insertions(+), 171 deletions(-)
drivers/staging/iio/trigger/Kconfig +4 -4
index d0ae99c..4ecb213 100644
@@ -12,11 +12,11 @@ config IIO_PERIODIC_RTC_TRIGGER
 	  Provides support for using periodic capable real time
 	  clocks as IIO triggers.
 
-config IIO_GPIO_TRIGGER
-	tristate "GPIO trigger"
-	depends on GENERIC_GPIO
+config IIO_INTERRUPT_TRIGGER
+	tristate "Generic interrupt trigger"
 	help
-	  Provides support for using GPIO pins as IIO triggers.
+	  Provides support for using interrupts of various types as IIO
+	  triggers.  These may be provided by a gpio driver for example.
 
 config IIO_BFIN_TMR_TRIGGER
 	tristate "Blackfin TIMER trigger"
drivers/staging/iio/trigger/Makefile +1 -1
index 8a53041..48f2236 100644
@@ -3,5 +3,5 @@
 #
 
 obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
-obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
+obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o
 obj-$(CONFIG_IIO_BFIN_TMR_TRIGGER) += iio-trig-bfin-timer.o
drivers/staging/iio/trigger/iio-trig-gpio.c deleted +0 -166

166 lines not on this page · raw message

drivers/staging/iio/trigger/iio-trig-interrupt.c new file +152 -0

152 lines not on this page · raw message

Signature · 1 line
1.8.2.1
author maintainer
#3 lore raw

PATCH 3/4 staging:iio:triggers:interrupt trigger - one per platform device.

In reply to Jonathan Cameron

Switching from one platform device registering a lot of triggers to one for each trigger simplifies the code somewhat. It would be relatively unusual to have more than a couple of such devices registered so this change will not result in much additional overhead.

---
 drivers/staging/iio/trigger/iio-trig-interrupt.c | 119 +++++++++--------------
 1 file changed, 45 insertions(+), 74 deletions(-)
drivers/staging/iio/trigger/iio-trig-interrupt.c +45 -74
index 9afba96..e015de2 100644
@@ -17,11 +17,8 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>
 
-static LIST_HEAD(iio_interrupt_trigger_list);
-static DEFINE_MUTEX(iio_interrupt_trigger_list_lock);
 
 struct iio_interrupt_trigger_info {
-	struct mutex in_use;
 	unsigned int irq;
 };
 
@@ -39,57 +36,48 @@ static const struct iio_trigger_ops iio_interrupt_trigger_ops = {
 static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 {
 	struct iio_interrupt_trigger_info *trig_info;
-	struct iio_trigger *trig, *trig2;
+	struct iio_trigger *trig;
 	unsigned long irqflags;
 	struct resource *irq_res;
 	int irq, ret = 0, irq_res_cnt = 0;
 
-	do {
-		irq_res = platform_get_resource(pdev,
-				IORESOURCE_IRQ, irq_res_cnt);
-
-		if (irq_res == NULL) {
-			if (irq_res_cnt == 0)
-				dev_err(&pdev->dev, "No IRQs specified");
-			break;
-		}
-		irqflags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
-
-		for (irq = irq_res->start; irq <= irq_res->end; irq++) {
-
-			trig = iio_trigger_alloc("irqtrig%d", irq);
-			if (!trig) {
-				ret = -ENOMEM;
-				goto error_free_completed_registrations;
-			}
-
-			trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
-			if (!trig_info) {
-				ret = -ENOMEM;
-				goto error_put_trigger;
-			}
-			iio_trigger_set_drvdata(trig, trig_info);
-			trig_info->irq = irq;
-			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);
-				goto error_free_trig_info;
-			}
-
-			ret = iio_trigger_register(trig);
-			if (ret)
-				goto error_release_irq;
-
-			list_add_tail(&trig->alloc_list,
-					&iio_interrupt_trigger_list);
-		}
-
-		irq_res_cnt++;
-	} while (irq_res != NULL);
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, irq_res_cnt);
 
+	if (irq_res == NULL) {
+		if (irq_res_cnt == 0)
+			dev_err(&pdev->dev, "No IRQs specified");
+		return -ENODEV;
+	}
+	irqflags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
+
+	irq = irq_res->start;
+
+	trig = iio_trigger_alloc("irqtrig%d", irq_res->start);
+	if (!trig) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
+	if (!trig_info) {
+		ret = -ENOMEM;
+		goto error_put_trigger;
+	}
+	iio_trigger_set_drvdata(trig, trig_info);
+	trig_info->irq = irq;
+	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);
+		goto error_free_trig_info;
+	}
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		goto error_release_irq;
+	platform_set_drvdata(pdev, trig);
 
 	return 0;
 
@@ -100,38 +88,21 @@ error_free_trig_info:
 	kfree(trig_info);
 error_put_trigger:
 	iio_trigger_put(trig);
-error_free_completed_registrations:
-	/* The rest should have been added to the iio_interrupt_trigger_list */
-	list_for_each_entry_safe(trig,
-				 trig2,
-				 &iio_interrupt_trigger_list,
-				 alloc_list) {
-		trig_info = iio_trigger_get_drvdata(trig);
-		free_irq(trig_info->irq, trig);
-		kfree(trig_info);
-		iio_trigger_unregister(trig);
-	}
-
+error_ret:
 	return ret;
 }
 
 static int iio_interrupt_trigger_remove(struct platform_device *pdev)
 {
-	struct iio_trigger *trig, *trig2;
+	struct iio_trigger *trig;
 	struct iio_interrupt_trigger_info *trig_info;
 
-	mutex_lock(&iio_interrupt_trigger_list_lock);
-	list_for_each_entry_safe(trig,
-				 trig2,
-				 &iio_interrupt_trigger_list,
-				 alloc_list) {
-		trig_info = iio_trigger_get_drvdata(trig);
-		iio_trigger_unregister(trig);
-		free_irq(trig_info->irq, trig);
-		kfree(trig_info);
-		iio_trigger_put(trig);
-	}
-	mutex_unlock(&iio_interrupt_trigger_list_lock);
+	trig = platform_get_drvdata(pdev);
+	trig_info = iio_trigger_get_drvdata(trig);
+	iio_trigger_unregister(trig);
+	free_irq(trig_info->irq, trig);
+	kfree(trig_info);
+	iio_trigger_put(trig);
 
 	return 0;
 }
Signature · 1 line
1.8.2.1
#4 lore raw

Re: [PATCH 3/4] staging:iio:triggers:interrupt trigger - one per platform device.

[...]

drivers/staging/iio/trigger/iio-trig-interrupt.c static const struct iio_trigger_ops iio_interrupt_trigger_ops = { · line 44

Quoted 12 lines
drivers/staging/iio/trigger/iio-trig-interrupt.c +2 -1
@@ -39,57 +36,48 @@ static const struct iio_trigger_ops iio_interrupt_trigger_ops = {
 static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 {
 	struct iio_interrupt_trigger_info *trig_info;
-	struct iio_trigger *trig, *trig2;
+	struct iio_trigger *trig;
 	unsigned long irqflags;
 	struct resource *irq_res;
 	int irq, ret = 0, irq_res_cnt = 0;
[...]
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, irq_res_cnt);

irq_res_cnt is always zero.

Quoted 1 line

[...]

author maintainer
#5 lore raw

Re: [PATCH 3/4] staging:iio:triggers:interrupt trigger - one per platform device.

Quoted 4 lines · Lars-Peter Clausen

[...]

Review static const struct iio_trigger_ops

Quoted 2 lines
(quoted diff) +0 -0
@@ -39,57 +36,48 @@ static const struct iio_trigger_ops

iio_interrupt_trigger_ops = {

Quoted 12 lines

drivers/staging/iio/trigger/iio-trig-interrupt.c static const struct iio_trigger_ops iio_interrupt_trigger_ops = { · line 44

Quoted 10 lines
drivers/staging/iio/trigger/iio-trig-interrupt.c +2 -1
 static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 {
 	struct iio_interrupt_trigger_info *trig_info;
-	struct iio_trigger *trig, *trig2;
+	struct iio_trigger *trig;
 	unsigned long irqflags;
 	struct resource *irq_res;
 	int irq, ret = 0, irq_res_cnt = 0;
[...]
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, irq_res_cnt);

irq_res_cnt is always zero.

Oops. Thanks

Quoted 6 lines
Quoted 2 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

Signature · 1 line
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
author maintainer
#6 lore raw

PATCH 4/4 iio:triggers:interrupt trigger - move out of staging.

In reply to Jonathan Cameron

This is now a very simple trigger indeed but useful in many common cases.

---
 drivers/iio/trigger/Kconfig                      |   9 ++
 drivers/iio/trigger/Makefile                     |   1 +
 drivers/iio/trigger/iio-trig-interrupt.c         | 123 +++++++++++++++++++++++
 drivers/staging/iio/trigger/Kconfig              |   6 --
 drivers/staging/iio/trigger/Makefile             |   1 -
 drivers/staging/iio/trigger/iio-trig-interrupt.c | 123 -----------------------
 6 files changed, 133 insertions(+), 130 deletions(-)
drivers/iio/trigger/Kconfig +9 -0
index a4e68db..360fd50 100644
@@ -3,6 +3,15 @@
 #
 menu "Triggers - standalone"
 
+config IIO_INTERRUPT_TRIGGER
+	tristate "Generic interrupt trigger"
+	help
+	  Provides support for using an interrupt of any type as an IIO
+	  trigger.  This may be provided by a gpio driver for example.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called iio-trig-interrupt.
+
 config IIO_SYSFS_TRIGGER
 	tristate "SYSFS trigger"
 	depends on SYSFS
drivers/iio/trigger/Makefile +1 -0
index e0b2183..ce319a5 100644
@@ -2,4 +2,5 @@
 # Makefile for triggers not associated with iio-devices
 #
 
+obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o
 obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
drivers/iio/trigger/iio-trig-interrupt.c new file +123 -0

123 lines not on this page · raw message

drivers/staging/iio/trigger/Kconfig +0 -6
index 4ecb213..2fd18c6 100644
@@ -12,12 +12,6 @@ config IIO_PERIODIC_RTC_TRIGGER
 	  Provides support for using periodic capable real time
 	  clocks as IIO triggers.
 
-config IIO_INTERRUPT_TRIGGER
-	tristate "Generic interrupt trigger"
-	help
-	  Provides support for using interrupts of various types as IIO
-	  triggers.  These may be provided by a gpio driver for example.
-
 config IIO_BFIN_TMR_TRIGGER
 	tristate "Blackfin TIMER trigger"
 	depends on BLACKFIN
drivers/staging/iio/trigger/Makefile +0 -1
index 48f2236..238481b 100644
@@ -3,5 +3,4 @@
 #
 
 obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
-obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o
 obj-$(CONFIG_IIO_BFIN_TMR_TRIGGER) += iio-trig-bfin-timer.o
drivers/staging/iio/trigger/iio-trig-interrupt.c deleted +0 -123

123 lines not on this page · raw message

Signature · 1 line
1.8.2.1