Dear ap@denx.de,
In message 1227214731-19542-1-git-send-email-ap@denx.de you wrote:
RnJvbTogQW5kcmVhcyBQZmVmZmVybGUgPGFwQGRlbnguZGU+CgpUaGlzIHBhdGNoIGFkZHMgZGlh Z25vc2lzIGZ1bmN0aW9ucyBmb3IgdGhlIGJ1enplciwgVUFSVHMgYW5kIGRpZ2l0YWwKSU9zIG9u IGlua2E0eDAgaGFyZHdhcmUuCsKgIMKgClNpZ25lZC1vZmYtYnk6IEFuZHJlYXMgUGZlZmZlcmxl IDxhcEBkZW54LmRlPgotLS0KIGJvYXJkL2lua2E0eDAvTWFrZWZpbGUgICB8ICAgIDIgKy0KIGJv YXJkL2lua2E0eDAvaW5rYWRpYWcuYyB8ICA1MTUgKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKwogMiBmaWxlcyBjaGFuZ2VkLCA1MTYgaW5zZXJ0aW9ucygrKSwg MSBkZWxldGlvbnMoLSkKIGNyZWF0ZSBtb2RlIDEwMDY0NCBib2FyZC9pbmthNHgwL2lua2FkaWFn LmMKCmRpZmYgLS1naXQgYS9ib2FyZC9pbmthNHgwL01ha2VmaWxlIGIvYm9hcmQvaW5rYTR4MC9N YWtlZmlsZQppbmRleCA0NDJlMmQwLi4yMjY0ZGFlIDEwMDY0NAotLS0gYS9ib2FyZC9pbmthNHgw L01ha2VmaWxlCisrKyBiL2JvYXJkL2lua2E0eDAvTWFrZWZpbGUKQEAgLTI1LDcgKzI1LDcgQEAg aW5jbHVkZSAkKFRPUERJUikvY29uZmlnLm1rCiAKIExJQgk9ICQob2JqKWxpYiQoQk9BUkQpLmEK IAotQ09CSlMJOj0gJChCT0FSRCkubworQ09CSlMJOj0gJChCT0FSRCkubyAgaW5rYWRpYWcubwog CiBTUkNTCTo9ICQoU09CSlM6Lm89LlMpICQoQ09CSlM6Lm89LmMpCiBPQkpTCTo9ICQoYWRkcHJl Zml4ICQob2JqKSwkKENPQkpTKSkKZGlmZiAtLWdpdCBhL2JvYXJkL2lua2E0eDAvaW5rYWRpYWcu
...
Please do not post base64 encoded messages!
Any further such postings will be ignored.
This patch adds diagnosis functions for the buzzer, UARTs and digital
Such hardware test code should be implemented as part of the POST system, i. e. please create a post/board/inka4x0/ directory for it, and make it part of the POST.
diff --git a/board/inka4x0/inkadiag.c b/board/inka4x0/inkadiag.c new file mode 100644 index 0000000..6194b58 --- /dev/null +++ b/board/inka4x0/inkadiag.c
...
+#define LED_HIGH(NUM)\ +out_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE,\
in_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE) | 0x10)+#define LED_LOW(NUM)\ +out_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE,\
in_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE) & ~0x10)
Please use "do { ...} while (0)" wrappers for these macros.
+static int io_helper(int argc, char *argv[]) +{
- unsigned int state = inka_digin_get_input();
- if (strcmp(argv[1], "drawer1") == 0) {
printf("exit code: 0x%X\n",(state & DIGIN_DRAWER_SW1) >> (ffs(DIGIN_DRAWER_SW1) - 1));- } else if (strcmp(argv[1], "drawer2") == 0) {
printf("exit code: 0x%X\n",(state & DIGIN_DRAWER_SW2) >> (ffs(DIGIN_DRAWER_SW2) - 1));- } else if (strcmp(argv[1], "other") == 0) {
printf("exit code: 0x%X\n",((state & DIGIN_KEYB_MASK) >> (ffs(DIGIN_KEYB_MASK) - 1))| ((state & DIGIN_TOUCHSCR_MASK) >>(ffs(DIGIN_TOUCHSCR_MASK) - 2)));- } else {
printf("Invalid argument: %s\n", argv[1]);return -1;- }
- return 0;
Please use the existing command parser code here and below.
+DECLARE_GLOBAL_DATA_PTR;
+static int ser_init(volatile struct mpc5xxx_psc *psc, int baudrate) +{
Maybe there should be some comments what these functions are doing?
+static void ser_putc(volatile struct mpc5xxx_psc *psc, const char c) +{
- /* Wait for last character to go. */
- int i = 0;
- while (!(psc->psc_status & PSC_SR_TXEMP) && (i++ < CONFIG_SYS_HZ))
udelay(10);- psc->psc_buffer_8 = c;
What exactly has CONFIG_SYS_HZ to do with that? I cannot understand this sort of magic.
+static int ser_getc(volatile struct mpc5xxx_psc *psc) +{
- /* Wait for a character to arrive. */
- int i = 0;
- while (!(psc->psc_status & PSC_SR_RXRDY) && (i++ < CONFIG_SYS_HZ))
udelay(10);
Ditto here.
- return psc->psc_buffer_8;
+}
+#define SERIAL_PORT_BASE (u_char *)0x80000000
+#define UART_RX 0 /* In: Receive buffer (DLAB=0) */ +#define UART_TX 0 /* Out: Transmit buffer (DLAB=0) */ +#define UART_DLL 0 /* Out: Divisor Latch Low (DLAB=1) */
+#define UART_LCR 3 /* Out: Line Control Register */ +#define UART_MCR 4 /* Out: Modem Control Register */
...
Lots of #defines right in the middle of a C file? That's uygly. Please move these in a separate header file (or at least at the begin of the C file).
+static unsigned char serial_in(unsigned char num, int offset) +{
- return in_8(SERIAL_PORT_BASE + (num << 3) + offset);
+}
+static void serial_out(unsigned char num, int offset, unsigned char value) +{
- out_8(SERIAL_PORT_BASE + (num << 3) + offset, value);
+}
The amount of comments in this file sucessfully avoids all risks of fatiguing the reader...
- if (strcmp(argv[2], "115200") == 0)
combrd = 1;- else if (strcmp(argv[2], "57600") == 0)
combrd = 2;- else if (strcmp(argv[2], "38400") == 0)
combrd = 3;- else if (strcmp(argv[2], "19200") == 0)
combrd = 6;- else if (strcmp(argv[2], "9600") == 0)
combrd = 12;- else if (strcmp(argv[2], "2400") == 0)
combrd = 48;- else if (strcmp(argv[2], "1200") == 0)
combrd = 96;- else if (strcmp(argv[2], "300") == 0)
combrd = 384;- else
printf("Invalid baudrate: %s", argv[2]);
Instead of calling strcmp() again and again, you could convert the number just once and use a switch. Or table lookup. See existing code.
if (mode & 1)/* turn on 'loopback' mode */serial_out(num, UART_MCR, UART_MCR_LOOP);else {
Multiline statements require braces.
/* establish the UART's operational parameters *//* set DLAB=1 */
Multiline comment style is
/* * establish the UART's operational parameters * set DLAB=1 */
volatile struct mpc5xxx_psc *psc =(struct mpc5xxx_psc *)address;
Indentation not by TAB.
- if (freq > gd->ipb_clk / 128)
printf("%dHz exceeds maximum (%dHz)\n", freq,gd->ipb_clk / 128);- else if (!freq)
Multiline statements require braces.
+static int TEST_FUNCTION(inkadiag) (cmd_tbl_t *cmdtp, int flag, int argc,
char *argv[]) {- switch (argc) {
- case 1:
PRINT_TEST_USAGE(inkadiag);
"break;" missing?
- default:
argc--;argv++;CHECK_TEST(io);CHECK_TEST(serial);CHECK_TEST(buzzer);- }
- PRINT_TEST_USAGE(inkadiag);
+}
+U_BOOT_CMD(inkadiag, 6, 1, TEST_FUNCTION(inkadiag),
"inkadiag- inka diagnosis\n","[inkadiag what ...]\n"" - perform a diagnosis on inka hardware\n""'inkadiag' performs hardware tests.\n\n""Without arguments, inkadiag prints a list of available tests.\n\n""To get detailed help information for specific tests you can type\n""'inkadiag what' with a test name 'what' as argument.\n");
Please no prose here - be terse, please.
Viele Grüße,
Wolfgang Denk