dwapb: a bug fix a few cleanups, v2

dwapb: a bug fix a few cleanups, v2

[Sebastian Andrzej Siewior]

Since Alan said that he had drop two patches from earlier series to make
it work I decided to spent some extra time to check if this is really the
case.
I dropped "gpio: dwapb: do not create the irq mapping upfront." until the
discussion there is over.

This series has been tested back ported and tested on a v3.13 kernel with
the dummy test [0] here. It was tested on the Arrow board and the dev kit. I
tested edge and level interrupts. On the Arrow board releasing the button
causes a lot of interrupts so I assume debouncing is no working well
there. On the dev kit I see only one interrupt. If I realse it really
slowly, then the extra interrupts are visible there as well but way less.

[0] http://breakpoint.cc/gpio-dwapb-test.c

Sebastian

[PATCH 1/7] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

The cycloneV has three gpio controllers, the first two with 29 gpios, the last
one with 27. This patch adds the three controller with the gpio driver which is
now sitting the gpio tree.

Cc: devicetree@vger.kernel.org
Acked-by: Alan Tull <atull@altera.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
	- #gpio-cells = <2>
	- third gpio block has now only 27 gpios

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

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 537f1a5..2a84e67 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -463,6 +463,70 @@
 			status = "disabled";
 		};
 
+		gpio@ff708000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff708000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio0: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <2>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 164 4>;
+			};
+		};
+
+		gpio@ff709000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff709000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio1: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <2>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 165 4>;
+			};
+		};
+
+		gpio@ff70a000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff70a000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio2: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <2>;
+				/*
+				 * Despite what the documentation says here, the
+				 * third gpio block has only 27 gpios available
+				 */
+				snps,nr-gpios = <27>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 166 4>;
+			};
+		};
+
 		L2: l2-cache@fffef000 {
 			compatible = "arm,pl310-cache";
 			reg = <0xfffef000 0x1000>;
-- 
1.9.1

[PATCH 2/7] gpio: dwapb: correct gpio-cells in binding document

[Sebastian Andrzej Siewior]

The example uses gpio-cells = 1 while it should be two (it is even
mentioned in the text above).

Cc: devicetree@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 91cc90c..dd5d2c0 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -41,7 +41,7 @@ gpio: gpio@20000 {
 	porta: gpio-controller@0 {
 		compatible = "snps,dw-apb-gpio-port";
 		gpio-controller;
-		#gpio-cells = <1>;
+		#gpio-cells = <2>;
 		snps,nr-gpios = <8>;
 		reg = <0>;
 		interrupt-controller;
@@ -53,7 +53,7 @@ gpio: gpio@20000 {
 	portb: gpio-controller@1 {
 		compatible = "snps,dw-apb-gpio-port";
 		gpio-controller;
-		#gpio-cells = <1>;
+		#gpio-cells = <2>;
 		snps,nr-gpios = <8>;
 		reg = <1>;
 	};
-- 
1.9.1

[PATCH 3/7] gpio: dwapb: drop irq_setup_generic_chip()

[Sebastian Andrzej Siewior]

The driver calls irq_alloc_domain_generic_chips() which creates a gc and
adds it to gc_list. The driver later then calls irq_setup_generic_chip()
which also initializes the gc and adds it to the gc_list() and this
corrupts the list. Enable LIST_DEBUG and you see the kernel complain.
This isn't required, irq_alloc_domain_generic_chips() did the init.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index ed5711f..4d25a06b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -260,9 +260,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	ct->regs.ack = GPIO_PORTA_EOI;
 	ct->regs.mask = GPIO_INTMASK;
 
-	irq_setup_generic_chip(irq_gc, IRQ_MSK(port->bgc.gc.ngpio),
-			IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
-
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
 
-- 
1.9.1

[PATCH 4/7] gpio: dwapb: use irq_linear_revmap() for the faster lookup

[Sebastian Andrzej Siewior]

According to irq_linear_revmap() comment, it is slightly faster compared
to irq_find_mapping() since we don't use a radix tree but a linear
mapping.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4d25a06b..541b893 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -92,7 +92,7 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
 
 	while (irq_status) {
 		int hwirq = fls(irq_status) - 1;
-		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
+		int gpio_irq = irq_linear_revmap(gpio->domain, hwirq);
 
 		generic_handle_irq(gpio_irq);
 		irq_status &= ~BIT(hwirq);
-- 
1.9.1

[PATCH 5/7] gpio: dwapb: use irq_gc_lock() for locking instead bc's lock

[Sebastian Andrzej Siewior]

The generic irq chip uses irq_gc_lock() for locking so the
enable/startup and type callbacks should use the same lock. None of
those registers (polarity, mask, enable) are used by the gpio part.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 541b893..752ccb1 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -20,7 +20,6 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
-#include <linux/spinlock.h>
 
 #define GPIO_SWPORTA_DR		0x00
 #define GPIO_SWPORTA_DDR	0x04
@@ -110,30 +109,26 @@ static void dwapb_irq_enable(struct irq_data *d)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&bgc->lock, flags);
+	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
 	val |= BIT(d->hwirq);
 	writel(val, gpio->regs + GPIO_INTEN);
-	spin_unlock_irqrestore(&bgc->lock, flags);
+	irq_gc_unlock(igc);
 }
 
 static void dwapb_irq_disable(struct irq_data *d)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&bgc->lock, flags);
+	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
 	val &= ~BIT(d->hwirq);
 	writel(val, gpio->regs + GPIO_INTEN);
-	spin_unlock_irqrestore(&bgc->lock, flags);
+	irq_gc_unlock(igc);
 }
 
 static int dwapb_irq_reqres(struct irq_data *d)
@@ -163,15 +158,14 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
 	int bit = d->hwirq;
-	unsigned long level, polarity, flags;
+	unsigned long level, polarity;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
 		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
 		return -EINVAL;
 
-	spin_lock_irqsave(&bgc->lock, flags);
+	irq_gc_lock(igc);
 	level = readl(gpio->regs + GPIO_INTTYPE_LEVEL);
 	polarity = readl(gpio->regs + GPIO_INT_POLARITY);
 
@@ -200,7 +194,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
-	spin_unlock_irqrestore(&bgc->lock, flags);
+	irq_gc_unlock(igc);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 6/7] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

Right new have one irq chip running always in level mode. It would nicer
to have two irq chips where one is handling level type interrupts and
the other one is doing edge interrupts. So we can have at runtime two users
where one is using edge and the other level.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 752ccb1..3c9cdda 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		break;
 	}
 
+	irq_setup_alt_chip(d, type);
+
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
 	irq_gc_unlock(igc);
@@ -207,7 +209,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	struct irq_chip_generic	*irq_gc;
 	unsigned int hwirq, ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
-	int err, irq;
+	int err, irq, i;
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	if (!gpio->domain)
 		return;
 
-	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
+	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
 					     "gpio-dwapb", handle_level_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
@@ -242,17 +244,28 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->reg_base = gpio->regs;
 	irq_gc->private = gpio;
 
-	ct = irq_gc->chip_types;
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->chip.irq_mask = irq_gc_mask_set_bit;
-	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
-	ct->chip.irq_set_type = dwapb_irq_set_type;
-	ct->chip.irq_enable = dwapb_irq_enable;
-	ct->chip.irq_disable = dwapb_irq_disable;
-	ct->chip.irq_request_resources = dwapb_irq_reqres;
-	ct->chip.irq_release_resources = dwapb_irq_relres;
-	ct->regs.ack = GPIO_PORTA_EOI;
-	ct->regs.mask = GPIO_INTMASK;
+	for (i = 0; i < 2; i++) {
+
+		ct = &irq_gc->chip_types[i];
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask = irq_gc_mask_set_bit;
+		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+		ct->chip.irq_set_type = dwapb_irq_set_type;
+		ct->chip.irq_enable = dwapb_irq_enable;
+		ct->chip.irq_disable = dwapb_irq_disable;
+		ct->chip.irq_request_resources = dwapb_irq_reqres;
+		ct->chip.irq_release_resources = dwapb_irq_relres;
+		ct->regs.ack = GPIO_PORTA_EOI;
+		ct->regs.mask = GPIO_INTMASK;
+
+		if (i == 0) {
+			ct->type = IRQ_TYPE_LEVEL_MASK;
+			ct->handler = handle_level_irq;
+		} else {
+			ct->type = IRQ_TYPE_EDGE_BOTH;
+			ct->handler = handle_edge_irq;
+		}
+	}
 
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
-- 
1.9.1

[PATCH 7/7] gpio: dwapb: use d->mask instead od BIT(bit)

[Sebastian Andrzej Siewior]

d->mask contains exact the same information as BIT(bit) so we could save
a few cycles here.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3c9cdda..ebfcf5c 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val |= BIT(d->hwirq);
+	val |= d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val &= ~BIT(d->hwirq);
+	val &= d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -158,7 +158,6 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	int bit = d->hwirq;
 	unsigned long level, polarity;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
@@ -171,24 +170,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_BOTH:
-		level |= BIT(bit);
-		dwapb_toggle_trigger(gpio, bit);
+		level |= d->mask;
+		dwapb_toggle_trigger(gpio, d->hwirq);
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		level |= BIT(bit);
-		polarity |= BIT(bit);
+		level |= d->mask;
+		polarity |= d->mask;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		level |= BIT(bit);
-		polarity &= ~BIT(bit);
+		level |= d->mask;
+		polarity &= ~d->mask;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		level &= ~BIT(bit);
-		polarity |= BIT(bit);
+		level &= ~d->mask;
+		polarity |= d->mask;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		level &= ~BIT(bit);
-		polarity &= ~BIT(bit);
+		level &= ~d->mask;
+		polarity &= ~d->mask;
 		break;
 	}
 
-- 
1.9.1

Re: [PATCH 7/7] gpio: dwapb: use d->mask instead od BIT(bit)

[Alexandre Courbot]

On Sun, Mar 23, 2014 at 1:16 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> d->mask contains exact the same information as BIT(bit) so we could save
> a few cycles here.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/gpio/gpio-dwapb.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 3c9cdda..ebfcf5c 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
>
>         irq_gc_lock(igc);
>         val = readl(gpio->regs + GPIO_INTEN);
> -       val |= BIT(d->hwirq);
> +       val |= d->mask;
>         writel(val, gpio->regs + GPIO_INTEN);
>         irq_gc_unlock(igc);
>  }
> @@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
>
>         irq_gc_lock(igc);
>         val = readl(gpio->regs + GPIO_INTEN);
> -       val &= ~BIT(d->hwirq);
> +       val &= d->mask;

Shouldn't that be ~d->mask here?

Re: [PATCH 7/7] gpio: dwapb: use d->mask instead od BIT(bit)

[Sebastian Andrzej Siewior]

On 03/25/2014 03:17 AM, Alexandre Courbot wrote:
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index 3c9cdda..ebfcf5c 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
>>
>>         irq_gc_lock(igc);
>>         val = readl(gpio->regs + GPIO_INTEN);
>> -       val &= ~BIT(d->hwirq);
>> +       val &= d->mask;
> 
> Shouldn't that be ~d->mask here?

Yes, indeed. Thanks for catching this.

Sebastian

Re: [PATCH 6/7] gpio: dwapb: use a second irq chip

[delicious quinoa]

On Sat, Mar 22, 2014 at 11:16 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/gpio/gpio-dwapb.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 752ccb1..3c9cdda 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>                 break;
>         }
>
> +       irq_setup_alt_chip(d, type);
> +
>         writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
>         writel(polarity, gpio->regs + GPIO_INT_POLARITY);
>         irq_gc_unlock(igc);
> @@ -207,7 +209,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>         struct irq_chip_generic *irq_gc;
>         unsigned int hwirq, ngpio = gc->ngpio;
>         struct irq_chip_type *ct;
> -       int err, irq;
> +       int err, irq, i;
>
>         irq = irq_of_parse_and_map(node, 0);
>         if (!irq) {
> @@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>         if (!gpio->domain)
>                 return;
>
> -       err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
> +       err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
>                                              "gpio-dwapb", handle_level_irq,
>                                              IRQ_NOREQUEST, 0,
>                                              IRQ_GC_INIT_NESTED_LOCK);
> @@ -242,17 +244,28 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>         irq_gc->reg_base = gpio->regs;
>         irq_gc->private = gpio;
>
> -       ct = irq_gc->chip_types;
> -       ct->chip.irq_ack = irq_gc_ack_set_bit;
> -       ct->chip.irq_mask = irq_gc_mask_set_bit;
> -       ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> -       ct->chip.irq_set_type = dwapb_irq_set_type;
> -       ct->chip.irq_enable = dwapb_irq_enable;
> -       ct->chip.irq_disable = dwapb_irq_disable;
> -       ct->chip.irq_request_resources = dwapb_irq_reqres;
> -       ct->chip.irq_release_resources = dwapb_irq_relres;
> -       ct->regs.ack = GPIO_PORTA_EOI;
> -       ct->regs.mask = GPIO_INTMASK;
> +       for (i = 0; i < 2; i++) {
> +
> +               ct = &irq_gc->chip_types[i];
> +               ct->chip.irq_ack = irq_gc_ack_set_bit;
> +               ct->chip.irq_mask = irq_gc_mask_set_bit;
> +               ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> +               ct->chip.irq_set_type = dwapb_irq_set_type;
> +               ct->chip.irq_enable = dwapb_irq_enable;
> +               ct->chip.irq_disable = dwapb_irq_disable;
> +               ct->chip.irq_request_resources = dwapb_irq_reqres;
> +               ct->chip.irq_release_resources = dwapb_irq_relres;

I'm trying to test against recent tag next-20140312.  Got a build error.

/home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:254:11:
error: 'struct irq_chip' has no member named 'irq_request_resources'
/home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:254:36:
error: 'dwapb_irq_reqres' undeclared (first use in this function)
/home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:255:11:
error: 'struct irq_chip' has no member named 'irq_release_resources'
/home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:255:36:
error: 'dwapb_irq_relres' undeclared (first use in this function)
Build failed...

Alan Tull
aka
delicious quinoa


> +               ct->regs.ack = GPIO_PORTA_EOI;
> +               ct->regs.mask = GPIO_INTMASK;
> +
> +               if (i == 0) {
> +                       ct->type = IRQ_TYPE_LEVEL_MASK;
> +                       ct->handler = handle_level_irq;
> +               } else {
> +                       ct->type = IRQ_TYPE_EDGE_BOTH;
> +                       ct->handler = handle_edge_irq;
> +               }
> +       }
>
>         irq_set_chained_handler(irq, dwapb_irq_handler);
>         irq_set_handler_data(irq, gpio);
> --
> 1.9.1
>

Re: [PATCH 6/7] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

On 03/25/2014 07:48 PM, delicious quinoa wrote:
> I'm trying to test against recent tag next-20140312.  Got a build error.
> 
> /home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:254:11:
> error: 'struct irq_chip' has no member named 'irq_request_resources'
> /home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:254:36:
> error: 'dwapb_irq_reqres' undeclared (first use in this function)
> /home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:255:11:
> error: 'struct irq_chip' has no member named 'irq_release_resources'
> /home/atull/repos/linux-socfpga/drivers/gpio/gpio-dwapb.c:255:36:
> error: 'dwapb_irq_relres' undeclared (first use in this function)
> Build failed...

The change in the driver got first introduced in commit 57ef0428
("gpio: switch drivers to use new callback") and it builds for me. You
probably miss the change in the core code c1bacba ("genirq: Provide
irq_request/release_resources chip callbacks").

I pushed to
	git://git.breakpoint.cc/bigeasy/linux.git gpio_dw_v3

my gpio-next + my patches + linux-next which is currently at -next-
20140325 btw. With those you should not get the build anymore.

> Alan Tull
> aka
> delicious quinoa

Sebastian

Re: dwapb: a bug fix a few cleanups, v2

[Linus Walleij]

On Sat, Mar 22, 2014 at 5:16 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Since Alan said that he had drop two patches from earlier series to make
> it work I decided to spent some extra time to check if this is really the
> case.
> I dropped "gpio: dwapb: do not create the irq mapping upfront." until the
> discussion there is over.
>
> This series has been tested back ported and tested on a v3.13 kernel with
> the dummy test [0] here. It was tested on the Arrow board and the dev kit. I
> tested edge and level interrupts. On the Arrow board releasing the button
> causes a lot of interrupts so I assume debouncing is no working well
> there. On the dev kit I see only one interrupt. If I realse it really
> slowly, then the extra interrupts are visible there as well but way less.
>
> [0] http://breakpoint.cc/gpio-dwapb-test.c

Okay so can we have Jamie and Sebastian H. have a look at this
series?

Yours,
Linus Walleij

Re: [PATCH 2/7] gpio: dwapb: correct gpio-cells in binding document

[Linus Walleij]

On Sat, Mar 22, 2014 at 5:16 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> The example uses gpio-cells = 1 while it should be two (it is even
> mentioned in the text above).
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This is obviously correct so patch applied.

How come I cannot see patch 1/7 in my mailbox :-(

I see patches 2-7/7...

Something on my end?

Yours,
Linus Walleij

Re: [PATCH 2/7] gpio: dwapb: correct gpio-cells in binding document

[Sebastian Andrzej Siewior]

On 03/25/2014 09:49 PM, Linus Walleij wrote:
> This is obviously correct so patch applied.

Thanks.

> How come I cannot see patch 1/7 in my mailbox :-(
> 
> I see patches 2-7/7...
> 
> Something on my end?

Maybe but you can still blame google for loosing your emails.
It make it to lkml:
	https://lkml.org/lkml/2014/3/22/112
> 
> Yours,
> Linus Walleij

Sebastian

Re: [PATCH 2/7] gpio: dwapb: correct gpio-cells in binding document

[Linus Walleij]

On Tue, Mar 25, 2014 at 9:57 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 03/25/2014 09:49 PM, Linus Walleij wrote:

> Maybe but you can still blame google for loosing your emails.
> It make it to lkml:
>         https://lkml.org/lkml/2014/3/22/112

OK that one should be applied to the ARM SoC tree anyway,
so I need not worry so much...

Yours,
Linus Walleij

[PATCH 7/7 v2] gpio: dwapb: use d->mask instead od BIT(bit)

[Sebastian Andrzej Siewior]

d->mask contains exact the same information as BIT(bit) so we could save
a few cycles here.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: proper bit delete in dwapb_irq_disable()

 drivers/gpio/gpio-dwapb.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index f4276fa..aedbb53 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val |= BIT(d->hwirq);
+	val |= d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val &= ~BIT(d->hwirq);
+	val &= ~d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -158,7 +158,6 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	int bit = d->hwirq;
 	unsigned long level, polarity;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
@@ -171,24 +170,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_BOTH:
-		level |= BIT(bit);
-		dwapb_toggle_trigger(gpio, bit);
+		level |= d->mask;
+		dwapb_toggle_trigger(gpio, d->hwirq);
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		level |= BIT(bit);
-		polarity |= BIT(bit);
+		level |= d->mask;
+		polarity |= d->mask;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		level |= BIT(bit);
-		polarity &= ~BIT(bit);
+		level |= d->mask;
+		polarity &= ~d->mask;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		level &= ~BIT(bit);
-		polarity |= BIT(bit);
+		level &= ~d->mask;
+		polarity |= d->mask;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		level &= ~BIT(bit);
-		polarity &= ~BIT(bit);
+		level &= ~d->mask;
+		polarity &= ~d->mask;
 		break;
 	}
 
-- 
1.9.1

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

[PATCH 7/7 v2] gpio: dwapb: use d->mask instead od BIT(bit)

[Sebastian Andrzej Siewior]

d->mask contains exact the same information as BIT(bit) so we could save
a few cycles here.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: proper bit delete in dwapb_irq_disable()

 drivers/gpio/gpio-dwapb.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index f4276fa..aedbb53 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val |= BIT(d->hwirq);
+	val |= d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val &= ~BIT(d->hwirq);
+	val &= ~d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -158,7 +158,6 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	int bit = d->hwirq;
 	unsigned long level, polarity;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
@@ -171,24 +170,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_BOTH:
-		level |= BIT(bit);
-		dwapb_toggle_trigger(gpio, bit);
+		level |= d->mask;
+		dwapb_toggle_trigger(gpio, d->hwirq);
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		level |= BIT(bit);
-		polarity |= BIT(bit);
+		level |= d->mask;
+		polarity |= d->mask;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		level |= BIT(bit);
-		polarity &= ~BIT(bit);
+		level |= d->mask;
+		polarity &= ~d->mask;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		level &= ~BIT(bit);
-		polarity |= BIT(bit);
+		level &= ~d->mask;
+		polarity |= d->mask;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		level &= ~BIT(bit);
-		polarity &= ~BIT(bit);
+		level &= ~d->mask;
+		polarity &= ~d->mask;
 		break;
 	}
 
-- 
1.9.1

Re: [PATCH 7/7 v2] gpio: dwapb: use d->mask instead od BIT(bit)

[Joe Perches]

On Tue, 2014-03-25 at 22:18 +0100, Sebastian Andrzej Siewior wrote:
> d->mask contains exact the same information as BIT(bit) so we could save
> a few cycles here.

I think you actually lose a few cycles here as
the pointer has to be dereferenced for each use.

> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
[]
> @@ -171,24 +170,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>  
>  	switch (type) {
>  	case IRQ_TYPE_EDGE_BOTH:
> -		level |= BIT(bit);
> -		dwapb_toggle_trigger(gpio, bit);
> +		level |= d->mask;
> +		dwapb_toggle_trigger(gpio, d->hwirq);
>  		break;
>  	case IRQ_TYPE_EDGE_RISING:
> -		level |= BIT(bit);
> -		polarity |= BIT(bit);
> +		level |= d->mask;
> +		polarity |= d->mask;
>  		break;
>  	case IRQ_TYPE_EDGE_FALLING:
> -		level |= BIT(bit);
> -		polarity &= ~BIT(bit);
> +		level |= d->mask;
> +		polarity &= ~d->mask;
>  		break;
>  	case IRQ_TYPE_LEVEL_HIGH:
> -		level &= ~BIT(bit);
> -		polarity |= BIT(bit);
> +		level &= ~d->mask;
> +		polarity |= d->mask;
>  		break;
>  	case IRQ_TYPE_LEVEL_LOW:
> -		level &= ~BIT(bit);
> -		polarity &= ~BIT(bit);
> +		level &= ~d->mask;
> +		polarity &= ~d->mask;
>  		break;
>  	}
>  

Re: dwapb: a bug fix a few cleanups, v2

[Sebastian Hesselbarth]

On 03/25/2014 09:45 PM, Linus Walleij wrote:
> On Sat, Mar 22, 2014 at 5:16 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> 
>> Since Alan said that he had drop two patches from earlier series to make
>> it work I decided to spent some extra time to check if this is really the
>> case.
>> I dropped "gpio: dwapb: do not create the irq mapping upfront." until the
>> discussion there is over.
>>
>> This series has been tested back ported and tested on a v3.13 kernel with
>> the dummy test [0] here. It was tested on the Arrow board and the dev kit. I
>> tested edge and level interrupts. On the Arrow board releasing the button
>> causes a lot of interrupts so I assume debouncing is no working well
>> there. On the dev kit I see only one interrupt. If I realse it really
>> slowly, then the extra interrupts are visible there as well but way less.
>>
>> [0] http://breakpoint.cc/gpio-dwapb-test.c
> 
> Okay so can we have Jamie and Sebastian H. have a look at this
> series?

Linus,

I'd love to test it and have a closer look, but we are way behind on
gpio and especially gpio irqs on mach-berlin.

I will look at the patches, but I guess if it doesn't break socfpga
or any other user of it, it is fine.

Sebastian

Re: [PATCH 6/7] gpio: dwapb: use a second irq chip

[Sebastian Hesselbarth]

On 03/22/2014 05:16 PM, Sebastian Andrzej Siewior wrote:
> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/gpio/gpio-dwapb.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 752ccb1..3c9cdda 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>  		break;
>  	}
>  
> +	irq_setup_alt_chip(d, type);
> +
>  	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
>  	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
>  	irq_gc_unlock(igc);
> @@ -207,7 +209,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	struct irq_chip_generic	*irq_gc;
>  	unsigned int hwirq, ngpio = gc->ngpio;
>  	struct irq_chip_type *ct;
> -	int err, irq;
> +	int err, irq, i;
>  
>  	irq = irq_of_parse_and_map(node, 0);
>  	if (!irq) {
> @@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	if (!gpio->domain)
>  		return;
>  
> -	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
> +	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
>  					     "gpio-dwapb", handle_level_irq,
>  					     IRQ_NOREQUEST, 0,
>  					     IRQ_GC_INIT_NESTED_LOCK);
> @@ -242,17 +244,28 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	irq_gc->reg_base = gpio->regs;
>  	irq_gc->private = gpio;
>  
> -	ct = irq_gc->chip_types;
> -	ct->chip.irq_ack = irq_gc_ack_set_bit;
> -	ct->chip.irq_mask = irq_gc_mask_set_bit;
> -	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> -	ct->chip.irq_set_type = dwapb_irq_set_type;
> -	ct->chip.irq_enable = dwapb_irq_enable;
> -	ct->chip.irq_disable = dwapb_irq_disable;
> -	ct->chip.irq_request_resources = dwapb_irq_reqres;
> -	ct->chip.irq_release_resources = dwapb_irq_relres;
> -	ct->regs.ack = GPIO_PORTA_EOI;
> -	ct->regs.mask = GPIO_INTMASK;
> +	for (i = 0; i < 2; i++) {
> +
> +		ct = &irq_gc->chip_types[i];
> +		ct->chip.irq_ack = irq_gc_ack_set_bit;
> +		ct->chip.irq_mask = irq_gc_mask_set_bit;
> +		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> +		ct->chip.irq_set_type = dwapb_irq_set_type;
> +		ct->chip.irq_enable = dwapb_irq_enable;
> +		ct->chip.irq_disable = dwapb_irq_disable;
> +		ct->chip.irq_request_resources = dwapb_irq_reqres;
> +		ct->chip.irq_release_resources = dwapb_irq_relres;
> +		ct->regs.ack = GPIO_PORTA_EOI;
> +		ct->regs.mask = GPIO_INTMASK;
> +
> +		if (i == 0) {
> +			ct->type = IRQ_TYPE_LEVEL_MASK;
> +			ct->handler = handle_level_irq;
> +		} else {
> +			ct->type = IRQ_TYPE_EDGE_BOTH;
> +			ct->handler = handle_edge_irq;
> +		}

Sebastian,

IMHO the loop looks strange, especially with the (i == 0) check.

How about unrolling it again and assign both chip_types independently?

Sebastian

> +	}
>  
>  	irq_set_chained_handler(irq, dwapb_irq_handler);
>  	irq_set_handler_data(irq, gpio);
> 

Re: dwapb: a bug fix a few cleanups, v2

[Sebastian Hesselbarth]

On 03/25/2014 10:26 PM, Sebastian Hesselbarth wrote:
> On 03/25/2014 09:45 PM, Linus Walleij wrote:
>> On Sat, Mar 22, 2014 at 5:16 PM, Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de> wrote:
>>
>>> Since Alan said that he had drop two patches from earlier series to make
>>> it work I decided to spent some extra time to check if this is really the
>>> case.
>>> I dropped "gpio: dwapb: do not create the irq mapping upfront." until the
>>> discussion there is over.
>>>
>>> This series has been tested back ported and tested on a v3.13 kernel with
>>> the dummy test [0] here. It was tested on the Arrow board and the dev kit. I
>>> tested edge and level interrupts. On the Arrow board releasing the button
>>> causes a lot of interrupts so I assume debouncing is no working well
>>> there. On the dev kit I see only one interrupt. If I realse it really
>>> slowly, then the extra interrupts are visible there as well but way less.
>>>
>>> [0] http://breakpoint.cc/gpio-dwapb-test.c
>>
>> Okay so can we have Jamie and Sebastian H. have a look at this
>> series?
> 
> I'd love to test it and have a closer look, but we are way behind on
> gpio and especially gpio irqs on mach-berlin.
> 
> I will look at the patches, but I guess if it doesn't break socfpga
> or any other user of it, it is fine.

Except a small comment about for loop in 6/7 the dwapb related patches
look good to me.

Sebastian

Re: [PATCH 6/7] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

On 03/25/2014 10:36 PM, Sebastian Hesselbarth wrote:
>> @@ -242,17 +244,28 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>>  	irq_gc->reg_base = gpio->regs;
>>  	irq_gc->private = gpio;
>>  
>> -	ct = irq_gc->chip_types;
>> -	ct->chip.irq_ack = irq_gc_ack_set_bit;
>> -	ct->chip.irq_mask = irq_gc_mask_set_bit;
>> -	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>> -	ct->chip.irq_set_type = dwapb_irq_set_type;
>> -	ct->chip.irq_enable = dwapb_irq_enable;
>> -	ct->chip.irq_disable = dwapb_irq_disable;
>> -	ct->chip.irq_request_resources = dwapb_irq_reqres;
>> -	ct->chip.irq_release_resources = dwapb_irq_relres;
>> -	ct->regs.ack = GPIO_PORTA_EOI;
>> -	ct->regs.mask = GPIO_INTMASK;
>> +	for (i = 0; i < 2; i++) {
>> +
>> +		ct = &irq_gc->chip_types[i];
>> +		ct->chip.irq_ack = irq_gc_ack_set_bit;
>> +		ct->chip.irq_mask = irq_gc_mask_set_bit;
>> +		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>> +		ct->chip.irq_set_type = dwapb_irq_set_type;
>> +		ct->chip.irq_enable = dwapb_irq_enable;
>> +		ct->chip.irq_disable = dwapb_irq_disable;
>> +		ct->chip.irq_request_resources = dwapb_irq_reqres;
>> +		ct->chip.irq_release_resources = dwapb_irq_relres;
>> +		ct->regs.ack = GPIO_PORTA_EOI;
>> +		ct->regs.mask = GPIO_INTMASK;
>> +
>> +		if (i == 0) {
>> +			ct->type = IRQ_TYPE_LEVEL_MASK;
>> +			ct->handler = handle_level_irq;
>> +		} else {
>> +			ct->type = IRQ_TYPE_EDGE_BOTH;
>> +			ct->handler = handle_edge_irq;
>> +		}
> 
> Sebastian,
> 
> IMHO the loop looks strange, especially with the (i == 0) check.

how so?

> How about unrolling it again and assign both chip_types independently?

If more code makes you happy so be it. I will post the series soon with
the loop unrolled.

> Sebastian

Sebastian

Re: [PATCH 6/7] gpio: dwapb: use a second irq chip

[delicious quinoa]

On Mon, Apr 7, 2014 at 4:59 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 03/25/2014 10:36 PM, Sebastian Hesselbarth wrote:
>>> @@ -242,17 +244,28 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>>>      irq_gc->reg_base = gpio->regs;
>>>      irq_gc->private = gpio;
>>>
>>> -    ct = irq_gc->chip_types;
>>> -    ct->chip.irq_ack = irq_gc_ack_set_bit;
>>> -    ct->chip.irq_mask = irq_gc_mask_set_bit;
>>> -    ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>>> -    ct->chip.irq_set_type = dwapb_irq_set_type;
>>> -    ct->chip.irq_enable = dwapb_irq_enable;
>>> -    ct->chip.irq_disable = dwapb_irq_disable;
>>> -    ct->chip.irq_request_resources = dwapb_irq_reqres;
>>> -    ct->chip.irq_release_resources = dwapb_irq_relres;
>>> -    ct->regs.ack = GPIO_PORTA_EOI;
>>> -    ct->regs.mask = GPIO_INTMASK;
>>> +    for (i = 0; i < 2; i++) {
>>> +
>>> +            ct = &irq_gc->chip_types[i];
>>> +            ct->chip.irq_ack = irq_gc_ack_set_bit;
>>> +            ct->chip.irq_mask = irq_gc_mask_set_bit;
>>> +            ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>>> +            ct->chip.irq_set_type = dwapb_irq_set_type;
>>> +            ct->chip.irq_enable = dwapb_irq_enable;
>>> +            ct->chip.irq_disable = dwapb_irq_disable;
>>> +            ct->chip.irq_request_resources = dwapb_irq_reqres;
>>> +            ct->chip.irq_release_resources = dwapb_irq_relres;
>>> +            ct->regs.ack = GPIO_PORTA_EOI;
>>> +            ct->regs.mask = GPIO_INTMASK;
>>> +
>>> +            if (i == 0) {
>>> +                    ct->type = IRQ_TYPE_LEVEL_MASK;
>>> +                    ct->handler = handle_level_irq;
>>> +            } else {
>>> +                    ct->type = IRQ_TYPE_EDGE_BOTH;
>>> +                    ct->handler = handle_edge_irq;
>>> +            }
>>
>> Sebastian,
>>
>> IMHO the loop looks strange, especially with the (i == 0) check.
>
> how so?
>
>> How about unrolling it again and assign both chip_types independently?
>
> If more code makes you happy so be it. I will post the series soon with
> the loop unrolled.
>
>> Sebastian
>
> Sebastian

You could keep the loop but take out the if (i==0).   Set the type and
handler after the loop:

irq_gc->chip_types[0]->type = IRQ_TYPE_LEVEL_MASK;
irq_gc->chip_types[0]->handler = handle_level_irq;
irq_gc->chip_types[1]->type = IRQ_TYPE_EDGE_BOTH;
irq_gc->chip_types[1]->handler = handle_edge_irq;

Alan Tull
aka
delicious quinoa

[PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

Right new have one irq chip running always in level mode. It would nicer
to have two irq chips where one is handling level type interrupts and
the other one is doing edge interrupts. So we can have at runtime two users
where one is using edge and the other level.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
I am very sorry for the delay. I assumed that I've already fixed the
patch sent it out. Just realized that it was not the case.

v1…v2:
    - using the lopp again but breaking the assignment of type and
      handler out of the loop as suggested by delicious quinoa.

 drivers/gpio/gpio-dwapb.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 752ccb1..ca36f11 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		break;
 	}
 
+	irq_setup_alt_chip(d, type);
+
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
 	irq_gc_unlock(igc);
@@ -207,7 +209,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	struct irq_chip_generic	*irq_gc;
 	unsigned int hwirq, ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
-	int err, irq;
+	int err, irq, i;
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	if (!gpio->domain)
 		return;
 
-	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
+	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
 					     "gpio-dwapb", handle_level_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
@@ -242,17 +244,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->reg_base = gpio->regs;
 	irq_gc->private = gpio;
 
-	ct = irq_gc->chip_types;
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->chip.irq_mask = irq_gc_mask_set_bit;
-	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
-	ct->chip.irq_set_type = dwapb_irq_set_type;
-	ct->chip.irq_enable = dwapb_irq_enable;
-	ct->chip.irq_disable = dwapb_irq_disable;
-	ct->chip.irq_request_resources = dwapb_irq_reqres;
-	ct->chip.irq_release_resources = dwapb_irq_relres;
-	ct->regs.ack = GPIO_PORTA_EOI;
-	ct->regs.mask = GPIO_INTMASK;
+	for (i = 0; i < 2; i++) {
+		ct = &irq_gc->chip_types[i];
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask = irq_gc_mask_set_bit;
+		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+		ct->chip.irq_set_type = dwapb_irq_set_type;
+		ct->chip.irq_enable = dwapb_irq_enable;
+		ct->chip.irq_disable = dwapb_irq_disable;
+		ct->chip.irq_request_resources = dwapb_irq_reqres;
+		ct->chip.irq_release_resources = dwapb_irq_relres;
+		ct->regs.ack = GPIO_PORTA_EOI;
+		ct->regs.mask = GPIO_INTMASK;
+		ct->type = IRQ_TYPE_LEVEL_MASK;
+	}
+
+	irq_gc->chip_types[0].type = IRQ_TYPE_LEVEL_MASK;
+	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
+	irq_gc->chip_types[1].handler = handle_edge_irq;
 
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
-- 
1.9.2

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

[PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

Right new have one irq chip running always in level mode. It would nicer
to have two irq chips where one is handling level type interrupts and
the other one is doing edge interrupts. So we can have at runtime two users
where one is using edge and the other level.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
I am very sorry for the delay. I assumed that I've already fixed the
patch sent it out. Just realized that it was not the case.

v1…v2:
    - using the lopp again but breaking the assignment of type and
      handler out of the loop as suggested by delicious quinoa.

 drivers/gpio/gpio-dwapb.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 752ccb1..ca36f11 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		break;
 	}
 
+	irq_setup_alt_chip(d, type);
+
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
 	irq_gc_unlock(igc);
@@ -207,7 +209,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	struct irq_chip_generic	*irq_gc;
 	unsigned int hwirq, ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
-	int err, irq;
+	int err, irq, i;
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	if (!gpio->domain)
 		return;
 
-	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
+	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
 					     "gpio-dwapb", handle_level_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
@@ -242,17 +244,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->reg_base = gpio->regs;
 	irq_gc->private = gpio;
 
-	ct = irq_gc->chip_types;
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->chip.irq_mask = irq_gc_mask_set_bit;
-	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
-	ct->chip.irq_set_type = dwapb_irq_set_type;
-	ct->chip.irq_enable = dwapb_irq_enable;
-	ct->chip.irq_disable = dwapb_irq_disable;
-	ct->chip.irq_request_resources = dwapb_irq_reqres;
-	ct->chip.irq_release_resources = dwapb_irq_relres;
-	ct->regs.ack = GPIO_PORTA_EOI;
-	ct->regs.mask = GPIO_INTMASK;
+	for (i = 0; i < 2; i++) {
+		ct = &irq_gc->chip_types[i];
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask = irq_gc_mask_set_bit;
+		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+		ct->chip.irq_set_type = dwapb_irq_set_type;
+		ct->chip.irq_enable = dwapb_irq_enable;
+		ct->chip.irq_disable = dwapb_irq_disable;
+		ct->chip.irq_request_resources = dwapb_irq_reqres;
+		ct->chip.irq_release_resources = dwapb_irq_relres;
+		ct->regs.ack = GPIO_PORTA_EOI;
+		ct->regs.mask = GPIO_INTMASK;
+		ct->type = IRQ_TYPE_LEVEL_MASK;
+	}
+
+	irq_gc->chip_types[0].type = IRQ_TYPE_LEVEL_MASK;
+	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
+	irq_gc->chip_types[1].handler = handle_edge_irq;
 
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
-- 
1.9.2

Re: [PATCH 1/7] ARM: dts: socfpga: add gpio pieces

[Olof Johansson]

Hi,

I saw this patch as it came in through Dinh's pull request, see below:


On Sat, Mar 22, 2014 at 9:16 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> The cycloneV has three gpio controllers, the first two with 29 gpios, the last
> one with 27. This patch adds the three controller with the gpio driver which is
> now sitting the gpio tree.
>
> Cc: devicetree@vger.kernel.org
> Acked-by: Alan Tull <atull@altera.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2:
>         - #gpio-cells = <2>
>         - third gpio block has now only 27 gpios
>
>  arch/arm/boot/dts/socfpga.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 537f1a5..2a84e67 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -463,6 +463,70 @@
>                         status = "disabled";
>                 };
>
> +               gpio@ff708000 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       compatible = "snps,dw-apb-gpio";
> +                       reg = <0xff708000 0x1000>;
> +                       clocks = <&per_base_clk>;
> +                       status = "disabled";
> +
> +                       gpio0: gpio-controller@0 {
> +                               compatible = "snps,dw-apb-gpio-port";
> +                               gpio-controller;
> +                               #gpio-cells = <2>;
> +                               snps,nr-gpios = <29>;
> +                               reg = <0>;
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +                               interrupts = <0 164 4>;
> +                       };
> +               };

This is an odd setup. We usually would have it all in one node, since
the @ff708000 is the GPIO controller, instead of adding a subnode with
the actual GPIO info.

So I would have expected something more like:


gpio0: gpio-controller@ff708000 {
                       #address-cells = <1>;
                       #size-cells = <0>;
                       compatible = "snps,dw-apb-gpio";
                       reg = <0xff708000 0x1000>;
                       interrupts = <0 164 4>;
                      clocks = <&per_base_clk>;
                       status = "disabled";
                       gpio-controller;
                       #gpio-cells = <2>;
                       snps,nr-gpios = <29>;
                       interrupt-controller;
                       #interrupt-cells = <2>;
               };


... or is there some underlying reason for having the two-layer
approach that isn't obvious from this device tree?


-Olof

Re: [PATCH 1/7] ARM: dts: socfpga: add gpio pieces

[Alan Tull]

On Mon, May 5, 2014 at 5:02 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> Hi,
>
> I saw this patch as it came in through Dinh's pull request, see below:
>
>
> On Sat, Mar 22, 2014 at 9:16 AM, Sebastian Andrzej Siewior
> <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>> The cycloneV has three gpio controllers, the first two with 29 gpios, the last
>> one with 27. This patch adds the three controller with the gpio driver which is
>> now sitting the gpio tree.
>>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Acked-by: Alan Tull <atull-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>> ---
>> v1…v2:
>>         - #gpio-cells = <2>
>>         - third gpio block has now only 27 gpios
>>
>>  arch/arm/boot/dts/socfpga.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 64 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 537f1a5..2a84e67 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -463,6 +463,70 @@
>>                         status = "disabled";
>>                 };
>>
>> +               gpio@ff708000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff708000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio0: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <2>;
>> +                               snps,nr-gpios = <29>;
>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 164 4>;
>> +                       };
>> +               };
>
> This is an odd setup. We usually would have it all in one node, since
> the @ff708000 is the GPIO controller, instead of adding a subnode with
> the actual GPIO info.
>
> So I would have expected something more like:
>
>
> gpio0: gpio-controller@ff708000 {
>                        #address-cells = <1>;
>                        #size-cells = <0>;
>                        compatible = "snps,dw-apb-gpio";
>                        reg = <0xff708000 0x1000>;
>                        interrupts = <0 164 4>;
>                       clocks = <&per_base_clk>;
>                        status = "disabled";
>                        gpio-controller;
>                        #gpio-cells = <2>;
>                        snps,nr-gpios = <29>;
>                        interrupt-controller;
>                        #interrupt-cells = <2>;
>                };
>
>
> ... or is there some underlying reason for having the two-layer
> approach that isn't obvious from this device tree?
>
>
> -Olof

The synopsys gpio ip can have 1, 2, or 3 gpio ports of varying widths
per IP block.  The simpler bindings assume one gpio port per IP block.

Alan Tull
aka
delicious quinoa
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] ARM: dts: socfpga: add gpio pieces

[Alan Tull]

On Mon, May 5, 2014 at 5:02 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> I saw this patch as it came in through Dinh's pull request, see below:
>
>
> On Sat, Mar 22, 2014 at 9:16 AM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>> The cycloneV has three gpio controllers, the first two with 29 gpios, the last
>> one with 27. This patch adds the three controller with the gpio driver which is
>> now sitting the gpio tree.
>>
>> Cc: devicetree@vger.kernel.org
>> Acked-by: Alan Tull <atull@altera.com>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>> v1…v2:
>>         - #gpio-cells = <2>
>>         - third gpio block has now only 27 gpios
>>
>>  arch/arm/boot/dts/socfpga.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 64 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 537f1a5..2a84e67 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -463,6 +463,70 @@
>>                         status = "disabled";
>>                 };
>>
>> +               gpio@ff708000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff708000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio0: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <2>;
>> +                               snps,nr-gpios = <29>;
>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 164 4>;
>> +                       };
>> +               };
>
> This is an odd setup. We usually would have it all in one node, since
> the @ff708000 is the GPIO controller, instead of adding a subnode with
> the actual GPIO info.
>
> So I would have expected something more like:
>
>
> gpio0: gpio-controller@ff708000 {
>                        #address-cells = <1>;
>                        #size-cells = <0>;
>                        compatible = "snps,dw-apb-gpio";
>                        reg = <0xff708000 0x1000>;
>                        interrupts = <0 164 4>;
>                       clocks = <&per_base_clk>;
>                        status = "disabled";
>                        gpio-controller;
>                        #gpio-cells = <2>;
>                        snps,nr-gpios = <29>;
>                        interrupt-controller;
>                        #interrupt-cells = <2>;
>                };
>
>
> ... or is there some underlying reason for having the two-layer
> approach that isn't obvious from this device tree?
>
>
> -Olof

The synopsys gpio ip can have 1, 2, or 3 gpio ports of varying widths
per IP block.  The simpler bindings assume one gpio port per IP block.

Alan Tull
aka
delicious quinoa

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Linus Walleij]

On Wed, Apr 30, 2014 at 1:13 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I am very sorry for the delay. I assumed that I've already fixed the
> patch sent it out. Just realized that it was not the case.
>
> v1…v2:
>     - using the lopp again but breaking the assignment of type and
>       handler out of the loop as suggested by delicious quinoa.

Can I have an ACK from Jamie &&|| Alan on this patch so I can apply
it?

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

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Linus Walleij]

On Wed, Apr 30, 2014 at 1:13 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I am very sorry for the delay. I assumed that I've already fixed the
> patch sent it out. Just realized that it was not the case.
>
> v1…v2:
>     - using the lopp again but breaking the assignment of type and
>       handler out of the loop as suggested by delicious quinoa.

Can I have an ACK from Jamie &&|| Alan on this patch so I can apply
it?

Yours,
Linus Walleij

Re: [PATCH 1/7] ARM: dts: socfpga: add gpio pieces

[Linus Walleij]

On Tue, May 6, 2014 at 12:02 AM, Olof Johansson <olof@lixom.net> wrote:

> ... or is there some underlying reason for having the two-layer
> approach that isn't obvious from this device tree?

There is, as explained by Alan. One coherent memory range with
several individual ports.

Yours,
Linus Walleij

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Alan Tull]

On Fri, May 9, 2014 at 6:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Apr 30, 2014 at 1:13 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>
>> Right new have one irq chip running always in level mode. It would nicer
>> to have two irq chips where one is handling level type interrupts and
>> the other one is doing edge interrupts. So we can have at runtime two users
>> where one is using edge and the other level.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>> I am very sorry for the delay. I assumed that I've already fixed the
>> patch sent it out. Just realized that it was not the case.
>>
>> v1…v2:
>>     - using the lopp again but breaking the assignment of type and
>>       handler out of the loop as suggested by delicious quinoa.
>
> Can I have an ACK from Jamie &&|| Alan on this patch so I can apply
> it?
>
> Yours,
> Linus Walleij

Yes

Acked-by: Alan Tull <delicious.quinoa@gmail.com>

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

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Alan Tull]

On Fri, May 9, 2014 at 6:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Apr 30, 2014 at 1:13 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>
>> Right new have one irq chip running always in level mode. It would nicer
>> to have two irq chips where one is handling level type interrupts and
>> the other one is doing edge interrupts. So we can have at runtime two users
>> where one is using edge and the other level.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>> I am very sorry for the delay. I assumed that I've already fixed the
>> patch sent it out. Just realized that it was not the case.
>>
>> v1…v2:
>>     - using the lopp again but breaking the assignment of type and
>>       handler out of the loop as suggested by delicious quinoa.
>
> Can I have an ACK from Jamie &&|| Alan on this patch so I can apply
> it?
>
> Yours,
> Linus Walleij

Yes

Acked-by: Alan Tull <delicious.quinoa@gmail.com>

Alan

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Linus Walleij]

On Wed, Apr 30, 2014 at 1:13 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I am very sorry for the delay. I assumed that I've already fixed the
> patch sent it out. Just realized that it was not the case.
>
> v1…v2:

OK this patch is ACKed by Alan so I would like to apply it but:

Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

This is not good. Quoted-printable makes the raw patch look like
that:

- int err, irq;
+ int err, irq, i;
=20
  irq =3D irq_of_parse_and_map(node, 0);

Notice spurious =20 and =3D encoding.

Please resend in ISO8859-1 and take this opportunity to add
Alan's ACK.

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

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Linus Walleij]

On Wed, Apr 30, 2014 at 1:13 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I am very sorry for the delay. I assumed that I've already fixed the
> patch sent it out. Just realized that it was not the case.
>
> v1…v2:

OK this patch is ACKed by Alan so I would like to apply it but:

Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

This is not good. Quoted-printable makes the raw patch look like
that:

- int err, irq;
+ int err, irq, i;
=20
  irq =3D irq_of_parse_and_map(node, 0);

Notice spurious =20 and =3D encoding.

Please resend in ISO8859-1 and take this opportunity to add
Alan's ACK.

Yours,
Linus Walleij

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

On 05/16/2014 05:01 PM, Linus Walleij wrote:
> OK this patch is ACKed by Alan so I would like to apply it but:
> 
> Content-Type: text/plain; charset=utf-8
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> This is not good. Quoted-printable makes the raw patch look like
> that:
> 
> - int err, irq;
> + int err, irq, i;
> =20
>   irq =3D irq_of_parse_and_map(node, 0);
> 
> Notice spurious =20 and =3D encoding.
> 
> Please resend in ISO8859-1 and take this opportunity to add

doesn't "git am" handle this on its own?

> Alan's ACK.

I will try to add his ack and tell git resent the patch.

> Yours,
> Linus Walleij

Sebastian

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Alan Tull]

On Thu, May 22, 2014 at 4:42 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 05/16/2014 05:01 PM, Linus Walleij wrote:
>> OK this patch is ACKed by Alan so I would like to apply it but:
>>
>> Content-Type: text/plain; charset=utf-8
>> Content-Disposition: inline
>> Content-Transfer-Encoding: quoted-printable
>>
>> This is not good. Quoted-printable makes the raw patch look like
>> that:
>>
>> - int err, irq;
>> + int err, irq, i;
>> =20
>>   irq =3D irq_of_parse_and_map(node, 0);
>>
>> Notice spurious =20 and =3D encoding.
>>
>> Please resend in ISO8859-1 and take this opportunity to add
>
> doesn't "git am" handle this on its own?

Neither 'git am' nor 'patch -p1' can apply this patch.  When I was
testing it, I assumed those '=3D' were  a product of my webmail, so I
manually edited the patch to get it to apply.

Alan

>
>> Alan's ACK.
>
> I will try to add his ack and tell git resent the patch.
>
>> Yours,
>> Linus Walleij
>
> Sebastian

[PATCH v3] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

Right new have one irq chip running always in level mode. It would nicer
to have two irq chips where one is handling level type interrupts and
the other one is doing edge interrupts. So we can have at runtime two users
where one is using edge and the other level.

Acked-by: Alan Tull <delicious.quinoa@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2..v3: added acked-by
v1..v2 - using the lopp again but breaking the assignment of type and
	handler out of the loop as suggested by delicious quinoa.

 drivers/gpio/gpio-dwapb.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 34b339b..5a51fd0 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		break;
 	}
 
+	irq_setup_alt_chip(d, type);
+
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
 	irq_gc_unlock(igc);
@@ -207,7 +209,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	struct irq_chip_generic	*irq_gc;
 	unsigned int hwirq, ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
-	int err, irq;
+	int err, irq, i;
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	if (!gpio->domain)
 		return;
 
-	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
+	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
 					     "gpio-dwapb", handle_level_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
@@ -242,17 +244,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->reg_base = gpio->regs;
 	irq_gc->private = gpio;
 
-	ct = irq_gc->chip_types;
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->chip.irq_mask = irq_gc_mask_set_bit;
-	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
-	ct->chip.irq_set_type = dwapb_irq_set_type;
-	ct->chip.irq_enable = dwapb_irq_enable;
-	ct->chip.irq_disable = dwapb_irq_disable;
-	ct->chip.irq_request_resources = dwapb_irq_reqres;
-	ct->chip.irq_release_resources = dwapb_irq_relres;
-	ct->regs.ack = GPIO_PORTA_EOI;
-	ct->regs.mask = GPIO_INTMASK;
+	for (i = 0; i < 2; i++) {
+		ct = &irq_gc->chip_types[i];
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask = irq_gc_mask_set_bit;
+		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+		ct->chip.irq_set_type = dwapb_irq_set_type;
+		ct->chip.irq_enable = dwapb_irq_enable;
+		ct->chip.irq_disable = dwapb_irq_disable;
+		ct->chip.irq_request_resources = dwapb_irq_reqres;
+		ct->chip.irq_release_resources = dwapb_irq_relres;
+		ct->regs.ack = GPIO_PORTA_EOI;
+		ct->regs.mask = GPIO_INTMASK;
+		ct->type = IRQ_TYPE_LEVEL_MASK;
+	}
+
+	irq_gc->chip_types[0].type = IRQ_TYPE_LEVEL_MASK;
+	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
+	irq_gc->chip_types[1].handler = handle_edge_irq;
 
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
-- 
2.0.0.rc4

Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

On 2014-05-22 09:08:20 [-0500], Alan Tull wrote:
> >> - int err, irq;
> >> + int err, irq, i;
> >> =20
> >>   irq =3D irq_of_parse_and_map(node, 0);
> >>
> >> Notice spurious =20 and =3D encoding.
> >>
> >> Please resend in ISO8859-1 and take this opportunity to add
> >
> > doesn't "git am" handle this on its own?
> 
> Neither 'git am' nor 'patch -p1' can apply this patch.  When I was
> testing it, I assumed those '=3D' were  a product of my webmail, so I
> manually edited the patch to get it to apply.

I've resent the email now.
btw: I used mutt to save the email I've sent and applied to git tree with
git-am and it did not complain about the encoding. Then I looked at the the
I email I received from vger and it was
|Content-Type: text/plain; charset=utf-8
|Content-Disposition: inline
|Content-Transfer-Encoding: 8BIT
so I did not receive the =20 & =3D in it.

> Alan

Sebastian

Re: [PATCH v3] gpio: dwapb: use a second irq chip

[Jamie Iles]

On Mon, May 26, 2014 at 10:58:14PM +0200, Sebastian Andrzej Siewior wrote:
> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
> 
> Acked-by: Alan Tull <delicious.quinoa@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Jamie Iles <jamie@jamieiles.com>

Re: [PATCH v3] gpio: dwapb: use a second irq chip

[Linus Walleij]

On Mon, May 26, 2014 at 10:58 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Right new have one irq chip running always in level mode. It would nicer
> to have two irq chips where one is handling level type interrupts and
> the other one is doing edge interrupts. So we can have at runtime two users
> where one is using edge and the other level.
>
> Acked-by: Alan Tull <delicious.quinoa@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v2..v3: added acked-by
> v1..v2 - using the lopp again but breaking the assignment of type and
>         handler out of the loop as suggested by delicious quinoa.

OK this version applied worked fine.

Please check the result because I had to rebase it a little.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

On 05/27/2014 04:21 PM, Linus Walleij wrote:
> OK this version applied worked fine.

Thank you.

> Please check the result because I had to rebase it a little.

Looks good. What remains in my queue is:

* gpio: dwapb: use irq_linear_revmap() for the faster lookup
* gpio: dwapb: use irq_gc_lock() for locking instead bc's lock
* gpio: dwapb: use d->mask instead of BIT(bit)

what do we do about those?

> Yours,
> Linus Walleij
> 
Sebastian

Re: [PATCH v3] gpio: dwapb: use a second irq chip

[Linus Walleij]

On Tue, May 27, 2014 at 6:30 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> What remains in my queue is:
>
> * gpio: dwapb: use irq_linear_revmap() for the faster lookup
> * gpio: dwapb: use irq_gc_lock() for locking instead bc's lock
> * gpio: dwapb: use d->mask instead of BIT(bit)
>
> what do we do about those?

Rebase on my devel branch and resend please ... but for linear
revmap I'd much prefer some effort being put into using the
gpiolib irqchip helpers with generic chips instead.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

On 05/28/2014 11:01 AM, Linus Walleij wrote:
> On Tue, May 27, 2014 at 6:30 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> 
>> What remains in my queue is:
>>
>> * gpio: dwapb: use irq_linear_revmap() for the faster lookup
>> * gpio: dwapb: use irq_gc_lock() for locking instead bc's lock
>> * gpio: dwapb: use d->mask instead of BIT(bit)
>>
>> what do we do about those?
> 
> Rebase on my devel branch and resend please ... but for linear
> revmap I'd much prefer some effort being put into using the
> gpiolib irqchip helpers with generic chips instead.

Hmmm. I would have to look what this is about. The irq_linear_revmap()
lookup is a linear lookup.

> 
> Yours,
> Linus Walleij
> 

Sebastian