On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote: > On Fri, Mar 10, 2017 at 5:26 PM, Thierry Reding > <thierry.reding@xxxxxxxxx> wrote: > > > From: Thierry Reding <treding@xxxxxxxxxx> > > > > Tegra186 has two GPIO controllers that are largely register compatible > > between one another but are completely different from the controller > > found on earlier generations. > > > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> > > --- > > Changes in v2: > > So this still doesn't use GPIOLIB_IRQCHIP even though I pointed > out that you can assign several parent interrupts to the same > irqchip. > > For a recent example see: > http://marc.info/?l=devicetree&m=149012117004066&w=2 > (Notice Gregory's elegant manipulation of the mask.) > > My earlier reply here: > > ---------------------- > >> I would prefer if you could try to convert this driver to use > >> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt > >> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip(). > >> It would save us so much trouble and so much complicated > >> code to maintain for this custom irqdomain. > >> > >> I suggest you to look into the mechanisms mentioned in my > >> previous mail for how to poke holes in the single linear > >> irqdomain used by this mechanism. > >> > >> As it seems, you only have one parent interrupt with all > >> these IRQs cascading off it as far as I can tell. > > > > Like I said in other email, I don't think this will work because the > > GPIOLIB_IRQCHIP support seems to be limited to cases where a single > > parent interrupt is used. We could possibly live with a single IRQ > > handler and data, but we definitely need to request multiple IRQs if > > we want to be able to use all GPIOs as interrupts. > > Sorry if I missed that. > > Actually it works. > > If you look at gpiochip_set_chained_irqchip() it just calls > irq_set_chained_handler_and_data() for the parent interrupt. > ------------------------ > > Just call gpiochip_set_chained_irqchip() for all the irqs, and > figure out a way to get the right interrupt hardware offset in the > interrupt handler. Okay, let me quote the gpiochip_set_chained_irqchip() function here and explain why I think it's not a proper fit here. This is actually quoting gpiochip_set_cascaded_irqchip(), but that's what ends up being called. > static void gpiochip_set_cascaded_irqchip(struct gpio_chip *gpiochip, > struct irq_chip *irqchip, > int parent_irq, > irq_flow_handler_t parent_handler) > { > unsigned int offset; > > if (!gpiochip->irqdomain) { > chip_err(gpiochip, "called %s before setting up irqchip\n", > __func__); > return; > } > > if (parent_handler) { > if (gpiochip->can_sleep) { > chip_err(gpiochip, > "you cannot have chained interrupts on a " > "chip that may sleep\n"); > return; > } > /* > * The parent irqchip is already using the chip_data for this > * irqchip, so our callbacks simply use the handler_data. > */ > irq_set_chained_handler_and_data(parent_irq, parent_handler, > gpiochip); > > gpiochip->irq_chained_parent = parent_irq; If I call this multiple times with different parent_irq parameters, then only the last one will be stored in gpiochip->irq_chained_parent. Later on, this is used to disconnect the chained handler and data upon GPIO chip removal, which means that handler and data for all but the last one end up dangling. > } > > /* Set the parent IRQ for all affected IRQs */ > for (offset = 0; offset < gpiochip->ngpio; offset++) { > if (!gpiochip_irqchip_irq_valid(gpiochip, offset)) > continue; > irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset), > parent_irq); > } > } Similarly here, we'd be setting the parent IRQ of all associated GPIOs to the first, second... last parent IRQ. This is completely unnecessary work and it's also setting the wrong parent. Note that we don't actually care about that in the driver right now, but it's still wrong. > > +config GPIO_TEGRA186 > > + tristate "NVIDIA Tegra186 GPIO support" > > + default ARCH_TEGRA_186_SOC > > + depends on ARCH_TEGRA_186_SOC || COMPILE_TEST > > + depends on OF_GPIO > > select GPIOLIB_IRQCHIP > > > +#include <linux/gpio/driver.h> > > +#include <linux/gpio.h> > > Only <linux/gpio/driver.h> > > You should not use <linux/gpio.h> in drivers, then you are > doing something wrong. Yeah, I don't seem to need this at all. > > +struct tegra_gpio { > > + struct gpio_chip gpio; > > + struct irq_chip intc; > > + unsigned int num_irq; > > + unsigned int *irq; > > Why are you keeping the irqs around after requesting? > Use devm_* First I prefer to keep resource request and driver initialization separate because it makes error recovery more robust. So this is used to first store the results from platform_get_irq() and later on to iterate over when installing the chained handlers. Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I need to keep these around in order to properly uninstall the handlers again on removal. > > + > > + const struct tegra_gpio_soc *soc; > > + > > + void __iomem *base; > > + > > + struct irq_domain *domain; > > I don't like custom irq domains it is messy. > Please do your best to try to use GPIOLIB_IRQCHIP Like I explained above, I don't think it works the way it is supposed to for this case. The armada-37xx patch that you linked to earlier seems to suffer from the same issues in that all but the last parent IRQ handlers will be left dangling after removal, which would cause the kernel to run non-existing code if by any chance the interrupts were to still fire for some reason. > If you still decide to go with a custom irqdomain, there is > stuff missing, especially the gpiochip_lock_as_irq() > calls from the irqchip .irq_request/release_resources() > callbacks, see the gpiolib.c implementation in the > helpers there for reference. Good catch. I will add those. Thierry
Attachment:
signature.asc
Description: PGP signature
