dwapb: a bug fix a few cleanups, v2
Subsystems (3)
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
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.
---
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(+)
arch/arm/boot/dts/socfpga.dtsi +64 -0
@@ -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>;
Signature · 1 line
1.9.1
Re: [PATCH 1/7] ARM: dts: socfpga: add gpio pieces
Hi,
I saw this patch as it came in through Dinh's pull request, see below:
Quoted 32 lines · 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.
Acked-by Alan Tull <[email protected]>Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
v1…v2:
- #gpio-cells = <2>
- third gpio block has now only 27 gpiosarch/arm/boot/dts/socfpga.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)arch/arm/boot/dts/socfpga.dtsi +9
-0@@ -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 {
arch/arm/boot/dts/socfpga.dtsi line 484
Quoted 10 lines
arch/arm/boot/dts/socfpga.dtsi +10
-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
Quoted 73 lines · Olof Johansson
Hi,
I saw this patch as it came in through Dinh's pull request, see below:
Quoted 32 lines · 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.
Acked-by Alan Tull <[email protected]>Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
v1…v2:
- #gpio-cells = <2>
- third gpio block has now only 27 gpiosarch/arm/boot/dts/socfpga.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)arch/arm/boot/dts/socfpga.dtsi +9
-0@@ -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 {arch/arm/boot/dts/socfpga.dtsi line 484
Quoted 10 lines
arch/arm/boot/dts/socfpga.dtsi +10
-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>; };
Quoted 5 lines
... 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] ARM: dts: socfpga: add gpio pieces
In reply to Olof JohanssonQuoted 2 lines · Olof Johansson
... 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
PATCH 2/7 gpio: dwapb: correct gpio-cells in binding document
In reply to Sebastian Andrzej SiewiorThe example uses gpio-cells = 1 while it should be two (it is even mentioned in the text above).
--- Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt +2 -2
@@ -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>; };
Signature · 1 line
1.9.1
Re: [PATCH 2/7] gpio: dwapb: correct gpio-cells in binding document
Quoted 5 lines · Sebastian Andrzej Siewior
The example uses gpio-cells = 1 while it should be two (it is even mentioned in the text above).
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
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
Quoted 1 line · Linus Walleij
This is obviously correct so patch applied.
Thanks.
Quoted 5 lines
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
Quoted 3 lines
Yours,
Linus Walleij
Sebastian
Re: [PATCH 2/7] gpio: dwapb: correct gpio-cells in binding document
Quoted 5 lines · Sebastian Andrzej Siewior
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 3/7 gpio: dwapb: drop irq_setup_generic_chip()
In reply to Sebastian Andrzej SiewiorThe 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.
--- drivers/gpio/gpio-dwapb.c | 3 --- 1 file changed, 3 deletions(-)
drivers/gpio/gpio-dwapb.c +0 -3
@@ -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);
Signature · 1 line
1.9.1
PATCH 4/7 gpio: dwapb: use irq_linear_revmap() for the faster lookup
In reply to Sebastian Andrzej SiewiorAccording 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.
--- drivers/gpio/gpio-dwapb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-dwapb.c +1 -1
@@ -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);
Signature · 1 line
1.9.1
PATCH 5/7 gpio: dwapb: use irq_gc_lock() for locking instead bc's lock
In reply to Sebastian Andrzej SiewiorThe 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.
--- drivers/gpio/gpio-dwapb.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
drivers/gpio/gpio-dwapb.c +7 -13
@@ -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 ; }
Signature · 1 line
1.9.1
PATCH 6/7 gpio: dwapb: use a second irq chip
In reply to Sebastian Andrzej SiewiorRight 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.
--- drivers/gpio/gpio-dwapb.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)
drivers/gpio/gpio-dwapb.c +26 -13
@@ -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);
Signature · 1 line
1.9.1
Re: [PATCH 6/7] gpio: dwapb: use a second irq chip
Quoted 57 lines · 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 <[email protected]>--- drivers/gpio/gpio-dwapb.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)drivers/gpio/gpio-dwapb.c +5
-13@@ -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++) {
drivers/gpio/gpio-dwapb.c static void dwapb_configure_irqs(struct dwapb_gpio *gpio, · line 257
Quoted 10 lines
drivers/gpio/gpio-dwapb.c +10
-0+ + 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
Quoted 17 lines
drivers/gpio/gpio-dwapb.c +11
-1+ 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
Quoted 11 lines · delicious quinoa
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: 'structirq_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: 'structirq_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.
Quoted 3 lines
Alan Tull
aka
delicious quinoa
Sebastian
Re: [PATCH 6/7] gpio: dwapb: use a second irq chip
In reply to Sebastian Andrzej SiewiorQuoted 67 lines · 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 <[email protected]>--- drivers/gpio/gpio-dwapb.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)drivers/gpio/gpio-dwapb.c +15
-13@@ -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;
drivers/gpio/gpio-dwapb.c static void dwapb_configure_irqs(struct dwapb_gpio *gpio, · line 267
Quoted 10 lines
drivers/gpio/gpio-dwapb.c +10
-0+ 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
Quoted 5 lines
drivers/gpio/gpio-dwapb.c +1
-0+ }irq_set_chained_handler (irq, dwapb_irq_handler);irq_set_handler_data (irq, gpio);
Re: [PATCH 6/7] gpio: dwapb: use a second irq chip
Quoted 39 lines · Sebastian Hesselbarth
Quoted 26 lines
drivers/gpio/gpio-dwapb.c +11
-11@@ -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;drivers/gpio/gpio-dwapb.c static void dwapb_configure_irqs(struct dwapb_gpio *gpio, · line 267
Quoted 10 lines
drivers/gpio/gpio-dwapb.c +10
-0+ 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,
Quoted 1 line
IMHO the loop looks strange, especially with the (i == 0) check.
how so?
Quoted 1 line
How about unrolling it again and assign both
chip_typesindependently?
If more code makes you happy so be it. I will post the series soon with the loop unrolled.
Quoted 1 line
Sebastian
Sebastian
Re: [PATCH 6/7] gpio: dwapb: use a second irq chip
Quoted 44 lines · Sebastian Andrzej Siewior
Quoted 39 lines · Sebastian Hesselbarth
Quoted 26 lines
drivers/gpio/gpio-dwapb.c +11
-11@@ -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;drivers/gpio/gpio-dwapb.c static void dwapb_configure_irqs(struct dwapb_gpio *gpio, · line 267
Quoted 10 lines
drivers/gpio/gpio-dwapb.c +10
-0+ 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,
Quoted 1 line
IMHO the loop looks strange, especially with the (i == 0) check.
how so?
Quoted 8 lines
Quoted 1 line
How about unrolling it again and assign both
chip_typesindependently?If more code makes you happy so be it. I will post the series soon with the loop unrolled.
Quoted 1 line
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 v2 6/7 gpio: dwapb: use a second irq chip
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.
---
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(-)
drivers/gpio/gpio-dwapb.c +22 -13
@@ -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);
Signature · 6 lines
1.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip
Quoted 5 lines · 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.
Quoted 8 lines
Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Tull
Acked-byRe: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip
Quoted 17 lines · Linus Walleij
Quoted 5 lines · 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.
Quoted 8 lines
Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
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.
Quoted 5 lines
Can I have an ACK from Jamie &&|| Alan on this patch so I can apply it?
Yours,
Linus Walleij
Yes
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip
In reply to Sebastian Andrzej SiewiorQuoted 11 lines · 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 <[email protected]>---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip
Quoted 9 lines · Linus Walleij
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-printableThis is not good. Quoted-printable makes the raw patch look like that:
Quoted 8 lines
- int err, irq;
+ int err, irq, i;
=20
irq =3Dirq_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?
Quoted 1 line
Alan's ACK.
I will try to add his ack and tell git resent the patch.
Quoted 2 lines
Yours,
Linus Walleij
Sebastian
Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip
Quoted 15 lines · Sebastian Andrzej Siewior
Quoted 14 lines · Linus Walleij
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-printableThis is not good. Quoted-printable makes the raw patch look like that:
- int err, irq;
+ int err, irq, i;
=20
irq =3Dirq_of_parse_and_map(node, 0);
Quoted 5 lines
Quoted 3 lines
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
Quoted 9 lines
Quoted 2 lines
Alan's ACK.
I will try to add his ack and tell git resent the patch.
Quoted 2 lines
Yours,
Linus WalleijSebastian
Re: [PATCH 6/7 v2] gpio: dwapb: use a second irq chip
Quoted 7 lines · Alan Tull
Quoted 7 lines
Quoted 7 lines
- int err, irq;
+ int err, irq, i;
=20
irq =3Dirq_of_parse_and_map(node, 0);Notice spurious =20 and =3D encoding.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 1 line
Alan
Sebastian
PATCH v3 1/1 gpio: dwapb: use a second irq chip
In reply to Linus WalleijRight 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.
---
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(-)
drivers/gpio/gpio-dwapb.c +22 -13
@@ -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);
Signature · 1 line
2.0.0.rc4
Re: [PATCH v3] gpio: dwapb: use a second irq chip
Quoted 7 lines · 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 <[email protected]>Signed-off-by Sebastian Andrzej Siewior <[email protected]>
Re: [PATCH v3] gpio: dwapb: use a second irq chip
In reply to Sebastian Andrzej SiewiorQuoted 11 lines · 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 <[email protected]>Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
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
Quoted 1 line · Linus Walleij
OK this version applied worked fine.
Thank you.
Quoted 1 line
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?
Quoted 3 lines
Yours,
Linus Walleij
Sebastian
Re: [PATCH v3] gpio: dwapb: use a second irq chip
Quoted 7 lines · Sebastian Andrzej Siewior
What remains in my queue is:
* gpio: dwapb: use
irq_linear_revmap()for the faster lookup
* gpio: dwapb: useirq_gc_lock()for locking instead bc's lock
* gpio: dwapb: used->maskinstead 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
Quoted 9 lines · Linus Walleij
Quoted 6 lines · Sebastian Andrzej Siewior
What remains in my queue is:
* gpio: dwapb: use
irq_linear_revmap()for the faster lookup
* gpio: dwapb: useirq_gc_lock()for locking instead bc's lock
* gpio: dwapb: used->maskinstead of BIT(bit)
Quoted 5 lines
Quoted 1 line
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.
Quoted 4 lines
Yours,
Linus Walleij
Sebastian
PATCH 7/7 gpio: dwapb: use d->mask instead od BIT(bit)
In reply to Sebastian Andrzej Siewiord->mask contains exact the same information as BIT(bit) so we could save a few cycles here.
--- drivers/gpio/gpio-dwapb.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
drivers/gpio/gpio-dwapb.c +12 -13
@@ -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 ; }
Signature · 1 line
1.9.1
Re: [PATCH 7/7] gpio: dwapb: use d->mask instead od BIT(bit)
Quoted 22 lines · Sebastian Andrzej Siewior
d->maskcontains exact the same information as BIT(bit) so we could save a few cycles here.Signed-off-by Sebastian Andrzej Siewior <[email protected]>--- drivers/gpio/gpio-dwapb.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)drivers/gpio/gpio-dwapb.c +1
-1@@ -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); }
drivers/gpio/gpio-dwapb.c static void dwapb_irq_disable(struct irq_data *d) · line 129
Quoted 5 lines
drivers/gpio/gpio-dwapb.c +1
-1@@ -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)
Quoted 6 lines · Alexandre Courbot
Quoted 6 lines
drivers/gpio/gpio-dwapb.c +0
-0@@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
Quoted 6 lines
drivers/gpio/gpio-dwapb.c static void dwapb_irq_disable(struct irq_data *d) · line 129
Quoted 4 lines
drivers/gpio/gpio-dwapb.c +1
-1irq_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
PATCH v2 7/7 gpio: dwapb: use d->mask instead od BIT(bit)
In reply to Alexandre Courbotd->mask contains exact the same information as BIT(bit) so we could save a few cycles here.
---
v1…v2: proper bit delete in dwapb_irq_disable()
drivers/gpio/gpio-dwapb.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
drivers/gpio/gpio-dwapb.c +12 -13
@@ -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 ; }
Signature · 6 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7 v2] gpio: dwapb: use d->mask instead od BIT(bit)
Quoted 2 lines · Sebastian Andrzej Siewior
d->maskcontains 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.
drivers/gpio/gpio-dwapb.c
Quoted 1 line
drivers/gpio/gpio-dwapb.c +0
-0
[]
Quoted 35 lines
drivers/gpio/gpio-dwapb.c +10
-10@@ -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 ; }
Quoted 6 lines · 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.
Quoted 8 lines
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.
Okay so can we have Jamie and Sebastian H. have a look at this series?
Yours,
Linus Walleij
Quoted 16 lines · Linus Walleij
Quoted 13 lines · 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.
Quoted 4 lines
Quoted 1 line
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
Quoted 19 lines · Sebastian Hesselbarth
Quoted 18 lines · Linus Walleij
Quoted 14 lines · 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.
Quoted 8 lines
Quoted 2 lines
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