Re: [PATCH 07/15] [POWERPC] Promess Motion-PRO DTS

On Sun, Oct 07, 2007 at 01:25:33PM +0200, Marian Balakowicz wrote:

+		gpt@600 {	// General Purpose Timer
+			compatible = "mpc5200b-gpt\0mpc5200-gpt";
+			device_type = "gpt";

"timer" would be a better node name than "gpt", and the device type should be
left out entirely.  As others pointed out, compatible should be
"fsl,mpc5200b-gpt", "fsl,mpc5200-gpt".

+			has-wdt;

fsl,has-wdt

+		rtc@800 {	// Real time clock
+			compatible = "mpc5200b-rtc\0mpc5200-rtc";
+			device_type = "rtc";

This doesn't actually implement the OF rtc interface...

+		mscan@980 {

What is mscan?

+			device_type = "mscan";

This is not a standard device type.

+		bestcomm@1200 {
+			device_type = "dma-controller";

dma-controller should be the node name, and device_type should be omitted.

+		ethernet@3000 {
+			device_type = "network";
+			compatible = "mpc5200b-fec\0mpc5200-fec";
+			reg = <3000 800>;
+			mac-address = [ 02 03 04 05 06 07 ]; // Bad!

Should be local-mac-address.  
And yes, hardcoding a mac address is bad.  Don't do it. :-)

+		i2c@3d40 {
+			device_type = "i2c";
+			compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
+			cell-index = <1>;

What is cell-index?  The fsl-i2c driver doesn't use it AFAICT.

+		sram@8000 {
+			device_type = "sram";

No device type.

+	cpld {
+		device_type = "cpld";
+		compatible = "cpld";
+		reg = <50010000 ffff>;
+	};

This device is compatible with every CPLD that has ever existed?  Wow! :-)

-Scott