Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Nov 9, 2016 at 11:28 AM, Shah, Nehal-bakulchandra
<Nehal-bakulchandra.Shah@xxxxxxx> 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@xxxxxxx>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@xxxxxxx>

This is not looking correct at all.

> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html