Dear Detlev Zundel, In message <1237914158-15693-6-git-send-email-...@denx.de> you wrote: > This patch adds advanced diagnosis functions for the inka4x0 board. > > Signed-off-by: Andreas Pfefferle <a...@denx.de> > Signed-off-by: Detlev Zundel <a...@denx.de> ... > + extern int inkadiag_init_r (void); > + /* > + * The command table used for the subcommands of inkadiag > + * needs to be relocated manually. > + */
Incorrect multiline comment style? > +} > + > int misc_init_f (void) > { > char tmp[10]; > diff --git a/board/inka4x0/inkadiag.c b/board/inka4x0/inkadiag.c > new file mode 100644 > index 0000000..bdbf652 ... > +#define LED_HIGH(NUM) \ > + do { \ > + out_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE, \ > + in_be32((unsigned *)MPC5XXX_GPT##NUM##_ENABLE) | 0x10); \ > + } while (0) Please use: setbits_be32 ((unsigned *)MPC5XXX_GPT##NUM##_ENABLE, 0x10); instead (and so on in the rest of the code). > + struct mpc5xxx_gpio *gpio = (struct mpc5xxx_gpio *)MPC5XXX_GPIO; > + if (which == 1) { > + /* drawer1 */ > + if (state) { > + gpio->simple_dvo &= ~0x1000; > + udelay(1); > + gpio->simple_dvo |= 0x1000; > + } else { > + gpio->simple_dvo |= 0x1000; > + udelay(1); > + gpio->simple_dvo &= ~0x1000; > + } > + } > + if (which == 2) { > + /* drawer 2 */ > + if (state) { > + gpio->simple_dvo &= ~0x2000; > + udelay(1); > + gpio->simple_dvo |= 0x2000; > + } else { > + gpio->simple_dvo |= 0x2000; > + udelay(1); > + gpio->simple_dvo &= ~0x2000; Please use accessor functions to write to I/O ports. ... > +static int ser_init(volatile struct mpc5xxx_psc *psc, int baudrate) > +{ > + unsigned long baseclk; > + int div; > + > + /* reset PSC */ > + psc->command = PSC_SEL_MODE_REG_1; Should we not use accessor function to access the device registers (here and in the following code) ? > +static int do_inkadiag_serial(cmd_tbl_t *cmdtp, int flag, int argc, > + char *argv[]) { > + if (argc < 5) { > + cmd_usage(cmdtp); > + return 1; > + } > + > + argc--; > + argv++; > + > + unsigned int num = simple_strtol(argv[0], NULL, 0); Please NEVER declare variables right in the middle of the code!! Did this actually pass a compile test? > + if (num < 0 || num > 11) { > + printf("invalid argument for num: %d\n", num); > + return -1; > + } > + > + unsigned int mode = simple_strtol(argv[1], NULL, 0); > + > + int combrd = 0; > + int baudrate = simple_strtoul(argv[2], NULL, 10); > + int i; Ditto. ... > + if (mode & 4) { > + /* set data terminal ready */ > + serial_out(num, UART_MCR, UART_MCR_DTR); > + udelay(10); > + /* check data set ready and carrier detect */ > + if ((serial_in(num, UART_MSR) & > + (UART_MSR_DSR | UART_MSR_DCD)) > + != (UART_MSR_DSR | UART_MSR_DCD)) > + return -1; > + } > + > + /* write each message-character, read it back, and display it */ > + int i, len = strlen(argv[3]); Ditto. Please make sure to fix this globally. > + for (i = 0; i < len; ++i) { > + int j = 0; > + while ((serial_in(num, UART_LSR) & UART_LSR_THRE) == > + 0x00) { This is actually difficult to read. Please reconsider indentation / line wrapping / adding a blank line. > + if (j++ > CONFIG_SYS_HZ) > + break; > + udelay(10); > + } > + serial_out(num, UART_TX, argv[3][i]); > + j = 0; > + while ((serial_in(num, UART_LSR) & UART_LSR_DR) == > + 0x00) { Ditto. > + if (j++ > CONFIG_SYS_HZ) > + break; > + udelay(10); > + } > + unsigned char data = serial_in(num, UART_RX); > + printf("%c", data); > + } > + printf("\n\n"); > + serial_out(num, UART_MCR, 0x00); > + } else { > + int address = 0; > + int irq; > + > + switch (num) { > + case 8: > + address = MPC5XXX_PSC6; > + irq = MPC5XXX_PSC6_IRQ; > + break; > + case 9: > + address = MPC5XXX_PSC3; > + irq = MPC5XXX_PSC3_IRQ; > + break; > + case 10: > + address = MPC5XXX_PSC2; > + irq = MPC5XXX_PSC2_IRQ; > + break; > + case 11: > + address = MPC5XXX_PSC1; > + irq = MPC5XXX_PSC1_IRQ; > + break; > + } "irq" seems to be unused (accept for the assignments) in this code. Wasn't there a compiler warning? > + volatile struct mpc5xxx_psc *psc = > + (struct mpc5xxx_psc *)address; Variable declaration in the middle of the code - see above. > + ser_init(psc, simple_strtol(argv[2], NULL, 0)); > + if (mode & 2) { > + /* set request to send */ > + out_8(&psc->op0, PSC_OP0_RTS); > + udelay(10); > + /* check clear to send */ > + if ((in_8(&psc->ip) & PSC_IPCR_CTS) == 0) > + return -1; > + } > + int i, len = strlen(argv[3]); > + for (i = 0; i < len; ++i) { > + ser_putc(psc, argv[3][i]); > + printf("%c", ser_getc(psc)); > + } > + printf("\n\n"); > + } > + return 0; > +} > + > +#define BUZZER_GPT (MPC5XXX_GPT + 0x60) /* GPT6 */ > +static void buzzer_turn_on(unsigned int freq) > +{ > + struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)(BUZZER_GPT); > + > + const u32 prescale = gd->ipb_clk / freq / 128; > + const u32 count = 128; > + const u32 width = 64; > + > + gpt->cir = (prescale << 16) | count; > + gpt->pwmcr = width << 16; > + gpt->emsr = 3; /* Timer enabled for PWM */ Accessor functions? > +} > + > +static void buzzer_turn_off(void) > +{ > + struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)(BUZZER_GPT); > + gpt->emsr = 0; Accessor functions? > +} > + > +static int do_inkadiag_buzzer(cmd_tbl_t *cmdtp, int flag, int argc, > + char *argv[]) { > + > + if (argc != 3) { > + cmd_usage(cmdtp); > + return 1; > + } > + > + argc--; > + argv++; > + > + unsigned int period = simple_strtol(argv[0], NULL, 0); Declaration in the middle of the code. > + if (!period) > + printf("Zero period is senseless\n"); > + argc--; > + argv++; > + > + unsigned int freq = simple_strtol(argv[0], NULL, 0); Declaration in the middle of the code. > + /* avoid zero prescale in buzzer_turn_on() */ > + if (freq > gd->ipb_clk / 128) { > + printf("%dHz exceeds maximum (%ldHz)\n", freq, > + gd->ipb_clk / 128); > + } else if (!freq) > + printf("Zero frequency is senseless\n"); > + else > + buzzer_turn_on(freq); > + > + clear_ctrlc(); > + int prev = disable_ctrlc(0); > + > + printf("Buzzing for %d ms. Type ^C to abort!\n\n", period); > + > + int i = 0; Declaration in the middle of the code. > + while (!ctrlc() && (i++ < CONFIG_SYS_HZ)) > + udelay(period); > + > + clear_ctrlc(); > + disable_ctrlc(prev); > + > + buzzer_turn_off(); > + > + return 0; > +} > + > +static int do_inkadiag_help(cmd_tbl_t *cmdtp, int flag, int argc, char > *argv[]); > + > +cmd_tbl_t cmd_inkadiag_sub[] = { > + U_BOOT_CMD_MKENT(io, 1, 1, do_inkadiag_io, "read digital input", > + "<drawer1|drawer2|other> [value] - get or set > specified signal\n"), > + U_BOOT_CMD_MKENT(serial, 4, 1, do_inkadiag_serial, "test serial port", > + "<num> <mode> <baudrate> <msg> - test uart num > [0..11] in mode\n" > + "and baudrate with msg\n"), > + U_BOOT_CMD_MKENT(buzzer, 2, 1, do_inkadiag_buzzer, "activate buzzer", > + "<period> <freq> - turn buzzer on for period ms with > freq hz\n"), > + U_BOOT_CMD_MKENT(help, 4, 1, do_inkadiag_help, "get help", > + "[command] - get help for command\n"), > +}; Lines too long. > +static int do_inkadiag_help(cmd_tbl_t *cmdtp, int flag, int argc, char > *argv[]) { > + extern int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * > cmdtp, > + int flag, int argc, char *argv[]); > + /* do_help prints command name - we prepend inkadiag to our > subcommands! */ Lines too long. 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 Substitute "damn" every time you're inclined to write "very"; your editor will delete it and the writing will be just as it should be. - Mark Twain _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot