[PATCH v5 0/7] gpio: ep93xx: fixes series patch

[PATCH v5 0/7] gpio: ep93xx: fixes series patch

[Nikita Shubin]

v2: 
https://lore.kernel.org/linux-gpio/20210127104617.1173-1-nikita.shubin@maquefel.me/

v3:
https://lore.kernel.org/linux-gpio/20210128122123.25341-1-nikita.shubin@maquefel.me/

v4:
https://lore.kernel.org/linux-gpio/20210205080507.16007-1-nikita.shubin@maquefel.me/

v4->v5 changes

[PATCH v5 1/7] gpio: ep93xx: fix BUG_ON port F usage
Alexander Sverdlin:
- make to_ep93xx_gpio_irq_chip() static 

[PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips
Alexander Sverdlin:
- generate IRQ chip's names dynamicaly from label

[PATCH v5 1/7] gpio: ep93xx: fix BUG_ON port F usage

[Nikita Shubin]

Two index spaces and ep93xx_gpio_port are confusing.

Instead add a separate struct to store necessary data and remove
ep93xx_gpio_port.

- add struct to store IRQ related data for each IRQ capable chip
- replace offset array with defined offsets
- add IRQ registers offset for each IRQ capable chip into
  ep93xx_gpio_banks

------------[ cut here ]------------
kernel BUG at drivers/gpio/gpio-ep93xx.c:64!
---[ end trace 3f6544e133e9f5ae ]---

Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers")
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
v4->v5:
- make to_ep93xx_gpio_irq_chip() static
---
 drivers/gpio/gpio-ep93xx.c | 186 ++++++++++++++++++++-----------------
 1 file changed, 99 insertions(+), 87 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 226da8df6f10..64d6c2b4282e 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -25,6 +25,9 @@
 /* Maximum value for gpio line identifiers */
 #define EP93XX_GPIO_LINE_MAX 63
 
+/* Number of GPIO chips in EP93XX */
+#define EP93XX_GPIO_CHIP_NUM 8
+
 /* Maximum value for irq capable line identifiers */
 #define EP93XX_GPIO_LINE_MAX_IRQ 23
 
@@ -34,74 +37,74 @@
  */
 #define EP93XX_GPIO_F_IRQ_BASE 80
 
-struct ep93xx_gpio {
-	void __iomem		*base;
-	struct gpio_chip	gc[8];
+struct ep93xx_gpio_irq_chip {
+	u8 irq_offset;
+	u8 int_unmasked;
+	u8 int_enabled;
+	u8 int_type1;
+	u8 int_type2;
+	u8 int_debounce;
 };
 
-/*************************************************************************
- * Interrupt handling for EP93xx on-chip GPIOs
- *************************************************************************/
-static unsigned char gpio_int_unmasked[3];
-static unsigned char gpio_int_enabled[3];
-static unsigned char gpio_int_type1[3];
-static unsigned char gpio_int_type2[3];
-static unsigned char gpio_int_debounce[3];
-
-/* Port ordering is: A B 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 };
-static const u8 int_en_register_offset[3]	= { 0x9c, 0xb8, 0x58 };
-static const u8 int_debounce_register_offset[3]	= { 0xa8, 0xc4, 0x64 };
-
-static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg, unsigned port)
-{
-	BUG_ON(port > 2);
+struct ep93xx_gpio_chip {
+	struct gpio_chip		gc;
+	struct ep93xx_gpio_irq_chip	*eic;
+};
 
-	writeb_relaxed(0, epg->base + int_en_register_offset[port]);
+struct ep93xx_gpio {
+	void __iomem		*base;
+	struct ep93xx_gpio_chip	gc[EP93XX_GPIO_CHIP_NUM];
+};
 
-	writeb_relaxed(gpio_int_type2[port],
-		       epg->base + int_type2_register_offset[port]);
+#define to_ep93xx_gpio_chip(x) container_of(x, struct ep93xx_gpio_chip, gc)
 
-	writeb_relaxed(gpio_int_type1[port],
-		       epg->base + int_type1_register_offset[port]);
+static struct ep93xx_gpio_irq_chip *to_ep93xx_gpio_irq_chip(struct gpio_chip *gc)
+{
+	struct ep93xx_gpio_chip *egc = to_ep93xx_gpio_chip(gc);
 
-	writeb(gpio_int_unmasked[port] & gpio_int_enabled[port],
-	       epg->base + int_en_register_offset[port]);
+	return egc->eic;
 }
 
-static int ep93xx_gpio_port(struct gpio_chip *gc)
+/*************************************************************************
+ * Interrupt handling for EP93xx on-chip GPIOs
+ *************************************************************************/
+#define EP93XX_INT_TYPE1_OFFSET		0x00
+#define EP93XX_INT_TYPE2_OFFSET		0x04
+#define EP93XX_INT_EOI_OFFSET		0x08
+#define EP93XX_INT_EN_OFFSET		0x0c
+#define EP93XX_INT_STATUS_OFFSET	0x10
+#define EP93XX_INT_RAW_STATUS_OFFSET	0x14
+#define EP93XX_INT_DEBOUNCE_OFFSET	0x18
+
+static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg,
+					  struct ep93xx_gpio_irq_chip *eic)
 {
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = 0;
+	writeb_relaxed(0, epg->base + eic->irq_offset + EP93XX_INT_EN_OFFSET);
 
-	while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
-		port++;
+	writeb_relaxed(eic->int_type2,
+		       epg->base + eic->irq_offset + EP93XX_INT_TYPE2_OFFSET);
 
-	/* This should not happen but is there as a last safeguard */
-	if (port == ARRAY_SIZE(epg->gc)) {
-		pr_crit("can't find the GPIO port\n");
-		return 0;
-	}
+	writeb_relaxed(eic->int_type1,
+		       epg->base + eic->irq_offset + EP93XX_INT_TYPE1_OFFSET);
 
-	return port;
+	writeb_relaxed(eic->int_unmasked & eic->int_enabled,
+		       epg->base + eic->irq_offset + EP93XX_INT_EN_OFFSET);
 }
 
 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);
+	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	int port_mask = BIT(offset);
 
 	if (enable)
-		gpio_int_debounce[port] |= port_mask;
+		eic->int_debounce |= port_mask;
 	else
-		gpio_int_debounce[port] &= ~port_mask;
+		eic->int_debounce &= ~port_mask;
 
-	writeb(gpio_int_debounce[port],
-	       epg->base + int_debounce_register_offset[port]);
+	writeb(eic->int_debounce,
+	       epg->base + eic->irq_offset + EP93XX_INT_DEBOUNCE_OFFSET);
 }
 
 static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
@@ -122,12 +125,12 @@ static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
 	 */
 	stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS);
 	for_each_set_bit(offset, &stat, 8)
-		generic_handle_irq(irq_find_mapping(epg->gc[0].irq.domain,
+		generic_handle_irq(irq_find_mapping(epg->gc[0].gc.irq.domain,
 						    offset));
 
 	stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS);
 	for_each_set_bit(offset, &stat, 8)
-		generic_handle_irq(irq_find_mapping(epg->gc[1].irq.domain,
+		generic_handle_irq(irq_find_mapping(epg->gc[1].gc.irq.domain,
 						    offset));
 
 	chained_irq_exit(irqchip, desc);
@@ -153,52 +156,52 @@ static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
 static void ep93xx_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
 	int port_mask = BIT(d->irq & 7);
 
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
-		gpio_int_type2[port] ^= port_mask; /* switch edge direction */
-		ep93xx_gpio_update_int_params(epg, port);
+		eic->int_type2 ^= port_mask; /* switch edge direction */
+		ep93xx_gpio_update_int_params(epg, eic);
 	}
 
-	writeb(port_mask, epg->base + eoi_register_offset[port]);
+	writeb(port_mask, epg->base + eic->irq_offset + EP93XX_INT_EOI_OFFSET);
 }
 
 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_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
 	int port_mask = BIT(d->irq & 7);
 
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
-		gpio_int_type2[port] ^= port_mask; /* switch edge direction */
+		eic->int_type2 ^= port_mask; /* switch edge direction */
 
-	gpio_int_unmasked[port] &= ~port_mask;
-	ep93xx_gpio_update_int_params(epg, port);
+	eic->int_unmasked &= ~port_mask;
+	ep93xx_gpio_update_int_params(epg, eic);
 
-	writeb(port_mask, epg->base + eoi_register_offset[port]);
+	writeb(port_mask, epg->base + eic->irq_offset + EP93XX_INT_EOI_OFFSET);
 }
 
 static void ep93xx_gpio_irq_mask(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
 
-	gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
-	ep93xx_gpio_update_int_params(epg, port);
+	eic->int_unmasked &= ~BIT(d->irq & 7);
+	ep93xx_gpio_update_int_params(epg, eic);
 }
 
 static void ep93xx_gpio_irq_unmask(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
 
-	gpio_int_unmasked[port] |= BIT(d->irq & 7);
-	ep93xx_gpio_update_int_params(epg, port);
+	eic->int_unmasked |= BIT(d->irq & 7);
+	ep93xx_gpio_update_int_params(epg, eic);
 }
 
 /*
@@ -209,8 +212,8 @@ static void ep93xx_gpio_irq_unmask(struct irq_data *d)
 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_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	int port = ep93xx_gpio_port(gc);
 	int offset = d->irq & 7;
 	int port_mask = BIT(offset);
 	irq_flow_handler_t handler;
@@ -219,32 +222,32 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
-		gpio_int_type1[port] |= port_mask;
-		gpio_int_type2[port] |= port_mask;
+		eic->int_type1 |= port_mask;
+		eic->int_type2 |= port_mask;
 		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		gpio_int_type1[port] |= port_mask;
-		gpio_int_type2[port] &= ~port_mask;
+		eic->int_type1 |= port_mask;
+		eic->int_type2 &= ~port_mask;
 		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		gpio_int_type1[port] &= ~port_mask;
-		gpio_int_type2[port] |= port_mask;
+		eic->int_type1 &= ~port_mask;
+		eic->int_type2 |= port_mask;
 		handler = handle_level_irq;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		gpio_int_type1[port] &= ~port_mask;
-		gpio_int_type2[port] &= ~port_mask;
+		eic->int_type1 &= ~port_mask;
+		eic->int_type2 &= ~port_mask;
 		handler = handle_level_irq;
 		break;
 	case IRQ_TYPE_EDGE_BOTH:
-		gpio_int_type1[port] |= port_mask;
+		eic->int_type1 |= port_mask;
 		/* set initial polarity based on current input level */
 		if (gc->get(gc, offset))
-			gpio_int_type2[port] &= ~port_mask; /* falling */
+			eic->int_type2 &= ~port_mask; /* falling */
 		else
-			gpio_int_type2[port] |= port_mask; /* rising */
+			eic->int_type2 |= port_mask; /* rising */
 		handler = handle_edge_irq;
 		break;
 	default:
@@ -253,9 +256,9 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
 
 	irq_set_handler_locked(d, handler);
 
-	gpio_int_enabled[port] |= port_mask;
+	eic->int_enabled |= port_mask;
 
-	ep93xx_gpio_update_int_params(epg, port);
+	ep93xx_gpio_update_int_params(epg, eic);
 
 	return 0;
 }
@@ -276,17 +279,19 @@ struct ep93xx_gpio_bank {
 	const char	*label;
 	int		data;
 	int		dir;
+	int		irq;
 	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(_label, _data, _dir, _irq, _base, _has_irq, _has_hier, _irq_base) \
 	{							\
 		.label		= _label,			\
 		.data		= _data,			\
 		.dir		= _dir,				\
+		.irq		= _irq,				\
 		.base		= _base,			\
 		.has_irq	= _has_irq,			\
 		.has_hierarchical_irq = _has_hier,		\
@@ -295,16 +300,16 @@ 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, 0x90, 0, true, false, 64),
 	/* Bank B has 8 IRQs */
-	EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
-	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("B", 0x04, 0x14, 0xac, 8, true, false, 72),
+	EP93XX_GPIO_BANK("C", 0x08, 0x18, 0x00, 40, false, false, 0),
+	EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 0x00, 24, false, false, 0),
+	EP93XX_GPIO_BANK("E", 0x20, 0x24, 0x00, 32, false, false, 0),
 	/* Bank F has 8 IRQs */
-	EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
-	EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
-	EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
+	EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, 0),
+	EP93XX_GPIO_BANK("G", 0x38, 0x3c, 0x00, 48, false, false, 0),
+	EP93XX_GPIO_BANK("H", 0x40, 0x44, 0x00, 56, false, false, 0),
 };
 
 static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
@@ -326,13 +331,14 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset)
 	return EP93XX_GPIO_F_IRQ_BASE + offset;
 }
 
-static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
+static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 				struct platform_device *pdev,
 				struct ep93xx_gpio *epg,
 				struct ep93xx_gpio_bank *bank)
 {
 	void __iomem *data = epg->base + bank->data;
 	void __iomem *dir = epg->base + bank->dir;
+	struct gpio_chip *gc = &egc->gc;
 	struct device *dev = &pdev->dev;
 	struct gpio_irq_chip *girq;
 	int err;
@@ -347,6 +353,12 @@ 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;
+		egc->eic = devm_kcalloc(dev, 1,
+					sizeof(*egc->eic),
+					GFP_KERNEL);
+		if (!egc->eic)
+			return -ENOMEM;
+		egc->eic->irq_offset = bank->irq;
 		girq->chip = &ep93xx_gpio_irq_chip;
 	}
 
@@ -415,7 +427,7 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(epg->base);
 
 	for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
-		struct gpio_chip *gc = &epg->gc[i];
+		struct ep93xx_gpio_chip *gc = &epg->gc[i];
 		struct ep93xx_gpio_bank *bank = &ep93xx_gpio_banks[i];
 
 		if (ep93xx_gpio_add_bank(gc, pdev, epg, bank))
-- 
2.26.2

Re: [PATCH v5 1/7] gpio: ep93xx: fix BUG_ON port F usage

[Alexander Sverdlin]

Hi Nikita!

On Mon, 2021-02-08 at 11:59 +0300, Nikita Shubin wrote:
> Two index spaces and ep93xx_gpio_port are confusing.
> 
> Instead add a separate struct to store necessary data and remove
> ep93xx_gpio_port.
> 
> - add struct to store IRQ related data for each IRQ capable chip
> - replace offset array with defined offsets
> - add IRQ registers offset for each IRQ capable chip into
>   ep93xx_gpio_banks
> 
> ------------[ cut here ]------------
> kernel BUG at drivers/gpio/gpio-ep93xx.c:64!
> ---[ end trace 3f6544e133e9f5ae ]---
> 
> Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers")
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

I performed a bootup with the whole patch-series and
confirm that the BUG is gone.

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

> ---
> v4->v5:
> - make to_ep93xx_gpio_irq_chip() static
> ---
>  drivers/gpio/gpio-ep93xx.c | 186 ++++++++++++++++++++-----------------
>  1 file changed, 99 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 226da8df6f10..64d6c2b4282e 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -25,6 +25,9 @@
>  /* Maximum value for gpio line identifiers */
>  #define EP93XX_GPIO_LINE_MAX 63
>  
> +/* Number of GPIO chips in EP93XX */
> +#define EP93XX_GPIO_CHIP_NUM 8
> +
>  /* Maximum value for irq capable line identifiers */
>  #define EP93XX_GPIO_LINE_MAX_IRQ 23
>  
> @@ -34,74 +37,74 @@
>   */
>  #define EP93XX_GPIO_F_IRQ_BASE 80
>  
> -struct ep93xx_gpio {
> -       void __iomem            *base;
> -       struct gpio_chip        gc[8];
> +struct ep93xx_gpio_irq_chip {
> +       u8 irq_offset;
> +       u8 int_unmasked;
> +       u8 int_enabled;
> +       u8 int_type1;
> +       u8 int_type2;
> +       u8 int_debounce;
>  };
>  
> -/*************************************************************************
> - * Interrupt handling for EP93xx on-chip GPIOs
> - *************************************************************************/
> -static unsigned char gpio_int_unmasked[3];
> -static unsigned char gpio_int_enabled[3];
> -static unsigned char gpio_int_type1[3];
> -static unsigned char gpio_int_type2[3];
> -static unsigned char gpio_int_debounce[3];
> -
> -/* Port ordering is: A B 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 };
> -static const u8 int_en_register_offset[3]      = { 0x9c, 0xb8, 0x58 };
> -static const u8 int_debounce_register_offset[3]        = { 0xa8, 0xc4, 0x64 };
> -
> -static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg, unsigned port)
> -{
> -       BUG_ON(port > 2);
> +struct ep93xx_gpio_chip {
> +       struct gpio_chip                gc;
> +       struct ep93xx_gpio_irq_chip     *eic;
> +};
>  
> -       writeb_relaxed(0, epg->base + int_en_register_offset[port]);
> +struct ep93xx_gpio {
> +       void __iomem            *base;
> +       struct ep93xx_gpio_chip gc[EP93XX_GPIO_CHIP_NUM];
> +};
>  
> -       writeb_relaxed(gpio_int_type2[port],
> -                      epg->base + int_type2_register_offset[port]);
> +#define to_ep93xx_gpio_chip(x) container_of(x, struct ep93xx_gpio_chip, gc)
>  
> -       writeb_relaxed(gpio_int_type1[port],
> -                      epg->base + int_type1_register_offset[port]);
> +static struct ep93xx_gpio_irq_chip *to_ep93xx_gpio_irq_chip(struct gpio_chip *gc)
> +{
> +       struct ep93xx_gpio_chip *egc = to_ep93xx_gpio_chip(gc);
>  
> -       writeb(gpio_int_unmasked[port] & gpio_int_enabled[port],
> -              epg->base + int_en_register_offset[port]);
> +       return egc->eic;
>  }
>  
> -static int ep93xx_gpio_port(struct gpio_chip *gc)
> +/*************************************************************************
> + * Interrupt handling for EP93xx on-chip GPIOs
> + *************************************************************************/
> +#define EP93XX_INT_TYPE1_OFFSET                0x00
> +#define EP93XX_INT_TYPE2_OFFSET                0x04
> +#define EP93XX_INT_EOI_OFFSET          0x08
> +#define EP93XX_INT_EN_OFFSET           0x0c
> +#define EP93XX_INT_STATUS_OFFSET       0x10
> +#define EP93XX_INT_RAW_STATUS_OFFSET   0x14
> +#define EP93XX_INT_DEBOUNCE_OFFSET     0x18
> +
> +static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg,
> +                                         struct ep93xx_gpio_irq_chip *eic)
>  {
> -       struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = 0;
> +       writeb_relaxed(0, epg->base + eic->irq_offset + EP93XX_INT_EN_OFFSET);
>  
> -       while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
> -               port++;
> +       writeb_relaxed(eic->int_type2,
> +                      epg->base + eic->irq_offset + EP93XX_INT_TYPE2_OFFSET);
>  
> -       /* This should not happen but is there as a last safeguard */
> -       if (port == ARRAY_SIZE(epg->gc)) {
> -               pr_crit("can't find the GPIO port\n");
> -               return 0;
> -       }
> +       writeb_relaxed(eic->int_type1,
> +                      epg->base + eic->irq_offset + EP93XX_INT_TYPE1_OFFSET);
>  
> -       return port;
> +       writeb_relaxed(eic->int_unmasked & eic->int_enabled,
> +                      epg->base + eic->irq_offset + EP93XX_INT_EN_OFFSET);
>  }
>  
>  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);
> +       struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>         int port_mask = BIT(offset);
>  
>         if (enable)
> -               gpio_int_debounce[port] |= port_mask;
> +               eic->int_debounce |= port_mask;
>         else
> -               gpio_int_debounce[port] &= ~port_mask;
> +               eic->int_debounce &= ~port_mask;
>  
> -       writeb(gpio_int_debounce[port],
> -              epg->base + int_debounce_register_offset[port]);
> +       writeb(eic->int_debounce,
> +              epg->base + eic->irq_offset + EP93XX_INT_DEBOUNCE_OFFSET);
>  }
>  
>  static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
> @@ -122,12 +125,12 @@ static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
>          */
>         stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS);
>         for_each_set_bit(offset, &stat, 8)
> -               generic_handle_irq(irq_find_mapping(epg->gc[0].irq.domain,
> +               generic_handle_irq(irq_find_mapping(epg->gc[0].gc.irq.domain,
>                                                     offset));
>  
>         stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS);
>         for_each_set_bit(offset, &stat, 8)
> -               generic_handle_irq(irq_find_mapping(epg->gc[1].irq.domain,
> +               generic_handle_irq(irq_find_mapping(epg->gc[1].gc.irq.domain,
>                                                     offset));
>  
>         chained_irq_exit(irqchip, desc);
> @@ -153,52 +156,52 @@ static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
>  static void ep93xx_gpio_irq_ack(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
>         int port_mask = BIT(d->irq & 7);
>  
>         if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
> -               gpio_int_type2[port] ^= port_mask; /* switch edge direction */
> -               ep93xx_gpio_update_int_params(epg, port);
> +               eic->int_type2 ^= port_mask; /* switch edge direction */
> +               ep93xx_gpio_update_int_params(epg, eic);
>         }
>  
> -       writeb(port_mask, epg->base + eoi_register_offset[port]);
> +       writeb(port_mask, epg->base + eic->irq_offset + EP93XX_INT_EOI_OFFSET);
>  }
>  
>  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_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
>         int port_mask = BIT(d->irq & 7);
>  
>         if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
> -               gpio_int_type2[port] ^= port_mask; /* switch edge direction */
> +               eic->int_type2 ^= port_mask; /* switch edge direction */
>  
> -       gpio_int_unmasked[port] &= ~port_mask;
> -       ep93xx_gpio_update_int_params(epg, port);
> +       eic->int_unmasked &= ~port_mask;
> +       ep93xx_gpio_update_int_params(epg, eic);
>  
> -       writeb(port_mask, epg->base + eoi_register_offset[port]);
> +       writeb(port_mask, epg->base + eic->irq_offset + EP93XX_INT_EOI_OFFSET);
>  }
>  
>  static void ep93xx_gpio_irq_mask(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
>  
> -       gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
> -       ep93xx_gpio_update_int_params(epg, port);
> +       eic->int_unmasked &= ~BIT(d->irq & 7);
> +       ep93xx_gpio_update_int_params(epg, eic);
>  }
>  
>  static void ep93xx_gpio_irq_unmask(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
>  
> -       gpio_int_unmasked[port] |= BIT(d->irq & 7);
> -       ep93xx_gpio_update_int_params(epg, port);
> +       eic->int_unmasked |= BIT(d->irq & 7);
> +       ep93xx_gpio_update_int_params(epg, eic);
>  }
>  
>  /*
> @@ -209,8 +212,8 @@ static void ep93xx_gpio_irq_unmask(struct irq_data *d)
>  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_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>         struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -       int port = ep93xx_gpio_port(gc);
>         int offset = d->irq & 7;
>         int port_mask = BIT(offset);
>         irq_flow_handler_t handler;
> @@ -219,32 +222,32 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
>  
>         switch (type) {
>         case IRQ_TYPE_EDGE_RISING:
> -               gpio_int_type1[port] |= port_mask;
> -               gpio_int_type2[port] |= port_mask;
> +               eic->int_type1 |= port_mask;
> +               eic->int_type2 |= port_mask;
>                 handler = handle_edge_irq;
>                 break;
>         case IRQ_TYPE_EDGE_FALLING:
> -               gpio_int_type1[port] |= port_mask;
> -               gpio_int_type2[port] &= ~port_mask;
> +               eic->int_type1 |= port_mask;
> +               eic->int_type2 &= ~port_mask;
>                 handler = handle_edge_irq;
>                 break;
>         case IRQ_TYPE_LEVEL_HIGH:
> -               gpio_int_type1[port] &= ~port_mask;
> -               gpio_int_type2[port] |= port_mask;
> +               eic->int_type1 &= ~port_mask;
> +               eic->int_type2 |= port_mask;
>                 handler = handle_level_irq;
>                 break;
>         case IRQ_TYPE_LEVEL_LOW:
> -               gpio_int_type1[port] &= ~port_mask;
> -               gpio_int_type2[port] &= ~port_mask;
> +               eic->int_type1 &= ~port_mask;
> +               eic->int_type2 &= ~port_mask;
>                 handler = handle_level_irq;
>                 break;
>         case IRQ_TYPE_EDGE_BOTH:
> -               gpio_int_type1[port] |= port_mask;
> +               eic->int_type1 |= port_mask;
>                 /* set initial polarity based on current input level */
>                 if (gc->get(gc, offset))
> -                       gpio_int_type2[port] &= ~port_mask; /* falling */
> +                       eic->int_type2 &= ~port_mask; /* falling */
>                 else
> -                       gpio_int_type2[port] |= port_mask; /* rising */
> +                       eic->int_type2 |= port_mask; /* rising */
>                 handler = handle_edge_irq;
>                 break;
>         default:
> @@ -253,9 +256,9 @@ static int ep93xx_gpio_irq_type(struct irq_data *d, unsigned int type)
>  
>         irq_set_handler_locked(d, handler);
>  
> -       gpio_int_enabled[port] |= port_mask;
> +       eic->int_enabled |= port_mask;
>  
> -       ep93xx_gpio_update_int_params(epg, port);
> +       ep93xx_gpio_update_int_params(epg, eic);
>  
>         return 0;
>  }
> @@ -276,17 +279,19 @@ struct ep93xx_gpio_bank {
>         const char      *label;
>         int             data;
>         int             dir;
> +       int             irq;
>         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(_label, _data, _dir, _irq, _base, _has_irq, _has_hier, _irq_base) \
>         {                                                       \
>                 .label          = _label,                       \
>                 .data           = _data,                        \
>                 .dir            = _dir,                         \
> +               .irq            = _irq,                         \
>                 .base           = _base,                        \
>                 .has_irq        = _has_irq,                     \
>                 .has_hierarchical_irq = _has_hier,              \
> @@ -295,16 +300,16 @@ 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, 0x90, 0, true, false, 64),
>         /* Bank B has 8 IRQs */
> -       EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
> -       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("B", 0x04, 0x14, 0xac, 8, true, false, 72),
> +       EP93XX_GPIO_BANK("C", 0x08, 0x18, 0x00, 40, false, false, 0),
> +       EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 0x00, 24, false, false, 0),
> +       EP93XX_GPIO_BANK("E", 0x20, 0x24, 0x00, 32, false, false, 0),
>         /* Bank F has 8 IRQs */
> -       EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
> -       EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
> -       EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
> +       EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, 0),
> +       EP93XX_GPIO_BANK("G", 0x38, 0x3c, 0x00, 48, false, false, 0),
> +       EP93XX_GPIO_BANK("H", 0x40, 0x44, 0x00, 56, false, false, 0),
>  };
>  
>  static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
> @@ -326,13 +331,14 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset)
>         return EP93XX_GPIO_F_IRQ_BASE + offset;
>  }
>  
> -static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
> +static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
>                                 struct platform_device *pdev,
>                                 struct ep93xx_gpio *epg,
>                                 struct ep93xx_gpio_bank *bank)
>  {
>         void __iomem *data = epg->base + bank->data;
>         void __iomem *dir = epg->base + bank->dir;
> +       struct gpio_chip *gc = &egc->gc;
>         struct device *dev = &pdev->dev;
>         struct gpio_irq_chip *girq;
>         int err;
> @@ -347,6 +353,12 @@ 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;
> +               egc->eic = devm_kcalloc(dev, 1,
> +                                       sizeof(*egc->eic),
> +                                       GFP_KERNEL);
> +               if (!egc->eic)
> +                       return -ENOMEM;
> +               egc->eic->irq_offset = bank->irq;
>                 girq->chip = &ep93xx_gpio_irq_chip;
>         }
>  
> @@ -415,7 +427,7 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
>                 return PTR_ERR(epg->base);
>  
>         for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
> -               struct gpio_chip *gc = &epg->gc[i];
> +               struct ep93xx_gpio_chip *gc = &epg->gc[i];
>                 struct ep93xx_gpio_bank *bank = &ep93xx_gpio_banks[i];
>  
>                 if (ep93xx_gpio_add_bank(gc, pdev, epg, bank))

-- 
Alexander Sverdlin.

[PATCH v5 2/7] 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

Fixes: d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip")
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
v4->v5:
- generate IRQ chip's names dynamicaly from label
---
 drivers/gpio/gpio-ep93xx.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 64d6c2b4282e..3d8eb8769470 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -38,6 +38,7 @@
 #define EP93XX_GPIO_F_IRQ_BASE 80
 
 struct ep93xx_gpio_irq_chip {
+	struct irq_chip ic;
 	u8 irq_offset;
 	u8 int_unmasked;
 	u8 int_enabled;
@@ -331,6 +332,16 @@ 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_chip(struct device *dev, struct irq_chip *ic, const char *label)
+{
+	ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", label);
+	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 ep93xx_gpio_chip *egc,
 				struct platform_device *pdev,
 				struct ep93xx_gpio *epg,
@@ -352,6 +363,8 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 
 	girq = &gc->irq;
 	if (bank->has_irq || bank->has_hierarchical_irq) {
+		struct irq_chip *ic;
+
 		gc->set_config = ep93xx_gpio_set_config;
 		egc->eic = devm_kcalloc(dev, 1,
 					sizeof(*egc->eic),
@@ -359,7 +372,9 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 		if (!egc->eic)
 			return -ENOMEM;
 		egc->eic->irq_offset = bank->irq;
-		girq->chip = &ep93xx_gpio_irq_chip;
+		ic = &egc->eic->ic;
+		ep93xx_init_irq_chip(dev, ic, bank->label);
+		girq->chip = ic;
 	}
 
 	if (bank->has_irq) {
-- 
2.26.2

Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Andy Shevchenko]

On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin <nikita.shubin@maquefel.me> 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

...

> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip *ic, const char *label)
> +{

> +       ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", label);

Is the label being NULL okay?

> +       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;
> +}

...

> -               girq->chip = &ep93xx_gpio_irq_chip;

I don't see where you remove that static structure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Nikita Shubin]

Hello Andy.

On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote:
>On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin 
<nikita.shubin@maquefel.me> 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
>
>...
>
>> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip
>> *ic, const char *label) +{
>> 
>> +       ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s",
>> label);
>Is the label being NULL okay?

The label is taken from ep93xx_gpio_banks[], so unless we explicitly 
pass zero to ep93xx_init_irq_chip(), we are ok.

>
>> +       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;
>> +}
>
>...
>
>> -               girq->chip = &ep93xx_gpio_irq_chip;
>
>I don't see where you remove that static structure.

Good catch - thank you very much, also i noticed that i forgot to switch 
IRQ chip in irq_set_chip_and_handler() for port F.

Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Andy Shevchenko]

On Tue, Feb 9, 2021 at 2:35 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote:
> >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:

...

> >> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip
> >> *ic, const char *label) +{
> >>
> >> +       ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s",
> >> label);
> >Is the label being NULL okay?
>
> The label is taken from ep93xx_gpio_banks[], so unless we explicitly
> pass zero to ep93xx_init_irq_chip(), we are ok.

Maybe I was unclear, let me rephrase: Is the *resulting* label being NULL okay?

> >> +       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;
> >> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Nikita Shubin]

On Tuesday, 9 February 2021 15:46:19 MSK Andy Shevchenko wrote:
>On Tue, Feb 9, 2021 at 2:35 PM Nikita Shubin 
<nikita.shubin@maquefel.me> wrote:
>> On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote:
>> >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin
>> 
>> <nikita.shubin@maquefel.me> wrote:
>...
>
>> >> +static void ep93xx_init_irq_chip(struct device *dev, struct
>> >> irq_chip
>> >> *ic, const char *label) +{
>> >> 
>> >> +       ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s",
>> >> label);
>> >
>> >Is the label being NULL okay?

You mean ENOMEM should be honored ? I think you are right about it.

>> 
>> The label is taken from ep93xx_gpio_banks[], so unless we explicitly
>> pass zero to ep93xx_init_irq_chip(), we are ok.
>
>Maybe I was unclear, let me rephrase: Is the *resulting* label being
>NULL okay?
>> >> +       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;
>> >> +}

Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Andy Shevchenko]

On Tue, Feb 9, 2021 at 2:54 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> On Tuesday, 9 February 2021 15:46:19 MSK Andy Shevchenko wrote:
> >On Tue, Feb 9, 2021 at 2:35 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >> On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote:
> >> >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin
> >> <nikita.shubin@maquefel.me> wrote:

...

> >> >> +       ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s",
> >> >> label);
> >> >
> >> >Is the label being NULL okay?
>
> You mean ENOMEM should be honored ? I think you are right about it.

Depending on what is the answer to the question below. If NULL label
is okay (and here is just optional) then simply comment it in the
code, otherwise check is missed.

> >> The label is taken from ep93xx_gpio_banks[], so unless we explicitly
> >> pass zero to ep93xx_init_irq_chip(), we are ok.
> >
> >Maybe I was unclear, let me rephrase: Is the *resulting* label being
> >NULL okay?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips

[Alexander Sverdlin]

Hello Nikita,

On Mon, 2021-02-08 at 11:59 +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
> 
> Fixes: d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip")
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

I performed a bootup with the whole patch-series and
confirm that the warning is gone. Thank you for looking into this!

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

> ---
> v4->v5:
> - generate IRQ chip's names dynamicaly from label
> ---
>  drivers/gpio/gpio-ep93xx.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 64d6c2b4282e..3d8eb8769470 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -38,6 +38,7 @@
>  #define EP93XX_GPIO_F_IRQ_BASE 80
>  
>  struct ep93xx_gpio_irq_chip {
> +       struct irq_chip ic;
>         u8 irq_offset;
>         u8 int_unmasked;
>         u8 int_enabled;
> @@ -331,6 +332,16 @@ 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_chip(struct device *dev, struct irq_chip *ic, const char *label)
> +{
> +       ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", label);
> +       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 ep93xx_gpio_chip *egc,
>                                 struct platform_device *pdev,
>                                 struct ep93xx_gpio *epg,
> @@ -352,6 +363,8 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
>  
>         girq = &gc->irq;
>         if (bank->has_irq || bank->has_hierarchical_irq) {
> +               struct irq_chip *ic;
> +
>                 gc->set_config = ep93xx_gpio_set_config;
>                 egc->eic = devm_kcalloc(dev, 1,
>                                         sizeof(*egc->eic),
> @@ -359,7 +372,9 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
>                 if (!egc->eic)
>                         return -ENOMEM;
>                 egc->eic->irq_offset = bank->irq;
> -               girq->chip = &ep93xx_gpio_irq_chip;
> +               ic = &egc->eic->ic;
> +               ep93xx_init_irq_chip(dev, ic, bank->label);
> +               girq->chip = ic;
>         }
>  
>         if (bank->has_irq) {

-- 
Alexander Sverdlin.

[PATCH v5 3/7] 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
IRQ's get mapped dynamically on the fly"

And that's not the thing we want.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
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 3d8eb8769470..942da366220a 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -423,6 +423,7 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 		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.26.2

[PATCH v5 4/7] 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.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
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 942da366220a..f8b21e1d55ed 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -327,11 +327,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_chip(struct device *dev, struct irq_chip *ic, const char *label)
 {
 	ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", label);
@@ -422,7 +417,6 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 		}
 		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.26.2

[PATCH v5 5/7] gpio: ep93xx: Fix typo s/hierarchial/hierarchical

[Nikita Shubin]

Fix typo in comment.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
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 f8b21e1d55ed..9ac8a8b830c1 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -395,7 +395,7 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 
 		/*
 		 * 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.26.2

[PATCH v5 6/7] gpio: ep93xx: refactor ep93xx_gpio_add_bank

[Nikita Shubin]

- replace plain numbers with girq->num_parents in devm_kcalloc
- replace plain numbers with girq->num_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

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
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 9ac8a8b830c1..e75f7a9e40a0 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -377,7 +377,7 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 
 		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)
@@ -399,15 +399,14 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 		 */
 		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 < girq->num_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.26.2

[PATCH v5 7/7] 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

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/gpio/gpio-ep93xx.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index e75f7a9e40a0..9cc2c2b4309f 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -31,6 +31,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.
@@ -301,14 +303,14 @@ struct ep93xx_gpio_bank {
 
 static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
 	/* Bank A has 8 IRQs */
-	EP93XX_GPIO_BANK("A", 0x00, 0x10, 0x90, 0, true, false, 64),
+	EP93XX_GPIO_BANK("A", 0x00, 0x10, 0x90, 0, true, false, EP93XX_GPIO_A_IRQ_BASE),
 	/* Bank B has 8 IRQs */
-	EP93XX_GPIO_BANK("B", 0x04, 0x14, 0xac, 8, true, false, 72),
+	EP93XX_GPIO_BANK("B", 0x04, 0x14, 0xac, 8, true, false, EP93XX_GPIO_B_IRQ_BASE),
 	EP93XX_GPIO_BANK("C", 0x08, 0x18, 0x00, 40, false, false, 0),
 	EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 0x00, 24, false, false, 0),
 	EP93XX_GPIO_BANK("E", 0x20, 0x24, 0x00, 32, false, false, 0),
 	/* Bank F has 8 IRQs */
-	EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, 0),
+	EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, EP93XX_GPIO_F_IRQ_BASE),
 	EP93XX_GPIO_BANK("G", 0x38, 0x3c, 0x00, 48, false, false, 0),
 	EP93XX_GPIO_BANK("H", 0x40, 0x44, 0x00, 56, false, false, 0),
 };
@@ -407,7 +409,7 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 		/* Pick resources 1..8 for these IRQs */
 		for (i = 0; i < girq->num_parents; i++) {
 			girq->parents[i] = platform_get_irq(pdev, i + 1);
-			gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
+			gpio_irq = bank->irq_base + i;
 			irq_set_chip_data(gpio_irq, &epg->gc[5]);
 			irq_set_chip_and_handler(gpio_irq,
 						 &ep93xx_gpio_irq_chip,
@@ -416,7 +418,7 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
 		}
 		girq->default_type = IRQ_TYPE_NONE;
 		girq->handler = handle_level_irq;
-		girq->first = EP93XX_GPIO_F_IRQ_BASE;
+		girq->first = bank->irq_base;
 	}
 
 	return devm_gpiochip_add_data(dev, gc, epg);
-- 
2.26.2