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

Reply via email to