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

[Nicolas Ferre <nicolas.ferre@atmel.com>]

Le 04/01/2016 10:02, Boris Brezillon a écrit :
> 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?

I was about to ask the same question.

I'm really sorry that you spent so much time and effort for something
that I'm absolutely against: reworking stable code that had been
maturing for years and that is now nicely stable... for no obvious reason...

So unless there is a very strong benefit, it's a pretty firm NACK for
this series. We will certainly take some time to review the bits that
can be taken further to enhance our current drivers.

I also advice you to talk to us before starting such a big project and
you'll notice that we are open to changes and have many aspect that you
can work on with AT91.

Bye,


>> 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
>>
> 
> 
> 


-- 
Nicolas Ferre