Re: [PATCH v3 1/2] gpio: tegra: add multiple interrupt support

From: Thierry Reding <thierry.reding@gmail.com>
To: Prathamesh Shete <pshete@nvidia.com>
Cc: linus.walleij@linaro.org, bgolaszewski@baylibre.com,
	jonathanh@nvidia.com, linux-gpio@vger.kernel.org,
	linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
	smangipudi@nvidia.com
Subject: Re: [PATCH v3 1/2] gpio: tegra: add multiple interrupt support
Date: Fri, 17 Sep 2021 12:47:12 +0200	[thread overview]
Message-ID: <YURyMAsWVYH+/0UI@orome.fritz.box> (raw)
In-Reply-To: <20210907073224.3070-2-pshete@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 2785 bytes --]

On Tue, Sep 07, 2021 at 01:02:23PM +0530, Prathamesh Shete wrote:
> From: pshete <pshete@nvidia.com>
> 
> T19x GPIO controller's support multiple interrupts. The GPIO
> controller is capable to route 8 interrupts per controller in
> case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
> This is new feature starting Tegra194
> The interrupt route map determines which interrupt line is to be used.
> 
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index d38980b9923a..c1172da9aebf 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2016-2017 NVIDIA Corporation
> + * Copyright (c) 2016-2021 NVIDIA Corporation
>   *
>   * Author: Thierry Reding <treding@nvidia.com>
>   */
> @@ -68,6 +68,7 @@ struct tegra_gpio_soc {
>  	unsigned int num_ports;
>  	const char *name;
>  	unsigned int instance;
> +	bool multi_ints;
>  
>  	const struct tegra186_pin_range *pin_ranges;
>  	unsigned int num_pin_ranges;
> @@ -450,7 +451,8 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
>  	struct irq_domain *domain = gpio->gpio.irq.domain;
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	unsigned int parent = irq_desc_get_irq(desc);
> -	unsigned int i, offset = 0;
> +	unsigned int i, j, offset = 0;
> +	int intr_cntr;
>  
>  	chained_irq_enter(chip, desc);
>  
> @@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
>  
>  		base = gpio->base + port->bank * 0x1000 + port->port * 0x200;
>  
> -		/* skip ports that are not associated with this bank */
> -		if (parent != gpio->irq[port->bank])
> -			goto skip;
> +		if (!gpio->soc->multi_ints) {
> +			/* skip ports that are not associated with this bank */
> +			if (parent != gpio->irq[port->bank])
> +				goto skip;
> +
> +		} else {
> +			intr_cntr = 0;
> +			for (j = 0; j < 8; j++) {
> +				if (parent != gpio->irq[(port->bank * 8) + j])

Again, I don't see how this would work. Currently the DT for Tegra194
(where you set multi_ints = true) lists 6 interrupts. So as soon as j
goes beyond 5, this will end up accessing data beyond the bounds of
the gpio->irq array.

I've revised the patches that I created to support this a while ago and
which I had sent earlier as a counter-proposal that keeps compatibility
with earlier device trees. I've now tested it and found a few issues I
had not run into earlier, but it should now work correctly with older
and updated device trees.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]