On Fri, Jul 24, 2020 at 8:15 PM Srinivas Neeli [off-list ref] wrote:

...

quoted

quoted

+#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.

...

quoted

quoted

+       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.

...

quoted

quoted

+       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.

...

quoted

quoted

+               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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel