On Sat, Dec 22, 2007 at 08:13:31PM +0100, Jochen Friedrich wrote:
> + [EMAIL PROTECTED] {
> + compatible = "c-cube,gtx";
> + reg = <400000 3000 0 200000>;
> + interrupts = <2 2>;
> + interrupt-parent = <&PIC>;
> + };
> +
> + [EMAIL PROTECTED] {
> + compatible = "betaresearch,dbox2-fp";
> + interrupts = <4 2>;
> + interrupt-parent = <&PIC>;
> + gpios = <0 e>;
> + gpio-parent = <&CPM1_PIO>;
> + };
> +
> + [EMAIL PROTECTED] {
> + compatible = "betaresearch,dbox2-fe";
> + interrupts = <e 2>;
> + interrupt-parent = <&PIC>;
> + };
These unit addresses look wrong.
> + [EMAIL PROTECTED] {
> + compatible = "betaresearch,dbox2-cam";
> + reg = <4000000 20000>;
> + interrupts = <6 2>;
> + interrupt-parent = <&PIC>;
> + gpios = <1 1c 1 1d 1 1e 1 1f>;
> + gpio-parent = <&CPM1_PIO>;
> + };
> +
> + [EMAIL PROTECTED] {
> + compatible = "betaresearch,dbox2-cam";
> + reg = <4040000 20000>;
> + interrupts = <6 2>;
> + interrupt-parent = <&PIC>;
> + gpios = <1 1c 1 1d 1 1e 1 1f>;
> + gpio-parent = <&CPM1_PIO>;
> + };
Maybe gpios should have a length field? Or maybe we should just
phandle to a separate node under the gpio controller node. What happens if
a device sits on two different gpio-parents?
> + [EMAIL PROTECTED] {
> + // Flash also has info about model needed by setup
> + compatible = "cfi-flash",
> + "betaresearch,dbox2-config";
What does dbox2-config mean?
> + [EMAIL PROTECTED] {
> + label = "Flash without bootloader";
> + reg = <20000 7e0000>;
> + };
> + [EMAIL PROTECTED] {
> + label = "Complete Flash";
> + reg = <0 800000>;
> + read-only;
> + };
What is "ovpartition"?
> + [EMAIL PROTECTED] {
> + device_type = "watchdog";
> + compatible = "fsl,mpc823-wdt",
> + "fsl,pq1-wdt";
> + reg = <0 10>;
> + };
No device_type.
> + [EMAIL PROTECTED] {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + reg = <9c0 40>;
> + command-proc = <9c0>;
command-proc is obsolete.
> + [EMAIL PROTECTED] {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 2000 2000>;
> +
> + [EMAIL PROTECTED] {
> + compatible = "fsl,cpm-muram-data";
> + reg = <0 1c00>;
> + };
> + };
Should have compatible of fsl,cpm-muram on the muram node as well, even
though the current code doesn't use it.
> + // Port D is LCD exclusive. Don't export as GPIO
> + CPM1_PIO: [EMAIL PROTECTED] {
> + compatible = "fsl,cpm1-pario";
> + reg = <970 180>;
> + num-ports = <3>;
> + #gpio-cells = <2>;
> + };
Why are we doing things differently just because all of the pins on this
port happen to be directed to one device? If it's because the Linux GPIO
API sucks, then either fix the API, or work around the suckage in software,
not the device tree.
> + [EMAIL PROTECTED] {
> + reg = <970 10>;
> + compatible = "samsung,ks0713";
> + };
So some driver that matches on samsung,ks0713 has to know the details of the
mpc8xx GPIO registers?
> + [EMAIL PROTECTED] {
> + compatible = "fsl,mpc823-brg",
> + "fsl,cpm1-brg",
> + "fsl,cpm-brg";
> + reg = <9f0 10>;
> + };
Should have clock-frequency in the brg node.
> + [EMAIL PROTECTED] {
> + compatible = "fsl,mpc823-i2c",
> + "fsl,cpm1-i2c",
> + "fsl,cpm-i2c";
> + reg = <860 20 3c80 30>;
> + interrupts = <10 3>;
> + interrupt-parent = <&CPM_PIC>;
> + fsl,cpm-command = <0010>;
> + };
Should have #address-cells and #size-cells.
> +enum dbox2_mid dbox2_get_mid(void)
> +{
> + return dbox2_manuf_id;
> +}
> +EXPORT_SYMBOL_GPL(dbox2_get_mid);
Honestly, you're claiming derived-work status by calling a function that
returns an integer, that was read directly from hardware?
> + if (dbox2_manuf_id == DBOX2_MID_NOKIA)
> + np = of_find_node_by_path("/[EMAIL PROTECTED]/[EMAIL
> PROTECTED]");
> + else
> + np = of_find_node_by_path("/[EMAIL PROTECTED]/[EMAIL
> PROTECTED]");
> +
> + if (np) {
> + of_detach_node(np);
> + of_node_put(np);
> + }
> +
> + if (dbox2_manuf_id == DBOX2_MID_PHILIPS)
> + np = of_find_node_by_path("/[EMAIL PROTECTED]/[EMAIL
> PROTECTED]");
> + else
> + np = of_find_node_by_path("/[EMAIL PROTECTED]/[EMAIL
> PROTECTED]");
> +
> + if (np) {
> + of_detach_node(np);
> + of_node_put(np);
> + }
> +}
I'd use separate device trees (I only did this kind of thing in mpc885ads
because it's dip-switchable), but whatever...
> diff --git a/include/asm-powerpc/mpc8xx.h b/include/asm-powerpc/mpc8xx.h
> index 2be014b..b6fd7d6 100644
> --- a/include/asm-powerpc/mpc8xx.h
> +++ b/include/asm-powerpc/mpc8xx.h
> @@ -23,6 +23,10 @@
> #include <platforms/8xx/mpc885ads.h>
> #endif
>
> +#if defined(CONFIG_DBOX2)
> +#include <platforms/8xx/dbox2.h>
> +#endif
This shouldn't be needed.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
