Re: [PATCH] pinctrl: amd: Add support for additional GPIO
From: Linus Walleij <hidden>
Date: 2016-11-14 13:18:41
On Wed, Nov 9, 2016 at 11:28 AM, Shah, Nehal-bakulchandra [off-list ref] wrote:
This Provide IRQ sharing for AMD's GPIO devices.And set IRQCHIP_SKIP_SET_WAKE flag. Also, fix the build issue for devm_request_irq() Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> Signed-off-by: Nehal Shah <redacted>
This is not looking correct at all.
quoted hunk ↗ jump to hunk
@@ -529,16 +531,13 @@ static void amd_gpio_irq_handler(struct irq_desc *desc) } } - if (handled == 0) - handle_bad_irq(desc); - spin_lock_irqsave(&gpio_dev->lock, flags); reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG); reg |= EOI_MASK; writel(reg, gpio_dev->base + WAKE_INT_MASTER_REG); spin_unlock_irqrestore(&gpio_dev->lock, flags); - chained_irq_exit(chip, desc); + return 0;
You are removing the chained interrupt handling for no good reason. The commit message does not say why this is being done. It's especially erroneous to remove chained_irq_exit() but not chained_irq_enter(), but generally, NONE of them should be removed this *is* a chained interrupt handler.
gpiochip_set_chained_irqchip(&gpio_dev->gc,
&amd_gpio_irqchip,
irq_base,
- amd_gpio_irq_handler);
+ NULL);
+
+ ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
+ IRQF_SHARED, dev_name(&pdev->dev), gpio_dev);
+ if (ret)
+ goto out2;This is just wrong. The gpiochip_set_chained_irqchip() should not be called in combination with devm_request_irq() like this. devm_request_irq() should not be used at all. Keep the chained handler. I'm worried that this patch has a "trial-and-error" quality, it seems you don't really know what is going on. Please investigate and send the patch with a commitlog that describes exactly why you are doing what you are doing. Yours, Linus Walleij