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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Milo,

On Wed, 6 Jan 2016 16:48:23 +0900
Milo Kim <milo.kim@ti.com> wrote:

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

Except that this major difference is a central aspect, and if you look
at your changes, you'll see that you're introducing
'if (aic_is_ssr_used())' statements in pretty much all irqchip
callbacks.
As I said, I'm not against code factorization, but it's not really
one to me, because you're adding extra conditional path all over the
code to differentiate the two chips, which means those are not so
similar.

Could you list the common aspects shared by those two controllers?

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

Nope, because at91bootstrap is not mandatory, you can use u-boot's
or barebox's SPL, or even develop your own bootstrap code. Not to
mention that kexeced kernels can also experience some bugs. It should
not happen if all drivers implement a ->shutdown() function disabling
theie interrupt, but I'm pretty sure that's not currently the case.
Anyway, we have to support existing systems, so removing those fixup is
simply not an option.

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

As pointed, I don't think that's a good idea, but let's see what others
say.
Thomas, Jason, any comments?

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

What would you like to rework? Could you describe in more details
what's bothering you in this code?

Also, you can join #at91 on IRC (freenode server) if you want to
discuss that in a more interactive way.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com