[U-Boot] [RFC] Add 'led' command

[Mike Frysinger]

On Friday, November 05, 2010 01:50:36 Jason Kridner wrote:
> +int do_led ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[] )

much of the style of this code is broken.  and i cant imagine this code 
compiling warning free with current master.

no spaces around the paren, and the argv has been constified.

also, this should be marked static

> +	if ((argc != 3)){

space before the brace and useless set of paren here

> +		printf("Usage:\n%s\n", cmdtp->usage);
> +		return 1;

return cmd_usage(cmdtp);

> +	if (strcmp(argv[2], "off") == 0) {
> +		state = 0;
> +	} else if (strcmp(argv[2], "on") == 0) {
> +		state = 1;

i could have sworn we had a helper somewhere to handle "boolean strings" ...

> +		printf ("Usage:\n%s\n", cmdtp->usage);
> +		return 1;

return cmd_usage(cmdtp);

> +#if defined(STATUS_LED_BIT) && defined(CONFIG_BOARD_SPECIFIC_LED)
> +	if (strcmp(argv[1], "0") == 0) {
> +		mask = STATUS_LED_BIT;
> +		__led_set(mask, state);
> +	}
> +	else
> +#endif
> +#if defined(STATUS_LED_BIT1) && defined(CONFIG_BOARD_SPECIFIC_LED)
> +	if (strcmp(argv[1], "1") == 0) {
> +		mask = STATUS_LED_BIT1;
> +		__led_set(mask, state);
> +	}
> +	else
> +#endif
> +#if defined(STATUS_LED_BIT2) && defined(CONFIG_BOARD_SPECIFIC_LED)
> +	if (strcmp(argv[1], "2") == 0) {
> +		mask = STATUS_LED_BIT2;
> +		__led_set(mask, state);
> +	}
> +	else
> +#endif
> +#if defined(STATUS_LED_BIT3) && defined(CONFIG_BOARD_SPECIFIC_LED)
> +	if (strcmp(argv[1], "3") == 0) {
> +		mask = STATUS_LED_BIT3;
> +		__led_set(mask, state);
> +	}
> +	else
> +#endif

i dont know why you need the mask variable here at all

also, these #ifdef trees scream for some sort of unification

> +	} else {
> +		printf ("Usage:\n%s\n", cmdtp->usage);
> +		return 1;

return cmd_usage(cmptp);

> +

files should not have trailing new lines
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101109/4f9a2022/attachment.pgp