Hi Lina, thanks for your comments! On Wed, Jun 26, 2019 at 10:09 PM Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote: > Thanks for the patch Linus. I was running into the warning in > gpiochip_set_irq_hooks(), because it was called from two places. > Hopefully, this will fix that as well. I will give it a try. That's usually when creating two irqchips from a static config. The most common solution is to put struct irq_chip into the driver state container and assign all functions dynamically so the irqchip is a "live" struct if you see how I mean. This is needed because the gpiolib irqchip core will augment some of the pointers in the irqchip, so if that is done twice, it feels a bit shaky. > On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote: > >+ girq->num_parents = 1; > >+ girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents), > >+ GFP_KERNEL); > > Could this be folded into the gpiolib? It is part of the existing API for providing an irq_chip along with the gpio_chip but you are right, it's kludgy. I do have a patch like this, making the parents a static sized field simply: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=devel-gpio-irqchip So I might go on this approach. (In a separate patch.) > >+ /* Get a pointer to the gpio_irq_chip */ > >+ girq = &g->gc.irq; > >+ girq->chip = &g->irq; > >+ girq->default_type = IRQ_TYPE_NONE; > >+ girq->handler = handle_bad_irq; > >+ girq->fwnode = g->fwnode; > >+ girq->parent_domain = parent; > >+ girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq; > >+ > Should be the necessary, if the driver implements it's own .alloc? The idea is that when using GPIOLIB_IRQCHIP and IRQ_DOMAIN_HIERARCHY together, the drivers utilizing GPIOLIB_IRQCHIP will rely on the .alloc() and .translate() implementations in gpiolib so the ambition is that these should cover all hierarchical use cases. > >+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc) > >+{ > >+ if (!gc->irq.parent_domain) { > >+ chip_err(gc, "missing parent irqdomain\n"); > >+ return -EINVAL; > >+ } > >+ > >+ if (!gc->irq.parent_domain || > >+ !gc->irq.child_to_parent_hwirq || > > This should probably be validated if the .ops have not been set. Yeah the idea here is a pretty imperialistic one: the gpiolib always provide the ops for hierarchical IRQ. The library implementation should cover all needs of all consumers, for the hierarchical case. I might be wrong, but then I need to see some example of hierarchies that need something more than what the gpiolib core is providing and idiomatic enough that it can't be rewritten and absolutely must have its own ops. > >+ int (*child_to_parent_hwirq)(struct gpio_chip *chip, > >+ unsigned int child_hwirq, > >+ unsigned int child_type, > >+ unsigned int *parent_hwirq, > >+ unsigned int *parent_type); > > Would irq_fwspec(s) be better than passing all these arguments around? I looked over these three drivers that I patched in the series and they all seemed to need pretty much these arguments, so wrapping it into fwspec would probably just make all drivers have to unwrap them to get child (I guess not parent) back out. But we can patch it later if it proves this is too much arguments for some drivers. Its the right amount for those I changed, AFAICT. Yours, Linus Walleij
