Re: [RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>]

On Thu, 25 Apr 2019 at 15:24, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> Hi Ard, thanks for your patch!
>
> Including Mika here as well, he knows how ACPI is supposed to
> be wired up with GPIOs.
>
> On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel
> <ard.biesheuvel@xxxxxxxxxx> wrote:
>
> > In order to support this GPIO block in combination with an EXIU
> > interrupt controller on ACPI systems such as Socionext SynQuacer,
> > add the enumeration boilerplate, and add the EXIU irqchip handling
> > to the probe path. Also, make the clock handling conditonal on
> > non-ACPI enumeration, since ACPI handles this internally.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> (...)
>
> > +config GPIO_MB86S7X_ACPI
> > +       def_bool y
> > +       depends on ACPI && ARCH_SYNQUACER
>
> I would say
>
> depends on ACPI
> depends on (ARCH_SYNQUACER || COMPILE_TEST)
>
> so we get some compiler coverage.
>
> > +#include <linux/acpi.h>
> >  #include <linux/io.h>
> >  #include <linux/init.h>
> >  #include <linux/clk.h>
> > @@ -27,6 +28,8 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/slab.h>
> >
> > +#include "gpiolib.h"
>
> Normally not a good sign to dereference gpiolib internals,
> but there are some few exceptions. Why do you do this?
>

I needed this for acpi_gpiochip_request_interrupts() et al

> > +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);
>
> Just merge it all into one file instead of having these crisscrossy calls.
> I prefer some minor #ifdef or straight C if (IS_ENABLED()) around
> specific code.
>

Fair enough, but please see below.

> > -       return ret;
> > +       if (mb86s70_gpio_have_acpi(pdev))
> > +               acpi_gpiochip_request_interrupts(&gchip->gc);
> > +
> > +       return 0;
>
> I guess this is right...
>
> >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
> >
> > +       if (gchip->gc.irq.domain) {
> > +               acpi_gpiochip_free_interrupts(&gchip->gc);
> > +               irq_domain_remove(gchip->gc.irq.domain);
> > +       }
>
> Mika can possibly comment on the right way to do ACPI bring-up
> and take-down.
>
> Overall it looks pretty straightforward to me, but I'd prefer to just
> merge the irqchip driver over into the gpio driver, I can't see why
> not.
>
> Or is the irqchip used without the GPIO driver sometimes?
>

This is where it becomes a bit nasty: the GPIO controller and the EXIU
interrupt controller are two separate IP blocks, but they are
obviously complimentary. *However*, on SynQuacer, the first 4 physical
lines are not shared between the two, and so you could have 4
interrupt lines handled through the EXIU and 4 different GPIO lines
through the GPIO unit.

On DT, we don't care since we model them as two different units. On
ACPI, however, we cannot model interrupt controllers in the same way,
and so I decided to merge them.

Perhaps the solution is to model the EXIU as a pseudo-GPIO block that
only supports interrupts and not ordinary GPIO?