Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 24 Jul 2020 22:58:56 +0300 | Subject | Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support |
| |
On Fri, Jul 24, 2020 at 8:15 PM Srinivas Neeli <sneeli@xilinx.com> wrote:
...
> > > +#include <linux/irqchip/chained_irq.h> > > > > Not sure I see a user of it. > > > > ... > we are using chained_irq_enter() and chained_irq_exit() > APIs , so need "chained_irq.h"
I see. But gpio/driver.h does it for you.
...
> > > + for (index = 0; index < num_channels; index++) { > > > + if ((status & BIT(index))) { > > > > If gpio_width is the same among banks, you can use for_each_set_bit() > > here as well. > > > > ... > gpio_wdith vary depends on design. We can configure gpio pins for each bank.
I see.
...
> > > + chip->irq = platform_get_irq_optional(pdev, 0); > > > + if (chip->irq <= 0) { > > > + dev_info(&pdev->dev, "GPIO IRQ not set\n"); > > > > Why do you need an optional variant if you print an error anyway? > > Here intention is just printing a debug message to user.
Debug message should be debug, and not info. But in any case, I would rather drop it, or use platform_get_irq() b/c now you have a code controversy.
...
> > > + chip->gc.irq.parents = (unsigned int *)&chip->irq; > > > + chip->gc.irq.num_parents = 1; > > > > Current pattern is to use devm_kcalloc() for it (Linus has plans to > > simplify this in the future and this will help him to find what > > patterns are being used) > > I didn't get this , Could you please explain more.
girq->num_parents = 1; girq->parents = devm_kcalloc(dev, girq->num_parents, ...); if (!girq->parents) return -ENOMEM;
girq->parents[0] = chip->irq;
Also, use temporary variable for IRQ chip structure: girq = &chip->gc.irq;
-- With Best Regards, Andy Shevchenko
| |