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
