[PATCH 0/3] gpio: lpc32xx: enable interrupts for port 3

[PATCH 0/3] gpio: lpc32xx: enable interrupts for port 3

[Alexandre Belloni]

Hi,

This series enables interrupt support for GPIOs on port 3. Those are the
GPIOs that are connected directl to an interrupt controller (sic1 or
sic2). This was tested on a custom LPC3250 design.

The binding includes support for interrupts on port 0 and port 1 but
this requires the registration of a proper irqchip and I don't have any
way to test it.

Patch 1 and 2 should probably go through the gpio tree while patch 3
should go through arm-soc. They are quite independant and this shoud
cause no issue.

Alexandre Belloni (3):
  dt-bindings: gpio: lpc32xx: document interrupt bindings
  gpio: lpc32xx: enable interrupt lookup for port 3
  ARM: dts: lpc32xx: add GPIO interrupts

 .../devicetree/bindings/gpio/gpio_lpc32xx.txt |  4 +++
 arch/arm/boot/dts/lpc32xx.dtsi                | 25 +++++++++++++++++++
 drivers/gpio/gpio-lpc32xx.c                   | 14 ++++-------
 3 files changed, 34 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/3] dt-bindings: gpio: lpc32xx: document interrupt bindings

[Alexandre Belloni]

Some of the LPC32xx gpios are wired directly to one of the interrupt
controllers while port 0 and port 1 share the same interrupt for their
interrupt capable gpios.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
index 49819367a011..e7957a17e4db 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
@@ -16,6 +16,10 @@ Required properties:
    3) optional parameters:
       - bit 0 specifies polarity (0 for normal, 1 for inverted)
 - reg: Index of the GPIO group
+- interrupts: Should be the interrupt shared by port 0 and port 1 and the
+  interrupts for individual pins from port 3.
+- interrupt-names : Should be the names of irq resources. The shared port
+  interrupt is named "p01", the other use the pin names.
 
 Example:
 
-- 
2.20.1

Re: [PATCH 1/3] dt-bindings: gpio: lpc32xx: document interrupt bindings

[Linus Walleij]

Hi Alexandre,

thanks for your patch!

On Wed, Apr 10, 2019 at 12:39 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:

> Some of the LPC32xx gpios are wired directly to one of the interrupt
> controllers while port 0 and port 1 share the same interrupt for their
> interrupt capable gpios.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
(...)

> +- interrupts: Should be the interrupt shared by port 0 and port 1 and the
> +  interrupts for individual pins from port 3.
> +- interrupt-names : Should be the names of irq resources. The shared port
> +  interrupt is named "p01", the other use the pin names.

The recent design pattern with hierarchical interrupts such as this one
is to simply hardcode the relationship between parent interrupt controller
and child interrupt controller in the driver.

However you should add interrupt-controller and possibly
interrupt-parent to the bindings and example.

More comments in patch 2.

Yours,
Linus Walleij

[PATCH 2/3] gpio: lpc32xx: enable interrupt lookup for port 3

[Alexandre Belloni]

Interrupt support was disabled "temporarily" in commit 320a6480ef24 ("gpio:
lpc32xx: disable broken to_irq support").

Reenable to_irq for port 3 as they are directly connected to an interrupt
controller and a simple lookup is working.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/gpio/gpio-lpc32xx.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-lpc32xx.c b/drivers/gpio/gpio-lpc32xx.c
index aa74cc4d8b14..24f09b08e319 100644
--- a/drivers/gpio/gpio-lpc32xx.c
+++ b/drivers/gpio/gpio-lpc32xx.c
@@ -22,6 +22,7 @@
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 
@@ -385,14 +386,9 @@ static int lpc32xx_gpio_to_irq_p01(struct gpio_chip *chip, unsigned offset)
 	return -ENXIO;
 }
 
-static int lpc32xx_gpio_to_irq_gpio_p3(struct gpio_chip *chip, unsigned offset)
+static int lpc32xx_gpio_to_irq_p3(struct gpio_chip *chip, unsigned int offset)
 {
-	return -ENXIO;
-}
-
-static int lpc32xx_gpio_to_irq_gpi_p3(struct gpio_chip *chip, unsigned offset)
-{
-	return -ENXIO;
+	return of_irq_get_byname(chip->of_node, chip->names[offset]);
 }
 
 static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = {
@@ -451,7 +447,7 @@ static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = {
 			.direction_output	= lpc32xx_gpio_dir_output_p3,
 			.set			= lpc32xx_gpio_set_value_p3,
 			.request		= lpc32xx_gpio_request,
-			.to_irq			= lpc32xx_gpio_to_irq_gpio_p3,
+			.to_irq			= lpc32xx_gpio_to_irq_p3,
 			.base			= LPC32XX_GPIO_P3_GRP,
 			.ngpio			= LPC32XX_GPIO_P3_MAX,
 			.names			= gpio_p3_names,
@@ -465,7 +461,7 @@ static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = {
 			.direction_input	= lpc32xx_gpio_dir_in_always,
 			.get			= lpc32xx_gpi_get_value,
 			.request		= lpc32xx_gpio_request,
-			.to_irq			= lpc32xx_gpio_to_irq_gpi_p3,
+			.to_irq			= lpc32xx_gpio_to_irq_p3,
 			.base			= LPC32XX_GPI_P3_GRP,
 			.ngpio			= LPC32XX_GPI_P3_MAX,
 			.names			= gpi_p3_names,
-- 
2.20.1

Re: [PATCH 2/3] gpio: lpc32xx: enable interrupt lookup for port 3

[Linus Walleij]

On Wed, Apr 10, 2019 at 12:39 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:

> Interrupt support was disabled "temporarily" in commit 320a6480ef24 ("gpio:
> lpc32xx: disable broken to_irq support").
>
> Reenable to_irq for port 3 as they are directly connected to an interrupt
> controller and a simple lookup is working.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Unfortunately this seems to be one of these hacks for
hierarchical interrupts that does not quite cut it.

> +static int lpc32xx_gpio_to_irq_p3(struct gpio_chip *chip, unsigned int offset)
> +       return of_irq_get_byname(chip->of_node, chip->names[offset]);

This works as long as consuming drivers pick a GPIO first
using gpiod_get() (and variants) and then convert it to an
IRQ using gpiod_to_irq().

But it doesn't work when some consumer is just picking an
IRQ off of the node without picking the GPIO first.
And in DT that is OK, this DT node is definately an interrupt
controller. (Leaving out the attribute for interrupt controller
as is currently the case doesn't really fix the issue, it is
just inconsistent.)

Look at Brian Masney's series for the qualcomm pin controller
chips for inspiration on how to do things right, see:
commit 9d2b563bc23acfa93e7716b3396fd2f79fa8f0cd
and down.

Also:
https://marc.info/?l=linux-gpio&m=154959228527643&w=2

Especially note how he removes the kind of hack you are
adding in e.g.
commit a796fab2c605d6340512c51c6c3fa1c581357993

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: lpc32xx: enable interrupt lookup for port 3

[Sylvain Lemieux]

On Thu, Apr 11, 2019 at 9:55 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Apr 10, 2019 at 12:39 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
>
> > Interrupt support was disabled "temporarily" in commit 320a6480ef24 ("gpio:
> > lpc32xx: disable broken to_irq support").
> >
> > Reenable to_irq for port 3 as they are directly connected to an interrupt
> > controller and a simple lookup is working.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> Unfortunately this seems to be one of these hacks for
> hierarchical interrupts that does not quite cut it.
>
> > +static int lpc32xx_gpio_to_irq_p3(struct gpio_chip *chip, unsigned int offset)
> > +       return of_irq_get_byname(chip->of_node, chip->names[offset]);
>
> This works as long as consuming drivers pick a GPIO first
> using gpiod_get() (and variants) and then convert it to an
> IRQ using gpiod_to_irq().
>
> But it doesn't work when some consumer is just picking an
> IRQ off of the node without picking the GPIO first.
> And in DT that is OK, this DT node is definately an interrupt
> controller. (Leaving out the attribute for interrupt controller
> as is currently the case doesn't really fix the issue, it is
> just inconsistent.)
>
> Look at Brian Masney's series for the qualcomm pin controller
> chips for inspiration on how to do things right, see:
> commit 9d2b563bc23acfa93e7716b3396fd2f79fa8f0cd
> and down.
>
> Also:
> https://marc.info/?l=linux-gpio&m=154959228527643&w=2
>
> Especially note how he removes the kind of hack you are
> adding in e.g.
> commit a796fab2c605d6340512c51c6c3fa1c581357993
>
> Yours,
> Linus Walleij
>

Hi Alexandre,

you can also look at the original attempt, in 2015, to replace
this driver by a new version:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386708.html

Regards,
Sylvain

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] ARM: dts: lpc32xx: add GPIO interrupts

[Alexandre Belloni]

Add the interrupts for the GPIO controller.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 arch/arm/boot/dts/lpc32xx.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
index 20b38f4ade37..9c61b1856291 100644
--- a/arch/arm/boot/dts/lpc32xx.dtsi
+++ b/arch/arm/boot/dts/lpc32xx.dtsi
@@ -392,6 +392,31 @@
 				reg = <0x40028000 0x1000>;
 				gpio-controller;
 				#gpio-cells = <3>; /* bank, pin, flags */
+				interrupts-extended = <&sic2 8 IRQ_TYPE_NONE>,
+						<&sic2 22 IRQ_TYPE_NONE>,
+						<&sic2 23 IRQ_TYPE_NONE>,
+						<&sic2 24 IRQ_TYPE_NONE>,
+						<&sic2 25 IRQ_TYPE_NONE>,
+						<&sic2 26 IRQ_TYPE_NONE>,
+						<&sic2 27 IRQ_TYPE_NONE>,
+						<&sic2 28 IRQ_TYPE_NONE>,
+						<&sic2 15 IRQ_TYPE_NONE>,
+						<&sic2 9 IRQ_TYPE_NONE>,
+						<&sic2 10 IRQ_TYPE_NONE>,
+						<&sic1 4 IRQ_TYPE_NONE>,
+						<&sic2 0 IRQ_TYPE_NONE>,
+						<&sic2 1 IRQ_TYPE_NONE>,
+						<&sic2 2 IRQ_TYPE_NONE>,
+						<&sic2 3 IRQ_TYPE_NONE>,
+						<&sic2 4 IRQ_TYPE_NONE>,
+						<&sic2 5 IRQ_TYPE_NONE>;
+				interrupt-names = "p01",
+						"gpi00", "gpi01", "gpi02",
+						"gpi03", "gpi04", "gpi05",
+						"gpi06", "gpi07", "gpi08",
+						"gpi09", "gpi19", "gpi28",
+						"gpio00", "gpio01", "gpio02",
+						"gpio03", "gpio04", "gpio05";
 			};
 
 			timer4: timer@4002c000 {
-- 
2.20.1

[PATCH v2 3/3] ARM: dts: lpc32xx: add GPIO interrupts

[Alexandre Belloni]

Add the interrupts for the GPIO controller.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---

the previous patch was missing a line, sorry about that.

Changes in v2:
 - add missing interrupt for gpi19

 arch/arm/boot/dts/lpc32xx.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
index 20b38f4ade37..a012f668ebd1 100644
--- a/arch/arm/boot/dts/lpc32xx.dtsi
+++ b/arch/arm/boot/dts/lpc32xx.dtsi
@@ -392,6 +392,32 @@
 				reg = <0x40028000 0x1000>;
 				gpio-controller;
 				#gpio-cells = <3>; /* bank, pin, flags */
+				interrupts-extended = <&sic2 8 IRQ_TYPE_NONE>,
+						<&sic2 22 IRQ_TYPE_NONE>,
+						<&sic2 23 IRQ_TYPE_NONE>,
+						<&sic2 24 IRQ_TYPE_NONE>,
+						<&sic2 25 IRQ_TYPE_NONE>,
+						<&sic2 26 IRQ_TYPE_NONE>,
+						<&sic2 27 IRQ_TYPE_NONE>,
+						<&sic2 28 IRQ_TYPE_NONE>,
+						<&sic2 15 IRQ_TYPE_NONE>,
+						<&sic2 9 IRQ_TYPE_NONE>,
+						<&sic2 10 IRQ_TYPE_NONE>,
+						<&sic2 11 IRQ_TYPE_NONE>,
+						<&sic1 4 IRQ_TYPE_NONE>,
+						<&sic2 0 IRQ_TYPE_NONE>,
+						<&sic2 1 IRQ_TYPE_NONE>,
+						<&sic2 2 IRQ_TYPE_NONE>,
+						<&sic2 3 IRQ_TYPE_NONE>,
+						<&sic2 4 IRQ_TYPE_NONE>,
+						<&sic2 5 IRQ_TYPE_NONE>;
+				interrupt-names = "p01",
+						"gpi00", "gpi01", "gpi02",
+						"gpi03", "gpi04", "gpi05",
+						"gpi06", "gpi07", "gpi08",
+						"gpi09", "gpi19", "gpi28",
+						"gpio00", "gpio01", "gpio02",
+						"gpio03", "gpio04", "gpio05";
 			};
 
 			timer4: timer@4002c000 {
-- 
2.20.1