On Thu, Nov 28, 2013 at 4:15 AM, <thloh@xxxxxxxxxx> wrote: > From: Tien Hock Loh <thloh@xxxxxxxxxx> > > Add driver support for Altera GPIO soft IP, including interrupts and I/O. > Tested on Altera CV SoC board using dipsw and LED using LED framework. > > Signed-off-by: Tien Hock Loh <thloh@xxxxxxxxxx> Overall it looks good (some stuff to fix below). I want an ACK from some device tree maintainer on the bindings, ideally. > +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO > + hardware is synthesized. This field is required if the Altera GPIO controller > + used has IRQ enabled as the interrupt type is not software controlled, > + but hardware synthesized. Required if GPIO is used as an interrupt > + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h> > + Only the following flags are supported: > + IRQ_TYPE_EDGE_RISING > + IRQ_TYPE_EDGE_FALLING > + IRQ_TYPE_EDGE_BOTH > + IRQ_TYPE_LEVEL_HIGH I remember discussing this, and it's OK from my side. > +/** > +* struct altera_gpio_chip > +* @mmchip : memory mapped chip structure. > +* @irq : irq domain that this driver is registered to. > +* @gpio_lock : synchronization lock so that new irq/set/get requests > + will be blocked until the current one completes. > +* @interrupt_trigger : specifies the hardware configured IRQ trigger type > + (rising, falling, both, high) > +* @mapped_irq : kernel mapped irq number. > +*/ > +struct altera_gpio_chip { > + struct of_mm_gpio_chip mmchip; > + struct irq_domain *irq; Argh that's a bad member name for an irqdomain, please just call this "domain" like the other drivers do. > + spinlock_t gpio_lock; > + int interrupt_trigger; > + int edge_type; > + int mapped_irq; Why are you keeping this around? Well I guess I might figure out... > +static int altera_gpio_irq_set_type(struct irq_data *d, > + unsigned int type) > +{ > + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d); > + > + if (type == IRQ_TYPE_NONE) > + return 0; > + > + if (type == IRQ_TYPE_LEVEL_HIGH && > + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > + return 0; > + } else { > + if (type == IRQ_TYPE_EDGE_RISING && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING) > + return 0; > + else if (type == IRQ_TYPE_EDGE_FALLING && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING) > + return 0; > + else if (type == IRQ_TYPE_EDGE_BOTH && > + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH) > + return 0; > + } There is something very convoluted about this non-special else clause. It can be just a else if like all the others, right? > +static struct irq_chip altera_irq_chip = { > + .name = "altera-gpio", > + .irq_mask = altera_gpio_irq_mask, > + .irq_unmask = altera_gpio_irq_unmask, > + .irq_set_type = altera_gpio_irq_set_type, > +}; We have added new APIs to flag GPIO lines as IRQs. Please implement this in the .irq_startup()/.irq_shutdown() callbacks in accordance with the style of e.g. this patch: http://marc.info/?l=linux-gpio&m=138571851304612&w=2 Note that you need to call unmask/mask from these callbacks. > +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset) > +{ > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > + > + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1; Use this style to clamp a bit to {0,1}: return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset)); > +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value) > +{ > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > + struct altera_gpio_chip *chip = container_of(mm_gc, > + struct altera_gpio_chip, mmchip); > + unsigned long flags; > + unsigned int data_reg; > + > + spin_lock_irqsave(&chip->gpio_lock, flags); > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > + data_reg = (data_reg & ~BIT(offset)) | (value << offset); This has the same arithmetic effect but is easier to read: if (value) data_reg |= BIT(offset); else data_reg &= ~BIT(offset); > +static int altera_gpio_direction_output(struct gpio_chip *gc, > + unsigned offset, int value) > +{ > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > + struct altera_gpio_chip *chip = container_of(mm_gc, > + struct altera_gpio_chip, mmchip); > + unsigned long flags; > + unsigned int data_reg, gpio_ddr; > + > + spin_lock_irqsave(&chip->gpio_lock, flags); > + /* Sets the GPIO value */ > + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA); > + data_reg = (data_reg & ~BIT(offset)) | (value << offset); Same comment. As for set(). > +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > +{ > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > + struct altera_gpio_chip *altera_gc = container_of(mm_gc, > + struct altera_gpio_chip, mmchip); > + > + if (!altera_gc->irq) > + return -ENXIO; > + if (offset < altera_gc->mmchip.gc.ngpio) > + return irq_create_mapping(altera_gc->irq, offset); Recently we have established that you should call irq_create_mapping() for all valid IRQs in probe() and just use irq_find_mapping() here, as it is valid semantics to use an IRQ in an irq_chip without converting it from a GPIO line first. This is because in DT use cases you may use an interrupt from an interrupt-controller without first calling gpio_to_irq() and then this explodes. So please fix this... > +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) > +{ > + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip; > + unsigned long status; > + > + int i; > + chip->irq_mask(&desc->irq_data); Why do you call mask()/unmask()? All interrupt handlers are called with interrupts disabled, I bet you can just delete these two calls. > + > + /* Handling for level trigger and edge trigger is different */ > + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) { > + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA); > + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK); > + > + for (i = 0; i < mm_gc->gc.ngpio; i++) { > + if (BIT(i) & status) { > + generic_handle_irq(irq_linear_revmap( > + altera_gc->irq, i)); Why do you do it like that? What us wrong with this: generic_handle_irq(irq_find_mapping(altera_gc->domain, i)) ? > + } > + } > + } else { > + while ((status = > + (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) & > + readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) { > + writel_relaxed(status, > + mm_gc->regs + ALTERA_GPIO_EDGE_CAP); > + for (i = 0; i < mm_gc->gc.ngpio; i++) { > + if (BIT(i) & status) { > + generic_handle_irq(irq_linear_revmap( > + altera_gc->irq, i)); Dito. > + } > + } > + } > + } > + > + chip->irq_eoi(irq_desc_get_irq_data(desc)); I don't think you should call that directly either. Why do you do this? > + chip->irq_unmask(&desc->irq_data); See above about mask/unmask. > +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq, > + irq_hw_number_t hw_irq_num) > +{ > + irq_set_chip_data(virq, h->host_data); > + irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq); > + irq_set_irq_type(virq, IRQ_TYPE_NONE); > + > + return 0; > +} Rename the argument "virq" to just irq. These IRQs are not really virtual any more than any other Linux IRQ. (...) > + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio, > + &altera_gpio_irq_ops, altera_gc); Here you should call irq_create_map() for all valid IRQs. > +static int altera_gpio_remove(struct platform_device *pdev) > +{ > + unsigned int irq, i; > + int status; > + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev); > + > + status = gpiochip_remove(&altera_gc->mmchip.gc); > + > + if (status < 0) > + return status; > + > + if (altera_gc->irq) { > + irq_dispose_mapping(altera_gc->mapped_irq); > + > + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) { > + irq = irq_find_mapping(altera_gc->irq, i); > + if (irq > 0) > + irq_dispose_mapping(irq); > + } Look, you're already disposing all mappings properly :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html