On Thu, Jul 20, 2023 at 02:29:01PM +0300, Nikita Shubin via B4 Relay wrote:

From: Nikita Shubin <nikita.shubin@maquefel.me>

This prepares ep93xx SOC gpio to convert into device tree driver:
- dropped banks and legacy defines
- split AB IRQ and make it shared

We are relying on IRQ number information A, B ports have single shared
IRQ, while F port have dedicated IRQ for each line.

Also we had to split single ep93xx platform_device into multiple, one
for each port, without this we can't do a full working transition from
legacy platform code into device tree capable. All GPIO_LOOKUP were
change to match new chip namings.

...

-static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
+static u32 ep93xx_gpio_ab_irq_handler(struct gpio_chip *gc)
 {
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
-	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
 	unsigned long stat;
 	int offset;
 
-	chained_irq_enter(irqchip, desc);
-
-	/*
-	 * Dispatch the IRQs to the irqdomain of each A and B
-	 * gpiochip irqdomains depending on what has fired.
-	 * The tricky part is that the IRQ line is shared
-	 * between bank A and B and each has their own gpiochip.
-	 */
-	stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS);
+	stat = readb(eic->base + EP93XX_INT_STATUS_OFFSET);
 	for_each_set_bit(offset, &stat, 8)
-		generic_handle_domain_irq(epg->gc[0].gc.irq.domain,
-					  offset);
+		generic_handle_domain_irq(gc->irq.domain, offset);
 
-	stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS);
-	for_each_set_bit(offset, &stat, 8)
-		generic_handle_domain_irq(epg->gc[1].gc.irq.domain,
-					  offset);
+	return stat;
+}
 
-	chained_irq_exit(irqchip, desc);
+static irqreturn_t ep93xx_ab_irq_handler(int irq, void *dev_id)
+{
+	return IRQ_RETVAL(ep93xx_gpio_ab_irq_handler(dev_id));
 }
 
 static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
 {
-	/*
-	 * map discontiguous hw irq range to continuous sw irq range:
-	 *
-	 *  IRQ_EP93XX_GPIO{0..7}MUX -> EP93XX_GPIO_LINE_F{0..7}
-	 */
 	struct irq_chip *irqchip = irq_desc_get_chip(desc);
-	unsigned int irq = irq_desc_get_irq(desc);
-	int port_f_idx = (irq & 7) ^ 4; /* {20..23,48..51} -> {0..7} */
-	int gpio_irq = EP93XX_GPIO_F_IRQ_BASE + port_f_idx;
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct gpio_irq_chip *gic = &gc->irq;
+	unsigned int parent = irq_desc_get_irq(desc);
+	unsigned int i;
 
 	chained_irq_enter(irqchip, desc);
-	generic_handle_irq(gpio_irq);
+	for (i = 0; i < gic->num_parents; i++)
+		if (gic->parents[i] == parent)
+			break;
+
+	if (i < gic->num_parents)
+		generic_handle_irq(irq_find_mapping(gc->irq.domain, i));

Can we use

		generic_handle_domain_irq(gc->irq.domain, i);

here as well?

 	chained_irq_exit(irqchip, desc);
 }

...

-	int offset = d->irq & 7;
+	int offset = irqd_to_hwirq(d);

	irq_hw_number_t ?

 	irq_flow_handler_t handler;

...

+	int ret, irq, i = 0;

What do you need this assignment for?

...

+		ret = devm_request_irq(dev, irq,
+				ep93xx_ab_irq_handler,

It can be located on the previous line.

+				IRQF_SHARED, gc->label, gc);
+		if (ret)
+			return dev_err_probe(dev, ret, "error requesting IRQ : %d\n", irq);

Drop duplicating word 'error' in the message.
Space is not needed before colon.

...

+	/* TODO: replace with handle_bad_irq once we are fully hierarchical */

To be pedantic: handle_bad_irq()

+	gc->label = dev_name(&pdev->dev);
+	if (platform_irq_count(pdev) > 0) {
+		dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev));
+		ret = ep93xx_setup_irqs(pdev, egc);
+		if (ret)

+			dev_err(&pdev->dev, "setup irqs failed for %s\n", dev_name(&pdev->dev));

What's the point to print dev name twice? Esp. taking into account
gc->label assignment above. Why not use dev_err_probe() to unify
the format of the messages from ->probe()?

-- 
With Best Regards,
Andy Shevchenko