[PATCH] gpio: tegra186: remove unneeded loop in tegra186_gpio_init_route_mapping()

[PATCH] gpio: tegra186: remove unneeded loop in tegra186_gpio_init_route_mapping()

[Tom Rix]

Reviewing the j loop over num_irqs_per_bank, in the code previous
to the fixes: commit, every j was used. now only when j == 0.
If only j == 0 is used, there is no need for the loop.

Fixes: 210386804745 ("gpio: tegra186: Support multiple interrupts per bank")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/gpio/gpio-tegra186.c | 40 ++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 9941f35af823..14c872b6ad05 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -677,7 +677,7 @@ static const struct of_device_id tegra186_pmc_of_match[] = {
 static void tegra186_gpio_init_route_mapping(struct tegra_gpio *gpio)
 {
 	struct device *dev = gpio->gpio.parent;
-	unsigned int i, j;
+	unsigned int i;
 	u32 value;
 
 	for (i = 0; i < gpio->soc->num_ports; i++) {
@@ -699,27 +699,23 @@ static void tegra186_gpio_init_route_mapping(struct tegra_gpio *gpio)
 			 * On Tegra194 and later, each pin can be routed to one or more
 			 * interrupts.
 			 */
-			for (j = 0; j < gpio->num_irqs_per_bank; j++) {
-				dev_dbg(dev, "programming default interrupt routing for port %s\n",
-					port->name);
-
-				offset = TEGRA186_GPIO_INT_ROUTE_MAPPING(p, j);
-
-				/*
-				 * By default we only want to route GPIO pins to IRQ 0. This works
-				 * only under the assumption that we're running as the host kernel
-				 * and hence all GPIO pins are owned by Linux.
-				 *
-				 * For cases where Linux is the guest OS, the hypervisor will have
-				 * to configure the interrupt routing and pass only the valid
-				 * interrupts via device tree.
-				 */
-				if (j == 0) {
-					value = readl(base + offset);
-					value = BIT(port->pins) - 1;
-					writel(value, base + offset);
-				}
-			}
+			dev_dbg(dev, "programming default interrupt routing for port %s\n",
+				port->name);
+
+			offset = TEGRA186_GPIO_INT_ROUTE_MAPPING(p, 0);
+
+			/*
+			 * By default we only want to route GPIO pins to IRQ 0. This works
+			 * only under the assumption that we're running as the host kernel
+			 * and hence all GPIO pins are owned by Linux.
+			 *
+			 * For cases where Linux is the guest OS, the hypervisor will have
+			 * to configure the interrupt routing and pass only the valid
+			 * interrupts via device tree.
+			 */
+			value = readl(base + offset);
+			value = BIT(port->pins) - 1;
+			writel(value, base + offset);
 		}
 	}
 }
-- 
2.26.3

Re: [PATCH] gpio: tegra186: remove unneeded loop in tegra186_gpio_init_route_mapping()

[Thierry Reding]

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

On Wed, Jan 25, 2023 at 01:26:31PM -0800, Tom Rix wrote:
> Reviewing the j loop over num_irqs_per_bank, in the code previous
> to the fixes: commit, every j was used. now only when j == 0.
> If only j == 0 is used, there is no need for the loop.
> 
> Fixes: 210386804745 ("gpio: tegra186: Support multiple interrupts per bank")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 40 ++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index 9941f35af823..14c872b6ad05 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -677,7 +677,7 @@ static const struct of_device_id tegra186_pmc_of_match[] = {
>  static void tegra186_gpio_init_route_mapping(struct tegra_gpio *gpio)
>  {
>  	struct device *dev = gpio->gpio.parent;
> -	unsigned int i, j;
> +	unsigned int i;
>  	u32 value;
>  
>  	for (i = 0; i < gpio->soc->num_ports; i++) {
> @@ -699,27 +699,23 @@ static void tegra186_gpio_init_route_mapping(struct tegra_gpio *gpio)
>  			 * On Tegra194 and later, each pin can be routed to one or more
>  			 * interrupts.
>  			 */
> -			for (j = 0; j < gpio->num_irqs_per_bank; j++) {
> -				dev_dbg(dev, "programming default interrupt routing for port %s\n",
> -					port->name);
> -
> -				offset = TEGRA186_GPIO_INT_ROUTE_MAPPING(p, j);
> -
> -				/*
> -				 * By default we only want to route GPIO pins to IRQ 0. This works
> -				 * only under the assumption that we're running as the host kernel
> -				 * and hence all GPIO pins are owned by Linux.
> -				 *
> -				 * For cases where Linux is the guest OS, the hypervisor will have
> -				 * to configure the interrupt routing and pass only the valid
> -				 * interrupts via device tree.
> -				 */
> -				if (j == 0) {
> -					value = readl(base + offset);
> -					value = BIT(port->pins) - 1;
> -					writel(value, base + offset);
> -				}
> -			}
> +			dev_dbg(dev, "programming default interrupt routing for port %s\n",
> +				port->name);
> +
> +			offset = TEGRA186_GPIO_INT_ROUTE_MAPPING(p, 0);
> +
> +			/*
> +			 * By default we only want to route GPIO pins to IRQ 0. This works
> +			 * only under the assumption that we're running as the host kernel
> +			 * and hence all GPIO pins are owned by Linux.
> +			 *
> +			 * For cases where Linux is the guest OS, the hypervisor will have
> +			 * to configure the interrupt routing and pass only the valid
> +			 * interrupts via device tree.
> +			 */
> +			value = readl(base + offset);
> +			value = BIT(port->pins) - 1;
> +			writel(value, base + offset);

This was supposed to be a placeholder so that a more complex routing
could be added later on. Maybe adding "j" to the debug message would
have made that a bit clearer.

Anyway, no complex routing has been needed so far, so I don't have a
strong objection to this. We can always add it back if we ever need it.

On the other hand, we don't do much in the loop, so leaving it as-is
wouldn't be harmful either.

I'll leave it up to Linus and Bartosz to decide. If they want to pick it
up:

Acked-by: Thierry Reding <treding@nvidia.com>

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

Re: [PATCH] gpio: tegra186: remove unneeded loop in tegra186_gpio_init_route_mapping()

[Bartosz Golaszewski]

On Wed, Jan 25, 2023 at 10:26 PM Tom Rix <trix@redhat.com> wrote:
>
> Reviewing the j loop over num_irqs_per_bank, in the code previous
> to the fixes: commit, every j was used. now only when j == 0.
> If only j == 0 is used, there is no need for the loop.
>
> Fixes: 210386804745 ("gpio: tegra186: Support multiple interrupts per bank")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 40 ++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index 9941f35af823..14c872b6ad05 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -677,7 +677,7 @@ static const struct of_device_id tegra186_pmc_of_match[] = {
>  static void tegra186_gpio_init_route_mapping(struct tegra_gpio *gpio)
>  {
>         struct device *dev = gpio->gpio.parent;
> -       unsigned int i, j;
> +       unsigned int i;
>         u32 value;
>
>         for (i = 0; i < gpio->soc->num_ports; i++) {
> @@ -699,27 +699,23 @@ static void tegra186_gpio_init_route_mapping(struct tegra_gpio *gpio)
>                          * On Tegra194 and later, each pin can be routed to one or more
>                          * interrupts.
>                          */
> -                       for (j = 0; j < gpio->num_irqs_per_bank; j++) {
> -                               dev_dbg(dev, "programming default interrupt routing for port %s\n",
> -                                       port->name);
> -
> -                               offset = TEGRA186_GPIO_INT_ROUTE_MAPPING(p, j);
> -
> -                               /*
> -                                * By default we only want to route GPIO pins to IRQ 0. This works
> -                                * only under the assumption that we're running as the host kernel
> -                                * and hence all GPIO pins are owned by Linux.
> -                                *
> -                                * For cases where Linux is the guest OS, the hypervisor will have
> -                                * to configure the interrupt routing and pass only the valid
> -                                * interrupts via device tree.
> -                                */
> -                               if (j == 0) {
> -                                       value = readl(base + offset);
> -                                       value = BIT(port->pins) - 1;
> -                                       writel(value, base + offset);
> -                               }
> -                       }
> +                       dev_dbg(dev, "programming default interrupt routing for port %s\n",
> +                               port->name);
> +
> +                       offset = TEGRA186_GPIO_INT_ROUTE_MAPPING(p, 0);
> +
> +                       /*
> +                        * By default we only want to route GPIO pins to IRQ 0. This works
> +                        * only under the assumption that we're running as the host kernel
> +                        * and hence all GPIO pins are owned by Linux.
> +                        *
> +                        * For cases where Linux is the guest OS, the hypervisor will have
> +                        * to configure the interrupt routing and pass only the valid
> +                        * interrupts via device tree.
> +                        */
> +                       value = readl(base + offset);
> +                       value = BIT(port->pins) - 1;
> +                       writel(value, base + offset);
>                 }
>         }
>  }
> --
> 2.26.3
>

Applied, thanks!

Bart