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

From	Boris Brezillon <boris.brezillon@free-electrons.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver
Date	2016-01-04 10:10 +0100
Message-ID	<qN9jk-1sI-7@gated-at.bofh.it> (permalink)
References	<qN4Wl-6EU-3@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Thread

[PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 07/19] irqchip: atmel-aic: make common IRQ domain translate function Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 09/19] irqchip: atmel-aic: add common retrigger function Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 13/19] irqchip: atmel-aic: clean up irq_chip_generic Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 01/19] irqchip: atmel-aic: fix wrong bit operation for IRQ priority Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
    Re: [PATCH 01/19] irqchip: atmel-aic: fix wrong bit operation for  IRQ priority Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 09:20 +0100
  [PATCH 17/19] irqchip: atmel-aic: use unified IRQ chip initialization function Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 06/19] irqchip: atmel-aic: introduce register data structure Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
    Re: [PATCH 06/19] irqchip: atmel-aic: introduce register data  structure Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 10:00 +0100
      Re: [PATCH 06/19] irqchip: atmel-aic: introduce register data  structure Milo Kim <milo.kim@ti.com> - 2016-01-06 09:40 +0100
  [PATCH 04/19] irqchip: atmel-aic: replace magic numbers with named constant Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
    Re: [PATCH 04/19] irqchip: atmel-aic: replace magic numbers with  named constant Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 09:30 +0100
  [PATCH 19/19] irqchip: atmel-aic: rename AIC driver and fix Kconfig Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 11/19] irqchip: atmel-aic: add common PM IRQ chip operation Milo Kim <milo.kim@ti.com> - 2016-01-04 05:30 +0100
  [PATCH 12/19] irqchip: atmel-aic: use EOI register data in aic_reg_data Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
  [PATCH 14/19] irqchip: atmel-aic: add common HW init function Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
  [PATCH 10/19] irqchip: atmel-aic: add common set_type function Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
  [PATCH 08/19] irqchip: atmel-aic: add common mask and unmask functions Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
    Re: [PATCH 08/19] irqchip: atmel-aic: add common mask and unmask  functions Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 09:50 +0100
  [PATCH 02/19] irqchip: atmel-aic: clean up RTC interrupt code Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
    Re: [PATCH 02/19] irqchip: atmel-aic: clean up RTC interrupt code Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 09:20 +0100
  [PATCH 05/19] irqchip: atmel-aic: use simple constant to get number of interrupts per chip Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
    Re: [PATCH 05/19] irqchip: atmel-aic: use simple constant to get  number of interrupts per chip Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 09:40 +0100
  [PATCH 03/19] irqchip: atmel-aic: clean up RTT interrupt code Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
    Re: [PATCH 03/19] irqchip: atmel-aic: clean up RTT interrupt code Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 09:20 +0100
  [PATCH 15/19] irqchip: atmel-aic: add common interrupt handler Milo Kim <milo.kim@ti.com> - 2016-01-04 05:40 +0100
  Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-04 10:10 +0100
    Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-01-04 10:40 +0100
      Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Milo Kim <milo.kim@ti.com> - 2016-01-06 09:00 +0100
    Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Milo Kim <milo.kim@ti.com> - 2016-01-06 08:50 +0100
      Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-06 10:10 +0100
        Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Jason Cooper <jason@lakedaemon.net> - 2016-01-06 15:50 +0100
        Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Milo Kim <milo.kim@ti.com> - 2016-01-07 08:50 +0100

csiph-web