On Thu, Jan 23, 2014 at 2:24 AM, Gerhard Sittig <gsi@xxxxxxx> wrote:
> [ 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?
Totally my fault. Do I add them in now or do I create a v7 patch in this case?
>
> On Wed, Jan 22, 2014 at 10:54 +0800, thloh@xxxxxxxxxx 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.
Okay, will fix 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
Fix this I will.
>
>> + 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
Yeah makes sense. I'll update this.
>
>> + 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?
Good catch. The irq_create_mapping was added from the last patch and I
totally overlooked this.
>
>> +
>> + 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: office@xxxxxxx
Tien Hock Loh
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
