[U-Boot] [PATCH ARM 2/3] s3c24x0 code style changes

[Wolfgang Denk]

Dear "kevin.morfitt at fearnside-systems.co.uk",

In message <4B2548F7.2060605 at fearnside-systems.co.uk> you wrote:
> Cleans up the s3c24x0 header files by changing the upper case members
> of the s3c24x0 register structures to lower case and changing all code
> that uses these register structures.
> 
> Signed-off-by: Kevin Morfitt <kevin.morfitt at fearnside-systems.co.uk>
> ---
>  board/mpl/vcma9/vcma9.c           |  264 ++++++++++---------
>  board/mpl/vcma9/vcma9.h           |   91 +++---
>  board/samsung/smdk2400/smdk2400.c |   53 ++--
>  board/samsung/smdk2410/smdk2410.c |   85 +++---
>  board/sbc2410x/sbc2410x.c         |  131 +++++-----
>  board/trab/cmd_trab.c             |  547 +++++++++++++++++-------------------
>  board/trab/rs485.c                |   92 ++++---
>  7 files changed, 626 insertions(+), 637 deletions(-)
> 
> diff --git a/board/mpl/vcma9/vcma9.c b/board/mpl/vcma9/vcma9.c
> index 1835677..84338eb 100644
> --- a/board/mpl/vcma9/vcma9.c
> +++ b/board/mpl/vcma9/vcma9.c
> @@ -39,32 +39,31 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define FCLK_SPEED 1
>  
>  #if FCLK_SPEED==0		/* Fout = 203MHz, Fin = 12MHz for Audio */
> -#define M_MDIV	0xC3
> -#define M_PDIV	0x4
> -#define M_SDIV	0x1
> +	#define M_MDIV	0xC3
> +	#define M_PDIV	0x4
> +	#define M_SDIV	0x1
>  #elif FCLK_SPEED==1		/* Fout = 202.8MHz */
> -#define M_MDIV	0xA1
> -#define M_PDIV	0x3
> -#define M_SDIV	0x1
> +	#define M_MDIV	0xA1
> +	#define M_PDIV	0x3
> +	#define M_SDIV	0x1
>  #endif
>  
>  #define USB_CLOCK 1
>  
>  #if USB_CLOCK==0
> -#define U_M_MDIV	0xA1
> -#define U_M_PDIV	0x3
> -#define U_M_SDIV	0x1
> +	#define U_M_MDIV	0xA1
> +	#define U_M_PDIV	0x3
> +	#define U_M_SDIV	0x1
>  #elif USB_CLOCK==1
> -#define U_M_MDIV	0x48
> -#define U_M_PDIV	0x3
> -#define U_M_SDIV	0x2
> +	#define U_M_MDIV	0x48
> +	#define U_M_PDIV	0x3
> +	#define U_M_SDIV	0x2
>  #endif

NAK.  C preprocessor lines always start in the first column. Do not do
this.


>  int board_init(void)
>  {
> -	struct s3c24x0_clock_power * const clk_power =
> -					s3c24x0_get_base_clock_power();
> -	struct s3c24x0_gpio * const gpio = s3c24x0_get_base_gpio();
> +	struct s3c24x0_clock_power *const clk_power =
> +	    s3c24x0_get_base_clock_power();

Indentation not by TAB.


>  	/* set up the I/O ports */
> -	gpio->GPACON = 0x007FFFFF;
> -	gpio->GPBCON = 0x002AAAAA;
> -	gpio->GPBUP = 0x000002BF;
> -	gpio->GPCCON = 0xAAAAAAAA;
> -	gpio->GPCUP = 0x0000FFFF;
> -	gpio->GPDCON = 0xAAAAAAAA;
> -	gpio->GPDUP = 0x0000FFFF;
> -	gpio->GPECON = 0xAAAAAAAA;
> -	gpio->GPEUP = 0x000037F7;
> -	gpio->GPFCON = 0x00000000;
> -	gpio->GPFUP = 0x00000000;
> -	gpio->GPGCON = 0xFFEAFF5A;
> -	gpio->GPGUP = 0x0000F0DC;
> -	gpio->GPHCON = 0x0028AAAA;
> -	gpio->GPHUP = 0x00000656;
> +	writel(0x007FFFFF, &gpio->gpacon);
> +	writel(0x002AAAAA, &gpio->gpbcon);
> +	writel(0x000002BF,  &gpio->gpbup);
> +	writel(0xAAAAAAAA, &gpio->gpccon);
> +	writel(0x0000FFFF,  &gpio->gpcup);
> +	writel(0xAAAAAAAA, &gpio->gpdcon);
> +	writel(0x0000FFFF,  &gpio->gpdup);
> +	writel(0xAAAAAAAA, &gpio->gpecon);
> +	writel(0x000037F7,  &gpio->gpeup);
> +	writel(0x00000000, &gpio->gpfcon);
> +	writel(0x00000000, &gpio->gpfup);
> +	writel(0xFFEAFF5A, &gpio->gpgcon);
> +	writel(0x0000F0DC,  &gpio->gpgup);
> +	writel(0x0028AAAA, &gpio->gphcon);
> +	writel(0x00000656,  &gpio->gphup);

Such a change should introduce symbolic constants for all thes magic
numbers, and add comments what they actually do.

>  #if defined(CONFIG_CMD_NAND)
> -extern ulong
> -nand_probe(ulong physadr);
> -
> +extern ulong nand_probe(ulong physadr);

Please move proprtypes to appropriate header files.

...
> -    NF_Conf((1<<15)|(0<<14)|(0<<13)|(1<<12)|(1<<11)|(TACLS<<8)|(TWRPH0<<4)|(TWRPH1<<0));
> -    /*nand->NFCONF = (1<<15)|(1<<14)|(1<<13)|(1<<12)|(1<<11)|(TACLS<<8)|(TWRPH0<<4)|(TWRPH1<<0); */
> -    /* 1  1    1     1,   1      xxx,  r xxx,   r xxx */
> -    /* En 512B 4step ECCR nFCE=H tACLS   tWRPH0   tWRPH1 */
> +	nf_conf((1 << 15) | (0 << 14) | (0 << 13) | (1 << 12) | (1 << 11) |
> +		(TACLS << 8) | (TWRPH0 << 4) | (TWRPH1 << 0));
> +	/*nand->NFCONF = (1<<15)|(1<<14)|(1<<13)|(1<<12)|(1<<11)|(TACLS<<8)|(TWRPH0<<4)|(TWRPH1<<0); */

Line too long. Please check globally.

>  #ifdef DEBUG
> -	printf("NAND flash probing at 0x%.8lX\n", (ulong)nand);
> +	printf("NAND flash probing at 0x%.8lX\n", (ulong) nand);
>  #endif
> -	printf ("%4lu MB\n", nand_probe((ulong)nand) >> 20);
> +	printf("%4lu MB\n", nand_probe((ulong) nand) >> 20);

No additional spaces here.

> -static inline void NF_WaitRB(void)
> +static inline void nf_waitrb(void)
>  {
> -	struct s3c2410_nand * const nand = s3c2410_get_base_nand();
> +	struct s3c2410_nand *const nand = s3c2410_get_base_nand();
>  
> -	while (!(nand->NFSTAT & (1<<0)));
> +	while (!(readl(&nand->nfstat) & (1 << 0)));

Please write as:

	while (!(readl(&nand->nfstat) & 1))
		;


> @@ -105,16 +106,16 @@ int dram_init (void)
>  static int key_pressed(void)
>  {
>  	int rc;
> -	if (1) {	/* check for button push here, now just return 1 */
> +	if (1) {		/* check for button push here, now just return 1 */

Line too long. Please check globally.

Hey, why do you change this line at all? It was OK before!

>  #ifdef CONFIG_CMD_NET
> -int board_eth_init(bd_t *bis)
> +int board_eth_init(bd_t * bis)

No additional spaces here. Please check globally.

...
> -static inline void delay (unsigned long loops)
> +static inline void delay(unsigned long loops)
>  {
> -	__asm__ volatile ("1:\n"
> -	  "subs %0, %1, #1\n"
> -	  "bne 1b":"=r" (loops):"0" (loops));
> +	__asm__ volatile("1:\n"
> +			  "subs %0, %1, #1\n" "bne 1b":"=r" (loops):"0"(loops));

Line too long, and much less readable.  Don't do this!!!


I give up here.

The same comments apply in many p[laces, please make sure to check
globally and fix all ocurrences.

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
In my experience the best way to get something done  is to give it to
someone who is busy.               - Terry Pratchett, _Going_Postal_