[v2,05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

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. Note this
shalln't give any regression
6) Alter CONFIG_GPIO_DWAPB kernel config to select
CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.

Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5
("gpio: dwapb: use a second irq chip"), since the later isn't properly
used here anyway.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---

Note in this patch we omit the next GPIO-lib IRQ-chip settings
initialization:
gc->irq.map
gc->irq.init_valid_mask

That's intentionally done in order to keep the driver functionality at the
same level as before the conversion: each GPIO line will be registered as
the source of IRQs, no matter whether DW APB GPIO IP is configured to
generate combined or multiple IRQ signals.

If more thorough mapping is needed, the driver should be altered to detect
the IRQ signaling mode (for instance by checking a dedicated DT-property).
Then based on that it will be able to create a proper hardware GPIO-IRQ
to/from parental IRQs mapping and valid hardware GPIO-IRQs mask.

Changelog v2:
- Replace gc->to_irq() with irq_find_mapping() method.
- Refactor dwapb_irq_set_type() method to directly set IRQ flow handler
  instead of using a temporary variable.
- Initialize GPIOlib IRQ-chip settings before calling request_irq()
  method.
- Add a notice regarding regression of commit 6a2f4b7dadd5 ("gpio:
dwapb: use a second irq chip").
- Move the acpi_gpiochip_{request,free}_interrupts() methods invocation
  removal to a dedicated commit.
- Move GPIO-chip to_irq callback removal to a dedicated commit.
- Introduce dwapb_convert_irqs() method to convert the sparse parental
  IRQs array into an array of linearly distributed IRQs correctly
  perceived by GPIO-lib.
---
 drivers/gpio/Kconfig      |   2 +-
 drivers/gpio/gpio-dwapb.c | 191 +++++++++++++++++++-------------------
 2 files changed, 98 insertions(+), 95 deletions(-)

On Thu, Jul 30, 2020 at 04:55:31PM +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.

Easy to read if you put blank lines in between of items.

> 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. Note this
> shalln't give any regression
> 6) Alter CONFIG_GPIO_DWAPB kernel config to select
> CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.
> 
> Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5
> ("gpio: dwapb: use a second irq chip"), since the later isn't properly
> used here anyway.

...

>  struct dwapb_gpio_port {
>  	struct gpio_chip	gc;
> +	unsigned int		nr_irqs;
> +	unsigned int		irq[DWAPB_MAX_GPIOS];
> +	struct irq_chip		irqchip;
>  	bool			is_registered;
>  	struct dwapb_gpio	*gpio;

Isn't it too much wasted memory (imagine 4 port controller)?

What if we have it in another structure and allocate dynamically?

struct dwapb_gpio_port_irqchip {
	struct irq_chip		irqchip;
	unsigned int		nr_irqs;
	unsigned int		irq[DWAPB_MAX_GPIOS];
};

	...
	struct dwapb_gpio_port_irqchip *pirq;
	...

(I agree that IRQ chip is rather property of a port than controller)

On Thu, Jul 30, 2020 at 05:26:18PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 30, 2020 at 04:55:31PM +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.
> 

> Easy to read if you put blank lines in between of items.

Ok.

> 
> > 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. Note this
> > shalln't give any regression
> > 6) Alter CONFIG_GPIO_DWAPB kernel config to select
> > CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.
> > 
> > Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5
> > ("gpio: dwapb: use a second irq chip"), since the later isn't properly
> > used here anyway.
> 
> ...
> 
> >  struct dwapb_gpio_port {
> >  	struct gpio_chip	gc;
> > +	unsigned int		nr_irqs;
> > +	unsigned int		irq[DWAPB_MAX_GPIOS];
> > +	struct irq_chip		irqchip;
> >  	bool			is_registered;
> >  	struct dwapb_gpio	*gpio;
> 

> Isn't it too much wasted memory (imagine 4 port controller)?
> 
> What if we have it in another structure and allocate dynamically?
> 
> struct dwapb_gpio_port_irqchip {
> 	struct irq_chip		irqchip;
> 	unsigned int		nr_irqs;
> 	unsigned int		irq[DWAPB_MAX_GPIOS];
> };

Agree. I have to send a new revision of the series anyway. I'll do that shortly.

-Sergey

> 
> 	...
> 	struct dwapb_gpio_port_irqchip *pirq;
> 	...
> 
> (I agree that IRQ chip is rather property of a port than controller)
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>