All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup()
@ 2019-01-16  9:31 Thomas Petazzoni
  2019-01-16  9:31 ` [PATCH 2/2] gpio: pca953x: use a per instance irq_chip structure Thomas Petazzoni
  2019-01-21 13:05 ` [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup() Linus Walleij
  0 siblings, 2 replies; 4+ messages in thread
From: Thomas Petazzoni @ 2019-01-16  9:31 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski
  Cc: linux-gpio, linux-kernel, Marek Vasut, Paul Kocialkowski,
	Thomas Petazzoni

The current design of pca953x_irq_setup() is:

 if (all conditions to support IRQ are met) {
   lots of code to support IRQs, which goes to a serious indentation
   level.
 }

 return 0;

It makes more sense to handle this like this:

 if (!all conditions to support IRQ are met)
   return 0;

 handle IRQ support

This commit does just this change, reducing by one tab the indentation
level of the IRQ setup code. Thanks to this reduced indentation level,
we are less restricted by the 80-column limit, and we can have more
function arguments on the same line.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 drivers/gpio/gpio-pca953x.c | 84 ++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 0dc96419efe3..4f91ce497dd1 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -702,53 +702,53 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 	int reg_direction[MAX_BANK];
 	int ret, i;
 
-	if (client->irq && irq_base != -1
-			&& (chip->driver_data & PCA_INT)) {
-		ret = pca953x_read_regs(chip,
-					chip->regs->input, chip->irq_stat);
-		if (ret)
-			return ret;
+	if (!client->irq)
+		return 0;
 
-		/*
-		 * There is no way to know which GPIO line generated the
-		 * interrupt.  We have to rely on the previous read for
-		 * this purpose.
-		 */
-		regmap_bulk_read(chip->regmap, chip->regs->direction,
-				 reg_direction, NBANK(chip));
-		for (i = 0; i < NBANK(chip); i++)
-			chip->irq_stat[i] &= reg_direction[i];
-		mutex_init(&chip->irq_lock);
-
-		ret = devm_request_threaded_irq(&client->dev,
-					client->irq,
-					   NULL,
-					   pca953x_irq_handler,
-					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
-						   IRQF_SHARED,
-					   dev_name(&client->dev), chip);
-		if (ret) {
-			dev_err(&client->dev, "failed to request irq %d\n",
-				client->irq);
-			return ret;
-		}
+	if (irq_base == -1)
+		return 0;
 
-		ret =  gpiochip_irqchip_add_nested(&chip->gpio_chip,
-						   &pca953x_irq_chip,
-						   irq_base,
-						   handle_simple_irq,
-						   IRQ_TYPE_NONE);
-		if (ret) {
-			dev_err(&client->dev,
-				"could not connect irqchip to gpiochip\n");
-			return ret;
-		}
+	if (!(chip->driver_data & PCA_INT))
+		return 0;
 
-		gpiochip_set_nested_irqchip(&chip->gpio_chip,
-					    &pca953x_irq_chip,
-					    client->irq);
+	ret = pca953x_read_regs(chip, chip->regs->input, chip->irq_stat);
+	if (ret)
+		return ret;
+
+	/*
+	 * There is no way to know which GPIO line generated the
+	 * interrupt.  We have to rely on the previous read for
+	 * this purpose.
+	 */
+	regmap_bulk_read(chip->regmap, chip->regs->direction, reg_direction,
+			 NBANK(chip));
+	for (i = 0; i < NBANK(chip); i++)
+		chip->irq_stat[i] &= reg_direction[i];
+	mutex_init(&chip->irq_lock);
+
+	ret = devm_request_threaded_irq(&client->dev, client->irq,
+					NULL, pca953x_irq_handler,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT |
+					IRQF_SHARED,
+					dev_name(&client->dev), chip);
+	if (ret) {
+		dev_err(&client->dev, "failed to request irq %d\n",
+			client->irq);
+		return ret;
+	}
+
+	ret =  gpiochip_irqchip_add_nested(&chip->gpio_chip, &pca953x_irq_chip,
+					   irq_base, handle_simple_irq,
+					   IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(&client->dev,
+			"could not connect irqchip to gpiochip\n");
+		return ret;
 	}
 
+	gpiochip_set_nested_irqchip(&chip->gpio_chip, &pca953x_irq_chip,
+				    client->irq);
+
 	return 0;
 }
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] gpio: pca953x: use a per instance irq_chip structure
  2019-01-16  9:31 [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup() Thomas Petazzoni
@ 2019-01-16  9:31 ` Thomas Petazzoni
  2019-01-21 13:06   ` Linus Walleij
  2019-01-21 13:05 ` [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup() Linus Walleij
  1 sibling, 1 reply; 4+ messages in thread
From: Thomas Petazzoni @ 2019-01-16  9:31 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski
  Cc: linux-gpio, linux-kernel, Marek Vasut, Paul Kocialkowski,
	Thomas Petazzoni

When a system has two PCA953x GPIO expanders, the kernel complains with:

gpio gpiochip2: (0-0021): detected irqchip that is shared with multiple gpiochips: please fix the driver.

Indeed, there is a single instance of "struct irq_chip" that gets
re-used for both PCA953x instance. This commit moves the "struct
irq_chip" to be part of the "struct pca953x_chip", so that we have one
"struct irq_chip" per PCA953X instance.

As part of this, the name of the irq_chip is also made different on a
per-instance basis, now using the dev_name() of the I2C device. This
changes what is visible in /proc/interrupts.

Before:

 47:          0          0   pca953x  10 Edge      e0100000.sdhci cd
 48:          0          0   pca953x   6 Edge      e0101000.sdhci cd

After:

 47:          0          0    0-0020  10 Edge      e0100000.sdhci cd
 48:          2          0    0-0020   6 Edge      e0101000.sdhci cd

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 drivers/gpio/gpio-pca953x.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4f91ce497dd1..de52f63863db 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -150,6 +150,7 @@ struct pca953x_chip {
 	u8 irq_stat[MAX_BANK];
 	u8 irq_trig_raise[MAX_BANK];
 	u8 irq_trig_fall[MAX_BANK];
+	struct irq_chip irq_chip;
 #endif
 
 	struct i2c_client *client;
@@ -594,16 +595,6 @@ static void pca953x_irq_shutdown(struct irq_data *d)
 	chip->irq_trig_fall[d->hwirq / BANK_SZ] &= ~mask;
 }
 
-static struct irq_chip pca953x_irq_chip = {
-	.name			= "pca953x",
-	.irq_mask		= pca953x_irq_mask,
-	.irq_unmask		= pca953x_irq_unmask,
-	.irq_bus_lock		= pca953x_irq_bus_lock,
-	.irq_bus_sync_unlock	= pca953x_irq_bus_sync_unlock,
-	.irq_set_type		= pca953x_irq_set_type,
-	.irq_shutdown		= pca953x_irq_shutdown,
-};
-
 static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 {
 	u8 cur_stat[MAX_BANK];
@@ -699,6 +690,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 			     int irq_base)
 {
 	struct i2c_client *client = chip->client;
+	struct irq_chip *irq_chip = &chip->irq_chip;
 	int reg_direction[MAX_BANK];
 	int ret, i;
 
@@ -737,7 +729,15 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 		return ret;
 	}
 
-	ret =  gpiochip_irqchip_add_nested(&chip->gpio_chip, &pca953x_irq_chip,
+	irq_chip->name = dev_name(&chip->client->dev);
+	irq_chip->irq_mask = pca953x_irq_mask;
+	irq_chip->irq_unmask = pca953x_irq_unmask;
+	irq_chip->irq_bus_lock = pca953x_irq_bus_lock;
+	irq_chip->irq_bus_sync_unlock = pca953x_irq_bus_sync_unlock;
+	irq_chip->irq_set_type = pca953x_irq_set_type;
+	irq_chip->irq_shutdown = pca953x_irq_shutdown;
+
+	ret =  gpiochip_irqchip_add_nested(&chip->gpio_chip, irq_chip,
 					   irq_base, handle_simple_irq,
 					   IRQ_TYPE_NONE);
 	if (ret) {
@@ -746,8 +746,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 		return ret;
 	}
 
-	gpiochip_set_nested_irqchip(&chip->gpio_chip, &pca953x_irq_chip,
-				    client->irq);
+	gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
 
 	return 0;
 }
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup()
  2019-01-16  9:31 [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup() Thomas Petazzoni
  2019-01-16  9:31 ` [PATCH 2/2] gpio: pca953x: use a per instance irq_chip structure Thomas Petazzoni
@ 2019-01-21 13:05 ` Linus Walleij
  1 sibling, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2019-01-21 13:05 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel,
	Marek Vasut, Paul Kocialkowski

On Wed, Jan 16, 2019 at 10:32 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> The current design of pca953x_irq_setup() is:
>
>  if (all conditions to support IRQ are met) {
>    lots of code to support IRQs, which goes to a serious indentation
>    level.
>  }
>
>  return 0;
>
> It makes more sense to handle this like this:
>
>  if (!all conditions to support IRQ are met)
>    return 0;
>
>  handle IRQ support
>
> This commit does just this change, reducing by one tab the indentation
> level of the IRQ setup code. Thanks to this reduced indentation level,
> we are less restricted by the 80-column limit, and we can have more
> function arguments on the same line.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Patch applied.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] gpio: pca953x: use a per instance irq_chip structure
  2019-01-16  9:31 ` [PATCH 2/2] gpio: pca953x: use a per instance irq_chip structure Thomas Petazzoni
@ 2019-01-21 13:06   ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2019-01-21 13:06 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel,
	Marek Vasut, Paul Kocialkowski

On Wed, Jan 16, 2019 at 10:32 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> When a system has two PCA953x GPIO expanders, the kernel complains with:
>
> gpio gpiochip2: (0-0021): detected irqchip that is shared with multiple gpiochips: please fix the driver.
>
> Indeed, there is a single instance of "struct irq_chip" that gets
> re-used for both PCA953x instance. This commit moves the "struct
> irq_chip" to be part of the "struct pca953x_chip", so that we have one
> "struct irq_chip" per PCA953X instance.
>
> As part of this, the name of the irq_chip is also made different on a
> per-instance basis, now using the dev_name() of the I2C device. This
> changes what is visible in /proc/interrupts.
>
> Before:
>
>  47:          0          0   pca953x  10 Edge      e0100000.sdhci cd
>  48:          0          0   pca953x   6 Edge      e0101000.sdhci cd
>
> After:
>
>  47:          0          0    0-0020  10 Edge      e0100000.sdhci cd
>  48:          2          0    0-0020   6 Edge      e0101000.sdhci cd
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Patch applied.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-01-21 13:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16  9:31 [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup() Thomas Petazzoni
2019-01-16  9:31 ` [PATCH 2/2] gpio: pca953x: use a per instance irq_chip structure Thomas Petazzoni
2019-01-21 13:06   ` Linus Walleij
2019-01-21 13:05 ` [PATCH 1/2] gpio: pca953x: reduce indentation level in pca953x_irq_setup() Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.