Message ID | 54244243.3090904@ti.com |
---|---|
State | Not Applicable, archived |
Headers | show |
On Thu, Sep 25, 2014 at 6:26 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 09/25/2014 11:07 AM, Linus Walleij wrote: >> On Wed, Sep 24, 2014 at 2:28 PM, Grygorii Strashko >> <grygorii.strashko@ti.com> wrote: >>> On 09/24/2014 02:17 PM, Linus Walleij wrote: >> >>>> So PCA cannot use gpiochip_set_chained_irqchip()? >>> >>> Yes. It can't - pca is i2c device. >> >> ? I don't get this statement. >> >> Why does the fact that it is an I2C device matter? >> We have several devices that are in fact on top of >> I2C (albeit as MFD cells) like gpio-tc3589x.c. > > gpio-tc3589x.c doesn't call gpiochip_set_chained_irqchip() Ah yeah you're right of course. I considered adding a separate registration function for the sleeping handlers, like gpiochip_set_threaded_irqchip() that would handle setting up the nested case. >> Yes, but the .map function isn't called until a client >> wants to use an IRQ. And that won't happen until after >> we exit the whole .probe() function. > > .map function is called from irq_create_mapping() which, > in turn, can be called as from irq_domain_add_simple() - > as result .map will be always called from gpiochip_irqchip_add(). Ah yeah you're right, I was just wrong here :-/ >> Well it happens in one single driver, and was done by me. >> Maybe I should either disallow that, as that means we're adding >> multiple parents (which is what you want, right?) or actually >> implement it in a way so that multiple parents can be handled >> by the helpers, by adding the parents to a list or something. > > Sorry, but It seems the simplest way is to allow drivers to manage > parent IRQs for the complex cases. So, I vote for custom .map() > callback, but It could be not too simple to implement it, because > private driver data need to be passed as parameter to this callback > somehow. Yeah. Well what I'm thinking as subsystem maintainer, is that when I added the generic GPIOlib irqchip helpers we managed to smoke out a large:ish set of subtle bugs that different drivers had created independently of each other. So centralizing code is very important if at all possible to bring down the maintainer burden. So for that reason I'm looking a second and a third time at these things before going ahead with custom code in drivers... > === Simple one - driver need to set parent_irq before adding gpiochip === > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 8aa84d5..292840b 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -447,8 +447,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > irq_set_lockdep_class(irq, &gpiochip_irq_lock_class); > irq_set_chip_and_handler(irq, chip->irqchip, chip->irq_handler); > /* Chips that can sleep need nested thread handlers */ > - if (chip->can_sleep) > + if (chip->can_sleep) { > irq_set_nested_thread(irq, 1); > + if (chip->parent_irq) > + irq_set_parent(irq, chip->parent_irq); > + } Yeah I need to think of something like this... Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all 2014-09-26 11:07 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Sep 25, 2014 at 6:26 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: >> On 09/25/2014 11:07 AM, Linus Walleij wrote: >>> On Wed, Sep 24, 2014 at 2:28 PM, Grygorii Strashko >>> <grygorii.strashko@ti.com> wrote: >>>> On 09/24/2014 02:17 PM, Linus Walleij wrote: >>> >>>>> So PCA cannot use gpiochip_set_chained_irqchip()? >>>> >>>> Yes. It can't - pca is i2c device. >>> >>> ? I don't get this statement. >>> >>> Why does the fact that it is an I2C device matter? >>> We have several devices that are in fact on top of >>> I2C (albeit as MFD cells) like gpio-tc3589x.c. >> >> gpio-tc3589x.c doesn't call gpiochip_set_chained_irqchip() > > Ah yeah you're right of course. I considered adding a separate > registration function for the sleeping handlers, like > gpiochip_set_threaded_irqchip() that would handle > setting up the nested case. > >>> Yes, but the .map function isn't called until a client >>> wants to use an IRQ. And that won't happen until after >>> we exit the whole .probe() function. >> >> .map function is called from irq_create_mapping() which, >> in turn, can be called as from irq_domain_add_simple() - >> as result .map will be always called from gpiochip_irqchip_add(). > > Ah yeah you're right, I was just wrong here :-/ > >>> Well it happens in one single driver, and was done by me. >>> Maybe I should either disallow that, as that means we're adding >>> multiple parents (which is what you want, right?) or actually >>> implement it in a way so that multiple parents can be handled >>> by the helpers, by adding the parents to a list or something. >> >> Sorry, but It seems the simplest way is to allow drivers to manage >> parent IRQs for the complex cases. So, I vote for custom .map() >> callback, but It could be not too simple to implement it, because >> private driver data need to be passed as parameter to this callback >> somehow. > > Yeah. Well what I'm thinking as subsystem maintainer, is that > when I added the generic GPIOlib irqchip helpers we managed to > smoke out a large:ish set of subtle bugs that different drivers > had created independently of each other. > > So centralizing code is very important if at all possible to bring > down the maintainer burden. > > So for that reason I'm looking a second and a third time at these > things before going ahead with custom code in drivers... > >> === Simple one - driver need to set parent_irq before adding gpiochip === >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index 8aa84d5..292840b 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -447,8 +447,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, >> irq_set_lockdep_class(irq, &gpiochip_irq_lock_class); >> irq_set_chip_and_handler(irq, chip->irqchip, chip->irq_handler); >> /* Chips that can sleep need nested thread handlers */ >> - if (chip->can_sleep) >> + if (chip->can_sleep) { >> irq_set_nested_thread(irq, 1); >> + if (chip->parent_irq) >> + irq_set_parent(irq, chip->parent_irq); >> + } > > Yeah I need to think of something like this... > I did run into exact the same situation with a 4.1.1 kernel :) [ 312.863047] ------------[ cut here ]------------ [ 312.867681] WARNING: CPU: 1 PID: 12 at kernel/irq/manage.c:696 irq_nested_primary_handler+0x30/0x38() [ 312.876901] Primary handler called for nested irq 301 [ 312.881953] Modules linked in: uinput ipv6 smsc95xx usbnet mii imx2_wdt etnaviv(C) matrix_keypad matrix_keymap ar1021_i2c [ 312.893067] CPU: 1 PID: 12 Comm: ksoftirqd/1 Tainted: G WC 4.1.1 #9 [ 312.900378] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 312.906906] Backtrace: [ 312.909377] [<c0013298>] (dump_backtrace) from [<c0013488>] (show_stack+0x20/0x24) [ 312.916947] r6:00000009 r5:ffffffff r4:00000000 r3:dc8ba301 [ 312.922673] [<c0013468>] (show_stack) from [<c05743c4>] (dump_stack+0x70/0xc0) [ 312.929924] [<c0574354>] (dump_stack) from [<c002b7b8>] (warn_slowpath_common+0x88/0xc0) [ 312.938029] r5:000002b8 r4:00000000 [ 312.941678] [<c002b730>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x40/0x48) [ 312.950391] r8:00000000 r7:c07b1314 r6:0000012d r5:eea07720 r4:c3048840 [ 312.957233] [<c002b870>] (warn_slowpath_fmt) from [<c0075798>] (irq_nested_primary_handler+0x30/0x38) [ 312.966467] r3:0000012d r2:c06b9128 [ 312.970113] [<c0075768>] (irq_nested_primary_handler) from [<c0075200>] (handle_irq_event_percpu+0x70/0x2d0) [ 312.979967] [<c0075190>] (handle_irq_event_percpu) from [<c00754ac>] (handle_irq_event+0x4c/0x6c) [ 312.988854] r10:00000002 r9:00000018 r8:00000000 r7:c07b1314 r6:c3048840 r5:eea07720 [ 312.996812] r4:eea076c0 [ 312.999395] [<c0075460>] (handle_irq_event) from [<c0078204>] (handle_simple_irq+0xa4/0xc8) [ 313.007760] r6:c07c5404 r5:eea07720 r4:eea076c0 r3:00000003 [ 313.013536] [<c0078160>] (handle_simple_irq) from [<c0077cd4>] (resend_irqs+0x50/0x7c) [ 313.021468] r5:c07c5404 r4:0000012d [ 313.025119] [<c0077c84>] (resend_irqs) from [<c002f99c>] (tasklet_action+0x94/0x140) [ 313.032877] r6:00000000 r5:c07c5444 r4:c07c5440 r3:c0077c84 [ 313.038655] [<c002f908>] (tasklet_action) from [<c002eea8>] (__do_softirq+0xa0/0x3c8) [ 313.046500] r8:c07c1908 r7:00000006 r6:00000001 r5:00000040 r4:c07ba098 r3:c002f908 [ 313.054387] [<c002ee08>] (__do_softirq) from [<c002f208>] (run_ksoftirqd+0x38/0x54) [ 313.062058] r10:00000002 r9:00000000 r8:c07c1908 r7:00000000 r6:00000001 r5:00000001 [ 313.070017] r4:ee893980 [ 313.072605] [<c002f1d0>] (run_ksoftirqd) from [<c004b1e4>] (smpboot_thread_fn+0x1f8/0x2f0) [ 313.080906] [<c004afec>] (smpboot_thread_fn) from [<c0047744>] (kthread+0xe8/0x104) [ 313.088578] r10:00000000 r8:00000000 r7:c004afec r6:ee893980 r5:00000000 r4:ee893a40 [ 313.096558] [<c004765c>] (kthread) from [<c000fac8>] (ret_from_fork+0x14/0x2c) [ 313.103796] r7:00000000 r6:00000000 r5:c004765c r4:ee893a40 [ 313.109560] ---[ end trace 96052cda48865769 ]--- Linus, what is the state of the your last thinking about this topic? greets -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
=== Simple one - driver need to set parent_irq before adding gpiochip === diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 8aa84d5..292840b 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -447,8 +447,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, irq_set_lockdep_class(irq, &gpiochip_irq_lock_class); irq_set_chip_and_handler(irq, chip->irqchip, chip->irq_handler); /* Chips that can sleep need nested thread handlers */ - if (chip->can_sleep) + if (chip->can_sleep) { irq_set_nested_thread(irq, 1); + if (chip->parent_irq) + irq_set_parent(irq, chip->parent_irq); + } #ifdef CONFIG_ARM set_irq_flags(irq, IRQF_VALID); #else diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 976276a..88486c7 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -112,6 +112,7 @@ struct gpio_chip { struct irq_domain *irqdomain; irq_flow_handler_t irq_handler; unsigned int irq_default_type; + int parent_irq; #endif #if defined(CONFIG_OF_GPIO)