Messages in this thread | | | Date | Tue, 05 Nov 2013 22:32:09 +0100 | From | Sebastian Hesselbarth <> | Subject | Re: [PATCH 3/3] use linear irq domain in gpio-dwapb |
| |
On 11/05/2013 05:55 PM, Alan Tull wrote: > From: Alan Tull <atull@altera.com> > > * Changes to get gpio-dwapb (originally v3.2-rc7) driver building and > working on current kernel. > * Use linear irq domain. > * Fix setting irq edge type for 'rising' and 'both'. > * Support as a loadable module. > * Use bgpio_chip's spinlock during register access.
As this patch is basically messing what I have commented before, I really lost interest in reviewing this again. Please just squash them into one.
BTW, I do have a driver for dw-apb-gpio that almost looks feature equivalent to this one. I am not ready for testing on Berlin SoCs, so I haven't posted it. After the review, I somehow think I should post it, especially because I see no point in port child nodes.
Anyway, some remarks below.
> Signed-off-by: Alan Tull <atull@altera.com> > --- > drivers/gpio/Kconfig | 2 +- > drivers/gpio/gpio-dwapb.c | 235 ++++++++++++++++++++++++++++++--------------- > 2 files changed, 160 insertions(+), 77 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 4cc26ed..5fc674a 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -122,7 +122,7 @@ config GPIO_GENERIC_PLATFORM > Say yes here to support basic platform_device memory-mapped GPIO controllers. > > config GPIO_DWAPB > - bool "Synopsys DesignWare APB GPIO driver" > + tristate "Synopsys DesignWare APB GPIO driver" > select GPIO_GENERIC > select GENERIC_IRQ_CHIP > depends on OF_GPIO && IRQ_DOMAIN > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 64c7183..cca97e3 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -18,6 +18,7 @@ > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/platform_device.h> > +#include <linux/spinlock.h> > > #define INT_EN_REG_OFFS 0x30 > #define INT_MASK_REG_OFFS 0x34 > @@ -30,7 +31,6 @@ struct dwapb_gpio; > > struct dwapb_gpio_bank { > struct bgpio_chip bgc; > - unsigned int bank_idx; > bool is_registered; > struct dwapb_gpio *gpio; > }; > @@ -41,8 +41,9 @@ 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; > + struct irq_domain *domain; > + int hwirq; > }; > > static unsigned int dwapb_gpio_nr_banks(struct device_node *of_node) > @@ -63,29 +64,30 @@ static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > dwapb_gpio_bank, bgc); > struct dwapb_gpio *gpio = bank->gpio; > > - return irq_domain_to_irq(&gpio->irq_gc->domain, offset); > + return irq_create_mapping(gpio->domain, offset); > } > > static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs) > { > - u32 v = readl(gpio->regs + INT_TYPE_REG_OFFS); > + u32 v = readl(gpio->regs + INT_POLARITY_REG_OFFS); > > 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); > + writel(v, gpio->regs + INT_POLARITY_REG_OFFS); > } > > static void dwapb_irq_handler(u32 irq, struct irq_desc *desc) > { > struct dwapb_gpio *gpio = irq_get_handler_data(irq); > + struct irq_chip *chip = irq_desc_get_chip(desc); > 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); > + int irq = irq_linear_revmap(gpio->domain, irqoffset); > > generic_handle_irq(irq); > irq_status &= ~(1 << irqoffset); > @@ -93,44 +95,56 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc) > if (gpio->toggle_edge & BIT(irqoffset)) > dwapb_toggle_trigger(gpio, irqoffset); > } > + > + if (chip->irq_eoi) > + chip->irq_eoi(irq_desc_get_irq_data(desc)); > } > > 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; > + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d); > + struct bgpio_chip *bgc = &gpio->banks[0].bgc; > + unsigned long flags; > + u32 val; > > - u32 val = readl(gpio->regs + INT_EN_REG_OFFS); > + spin_lock_irqsave(&bgc->lock, flags); > + val = readl(gpio->regs + INT_EN_REG_OFFS); > val |= 1 << d->hwirq; > writel(val, gpio->regs + INT_EN_REG_OFFS); > + spin_unlock_irqrestore(&bgc->lock, flags); > } > > 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; > + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d); > + struct bgpio_chip *bgc = &gpio->banks[0].bgc; > + unsigned long flags; > + u32 val; > > - u32 val = readl(gpio->regs + INT_EN_REG_OFFS); > + spin_lock_irqsave(&bgc->lock, flags); > + val = readl(gpio->regs + INT_EN_REG_OFFS); > val &= ~(1 << d->hwirq); > writel(val, gpio->regs + INT_EN_REG_OFFS); > + spin_unlock_irqrestore(&bgc->lock, flags); > } > > 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; > + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d); > + struct bgpio_chip *bgc = &gpio->banks[0].bgc; > int bit = d->hwirq; > - unsigned long level, polarity; > + unsigned long level, polarity, flags; > > 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); > 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) { > + if (type == IRQ_TYPE_EDGE_BOTH) { > gpio->toggle_edge |= BIT(bit); > level |= (1 << bit); > dwapb_toggle_trigger(gpio, bit); > @@ -150,72 +164,102 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) > > writel(level, gpio->regs + INT_TYPE_REG_OFFS); > writel(polarity, gpio->regs + INT_POLARITY_REG_OFFS); > + spin_unlock_irqrestore(&bgc->lock, flags); > > return 0; > } > > -static int dwapb_create_irqchip(struct dwapb_gpio *gpio, > - struct dwapb_gpio_bank *bank, > - unsigned int irq_base) > +static void dwapb_irq_ack(struct irq_data *d) > { > - 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; > - 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); > + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d); > + struct bgpio_chip *bgc = &gpio->banks[0].bgc; > + unsigned long flags; > > - return 0; > + spin_lock_irqsave(&bgc->lock, flags); > + writel(1 << d->hwirq, gpio->regs + EOI_REG_OFFS); > + spin_unlock_irqrestore(&bgc->lock, flags); > } > > -static int dwapb_configure_irqs(struct dwapb_gpio *gpio, > - struct dwapb_gpio_bank *bank) > +static void dwapb_irq_mask(struct irq_data *d) > { > - 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); > - 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; > + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d); > + struct bgpio_chip *bgc = &gpio->banks[0].bgc; > + unsigned long value, flags; > + > + spin_lock_irqsave(&bgc->lock, flags); > + value = readl(gpio->regs + INT_MASK_REG_OFFS); > + value |= 1 << d->hwirq; > + writel(value, gpio->regs + INT_MASK_REG_OFFS); > + spin_unlock_irqrestore(&bgc->lock, flags); > +} > > - irq_base = irq_alloc_descs(-1, 0, ngpio, NUMA_NO_NODE); > - if (irq_base < 0) > - return irq_base; > +static void dwapb_irq_unmask(struct irq_data *d) > +{ > + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d); > + struct bgpio_chip *bgc = &gpio->banks[0].bgc; > + unsigned long value, flags; > + > + spin_lock_irqsave(&bgc->lock, flags); > + value = readl(gpio->regs + INT_MASK_REG_OFFS); > + value &= ~(1 << d->hwirq); > + writel(value, gpio->regs + INT_MASK_REG_OFFS); > + spin_unlock_irqrestore(&bgc->lock, flags); > +} > > - if (dwapb_create_irqchip(gpio, bank, irq_base)) > - goto out_free_descs; > +static struct irq_chip dwapb_irq_chip = { > + .name = "gpio-dwapb", > + .irq_ack = dwapb_irq_ack, > + .irq_mask = dwapb_irq_mask, > + .irq_unmask = dwapb_irq_unmask, > + .irq_set_type = dwapb_irq_set_type, > + .irq_enable = dwapb_irq_enable, > + .irq_disable = dwapb_irq_disable, > +}; > + > +static int dwapb_gpio_irq_map(struct irq_domain *domain, > + unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + struct dwapb_gpio *gpio = domain->host_data; > + > + irq_set_chip_data(irq, gpio); > + irq_set_chip_and_handler(irq, &dwapb_irq_chip, handle_level_irq); > + irq_set_irq_type(irq, IRQ_TYPE_NONE); > + irq_set_handler_data(irq, gpio); > > return 0; > +} > > -out_free_descs: > - irq_free_descs(irq_base, ngpio); > +static struct irq_domain_ops dwapb_gpio_irq_ops = { > + .map = dwapb_gpio_irq_map, > + .xlate = irq_domain_xlate_twocell, > +}; > > - return -EIO; > +static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > + struct dwapb_gpio_bank *bank) > +{ > + struct gpio_chip *gc = &bank->bgc.gc; > + struct device_node *node = gc->of_node; > + unsigned int ngpio = gc->ngpio; > + int reg; > + > + if (of_get_property(node, "interrupts", ®) == NULL) > + return; > + > + gpio->hwirq = irq_of_parse_and_map(node, 0); > + if (gpio->hwirq == NO_IRQ) > + return; > + > + gpio->domain = irq_domain_add_linear(node, ngpio, &dwapb_gpio_irq_ops, > + gpio); > + if (!gpio->domain) { > + irq_dispose_mapping(gpio->hwirq); > + return; > + } > + > + irq_set_handler_data(gpio->hwirq, gpio); > + irq_set_chained_handler(gpio->hwirq, dwapb_irq_handler); > + bank->bgc.gc.to_irq = dwapb_gpio_to_irq; > } > > static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio, > @@ -240,7 +284,6 @@ static int dwapb_gpio_add_bank(struct dwapb_gpio *gpio, > return -EINVAL; > } > > - bank->bank_idx = bank_idx; > err = bgpio_init(&bank->bgc, gpio->dev, 4, > gpio->regs + 0x50 + (bank_idx * 0x4), > gpio->regs + 0x00 + (bank_idx * 0xc), > @@ -282,7 +325,7 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > of_node_put(gpio->of_node); > } > > -static int __devinit dwapb_gpio_probe(struct platform_device *pdev) > +static int dwapb_gpio_probe(struct platform_device *pdev) > { > struct resource *res; > struct dwapb_gpio *gpio; > @@ -296,21 +339,29 @@ static int __devinit dwapb_gpio_probe(struct platform_device *pdev) > gpio->dev = &pdev->dev; > > gpio->nr_banks = dwapb_gpio_nr_banks(pdev->dev.of_node); > - if (!gpio->nr_banks) > - return -EINVAL; > + if (!gpio->nr_banks) { > + err = -EINVAL; > + goto out_err; > + } > gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks * > sizeof(*gpio->banks), GFP_KERNEL); > - if (!gpio->banks) > - return -ENOMEM; > + if (!gpio->banks) { > + err = -ENOMEM; > + goto out_err; > + } > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > dev_err(&pdev->dev, "failed to get iomem\n"); > - return -ENXIO; > + err = -ENXIO; > + goto out_free_banks; > } > + > gpio->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > - if (!gpio->regs) > - return -ENOMEM; > + if (!gpio->regs) { > + err = -ENOMEM; > + goto out_free_banks; > + } > > gpio->of_node = of_node_get(pdev->dev.of_node); > for_each_child_of_node(pdev->dev.of_node, np) { > @@ -325,9 +376,34 @@ static int __devinit dwapb_gpio_probe(struct platform_device *pdev) > out_unregister: > dwapb_gpio_unregister(gpio); > > + if (gpio->domain) { > + irq_dispose_mapping(gpio->hwirq); > + irq_domain_remove(gpio->domain); > + } > + > +out_free_banks: > + devm_kfree(&pdev->dev, gpio->banks); > + > +out_err: > + devm_kfree(&pdev->dev, gpio);
devm_ will take care of kfree for you.
> return err; > } > > +static int dwapb_gpio_remove(struct platform_device *pdev) > +{ > + struct dwapb_gpio *gpio = platform_get_drvdata(pdev); > + > + dwapb_gpio_unregister(gpio); > + if (gpio->domain) { > + irq_dispose_mapping(gpio->hwirq); > + irq_domain_remove(gpio->domain); > + } > + devm_kfree(&pdev->dev, gpio->banks); > + devm_kfree(&pdev->dev, gpio);
ditto.
> + > + return 0; > +} > + > static const struct of_device_id dwapb_of_match_table[] = { > { .compatible = "snps,dw-apb-gpio" }, > { /* Sentinel */ } > @@ -340,6 +416,7 @@ static struct platform_driver dwapb_gpio_driver = { > .of_match_table = dwapb_of_match_table, > }, > .probe = dwapb_gpio_probe, > + .remove = dwapb_gpio_remove, > }; > > static int __init dwapb_gpio_init(void) > @@ -348,6 +425,12 @@ static int __init dwapb_gpio_init(void) > } > postcore_initcall(dwapb_gpio_init); > > +static void __exit dwapb_gpio_exit(void) > +{ > + platform_driver_unregister(&dwapb_gpio_driver); > +} > +module_exit(dwapb_gpio_exit);
module_platform_driver() for _init and _exit above and get rid of postcore_initcall?
> MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Jamie Iles"); > MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver"); >
| |