kirkwood: add default leds in NSA310 and NSA325 - #938
Conversation
|
Is there any specific reason why you can not use I'm not that thrilled of adding a new function to Regarding CONFIG_ATA_LEDS, I would like to see that the port specific blinking gets integrated into the upstream LED Disk Trigger. This way we can drop one more LEDE specific patch. |
As said in the commit: "to keep consistent behaviour with the Sata leds that show sata activity, this led uses usb-host trigger to show usb activity." usbport trigger lights up the led if there are devices connected, and turns it off if there is nothing connected. It's not an usb activity led currently. Also, I personally prefer using leds to show activity for storage devices. I can check their "connected" status pretty easily by looking at the physical ports, while I can't check activity with the same ease.
I placed it there because it is very generic, it allows to set any simple led trigger like the ones I'm using (usb-host, ata1, ata2), but it can easily consolidate the current ucidef_set_led_ide (ide-disk, to be replaced by disk-activity I suppose) and ucidef_set_led_wlan (which actually is exactly the same as mine, but with a different name), and probably also some target-only triggers or stuff that is written in the dts file like the disk-activity trigger in mvebu target. I volunteer to do this change treewide if you agree.
@dangowrt (the patch author) said he tried to upstream it a long time ago and it was dropped with no explanation, see here http://lists.infradead.org/pipermail/lede-dev/2017-February/006358.html If you want that upstreamed I'm afraid you will have to discuss this with him, as I don't understand the patch enough, I can only send it as-is and hope they don't request changes. |
|
Is there anything that needs to be done here? |
No, it was meant as question/starter for a discussion. It is just that I don't really like the new function and therefore I will not merge the PR. But I'm not going to reject this PR. Might be that other devs see the need of the new function and I'm the one who is wrong here. |
@mkresin If I make that a device-specific function, will you merge this? I suspect that a serious discussion about led helper functions is too low in priorities of everyone (and I'm not even blaming them, lol), so I doubt we will see anyone else here. |
|
@bobafetthotmail looks ok, but please make ucidef_set_led_usbport() and all other helpers setting triggers use this new function as to avoid code duplication |
delete useless or unusable default led triggers from dts file. Signed-off-by: Alberto Bursi <alberto.bursi@outlook.it>
create a function with code common to all led functions, create another function with code common to functions setting a simple led trigger, restore alphabetical order in function names. Signed-off-by: Alberto Bursi <alberto.bursi@outlook.it>
uci_set_leds_ataport() allows to set a led to show activity on a specific (s)ata port, which is needed for devices that have a Sata led for each sata port. The led trigger is from the 834-ledtrig-libata.patch LEDE kernel patch. uci_set_leds_usbhost() allows to set a led to show total usb activity. Signed-off-by: Alberto Bursi <alberto.bursi@outlook.it>
these two devices have a Sata led for each sata port. These leds must be controlled separately by a special sata led trigger already used in oxnas target. Both these devices have a single USB led, and to keep consistent behaviour with the Sata leds that show sata activity, this led uses usb-host trigger to show usb activity. Signed-off-by: Alberto Bursi <alberto.bursi@outlook.it>
|
ok, updated. |
|
Umm, is there something I can do to help the review of this PR? Splitting more the commits, making a different PR for the led functions consolidation, whatever. |
|
I'll take it :) |
|
Merged into my staging tree - thanks! |
5 participants
creating a generic uci helper function because it would make no sense to have 3-4 uci helpers that differ only in name and trigger name.
Signed-off-by: Alberto Bursi alberto.bursi@outlook.it