On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:

GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
top of a GPIO chip. It's better from maintainability and readability
point of view to use one instead of supporting a hand-written Generic
IRQ-chip-based implementation. Moreover the new implementation won't
cause much functional overhead but will provide a cleaner driver code.
All of that makes the DW APB GPIO driver conversion pretty much justified
especially seeing a tendency of the other GPIO drivers getting converted
too.

Here is what we do in the framework of this commit to convert the driver
to using the GPIO-lib-based IRQ-chip interface:
1) IRQ ack, mask and unmask callbacks are locally defined instead of
using the Generic IRQ-chip ones.
2) An irq_chip structure instance is embedded into the dwapb_gpio
private data. Note we can't have a static instance of that structure since
GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks().
A warning about that would have been printed by the GPIO-lib code if we
used a single irq_chip structure instance for multiple DW APB GPIO
controllers.
3) Initialize the gpio_irq_chip structure embedded into the gpio_chip
descriptor. By default there is no IRQ enabled so any event raised will be
handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core
is synthesized to have non-shared reference IRQ-lines, then as before the
hierarchical and cascaded cases are distinguished by checking how many
parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't
initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP
is used on a platform with shared IRQ line, then we simply won't let the
GPIO-lib to initialize the parental IRQs, but will handle them locally in
the driver.
4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since
GPIO-lib IRQ-chip interface will create a new domain and accept a standard
IRQ-chip structure pointer based on the setting we provided in the
gpio_irq_chip structure.
5) Manually select a proper IRQ flow handler directly in the
irq_set_type() callback by calling irq_set_handler_locked() method, since
an ordinary (not Generic) irq_chip descriptor is now utilized.

Can you also emphasize that this make no regression to the 6a2f4b7dadd5 ("gpio:
dwapb: use a second irq chip")?

(And I hope you have means to test that scenario, because in my case I have
 only one IRQ and it's actually as input from other GPIO IRQ chip).

6) Discard the custom GPIO-to-IRQ mapping function since GPIO-lib defines
the standard method gpiochip_to_irq(), which will be used anyway no matter
whether the custom to_irq callback is specified or not.
7) Discard the acpi_gpiochip_{request,free}_interrupts()
invocations, since they will be called from
gpiochip_add_irqchip()/gpiochip_irqchip_remove() anyway.
8) Alter CONFIG_GPIO_DWAPB kernel config to select
CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.

I like the idea, but is it possible to split this?

...

Can we rather do like other GPIO IRQ chip implementations are doing, i.e.
instead of repeating same handler in each branch, use one conditional:

	if (type & IRQ_TYPE_LEVEL_MASK) {
		...
		irq_set_handler_locked(d, handle_level_irq);
	} else if (type & IRQ_TYPE_EDGE_BOTH) {
		...
		irq_set_handler_locked(d, handle_edge_irq);
	}

?

...

+		/*
+		 * If more than one IRQ line is specified then try to
+		 * initialize the hierarchical interrupts. Otherwise it's
+		 * a simple cascaded case with a common IRQ signal.
+		 */
+		girq->num_parents = pp->irq[1] ? pp->ngpio : 1;

Can it be sparse in the array? (It's actually the main point why I went with
memchr_inv() instead of doing something like above)

+		girq->parents = pp->irq;
+		girq->parent_handler_data = gpio;
+		girq->parent_handler = dwapb_irq_handler;

...

+ blank line.

+		/* This will let us handle the parent IRQ in the driver */
+		girq->parents = NULL;
+		girq->num_parents = 0;
+		girq->parent_handler = NULL;

Shan't we do this before request_irq() call (at least for consistency with the
rest of the drivers)?

-- 
With Best Regards,
Andy Shevchenko