Hi Biju,
On Mon, May 9, 2022 at 9:01 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > > ---
> > > > 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
