Hi Wolfgang,
> 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?
Nothing escapes the eye of an eagle ;)
>> +}
>> +
>> 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).
Done.
>> + 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.
Fixed.
>> +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) ?
Indeed we should. But here we go, I can understand, why this wasn't
done the first time around. The new code looks like this:
/* reset PSC */
out_8(&psc->command, PSC_SEL_MODE_REG_1);
/* select clock sources */
out_be16(&psc->psc_clock_select, 0);
baseclk = (gd->ipb_clk + 16) / 32;
/* switch to UART mode */
out_be32(&psc->sicr, 0);
/* configure parity, bit length and so on */
out_8(&psc->mode, PSC_MODE_8_BITS | PSC_MODE_PARNONE);
out_8(&psc->mode, PSC_MODE_ONE_STOP);
/* set up UART divisor */
div = (baseclk + (baudrate / 2)) / baudrate;
out_8(&psc->ctur, (div >> 8) & 0xff);
out_8(&psc->ctlr, div & 0xff);
/* disable all interrupts */
out_be16(&psc->psc_imr, 0);
/* reset and enable Rx/Tx */
out_8(&psc->command, PSC_RST_RX);
out_8(&psc->command, PSC_RST_TX);
out_8(&psc->command, PSC_RX_ENABLE | PSC_TX_ENABLE);
Look at the wonderful mix of out_{8,be16,be32}. What a heart refreshing
break from the monotonic coding routine :(
Actully this is due to to mpc5xxx.h:
struct mpc5xxx_psc {
volatile u8 mode; /* PSC + 0x00 */
volatile u8 reserved0[3];
union { /* PSC + 0x04 */
volatile u16 status;
volatile u16 clock_select;
} sr_csr;
#define psc_status sr_csr.status
#define psc_clock_select sr_csr.clock_select
volatile u16 reserved1;
volatile u8 command; /* PSC + 0x08 */
volatile u8 reserved2[3];
union { /* PSC + 0x0c */
volatile u8 buffer_8;
volatile u16 buffer_16;
volatile u32 buffer_32;
} buffer;
#define psc_buffer_8 buffer.buffer_8
#define psc_buffer_16 buffer.buffer_16
#define psc_buffer_32 buffer.buffer_32
union { /* PSC + 0x10 */
volatile u8 ipcr;
volatile u8 acr;
} ipcr_acr;
#define psc_ipcr ipcr_acr.ipcr
#define psc_acr ipcr_acr.acr
volatile u8 reserved3[3];
union { /* PSC + 0x14 */
volatile u16 isr;
volatile u16 imr;
} isr_imr;
Git-blame points the finger to someone called 'wd'. Maybe he knows
about the advantages of such structure definitions?
On second check the corresponding header file in Linux (mpc52xx_uart.h) has
stuff like this introduced by Kumar Gala, maybe he can enlighten me?
To be honest, I wouldn't even dare accessing memory mapped registers in
the non-native size, but maybe I'm missing something here.
>> +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!!
I won't, I PROMISE.
> Did this actually pass a compile test?
Heck, do you think I dare send a patch without compiling?
>> + 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.
I believe I succeeded.
>> + 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.
The switch to using struct NS16650 allowed for nicer layout without
violating line length.
>> + 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?
Nope.
>> +#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?
Yep.
>> +}
>> +
>> +static void buzzer_turn_off(void)
>> +{
>> + struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)(BUZZER_GPT);
>> + gpt->emsr = 0;
>
> Accessor functions?
Again, not a question.
>> +}
>> +
>> +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.
Fixed.
>> + 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.
What insolence! Fixed.
>> + /* 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.
Fixed.
>> + 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.
Personally I do not think that the new version is any more readable. To
fix the "problem", I can split the string or violate indentation. As
the first seems very unattractive from a reading point of view, I
actually willingly decided for the latter. I hope you don't criticize
that in the next round.
>> +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.
Again I believe the new version is harder to read, but I don't really
care.
Cheers
Detlev
--
We have a live-manual. It's called emacs-de...@gnu.org.
You can stick to just reading it, but you can skip to a specific chapter
by simply sending an email asking for it ;-)
-- Stefan Monnier
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
