Re: [PATCH 1/6 v2] gpio: Add support for hierarchical IRQ domains

[Lina Iyer <ilina@codeaurora.org>]

On Mon, Aug 26 2019 at 13:02 -0600, Marc Zyngier wrote:
>On Mon, 26 Aug 2019 10:15:26 -0600
>Lina Iyer <ilina@codeaurora.org> wrote:
>
>> On Fri, Aug 23 2019 at 03:12 -0600, Marc Zyngier wrote:
>> >On 23/08/2019 09:24, Linus Walleij wrote:
>> >> Hi Brian,
>> >>
>> >> I tried to look into this ssbi-gpio problem...
>> >>
>> >> Paging in Marc Z as well.
>> >>
>> >> On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@onstation.org> wrote:
>> >>
>> >>> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
>> >>> this little snippet that's different from spmi-gpio:
>> >>>
>> >>>         [ fwspec mapping code ]
>> >>>
>> >>>         /*
>> >>>          * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
>> >>>          * line level.
>> >>>          */
>> >>>         pin->irq = ret;
>> >>>
>> >>> Here's the relevant code in pm8xxx_gpio_get():
>> >>>
>> >>>         if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
>> >>>                 ret = pin->output_value;
>> >>>         } else if (pin->irq >= 0) {
>> >>>                 ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
>> >>>                 ...
>> >>>         }
>> >>
>> >> It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
>> >> the global irq numbers and the internal function using irqdesc
>> >> __irq_get_irqchip_state() is not exported.
>> >>
>> >> We should be encouraged to operate on IRQ descriptors rather
>> >> than IRQ numbers when doing this kind of deep core work,
>> >> right?
>> >
>> >Certainly, I'd like to gradually move over from the IRQ number to an
>> >irq_desc. In interrupt-heavy contexts, this ends up being quite time
>> >consuming. I have an old patch series somewhere changing irq domains to
>> >use irq_descs internally instead of IRQ numbers, which I should maybe
>> >revive.
>> >
>> >As for __irq_get_irqchip_state, the main issue is that it doesn't
>> >perform any locking on its own, so you'd have to either provide your
>> >own, or wrap it with something else.
>> >
>> Marc, on a related note, What are your thoughts on a
>> irq_set_irqchip_state? We are running into an issue where chip state
>> clear is required as well as clearing out that of the parent. Do you see
>> problems in doing that from an irqchip driver?
>
>[desperately trying to switch to my korg address...]
>
>I'm not sure I fully get the question: if this is whether it is worth
>implementing it, it usually is a good idea if the HW supports is. But I
>have the feeling that this isn't your question, so maybe explaining
>your use-case would help.
>
>Do you mean using irq_set_irqchip_state from within an irqchip driver?
>That'd be quite odd, as this function is usually used when something
>*outside* of the irqchip driver needs to poke at some internal state
>because "it knows best" (see for example what KVM does with the active
>state).
>
Yeah, I meant to say, when invoked on a parent irqchip from a child
irqchip.

In my case when GPIO is used as an interrupt. Even though the interrupt
is not enabled, the GPIO could cause the GIC_ISPEND to be set at GIC. We
were wondering if we could use the irq_set_irqchip_state to clear
pending state at GPIO's irqchip parent and all the way to GIC,  when the
GPIO is enabled as an interrupt.

-- Lina