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 <[email protected]>
---
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 surprises.
- */
- if (d->status & IRQ_DISABLED)
- note_interrupt(module_irq, d,
- IRQ_NONE);
- else
- d->handle_irq(module_irq, d);
+ disable_irq_nosync(irq);
+
+ 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 */
+ 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 IRQ_NONE;
}
- }
- local_irq_enable();
- enable_irq(irq);
+ /* These can't be masked ... always warn
+ * if we get any surprises.
+ */
+ if (d->status & IRQ_DISABLED)
+ note_interrupt(module_irq, d,
+ IRQ_NONE);
+ else
+ handle_nested_irq(module_irq);
+ }
}
+ local_irq_enable();
+ enable_irq(irq);
+
return 0;
}
-/*
- * handle_twl4030_pih() is the desc->handle method for the twl4030 interrupt.
- * This is a chained interrupt, so there is no desc->action method for it.
- * Now we need to query the interrupt controller in the twl4030 to determine
- * which module is generating the interrupt request. However, we can't do i2c
- * transactions in interrupt context, so we must defer that work to a kernel
- * thread. All we do here is acknowledge and mask the interrupt and wakeup
- * the kernel thread.
- */
-static irqreturn_t handle_twl4030_pih(int irq, void *devid)
-{
- /* Acknowledge, clear *AND* mask the interrupt... */
- disable_irq_nosync(irq);
- complete(devid);
- return IRQ_HANDLED;
-}
/*----------------------------------------------------------------------*/
/*
@@ -788,7 +753,6 @@ int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
int status;
int i;
- struct task_struct *task;
/*
* Mask and clear all TWL4030 interrupts since initially we do
@@ -830,28 +794,15 @@ int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
goto fail;
}
- /* install an irq handler to demultiplex the TWL4030 interrupt */
-
-
- init_completion(&irq_event);
-
- status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
- "TWL4030-PIH", &irq_event);
+ status = request_threaded_irq(irq_num, NULL, twl4030_irq_thread,
+ IRQF_DISABLED, "TWL4030-PIH", NULL);
if (status < 0) {
pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
goto fail_rqirq;
}
- task = kthread_run(twl4030_irq_thread, (void *)(long)irq_num,
- "twl4030-irq");
- if (IS_ERR(task)) {
- pr_err("twl4030: could not create irq %d thread!\n", irq_num);
- status = PTR_ERR(task);
- goto fail_kthread;
- }
- return status;
-fail_kthread:
- free_irq(irq_num, &irq_event);
+ return 0;
+
fail_rqirq:
/* clean up twl4030_sih_setup */
fail:
--
1.7.3.4.598.g85356
... and while at that, also start using
handle_nested_irq() as we should.
Signed-off-by: Felipe Balbi <[email protected]>
---
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++) {
- local_irq_disable();
- if (sts.int_sts & 0x1) {
- int module_irq = twl6030_irq_base +
- twl6030_interrupt_mapping[i];
- struct irq_desc *d = irq_to_desc(module_irq);
- if (!d) {
- pr_err("twl6030: Invalid SIH IRQ: %d\n",
- module_irq);
- return -EINVAL;
- }
+ sts.bytes[3] = 0; /* Only 24 bits are valid*/
- /* These can't be masked ... always warn
- * if we get any surprises.
- */
- if (d->status & IRQ_DISABLED)
- note_interrupt(module_irq, d,
- IRQ_NONE);
- else
- d->handle_irq(module_irq, d);
+ for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
+ local_irq_disable();
+ if (sts.int_sts & 0x1) {
+ int module_irq = twl6030_irq_base +
+ twl6030_interrupt_mapping[i];
+ struct irq_desc *d = irq_to_desc(module_irq);
+ if (!d) {
+ pr_err("twl6030: Invalid SIH IRQ: %d\n",
+ module_irq);
+ return IRQ_NONE;
}
- local_irq_enable();
- }
- ret = twl_i2c_write(TWL_MODULE_PIH, sts.bytes,
- REG_INT_STS_A, 3); /* clear INT_STS_A */
- if (ret)
- pr_warning("twl6030: I2C error in clearing PIH ISR\n");
- enable_irq(irq);
+ /* These can't be masked ... always warn
+ * if we get any surprises.
+ */
+ if (d->status & IRQ_DISABLED)
+ note_interrupt(module_irq, d,
+ IRQ_NONE);
+ else
+ handle_nested_irq(module_irq);
+
+ }
+ local_irq_enable();
}
+ ret = twl_i2c_write(TWL_MODULE_PIH, sts.bytes,
+ REG_INT_STS_A, 3); /* clear INT_STS_A */
+ if (ret)
+ pr_warning("twl6030: I2C error in clearing PIH ISR\n");
- return 0;
-}
+ enable_irq(irq);
-/*
- * handle_twl6030_int() is the desc->handle method for the twl6030 interrupt.
- * This is a chained interrupt, so there is no desc->action method for it.
- * Now we need to query the interrupt controller in the twl6030 to determine
- * which module is generating the interrupt request. However, we can't do i2c
- * transactions in interrupt context, so we must defer that work to a kernel
- * thread. All we do here is acknowledge and mask the interrupt and wakeup
- * the kernel thread.
- */
-static irqreturn_t handle_twl6030_pih(int irq, void *devid)
-{
- disable_irq_nosync(irq);
- complete(devid);
return IRQ_HANDLED;
}
@@ -303,7 +268,6 @@ int twl6030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
int status = 0;
int i;
- struct task_struct *task;
int ret;
u8 mask[4];
@@ -337,28 +301,21 @@ int twl6030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
pr_info("twl6030: %s (irq %d) chaining IRQs %d..%d\n", "PIH",
irq_num, irq_base, twl6030_irq_next - 1);
- /* install an irq handler to demultiplex the TWL6030 interrupt */
- init_completion(&irq_event);
- task = kthread_run(twl6030_irq_thread, (void *)irq_num, "twl6030-irq");
- if (IS_ERR(task)) {
- pr_err("twl6030: could not create irq %d thread!\n", irq_num);
- status = PTR_ERR(task);
- goto fail_kthread;
- }
-
- status = request_irq(irq_num, handle_twl6030_pih, IRQF_DISABLED,
- "TWL6030-PIH", &irq_event);
+ status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
+ IRQF_DISABLED, "TWL6030-PIH", NULL);
if (status < 0) {
pr_err("twl6030: could not claim irq%d: %d\n", irq_num, status);
- goto fail_irq;
+ goto fail;
}
- return status;
-fail_irq:
- free_irq(irq_num, &irq_event);
-fail_kthread:
+ return 0;
+
+fail:
+ free_irq(irq_num, NULL);
+
for (i = irq_base; i < irq_end; i++)
set_irq_chip_and_handler(i, NULL, NULL);
+
return status;
}
--
1.7.3.4.598.g85356
On Tue, Dec 28, 2010 at 03:59:49PM +0200, Felipe Balbi wrote:
> + disable_irq_nosync(irq);
You shouldn't need this any more; the driver used to be faffing around
with this because it wasn't using genirq for this in the past.
> + for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
> + local_irq_disable();
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,
On Tue, Dec 28, 2010 at 03:46:17PM +0000, Mark Brown wrote:
>On Tue, Dec 28, 2010 at 03:59:49PM +0200, Felipe Balbi wrote:
>
>> + disable_irq_nosync(irq);
>
>You shouldn't need this any more; the driver used to be faffing around
>with this because it wasn't using genirq for this in the past.
>
>> + for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
>> + local_irq_disable();
>
>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.
Aa, true. Forgot that one.
>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.
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 <[email protected]>
---
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 */
- 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 surprises.
- */
- if (d->status & IRQ_DISABLED)
- note_interrupt(module_irq, d,
- IRQ_NONE);
- else
- d->handle_irq(module_irq, d);
+ /* these handlers deal with the relevant SIH irq status */
+ 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 IRQ_NONE;
}
- }
- local_irq_enable();
- enable_irq(irq);
+ /* These can't be masked ... always warn
+ * if we get any surprises.
+ */
+ if (d->status & IRQ_DISABLED)
+ note_interrupt(module_irq, d,
+ IRQ_NONE);
+ else
+ handle_nested_irq(module_irq);
+ }
}
- return 0;
-}
-
-/*
- * handle_twl4030_pih() is the desc->handle method for the twl4030 interrupt.
- * This is a chained interrupt, so there is no desc->action method for it.
- * Now we need to query the interrupt controller in the twl4030 to determine
- * which module is generating the interrupt request. However, we can't do i2c
- * transactions in interrupt context, so we must defer that work to a kernel
- * thread. All we do here is acknowledge and mask the interrupt and wakeup
- * the kernel thread.
- */
-static irqreturn_t handle_twl4030_pih(int irq, void *devid)
-{
- /* Acknowledge, clear *AND* mask the interrupt... */
- disable_irq_nosync(irq);
- complete(devid);
return IRQ_HANDLED;
}
+
/*----------------------------------------------------------------------*/
/*
@@ -788,7 +746,6 @@ int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
int status;
int i;
- struct task_struct *task;
/*
* Mask and clear all TWL4030 interrupts since initially we do
@@ -817,6 +774,7 @@ int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
for (i = irq_base; i < irq_end; i++) {
set_irq_chip_and_handler(i, &twl4030_irq_chip,
handle_simple_irq);
+ set_irq_nested_thread(i, 1);
activate_irq(i);
}
twl4030_irq_next = i;
@@ -830,28 +788,15 @@ int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
goto fail;
}
- /* install an irq handler to demultiplex the TWL4030 interrupt */
-
-
- init_completion(&irq_event);
-
- status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
- "TWL4030-PIH", &irq_event);
+ status = request_threaded_irq(irq_num, NULL, twl4030_irq_thread,
+ IRQF_DISABLED, "TWL4030-PIH", NULL);
if (status < 0) {
pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
goto fail_rqirq;
}
- task = kthread_run(twl4030_irq_thread, (void *)(long)irq_num,
- "twl4030-irq");
- if (IS_ERR(task)) {
- pr_err("twl4030: could not create irq %d thread!\n", irq_num);
- status = PTR_ERR(task);
- goto fail_kthread;
- }
- return status;
-fail_kthread:
- free_irq(irq_num, &irq_event);
+ return 0;
+
fail_rqirq:
/* clean up twl4030_sih_setup */
fail:
--
1.7.3.4.598.g85356
drop all the locking around mask/unmask and
implement bus_lock and bus_sync_unlock methods.
Signed-off-by: Felipe Balbi <[email protected]>
---
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 irq, unsigned trigger)
if (!d) {
pr_err("twl4030: Invalid IRQ: %d\n",
i + agent->irq_base);
- status = -ENODEV;
- goto out;
+ return -ENODEV;
}
bytes[byte] &= ~(0x03 << off);
@@ -574,17 +567,30 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
"write", status);
}
-out:
- mutex_unlock(&agent->irq_lock);
-
return status;
}
+static void twl4030_sih_bus_lock(unsigned int irq)
+{
+ struct sih_agent *agent = get_irq_chip_data(irq);
+
+ mutex_lock(&agent->irq_lock);
+}
+
+static void twl4030_sih_bus_sync_unlock(unsigned int irq)
+{
+ struct sih_agent *agent = get_irq_chip_data(irq);
+
+ mutex_unlock(&agent->irq_lock);
+}
+
static struct irq_chip twl4030_sih_irq_chip = {
.name = "twl4030",
.mask = twl4030_sih_mask,
.unmask = twl4030_sih_unmask,
.set_type = twl4030_sih_set_type,
+ .bus_lock = twl4030_sih_bus_lock,
+ .bus_sync_unlock = twl4030_sih_bus_sync_unlock,
};
/*----------------------------------------------------------------------*/
--
1.7.3.4.598.g85356
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 <[email protected]>
---
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 - 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,
@@ -481,111 +472,42 @@ static void twl4030_sih_do_mask(struct work_struct *work)
if (status)
pr_err("twl4030: %s, %s --> %d\n", __func__,
"write", status);
+ mutex_unlock(&agent->irq_lock);
}
-static void twl4030_sih_do_edge(struct work_struct *work)
+static void twl4030_sih_unmask(unsigned irq)
{
- struct sih_agent *agent;
- const struct sih *sih;
- u8 bytes[6];
- u32 edge_change;
- int status;
-
- agent = container_of(work, struct sih_agent, edge_work);
-
- /* see what work we have */
- spin_lock_irq(&sih_agent_lock);
- edge_change = agent->edge_change;
- agent->edge_change = 0;
- sih = edge_change ? agent->sih : NULL;
- spin_unlock_irq(&sih_agent_lock);
- if (!sih)
- return;
-
- /* Read, reserving first byte for write scratch. Yes, this
- * could be cached for some speedup ... but be careful about
- * any processor on the other IRQ line, EDR registers are
- * shared.
- */
- status = twl_i2c_read(sih->module, bytes + 1,
- sih->edr_offset, sih->bytes_edr);
- if (status) {
- pr_err("twl4030: %s, %s --> %d\n", __func__,
- "read", status);
- return;
- }
+ struct sih_agent *agent = get_irq_chip_data(irq);
+ const struct sih *sih = agent->sih;
- /* Modify only the bits we know must change */
- while (edge_change) {
- int i = fls(edge_change) - 1;
- struct irq_desc *d = irq_to_desc(i + agent->irq_base);
- int byte = 1 + (i >> 2);
- int off = (i & 0x3) * 2;
-
- if (!d) {
- pr_err("twl4030: Invalid IRQ: %d\n",
- i + agent->irq_base);
- return;
- }
+ union {
+ u8 bytes[4];
+ u32 word;
+ } imr;
- bytes[byte] &= ~(0x03 << off);
+ int status;
- raw_spin_lock_irq(&d->lock);
- if (d->status & IRQ_TYPE_EDGE_RISING)
- bytes[byte] |= BIT(off + 1);
- if (d->status & IRQ_TYPE_EDGE_FALLING)
- bytes[byte] |= BIT(off + 0);
- raw_spin_unlock_irq(&d->lock);
+ mutex_lock(&agent->irq_lock);
+ agent->imr &= ~BIT(irq - agent->irq_base);
- edge_change &= ~BIT(i);
- }
+ /* byte[0] gets overwritten as we write ... */
+ imr.word = cpu_to_le32(agent->imr << 8);
- /* Write */
- status = twl_i2c_write(sih->module, bytes,
- sih->edr_offset, sih->bytes_edr);
+ /* 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);
-}
-
-/*----------------------------------------------------------------------*/
-
-/*
- * All irq_chip methods get issued from code holding irq_desc[irq].lock,
- * which can't perform the underlying I2C operations (because they sleep).
- * So we must hand them off to a thread (workqueue) and cope with asynch
- * completion, potentially including some re-ordering, of these requests.
- */
-
-static void twl4030_sih_mask(unsigned irq)
-{
- struct sih_agent *sih = get_irq_chip_data(irq);
- unsigned long flags;
-
- spin_lock_irqsave(&sih_agent_lock, flags);
- sih->imr |= BIT(irq - sih->irq_base);
- sih->imr_change_pending = true;
- queue_work(wq, &sih->mask_work);
- spin_unlock_irqrestore(&sih_agent_lock, flags);
-}
-
-static void twl4030_sih_unmask(unsigned irq)
-{
- struct sih_agent *sih = get_irq_chip_data(irq);
- unsigned long flags;
-
- spin_lock_irqsave(&sih_agent_lock, flags);
- sih->imr &= ~BIT(irq - sih->irq_base);
- sih->imr_change_pending = true;
- queue_work(wq, &sih->mask_work);
- spin_unlock_irqrestore(&sih_agent_lock, flags);
+ mutex_unlock(&agent->irq_lock);
}
static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
{
- struct sih_agent *sih = get_irq_chip_data(irq);
- struct irq_desc *desc = irq_to_desc(irq);
- unsigned long flags;
+ struct sih_agent *agent = get_irq_chip_data(irq);
+ const struct sih *sih = agent->sih;
+ struct irq_desc *desc = irq_to_desc(irq);
+ int status = 0;
if (!desc) {
pr_err("twl4030: Invalid IRQ: %d\n", irq);
@@ -595,15 +517,67 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
- spin_lock_irqsave(&sih_agent_lock, flags);
+ mutex_lock(&agent->irq_lock);
if ((desc->status & IRQ_TYPE_SENSE_MASK) != trigger) {
+ u8 bytes[6];
+ u32 edge_change;
+
desc->status &= ~IRQ_TYPE_SENSE_MASK;
desc->status |= trigger;
- sih->edge_change |= BIT(irq - sih->irq_base);
- queue_work(wq, &sih->edge_work);
+ agent->edge_change |= BIT(irq - agent->irq_base);
+ edge_change = agent->edge_change;
+
+ /* Read, reserving first byte for write scratch. Yes, this
+ * could be cached for some speedup ... but be careful about
+ * any processor on the other IRQ line, EDR registers are
+ * shared.
+ */
+ status = twl_i2c_read(sih->module, bytes + 1,
+ sih->edr_offset, sih->bytes_edr);
+ if (status) {
+ pr_err("twl4030: %s, %s --> %d\n", __func__,
+ "read", status);
+ goto out;
+ }
+
+ /* Modify only the bits we know must change */
+ while (edge_change) {
+ int i = fls(edge_change) - 1;
+ struct irq_desc *d = irq_to_desc(i + agent->irq_base);
+ int byte = 1 + (i >> 2);
+ int off = (i & 0x3) * 2;
+
+ if (!d) {
+ pr_err("twl4030: Invalid IRQ: %d\n",
+ i + agent->irq_base);
+ status = -ENODEV;
+ goto out;
+ }
+
+ bytes[byte] &= ~(0x03 << off);
+
+ raw_spin_lock_irq(&d->lock);
+ if (d->status & IRQ_TYPE_EDGE_RISING)
+ bytes[byte] |= BIT(off + 1);
+ if (d->status & IRQ_TYPE_EDGE_FALLING)
+ bytes[byte] |= BIT(off + 0);
+ raw_spin_unlock_irq(&d->lock);
+
+ edge_change &= ~BIT(i);
+ }
+
+ /* Write */
+ status = twl_i2c_write(sih->module, bytes,
+ sih->edr_offset, sih->bytes_edr);
+ if (status)
+ pr_err("twl4030: %s, %s --> %d\n", __func__,
+ "write", status);
}
- spin_unlock_irqrestore(&sih_agent_lock, flags);
- return 0;
+
+out:
+ mutex_unlock(&agent->irq_lock);
+
+ return status;
}
static struct irq_chip twl4030_sih_irq_chip = {
@@ -706,8 +680,7 @@ int twl4030_sih_setup(int module)
agent->irq_base = irq_base;
agent->sih = sih;
agent->imr = ~0;
- INIT_WORK(&agent->mask_work, twl4030_sih_do_mask);
- INIT_WORK(&agent->edge_work, twl4030_sih_do_edge);
+ mutex_init(&agent->irq_lock);
for (i = 0; i < sih->bits; i++) {
irq = irq_base + i;
@@ -755,12 +728,6 @@ int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
if (status < 0)
return status;
- wq = create_singlethread_workqueue("twl4030-irqchip");
- if (!wq) {
- pr_err("twl4030: workqueue FAIL\n");
- return -ESRCH;
- }
-
twl4030_irq_base = irq_base;
/* install an irq handler for each of the SIH modules;
@@ -802,8 +769,7 @@ fail_rqirq:
fail:
for (i = irq_base; i < irq_end; i++)
set_irq_chip_and_handler(i, NULL, NULL);
- destroy_workqueue(wq);
- wq = NULL;
+
return status;
}
--
1.7.3.4.598.g85356
Hi,
On Tue, Dec 28, 2010 at 07:14:16PM +0200, Felipe Balbi wrote:
>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.
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 <[email protected]>
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 <[email protected]>
---
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 = 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];
@@ -483,7 +494,13 @@ static void twl4030_sih_unmask(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 ... */
@@ -497,14 +514,15 @@ static void twl4030_sih_unmask(unsigned irq)
"write", status);
}
-static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
+static int twl4030_sih_set_type(struct irq_data *data, unsigned trigger)
{
- struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
- struct irq_desc *desc = irq_to_desc(irq);
+ struct irq_desc *d = irq_data_to_desc(data);
+ struct sih_agent *agent;
+ const struct sih *sih;
int status = 0;
+ int irq = data->irq;
- if (!desc) {
+ if (!d) {
pr_err("twl4030: Invalid IRQ: %d\n", irq);
return -EINVAL;
}
@@ -512,12 +530,15 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
- if ((desc->status & IRQ_TYPE_SENSE_MASK) != trigger) {
+ agent = d->chip_data;
+ sih = agent->sih;
+
+ if ((d->status & IRQ_TYPE_SENSE_MASK) != trigger) {
u8 bytes[6];
u32 edge_change;
- desc->status &= ~IRQ_TYPE_SENSE_MASK;
- desc->status |= trigger;
+ d->status &= ~IRQ_TYPE_SENSE_MASK;
+ d->status |= trigger;
agent->edge_change |= BIT(irq - agent->irq_base);
edge_change = agent->edge_change;
@@ -537,7 +558,6 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
/* Modify only the bits we know must change */
while (edge_change) {
int i = fls(edge_change) - 1;
- struct irq_desc *d = irq_to_desc(i + agent->irq_base);
int byte = 1 + (i >> 2);
int off = (i & 0x3) * 2;
@@ -570,27 +590,43 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
return status;
}
-static void twl4030_sih_bus_lock(unsigned int irq)
+static void twl4030_sih_bus_lock(struct irq_data *data)
{
- struct sih_agent *agent = get_irq_chip_data(irq);
+ struct irq_desc *d = irq_data_to_desc(data);
+ struct sih_agent *agent;
+ int irq = data->irq;
+
+ if (!d) {
+ pr_err("twl4030: Invalid IRQ: %d\n", irq);
+ return;
+ }
+ agent = d->chip_data;
mutex_lock(&agent->irq_lock);
}
-static void twl4030_sih_bus_sync_unlock(unsigned int irq)
+static void twl4030_sih_bus_sync_unlock(struct irq_data *data)
{
- struct sih_agent *agent = get_irq_chip_data(irq);
+ struct irq_desc *d = irq_data_to_desc(data);
+ struct sih_agent *agent;
+ int irq = data->irq;
+
+ if (!d) {
+ pr_err("twl4030: Invalid IRQ: %d\n", irq);
+ return;
+ }
+ agent = d->chip_data;
mutex_unlock(&agent->irq_lock);
}
static struct irq_chip twl4030_sih_irq_chip = {
- .name = "twl4030",
- .mask = twl4030_sih_mask,
- .unmask = twl4030_sih_unmask,
- .set_type = twl4030_sih_set_type,
- .bus_lock = twl4030_sih_bus_lock,
- .bus_sync_unlock = twl4030_sih_bus_sync_unlock,
+ .name = "twl4030",
+ .irq_mask = twl4030_sih_mask,
+ .irq_unmask = twl4030_sih_unmask,
+ .irq_set_type = twl4030_sih_set_type,
+ .irq_bus_lock = twl4030_sih_bus_lock,
+ .irq_bus_sync_unlock = twl4030_sih_bus_sync_unlock,
};
/*----------------------------------------------------------------------*/
--
1.7.3.4.598.g85356
--
balbi
--- On Tue, 12/28/10, Mark Brown <[email protected]> wrote:
> F
> You shouldn't need this any more; the driver used to be
> faffing around
> with this because it wasn't using genirq for
> this in the past.
Originally it couldn't, since genirq didn't
support threaded IRQ handling...
...
>
> 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
I'd expect that genirq solve all the issues and
that its support be used. That's not the same
as dropping anything except the initial code to
handle what genirq didn't ... some locking/etc
would still mostly need doing, but where genirq
now handles it, that'd be preferable.
and the
> bus_lock() and
> bus_sync_unlock() operations would be implemented.
h
ISTR maybe four or five genirq updates in the
area of threaded IRQ management, added so that
issues the twl4030 driver needed to be solved
could be solved in generic ways.
The first was just having threaded IRQ handlers,
and another was I think removing the initial
quick'n'dirty thread-per-irq restriction; there
was no point in having a few dozen IRQ threads
in e.g. a twl4030 driver, since two could never
do constructive work concurrently.
I'm glad to see this conversion finally start.
Even if all the threaded IRQ hooks don't get used
to best advantage, it'll be an improvement.
On Tue, Dec 28, 2010 at 07:36:04PM +0200, Felipe Balbi wrote:
> 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() ?
The general idea is to move to struct irq_data sooner rather than later
(all the existing MFD drivers have already been converted).
> -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)
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.
On Tue, Dec 28, 2010 at 09:40:03AM -0800, David Brownell wrote:
> > What I'd expect to see from a conversion like this would be
> > that most of
> > the locking/IRQ management stuff would be dropped
> I'd expect that genirq solve all the issues and
> that its support be used. That's not the same
> as dropping anything except the initial code to
> handle what genirq didn't ... some locking/etc
> would still mostly need doing, but where genirq
> now handles it, that'd be preferable.
It should solve everything - there's rather a lot of I2C/SPI connected
MFDs using genirq fully now without any hassle, the APIs are really
straightforward and easy to use.
> and the
> > bus_lock() and
> > bus_sync_unlock() operations would be implemented.
> ISTR maybe four or five genirq updates in the
> area of threaded IRQ management, added so that
> issues the twl4030 driver needed to be solved
> could be solved in generic ways.
Yup - the main one on top of threaded IRQs was the bus_lock() and
bus_sync_unlock() methods.
> The first was just having threaded IRQ handlers,
> and another was I think removing the initial
> quick'n'dirty thread-per-irq restriction; there
> was no point in having a few dozen IRQ threads
> in e.g. a twl4030 driver, since two could never
> do constructive work concurrently.
The thread per IRQ thing is dealt with too, the secondary IRQs share the
thread used for demux.
On Tue, Dec 28, 2010 at 07:14:19PM +0200, Felipe Balbi wrote:
> +static void twl4030_sih_bus_sync_unlock(unsigned int irq)
> +{
> + struct sih_agent *agent = get_irq_chip_data(irq);
> +
> + mutex_unlock(&agent->irq_lock);
> +}
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
out here before dropping the mutex.
> static struct irq_chip twl4030_sih_irq_chip = {
> .name = "twl4030",
> .mask = twl4030_sih_mask,
> .unmask = twl4030_sih_unmask,
> .set_type = twl4030_sih_set_type,
> + .bus_lock = twl4030_sih_bus_lock,
> + .bus_sync_unlock = twl4030_sih_bus_sync_unlock,
> };
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,
On Tue, 2010-12-28 at 23:58 +0000, Mark Brown wrote:
> On Tue, Dec 28, 2010 at 07:14:19PM +0200, Felipe Balbi wrote:
>
> > +static void twl4030_sih_bus_sync_unlock(unsigned int irq)
> > +{
> > + struct sih_agent *agent = get_irq_chip_data(irq);
> > +
> > + mutex_unlock(&agent->irq_lock);
> > +}
>
> 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
> out here before dropping the mutex.
now that I look at some gpio chips I see what you're saying, will update
that tomorrow. Thanks
> > static struct irq_chip twl4030_sih_irq_chip = {
> > .name = "twl4030",
> > .mask = twl4030_sih_mask,
> > .unmask = twl4030_sih_unmask,
> > .set_type = twl4030_sih_set_type,
> > + .bus_lock = twl4030_sih_bus_lock,
> > + .bus_sync_unlock = twl4030_sih_bus_sync_unlock,
> > };
>
> 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.
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,
On Tue, 2010-12-28 at 17:41 +0000, Mark Brown wrote:
> > 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() ?
>
> The general idea is to move to struct irq_data sooner rather than later
> (all the existing MFD drivers have already been converted).
Great, I'll put this one together then.
> > -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)
>
> 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.
you're right, just didn't know that was such a helper. BTW, quite a big
name.
--
balbi
Hi,
On Wed, Dec 29, 2010 at 02:38:28AM +0200, Felipe Balbi wrote:
>On Tue, 2010-12-28 at 23:58 +0000, Mark Brown wrote:
>> On Tue, Dec 28, 2010 at 07:14:19PM +0200, Felipe Balbi wrote:
>>
>> > +static void twl4030_sih_bus_sync_unlock(unsigned int irq)
>> > +{
>> > + struct sih_agent *agent = get_irq_chip_data(irq);
>> > +
>> > + mutex_unlock(&agent->irq_lock);
>> > +}
>>
>> 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
>> out here before dropping the mutex.
>
>now that I look at some gpio chips I see what you're saying, will update
>that tomorrow. Thanks
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 <[email protected]>
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 <[email protected]>
---
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 &= ~BIT(irq - agent->irq_base);
-
- /* 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 int twl4030_sih_set_type(unsigned irq, unsigned trigger)
{
struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
struct irq_desc *desc = irq_to_desc(irq);
- int status = 0;
if (!desc) {
pr_err("twl4030: Invalid IRQ: %d\n", irq);
@@ -517,17 +477,57 @@ 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;
+ agent->edge_change |= BIT(irq - agent->irq_base);
desc->status &= ~IRQ_TYPE_SENSE_MASK;
desc->status |= trigger;
- agent->edge_change |= BIT(irq - agent->irq_base);
+ }
+
+ return 0;
+}
+
+static void twl4030_sih_bus_lock(unsigned int irq)
+{
+ struct sih_agent *agent = get_irq_chip_data(irq);
+
+ mutex_lock(&agent->irq_lock);
+}
+
+static void twl4030_sih_bus_sync_unlock(unsigned int irq)
+{
+ struct sih_agent *agent = get_irq_chip_data(irq);
+ const struct sih *sih = agent->sih;
+
+ int status;
+
+ union {
+ u8 bytes[4];
+ u32 word;
+ } imr;
+
+ if (agent->imr_change_pending) {
+ /* 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);
+ agent->imr_change_pending = false;
+ }
+
+ if (agent->edge_change) {
+ u32 edge_change;
+ u8 bytes[6];
+
edge_change = agent->edge_change;
+ agent->edge_change = 0;
- /* Read, reserving first byte for write scratch. Yes, this
+ /*
+ * Read, reserving first byte for write scratch. Yes, this
* could be cached for some speedup ... but be careful about
* any processor on the other IRQ line, EDR registers are
* shared.
@@ -550,7 +550,6 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (!d) {
pr_err("twl4030: Invalid IRQ: %d\n",
i + agent->irq_base);
- status = -ENODEV;
goto out;
}
@@ -576,8 +575,6 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
out:
mutex_unlock(&agent->irq_lock);
-
- return status;
}
static struct irq_chip twl4030_sih_irq_chip = {
@@ -585,6 +582,8 @@ static struct irq_chip twl4030_sih_irq_chip = {
.mask = twl4030_sih_mask,
.unmask = twl4030_sih_unmask,
.set_type = twl4030_sih_set_type,
+ .bus_lock = twl4030_sih_bus_lock,
+ .bus_sync_unlock = twl4030_sih_bus_sync_unlock,
};
/*----------------------------------------------------------------------*/
--
1.7.3.4.598.g85356
--
balbi
On Wed, Dec 29, 2010 at 02:28:08PM +0200, Felipe Balbi wrote:
> I believe you meant something like below, I'll get back to this in a
> little while. Have lots to test:
Yes, that looks like what I'd expect.
On Thu, Dec 30, 2010 at 12:18:04PM +0000, Mark Brown wrote:
>On Wed, Dec 29, 2010 at 02:28:08PM +0200, Felipe Balbi wrote:
>
>> I believe you meant something like below, I'll get back to this in a
>> little while. Have lots to test:
>
>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