Dear Hannes,
In message <1391615224-26493-1-git-send-email-oe5...@oevsv.at> you wrote:
> Adds support for Bernecker & Rainer Industrieelektronik GmbH KWB
> Motherboard, using TI's AM3352 SoC.
>
> Most of code is derived from TI's AM335x_EVM
>
> Signed-off-by: Hannes Petermaier <oe5...@oevsv.at>
> Cc: tr...@ti.com
> ---
> board/BuR/bur_kwb/Makefile | 16 ++
> board/BuR/bur_kwb/am335xScreen.c | 548
> ++++++++++++++++++++++++++++++++++++++
> board/BuR/bur_kwb/am335xScreen.h | 45 ++++
> board/BuR/bur_kwb/board.c | 522 ++++++++++++++++++++++++++++++++++++
> board/BuR/bur_kwb/board.h | 18 ++
> board/BuR/bur_kwb/mux.c | 213 +++++++++++++++
> board/BuR/bur_kwb/u-boot.lds | 101 +++++++
See previous comments. Repeating the "bur_" part in the board
directory name is redundant; I recommend to drop that.
> + * History
> + * =======
> + * 17.12.2013 hpm created
Please drop this. Please fix globally.
> +#define DEBUG
> +#ifdef DEBUG
> +#define DBG(...) printf(__VA_ARGS__)
> +#else
> +#define DBG(...)
> +#endif /* DEBUG */
We already have debug(); please use existing standard facilities.
> + if ('\r' == *str) {
...
> + } else if ('\n' == *str) {
Please avoid this reversed notation style. Please fix globally.
...
> +}
> +int drawrecticle(unsigned int x, unsigned int y,
> + unsigned int w, unsigned int h, unsigned int color)
> +{
CodingStyle says: "In source files, separate functions with one blank
line." Please fix globally.
> + setPix(&screen, j, i, color);
We do not allow CamelCase identifiers. Please fix globally.
> +int prints(const char *fmt, ...)
...
> +int printsxy(unsigned int x, unsigned int y, const char *fmt, ...)
etc.
BTW - are you not reinventing the wheel here? What about functions
like video_drawstring(), video_putchar(), ... ?
> index 0000000..5a1b15f
> --- /dev/null
> +++ b/board/BuR/bur_kwb/board.c
> @@ -0,0 +1,522 @@
Pretty significant parts of this file appear to be identical to code
in board/BuR/bur_tseries/board.c as added by your earlier patch.
Please factor out such common code into a common/ directory so it can
be shared across boards.
> + * History
> + * =======
> + * xx.10.2013 hpm - created, adopted from ti_am335x_evm
> + * 06.12.2013 hpm - setting MPU_PLL to 600 MHz (as required for
> + * industrial)
> + * - implemented simple boot-mode detection
> + * - reduced resetpulse for USB2SD controller
> + * from 5ms to 1ms
> + * 10.12.2013 hpm - switch OFF LCD-Screen within SPL-Stage
> + * (due to invalid 3V3 pullup resistor)
> + * - powerUP 3V3 via I2C-Resetcontroller within SPL-Stage
> + * 16.12.2013 hpm - V1.10: changed bootaddr of VxWorks from
> + * 0x82000000 to 0x80100000
> + * 24.12.2013 hpm - V1.11: added LCD-support
> + * 30.01.2014 hpm - V2.00: ported to new U-Boot (2014) sources
As mentioned before: please drop globally!
> + printf("echo setting PMIC Register in the RTC.\n");
> + *(unsigned int *)0x44e3e098 = 0x1f010;
> + printf("read back PMIC Register: 0x%08x\n",
> + *(unsigned int *)0x44e3e098);
> +
> + year = *(unsigned int *)0x44e3e014;
> + mon = *(unsigned int *)0x44e3e010;
> + d = *(unsigned int *)0x44e3e00C;
> + h = *(unsigned int *)0x44e3e008;
> + min = *(unsigned int *)0x44e3e004;
Please use proper I/O accessors to access hardware registers, and
declare a proper C struct to describe the register layout. Please fix
globally.
> +static struct cpsw_slave_data cpsw_slaves[] = {
> + {
> + .slave_reg_ofs = 0x208,
> + .sliver_reg_ofs = 0xd80,
...
> + .slave_reg_ofs = 0x308,
> + .sliver_reg_ofs = 0xdc0,
Would it make sense to provide symboolic names for such magic numbers?
And how about some comments to explain the actual settings?
> diff --git a/board/BuR/bur_kwb/board.h b/board/BuR/bur_kwb/board.h
> new file mode 100644
> index 0000000..def41aa
> --- /dev/null
> +++ b/board/BuR/bur_kwb/board.h
> @@ -0,0 +1,18 @@
> +/*
> + * board.h
> + *
> + * BUR-KWB boards information header
> + *
> + * Copyright (C) 2013, Bernecker & Rainer Industrieelektronik GmbH -
> + * http://www.br-automation.com
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef _BOARD_H_
> +#define _BOARD_H_
> +
> +void enable_uart0_pin_mux(void);
> +void enable_i2c0_pin_mux(void);
> +void enable_board_pin_mux(void);
This file appears to be identical to board/BuR/bur_tseries/board.h as
added by your earlier patch. Please use a common header file instead.
> diff --git a/board/BuR/bur_kwb/mux.c b/board/BuR/bur_kwb/mux.c
> new file mode 100644
> index 0000000..da4b9f3
> --- /dev/null
> +++ b/board/BuR/bur_kwb/mux.c
> @@ -0,0 +1,213 @@
> +static struct module_pin_mux uart0_pin_mux[] = {
> + /* UART0_CTS */
> + {OFFSET(uart0_ctsn), (MODE(7) | PULLUDEN | PULLUP_EN | RXACTIVE)},
> + /* UART0_RXD */
> + {OFFSET(uart0_rxd), (MODE(0) | PULLUDEN | PULLUP_EN | RXACTIVE)},
> + /* UART0_TXD */
> + {OFFSET(uart0_txd), (MODE(0) | PULLUDEN)},
> + {-1},
> +};
Again, there is lots of duplicated code. Please factor out common
parts. Please fix globally.
> diff --git a/board/BuR/bur_kwb/u-boot.lds b/board/BuR/bur_kwb/u-boot.lds
> new file mode 100644
> index 0000000..021f869
> --- /dev/null
> +++ b/board/BuR/bur_kwb/u-boot.lds
Not needed? Drop??
> diff --git a/include/configs/bur_kwb.h b/include/configs/bur_kwb.h
> new file mode 100644
> index 0000000..66048dd
> --- /dev/null
> +++ b/include/configs/bur_kwb.h
Large duplicated sections again...
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
If the facts don't fit the theory, change the facts.
-- Albert Einstein
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
