Messages in this thread Patch in this message |  | | Date | Tue, 05 Nov 2013 22:32:00 +0100 | From | Sebastian Hesselbarth <> | Subject | Re: [PATCH 2/3] gpio: dwapb: add support for GPIO interrupts |
| |
On 11/05/2013 05:55 PM, Alan Tull wrote: > From: Jamie Iles <jamie@jamieiles.com> > > 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@secretlab.ca> > Cc: Linus Walleij <linus.walleij@stericsson.com> > Acked-by: Rob Herring <rob.herring@calxeda.com> > Signed-off-by: Jamie Iles <jamie@jamieiles.com> > --- > 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", >
|  |