Dear Hannes,
In message 1391615224-26493-1-git-send-email-oe5hpm@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 oe5hpm@oevsv.at Cc: trini@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 -
- 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