Hello Tom!
To be honest, I have no convincing idea on how to move forward with this. My original intent was to get some GPIO LED support for the sunxi family of devices, namely my Banana Pi (sun7i/A20).
Following Simon's suggestion, I had a closer look at the new DM-based drivers/led/led_gpio.c. I agree that this is basically where I'd want to move my stuff to, instead of adding onto the drivers/misc/gpio_led.c code.
Unfortunately the driver model code currently has no connection at all to the actual "led" command handler in common/cmd_led.c. To make things worse, cmd_led.c declares the available LEDs 'statically' at compile time (via array initializers). Also CMD_LED is not available in Kconfig yet, and still uses the old .h includes.
Taking a closer look at the existing LED code, I get the impression that a lot of it has 'grown' historically - and probably should go through a major refactor / rewrite cycle. I have identified no less than three other "subsystems" dealing with LEDs (in addition to drivers/led/ and cmd_led.c):
* The "status led" code in drivers/misc/ deals with LED functionality using a "bit mask" approach, which in turns calls board-specific implementations of __led_init, __led_set and/or __led_toggle. It also offers limited support for 'software blinking' of LEDs with the status_led_tick() callback handler, but that only seems supported (i.e. actually used) on NIOS2 and PowerPC.
* drivers/misc/ also offers 'legacy' support for GPIO LEDs. For this, the __led_init, __led_set and __led_toggle functions are rerouted to appropriate gpio_*() calls.
* U-Boot also supports "colored" LEDs via STATUS_LED_(RED|GREEN|YELLOW|BLUE). For this a set of weak functions is prefined at board level (e.g. common/board_f.c), and supposed to be overridden by (device-)specific implementations. These "colored" LED functions (or stubs) are made available through cmd_led.c again.
And last but not least, cmd_led.c sneaks in a 'hardware blink' function __led_blink() as yet another stub that can be overwritten...
I don't feel like I would actually be in a position (or know a major part of board implementations well enough) to tackle all of this. I'd expect that all of these "subsystems" would need to be cleaned up and unified in some sort of common (and 'generic') LED framework, which in turn would work closely together with the actual "led" command. cmd_led.c should be able to manage LED "descriptors" dynamically, allowing them to be registered by the various implementations (possibly together with a set of function pointers that provides cmd_led.c with a uniform interface to actual LED 'functionality').
With regard to the original proposed patch: I'm not even sure I'd want to go forward with this given the current state. My checking did not seem to reveal any boards that make use of the "colored" LED (weak) functions and legacy gpio_led.c at the same time, so it might be safe to introduce the proposed GPIO LED stubs even without having a new CONFIG_GPIO_LED_STUBS option.
OTOH, the whole patch seems a bit pointless - given that the "future" direction we want to take for GPIO LEDs will be based on driver model anyway...
Regards, B. Nortmann
Am 28.08.2015 um 17:00 schrieb Tom Rini:
On Mon, Aug 24, 2015 at 11:51:46AM +0200, Bernhard Nortmann wrote:
Hello Simon!
Am 23.08.2015 23:21, schrieb Simon Glass:
Hi Bernard,
[...] If this is a new option it should be added to Kconfig. Otherwise:
Reviewed-by: Simon Glass sjg@chromium.org
Right. Kconfig wasn't on my agenda, but I agree that it should go in
there.
Unfortunately this points out further problems. The existing
CONFIG_GPIO_LED
is not part of Kconfig, but gets set via board-specific (.h) includes. Kconfig has/supports CONFIG_LED_GPIO, but that is related to the driver model variant, and doesn't affect inclusion of drivers/misc/gpio_led.c ... admittedly it's a bit of a mess.
I am not a huge fan of the existing API. I would suggest that if you have the energy you could replace it with something like:
enum led_colour_t { led_red, red_green, ... };
int led_set_on(enum led_colour_t colour, bool on)
Yes, I've been code-dancing a bit back and forth to get this stuff
to fit in
with the existing API. One reason I have selected these "stub" functions is that they are meant to match (and replace) the existing weak
definitions
in arch/arm/lib/board.c. Also, my impression is that they exist as
separate
functions to allow common/cmd_led.c to use them as function pointers, namely for the construction of the led_commands[] array.
The API may not be pretty, but I have in fact tried to keep it as
close to
the original as possible, with the goal of allowing to simply substitute GPIO numbers for the "LED bits".
BTW there is a device-tree-enabled LED driver node (see drivers/led). It might be worth considering using this on some sunxi boards.
Thanks! I'll definitely have a look into it. (That one is related to the CONFIG_LED_GPIO mentioned above.)
So do you want to do these further clean-ups on top of this series or in addition to? Thanks!