Dear "kevin.morf...@fearnside-systems.co.uk",

In message <4b2548f7.2060...@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.morf...@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: w...@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_
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to