[PATCH] gpiolib: warning on gpiochip->to_irq defined

[PATCH] gpiolib: warning on gpiochip->to_irq defined

[Nikita Shubin]

gpiochip->to_irq method is redefined in gpiochip_add_irqchip.

A lot of gpiod driver's still define ->to_irq method, let's give
a gentle warning that they can no longer rely on it, so they can remove
it on ocassion.

Fixes: e0d8972898139 ("gpio: Implement tighter IRQ chip integration")
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5ce0c14c637b..44538d1a771a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1489,6 +1489,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 		type = IRQ_TYPE_NONE;
 	}
 
+	if (gc->to_irq)
+		chip_err(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
+
 	gc->to_irq = gpiochip_to_irq;
 	gc->irq.default_type = type;
 	gc->irq.lock_key = lock_key;
-- 
2.29.2

Re: [PATCH] gpiolib: warning on gpiochip->to_irq defined

[Bartosz Golaszewski]

On Mon, Dec 28, 2020 at 4:02 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> gpiochip->to_irq method is redefined in gpiochip_add_irqchip.
>
> A lot of gpiod driver's still define ->to_irq method, let's give
> a gentle warning that they can no longer rely on it, so they can remove
> it on ocassion.
>
> Fixes: e0d8972898139 ("gpio: Implement tighter IRQ chip integration")
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpiolib.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5ce0c14c637b..44538d1a771a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1489,6 +1489,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>                 type = IRQ_TYPE_NONE;
>         }
>
> +       if (gc->to_irq)
> +               chip_err(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
> +
>         gc->to_irq = gpiochip_to_irq;
>         gc->irq.default_type = type;
>         gc->irq.lock_key = lock_key;
> --
> 2.29.2
>

I know Linus suggested using chip_err() here but since this doesn't
cause the function to fail, I'd say this should be chip_warn().

Unless Linus has any objections, please change it.

Bartosz

[PATCH v2] gpiolib: warning on gpiochip->to_irq defined

[Nikita Shubin]

gpiochip->to_irq method is redefined in gpiochip_add_irqchip.

A lot of gpiod driver's still define ->to_irq method, let's give
a gentle warning that they can no longer rely on it, so they can remove
it on ocassion.

Fixes: e0d8972898139 ("gpio: Implement tighter IRQ chip integration")
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
v1->v2:
- Change chip_err to chip_warn
---
 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5ce0c14c637b..5a9410c2537d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1489,6 +1489,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 		type = IRQ_TYPE_NONE;
 	}
 
+	if (gc->to_irq)
+		chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
+
 	gc->to_irq = gpiochip_to_irq;
 	gc->irq.default_type = type;
 	gc->irq.lock_key = lock_key;
-- 
2.29.2

Re: [PATCH v2] gpiolib: warning on gpiochip->to_irq defined

[Bartosz Golaszewski]

On Mon, Jan 18, 2021 at 10:05 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
>
> gpiochip->to_irq method is redefined in gpiochip_add_irqchip.
>
> A lot of gpiod driver's still define ->to_irq method, let's give
> a gentle warning that they can no longer rely on it, so they can remove
> it on ocassion.
>
> Fixes: e0d8972898139 ("gpio: Implement tighter IRQ chip integration")
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> v1->v2:
> - Change chip_err to chip_warn
> ---
>  drivers/gpio/gpiolib.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5ce0c14c637b..5a9410c2537d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1489,6 +1489,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>                 type = IRQ_TYPE_NONE;
>         }
>
> +       if (gc->to_irq)
> +               chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
> +
>         gc->to_irq = gpiochip_to_irq;
>         gc->irq.default_type = type;
>         gc->irq.lock_key = lock_key;
> --
> 2.29.2
>

Applied to fixes, thanks!

Bartosz

[PATCH v2 0/9] gpio: ep93xx: fixes series patch

[Nikita Shubin]

Series of patches to fix ep93xx gpio driver to make IRQ's working.

The following are fix patches (these are enough to get gpio-ep93xx
working with modern kernel):

[PATCH v2 1/9] gpio: ep93xx: fix BUG_ON port F usage
[PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi
 gpiochips
[PATCH v2 3/9] gpio: ep93xx: Fix wrong irq numbers in port F

The following are cleanup and style patches:

[PATCH v2 4/9] gpio: ep93xx: drop to_irq binding
[PATCH v2 5/9] gpio: ep93xx: Fix typo s/hierarchial/hierarchical
[PATCH v2 6/9] gpio: ep93xx: refactor ep93xx_gpio_add_bank
[PATCH v2 7/9] gpio: ep93xx: separate IRQ's setup
[PATCH v2 8/9] gpio: ep93xx: refactor base IRQ number
[PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports

The futher work can be done by removing ep93xx_gpio_port function
and, maybe, ep93xx_gpio_update_int_params. But i think it's better 
to be done in conjunction with converting ep93xx to DT support.

[PATCH v2 1/9] gpio: ep93xx: fix BUG_ON port F usage

[Nikita Shubin]

The port F is actually index 2 not 5 as it is calculated 
by ep93xx_gpio_port.

Looks like it happened because of the confusing ep93xx_gpio_banks 
table that shows like port F is having index 5, but is actually 
index 2 if we rely on .base instead of index position.

------------[ cut here ]------------
kernel BUG at drivers/gpio/gpio-ep93xx.c:64!
Internal error: Oops - BUG: 0 [#1] ARM
Modules linked in:
CPU: 0 PID: 403 Comm: gpio-event-mon Not tainted 5.9.10-00011-ge93e9618628b-dirty #19
Hardware name: Technologic Systems TS-72xx SBC
PC is at ep93xx_gpio_update_int_params+0x1c/0x80
LR is at ep93xx_gpio_update_int_params+0x14/0x80
pc : [<c03abc44>] lr : [<c03abc3c>] psr: 20000093
sp : c158de00 ip : 00000000 fp : 00000001
r10: c44154d4 r9 : 00000000 r8 : c4415020
r7 : c04ef884 r6 : c051c842 r5 : c4415020 r4 : 00000005
r3 : 00000000 r2 : 00000000 r1 : c04eb768 r0 : 00000008
Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
Control: 0000717f Table: 01684000 DAC: 00000051
Process gpio-event-mon (pid: 403, stack limit = 0x(ptrval))
Stack: (0xc158de00 to 0xc158e000)
de00: 00000005 00000002 c051c842 c0238dc0 c0238c98 c0238c98 c04ef874 00000000
de20: 00000003 c04fcfcc 60000013 c04ef910 c04ef8d4 c00456f0 c04ef874 c15f1e00
de40: 00000000 00000000 00000001 c0045d40 c15f1e00 c4400160 c0044ca8 c04ef8a8
de60: 60000013 00000000 c15f1e00 c04ef874 c04ef884 00000001 c0235d70 c158b800
de80: be825f0f c0045ec8 00000003 c158b800 c440aa00 be825bc8 00000003 00000001
dea0: 00000000 c0236f00 c44ed3a0 c158b800 c45c2015 00000000 00000001 00000003
dec0: 6f697067 6576652d 6d2d746e 00006e6f 00000000 00000000 00000000 00000000
dee0: be825df4 c00abb0c c440c500 c00aabd4 c440c500 c528b840 c45c2010 c04e1228
df00: 00000ff0 c4478d28 c030b404 be825bc8 c1550e20 00000003 c1550e20 c00c3388
df20: c4478d28 c00c3d48 be825f0f c00abd00 c45c2000 c45c2000 c1550e20 c00bfea8
df40: 00000003 c00b0714 00000000 c4450000 00000004 00000100 00000001 c04e1228
df60: c158dfb0 ffffff9c 000231f8 00000003 00000142 c00b085c 00000000 c04e1228
df80: 00000000 be825f0f 00000003 00000003 00000036 c00083c4 c158c000 00000000
dfa0: be825f0f c00081e0 be825f0f 00000003 00000003 c030b404 be825bc8 00000000
dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f
dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec 60000010 00000003 00000000 00000000
[<c03abc44>] (ep93xx_gpio_update_int_params) from [<c0238dc0>] (ep93xx_gpio_irq_type+0x128/0x1c0)
[<c0238dc0>] (ep93xx_gpio_irq_type) from [<c00456f0>] (__irq_set_trigger+0x6c/0x128)
[<c00456f0>] (__irq_set_trigger) from [<c0045d40>] (__setup_irq+0x594/0x678)
[<c0045d40>] (__setup_irq) from [<c0045ec8>] (request_threaded_irq+0xa4/0x128)
[<c0045ec8>] (request_threaded_irq) from [<c0236f00>] (gpio_ioctl+0x300/0x4d8)
[<c0236f00>] (gpio_ioctl) from [<c00c3388>] (vfs_ioctl+0x24/0x3c)
[<c00c3388>] (vfs_ioctl) from [<c00c3d48>] (sys_ioctl+0xbc/0x768)
[<c00c3d48>] (sys_ioctl) from [<c00081e0>] (ret_fast_syscall+0x0/0x50)
Exception stack(0xc158dfa8 to 0xc158dff0)
dfa0: be825f0f 00000003 00000003 c030b404 be825bc8 00000000
dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f
dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec
Code: e59f0060 ebfff3e1 e3540002 9a000000 (e7f001f2)
---[ end trace 3f6544e133e9f5ae ]---

Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers")
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 226da8df6f10..0d0435c07a5a 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -39,6 +39,12 @@ struct ep93xx_gpio {
 	struct gpio_chip	gc[8];
 };
 
+/*
+ * F Port index in GPIOCHIP'S array is 5
+ * but we use index 2 for stored values and offsets
+ */
+#define EP93XX_GPIO_F_PORT_INDEX 5
+
 /*************************************************************************
  * Interrupt handling for EP93xx on-chip GPIOs
  *************************************************************************/
@@ -85,6 +91,9 @@ static int ep93xx_gpio_port(struct gpio_chip *gc)
 		return 0;
 	}
 
+	if (port == EP93XX_GPIO_F_PORT_INDEX)
+		port = 2;
+
 	return port;
 }
 
-- 
2.29.2

[PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Nikita Shubin]

Fixes the following warnings which results in interrupts disabled on 
port B/F:

gpio gpiochip1: (B): detected irqchip that is shared with multiple gpiochips: please fix the driver.
gpio gpiochip5: (F): detected irqchip that is shared with multiple gpiochips: please fix the driver.

- added separate irqchip for each interrupt capable gpiochip
- provided unique names for each irqchip
- reworked ep93xx_gpio_port to make it usable before chip_add_data 
  in ep93xx_init_irq_chips

Fixes: a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy disable")
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 45 ++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 0d0435c07a5a..2eea02c906e0 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -34,9 +34,12 @@
  */
 #define EP93XX_GPIO_F_IRQ_BASE 80
 
+#define EP93XX_GPIO_IRQ_CHIPS_NUM 3
+
 struct ep93xx_gpio {
 	void __iomem		*base;
 	struct gpio_chip	gc[8];
+	struct irq_chip		ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
 };
 
 /*
@@ -55,6 +58,11 @@ static unsigned char gpio_int_type2[3];
 static unsigned char gpio_int_debounce[3];
 
 /* Port ordering is: A B F */
+static const char * const irq_chip_names[] = {
+	"gpio-irq-a",
+	"gpio-irq-b",
+	"gpio-irq-f"
+};
 static const u8 int_type1_register_offset[3]	= { 0x90, 0xac, 0x4c };
 static const u8 int_type2_register_offset[3]	= { 0x94, 0xb0, 0x50 };
 static const u8 eoi_register_offset[3]		= { 0x98, 0xb4, 0x54 };
@@ -77,9 +85,8 @@ static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg, unsigned port
 	       epg->base + int_en_register_offset[port]);
 }
 
-static int ep93xx_gpio_port(struct gpio_chip *gc)
+static int ep93xx_gpio_port(struct ep93xx_gpio *epg, struct gpio_chip *gc)
 {
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
 	int port = 0;
 
 	while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
@@ -101,7 +108,7 @@ static void ep93xx_gpio_int_debounce(struct gpio_chip *gc,
 				     unsigned int offset, bool enable)
 {
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	int port = ep93xx_gpio_port(epg, gc);
 	int port_mask = BIT(offset);
 
 	if (enable)
@@ -163,7 +170,7 @@ static void ep93xx_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	int port = ep93xx_gpio_port(epg, gc);
 	int port_mask = BIT(d->irq & 7);
 
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
@@ -178,7 +185,7 @@ static void ep93xx_gpio_irq_mask_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	int port = ep93xx_gpio_port(epg, gc);
 	int port_mask = BIT(d->irq & 7);
 
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
@@ -194,7 +201,7 @@ static void ep93xx_gpio_irq_mask(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	int port = ep93xx_gpio_port(epg, gc);
 
 	gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
 	ep93xx_gpio_update_int_params(epg, port);
@@ -204,7 +211,7 @@ static void ep93xx_gpio_irq_unmask(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	int port = ep93xx_gpio_port(epg, gc);
 
 	gpio_int_unmasked[port] |= BIT(d->irq & 7);
 	ep93xx_gpio_update_int_params(epg, port);
@@ -219,7 +226,7 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
+	int port = ep93xx_gpio_port(epg, gc);
 	int offset = d->irq & 7;
 	int port_mask = BIT(offset);
 	irq_flow_handler_t handler;
@@ -335,6 +342,22 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset)
 	return EP93XX_GPIO_F_IRQ_BASE + offset;
 }
 
+static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
+{
+	int i;
+
+	for (i = 0; i < EP93XX_GPIO_IRQ_CHIPS_NUM; i++) {
+		struct irq_chip *ic = &epg->ic[i];
+
+		ic->name = irq_chip_names[i];
+		ic->irq_ack = ep93xx_gpio_irq_ack;
+		ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
+		ic->irq_mask = ep93xx_gpio_irq_mask;
+		ic->irq_unmask = ep93xx_gpio_irq_unmask;
+		ic->irq_set_type = ep93xx_gpio_irq_type;
+	}
+}
+
 static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 				struct platform_device *pdev,
 				struct ep93xx_gpio *epg,
@@ -345,6 +368,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 	struct device *dev = &pdev->dev;
 	struct gpio_irq_chip *girq;
 	int err;
+	int port;
 
 	err = bgpio_init(gc, dev, 1, data, NULL, NULL, dir, NULL, 0);
 	if (err)
@@ -356,7 +380,8 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 	girq = &gc->irq;
 	if (bank->has_irq || bank->has_hierarchical_irq) {
 		gc->set_config = ep93xx_gpio_set_config;
-		girq->chip = &ep93xx_gpio_irq_chip;
+		port = ep93xx_gpio_port(epg, gc);
+		girq->chip = &epg->ic[port];
 	}
 
 	if (bank->has_irq) {
@@ -423,6 +448,8 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(epg->base))
 		return PTR_ERR(epg->base);
 
+	ep93xx_init_irq_chips(epg);
+
 	for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
 		struct gpio_chip *gc = &epg->gc[i];
 		struct ep93xx_gpio_bank *bank = &ep93xx_gpio_banks[i];
-- 
2.29.2

Re: [PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Alexander Sverdlin]

Hello Nikita,

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Fixes the following warnings which results in interrupts disabled on 
> port B/F:
> 
> gpio gpiochip1: (B): detected irqchip that is shared with multiple
> gpiochips: please fix the driver.
> gpio gpiochip5: (F): detected irqchip that is shared with multiple
> gpiochips: please fix the driver.
> 
> - added separate irqchip for each interrupt capable gpiochip
> - provided unique names for each irqchip
> - reworked ep93xx_gpio_port to make it usable before chip_add_data 
>   in ep93xx_init_irq_chips
> 
> Fixes: a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy
> disable")

I'd rather say, it fixes commit d2b091961510
("gpio: ep93xx: Pass irqchip when adding gpiochip").
But for sure, not the gpiolib code as above tag claims.

> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpio-ep93xx.c | 45 ++++++++++++++++++++++++++++++------
> --
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 0d0435c07a5a..2eea02c906e0 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -34,9 +34,12 @@
>   */
>  #define EP93XX_GPIO_F_IRQ_BASE 80
>  
> +#define EP93XX_GPIO_IRQ_CHIPS_NUM 3
> +
>  struct ep93xx_gpio {
>         void __iomem            *base;
>         struct gpio_chip        gc[8];
> +       struct irq_chip         ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
>  };
>  
>  /*
> @@ -55,6 +58,11 @@ static unsigned char gpio_int_type2[3];
>  static unsigned char gpio_int_debounce[3];
>  
>  /* Port ordering is: A B F */
> +static const char * const irq_chip_names[] = {
> +       "gpio-irq-a",
> +       "gpio-irq-b",
> +       "gpio-irq-f"
> +};
>  static const u8 int_type1_register_offset[3]   = { 0x90, 0xac, 0x4c
> };
>  static const u8 int_type2_register_offset[3]   = { 0x94, 0xb0, 0x50
> };
>  static const u8 eoi_register_offset[3]         = { 0x98, 0xb4, 0x54
> };
> @@ -77,9 +85,8 @@ static void ep93xx_gpio_update_int_params(struct
> ep93xx_gpio *epg, unsigned port
>                epg->base + int_en_register_offset[port]);
>  }
>  
> -static int ep93xx_gpio_port(struct gpio_chip *gc)
> +static int ep93xx_gpio_port(struct ep93xx_gpio *epg, struct
> gpio_chip *gc)
>  {
> -       struct ep93xx_gpio *epg = gpiochip_get_data(gc);
>         int port = 0;
>  
>         while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
> @@ -101,7 +108,7 @@ static void ep93xx_gpio_int_debounce(struct
> gpio_chip *gc,
>                                      unsigned int offset, bool
> enable)
>  {
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int port_mask = BIT(offset);
>  
>         if (enable)
> @@ -163,7 +170,7 @@ static void ep93xx_gpio_irq_ack(struct irq_data
> *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int port_mask = BIT(d->irq & 7);
>  
>         if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
> @@ -178,7 +185,7 @@ static void ep93xx_gpio_irq_mask_ack(struct
> irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int port_mask = BIT(d->irq & 7);
>  
>         if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
> @@ -194,7 +201,7 @@ static void ep93xx_gpio_irq_mask(struct irq_data
> *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>  
>         gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
>         ep93xx_gpio_update_int_params(epg, port);
> @@ -204,7 +211,7 @@ static void ep93xx_gpio_irq_unmask(struct
> irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>  
>         gpio_int_unmasked[port] |= BIT(d->irq & 7);
>         ep93xx_gpio_update_int_params(epg, port);
> @@ -219,7 +226,7 @@ static int ep93xx_gpio_irq_type(struct irq_data
> *d, unsigned int type)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int offset = d->irq & 7;
>         int port_mask = BIT(offset);
>         irq_flow_handler_t handler;
> @@ -335,6 +342,22 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip
> *gc, unsigned offset)
>         return EP93XX_GPIO_F_IRQ_BASE + offset;
>  }
>  
> +static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
> +{
> +       int i;
> +
> +       for (i = 0; i < EP93XX_GPIO_IRQ_CHIPS_NUM; i++) {
> +               struct irq_chip *ic = &epg->ic[i];
> +
> +               ic->name = irq_chip_names[i];
> +               ic->irq_ack = ep93xx_gpio_irq_ack;
> +               ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
> +               ic->irq_mask = ep93xx_gpio_irq_mask;
> +               ic->irq_unmask = ep93xx_gpio_irq_unmask;
> +               ic->irq_set_type = ep93xx_gpio_irq_type;
> +       }
> +}
> +
>  static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
>                                 struct platform_device *pdev,
>                                 struct ep93xx_gpio *epg,
> @@ -345,6 +368,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>         struct device *dev = &pdev->dev;
>         struct gpio_irq_chip *girq;
>         int err;
> +       int port;
>  
>         err = bgpio_init(gc, dev, 1, data, NULL, NULL, dir, NULL, 0);
>         if (err)
> @@ -356,7 +380,8 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>         girq = &gc->irq;
>         if (bank->has_irq || bank->has_hierarchical_irq) {
>                 gc->set_config = ep93xx_gpio_set_config;
> -               girq->chip = &ep93xx_gpio_irq_chip;
> +               port = ep93xx_gpio_port(epg, gc);
> +               girq->chip = &epg->ic[port];
>         }
>  
>         if (bank->has_irq) {
> @@ -423,6 +448,8 @@ static int ep93xx_gpio_probe(struct
> platform_device *pdev)
>         if (IS_ERR(epg->base))
>                 return PTR_ERR(epg->base);
>  
> +       ep93xx_init_irq_chips(epg);
> +
>         for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
>                 struct gpio_chip *gc = &epg->gc[i];
>                 struct ep93xx_gpio_bank *bank =
> &ep93xx_gpio_banks[i];

-- 
Alexander Sverdlin.

Re: [PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Alexander Sverdlin]

Hi!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Fixes the following warnings which results in interrupts disabled on 
> port B/F:
> 
> gpio gpiochip1: (B): detected irqchip that is shared with multiple
> gpiochips: please fix the driver.
> gpio gpiochip5: (F): detected irqchip that is shared with multiple
> gpiochips: please fix the driver.
> 
> - added separate irqchip for each interrupt capable gpiochip
> - provided unique names for each irqchip
> - reworked ep93xx_gpio_port to make it usable before chip_add_data 
>   in ep93xx_init_irq_chips
> 
> Fixes: a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy
> disable")
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Yes, it indeed fixes the warnigs mentioned above,

Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/gpio-ep93xx.c | 45 ++++++++++++++++++++++++++++++------
> --
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 0d0435c07a5a..2eea02c906e0 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -34,9 +34,12 @@
>   */
>  #define EP93XX_GPIO_F_IRQ_BASE 80
>  
> +#define EP93XX_GPIO_IRQ_CHIPS_NUM 3
> +
>  struct ep93xx_gpio {
>         void __iomem            *base;
>         struct gpio_chip        gc[8];
> +       struct irq_chip         ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
>  };
>  
>  /*
> @@ -55,6 +58,11 @@ static unsigned char gpio_int_type2[3];
>  static unsigned char gpio_int_debounce[3];
>  
>  /* Port ordering is: A B F */
> +static const char * const irq_chip_names[] = {
> +       "gpio-irq-a",
> +       "gpio-irq-b",
> +       "gpio-irq-f"
> +};
>  static const u8 int_type1_register_offset[3]   = { 0x90, 0xac, 0x4c
> };
>  static const u8 int_type2_register_offset[3]   = { 0x94, 0xb0, 0x50
> };
>  static const u8 eoi_register_offset[3]         = { 0x98, 0xb4, 0x54
> };
> @@ -77,9 +85,8 @@ static void ep93xx_gpio_update_int_params(struct
> ep93xx_gpio *epg, unsigned port
>                epg->base + int_en_register_offset[port]);
>  }
>  
> -static int ep93xx_gpio_port(struct gpio_chip *gc)
> +static int ep93xx_gpio_port(struct ep93xx_gpio *epg, struct
> gpio_chip *gc)
>  {
> -       struct ep93xx_gpio *epg = gpiochip_get_data(gc);
>         int port = 0;
>  
>         while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
> @@ -101,7 +108,7 @@ static void ep93xx_gpio_int_debounce(struct
> gpio_chip *gc,
>                                      unsigned int offset, bool
> enable)
>  {
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int port_mask = BIT(offset);
>  
>         if (enable)
> @@ -163,7 +170,7 @@ static void ep93xx_gpio_irq_ack(struct irq_data
> *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int port_mask = BIT(d->irq & 7);
>  
>         if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
> @@ -178,7 +185,7 @@ static void ep93xx_gpio_irq_mask_ack(struct
> irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int port_mask = BIT(d->irq & 7);
>  
>         if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
> @@ -194,7 +201,7 @@ static void ep93xx_gpio_irq_mask(struct irq_data
> *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>  
>         gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
>         ep93xx_gpio_update_int_params(epg, port);
> @@ -204,7 +211,7 @@ static void ep93xx_gpio_irq_unmask(struct
> irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>  
>         gpio_int_unmasked[port] |= BIT(d->irq & 7);
>         ep93xx_gpio_update_int_params(epg, port);
> @@ -219,7 +226,7 @@ static int ep93xx_gpio_irq_type(struct irq_data
> *d, unsigned int type)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
> +       int port = ep93xx_gpio_port(epg, gc);
>         int offset = d->irq & 7;
>         int port_mask = BIT(offset);
>         irq_flow_handler_t handler;
> @@ -335,6 +342,22 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip
> *gc, unsigned offset)
>         return EP93XX_GPIO_F_IRQ_BASE + offset;
>  }
>  
> +static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
> +{
> +       int i;
> +
> +       for (i = 0; i < EP93XX_GPIO_IRQ_CHIPS_NUM; i++) {
> +               struct irq_chip *ic = &epg->ic[i];
> +
> +               ic->name = irq_chip_names[i];
> +               ic->irq_ack = ep93xx_gpio_irq_ack;
> +               ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
> +               ic->irq_mask = ep93xx_gpio_irq_mask;
> +               ic->irq_unmask = ep93xx_gpio_irq_unmask;
> +               ic->irq_set_type = ep93xx_gpio_irq_type;
> +       }
> +}
> +
>  static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
>                                 struct platform_device *pdev,
>                                 struct ep93xx_gpio *epg,
> @@ -345,6 +368,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>         struct device *dev = &pdev->dev;
>         struct gpio_irq_chip *girq;
>         int err;
> +       int port;
>  
>         err = bgpio_init(gc, dev, 1, data, NULL, NULL, dir, NULL, 0);
>         if (err)
> @@ -356,7 +380,8 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>         girq = &gc->irq;
>         if (bank->has_irq || bank->has_hierarchical_irq) {
>                 gc->set_config = ep93xx_gpio_set_config;
> -               girq->chip = &ep93xx_gpio_irq_chip;
> +               port = ep93xx_gpio_port(epg, gc);
> +               girq->chip = &epg->ic[port];
>         }
>  
>         if (bank->has_irq) {
> @@ -423,6 +448,8 @@ static int ep93xx_gpio_probe(struct
> platform_device *pdev)
>         if (IS_ERR(epg->base))
>                 return PTR_ERR(epg->base);
>  
> +       ep93xx_init_irq_chips(epg);
> +
>         for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
>                 struct gpio_chip *gc = &epg->gc[i];
>                 struct ep93xx_gpio_bank *bank =
> &ep93xx_gpio_banks[i];

-- 
Alexander Sverdlin.

[PATCH v2 3/9] gpio: ep93xx: Fix wrong irq numbers in port F

[Nikita Shubin]

Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE.

So we need to specify girq->first otherwise:

"If device tree is used, then first_irq will be 0 and
irqs get mapped dynamically on the fly"

And that's not the thing we want.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 2eea02c906e0..9c3d049e5af7 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -430,6 +430,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 		girq->default_type = IRQ_TYPE_NONE;
 		girq->handler = handle_level_irq;
 		gc->to_irq = ep93xx_gpio_f_to_irq;
+		girq->first = EP93XX_GPIO_F_IRQ_BASE;
 	}
 
 	return devm_gpiochip_add_data(dev, gc, epg);
-- 
2.29.2

Re: [PATCH v2 3/9] gpio: ep93xx: Fix wrong irq numbers in port F

[Linus Walleij]

On Wed, Jan 27, 2021 at 11:46 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:

> Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE.
>
> So we need to specify girq->first otherwise:
>
> "If device tree is used, then first_irq will be 0 and
> irqs get mapped dynamically on the fly"
>
> And that's not the thing we want.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

True, to satisfy old board file-type machines we unfortunately
have to do this.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 3/9] gpio: ep93xx: Fix wrong irq numbers in port F

[Alexander Sverdlin]

Hi!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE.
> 
> So we need to specify girq->first otherwise:
> 
> "If device tree is used, then first_irq will be 0 and
> irqs get mapped dynamically on the fly"
> 
> And that's not the thing we want.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

I have no code out-of-the-box to test the GPIO interrupts on EP93xx,
so I just did a bootup with this patch. But the change looks fine to
me:

Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/gpio-ep93xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 2eea02c906e0..9c3d049e5af7 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -430,6 +430,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>                 girq->default_type = IRQ_TYPE_NONE;
>                 girq->handler = handle_level_irq;
>                 gc->to_irq = ep93xx_gpio_f_to_irq;
> +               girq->first = EP93XX_GPIO_F_IRQ_BASE;
>         }
>  
>         return devm_gpiochip_add_data(dev, gc, epg);

-- 
Alexander Sverdlin.

[PATCH v2 4/9] gpio: ep93xx: drop to_irq binding

[Nikita Shubin]

As ->to_irq is redefined in gpiochip_add_irqchip, having it defined in
driver is useless, so let's drop it.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 9c3d049e5af7..dee19372ebbd 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -337,11 +337,6 @@ static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
 	return 0;
 }
 
-static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset)
-{
-	return EP93XX_GPIO_F_IRQ_BASE + offset;
-}
-
 static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
 {
 	int i;
@@ -429,7 +424,6 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 		}
 		girq->default_type = IRQ_TYPE_NONE;
 		girq->handler = handle_level_irq;
-		gc->to_irq = ep93xx_gpio_f_to_irq;
 		girq->first = EP93XX_GPIO_F_IRQ_BASE;
 	}
 
-- 
2.29.2

Re: [PATCH v2 4/9] gpio: ep93xx: drop to_irq binding

[Alexander Sverdlin]

Hi!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> As ->to_irq is redefined in gpiochip_add_irqchip, having it defined
> in
> driver is useless, so let's drop it.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/gpio-ep93xx.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 9c3d049e5af7..dee19372ebbd 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -337,11 +337,6 @@ static int ep93xx_gpio_set_config(struct
> gpio_chip *gc, unsigned offset,
>         return 0;
>  }
>  
> -static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned
> offset)
> -{
> -       return EP93XX_GPIO_F_IRQ_BASE + offset;
> -}
> -
>  static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
>  {
>         int i;
> @@ -429,7 +424,6 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>                 }
>                 girq->default_type = IRQ_TYPE_NONE;
>                 girq->handler = handle_level_irq;
> -               gc->to_irq = ep93xx_gpio_f_to_irq;
>                 girq->first = EP93XX_GPIO_F_IRQ_BASE;
>         }
>  

-- 
Alexander Sverdlin.

[PATCH v2 5/9] gpio: ep93xx: Fix typo s/hierarchial/hierarchical

[Nikita Shubin]

Fix typo in comment.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index dee19372ebbd..8f66e3ca0cfb 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -402,7 +402,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 
 		/*
 		 * FIXME: convert this to use hierarchical IRQ support!
-		 * this requires fixing the root irqchip to be hierarchial.
+		 * this requires fixing the root irqchip to be hierarchical.
 		 */
 		girq->parent_handler = ep93xx_gpio_f_irq_handler;
 		girq->num_parents = 8;
-- 
2.29.2

Re: [PATCH v2 5/9] gpio: ep93xx: Fix typo s/hierarchial/hierarchical

[Alexander Sverdlin]

Hi!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Fix typo in comment.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/gpio-ep93xx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index dee19372ebbd..8f66e3ca0cfb 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -402,7 +402,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>  
>                 /*
>                  * FIXME: convert this to use hierarchical IRQ
> support!
> -                * this requires fixing the root irqchip to be
> hierarchial.
> +                * this requires fixing the root irqchip to be
> hierarchical.
>                  */
>                 girq->parent_handler = ep93xx_gpio_f_irq_handler;
>                 girq->num_parents = 8;

-- 
Alexander Sverdlin.

[PATCH v2 6/9] gpio: ep93xx: refactor ep93xx_gpio_add_bank

[Nikita Shubin]

- replace plain numbers with girq->num_parents in devm_kcalloc
- replace plain numbers with ARRAY_SIZE(girq->parents) for port F
- refactor i - 1 to i + 1 to make loop more readable
- combine getting IRQ's loop and setting handler's into single loop

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 8f66e3ca0cfb..e4270b4e5f26 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -384,7 +384,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 
 		girq->parent_handler = ep93xx_gpio_ab_irq_handler;
 		girq->num_parents = 1;
-		girq->parents = devm_kcalloc(dev, 1,
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
 					     sizeof(*girq->parents),
 					     GFP_KERNEL);
 		if (!girq->parents)
@@ -406,15 +406,14 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 		 */
 		girq->parent_handler = ep93xx_gpio_f_irq_handler;
 		girq->num_parents = 8;
-		girq->parents = devm_kcalloc(dev, 8,
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
 					     sizeof(*girq->parents),
 					     GFP_KERNEL);
 		if (!girq->parents)
 			return -ENOMEM;
 		/* Pick resources 1..8 for these IRQs */
-		for (i = 1; i <= 8; i++)
-			girq->parents[i - 1] = platform_get_irq(pdev, i);
-		for (i = 0; i < 8; i++) {
+		for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
+			girq->parents[i] = platform_get_irq(pdev, i + 1);
 			gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
 			irq_set_chip_data(gpio_irq, &epg->gc[5]);
 			irq_set_chip_and_handler(gpio_irq,
-- 
2.29.2

Re: [PATCH v2 6/9] gpio: ep93xx: refactor ep93xx_gpio_add_bank

[Alexander Sverdlin]

Hello Nikita,

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> - replace plain numbers with girq->num_parents in devm_kcalloc
> - replace plain numbers with ARRAY_SIZE(girq->parents) for port F
> - refactor i - 1 to i + 1 to make loop more readable
> - combine getting IRQ's loop and setting handler's into single loop
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpio-ep93xx.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 8f66e3ca0cfb..e4270b4e5f26 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -384,7 +384,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>  
>                 girq->parent_handler = ep93xx_gpio_ab_irq_handler;
>                 girq->num_parents = 1;
> -               girq->parents = devm_kcalloc(dev, 1,
> +               girq->parents = devm_kcalloc(dev, girq->num_parents,
>                                              sizeof(*girq->parents),
>                                              GFP_KERNEL);
>                 if (!girq->parents)
> @@ -406,15 +406,14 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
>                  */
>                 girq->parent_handler = ep93xx_gpio_f_irq_handler;
>                 girq->num_parents = 8;
> -               girq->parents = devm_kcalloc(dev, 8,
> +               girq->parents = devm_kcalloc(dev, girq->num_parents,
>                                              sizeof(*girq->parents),
>                                              GFP_KERNEL);
>                 if (!girq->parents)
>                         return -ENOMEM;
>                 /* Pick resources 1..8 for these IRQs */
> -               for (i = 1; i <= 8; i++)
> -                       girq->parents[i - 1] = platform_get_irq(pdev,
> i);
> -               for (i = 0; i < 8; i++) {
> +               for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {

Why do you use ARRAY_SIZE() here instead of ->num_parents like above?

> +                       girq->parents[i] = platform_get_irq(pdev, i +
> 1);
>                         gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
>                         irq_set_chip_data(gpio_irq, &epg->gc[5]);
>                         irq_set_chip_and_handler(gpio_irq,

-- 
Alexander Sverdlin.

[PATCH v2 7/9] gpio: ep93xx: separate IRQ's setup

[Nikita Shubin]

Separate IRQ's setup for port A,B,F.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 104 +++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 40 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index e4270b4e5f26..b212c007240e 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -353,6 +353,64 @@ static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
 	}
 }
 
+static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev,
+					struct gpio_irq_chip *girq,
+					unsigned int irq_base)
+{
+	struct device *dev = &pdev->dev;
+	int ab_parent_irq = platform_get_irq(pdev, 0);
+
+	girq->parent_handler = ep93xx_gpio_ab_irq_handler;
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(dev, girq->num_parents,
+				     sizeof(*girq->parents),
+				     GFP_KERNEL);
+	if (!girq->parents)
+		return -ENOMEM;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+	girq->parents[0] = ab_parent_irq;
+	girq->first = irq_base;
+
+	return 0;
+}
+
+static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev,
+					struct gpio_irq_chip *girq,
+					unsigned int irq_base)
+{
+	int gpio_irq;
+	int i;
+	struct device *dev = &pdev->dev;
+
+	/*
+	 * FIXME: convert this to use hierarchical IRQ support!
+	 * this requires fixing the root irqchip to be hierarchical.
+	 */
+	girq->parent_handler = ep93xx_gpio_f_irq_handler;
+	girq->num_parents = 8;
+	girq->parents = devm_kcalloc(dev, girq->num_parents,
+				     sizeof(*girq->parents),
+				     GFP_KERNEL);
+	if (!girq->parents)
+		return -ENOMEM;
+	/* Pick resources 1..8 for these IRQs */
+	for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
+		girq->parents[i] = platform_get_irq(pdev, i + 1);
+		gpio_irq = irq_base + i;
+		irq_set_chip_data(gpio_irq, &epg->gc[5]);
+		irq_set_chip_and_handler(gpio_irq,
+					 &ep93xx_gpio_irq_chip,
+					 handle_level_irq);
+		irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
+	}
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+	girq->first = irq_base;
+
+	return 0;
+}
+
 static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 				struct platform_device *pdev,
 				struct ep93xx_gpio *epg,
@@ -380,50 +438,16 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 	}
 
 	if (bank->has_irq) {
-		int ab_parent_irq = platform_get_irq(pdev, 0);
-
-		girq->parent_handler = ep93xx_gpio_ab_irq_handler;
-		girq->num_parents = 1;
-		girq->parents = devm_kcalloc(dev, girq->num_parents,
-					     sizeof(*girq->parents),
-					     GFP_KERNEL);
-		if (!girq->parents)
-			return -ENOMEM;
-		girq->default_type = IRQ_TYPE_NONE;
-		girq->handler = handle_level_irq;
-		girq->parents[0] = ab_parent_irq;
-		girq->first = bank->irq_base;
+		err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank->irq_base);
+		if (err)
+			return err;
 	}
 
 	/* Only bank F has especially funky IRQ handling */
 	if (bank->has_hierarchical_irq) {
-		int gpio_irq;
-		int i;
-
-		/*
-		 * FIXME: convert this to use hierarchical IRQ support!
-		 * this requires fixing the root irqchip to be hierarchical.
-		 */
-		girq->parent_handler = ep93xx_gpio_f_irq_handler;
-		girq->num_parents = 8;
-		girq->parents = devm_kcalloc(dev, girq->num_parents,
-					     sizeof(*girq->parents),
-					     GFP_KERNEL);
-		if (!girq->parents)
-			return -ENOMEM;
-		/* Pick resources 1..8 for these IRQs */
-		for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
-			girq->parents[i] = platform_get_irq(pdev, i + 1);
-			gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
-			irq_set_chip_data(gpio_irq, &epg->gc[5]);
-			irq_set_chip_and_handler(gpio_irq,
-						 &ep93xx_gpio_irq_chip,
-						 handle_level_irq);
-			irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
-		}
-		girq->default_type = IRQ_TYPE_NONE;
-		girq->handler = handle_level_irq;
-		girq->first = EP93XX_GPIO_F_IRQ_BASE;
+		err = ep93xx_gpio_add_f_irq_chip(pdev, girq, EP93XX_GPIO_F_IRQ_BASE);
+		if (err)
+			return err;
 	}
 
 	return devm_gpiochip_add_data(dev, gc, epg);
-- 
2.29.2

Re: [PATCH v2 7/9] gpio: ep93xx: separate IRQ's setup

[Alexander Sverdlin]

Hello Nikita,

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Separate IRQ's setup for port A,B,F.

Somehow I don't feel that moving "FIXME" code around makes much
sense. Maybe the anticipated conversion to hierarhical IRQ chip
will result in a cleanup automatically?

> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpio-ep93xx.c | 104 +++++++++++++++++++++++------------
> --
>  1 file changed, 64 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index e4270b4e5f26..b212c007240e 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -353,6 +353,64 @@ static void ep93xx_init_irq_chips(struct
> ep93xx_gpio *epg)
>         }
>  }
>  
> +static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev,
> +                                       struct gpio_irq_chip *girq,
> +                                       unsigned int irq_base)
> +{
> +       struct device *dev = &pdev->dev;
> +       int ab_parent_irq = platform_get_irq(pdev, 0);
> +
> +       girq->parent_handler = ep93xx_gpio_ab_irq_handler;
> +       girq->num_parents = 1;
> +       girq->parents = devm_kcalloc(dev, girq->num_parents,
> +                                    sizeof(*girq->parents),
> +                                    GFP_KERNEL);
> +       if (!girq->parents)
> +               return -ENOMEM;
> +       girq->default_type = IRQ_TYPE_NONE;
> +       girq->handler = handle_level_irq;
> +       girq->parents[0] = ab_parent_irq;
> +       girq->first = irq_base;
> +
> +       return 0;
> +}
> +
> +static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev,
> +                                       struct gpio_irq_chip *girq,
> +                                       unsigned int irq_base)
> +{
> +       int gpio_irq;
> +       int i;
> +       struct device *dev = &pdev->dev;
> +
> +       /*
> +        * FIXME: convert this to use hierarchical IRQ support!
> +        * this requires fixing the root irqchip to be hierarchical.
> +        */
> +       girq->parent_handler = ep93xx_gpio_f_irq_handler;
> +       girq->num_parents = 8;
> +       girq->parents = devm_kcalloc(dev, girq->num_parents,
> +                                    sizeof(*girq->parents),
> +                                    GFP_KERNEL);
> +       if (!girq->parents)
> +               return -ENOMEM;
> +       /* Pick resources 1..8 for these IRQs */
> +       for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
> +               girq->parents[i] = platform_get_irq(pdev, i + 1);
> +               gpio_irq = irq_base + i;
> +               irq_set_chip_data(gpio_irq, &epg->gc[5]);
> +               irq_set_chip_and_handler(gpio_irq,
> +                                        &ep93xx_gpio_irq_chip,
> +                                        handle_level_irq);
> +               irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
> +       }
> +       girq->default_type = IRQ_TYPE_NONE;
> +       girq->handler = handle_level_irq;
> +       girq->first = irq_base;
> +
> +       return 0;
> +}
> +
>  static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
>                                 struct platform_device *pdev,
>                                 struct ep93xx_gpio *epg,
> @@ -380,50 +438,16 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
>         }
>  
>         if (bank->has_irq) {
> -               int ab_parent_irq = platform_get_irq(pdev, 0);
> -
> -               girq->parent_handler = ep93xx_gpio_ab_irq_handler;
> -               girq->num_parents = 1;
> -               girq->parents = devm_kcalloc(dev, girq->num_parents,
> -                                            sizeof(*girq->parents),
> -                                            GFP_KERNEL);
> -               if (!girq->parents)
> -                       return -ENOMEM;
> -               girq->default_type = IRQ_TYPE_NONE;
> -               girq->handler = handle_level_irq;
> -               girq->parents[0] = ab_parent_irq;
> -               girq->first = bank->irq_base;
> +               err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank-
> >irq_base);
> +               if (err)
> +                       return err;
>         }
>  
>         /* Only bank F has especially funky IRQ handling */
>         if (bank->has_hierarchical_irq) {
> -               int gpio_irq;
> -               int i;
> -
> -               /*
> -                * FIXME: convert this to use hierarchical IRQ
> support!
> -                * this requires fixing the root irqchip to be
> hierarchical.
> -                */
> -               girq->parent_handler = ep93xx_gpio_f_irq_handler;
> -               girq->num_parents = 8;
> -               girq->parents = devm_kcalloc(dev, girq->num_parents,
> -                                            sizeof(*girq->parents),
> -                                            GFP_KERNEL);
> -               if (!girq->parents)
> -                       return -ENOMEM;
> -               /* Pick resources 1..8 for these IRQs */
> -               for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
> -                       girq->parents[i] = platform_get_irq(pdev, i +
> 1);
> -                       gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> -                       irq_set_chip_data(gpio_irq, &epg->gc[5]);
> -                       irq_set_chip_and_handler(gpio_irq,
> -                                               
> &ep93xx_gpio_irq_chip,
> -                                                handle_level_irq);
> -                       irq_clear_status_flags(gpio_irq,
> IRQ_NOREQUEST);
> -               }
> -               girq->default_type = IRQ_TYPE_NONE;
> -               girq->handler = handle_level_irq;
> -               girq->first = EP93XX_GPIO_F_IRQ_BASE;
> +               err = ep93xx_gpio_add_f_irq_chip(pdev, girq,
> EP93XX_GPIO_F_IRQ_BASE);
> +               if (err)
> +                       return err;
>         }
>  
>         return devm_gpiochip_add_data(dev, gc, epg);

-- 
Alexander Sverdlin.

[PATCH v2 8/9] gpio: ep93xx: refactor base IRQ number

[Nikita Shubin]

- use predefined constants instead of plain numbers
- use provided bank IRQ number instead of defined constant
  for port F

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index b212c007240e..2aee13b5067d 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -28,6 +28,8 @@
 /* Maximum value for irq capable line identifiers */
 #define EP93XX_GPIO_LINE_MAX_IRQ 23
 
+#define EP93XX_GPIO_A_IRQ_BASE 64
+#define EP93XX_GPIO_B_IRQ_BASE 72
 /*
  * Static mapping of GPIO bank F IRQS:
  * F0..F7 (16..24) to irq 80..87.
@@ -311,14 +313,14 @@ struct ep93xx_gpio_bank {
 
 static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
 	/* Bank A has 8 IRQs */
-	EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64),
+	EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, EP93XX_GPIO_A_IRQ_BASE),
 	/* Bank B has 8 IRQs */
-	EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
+	EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, EP93XX_GPIO_B_IRQ_BASE),
 	EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
 	EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
 	EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
 	/* Bank F has 8 IRQs */
-	EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
+	EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, EP93XX_GPIO_F_IRQ_BASE),
 	EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
 	EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
 };
@@ -445,7 +447,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 
 	/* Only bank F has especially funky IRQ handling */
 	if (bank->has_hierarchical_irq) {
-		err = ep93xx_gpio_add_f_irq_chip(pdev, girq, EP93XX_GPIO_F_IRQ_BASE);
+		err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank->irq_base);
 		if (err)
 			return err;
 	}
-- 
2.29.2

Re: [PATCH v2 8/9] gpio: ep93xx: refactor base IRQ number

[Alexander Sverdlin]

Hi!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> - use predefined constants instead of plain numbers
> - use provided bank IRQ number instead of defined constant
>   for port F
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/gpio-ep93xx.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index b212c007240e..2aee13b5067d 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -28,6 +28,8 @@
>  /* Maximum value for irq capable line identifiers */
>  #define EP93XX_GPIO_LINE_MAX_IRQ 23
>  
> +#define EP93XX_GPIO_A_IRQ_BASE 64
> +#define EP93XX_GPIO_B_IRQ_BASE 72
>  /*
>   * Static mapping of GPIO bank F IRQS:
>   * F0..F7 (16..24) to irq 80..87.
> @@ -311,14 +313,14 @@ struct ep93xx_gpio_bank {
>  
>  static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
>         /* Bank A has 8 IRQs */
> -       EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64),
> +       EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false,
> EP93XX_GPIO_A_IRQ_BASE),
>         /* Bank B has 8 IRQs */
> -       EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
> +       EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false,
> EP93XX_GPIO_B_IRQ_BASE),
>         EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
>         EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
>         EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
>         /* Bank F has 8 IRQs */
> -       EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
> +       EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true,
> EP93XX_GPIO_F_IRQ_BASE),
>         EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
>         EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
>  };
> @@ -445,7 +447,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>  
>         /* Only bank F has especially funky IRQ handling */
>         if (bank->has_hierarchical_irq) {
> -               err = ep93xx_gpio_add_f_irq_chip(pdev, girq,
> EP93XX_GPIO_F_IRQ_BASE);
> +               err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank-
> >irq_base);
>                 if (err)
>                         return err;
>         }

-- 
Alexander Sverdlin.

[PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports

[Nikita Shubin]

Replace boolean values used for determining if gpiochip is IRQ capable
or not with index.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 47 ++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 2aee13b5067d..f75a33b79504 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -44,6 +44,8 @@ struct ep93xx_gpio {
 	struct irq_chip		ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
 };
 
+#define EP93XX_GPIO_A_PORT_INDEX 0
+#define EP93XX_GPIO_B_PORT_INDEX 1
 /*
  * F Port index in GPIOCHIP'S array is 5
  * but we use index 2 for stored values and offsets
@@ -291,38 +293,36 @@ static struct irq_chip ep93xx_gpio_irq_chip = {
  * gpiolib interface for EP93xx on-chip GPIOs
  *************************************************************************/
 struct ep93xx_gpio_bank {
+	u8		idx;
 	const char	*label;
 	int		data;
 	int		dir;
 	int		base;
-	bool		has_irq;
-	bool		has_hierarchical_irq;
 	unsigned int	irq_base;
 };
 
-#define EP93XX_GPIO_BANK(_label, _data, _dir, _base, _has_irq, _has_hier, _irq_base) \
+#define EP93XX_GPIO_BANK(_index, _label, _data, _dir, _base, _irq_base) \
 	{							\
+		.idx		= _index,			\
 		.label		= _label,			\
 		.data		= _data,			\
 		.dir		= _dir,				\
 		.base		= _base,			\
-		.has_irq	= _has_irq,			\
-		.has_hierarchical_irq = _has_hier,		\
 		.irq_base	= _irq_base,			\
 	}
 
 static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
 	/* Bank A has 8 IRQs */
-	EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, EP93XX_GPIO_A_IRQ_BASE),
+	EP93XX_GPIO_BANK(0, "A", 0x00, 0x10, 0, EP93XX_GPIO_A_IRQ_BASE),
 	/* Bank B has 8 IRQs */
-	EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, EP93XX_GPIO_B_IRQ_BASE),
-	EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
-	EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
-	EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
+	EP93XX_GPIO_BANK(1, "B", 0x04, 0x14, 8, EP93XX_GPIO_B_IRQ_BASE),
+	EP93XX_GPIO_BANK(2, "C", 0x08, 0x18, 40, 0),
+	EP93XX_GPIO_BANK(3, "D", 0x0c, 0x1c, 24, 0),
+	EP93XX_GPIO_BANK(4, "E", 0x20, 0x24, 32, 0),
 	/* Bank F has 8 IRQs */
-	EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, EP93XX_GPIO_F_IRQ_BASE),
-	EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
-	EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
+	EP93XX_GPIO_BANK(5, "F", 0x30, 0x34, 16, EP93XX_GPIO_F_IRQ_BASE),
+	EP93XX_GPIO_BANK(6, "G", 0x38, 0x3c, 48, 0),
+	EP93XX_GPIO_BANK(7, "H", 0x40, 0x44, 56, 0),
 };
 
 static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
@@ -356,10 +356,11 @@ static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
 }
 
 static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev,
-					struct gpio_irq_chip *girq,
+					struct gpio_chip *gc,
 					unsigned int irq_base)
 {
 	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq = &gc->irq;
 	int ab_parent_irq = platform_get_irq(pdev, 0);
 
 	girq->parent_handler = ep93xx_gpio_ab_irq_handler;
@@ -378,12 +379,13 @@ static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev,
 }
 
 static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev,
-					struct gpio_irq_chip *girq,
+					struct gpio_chip *gc,
 					unsigned int irq_base)
 {
 	int gpio_irq;
 	int i;
 	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq = &gc->irq;
 
 	/*
 	 * FIXME: convert this to use hierarchical IRQ support!
@@ -397,10 +399,10 @@ static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev,
 	if (!girq->parents)
 		return -ENOMEM;
 	/* Pick resources 1..8 for these IRQs */
-	for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
+	for (i = 0; i < girq->num_parents; i++) {
 		girq->parents[i] = platform_get_irq(pdev, i + 1);
 		gpio_irq = irq_base + i;
-		irq_set_chip_data(gpio_irq, &epg->gc[5]);
+		irq_set_chip_data(gpio_irq, gc);
 		irq_set_chip_and_handler(gpio_irq,
 					 &ep93xx_gpio_irq_chip,
 					 handle_level_irq);
@@ -433,21 +435,22 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
 	gc->base = bank->base;
 
 	girq = &gc->irq;
-	if (bank->has_irq || bank->has_hierarchical_irq) {
+	if (bank->irq_base != 0) {
 		gc->set_config = ep93xx_gpio_set_config;
 		port = ep93xx_gpio_port(epg, gc);
 		girq->chip = &epg->ic[port];
 	}
 
-	if (bank->has_irq) {
-		err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank->irq_base);
+	if (bank->idx == EP93XX_GPIO_A_PORT_INDEX ||
+		bank->idx == EP93XX_GPIO_B_PORT_INDEX) {
+		err = ep93xx_gpio_add_ab_irq_chip(pdev, gc, bank->irq_base);
 		if (err)
 			return err;
 	}
 
 	/* Only bank F has especially funky IRQ handling */
-	if (bank->has_hierarchical_irq) {
-		err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank->irq_base);
+	if (bank->idx == EP93XX_GPIO_F_PORT_INDEX) {
+		err = ep93xx_gpio_add_f_irq_chip(pdev, gc, bank->irq_base);
 		if (err)
 			return err;
 	}
-- 
2.29.2

Re: [PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports

[Alexander Sverdlin]

Hello Nikita!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Replace boolean values used for determining if gpiochip is IRQ
> capable
> or not with index.

What's the purpose of this patch?
It neither makes the code more effective nor does it make
the code more readable IMO. There are also some changes in the
code which were not mentioned in the commit message.

> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpio-ep93xx.c | 47 ++++++++++++++++++++----------------
> --
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 2aee13b5067d..f75a33b79504 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -44,6 +44,8 @@ struct ep93xx_gpio {
>         struct irq_chip         ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
>  };
>  
> +#define EP93XX_GPIO_A_PORT_INDEX 0
> +#define EP93XX_GPIO_B_PORT_INDEX 1
>  /*
>   * F Port index in GPIOCHIP'S array is 5
>   * but we use index 2 for stored values and offsets
> @@ -291,38 +293,36 @@ static struct irq_chip ep93xx_gpio_irq_chip = {
>   * gpiolib interface for EP93xx on-chip GPIOs
>  
> *********************************************************************
> ****/
>  struct ep93xx_gpio_bank {
> +       u8              idx;
>         const char      *label;
>         int             data;
>         int             dir;
>         int             base;
> -       bool            has_irq;
> -       bool            has_hierarchical_irq;
>         unsigned int    irq_base;
>  };
>  
> -#define EP93XX_GPIO_BANK(_label, _data, _dir, _base, _has_irq,
> _has_hier, _irq_base) \
> +#define EP93XX_GPIO_BANK(_index, _label, _data, _dir, _base,
> _irq_base) \
>         {                                                       \
> +               .idx            = _index,                       \
>                 .label          = _label,                       \
>                 .data           = _data,                        \
>                 .dir            = _dir,                         \
>                 .base           = _base,                        \
> -               .has_irq        = _has_irq,                     \
> -               .has_hierarchical_irq = _has_hier,              \
>                 .irq_base       = _irq_base,                    \
>         }
>  
>  static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
>         /* Bank A has 8 IRQs */
> -       EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false,
> EP93XX_GPIO_A_IRQ_BASE),
> +       EP93XX_GPIO_BANK(0, "A", 0x00, 0x10, 0,
> EP93XX_GPIO_A_IRQ_BASE),
>         /* Bank B has 8 IRQs */
> -       EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false,
> EP93XX_GPIO_B_IRQ_BASE),
> -       EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
> -       EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
> -       EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
> +       EP93XX_GPIO_BANK(1, "B", 0x04, 0x14, 8,
> EP93XX_GPIO_B_IRQ_BASE),
> +       EP93XX_GPIO_BANK(2, "C", 0x08, 0x18, 40, 0),
> +       EP93XX_GPIO_BANK(3, "D", 0x0c, 0x1c, 24, 0),
> +       EP93XX_GPIO_BANK(4, "E", 0x20, 0x24, 32, 0),
>         /* Bank F has 8 IRQs */
> -       EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true,
> EP93XX_GPIO_F_IRQ_BASE),
> -       EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
> -       EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
> +       EP93XX_GPIO_BANK(5, "F", 0x30, 0x34, 16,
> EP93XX_GPIO_F_IRQ_BASE),
> +       EP93XX_GPIO_BANK(6, "G", 0x38, 0x3c, 48, 0),
> +       EP93XX_GPIO_BANK(7, "H", 0x40, 0x44, 56, 0),
>  };
>  
>  static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned
> offset,
> @@ -356,10 +356,11 @@ static void ep93xx_init_irq_chips(struct
> ep93xx_gpio *epg)
>  }
>  
>  static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev,
> -                                       struct gpio_irq_chip *girq,
> +                                       struct gpio_chip *gc,
>                                         unsigned int irq_base)
>  {
>         struct device *dev = &pdev->dev;
> +       struct gpio_irq_chip *girq = &gc->irq;
>         int ab_parent_irq = platform_get_irq(pdev, 0);
>  
>         girq->parent_handler = ep93xx_gpio_ab_irq_handler;
> @@ -378,12 +379,13 @@ static int ep93xx_gpio_add_ab_irq_chip(struct
> platform_device *pdev,
>  }
>  
>  static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev,
> -                                       struct gpio_irq_chip *girq,
> +                                       struct gpio_chip *gc,
>                                         unsigned int irq_base)
>  {
>         int gpio_irq;
>         int i;
>         struct device *dev = &pdev->dev;
> +       struct gpio_irq_chip *girq = &gc->irq;
>  
>         /*
>          * FIXME: convert this to use hierarchical IRQ support!
> @@ -397,10 +399,10 @@ static int ep93xx_gpio_add_f_irq_chip(struct
> platform_device *pdev,
>         if (!girq->parents)
>                 return -ENOMEM;
>         /* Pick resources 1..8 for these IRQs */
> -       for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
> +       for (i = 0; i < girq->num_parents; i++) {
>                 girq->parents[i] = platform_get_irq(pdev, i + 1);
>                 gpio_irq = irq_base + i;
> -               irq_set_chip_data(gpio_irq, &epg->gc[5]);
> +               irq_set_chip_data(gpio_irq, gc);
>                 irq_set_chip_and_handler(gpio_irq,
>                                          &ep93xx_gpio_irq_chip,
>                                          handle_level_irq);
> @@ -433,21 +435,22 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
>         gc->base = bank->base;
>  
>         girq = &gc->irq;
> -       if (bank->has_irq || bank->has_hierarchical_irq) {
> +       if (bank->irq_base != 0) {
>                 gc->set_config = ep93xx_gpio_set_config;
>                 port = ep93xx_gpio_port(epg, gc);
>                 girq->chip = &epg->ic[port];
>         }
>  
> -       if (bank->has_irq) {
> -               err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank-
> >irq_base);
> +       if (bank->idx == EP93XX_GPIO_A_PORT_INDEX ||
> +               bank->idx == EP93XX_GPIO_B_PORT_INDEX) {
> +               err = ep93xx_gpio_add_ab_irq_chip(pdev, gc, bank-
> >irq_base);
>                 if (err)
>                         return err;
>         }
>  
>         /* Only bank F has especially funky IRQ handling */
> -       if (bank->has_hierarchical_irq) {
> -               err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank-
> >irq_base);
> +       if (bank->idx == EP93XX_GPIO_F_PORT_INDEX) {
> +               err = ep93xx_gpio_add_f_irq_chip(pdev, gc, bank-
> >irq_base);
>                 if (err)
>                         return err;
>         }

-- 
Alexander Sverdlin.

Re: [PATCH v2 0/9] gpio: ep93xx: fixes series patch

[Linus Walleij]

On Wed, Jan 27, 2021 at 11:46 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:

> Series of patches to fix ep93xx gpio driver to make IRQ's working.
>
> The following are fix patches (these are enough to get gpio-ep93xx
> working with modern kernel):

I see that there is a strange level of attention to patches to this
platform!

Since you fix all my mistakes made in converting this driver
in the past I will just say:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

For all of them.

There are some nitpicks from the reviewers to fix up but
overall this looks very very good.

Yours,
Linus Walleij

Re: [PATCH v2 0/9] gpio: ep93xx: fixes series patch

[Alexander Sverdlin]

Hello Linus,

On Wed, 2021-01-27 at 22:54 +0100, Linus Walleij wrote:
> > Series of patches to fix ep93xx gpio driver to make IRQ's working.
> > 
> > The following are fix patches (these are enough to get gpio-ep93xx
> > working with modern kernel):
> 
> I see that there is a strange level of attention to patches to this
> platform!
> 
> Since you fix all my mistakes made in converting this driver
> in the past I will just say:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> For all of them.
> 
> There are some nitpicks from the reviewers to fix up but
> overall this looks very very good.

as we don't have a dedicated EP93xx tree, would you like to take
the series in one of your trees?

-- 
Alexander Sverdlin.

Re: [PATCH v2 0/9] gpio: ep93xx: fixes series patch

[Linus Walleij]

On Thu, Jan 28, 2021 at 11:19 AM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:

> as we don't have a dedicated EP93xx tree, would you like to take
> the series in one of your trees?

Bartosz is managing the GPIO tree right now and I think he will
queue it once all reviews are finished.

Yours,
Linus Walleij