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

From: Boris Brezillon
Date: Mon Jan 04 2016 - 04:02:48 EST


Hi Milo,

On Mon, 4 Jan 2016 13:28:24 +0900
Milo Kim <milo.kim@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/