lkml.org 
[lkml]   [2016]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver
From
Date
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


\
 
 \ /
  Last update: 2016-01-04 11:21    [W:0.146 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site