Date:   Mon, 7 Dec 2020 13:34:18 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Jerome Brunet <jbrunet@...libre.com>, Marc Zyngier <maz@...nel.org>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        林圣欢 <linshenghuan@...gtu-china.com>,
        khilman <khilman@...libre.com>,
        narmstrong <narmstrong@...libre.com>,
        "martin.blumenstingl" <martin.blumenstingl@...glemail.com>,
        linux-gpio <linux-gpio@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-amlogic <linux-amlogic@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: 0001-add-amlogic-gpio-to-irq

On Mon, Dec 7, 2020 at 12:07 PM Jerome Brunet <jbrunet@...libre.com> wrote:
> On Mon 07 Dec 2020 at 11:18, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> > On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet <jbrunet@...libre.com> wrote:
> >> On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@...aro.org> wrote:
> >
> >> This HW only has 8 irqs that can each be mapped to a pin. No direct
> >> translation can be made, we have to allocate an irq to monitor the line.
> >> So when gpio_to_irq() was called, we had to do that allocation dynamically
> >> to return a valid irq number. Since there was no counter part to
> >> gpio_to_irq(), those allocation cannot be freed during the lifetime of
> >> the device.

gpio_to_irq() is just a helper really and should not really be used to allocate
anything.

In device tree systems, the GPIO provider should nominally present itsel
as a dual-mode gpio-controller and interrupt-controller for example:

                gpio1: gpio@...00000 {
                        compatible = "cortina,gemini-gpio", "faraday,ftgpio010";
                        reg = <0x4e000000 0x100>;
                        interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
                        resets = <&syscon GEMINI_RESET_GPIO1>;
                        clocks = <&syscon GEMINI_CLK_APB>;
                        gpio-controller;
                        #gpio-cells = <2>;
                        interrupt-controller;
                        #interrupt-cells = <2>;
                };

The GPIOs are normally *not* translated to IRQs in this set-up. Rather the
interrupts are requested by consumers using request_[threaded_]irq()
which means you should be using the irqchip callbacks such as
.irq_request_resources() and .irq_release_resources() to allocate one
of the free irq lines to use. These will be called at the right points if a
properly written driver requests an IRQ and when the driver is removed.

In some rare cases gpio_to_irq() is used because all the driver knows is
a GPIO number and it want to try to obtain an IRQ for it, and if a 1-to-1
mapping exists it returns this number. This is not the norm, but the
exception.

So maybe the problem is that you need to go back and think about
updating the DT bindings for this thing to include interrupt-controller
as well?

>  * This HW has to create the mapping between GPIO and irq number
>    dynamically. The number of irqs available is very limited.

This should be done using irq_chip callbacks.

>  * We only get to know a mapping is required when gpio_to_irq() is called

No that callback should not be used for that.

>  * There is no way to know when it is safe to dispose of the created
>    mapping

The way that is done is when .irq_release_resources() is called.

>  * Some drivers require a trigger type we don't support. These will create
>    mappings and not use it because of the failure when .set_type() is
>    called

I don't quite understand this. Do you mean you are bombarded by pointless
requests for interrupts that will not work anyways? Then do not assign
interrupts to these drivers in the device tree. These requesting devices
and their requests are under your control. The drivers should be able to
back out and work without interrupt if request_irq() fails because it
can't provide the type on interrupt you want:

int irq = request_irq(irq, my_isr, IRQF_TRIGGER_RISING |
IRQF_TRIGGER_FALLING, "My ISR", cookie);
// This results in .irq_request_resources() and .irq_set_type()
if (irq < 0) {
   // Oopps out of IRQs or couldn't support double edges, bail out or
use polling
}

Just do it like this (you might have to augment your drivers) and you'll
be fine?

> To answer your question, there an API which lets us know a mapping is
> needed, but none to inform that it is not required anymore. The GPIO API
> was not meant to used like this. Not saying it is good or bad, this is
> just how it is.

So don't use it?

Yours,
Linus Walleij

Powered by blists - more mailing lists