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
