Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1595513168-11965-1-git-send-email-srinivas.neeli@xilinx.com>
 <1595513168-11965-3-git-send-email-srinivas.neeli@xilinx.com>
 <CAHp75Vd7BU5DYqyQFGfBtKrb6jWFEQjMCu2MOa_7M8XYkt6BFA@mail.gmail.com> <DM6PR02MB53867D532A3298BE06E32B86AF770@DM6PR02MB5386.namprd02.prod.outlook.com>
In-Reply-To: <DM6PR02MB53867D532A3298BE06E32B86AF770@DM6PR02MB5386.namprd02.prod.outlook.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 24 Jul 2020 22:58:56 +0300
Message-ID: <CAHp75VeFE3b4mpvkQifuC5WPkTk9RUQy4yM8jTQXtrfNWRmDXA@mail.gmail.com>
Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
To:     Srinivas Neeli <sneeli@xilinx.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Michal Simek <michals@xilinx.com>,
        Shubhrajyoti Datta <shubhraj@xilinx.com>,
        Srinivas Goud <sgoud@xilinx.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        git <git@xilinx.com>, Robert Hancock <hancock@sedsystems.ca>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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