Re: [PATCH/RFC] powerpc: Add MPC5200 Interrupt Controller support.

[Sylvain Munaut <tnt@246tNt.com>]

> +
> +	/*
> +	 * Most of ours IRQs will be level low
> +	 * Only external IRQs on some platform may be others
> +	 */
> +	type = IRQ_TYPE_LEVEL_LOW;
>   
I've been wondering : Why LEVEL_LOW ?
Aren't they LEVEL_HIGH ?
(not that it changes much here ...)


> +
> +end:
> +	of_node_put(picnode);
> +	of_node_put(sdmanode);
>   
Is of_node_put specified as resilient to NULL argument ? (in the error
path, that could happen)

Also, I think "PANIC" would be appropriate in the error path. If we
can't init
the PIC properly we may as well give up ... It's not like we're going to
do much
without it ...


> +/* HW IRQ mapping */
> +#define MPC52xx_IRQ_L1_CRIT	(0)
> +#define MPC52xx_IRQ_L1_MAIN	(1)
> +#define MPC52xx_IRQ_L1_PERP	(2)
> +#define MPC52xx_IRQ_L1_SDMA	(3)
> +
> +#define MPC52xx_IRQ_L1_OFFSET   (6)
> +#define MPC52xx_IRQ_L1_MASK     (0xc0)
> +
> +#define MPC52xx_IRQ_L2_OFFSET   (0)
> +#define MPC52xx_IRQ_L2_MASK     (0x3f)
> +
> +#define MPC52xx_IRQ_HIGHTESTHWIRQ (0xd0)
> +
> +/* Interrupt controller Register set */
> +struct mpc52xx_intr {
> +	u32 per_mask;		/* INTR + 0x00 */
> +	u32 per_pri1;		/* INTR + 0x04 */
> +	u32 per_pri2;		/* INTR + 0x08 */
> +	u32 per_pri3;		/* INTR + 0x0c */
> +	u32 ctrl;		/* INTR + 0x10 */
> +	u32 main_mask;		/* INTR + 0x14 */
> +	u32 main_pri1;		/* INTR + 0x18 */
> +	u32 main_pri2;		/* INTR + 0x1c */
> +	u32 reserved1;		/* INTR + 0x20 */
> +	u32 enc_status;		/* INTR + 0x24 */
> +	u32 crit_status;	/* INTR + 0x28 */
> +	u32 main_status;	/* INTR + 0x2c */
> +	u32 per_status;		/* INTR + 0x30 */
> +	u32 reserved2;		/* INTR + 0x34 */
> +	u32 per_error;		/* INTR + 0x38 */
> +};
>   
When I said on IRC you could remerge them, I meant a little more
smartly than just 'join' them. i.e., put the mpc52xx_intr with all the
other register set at the very least ...



    Sylvain