login
Search
Hi all, This is truly untested. I only compile tested thus far as I'm not sure it's the right path. It looks like that's what's supposed to be done, but I wanted to check with you guys before going any further. In a few minutes, I'll be starting to test these two patches and will try to find any possible errors with them but, please, give it a good review as IMO it's a rather invasive change. Thomas, I've got one extra question for you. I remember reading you suggested all drivers should actually try to use threaded IRQ infrastructure for handling the IRQ and the hardirq handler would only check if the IRQ is coming from this device. Is that how you expect theaded IRQ to be used ? What's the purpose ? Please, correct me if I'm wrong, but I came to the conclusion that threaded IRQs can be reniced and preempted which would mean we could be setting IRQ priorities from userland and we could also be running N IRQ threads if we have N cpu cores. Is that correct ? Is that your goal ? I tried searching for some more documentation on the threaded IRQ infrastructure but couldn't find much by grepping Documentation/ maybe my match string wasn't good enough. Anyway, let me know what are your feeling about the following two patches and what would be your goal should we move all IRQ handlers to threaded IRQ infrastructure as I remember you suggested. Happy new year all. Felipe Balbi (2): mfd: twl6030-irq: move to threaded_irq mfd: twl4030-irq: move to threaded_irq drivers/mfd/twl4030-irq.c | 133 +++++++++++++----------------------------- drivers/mfd/twl6030-irq.c | 141 ++++++++++++++++----------------------------- 2 files changed, 91 insertions(+), 183 deletions(-) -- 1.7.3.4.598.g85356 --
... and while at that, also start using
handle_nested_irq() as we should.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl4030-irq.c | 133 ++++++++++++++-------------------------------
1 files changed, 42 insertions(+), 91 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 5d3a147..e88d0c6 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -278,91 +278,56 @@ static const struct sih sih_modules_twl5031[8] = {
static unsigned twl4030_irq_base;
-static struct completion irq_event;
-
/*
* This thread processes interrupts reported by the Primary Interrupt Handler.
*/
-static int twl4030_irq_thread(void *data)
+static irqreturn_t twl4030_irq_thread(int irq, void *unused)
{
- long irq = (long)data;
- static unsigned i2c_errors;
- static const unsigned max_i2c_errors = 100;
-
-
- current->flags |= PF_NOFREEZE;
-
- while (!kthread_should_stop()) {
- int ret;
- int module_irq;
- u8 pih_isr;
-
- /* Wait for IRQ, then read PIH irq status (also blocking) */
- wait_for_completion_interruptible(&irq_event);
-
- ret = twl_i2c_read_u8(TWL4030_MODULE_PIH, &pih_isr,
- REG_PIH_ISR_P1);
- if (ret) {
- pr_warning("twl4030: I2C error %d reading PIH ISR\n",
- ret);
- if (++i2c_errors >= max_i2c_errors) {
- printk(KERN_ERR "Maximum I2C error count"
- " exceeded. Terminating %s.\n",
- __func__);
- break;
- }
- complete(&irq_event);
- continue;
- }
+ int ret;
+ int module_irq;
+ u8 pih_isr;
- /* these handlers deal with the relevant SIH irq status */
- local_irq_disable();
- for (module_irq = twl4030_irq_base;
- pih_isr;
- pih_isr >>= 1, module_irq++) {
- if (pih_isr & 0x1) {
- struct irq_desc *d = irq_to_desc(module_irq);
-
- if (!d) {
- pr_err("twl4030: Invalid SIH IRQ: %d\n",
- module_irq);
- return -EINVAL;
- }
-
- /* These can't be masked ... always warn
- * if we get any ...... and while at that, also start using
handle_nested_irq() as we should.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl6030-irq.c | 141 ++++++++++++++++-----------------------------
1 files changed, 49 insertions(+), 92 deletions(-)
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index aaedb11..1530411 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -84,99 +84,64 @@ static int twl6030_interrupt_mapping[24] = {
static unsigned twl6030_irq_base;
-static struct completion irq_event;
-
/*
* This thread processes interrupts reported by the Primary Interrupt Handler.
*/
-static int twl6030_irq_thread(void *data)
+static irqreturn_t twl6030_irq_thread(int irq, void *unused)
{
- long irq = (long)data;
- static unsigned i2c_errors;
- static const unsigned max_i2c_errors = 100;
int ret;
-
- current->flags |= PF_NOFREEZE;
-
- while (!kthread_should_stop()) {
- int i;
- union {
+ int i;
+ union {
u8 bytes[4];
u32 int_sts;
- } sts;
-
- /* Wait for IRQ, then read PIH irq status (also blocking) */
- wait_for_completion_interruptible(&irq_event);
-
- /* read INT_STS_A, B and C in one shot using a burst read */
- ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes,
- REG_INT_STS_A, 3);
- if (ret) {
- pr_warning("twl6030: I2C error %d reading PIH ISR\n",
- ret);
- if (++i2c_errors >= max_i2c_errors) {
- printk(KERN_ERR "Maximum I2C error count"
- " exceeded. Terminating %s.\n",
- __func__);
- break;
- }
- complete(&irq_event);
- continue;
- }
-
+ } sts;
+ disable_irq_nosync(irq);
- sts.bytes[3] = 0; /* Only 24 bits are valid*/
+ /* read INT_STS_A, B and C in one shot using a burst read */
+ ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes,
+ REG_INT_STS_A, 3);
+ if (ret) {
+ pr_warning("twl6030: I2C error %d reading PIH ISR\n",
+ ret);
+ return IRQ_NONE;
+ }
- for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) ...You shouldn't need this any more; the driver used to be faffing around Simiarly here as far as I know; the original code predates genirq support for this so is doing some hairy stuff that is no longer required and may actually be harmful. What I'd expect to see from a conversion like this would be that most of the locking/IRQ management stuff would be dropped and the bus_lock() and bus_sync_unlock() operations would be implemented. --
Hi, I'll look into it, thanks. -- balbi --
... and while at that, also start using
handle_nested_irq() as we should.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl4030-irq.c | 131 +++++++++++++--------------------------------
1 files changed, 38 insertions(+), 93 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 5d3a147..91331a7 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -30,7 +30,6 @@
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
-#include <linux/kthread.h>
#include <linux/slab.h>
#include <linux/i2c/twl.h>
@@ -278,91 +277,50 @@ static const struct sih sih_modules_twl5031[8] = {
static unsigned twl4030_irq_base;
-static struct completion irq_event;
-
/*
* This thread processes interrupts reported by the Primary Interrupt Handler.
*/
-static int twl4030_irq_thread(void *data)
+static irqreturn_t twl4030_irq_thread(int irq, void *unused)
{
- long irq = (long)data;
- static unsigned i2c_errors;
- static const unsigned max_i2c_errors = 100;
-
-
- current->flags |= PF_NOFREEZE;
-
- while (!kthread_should_stop()) {
- int ret;
- int module_irq;
- u8 pih_isr;
-
- /* Wait for IRQ, then read PIH irq status (also blocking) */
- wait_for_completion_interruptible(&irq_event);
-
- ret = twl_i2c_read_u8(TWL4030_MODULE_PIH, &pih_isr,
- REG_PIH_ISR_P1);
- if (ret) {
- pr_warning("twl4030: I2C error %d reading PIH ISR\n",
- ret);
- if (++i2c_errors >= max_i2c_errors) {
- printk(KERN_ERR "Maximum I2C error count"
- " exceeded. Terminating %s.\n",
- __func__);
- break;
- }
- complete(&irq_event);
- continue;
- }
+ int ret;
+ int module_irq;
+ u8 pih_isr;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_PIH, &pih_isr,
+ REG_PIH_ISR_P1);
+ if (ret) {
+ pr_warning("twl4030: I2C error %d reading PIH ISR\n",
+ ret);
+ return IRQ_NONE;
+ }
- /* these handlers deal with the relevant SIH irq status ...Hi all, I dropped the twl6030-irq.c changes because that thing is a bit messy. I hope the original author will feel inspired and fix that one up. Anyway, twl4030-irq.c seems to be going to the right direction now. Thanks to Mark Brown for pointing out the need to drop the locking and implement bus_lock/bus_sync_unlock methods. Compile tested only with omap2plus_defconfig. I'll test after merge window to be sure I have all the newest code. Felipe Balbi (3): mfd: twl4030-irq: move to threaded_irq mfd: twl4030-irq: drop the workqueue hackery mfd: twl4030-irq: implement bus_*lock drivers/mfd/twl4030-irq.c | 363 +++++++++++++++++--------------------------- 1 files changed, 140 insertions(+), 223 deletions(-) -- 1.7.3.4.598.g85356 --
Finally that workqueue isn't needed anymore.
Drop that hackery and move the spinlock_t to
a mutex so we can issue I2C operations with
a lock held.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl4030-irq.c | 226 +++++++++++++++++++--------------------------
1 files changed, 96 insertions(+), 130 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 91331a7..298956d 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -31,6 +31,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/slab.h>
+#include <linux/mutex.h>
#include <linux/i2c/twl.h>
@@ -434,46 +435,36 @@ static inline void activate_irq(int irq)
/*----------------------------------------------------------------------*/
-static DEFINE_SPINLOCK(sih_agent_lock);
-
-static struct workqueue_struct *wq;
-
struct sih_agent {
int irq_base;
const struct sih *sih;
u32 imr;
- bool imr_change_pending;
- struct work_struct mask_work;
-
u32 edge_change;
- struct work_struct edge_work;
+
+ struct mutex irq_lock;
};
-static void twl4030_sih_do_mask(struct work_struct *work)
+/*----------------------------------------------------------------------*/
+
+static void twl4030_sih_mask(unsigned irq)
{
- struct sih_agent *agent;
- const struct sih *sih;
+ struct sih_agent *agent = get_irq_chip_data(irq);
+ const struct sih *sih = agent->sih;
+
union {
u8 bytes[4];
u32 word;
} imr;
+
int status;
- agent = container_of(work, struct sih_agent, mask_work);
-
- /* see what work we have */
- spin_lock_irq(&sih_agent_lock);
- if (agent->imr_change_pending) {
- sih = agent->sih;
- /* byte[0] gets overwritten as we write ... */
- imr.word = cpu_to_le32(agent->imr << 8);
- agent->imr_change_pending = false;
- } else
- sih = NULL;
- spin_unlock_irq(&sih_agent_lock);
- if (!sih)
- return;
+ agent->imr |= BIT(irq - ...drop all the locking around mask/unmask and
implement bus_lock and bus_sync_unlock methods.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl4030-irq.c | 30 ++++++++++++++++++------------
1 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 298956d..ff7bb93 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -461,8 +461,6 @@ static void twl4030_sih_mask(unsigned irq)
agent->imr |= BIT(irq - agent->irq_base);
- mutex_lock(&agent->irq_lock);
-
/* byte[0] gets overwritten as we write ... */
imr.word = cpu_to_le32(agent->imr << 8);
@@ -472,7 +470,6 @@ static void twl4030_sih_mask(unsigned irq)
if (status)
pr_err("twl4030: %s, %s --> %d\n", __func__,
"write", status);
- mutex_unlock(&agent->irq_lock);
}
static void twl4030_sih_unmask(unsigned irq)
@@ -487,7 +484,6 @@ static void twl4030_sih_unmask(unsigned irq)
int status;
- mutex_lock(&agent->irq_lock);
agent->imr &= ~BIT(irq - agent->irq_base);
/* byte[0] gets overwritten as we write ... */
@@ -499,7 +495,6 @@ static void twl4030_sih_unmask(unsigned irq)
if (status)
pr_err("twl4030: %s, %s --> %d\n", __func__,
"write", status);
- mutex_unlock(&agent->irq_lock);
}
static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
@@ -517,7 +512,6 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
- mutex_lock(&agent->irq_lock);
if ((desc->status & IRQ_TYPE_SENSE_MASK) != trigger) {
u8 bytes[6];
u32 edge_change;
@@ -537,7 +531,7 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (status) {
pr_err("twl4030: %s, %s --> %d\n", __func__,
"read", status);
- goto out;
+ return status;
}
/* Modify only the bits we know must change */
@@ -550,8 +544,7 @@ static int twl4030_sih_set_type(unsigned ...I suspect you need to do some sort of sync with the hardware here - the _sync bit of the name comes from the fact that the mask and unmask stuff is still called with IRQs disabled and so can't touch and I2C chip, this is called after reenabling them give a chance for the updates done to be reflected in the hardware. The implementation everyone else has done is to update a register cache in the other functions then write that I just realised that this collides with the conversion to the irq_ versions that has been done on the driver in -next by either myself or Lennart (we both submitted essentially the same patches and a couple of his went in) - that was a purely mechanical conversion that didn't address any of the issues this patch addresses but they're touching the same code. --
Hi, now that I look at some gpio chips I see what you're saying, will update no problem. This will actually only be able on 2.6.39 merge window anyway, so I'll have plenty of time to rebase on 2.6.38 and get these patches queued. ps: sorry the mail change, out of the office. -- balbi --
Hi,
I believe you meant something like below, I'll get back to this in a
little while. Have lots to test:
From f15801a5d57041b7669222bdb7cccf8b592c2f63 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Tue, 28 Dec 2010 19:07:33 +0200
Subject: [PATCH 1/2] mfd: twl4030-irq: implement bus_*lock
Organization: Texas Instruments\n
drop all the locking around mask/unmask and
implement bus_lock and bus_sync_unlock methods.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl4030-irq.c | 101 ++++++++++++++++++++++-----------------------
1 files changed, 50 insertions(+), 51 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 298956d..84f6066 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -440,6 +440,7 @@ struct sih_agent {
const struct sih *sih;
u32 imr;
+ bool imr_change_pending;
u32 edge_change;
struct mutex irq_lock;
@@ -450,64 +451,23 @@ struct sih_agent {
static void twl4030_sih_mask(unsigned irq)
{
struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
-
- union {
- u8 bytes[4];
- u32 word;
- } imr;
-
- int status;
agent->imr |= BIT(irq - agent->irq_base);
-
- mutex_lock(&agent->irq_lock);
-
- /* byte[0] gets overwritten as we write ... */
- imr.word = cpu_to_le32(agent->imr << 8);
-
- /* write the whole mask ... simpler than subsetting it */
- status = twl_i2c_write(sih->module, imr.bytes,
- sih->mask[irq_line].imr_offset, sih->bytes_ixr);
- if (status)
- pr_err("twl4030: %s, %s --> %d\n", __func__,
- "write", status);
- mutex_unlock(&agent->irq_lock);
+ agent->imr_change_pending = true;
}
static void twl4030_sih_unmask(unsigned irq)
{
struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
- union {
- u8 bytes[4];
- u32 word;
- } imr;
-
- int status;
-
- mutex_lock(&agent->irq_lock);
agent->imr &= ...Yes, that looks like what I'd expect. --
Good, I'll clean the patches up and wait past the merge window before sending final versions. -- balbi --
Hi,
when we finally move to struct irq_data, the below could
be used. BTW, Thomas do you have any plans for exposing
irq_data_to_desc() ?
8<------------------------------ cut here --------------------------------------
From 6069e3ddb34608fef0cb3dca3e544fca8deb3840 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Tue, 28 Dec 2010 19:33:22 +0200
Subject: [PATCH] mfd: twl4030-irq: switch to new methods
Organization: Texas Instruments\n
Move everybody to struct irq_data-based
methods.
NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/twl4030-irq.c | 86 ++++++++++++++++++++++++++++++++-------------
1 files changed, 61 insertions(+), 25 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index ff7bb93..ad6197c 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -447,10 +447,14 @@ struct sih_agent {
/*----------------------------------------------------------------------*/
-static void twl4030_sih_mask(unsigned irq)
+/* REVISIT define it here until IRQ Subsystem exports its implementation */
+#define irq_data_to_desc(data) container_of(data, struct irq_desc, irq_data)
+
+static void twl4030_sih_mask(struct irq_data *data)
{
- struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
+ struct irq_desc *d = irq_data_to_desc(data);
+ struct sih_agent *agent;
+ const struct sih *sih;
union {
u8 bytes[4];
@@ -458,7 +462,13 @@ static void twl4030_sih_mask(unsigned irq)
} imr;
int status;
+ int irq = data->irq;
+ if (!d)
+ return;
+
+ agent = d->chip_data;
+ sih = agent->sih;
agent->imr |= BIT(irq - agent->irq_base);
/* byte[0] gets overwritten as we write ... */
@@ -472,10 +482,11 @@ static void twl4030_sih_mask(unsigned irq)
"write", status);
}
-static void twl4030_sih_unmask(unsigned irq)
+static void twl4030_sih_unmask(struct irq_data *data)
{
- struct sih_agent *agent = ...The general idea is to move to struct irq_data sooner rather than later It looks like all you're using this for is to get the chip_data? If that is the case you're looking for irq_data_get_irq_chip_data() which will go directly from the irq_data to the chip_data. I may have missed something, I only scanned the code. --
Hi, you're right, just didn't know that was such a helper. BTW, quite a big name. -- balbi --
