Re: [PATCH] mpc8308_p1m: support for MPC8308 P1M board

[Scott Wood <scottwood@freescale.com>]

On Tue, 7 Sep 2010 12:09:03 +0200
Ilya Yanok <yanok@emcraft.com> wrote:

> +	compatible = "mpc8308_p1m";

This needs a vendor prefix.

> +		i2c0@3000 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			cell-index = <0>;
> +			compatible = "fsl-i2c";
> +			reg = <0x3000 0x100>;
> +			interrupts = <14 0x8>;
> +			interrupt-parent = <&ipic>;
> +			dfsrr;
> +			fram@50 {
> +				compatible = "ramtron,24c64";
> +				reg = <0x50>;
> +			};
> +		};
> +
> +		i2c1@3100 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			cell-index = <0>;
> +			compatible = "fsl-i2c";
> +			reg = <0x3100 0x100>;
> +			interrupts = <15 0x8>;
> +			interrupt-parent = <&ipic>;
> +			dfsrr;
> +			pwm@28 {
> +				compatible = "maxim,ds1050";
> +				reg = <0x28>;
> +			};
> +			sensor0@48 {
> +				compatible = "maxim,max6625";
> +				reg = <0x48>;
> +			};
> +			sensor1@49 {
> +				compatible = "maxim,max6625";
> +				reg = <0x49>;
> +			};
> +			sensor2@4b {
> +				compatible = "maxim,max6625";
> +				reg = <0x4b>;
> +			};
> +		};

Why "i2c0@3000" and "i2c1@3100" rather than "i2c@3000" and "i2c@3100"?
Likewise for the sensor nodes.

Drop cell-index; it's not part of the fsl i2c binding (plus, they
probably shouldn't both be zero...).

> +		enet0: ethernet@24000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0x0 0x24000 0x1000>;
> +
> +			cell-index = <0>;
> +			device_type = "network";
> +			model = "eTSEC";
> +			compatible = "gianfar";
> +			reg = <0x24000 0x1000>;
> +			local-mac-address = [ 00 00 00 00 00 00 ];
> +			interrupts = <32 0x8 33 0x8 34 0x8>;
> +			interrupt-parent = <&ipic>;
> +			phy-handle = < &phy1 >;
> +			fsl,magic-packet;

8308 does not have magic packet.

> +		gpio@c00 {
> +			#gpio-cells = <2>;
> +			device_type = "gpio";
> +			compatible = "fsl,mpc8308-gpio", "fsl,mpc8349-gpio";
> +			reg = <0xc00 0x18>;
> +			interrupts = <74 0x8>;
> +			interrupt-parent = <&ipic>;
> +			gpio-controller;
> +		};

Drop device_type.

> +	pci0: pcie@e0009000 {
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		#interrupt-cells = <1>;
> +		device_type = "pci";
> +		compatible = "fsl,mpc8308-pcie", "fsl,mpc8314-pcie";
> +		reg = <0xe0009000 0x00001000
> +			0xb0000000 0x01000000>;
> +		ranges = <0x02000000 0 0xa0000000 0xa0000000 0 0x10000000
> +		          0x01000000 0 0x00000000 0xb1000000 0 0x00800000>;
> +		bus-range = <0 0>;
> +		interrupt-map-mask = <0xf800 0 0 7>;
> +		interrupt-map = <0 0 0 1 &ipic 1 8
> +				 0 0 0 2 &ipic 1 8
> +				 0 0 0 3 &ipic 1 8
> +				 0 0 0 4 &ipic 1 8>;

Should interrupt-map-mask be <0 0 0 7>?  Or possibly <0 0 0 0> with
just one map entry?

-Scott