On Thu, Jun 04, 2009 at 09:59:04PM +0100, Byron Bradley wrote:
> The Thecus N1200 is a NAS device with a single internal SATA disk and
> an eSATA port based on an MPC8347 SoC.
Comments on a number of fairly minor device tree nits below:
[snip]
> + soc8...@e0000000 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + device_type = "soc";
> + compatible = "simple-bus";
The compatible value should have something more specific
(e.g. "fsl,mpc8340-soc") before listing "simple-bus".
> + ranges = <0x0 0xe0000000 0x00100000
> + 0xfe000000 0xfe000000 0x0800000>;
> + reg = <0xe0000000 0x00000200>;
> + bus-frequency = <0>; // from bootloader
> +
> + physmap-fl...@fe000000 {
Calling this just "flash" would be more normal.
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "cfi-flash";
Ideally this should list the actual model of flash chip, before the
generic "cfi-flash".
> + reg = <0xfe000000 0x0800000>;
> + bank-width = <2>;
> + device-width = <1>;
> +
> + u-b...@0 {
> + reg = <0x0 0x040000>;
> + read-only;
> + };
> +
> + u-boot-con...@40000 {
> + reg = <0x040000 0x040000>;
> + label = "u-boot config";
> + };
> +
> + u...@080000 {
> + reg = <0x080000 0x100000>;
> + };
> +
> + ker...@180000 {
> + reg = <0x180000 0x1a0000>;
> + };
> +
> + ramd...@320000 {
> + reg = <0x320000 0x4e0000>;
> + };
> + };
> +
> + w...@200 {
> + device_type = "watchdog";
No device_type here.
> + compatible = "mpc83xx_wdt";
> + reg = <0x200 0x100>;
> + };
> +
> + i...@3000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + cell-index = <0>;
> + compatible = "fsl-i2c";
> + reg = <0x3000 0x100>;
> + interrupts = <14 0x8>;
> + interrupt-parent = <&ipic>;
> + dfsrr;
> +
> + fanc...@32 {
A less abbreviated name, like "fan-control" would be more usual here.
> + device_type = "hwmon";
No device_type here either.
> + compatible = "fintek,f75375";
> + reg = <0x2e>;
> + };
> +
> + r...@32 {
> + device_type = "rtc";
Or here.
> + compatible = "ricoh,rs5c372a";
> + reg = <0x32>;
> + };
> +
> + boardc...@32 {
Again "board-control" would be preferable.
> + device_type = "misc";
Especially no device_type here.
[snip]
> + ipic: p...@700 {
> + interrupt-controller;
> + #address-cells = <0>;
> + #interrupt-cells = <2>;
> + reg = <0x700 0x100>;
> + device_type = "ipic";
This should have a compatible property. It shouldn't really have
device_type, but I suspect that's a bug in the ipic binding, rather
than your tree per-se.
> + };
> +
> + gpio1: gpio-control...@c00 {
> + #gpio-cells = <2>;
> + compatible = "fsl,mpc8347-gpio", "fsl,mpc8349-gpio";
This actually is an 8349 board, yes? Generally compatible should be
listed from most specific to least specific, so the 8349 entry should
go first.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
