lkml.org 
[lkml]   [2016]   [Jan]   [6]   [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
Hi Boris,

Thanks a lot for your comments.

On 01/04/2016 06:02 PM, Boris Brezillon wrote:
> 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?

During my driver development on Atmel boards, I just found major
difference between two IRQ chips is how to select HW IRQ number. Other
parts could be merged into single driver like OMAP.

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

My apologies for this regression. I should check your log carefully.
Thanks for pointing this out.
I'm just thinking about boot code modification for this. How about
supporting RTC/RTT fixup code in at91bootstrap?

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

Yeap, thanks for your time. My idea is simple.

"Different IRQ chip operation can be consolidated if simple data
structure is used."

If AIC and AIC5 must be separate, then I'll try to rework
irq-atmel-common part.

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

Let me check it later if I could have a chance to create the 2nd patch ;)

Best regards,
Milo


\
 
 \ /
  Last update: 2016-01-06 09:41    [W:0.145 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site