Looking good. A few more comments below. On Wed, Jun 11, 2008 at 3:43 AM, David Jander <[EMAIL PROTECTED]> wrote: > > Made MPC5121_ADS board support generic
This description isn't verbose enough. Describe what this patch changes in more detail please. > > Signed-off-by: David Jander <[EMAIL PROTECTED]> > --- > diff --git a/arch/powerpc/boot/dts/prtlvt.dts > b/arch/powerpc/boot/dts/prtlvt.dts > new file mode 100644 > index 0000000..93d2fa2 > --- /dev/null > +++ b/arch/powerpc/boot/dts/prtlvt.dts > @@ -0,0 +1,267 @@ > + [EMAIL PROTECTED] { > + compatible = "fsl,mpc5121-immr"; As Scott mentioned, this should be: compatible = "fsl,mpc5121-immr", "simple-bus"; > + //[EMAIL PROTECTED] { > + // compatible = "mpc512x-axe"; Even though this is commented out; the compatible value is in the wrong form. > + // port1 using extern ULPI PHY > + [EMAIL PROTECTED] { > + device_type = "usb"; > + compatible = "fsl,fsl-usb2-dr"; Poor compatible form. fsl,fsl-usb2-dr does not specify a particular implementation of the device and is rather redundant. Should probably be: compatible = "fsl,mpc5121-usb2-dr", "fsl-usb2-dr"; Which means: 'this is a usb-dr controller on the MPC5121, which is also compatible with the pre-existing "fsl-usb2-dr" device tree binding.' "fsl-usb2-dr" is used on line 656 of fsl_soc.c. It's not a very good values for a compatible field (for various reasons), but it is already in existence so it is okay to claim compatibility with it. If this USB controller is *not* compatible with fsl-usb2-dr, then you should not have that value. > + // port0 using internal UTMI PHY > + [EMAIL PROTECTED] { > + device_type = "usb"; > + compatible = "fsl,fsl-usb2-dr"; Ditto > diff --git a/arch/powerpc/platforms/512x/Kconfig > b/arch/powerpc/platforms/512x/Kconfig > index 4c0da0c..67707f2 100644 > --- a/arch/powerpc/platforms/512x/Kconfig > +++ b/arch/powerpc/platforms/512x/Kconfig > @@ -9,11 +9,26 @@ config PPC_MPC5121 > select PPC_MPC512x > default n > > +config MPC5121_GENERIC > + bool > + default n > + > config MPC5121_ADS > bool "Freescale MPC5121E ADS" > depends on PPC_MULTIPLATFORM && PPC32 > select DEFAULT_UIMAGE > select PPC_MPC5121 > + select MPC5121_GENERIC > help > This option enables support for the MPC5121E ADS board. > default n > + > +config PRTLVT > + bool "Protonic LVT family of MPC5121 based boards" > + depends on PPC_MULTIPLATFORM && PPC32 > + select DEFAULT_UIMAGE > + select PPC_MPC5121 > + select MPC5121_GENERIC > + help > + This option enables support for the Protonic LVT family (ZANMCU and > VICVT2). > + default n Personally, I wouldn't bother with separate Kconfig entrys for each supported board. Just have a single MPC5121_GENERIC config property and add to the help text the list of boards that it supports. Boards that require extra platform code can add extra Kconfig entries. > diff --git a/arch/powerpc/platforms/512x/Makefile > b/arch/powerpc/platforms/512x/Makefile > index 232c89f..9d40a2e 100644 > --- a/arch/powerpc/platforms/512x/Makefile > +++ b/arch/powerpc/platforms/512x/Makefile > @@ -1,4 +1,4 @@ > # > # Makefile for the Freescale PowerPC 512x linux kernel. > # > -obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o > +obj-$(CONFIG_MPC5121_GENERIC) += mpc5121_generic.o > +static int __init mpc5121_generic_probe(void) > +{ > + unsigned long root = of_get_flat_dt_root(); > + > + return of_flat_dt_is_compatible(root, "fsl,mpc5121ads") || > + of_flat_dt_is_compatible(root, "prt,prtlvt"); This list is just going to get bigger. Maybe consider a list like is used in arch/powerpc/platforms/52xx/mpc5200_simple.c Otherwise, this patch is looking pretty good. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. _______________________________________________ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded