Re: [PATCH v2 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Biju,

On Mon, May 9, 2022 at 9:01 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> > Subject: Re: [PATCH v2 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> > to handle GPIO interrupt
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Mon, May 9, 2022 at 7:49 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the patch.
> > >
> > > > Subject: [PATCH v2 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > > domain to handle GPIO interrupt
> > > >
> > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> > > >
> > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > > > used as IRQ lines at given time. Selection of pins as IRQ lines is
> > > > handled by IA55 (which is the IRQC block) which sits in between the
> > GPIO and GIC.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 205
> > > > ++++++++++++++++++++++++
> > > >  1 file changed, 205 insertions(+)
> > > >
> > > > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > index a48cac55152c..275dfec74329 100644
> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > @@ -9,8 +9,10 @@
> > > >  #include <linux/clk.h>
> > > >  #include <linux/gpio/driver.h>
> > > >  #include <linux/io.h>
> > > > +#include <linux/interrupt.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of_device.h>
> > > > +#include <linux/of_irq.h>
> > > >  #include <linux/pinctrl/pinconf-generic.h>  #include
> > > > <linux/pinctrl/pinconf.h>  #include <linux/pinctrl/pinctrl.h> @@
> > > > -89,6
> > > > +91,7 @@
> > > >  #define PIN(n)                       (0x0800 + 0x10 + (n))
> > > >  #define IOLH(n)                      (0x1000 + (n) * 8)
> > > >  #define IEN(n)                       (0x1800 + (n) * 8)
> > > > +#define ISEL(n)                      (0x2c80 + (n) * 8)
> > > >  #define PWPR                 (0x3014)
> > > >  #define SD_CH(n)             (0x3000 + (n) * 4)
> > > >  #define QSPI                 (0x3008)
> > > > @@ -112,6 +115,10 @@
> > > >  #define RZG2L_PIN_ID_TO_PORT_OFFSET(id)      (RZG2L_PIN_ID_TO_PORT(id)
> > +
> > > > 0x10)
> > > >  #define RZG2L_PIN_ID_TO_PIN(id)              ((id) %
> > RZG2L_PINS_PER_PORT)
> > > >
> > > > +#define RZG2L_TINT_MAX_INTERRUPT     32
> > > > +#define RZG2L_TINT_IRQ_START_INDEX   9
> > > > +#define RZG2L_PACK_HWIRQ(t, i)               (((t) << 16) | (i))
> > > > +
> > > >  struct rzg2l_dedicated_configs {
> > > >       const char *name;
> > > >       u32 config;
> > > > @@ -137,6 +144,9 @@ struct rzg2l_pinctrl {
> > > >
> > > >       struct gpio_chip                gpio_chip;
> > > >       struct pinctrl_gpio_range       gpio_range;
> > > > +     DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT);
> > > > +     spinlock_t                      bitmap_lock;
> > > > +     unsigned int                    hwirq[RZG2L_TINT_MAX_INTERRUPT];
> > > >
> > > >       spinlock_t                      lock;
> > > >  };
> > > > @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip
> > > > *chip, unsigned int offset)
> > > >
> > > >  static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int
> > > > offset) {
> > > > +     unsigned int virq;
> > > > +
> > > >       pinctrl_gpio_free(chip->base + offset);
> > > >
> > > >       /*
> > > > @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip
> > > > *chip, unsigned int offset)
> > > >        * drive the GPIO pin as an output.
> > > >        */
> > > >       rzg2l_gpio_direction_input(chip, offset);
> > > > +
> > > > +     virq = irq_find_mapping(chip->irq.domain, offset);
> > > > +     if (virq)
> > > > +             irq_dispose_mapping(virq);
> > > >  }
> > > >
> > > >  static const char * const rzg2l_gpio_names[] = { @@ -1104,14
> > > > +1120,193 @@ static struct {
> > > >       }
> > > >  };
> > > >
> > > > +static int rzg2l_gpio_get_gpioint(unsigned int virq) {
> > > > +     unsigned int gpioint = 0;
> > > > +     unsigned int i = 0;
> > > > +     u32 port, bit;
> > > > +
> > > > +     port = virq / 8;
> > > > +     bit = virq % 8;
> > > > +
> > > > +     if (port >= ARRAY_SIZE(rzg2l_gpio_configs))
> > > > +             return -EINVAL;
> > > > +
> > > > +     for (i = 0; i < port; i++)
> > > > +             gpioint +=
> > > > + RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
> > > > +
> > > > +     if (bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]))
> > > > +             return -EINVAL;
> > >
> > > May be combine this statement to above with
> > >
> > > || (bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
> > >                 return -EINVAL;
> > >
> > The reason I have kept it outside the loop is that I'll have to check it
> > only once at the end of the loop instead of repeating the check every time
> > in the loop.
>
> I meant above for loop, so that validation happens before the for loop??
>
> if (port >= ARRAY_SIZE(rzg2l_gpio_configs)) || (bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
>         return -EINVAL;
>
Got that, will update in v3.

Cheers,
Prabhakar