Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Milo,

On Mon, 4 Jan 2016 13:28:24 +0900
Milo Kim <milo.kim@ti.com> wrote:

> This patch-set provides unified Atmel AIC (Advanced Interrupt Controller)
> driver. Currently, there are two AIC drivers, AIC and AIC5.
> Each driver consists of chip specific part (irq-atmel-aic.o or
> irq-atmel-aic5.o) and shared code (irq-atmel-aic-common.o).
> But consolidated AIC driver is just one file driver which supports both
> IRQ chip systems.

Sorry, but what's the real motivation behind this rework?

> 
> How to handle two IRQ chips in one driver
> -----------------------------------------
>   Structure aic_reg_offset is used for device configuration.
>   AIC5 IRQ chip uses SSR (Source Select Register) to select IRQ number.
>   On the other hand, AIC IRQ chip has simple register access.
>   To support both IRQ chips, aic_is_ssr_used() helper is used.
> 
> Patches
> -------
>   1 ~  5: fix IRQ priority issue, clean up RTC/RTT fixup code and etc.

As explained in my review, those irq fixup are essential, and cannot
remove them.

>   6 ~ 19: create unified IRQ chip operation with aic_reg_offset data.

I started to review those patches, but honestly I don't see the point of
this rework, since you're trying to merge drivers for 2 IPs that are
completely different from a functional POV (except for a few tiny things
like priority or irq type definition).

Before reviewing the remaining patches, I'd like to know more about your
real motivations for pushing those changes?

> 
> Target boards
> -------------
>   Tested with two boards.
>   * Arietta G25 (SoC: AT91SAM9G25)
>   * Xplained board (SoC: SAMA5D3)
> 
> Number of driver files
> ----------------------
>   AIC:              3 (irq-atmel-aic.c, irq-atmel-aic-common.c and h)
>   AIC5:             3 (irq-atmel-aic5.c, irq-atmel-aic-common.c and h)
>   Consolidated AIC: 1 (irq-aic.c)
> 
> Code size
> ---------
>   AIC (irq-atmel-aic.o and irq-atmel-aic-common.o)
>     text	   data	    bss	    dec	    hex	filename
>     5137	    196	      4	   5337	   14d9	drivers/irqchip/built-in.o
> 
>   AIC5 (irq-atmel-aic5.o and irq-atmel-aic-common.o)
>     text	   data	    bss	    dec	    hex	filename
>     5548	    196	      4	   5748	   1674	drivers/irqchip/built-in.o
> 
>   Consolidated AIC (irq-aic.o)
>     text	   data	    bss	    dec	    hex	filename
>     4841	    196	      8	   5045	   13b5	drivers/irqchip/built-in.o
> 
> Lines of code
> -------------
>   AIC:              597
>   AIC5:             688
>   Consolidated AIC: 609


Please, redo the same thing, but after keeping the IRQ fixup stuff, and
I'm pretty sure the text section of the AIC/AIC5 and the consolidated
version will be much closer.

Not to mention that you're adding a bunch of if () statements in the
critical IRQ path, which is never a good think for latency concern.

Best Regards,

Boris

> 
> Milo Kim (19):
>   irqchip: atmel-aic: fix wrong bit operation for IRQ priority
>   irqchip: atmel-aic: clean up RTC interrupt code
>   irqchip: atmel-aic: clean up RTT interrupt code
>   irqchip: atmel-aic: replace magic numbers with named constant
>   irqchip: atmel-aic: use simple constant to get number of interrupts
>     per chip
>   irqchip: atmel-aic: introduce register data structure
>   irqchip: atmel-aic: make common IRQ domain translate function
>   irqchip: atmel-aic: add common mask and unmask functions
>   irqchip: atmel-aic: add common retrigger function
>   irqchip: atmel-aic: add common set_type function
>   irqchip: atmel-aic: add common PM IRQ chip operation
>   irqchip: atmel-aic: use EOI register data in aic_reg_data
>   irqchip: atmel-aic: clean up irq_chip_generic
>   irqchip: atmel-aic: add common HW init function
>   irqchip: atmel-aic: add common interrupt handler
>   irqchip: atmel-aic: get total number of IRQs from device node
>   irqchip: atmel-aic: use unified IRQ chip initialization function
>   irqchip: atmel-aic: use unified AIC driver
>   irqchip: atmel-aic: rename AIC driver and fix Kconfig
> 
>  arch/arm/mach-at91/Kconfig             |   2 +-
>  drivers/irqchip/Kconfig                |   7 -
>  drivers/irqchip/Makefile               |   3 +-
>  drivers/irqchip/irq-aic.c              | 609 +++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-atmel-aic-common.c | 280 ---------------
>  drivers/irqchip/irq-atmel-aic-common.h |  41 ---
>  drivers/irqchip/irq-atmel-aic.c        | 276 ---------------
>  drivers/irqchip/irq-atmel-aic5.c       | 367 --------------------
>  8 files changed, 611 insertions(+), 974 deletions(-)
>  create mode 100644 drivers/irqchip/irq-aic.c
>  delete mode 100644 drivers/irqchip/irq-atmel-aic-common.c
>  delete mode 100644 drivers/irqchip/irq-atmel-aic-common.h
>  delete mode 100644 drivers/irqchip/irq-atmel-aic.c
>  delete mode 100644 drivers/irqchip/irq-atmel-aic5.c
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com