Re: [PLEASE REVIEW] ppc32 8xx: core PRxK board family support

[Dan Malek <dan@embeddededge.com>]

On Nov 14, 2005, at 7:45 AM, Marcelo Tosatti wrote:

> +++ linux-2.6.14-rc4/arch/ppc/boot/include/cyc_banner.h	2005-10-25 
> 07:01:57.000000000 -0500

I'm not a big fan of all of these banners, printing, and ascii
text in the kernel, but if you think you must have it ........

> +//XXX: remove? Its unused.
> +#if 0
> +struct type0000 {
> +    unsigned char	flash_sign[FLASH_SIGN_SIZE];    /* mark initialized 
> CMOS */
> +    unsigned char	version;            /* Configure vector version */
> +    unsigned char	routing_protocol;   /* RIP, OSPF, etc */
> +    unsigned char	save_sw;            /* TRUE : the RTBOOT must be 
> saved into flash */
> +};
> +#endif

Yes please, remove it.

> +    //[GB]May/06/05  Ethernet Receive Rate Limit
> +    // unsigned char    reserved1[4];

Get rid of trash like this, too, or make it a real C style comment
that explains what is going on here for the rest of us.

> +++ linux-2.6.14-rc4/arch/ppc/boot/simple/embed_config.c	2005-10-25 
> 13:55:04.000000000 -0500

This is a pretty big chunk of board specific code beyond just setting
up the board configuration info.  Consider making it a separate file.

> +++ linux-2.6.14-rc4/arch/ppc/platforms/cpld.h	2005-10-24 
> 15:35:25.000000000 -0500

Would you name this something a little more descriptive, like
perhaps cyc_cpld.h?  Makes it easier to understand where the
files are used.

> +struct fpga_pc_regs {
> +	unsigned char fpga_pc_misc;	// Controls PCMCIA IO's window size

I still don't like // comments ....... :-)

> +//	mem_addr = (unsigned long *)(&cpmp->cp_dpmem[dp_addr]);

Just remove it.

> +//	dp_addr = m8xx_cpm_dpalloc(32);

Ditto, and anywhere else.  If code isn't used, let's just get
rid of it.  Or put in a comment why there may be alternative
or if you are testing something.

Thanks!


	-- Dan