Re: [PATCH v5 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, May 24, 2022 at 11:01 AM Lad, Prabhakar
<prabhakar.csengg@xxxxxxxxx> wrote:
> On Tue, May 24, 2022 at 9:57 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:> >
> > On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> >
> > > Add IRQ domain to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > >
> > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > used as IRQ lines at a given time. Selection of pins as IRQ lines
> > > is handled by IA55 (which is the IRQC block) which sits in between the
> > > GPIO and GIC.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > I don't know if I'm too tired or reading it wrong, but it seems you
> > went through the trouble of making it possible to override .free() in
> > the irqdomain in patch 3/5 and yet not using it in this patch 5/5?
> >
> I think you missed it, free callback is overridden with
> rzg2l_gpio_irq_domain_free().
>
> [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220523174238.28942-6-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/

Yeah my bad, can't read properly today :/

Why is it necessary to do this stuff in the irqdomain rather than
in the irqchip? Especially this:

+ bitmap_release_region(pctrl->tint_slot, i, get_order(1));

Since the idea with irq_domain is to translate physical (hardware) IRQs
to Linux IRQ numbers, I don't see how this is related to that.

To me it seems you have taken the usecase that is normally
in irqchip and moved it to irqdomain.

To me this seems much more like a job that needs to happen in
the irqchip .irq_enable()/.irq_disable() pair, and which we have
done before in Hans Verkuils patch series:

461c1a7d4733 gpiolib: override irq_enable/disable
4e9439ddacea gpiolib: add flag to indicate if the irq is disabled
ca620f2de153 gliolib: set hooks in gpiochip_set_irq_hooks()

This gets used by drivers such as:
drivers/media/cec/platform/cec-gpio/cec-gpio.c

Where you can see these dynamic calls:

static bool cec_gpio_enable_irq(struct cec_adapter *adap)
{
        struct cec_gpio *cec = cec_get_drvdata(adap);

        enable_irq(cec->cec_irq);
        return true;
}

static void cec_gpio_disable_irq(struct cec_adapter *adap)
{
        struct cec_gpio *cec = cec_get_drvdata(adap);

        disable_irq(cec->cec_irq);
}

Which end up calling .irq_enable()/.irq_disable() on the irq_chip
dynamically enabling/disabling the irq.

If you prefer to have this done in process context up front when
the irq is requested/released then irq_chip also have these
callbacks:

        int             (*irq_request_resources)(struct irq_data *data);
        void            (*irq_release_resources)(struct irq_data *data);

So I would think over the usecase here a bit. Why does this have
to be in the irqdomain?

Yours,
Linus Walleij