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