Thanks for the patch Wolfram. Comments below. On Wed, Feb 25, 2009 at 8:32 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: > Signed-off-by: Wolfram Sang <w.s...@pengutronix.de> > --- > arch/powerpc/boot/dts/pcm032.dts | 391 +++++++ > arch/powerpc/configs/52xx/pcm032_defconfig | 1394 > ++++++++++++++++++++++++++
Do you really need a separate defconfig for this board? Can it be merged with an existing defconfig? > arch/powerpc/platforms/52xx/Kconfig | 1 + > arch/powerpc/platforms/52xx/mpc5200_simple.c | 3 +- > 4 files changed, 1788 insertions(+), 1 deletions(-) > create mode 100644 arch/powerpc/boot/dts/pcm032.dts > create mode 100644 arch/powerpc/configs/52xx/pcm032_defconfig > > diff --git a/arch/powerpc/boot/dts/pcm032.dts > b/arch/powerpc/boot/dts/pcm032.dts > new file mode 100644 > index 0000000..ebaf660 > --- /dev/null > +++ b/arch/powerpc/boot/dts/pcm032.dts > @@ -0,0 +1,391 @@ > [...] > + l...@e4000000 { As per generic names recommended practice, this node name should really be something like "localbus {", and you should drop the node address since the local bus doesn't really have an address. > + compatible = "fsl,lpb"; Ideally should be: compatible = "fsl,mpc5200b-lpb","fsl,mpc5200-lpb","simple-bus"; I know that 'fsl,lpb' has been used in the past, but it is far to generic and isn't good practice. The 'simple-bus' property ensures that the bus will get probed. > + ranges = <0x0 0xe4000000 0x08000000>; > + #size-cells = <1>; > + #address-cells = <1>; Should be #address-cells = <2>;. First cell is CS#, second cell is offset from base of CS. This is particularly important if the LPB FIFO is ever used with any of this hardware. motionpro.dts is a good example of what it should look like. > + > + /* free chipselect */ > + c...@00000000 { > + compatible = "free"; Very bad! If you want to show available chip selects, then add some commented out sections as examples. Creating nodes with the extremely generic compatible value of "free" is just asking for trouble (especially when someone new sees the example and decides to use the same thing in their driver). > + l...@f7000000 { Another localbus node? Why can't a single localbus node hold all the child nodes? > + compatible = "fsl,lpb"; > + ranges = <0x0 0xf7000000 0x09000000>; > + #size-cells = <1>; > + #address-cells = <1>; > + > + fp...@00e00000 { > + compatible = "fpga1"; Too generic. Be board specific. > + reg = <0x00e00000 0x02000000>; > + bank-width = <4>; > + }; > + > + fp...@02e00000 { > + compatible = "fpga2"; Ditto Otherwise, the rest of the patch looks good to me. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev