Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux