Re: [PATCH v2 2/4] powerpc, mpc52xx: add a4m072 board support

[Grant Likely <grant.likely@secretlab.ca>]

On Wed, Jun 22, 2011 at 12:39:10PM +0200, Heiko Schocher wrote:
> Signed-off-by: Heiko Schocher <hs@denx.de>
> cc: Grant Likely <grant.likely@secretlab.ca>
> cc: devicetree-discuss@ozlabs.org
> cc: Wolfgang Denk <wd@denx.de>
> cc: Wolfram Sang <w.sang@pengutronix.de>
> ---
> For this patchseries following patch is needed:
> 
> http://patchwork.ozlabs.org/patch/91919/
> 
> Grant? Do you have some comments on that patch?
> 
> changes for v2:
>   add comment from Wolfram Sang:
>   use mpc5200.dtsi
> 
>  arch/powerpc/boot/dts/a4m072.dts             |  172 ++++++++++++++++++++++++++
>  arch/powerpc/platforms/52xx/mpc5200_simple.c |    1 +
>  2 files changed, 173 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/boot/dts/a4m072.dts
> 
> diff --git a/arch/powerpc/boot/dts/a4m072.dts b/arch/powerpc/boot/dts/a4m072.dts
> new file mode 100644
> index 0000000..adb6746
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/a4m072.dts
> @@ -0,0 +1,172 @@
> +/*
> + * a4m072 board Device Tree Source
> + *
> + * Copyright (C) 2011 DENX Software Engineering GmbH
> + * Heiko Schocher <hs@denx.de>
> + *
> + * Copyright (C) 2007 Semihalf
> + * Marian Balakowicz <m8@semihalf.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +/include/ "mpc5200b.dtsi"

Ah, I missed this follow up patch.  Yes, this is better.

> +
> +/ {
> +	model = "anonymous,a4m072";
> +	compatible = "anonymous,a4m072";
> +
> +	soc5200@f0000000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "fsl,mpc5200b-immr";
> +		ranges = <0 0xf0000000 0x0000c000>;
> +		reg = <0xf0000000 0x00000100>;
> +		bus-frequency = <0>; /* From boot loader */
> +		system-frequency = <0>; /* From boot loader */
> +
> +		cdm@200 {
> +			fsl,ext_48mhz_en = <0x0>;
> +			fsl,fd_enable = <0x01>;
> +			fsl,fd_counters = <0xbbbb>;

Are these new properties documented?  They need to be.  Also,
convention is to use '-' instead of '_' in property names.

> +		};
> +
> +		timer@600 {
> +			compatible = "fsl,mpc5200b-gpt","fsl,mpc5200-gpt";
> +			reg = <0x600 0x80>;
> +			interrupts = <1 9 0>;
> +			fsl,has-wdt;
> +		};

Isn't this node already in the mpc5200b.dtsi file?

Otherwise, this patch looks pretty good.

g.