Re: [PATCH 2/3] gpio: dwapb: add support for GPIO interrupts

From: Sebastian Hesselbarth
Date: Tue Nov 05 2013 - 16:32:14 EST

On 11/05/2013 05:55 PM, Alan Tull wrote:
From: Jamie Iles <jamie@xxxxxxxxxxxxx>

The controller supports interrupts on bank A (up to 8 interrupt

s/bank/port/g

sources). Use the generic IRQ chip to implement interrupt support for
this bank.

Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
Cc: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
Acked-by: Rob Herring <rob.herring@xxxxxxxxxxx>
Signed-off-by: Jamie Iles <jamie@xxxxxxxxxxxxx>
---
drivers/gpio/Kconfig | 2 +-
drivers/gpio/gpio-dwapb.c | 181 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 182 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c5e7868..4cc26ed 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -125,7 +125,7 @@ config GPIO_DWAPB
bool "Synopsys DesignWare APB GPIO driver"
select GPIO_GENERIC
select GENERIC_IRQ_CHIP
- depends on OF_GPIO
+ depends on OF_GPIO && IRQ_DOMAIN
help
Say Y or M here to build support for the Synopsys DesignWare APB
GPIO block. This requires device tree support.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3a28697..64c7183 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -10,12 +10,22 @@
#include <linux/init.h>
#include <linux/io.h>
#include <linux/ioport.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/module.h>
#include <linux/basic_mmio_gpio.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/of_irq.h>
#include <linux/platform_device.h>

+#define INT_EN_REG_OFFS 0x30
+#define INT_MASK_REG_OFFS 0x34
+#define INT_TYPE_REG_OFFS 0x38
+#define INT_POLARITY_REG_OFFS 0x3c
+#define INT_STATUS_REG_OFFS 0x40
+#define EOI_REG_OFFS 0x4c

For sane register names, check [1] pp. 1271. I also suggest
to get rid of _REG_OFFS.

[1] http://www.altera.com/literature/hb/arria-v/hps.pdf

struct dwapb_gpio;

struct dwapb_gpio_bank {
@@ -31,6 +41,8 @@ struct dwapb_gpio {
void __iomem *regs;
struct dwapb_gpio_bank *banks;
unsigned int nr_banks;
+ struct irq_chip_generic *irq_gc;
+ unsigned long toggle_edge;
};

static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
@@ -44,6 +56,168 @@ static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node)
return nr_banks;
}

+static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+ struct dwapb_gpio_bank *bank = container_of(bgc, struct
+ dwapb_gpio_bank, bgc);
+ struct dwapb_gpio *gpio = bank->gpio;
+
+ return irq_domain_to_irq(&gpio->irq_gc->domain, offset);
+}
+
+static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
+{
+ u32 v = readl(gpio->regs + INT_TYPE_REG_OFFS);

s/readl/readl_relaxed/g
s/writel/writel_relaxed/g ?

+ if (gpio_get_value(gpio->banks[0].bgc.gc.base + offs))
+ v &= ~BIT(offs);
+ else
+ v |= BIT(offs);
+
+ writel(v, gpio->regs + INT_TYPE_REG_OFFS);
+}
+
+static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+{
+ struct dwapb_gpio *gpio = irq_get_handler_data(irq);
+ u32 irq_status = readl(gpio->regs + INT_STATUS_REG_OFFS);
+
+ while (irq_status) {
+ int irqoffset = fls(irq_status) - 1;
+ int irq = irq_domain_to_irq(&gpio->irq_gc->domain, irqoffset);
+
+ generic_handle_irq(irq);
+ irq_status &= ~(1 << irqoffset);
+
+ if (gpio->toggle_edge & BIT(irqoffset))
+ dwapb_toggle_trigger(gpio, irqoffset);

if ((irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)

and get rid of ->toggle_edge cache variable.

+ }
+}
+
+static void dwapb_irq_enable(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct dwapb_gpio *gpio = gc->private;
+
+ u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
+ val |= 1 << d->hwirq;
+ writel(val, gpio->regs + INT_EN_REG_OFFS);
+}
+
+static void dwapb_irq_disable(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct dwapb_gpio *gpio = gc->private;
+
+ u32 val = readl(gpio->regs + INT_EN_REG_OFFS);
+ val &= ~(1 << d->hwirq);
+ writel(val, gpio->regs + INT_EN_REG_OFFS);
+}
+
+static int dwapb_irq_set_type(struct irq_data *d, u32 type)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct dwapb_gpio *gpio = gc->private;
+ int bit = d->hwirq;
+ unsigned long level, polarity;
+
+ if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
+ IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+ return -EINVAL;
+
+ level = readl(gpio->regs + INT_TYPE_REG_OFFS);
+ polarity = readl(gpio->regs + INT_POLARITY_REG_OFFS);
+
+ gpio->toggle_edge &= ~BIT(bit);
+ if (type & IRQ_TYPE_EDGE_BOTH) {
+ gpio->toggle_edge |= BIT(bit);
+ level |= (1 << bit);

If you remove ->toggle_edge, read current pin value here and
set next edge accordingly.

+ dwapb_toggle_trigger(gpio, bit);
+ } else if (type & IRQ_TYPE_EDGE_RISING) {
+ level |= (1 << bit);
+ polarity |= (1 << bit);
+ } else if (type & IRQ_TYPE_EDGE_FALLING) {
+ level |= (1 << bit);
+ polarity &= ~(1 << bit);
+ } else if (type & IRQ_TYPE_LEVEL_HIGH) {
+ level &= ~(1 << bit);
+ polarity |= (1 << bit);
+ } else if (type & IRQ_TYPE_LEVEL_LOW) {
+ level &= ~(1 << bit);
+ polarity &= ~(1 << bit);
+ }

switch (type) for the above if-else-if ?

+ writel(level, gpio->regs + INT_TYPE_REG_OFFS);
+ writel(polarity, gpio->regs + INT_POLARITY_REG_OFFS);
+
+ return 0;
+}
+
+static int dwapb_create_irqchip(struct dwapb_gpio *gpio,
+ struct dwapb_gpio_bank *bank,
+ unsigned int irq_base)
+{
+ struct irq_chip_type *ct;
+
+ gpio->irq_gc = irq_alloc_generic_chip("gpio-dwapb", 1, irq_base,
+ gpio->regs, handle_level_irq);
+ if (!gpio->irq_gc)
+ return -EIO;
+
+ gpio->irq_gc->domain.of_node = of_node_get(bank->bgc.gc.of_node);
+ gpio->irq_gc->private = gpio;
+ ct = gpio->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;

I can see no functional difference between enable/mask and always
enable and mask initially. If there are no objections, remove
enable/disable callbacks and mask and enable in _probe().

+ ct->regs.ack = EOI_REG_OFFS;
+ ct->regs.mask = INT_MASK_REG_OFFS;
+ irq_setup_generic_chip(gpio->irq_gc, IRQ_MSK(bank->bgc.gc.ngpio),
+ IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
+
+ return 0;
+}
+
+static int dwapb_configure_irqs(struct dwapb_gpio *gpio,
+ struct dwapb_gpio_bank *bank)
+{
+ unsigned int m, irq, ngpio = bank->bgc.gc.ngpio;
+ int irq_base;
+
+ for (m = 0; m < ngpio; ++m) {
+ irq = irq_of_parse_and_map(bank->bgc.gc.of_node, m);

Only port A can raise interrupts. Are you sure, that all IP has
one irq per gpio? As you always use the same handler, just map
all irqs passed and install the handler; no need to match against
individual gpios here.

Sebastian

+ if (!irq && m == 0) {
+ dev_warn(gpio->dev, "no irq for bank %s\n",
+ bank->bgc.gc.of_node->full_name);
+ return -ENXIO;
+ } else if (!irq) {
+ break;
+ }
+
+ irq_set_chained_handler(irq, dwapb_irq_handler);
+ irq_set_handler_data(irq, gpio);
+ }
+ bank->bgc.gc.to_irq = dwapb_gpio_to_irq;
+
+ irq_base = irq_alloc_descs(-1, 0, ngpio, NUMA_NO_NODE);
+ if (irq_base < 0)
+ return irq_base;

I was going to mention irqdomain_linear here, but then I realized
that patch 3/3 reverts most of irq handling here and replaces it
with irqdomain. Alan, I think Jamie is fine with you mangling patches
2 and 3 into one.

+ if (dwapb_create_irqchip(gpio, bank, irq_base))
+ goto out_free_descs;
+
+ return 0;
+
+out_free_descs:
+ irq_free_descs(irq_base, ngpio);
+
+ return -EIO;
+}
+
static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
struct device_node *bank_np,
unsigned int offs)
@@ -81,6 +255,13 @@ static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio,
bank->bgc.gc.ngpio = ngpio;
bank->bgc.gc.of_node = bank_np;

+ /*
+ * Only bank A can provide interrupts in all configurations of the IP.
+ */
+ if (bank_idx == 0 &&
+ of_get_property(bank_np, "interrupt-controller", NULL))
+ dwapb_configure_irqs(gpio, bank);
+
err = gpiochip_add(&bank->bgc.gc);
if (err)
dev_err(gpio->dev, "failed to register gpiochip for %s\n",


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/