Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler
On Tue, 23 Jul 2013, Grygorii Strashko wrote:
From: Naga Venkata Srikanth V vnv.srika...@samsung.com
1) Removed request_irq() and replaced it with request_threaded_irq().
2) Removed generic_handle_irq() and replaced it with
handle_nested_irq().
Handling of these interrupts is nested, as we are handling an
interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
3) Removed I2C read-retry logic for the case when twl_i2c_read() is
failed inside IRQ handler - there is no sense to do that, so just report
an error and return.
Signed-off-by: Naga Venkata Srikanth V vnv.srika...@samsung.com
Signed-off-by: Oleg_Kosheliev oleg.koshel...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
drivers/mfd/twl6030-irq.c | 146
+++--
1 file changed, 49 insertions(+), 97 deletions(-)
Besides the points I mention below I like the way this patch is
going.
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 277a8db..b6030d9 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
static int twl_irq;
static bool twl_irq_wake_enabled;
-static struct completion irq_event;
static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
@@ -131,95 +130,57 @@ static struct notifier_block
twl6030_irq_pm_notifier_block = {
};
/*
- * This thread processes interrupts reported by the Primary Interrupt
Handler.
- */
-static int twl6030_irq_thread(void *data)
+* Threaded irq handler for the twl6030 interrupt.
+* We query the interrupt controller in the twl6030 to determine
+* which module is generating the interrupt request and call
+* handle_nested_irq for that module.
+*/
+static irqreturn_t twl6030_irq_thread(int irq, void *data)
{
- long irq = (long)data;
- static unsigned i2c_errors;
- static const unsigned max_i2c_errors = 100;
- int ret;
-
- while (!kthread_should_stop()) {
- int i;
- union {
+ int i, ret;
+ 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;
+ /* 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_warn(%s: I2C error %d reading PIH ISR\n, __func__, ret);
Does the user really care which function we're returning from.
Would it be better if you replace '__func__' with the device name?
+ return IRQ_HANDLED;
+ }
- sts.bytes[3] = 0; /* Only 24 bits are valid*/
+ sts.bytes[3] = 0; /* Only 24 bits are valid*/
- /*
- * Since VBUS status bit is not reliable for VBUS disconnect
- * use CHARGER VBUS detection status bit instead.
- */
- if (sts.bytes[2] 0x10)
- sts.bytes[2] |= 0x08;
+ /*
+ * Since VBUS status bit is not reliable for VBUS disconnect
+ * use CHARGER VBUS detection status bit instead.
+ */
+ if (sts.bytes[2] 0x10)
+ sts.bytes[2] |= 0x08;
- 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 +
+ for (i = 0; sts.int_sts; sts.int_sts = 1, i++)
+ if (sts.int_sts 0x1) {
I'm a little confused by this. Where does sts.int_sts come from?
+ int module_irq = twl6030_irq_base +
twl6030_interrupt_mapping[i];
- generic_handle_irq(module_irq);
-
- }
- local_irq_enable();
+ handle_nested_irq(module_irq);
+ pr_debug(%s: PIH ISR %u, virq%u\n,
+ __func__, i, module_irq);
}
Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler
On Tue, 23 Jul 2013, Grygorii Strashko wrote:
From: Naga Venkata Srikanth V vnv.srika...@samsung.com
1) Removed request_irq() and replaced it with request_threaded_irq().
2) Removed generic_handle_irq() and replaced it with
handle_nested_irq().
Handling of these interrupts is nested, as we are handling an
interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
3) Removed I2C read-retry logic for the case when twl_i2c_read() is
failed inside IRQ handler - there is no sense to do that, so just report
an error and return.
Signed-off-by: Naga Venkata Srikanth V vnv.srika...@samsung.com
Signed-off-by: Oleg_Kosheliev oleg.koshel...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
drivers/mfd/twl6030-irq.c | 146
+++--
1 file changed, 49 insertions(+), 97 deletions(-)
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
snip
+ status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
+ IRQF_ONESHOT, TWL6030-PIH, NULL);
Oh, and please use managed resources for this: devm_*
if (status 0) {
dev_err(dev, could not claim irq %d: %d\n, irq_num, status);
goto fail_irq;
}
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler
On 07/24/2013 01:49 PM, Lee Jones wrote:
On Tue, 23 Jul 2013, Grygorii Strashko wrote:
From: Naga Venkata Srikanth V vnv.srika...@samsung.com
1) Removed request_irq() and replaced it with request_threaded_irq().
2) Removed generic_handle_irq() and replaced it with
handle_nested_irq().
Handling of these interrupts is nested, as we are handling an
interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
3) Removed I2C read-retry logic for the case when twl_i2c_read() is
failed inside IRQ handler - there is no sense to do that, so just report
an error and return.
Signed-off-by: Naga Venkata Srikanth V vnv.srika...@samsung.com
Signed-off-by: Oleg_Kosheliev oleg.koshel...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
drivers/mfd/twl6030-irq.c | 146 +++--
1 file changed, 49 insertions(+), 97 deletions(-)
Besides the points I mention below I like the way this patch is
going.
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 277a8db..b6030d9 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
static int twl_irq;
static bool twl_irq_wake_enabled;
-static struct completion irq_event;
static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
@@ -131,95 +130,57 @@ static struct notifier_block
twl6030_irq_pm_notifier_block = {
};
/*
- * This thread processes interrupts reported by the Primary Interrupt Handler.
- */
-static int twl6030_irq_thread(void *data)
+* Threaded irq handler for the twl6030 interrupt.
+* We query the interrupt controller in the twl6030 to determine
+* which module is generating the interrupt request and call
+* handle_nested_irq for that module.
+*/
+static irqreturn_t twl6030_irq_thread(int irq, void *data)
{
- long irq = (long)data;
- static unsigned i2c_errors;
- static const unsigned max_i2c_errors = 100;
- int ret;
-
- while (!kthread_should_stop()) {
- int i;
- union {
+ int i, ret;
+ 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;
+ /* 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);
sts.int_sts - is filled here
+ if (ret) {
+ pr_warn(%s: I2C error %d reading PIH ISR\n, __func__, ret);
Does the user really care which function we're returning from.
Would it be better if you replace '__func__' with the device name?
This module hasn't been converted to the device yet:(
(I mean interrupt-controller).
But I'm thinking about it as the next step :) and then It will be
absolutely reasonable change to replace pr_*() with dev_*() and remove
__func__.
Now, the pointer on dev (in our case twl-core device) isn't passed
in IRQ handler, so It can't be used here.
Of course it can be done, but would it make code better?
My opinion - no.
+ return IRQ_HANDLED;
+ }
- sts.bytes[3] = 0; /* Only 24 bits are valid*/
+ sts.bytes[3] = 0; /* Only 24 bits are valid*/
- /*
-* Since VBUS status bit is not reliable for VBUS disconnect
-* use CHARGER VBUS detection status bit instead.
-*/
- if (sts.bytes[2] 0x10)
- sts.bytes[2] |= 0x08;
+ /*
+* Since VBUS status bit is not reliable for VBUS disconnect
+* use CHARGER VBUS detection status bit instead.
+*/
+ if (sts.bytes[2] 0x10)
+ sts.bytes[2] |= 0x08;
- 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 +
+ for (i = 0; sts.int_sts; sts.int_sts = 1, i++)
+ if (sts.int_sts 0x1) {
Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler
+ if (ret) {
+ pr_warn(%s: I2C error %d reading PIH ISR\n, __func__, ret);
Does the user really care which function we're returning from.
Would it be better if you replace '__func__' with the device name?
This module hasn't been converted to the device yet:(
(I mean interrupt-controller).
But I'm thinking about it as the next step :) and then It will be
absolutely reasonable change to replace pr_*() with dev_*() and
remove __func__.
I don't mean anything as compicated as that for 'this' patch. (NB: See my
comment in subsequent patches about creating a 'struct twl6030' where
you could store 'struct dev'.) In this patch I mean litterally
replacing %s: , with tw16030_irq: . Simples. :)
Now, the pointer on dev (in our case twl-core device) isn't passed
in IRQ handler, so It can't be used here.
Of course it can be done, but would it make code better?
My opinion - no.
+ if (sts.bytes[2] 0x10)
+ sts.bytes[2] |= 0x08;
- 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 +
+ for (i = 0; sts.int_sts; sts.int_sts = 1, i++)
+ if (sts.int_sts 0x1) {
I'm a little confused by this. Where does sts.int_sts come from?
See my comment above, pls
Okay, that's my fault for not understanding unions properly as I've
never had to use one, but now I do, thanks.
@@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
{
unregister_pm_notifier(twl6030_irq_pm_notifier_block);
- if (twl6030_irq_base) {
+ if (!twl6030_irq_base) {
pr_err(twl6030: can't yet clean up IRQs?\n);
return -ENOSYS;
}
+
+ free_irq(twl_irq, NULL);
+
If request_threaded_irq() fails, isn't there a chance that
twl6030_irq_base will be allocated, but twl_irq will still be
undefined?
Yes. A mess is here (historically:), thanks. Will use twl_irq
instead of twl6030_irq_base (I did it, actually, in patch [3]:).
Yes, I saw it. It would be better if you still fixed up this patch to
be correct though. Even if you break it out and add it as [PATCH 1/x].
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler
On 07/24/2013 03:50 PM, Lee Jones wrote:
+ if (ret) {
+ pr_warn(%s: I2C error %d reading PIH ISR\n, __func__, ret);
Does the user really care which function we're returning from.
Would it be better if you replace '__func__' with the device name?
This module hasn't been converted to the device yet:(
(I mean interrupt-controller).
But I'm thinking about it as the next step :) and then It will be
absolutely reasonable change to replace pr_*() with dev_*() and
remove __func__.
I don't mean anything as compicated as that for 'this' patch. (NB: See my
comment in subsequent patches about creating a 'struct twl6030' where
you could store 'struct dev'.) In this patch I mean litterally
replacing %s: , with tw16030_irq: . Simples. :)
Ok. I understand it now - will redo.
Now, the pointer on dev (in our case twl-core device) isn't passed
in IRQ handler, so It can't be used here.
Of course it can be done, but would it make code better?
My opinion - no.
+ if (sts.bytes[2] 0x10)
+ sts.bytes[2] |= 0x08;
- 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 +
+ for (i = 0; sts.int_sts; sts.int_sts = 1, i++)
+ if (sts.int_sts 0x1) {
I'm a little confused by this. Where does sts.int_sts come from?
See my comment above, pls
Okay, that's my fault for not understanding unions properly as I've
never had to use one, but now I do, thanks.
@@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
{
unregister_pm_notifier(twl6030_irq_pm_notifier_block);
- if (twl6030_irq_base) {
+ if (!twl6030_irq_base) {
pr_err(twl6030: can't yet clean up IRQs?\n);
return -ENOSYS;
}
+
+ free_irq(twl_irq, NULL);
+
If request_threaded_irq() fails, isn't there a chance that
twl6030_irq_base will be allocated, but twl_irq will still be
undefined?
Yes. A mess is here (historically:), thanks. Will use twl_irq
instead of twl6030_irq_base (I did it, actually, in patch [3]:).
Yes, I saw it. It would be better if you still fixed up this patch to
be correct though. Even if you break it out and add it as [PATCH 1/x].
ok
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler
From: Naga Venkata Srikanth V vnv.srika...@samsung.com
1) Removed request_irq() and replaced it with request_threaded_irq().
2) Removed generic_handle_irq() and replaced it with
handle_nested_irq().
Handling of these interrupts is nested, as we are handling an
interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
3) Removed I2C read-retry logic for the case when twl_i2c_read() is
failed inside IRQ handler - there is no sense to do that, so just report
an error and return.
Signed-off-by: Naga Venkata Srikanth V vnv.srika...@samsung.com
Signed-off-by: Oleg_Kosheliev oleg.koshel...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
drivers/mfd/twl6030-irq.c | 146 +++--
1 file changed, 49 insertions(+), 97 deletions(-)
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 277a8db..b6030d9 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
static int twl_irq;
static bool twl_irq_wake_enabled;
-static struct completion irq_event;
static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
@@ -131,95 +130,57 @@ static struct notifier_block
twl6030_irq_pm_notifier_block = {
};
/*
- * This thread processes interrupts reported by the Primary Interrupt Handler.
- */
-static int twl6030_irq_thread(void *data)
+* Threaded irq handler for the twl6030 interrupt.
+* We query the interrupt controller in the twl6030 to determine
+* which module is generating the interrupt request and call
+* handle_nested_irq for that module.
+*/
+static irqreturn_t twl6030_irq_thread(int irq, void *data)
{
- long irq = (long)data;
- static unsigned i2c_errors;
- static const unsigned max_i2c_errors = 100;
- int ret;
-
- while (!kthread_should_stop()) {
- int i;
- union {
+ int i, ret;
+ 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;
+ /* 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_warn(%s: I2C error %d reading PIH ISR\n, __func__, ret);
+ return IRQ_HANDLED;
+ }
- sts.bytes[3] = 0; /* Only 24 bits are valid*/
+ sts.bytes[3] = 0; /* Only 24 bits are valid*/
- /*
-* Since VBUS status bit is not reliable for VBUS disconnect
-* use CHARGER VBUS detection status bit instead.
-*/
- if (sts.bytes[2] 0x10)
- sts.bytes[2] |= 0x08;
+ /*
+* Since VBUS status bit is not reliable for VBUS disconnect
+* use CHARGER VBUS detection status bit instead.
+*/
+ if (sts.bytes[2] 0x10)
+ sts.bytes[2] |= 0x08;
- 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 +
+ for (i = 0; sts.int_sts; sts.int_sts = 1, i++)
+ if (sts.int_sts 0x1) {
+ int module_irq = twl6030_irq_base +
twl6030_interrupt_mapping[i];
- generic_handle_irq(module_irq);
-
- }
- local_irq_enable();
+ handle_nested_irq(module_irq);
+ pr_debug(%s: PIH ISR %u, virq%u\n,
+__func__, i, module_irq);
}
- /*
-* NOTE:
-* Simulation confirms that documentation is wrong w.r.t the
-* interrupt status clear operation. A single *byte* write to
-* any one of STS_A to STS_C register results in all three
-* STS registers being
