diff mbox series

[RFC,2/5] pinctrl: renesas: rzg2l: Fix configuring the GPIO pins as interrupts

Message ID 20221107175305.63975-3-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series Add IRQC support to RZ/G2UL SoC | expand

Commit Message

Lad, Prabhakar Nov. 7, 2022, 5:53 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

On the RZ/G2UL SoC we have less number of pins compared to RZ/G2L and also
the pin configs are completely different. This patch makes sure we use the
appropriate pin configs for each SoC (which is passed as part of the OF
data) while configuring the GPIO pin as interrupts instead of using
rzg2l_gpio_configs[] for all the SoCs.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Biju Das Nov. 8, 2022, 7:14 a.m. UTC | #1 
Hi Prabhakar,


> Subject: [PATCH RFC 2/5] pinctrl: renesas: rzg2l: Fix configuring the GPIO
> pins as interrupts
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On the RZ/G2UL SoC we have less number of pins compared to RZ/G2L and also
> the pin configs are completely different. This patch makes sure we use the
> appropriate pin configs for each SoC (which is passed as part of the OF
> data) while configuring the GPIO pin as interrupts instead of using
> rzg2l_gpio_configs[] for all the SoCs.
> 

Looks like you are missing fixes tag.
Fixes: db2e5f21a48ed ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")

As we have already pinctrl support for RZ/G2UL [1]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/renesas/pinctrl-rzg2l.c?h=v6.1-rc4&id=bfc69bdbaad141ac408e6de86b7e0d771c8e3ccb

Cheers,
Biju

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index a43824fd9505..dcc495baa678 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -127,6 +127,7 @@ struct rzg2l_dedicated_configs {  struct
> rzg2l_pinctrl_data {
>  	const char * const *port_pins;
>  	const u32 *port_pin_configs;
> +	unsigned int n_port_pin_configs;
>  	struct rzg2l_dedicated_configs *dedicated_pins;
>  	unsigned int n_port_pins;
>  	unsigned int n_dedicated_pins;
> @@ -1122,7 +1123,7 @@ static struct {
>  	}
>  };
> 
> -static int rzg2l_gpio_get_gpioint(unsigned int virq)
> +static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct
> +rzg2l_pinctrl_data *data)
>  {
>  	unsigned int gpioint;
>  	unsigned int i;
> @@ -1131,13 +1132,13 @@ static int rzg2l_gpio_get_gpioint(unsigned int virq)
>  	port = virq / 8;
>  	bit = virq % 8;
> 
> -	if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
> -	    bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
> +	if (port >= data->n_port_pin_configs ||
> +	    bit >= RZG2L_GPIO_PORT_GET_PINCNT(data->port_pin_configs[port]))
>  		return -EINVAL;
> 
>  	gpioint = bit;
>  	for (i = 0; i < port; i++)
> -		gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
> +		gpioint += RZG2L_GPIO_PORT_GET_PINCNT(data->port_pin_configs[i]);
> 
>  	return gpioint;
>  }
> @@ -1237,7 +1238,7 @@ static int rzg2l_gpio_child_to_parent_hwirq(struct
> gpio_chip *gc,
>  	unsigned long flags;
>  	int gpioint, irq;
> 
> -	gpioint = rzg2l_gpio_get_gpioint(child);
> +	gpioint = rzg2l_gpio_get_gpioint(child, pctrl->data);
>  	if (gpioint < 0)
>  		return gpioint;
> 
> @@ -1311,8 +1312,8 @@ static void rzg2l_init_irq_valid_mask(struct gpio_chip
> *gc,
>  		port = offset / 8;
>  		bit = offset % 8;
> 
> -		if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
> -		    bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
> +		if (port >= pctrl->data->n_port_pin_configs ||
> +		    bit >=
> +RZG2L_GPIO_PORT_GET_PINCNT(pctrl->data->port_pin_configs[port]))
>  			clear_bit(offset, valid_mask);
>  	}
>  }
> @@ -1517,6 +1518,7 @@ static int rzg2l_pinctrl_probe(struct platform_device
> *pdev)  static struct rzg2l_pinctrl_data r9a07g043_data = {
>  	.port_pins = rzg2l_gpio_names,
>  	.port_pin_configs = r9a07g043_gpio_configs,
> +	.n_port_pin_configs = ARRAY_SIZE(r9a07g043_gpio_configs),
>  	.dedicated_pins = rzg2l_dedicated_pins.common,
>  	.n_port_pins = ARRAY_SIZE(r9a07g043_gpio_configs) *
> RZG2L_PINS_PER_PORT,
>  	.n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common),
> @@ -1525,6 +1527,7 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
> static struct rzg2l_pinctrl_data r9a07g044_data = {
>  	.port_pins = rzg2l_gpio_names,
>  	.port_pin_configs = rzg2l_gpio_configs,
> +	.n_port_pin_configs = ARRAY_SIZE(rzg2l_gpio_configs),
>  	.dedicated_pins = rzg2l_dedicated_pins.common,
>  	.n_port_pins = ARRAY_SIZE(rzg2l_gpio_names),
>  	.n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +
> --
> 2.25.1
Lad, Prabhakar Nov. 8, 2022, 9:09 a.m. UTC | #2 
Hi Biju,

On Tue, Nov 8, 2022 at 7:14 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
>
> > Subject: [PATCH RFC 2/5] pinctrl: renesas: rzg2l: Fix configuring the GPIO
> > pins as interrupts
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the RZ/G2UL SoC we have less number of pins compared to RZ/G2L and also
> > the pin configs are completely different. This patch makes sure we use the
> > appropriate pin configs for each SoC (which is passed as part of the OF
> > data) while configuring the GPIO pin as interrupts instead of using
> > rzg2l_gpio_configs[] for all the SoCs.
> >
>
> Looks like you are missing fixes tag.
> Fixes: db2e5f21a48ed ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")
>
I did think about but then I realised this fixes the GPIO IRQ
functions only and we didn't support IRQC and GPIO interrupts up until
now so I hadn't added the fixes tag.

Cheers,
Prabhakar
Biju Das Nov. 8, 2022, 9:15 a.m. UTC | #3 
> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Sent: 08 November 2022 09:10
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>; Marc Zyngier <maz@kernel.org>; Rob
> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Magnus Damm <magnus.damm@gmail.com>; Linus Walleij
> <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> Subject: Re: [PATCH RFC 2/5] pinctrl: renesas: rzg2l: Fix configuring the
> GPIO pins as interrupts
> 
> Hi Biju,
> 
> On Tue, Nov 8, 2022 at 7:14 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Prabhakar,
> >
> >
> > > Subject: [PATCH RFC 2/5] pinctrl: renesas: rzg2l: Fix configuring
> > > the GPIO pins as interrupts
> > >
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > On the RZ/G2UL SoC we have less number of pins compared to RZ/G2L
> > > and also the pin configs are completely different. This patch makes
> > > sure we use the appropriate pin configs for each SoC (which is
> > > passed as part of the OF
> > > data) while configuring the GPIO pin as interrupts instead of using
> > > rzg2l_gpio_configs[] for all the SoCs.
> > >
> >
> > Looks like you are missing fixes tag.
> > Fixes: db2e5f21a48ed ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> > to handle GPIO interrupt")
> >
> I did think about but then I realised this fixes the GPIO IRQ functions only
> and we didn't support IRQC and GPIO interrupts up until now so I hadn't added
> the fixes tag.

Yep that is true, even though we have pinctrl support for both RZ/G2L and RZ/G2UL.
Interrupt support added only for RZ/G2L at that time. Maybe change to reflect
RZ/G2UL GPIO interrupt support.

Cheers,
Biju
Geert Uytterhoeven Nov. 17, 2022, 11:09 a.m. UTC | #4 
Hi Prabhakar,

On Mon, Nov 7, 2022 at 6:53 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On the RZ/G2UL SoC we have less number of pins compared to RZ/G2L and also
> the pin configs are completely different. This patch makes sure we use the
> appropriate pin configs for each SoC (which is passed as part of the OF
> data) while configuring the GPIO pin as interrupts instead of using
> rzg2l_gpio_configs[] for all the SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
But I do think there is room for improvement...

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -127,6 +127,7 @@ struct rzg2l_dedicated_configs {
>  struct rzg2l_pinctrl_data {
>         const char * const *port_pins;
>         const u32 *port_pin_configs;
> +       unsigned int n_port_pin_configs;

n_ports?

>         struct rzg2l_dedicated_configs *dedicated_pins;
>         unsigned int n_port_pins;

n_port_pins is now always n_port_pin_configs * RZG2L_PINS_PER_PORT,
right?

>         unsigned int n_dedicated_pins;

> @@ -1517,6 +1518,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
>  static struct rzg2l_pinctrl_data r9a07g043_data = {
>         .port_pins = rzg2l_gpio_names,
>         .port_pin_configs = r9a07g043_gpio_configs,
> +       .n_port_pin_configs = ARRAY_SIZE(r9a07g043_gpio_configs),
>         .dedicated_pins = rzg2l_dedicated_pins.common,
>         .n_port_pins = ARRAY_SIZE(r9a07g043_gpio_configs) * RZG2L_PINS_PER_PORT,
>         .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common),
> @@ -1525,6 +1527,7 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
>  static struct rzg2l_pinctrl_data r9a07g044_data = {
>         .port_pins = rzg2l_gpio_names,

.port_pins is always rzg2l_gpio_names

>         .port_pin_configs = rzg2l_gpio_configs,
> +       .n_port_pin_configs = ARRAY_SIZE(rzg2l_gpio_configs),
>         .dedicated_pins = rzg2l_dedicated_pins.common,
>         .n_port_pins = ARRAY_SIZE(rzg2l_gpio_names),

I think this should have become
ARRAY_SIZE(rzg2l_gpio_configs) * RZG2L_PINS_PER_PORT)
when support for r9a07g043 was introduced.

To avoid overflows when adding support for more SoCs, you can add a
bunch of checks like

    BUILD_BUG_ON(ARRAY_SIZE(r9a07g043_gpio_configs) *
RZG2L_PINS_PER_PORT > ARRAY_SIZE(rzg2l_gpio_names))
    BUILD_BUG_ON(ARRAY_SIZE(rzg2l_gpio_configs) * RZG2L_PINS_PER_PORT
> ARRAY_SIZE(rzg2l_gpio_names))

>         .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Lad, Prabhakar Nov. 17, 2022, 12:14 p.m. UTC | #5 
Hi Geert,

Thank you for the review.

On Thu, Nov 17, 2022 at 11:09 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Nov 7, 2022 at 6:53 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the RZ/G2UL SoC we have less number of pins compared to RZ/G2L and also
> > the pin configs are completely different. This patch makes sure we use the
> > appropriate pin configs for each SoC (which is passed as part of the OF
> > data) while configuring the GPIO pin as interrupts instead of using
> > rzg2l_gpio_configs[] for all the SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> But I do think there is room for improvement...
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -127,6 +127,7 @@ struct rzg2l_dedicated_configs {
> >  struct rzg2l_pinctrl_data {
> >         const char * const *port_pins;
> >         const u32 *port_pin_configs;
> > +       unsigned int n_port_pin_configs;
>
> n_ports?
>
Ok I will rename it to n_ports.

> >         struct rzg2l_dedicated_configs *dedicated_pins;
> >         unsigned int n_port_pins;
>
> n_port_pins is now always n_port_pin_configs * RZG2L_PINS_PER_PORT,
> right?
>
Yes, that's right. So are you suggesting to drop it and use it runtime instead?

> >         unsigned int n_dedicated_pins;
>
> > @@ -1517,6 +1518,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
> >  static struct rzg2l_pinctrl_data r9a07g043_data = {
> >         .port_pins = rzg2l_gpio_names,
> >         .port_pin_configs = r9a07g043_gpio_configs,
> > +       .n_port_pin_configs = ARRAY_SIZE(r9a07g043_gpio_configs),
> >         .dedicated_pins = rzg2l_dedicated_pins.common,
> >         .n_port_pins = ARRAY_SIZE(r9a07g043_gpio_configs) * RZG2L_PINS_PER_PORT,
> >         .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common),
> > @@ -1525,6 +1527,7 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
> >  static struct rzg2l_pinctrl_data r9a07g044_data = {
> >         .port_pins = rzg2l_gpio_names,
>
> .port_pins is always rzg2l_gpio_names
>
Yes to avoid the huge array to be duplicated for other SoCs but bound
checking is done by  n_port_pins.

> >         .port_pin_configs = rzg2l_gpio_configs,
> > +       .n_port_pin_configs = ARRAY_SIZE(rzg2l_gpio_configs),
> >         .dedicated_pins = rzg2l_dedicated_pins.common,
> >         .n_port_pins = ARRAY_SIZE(rzg2l_gpio_names),
>
> I think this should have become
> ARRAY_SIZE(rzg2l_gpio_configs) * RZG2L_PINS_PER_PORT)
> when support for r9a07g043 was introduced.
>
Agreed, I will update it as part of v2.

> To avoid overflows when adding support for more SoCs, you can add a
> bunch of checks like
>
>     BUILD_BUG_ON(ARRAY_SIZE(r9a07g043_gpio_configs) *
> RZG2L_PINS_PER_PORT > ARRAY_SIZE(rzg2l_gpio_names))
>     BUILD_BUG_ON(ARRAY_SIZE(rzg2l_gpio_configs) * RZG2L_PINS_PER_PORT
> > ARRAY_SIZE(rzg2l_gpio_names))
>
OK, I'll add those checks in the probe as a separate patch.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index a43824fd9505..dcc495baa678 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -127,6 +127,7 @@  struct rzg2l_dedicated_configs {
 struct rzg2l_pinctrl_data {
 	const char * const *port_pins;
 	const u32 *port_pin_configs;
+	unsigned int n_port_pin_configs;
 	struct rzg2l_dedicated_configs *dedicated_pins;
 	unsigned int n_port_pins;
 	unsigned int n_dedicated_pins;
@@ -1122,7 +1123,7 @@  static struct {
 	}
 };
 
-static int rzg2l_gpio_get_gpioint(unsigned int virq)
+static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct rzg2l_pinctrl_data *data)
 {
 	unsigned int gpioint;
 	unsigned int i;
@@ -1131,13 +1132,13 @@  static int rzg2l_gpio_get_gpioint(unsigned int virq)
 	port = virq / 8;
 	bit = virq % 8;
 
-	if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
-	    bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
+	if (port >= data->n_port_pin_configs ||
+	    bit >= RZG2L_GPIO_PORT_GET_PINCNT(data->port_pin_configs[port]))
 		return -EINVAL;
 
 	gpioint = bit;
 	for (i = 0; i < port; i++)
-		gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
+		gpioint += RZG2L_GPIO_PORT_GET_PINCNT(data->port_pin_configs[i]);
 
 	return gpioint;
 }
@@ -1237,7 +1238,7 @@  static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
 	unsigned long flags;
 	int gpioint, irq;
 
-	gpioint = rzg2l_gpio_get_gpioint(child);
+	gpioint = rzg2l_gpio_get_gpioint(child, pctrl->data);
 	if (gpioint < 0)
 		return gpioint;
 
@@ -1311,8 +1312,8 @@  static void rzg2l_init_irq_valid_mask(struct gpio_chip *gc,
 		port = offset / 8;
 		bit = offset % 8;
 
-		if (port >= ARRAY_SIZE(rzg2l_gpio_configs) ||
-		    bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port]))
+		if (port >= pctrl->data->n_port_pin_configs ||
+		    bit >= RZG2L_GPIO_PORT_GET_PINCNT(pctrl->data->port_pin_configs[port]))
 			clear_bit(offset, valid_mask);
 	}
 }
@@ -1517,6 +1518,7 @@  static int rzg2l_pinctrl_probe(struct platform_device *pdev)
 static struct rzg2l_pinctrl_data r9a07g043_data = {
 	.port_pins = rzg2l_gpio_names,
 	.port_pin_configs = r9a07g043_gpio_configs,
+	.n_port_pin_configs = ARRAY_SIZE(r9a07g043_gpio_configs),
 	.dedicated_pins = rzg2l_dedicated_pins.common,
 	.n_port_pins = ARRAY_SIZE(r9a07g043_gpio_configs) * RZG2L_PINS_PER_PORT,
 	.n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common),
@@ -1525,6 +1527,7 @@  static struct rzg2l_pinctrl_data r9a07g043_data = {
 static struct rzg2l_pinctrl_data r9a07g044_data = {
 	.port_pins = rzg2l_gpio_names,
 	.port_pin_configs = rzg2l_gpio_configs,
+	.n_port_pin_configs = ARRAY_SIZE(rzg2l_gpio_configs),
 	.dedicated_pins = rzg2l_dedicated_pins.common,
 	.n_port_pins = ARRAY_SIZE(rzg2l_gpio_names),
 	.n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +