Re: [PATCH v2] gpio: Add Tegra186 support

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Mar 10, 2017 at 5:26 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:

> From: Thierry Reding <treding@xxxxxxxxxx>
>
> Tegra186 has two GPIO controllers that are largely register compatible
> between one another but are completely different from the controller
> found on earlier generations.
>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> Changes in v2:

So this still doesn't use GPIOLIB_IRQCHIP even though I pointed
out that you can assign several parent interrupts to the same
irqchip.

For a recent example see:
http://marc.info/?l=devicetree&m=149012117004066&w=2
(Notice Gregory's elegant manipulation of the mask.)

My earlier reply here:

----------------------
>> I would prefer if you could try to convert this driver to use
>> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
>> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
>> It would save us so much trouble and so much complicated
>> code to maintain for this custom irqdomain.
>>
>> I suggest you to look into the mechanisms mentioned in my
>> previous mail for how to poke holes in the single linear
>> irqdomain used by this mechanism.
>>
>> As it seems, you only have one parent interrupt with all
>> these IRQs cascading off it as far as I can tell.
>
> Like I said in other email, I don't think this will work because the
> GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> parent interrupt is used. We could possibly live with a single IRQ
> handler and data, but we definitely need to request multiple IRQs if
> we want to be able to use all GPIOs as interrupts.

Sorry if I missed that.

Actually it works.

If you look at gpiochip_set_chained_irqchip() it just calls
irq_set_chained_handler_and_data() for the parent interrupt.
------------------------

Just call gpiochip_set_chained_irqchip() for all the irqs, and
figure out a way to get the right interrupt hardware offset in the
interrupt handler.

(...)

> +config GPIO_TEGRA186
> +       tristate "NVIDIA Tegra186 GPIO support"
> +       default ARCH_TEGRA_186_SOC
> +       depends on ARCH_TEGRA_186_SOC || COMPILE_TEST
> +       depends on OF_GPIO

select GPIOLIB_IRQCHIP

> +#include <linux/gpio/driver.h>
> +#include <linux/gpio.h>

Only <linux/gpio/driver.h>

You should not use <linux/gpio.h> in drivers, then you are
doing something wrong.

> +struct tegra_gpio {
> +       struct gpio_chip gpio;
> +       struct irq_chip intc;
> +       unsigned int num_irq;
> +       unsigned int *irq;

Why are you keeping the irqs around after requesting?
Use devm_*

> +
> +       const struct tegra_gpio_soc *soc;
> +
> +       void __iomem *base;
> +
> +       struct irq_domain *domain;

I don't like custom irq domains it is messy.
Please do your best to try to use GPIOLIB_IRQCHIP

If you still decide to go with a custom irqdomain, there is
stuff missing, especially the gpiochip_lock_as_irq()
calls from the irqchip .irq_request/release_resources()
callbacks, see the gpiolib.c implementation in the
helpers there for reference.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html