[ this reply is related to the Linux driver source code, I kept device tree and doc concerns in the other reply ]
you introduce a Linux driver, yet I don't see any Linux development ML in the Cc: list (only the GPIO maintainer and doc/DT related lists) -- am I missing something? On Wed, Jan 22, 2014 at 10:54 +0800, [email protected] wrote: > > --- /dev/null > +++ b/drivers/gpio/gpio-altera.c > [ ... ] > + > +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; > + } > + > + return -EINVAL; > +} This style of indentation makes readers miss the fact that the condition is multi-line and the body is just a single line. From a quick look one might assume broken code due to missing braces. Please adjust the alignment. > +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; > + > + chained_irq_enter(chip, desc); > + /* 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) { this looks like "Yoda programming" :) checking the constant value against a variable, making readers stop and wonder > + 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_find_mapping( > + altera_gc->domain, i)); > + } > + } > + } > + } > + > + chained_irq_exit(chip, desc); > +} > +int altera_gpio_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + int i, id, reg, ret; > + struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev, > + sizeof(*altera_gc), GFP_KERNEL); > + if (altera_gc == NULL) { > + pr_err("%s: out of memory\n", node->full_name); > + return -ENOMEM; > + } > + altera_gc->domain = 0; > + > + spin_lock_init(&altera_gc->gpio_lock); > + > + id = pdev->id; > + > + if (of_property_read_u32(node, "altr,gpio-bank-width", ®)) > + /*By default assume full GPIO controller*/ > + altera_gc->mmchip.gc.ngpio = 32; > + else > + altera_gc->mmchip.gc.ngpio = reg; > + > + if (altera_gc->mmchip.gc.ngpio > 32) { > + pr_warn("%s: ngpio is greater than 32, defaulting to 32\n", > + node->full_name); use dev_warn() so users can better identify the instance? hand-rolling this with pr_warn() and node->full_name is unexpected > + altera_gc->mmchip.gc.ngpio = 32; > + } > + > + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input; > + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output; > + altera_gc->mmchip.gc.get = altera_gpio_get; > + altera_gc->mmchip.gc.set = altera_gpio_set; > + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq; > + altera_gc->mmchip.gc.owner = THIS_MODULE; > + > + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip); > + if (ret) { > + pr_err("%s: Failed adding memory mapped gpiochip\n", > + node->full_name); > + return ret; > + } > + > + platform_set_drvdata(pdev, altera_gc); > + > + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0); > + > + if (!altera_gc->mapped_irq) > + goto skip_irq; interrupt controller support is optional in the code, too -- good > + > + altera_gc->domain = irq_domain_add_linear(node, > + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc); > + > + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) > + irq_create_mapping(altera_gc->domain, i); > + > + if (!altera_gc->domain) { > + ret = -ENODEV; > + goto dispose_irq; > + } do you reference the domain already between allocation and the check for successful allocation? > + > + if (of_property_read_u32(node, "altr,interrupt_trigger", ®)) { > + ret = -EINVAL; > + pr_err("%s: interrupt_trigger value not set in device tree\n", > + node->full_name); > + goto teardown; > + } > + altera_gc->interrupt_trigger = reg; > + > + irq_set_handler_data(altera_gc->mapped_irq, altera_gc); > + irq_set_chained_handler(altera_gc->mapped_irq, altera_gpio_irq_handler); > + > + return 0; > + > +teardown: > + irq_domain_remove(altera_gc->domain); > +dispose_irq: > + irq_dispose_mapping(altera_gc->mapped_irq); > + WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0); > + > + pr_err("%s: registration failed with status %d\n", > + node->full_name, ret); > + > + return ret; > +skip_irq: > + return 0; > +} virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [email protected] -- 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
