[U-Boot] [PATCH 05/11] ARM: mxs: Add Creative ZEN XFi3 board

[Wolfgang Denk]

Dear Marek,

In message <1375220281-11132-6-git-send-email-marex at denx.de> you wrote:
>
...
> +	/* Program the SmartLCD controller */
> +	writel(LCDIF_CTRL1_RECOVER_ON_UNDERFLOW, &regs->hw_lcdif_ctrl1_set);
> +
> +	writel(0x03030202, &regs->hw_lcdif_timing);

Please don't use such magic numbers.

> +	mxsfb_write_register(1, 0x01c);
> +
> +	mxsfb_write_register(2, 0x100);
> +	/* 0x30 flips the LCD */
> +	mxsfb_write_register(3, 0x1038);
> +	mxsfb_write_register(8, 0x808);
> +	/* This can possibly contain 0x111 */
> +	mxsfb_write_register(0xc, 0x0);
> +	mxsfb_write_register(0xf, 0xc01);
> +	mxsfb_write_register(0x20, 0);
> +	mxsfb_write_register(0x21, 0);
> +	mdelay(30);
> +	mxsfb_write_register(0x10, 0xa00);
> +	mxsfb_write_register(0x11, 0x1038);
> +	mdelay(30);
> +	mxsfb_write_register(0x12, 0x1010);
> +	mxsfb_write_register(0x13, 0x50);
> +	mxsfb_write_register(0x14, 0x4f58);
> +	mxsfb_write_register(0x30, 0);
> +	mxsfb_write_register(0x31, 0xdb);
> +	mxsfb_write_register(0x32, 0);
> +	mxsfb_write_register(0x33, 0);
> +	mxsfb_write_register(0x34, 0xdb);
> +	mxsfb_write_register(0x35, 0);
> +	mxsfb_write_register(0x36, 0xaf);
> +	mxsfb_write_register(0x37, 0);
> +	mxsfb_write_register(0x38, 0xdb);
> +	mxsfb_write_register(0x39, 0);
> +	mxsfb_write_register(0x50, 0);
> +	mxsfb_write_register(0x51, 0x705);
> +	mxsfb_write_register(0x52, 0xe0a);
> +	mxsfb_write_register(0x53, 0x300);
> +	mxsfb_write_register(0x54, 0xa0e);
> +	mxsfb_write_register(0x55, 0x507);
> +	mxsfb_write_register(0x56, 0);
> +	mxsfb_write_register(0x57, 3);
> +	mxsfb_write_register(0x58, 0x90a);
> +	mxsfb_write_register(0x59, 0xa09);
> +	mdelay(30);
> +	mxsfb_write_register(7, 0x1017);
> +	mdelay(40);
> +	mxsfb_write_register(0x36, 0xaf);
> +	mxsfb_write_register(0x37, 0);
> +	mxsfb_write_register(0x38, 0xdb);
> +	mxsfb_write_register(0x39, 0);
> +	mxsfb_write_register(0x20, 0);
> +	mxsfb_write_register(0x21, 0);

This is terrible. WHy don't you use an array for these values, and
then a simple loop to program the (register,value) pairs?  That would
not only be much easier to read and to maintain but also result in
smaller code...


> +#define CONFIG_MACH_TYPE	0xffffffff

NAK.  Please use a proper value.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
God made the integers; all else is the work of Man.       - Kronecker