From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Thu, 21 Apr 2011 00:04:48 +0200
Subject: [U-Boot] [PATCH] Add 'led' command
In-Reply-To: <1299013329-29931-1-git-send-email-jkridner@beagleboard.org>
References: <1299013329-29931-1-git-send-email-jkridner@beagleboard.org>
Message-ID: <20110420220448.47CDF152160@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Jason Kridner,

In message <1299013329-29931-1-git-send-email-jkridner@beagleboard.org> you wrote:
> This patch allows any board implementing the coloured LED API
> to control the LEDs from the console.
> 
> led [green | yellow | red | all ]  [ on | off ]
> 
> or
> 
> led [ 1 | 2 | 3 | all ]  [ on | off ]

I still wonder if such a patch will help to get rid of the ton of LEd
drivers we already have in U-Boot, or if it just adds another such
interface?

Which drivers ("led.c" files) will be obsoleted by this patch?


If it is intended to be a generic interface - how can this then be
combined with the status_led.c driver? 

The configuration "green", "yellow", "red" seems to be very specific
to me - I guess this applies just to very few boards?

...
> +struct led_tbl_s {
> +	char		*string;	/* String for use in the command */
> +	led_id_t	mask;		/* Mask used for calling __led_set() */
> +	void		(*on)(void);	/* Optional fucntion for turning LED on */
> +	void		(*off)(void);	/* Optional fucntion for turning LED on */
> +};
> +
> +typedef struct led_tbl_s led_tbl_t;
> +
> +static const led_tbl_t led_commands[] = {
> +#ifdef CONFIG_BOARD_SPECIFIC_LED
> +#ifdef STATUS_LED_BIT
> +	{ "0", STATUS_LED_BIT, NULL, NULL },
> +#endif
> +#ifdef STATUS_LED_BIT1
> +	{ "1", STATUS_LED_BIT1, NULL, NULL },
> +#endif
> +#ifdef STATUS_LED_BIT2
> +	{ "2", STATUS_LED_BIT2, NULL, NULL },
> +#endif
> +#ifdef STATUS_LED_BIT3
> +	{ "3", STATUS_LED_BIT3, NULL, NULL },
> +#endif

What are these "status bits" good for when they have no actual
handlers attached?  Where are they actually used?


> +#ifdef STATUS_LED_GREEN
> +	{ "green", STATUS_LED_GREEN, green_LED_off, green_LED_on },
> +#endif
> +#ifdef STATUS_LED_YELLOW
> +	{ "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on },
> +#endif
> +#ifdef STATUS_LED_RED
> +	{ "red", STATUS_LED_RED, red_LED_off, red_LED_on },
> +#endif
> +#ifdef STATUS_LED_BLUE
> +	{ "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on },
> +#endif

We do not allow CamelCase identifiers (like "green_LED_off").


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: wd at denx.de
Sometimes a man will tell his bartender things he'll never tell his doctor.
	-- Dr. Phillip Boyce, "The Menagerie" ("The Cage"),
	   stardate unknown.