Subject: Re: [PATCH 1/2] ppc32: Add initial support for DAVE "PPChameleon" board.

From: Matt Porter <[email-masked]>
Date: Tue, 1 Nov 2005 16:29:39 -0700

On Wed, Oct 12, 2005 at 05:38:17PM +0200, Wolfgang Denk wrote:
> the following patch (against current kernel.org tree) adds suport for
> the "PPChameleon" modules / eval boards manufactured by DAVE s.r.l.

See comments below.
 
> +config PPChameleonEVB
> +	bool "PPChameleonEVB"
> +	help
> +	  This option enables support for the DAVE 405EP evaluation board.
> +

It's unusual to have a mixed case config option. Is there a better
option that makes sense? PP_CHAM_EVB?

>  /* DCR defines */
> -#define DCRN_CPMSR_BASE         0x0BA
> -#define DCRN_CPMFR_BASE         0x0B9
> +#define DCRN_CPMSR_BASE		0x0BA
> +#define DCRN_CPMFR_BASE		0x0B9

Please drop these whitespace changes.

> -#define IBM_CPM_GPT             0x80000000      /* GPT interface */
> -#define IBM_CPM_PCI             0x40000000      /* PCI bridge */
> -#define IBM_CPM_UIC             0x00010000      /* Universal Int Controller */
> -#define IBM_CPM_CPU             0x00008000      /* processor core */
> -#define IBM_CPM_EBC             0x00002000      /* EBC controller */
> -#define IBM_CPM_SDRAM0          0x00004000      /* SDRAM memory controller */
> -#define IBM_CPM_GPIO0           0x00001000      /* General Purpose IO */
> -#define IBM_CPM_TMRCLK          0x00000400      /* CPU timers */
> -#define IBM_CPM_PLB             0x00000100      /* PLB bus arbiter */
> -#define IBM_CPM_OPB             0x00000080      /* PLB to OPB bridge */
> -#define IBM_CPM_DMA             0x00000040      /* DMA controller */
> -#define IBM_CPM_IIC0            0x00000010      /* IIC interface */
> -#define IBM_CPM_UART1           0x00000002      /* serial port 0 */
> -#define IBM_CPM_UART0           0x00000001      /* serial port 1 */
> +#define IBM_CPM_GPT		0x80000000      /* GPT interface */
> +#define IBM_CPM_PCI		0x40000000      /* PCI bridge */
> +#define IBM_CPM_UIC		0x00010000      /* Universal Int Controller */
> +#define IBM_CPM_CPU		0x00008000      /* processor core */
> +#define IBM_CPM_EBC		0x00002000      /* EBC controller */
> +#define IBM_CPM_SDRAM0		0x00004000      /* SDRAM memory controller */
> +#define IBM_CPM_GPIO0		0x00001000      /* General Purpose IO */
> +#define IBM_CPM_TMRCLK		0x00000400      /* CPU timers */
> +#define IBM_CPM_PLB		0x00000100      /* PLB bus arbiter */
> +#define IBM_CPM_OPB		0x00000080      /* PLB to OPB bridge */
> +#define IBM_CPM_DMA		0x00000040      /* DMA controller */
> +#define IBM_CPM_IIC0		0x00000010      /* IIC interface */
> +#define IBM_CPM_UART1		0x00000002      /* serial port 0 */
> +#define IBM_CPM_UART0		0x00000001      /* serial port 1 */

Same here, if whitespace chanes are important submit them separately.

> diff --git a/arch/ppc/platforms/4xx/ppchameleon.c b/arch/ppc/platforms/4xx/ppchameleon.c
> new file mode 100644
> --- /dev/null

<snip>

> +#if defined(CONFIG_BIOS_FIXUP)
> +void __init bios_fixup (struct pci_controller *hose, struct pcil0_regs *pcip)

<snip>

You don't use this bios_fixup garbage in this port (at least according
to your defconfig) so just drop it.

As an aside, this stuff is pretty awful and has been since 405 first
came into the tree. If the basic functionality were required, a new
port should simply reprogram the pci host bridge and let the pci
subsystem place the BARs.

-Matt