Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

From: Grygorii Strashko
Date: Wed Jul 24 2013 - 09:18:50 EST

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)

- 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

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


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at