| Message ID | 1397544229-20545-4-git-send-email-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 15, 2014 at 8:43 AM, Barry Song <21cnbao@gmail.com> wrote: > From: Barry Song <Baohua.Song@csr.com> > > since it is called handler_data, drivers should have the ability to > set handler_data based on real hardware. > GPIOLIB_IRQCHIP doesn't use handler_data by itself, so it will not > break GPIO core. > > Signed-off-by: Barry Song <Baohua.Song@csr.com> NAK, the whole idea with the function gpiochip_set_chained_irqchip() is that its sets things up so that the gpiochip is passed as handler data on the parent IRQ, akin to how the gpiochip is passed as chip data on the cascaded IRQs. Yours, Linus Walleij
2014-04-24 4:13 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Apr 15, 2014 at 8:43 AM, Barry Song <21cnbao@gmail.com> wrote: > >> From: Barry Song <Baohua.Song@csr.com> >> >> since it is called handler_data, drivers should have the ability to >> set handler_data based on real hardware. >> GPIOLIB_IRQCHIP doesn't use handler_data by itself, so it will not >> break GPIO core. >> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> > > NAK, the whole idea with the function gpiochip_set_chained_irqchip() > is that its sets things up so that the gpiochip is passed as handler > data on the parent IRQ, akin to how the gpiochip is passed as > chip data on the cascaded IRQs. i think it is a really bad idea as the parent handler might not want the whole chip if the chip has several parents and each parent want a separate handler_data. this patch doesn't break your way as the parameter is void *, drivers which use your way don't need any change. but for drivers which want more special handler_data, it supports better. > > Yours, > Linus Walleij -barry
On Thu, Apr 24, 2014 at 12:55 AM, Barry Song <21cnbao@gmail.com> wrote: >> NAK, the whole idea with the function gpiochip_set_chained_irqchip() >> is that its sets things up so that the gpiochip is passed as handler >> data on the parent IRQ, akin to how the gpiochip is passed as >> chip data on the cascaded IRQs. > > i think it is a really bad idea as the parent handler might not want > the whole chip if the chip has several parents and each parent want a > separate handler_data. > this patch doesn't break your way as the parameter is void *, drivers > which use your way don't need any change. but for drivers which want > more special handler_data, it supports better. This is perfectly possible, just don't use gpiochip_set_chained_irqchip() which has this semantic effect. Use irq_set_chained_handler() and irq_set_handler_data() directly instead. I do see that this is maybe easier for the handler to get the bank passed in. However when I in the RFT patches change the driver to allocate and pass a struct for the gpio chip, it all looks a bit different. You would have to get the sgpio from the bank instead, which may require to introduce a special pointer for that. What we want to get rid of is this: for (i = 0; i < SIRFSOC_GPIO_BANK_SIZE; i++) { bank = &sgpio->sgpio_bank[i]; if (bank->parent_irq == irq) break; } BUG_ON (i == SIRFSOC_GPIO_BANK_SIZE); So what about passing something like that to the handler: struct sirf_irq_handler_data { struct sirfsoc_gpio_chip *sgpio; struct sirfsoc_gpio_bank *bank; }; Then the irq handler instantly have pointers to all it needs. But maybe it's better to just pass the bank, whatdoIknow. Yours, Linus Walleij
2014-04-24 7:09 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Apr 24, 2014 at 12:55 AM, Barry Song <21cnbao@gmail.com> wrote: > >>> NAK, the whole idea with the function gpiochip_set_chained_irqchip() >>> is that its sets things up so that the gpiochip is passed as handler >>> data on the parent IRQ, akin to how the gpiochip is passed as >>> chip data on the cascaded IRQs. >> >> i think it is a really bad idea as the parent handler might not want >> the whole chip if the chip has several parents and each parent want a >> separate handler_data. >> this patch doesn't break your way as the parameter is void *, drivers >> which use your way don't need any change. but for drivers which want >> more special handler_data, it supports better. > > This is perfectly possible, just don't use > gpiochip_set_chained_irqchip() which has this semantic > effect. > > Use irq_set_chained_handler() and irq_set_handler_data() > directly instead. > > I do see that this is maybe easier for the handler to get the bank > passed in. > > However when I in the RFT patches change the driver to allocate > and pass a struct for the gpio chip, it all looks a bit different. > You would have to get the sgpio from the bank instead, which > may require to introduce a special pointer for that. > > What we want to get rid of is this: > > for (i = 0; i < SIRFSOC_GPIO_BANK_SIZE; i++) { > bank = &sgpio->sgpio_bank[i]; > if (bank->parent_irq == irq) > break; > } > BUG_ON (i == SIRFSOC_GPIO_BANK_SIZE); > > So what about passing something like that to the handler: > > struct sirf_irq_handler_data { > struct sirfsoc_gpio_chip *sgpio; > struct sirfsoc_gpio_bank *bank; > }; > > Then the irq handler instantly have pointers to all it needs. > But maybe it's better to just pass the bank, whatdoIknow. only if we move to irq_set_chained_handler() and irq_set_handler_data() directly and set bank-specific handler_data manually, it works. my point is that will we make the gpiochip_set_chained_irqchip() more general for this kind of cases too since you have had a good API to wrap things? > > Yours, > Linus Walleij -barry
On Thu, Apr 24, 2014 at 1:23 AM, Barry Song <21cnbao@gmail.com> wrote: > my point is that will we make the gpiochip_set_chained_irqchip() more > general for this kind of cases too since you have had a good API to > wrap things? As it only wraps two function calls I'm leaning toward deleting it as it might create more complexity than it removes. Not sure. We don't have a similar handler for registering nested threaded irq handlers so it's not really helpful. Yours, Linus Walleij
2014-04-24 21:06 GMT+08:00, Linus Walleij <linus.walleij@linaro.org>: > On Thu, Apr 24, 2014 at 1:23 AM, Barry Song <21cnbao@gmail.com> wrote: > >> my point is that will we make the gpiochip_set_chained_irqchip() more >> general for this kind of cases too since you have had a good API to >> wrap things? > > As it only wraps two function calls I'm leaning toward deleting it as it > might create more complexity than it removes. Not sure. i do agree that we can remove it if it doesn't really support general gpiochip. i think it is popular that some gpiochip has several irq parents. so people might get confused by this API as it actually doesn't really help many. > > We don't have a similar handler for registering nested threaded > irq handlers so it's not really helpful. > > Yours, > Linus Walleij > -barry -barry
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 761013f..4b90d14 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1351,24 +1351,20 @@ static struct gpio_chip *find_chip_by_name(const char *name) /** * gpiochip_add_chained_irqchip() - adds a chained irqchip to a gpiochip - * @gpiochip: the gpiochip to add the irqchip to + * @handler_data: handler_data which will be used by ISR of interrupt parent * @irqchip: the irqchip to add to the gpiochip * @parent_irq: the irq number corresponding to the parent IRQ for this * chained irqchip * @parent_handler: the parent interrupt handler for the accumulated IRQ * coming out of the gpiochip */ -void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip, +void gpiochip_set_chained_irqchip(void *handler_data, struct irq_chip *irqchip, int parent_irq, irq_flow_handler_t parent_handler) { irq_set_chained_handler(parent_irq, parent_handler); - /* - * The parent irqchip is already using the chip_data for this - * irqchip, so our callbacks simply use the handler_data. - */ - irq_set_handler_data(parent_irq, gpiochip); + irq_set_handler_data(parent_irq, handler_data); } EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 1827b43..52a0758 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -207,7 +207,7 @@ void gpiod_add_lookup_table(struct gpiod_lookup_table *table); #ifdef CONFIG_GPIOLIB_IRQCHIP -void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip, +void gpiochip_set_chained_irqchip(void *handler_data, struct irq_chip *irqchip, int parent_irq, irq_flow_handler_t parent_handler);