Hello. Kevin Hilman wrote:
Add support for Texas Instuments Common Platform Interrupt Controller (cp_intc).
It's being added as a part of DA830/OMAP-L137 support, however as cp_intc is not really specific to this architecture (or even ARM specific), I thought that the best place for this code will be in arch/arm/common/...
Are there other known, or upcoming users this block outside the
TI Puma 5 ARM SoC, used in the Avalanche family of the broadband processors (it even seems that this one can have 2 instances of cp_intc); support for it has never hit mainline though. And even as I suspected their Titan MIPS SoC also makes use of it; looks like it's used in the Avalanches too.
da830/omap-L1x family parts? If not, I would lean towards keeping it in arch/arm/mach-davinci and abstracting it out later if/when necessary since if it really is generic, and not ARM specific, arch/arm/common is no less appropriate than arch/arm/mach-davinci.
Come on, i8259 is generic, and yet every arch has its own driver of this PIC. ;-) I'm afraid that Linux jst doesn't have a proper place for the shared PIC drivers yet...
Signed-off-by: Steve Chen <[email protected]> Signed-off-by: Mark Greer <[email protected]> Signed-off-by: Sergei Shtylyov <[email protected]>
Mostly, it looks pretty good. Some comments below...
Index: linux-2.6/arch/arm/common/cp_intc.c =================================================================== --- /dev/null +++ linux-2.6/arch/arm/common/cp_intc.c @@ -0,0 +1,159 @@ +/* + * TI Common Platform Interrupt Controller (cp_intc) driver + * + * Author: Steve Chen <[email protected]> + * Copyright (C) 2008-2009, MontaVista Software, Inc. <[email protected]> + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed "as is" without any warranty of any + * kind, whether express or implied. + */ + +#include <linux/init.h> +#include <linux/sched.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/irq.h> +#include <linux/io.h> +#include <asm/hardware/cp_intc.h> + +static void __iomem *cp_intc_base; +
Is there only ever going to be a single CP_INTC on any given device? If not, this approach will fall down.
Sigh, I know... and it looks like TI has actually come up with a SoC that has 2 cp_intc's (I only don't understand why -- even a single one can support hell of a lot of interrupts).
Instead of this global, maybe adding a struct with the base and num_irqs that is allocated during cp_intc_init() ?
With pointers stored in an array? Can do in principle, though I'm not eager to add another indirection level without a dire need.
+static int cp_intc_set_irq_type(unsigned int irq, unsigned int flow_type) +{ + unsigned reg = irq >> 5;
minor nitpick: for a little more clarity, you can use BIT_WORD() here.
OK.
+ unsigned mask = irq & 0x1f;
maybe a #define here instead of hard-coded constant.
What kind of #define you'd want? This is symmetric to the above, i.e. it gets a bit number within the 32-bit word. I'm not seeing anything giving that in <linux/bitops.h>... though well, I can come up with something.
+ unsigned pol_mask = cp_intc_read(CP_INTC_SYS_POLARITY(reg)); + unsigned type_mask = cp_intc_read(CP_INTC_SYS_TYPE(reg));
[...] Ugh, lots of uncommented code left behind. Not very polite. :-/ WBR, Sergei _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
