Re: [PATCH 3/8] powerpc/85xx: Add support for the "socrates" board (MPC8544).

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Tue, Mar 31, 2009 at 6:37 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
> Supported are Ethernet, serial console, I2C, I2C-based RTC and
> temperature sensors, NOR and NAND flash, PCI, USB, CAN and Lime
> display controller.
>
> The multiplexing of FPGA interrupts onto PowerPC interrupt lines is
> supported through our own fpga_pic interrupt controller driver.
>
> For example the SJA1000 controller is level low sensitive connected to
> fpga_pic line 2 and is routed to the second (of three) irq lines to
> the CPU:
>
>    can@3,100 {
>            compatible = "philips,sja1000";
>            reg = <3 0x100 0x80>;
>            interrupts = <2 2>;
>            interrupts = <2 8 1>;   // number, type, routing
>            interrupt-parent = <&fpga_pic>;
>    };
>
> Signed-off-by: Sergei Poselenov <sposelenov-qv+LCo8X3VpBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Yuri Tikhonov <yur-qv+LCo8X3VpBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Ilya Yanok <yanok-qv+LCo8X3VpBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> Signed-off-by: Anatolij Gustschin <agust-ynQEQJNshbs@public.gmane.org>
> Signed-off-by: Dmitry Rakhchev <rda-qv+LCo8X3VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/powerpc/boot/dts/socrates.dts              |  338 +++++
>  arch/powerpc/configs/85xx/socrates_defconfig    | 1410 ++++++++++++++++++++++++

I always ask this question: Do you really need a board specific
defconfig file?  We are multiplatform now.

>  arch/powerpc/platforms/85xx/Kconfig             |    6
>  arch/powerpc/platforms/85xx/Makefile            |    1
>  arch/powerpc/platforms/85xx/socrates.c          |  133 ++
>  arch/powerpc/platforms/85xx/socrates_fpga_pic.c |  327 +++++
>  arch/powerpc/platforms/85xx/socrates_fpga_pic.h |   16

socrates_fpga_pic is only ever going to be used by socrates.c.  I'd
roll the two socrates.c files into one and eliminate the header file.

> Index: linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.c
[...]
> +struct socrates_fpga_irq_info {
> +       unsigned int irq_line;
> +       int type;
> +};
> +
> +/*
> + * Interrupt routing and type table
> + *
> + * IRQ_TYPE_NONE means the interrupt type is configurable,
> + * otherwise it's fixed to the specified value.
> + */
> +static struct socrates_fpga_irq_info fpga_irqs[SOCRATES_FPGA_NUM_IRQS] = {
> +       [0] = {0, IRQ_TYPE_NONE},
> +       [1] = {0, IRQ_TYPE_LEVEL_HIGH},
> +       [2] = {0, IRQ_TYPE_LEVEL_LOW},
> +       [3] = {0, IRQ_TYPE_NONE},
> +       [4] = {0, IRQ_TYPE_NONE},
> +       [5] = {0, IRQ_TYPE_NONE},
> +       [6] = {0, IRQ_TYPE_NONE},
> +       [7] = {0, IRQ_TYPE_NONE},
> +       [8] = {0, IRQ_TYPE_LEVEL_HIGH},
> +};

It is good practice to use named elements in initializers: {.type =
IRQ_TYPE_LEVEL_HIGH},

Static variables are initialized to zero.  Everything that is 0 can be
dropped from this initializer (lines 0 and 3-7, and the irq_line
field).

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.