Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>]

Linus Walleij <linus.walleij@xxxxxxxxxx> writes:

> On Tue, Jul 5, 2022 at 1:08 PM Aidan MacDonald
> <aidanmacdonald.0x0@xxxxxxxxx> wrote:
>> Linus Walleij <linus.walleij@xxxxxxxxxx> writes:
>
>> I'm not trying to argue that hierarchical IRQ domains are always a bad
>> thing -- I'm just pointing out they're not always useful or necessary.
>> All your points make sense when the GPIO controller is a large distinct
>> block with potentially many GPIOs. When we're dealing with an MFD device
>> with just a few GPIOs, maybe even just one, having a separate IRQ domain
>> makes less sense; the added structure is generally not useful.
>
> Do you mean your driver does this:
>
> MFD main device
> MFD irqchip
>  |
>  +->  MFD gpiochip
>          No irqchip here, so .to_irq() just refers ^ to that one up there
>
> IIUC you mean that if I want to use the irqchip directly then
> I have to refer to the MFD irqchip, I just cannot refer to the
> gpiochip subnode because that one does not have an irqchip.

Yep, that's right.

> // Getting GPIO from gpiochip and irq from MFD device
> // for the same GPIO line
> gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
> irqs = <&mfd 114 IRQ_EDGE_RISING>;
>
> Then for a Linux driver this can be papered over by using the
> .to_irq() callback and just defining gpios.
>
> This isn't very good, if you created a separate gpiochip then you
> should have a separate (hierarchical) irqchip associated with that
> gpiochip as well.
>
> // Getting GPIO and irq from the same gpiochip node
> gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
> irqs = <&gpio 3 IRQ_EDGE_RISING>;
>
> I made this mistake with the ab8500 driver and
> I would not do it like this today. I would use hierarchical gpio
> irqchip. And I should go and fix it. (Is on my TODO.)
>

If moving to hierarchical IRQ chips is the plan, could we add a note
to say .to_irq() is discouraged and shouldn't be used in new code?
Based on what you're saying (which I agree makes sense) it sounds
like there's really no reason to ever use .to_irq().

>> Looking at other GPIO drivers using a hierarchical IRQ domain, they
>> include their own IRQ chips with specialized ops. In my case I don't
>> need any of that (and it'd be the same with other MFD devices) so it
>> looks like using an IRQ domain would mean I'd have to create a fake
>> IRQ chip and domain just to translate between two number spaces.
>>
>> Is that really better than simply using ->to_irq()?
>
> To be honest most irqchips are "fake", what they mostly do is figure
> out which of a few internal sources that fired the irq, so it models the
> different things connected to a single IRQ line.
>
> So yeah, I think the hierarchical irqchip is worth it, especially if that
> means the offset of the irqs and gpios become the same.
>
> Maybe we can add more helpers in the core to make it dirt simple
> though? It would help others with the same problem.
>
> Yours,
> Linus Walleij

Okay, that sounds like a good plan. I'll look more carefully at the
existing drivers and see if I can use existing gpiolib helpers.

One potential issue (from reading the code) is that hierarchical IRQ
domains seemingly can't have a non-hierarchical domain as the parent:
irq_domain_alloc_irqs_parent() calls irq_domain_alloc_irqs_hierarchy()
and the latter fails with -ENOSYS for a non-hierarchical domain.

In my case I'm using a regmap IRQ chip, which is non-hierarchical,
so perhaps that will need to be expanded? 

Regards,
Aidan