Hi Christian, On Thu, Jan 12, 2023 at 6:34 AM Christian Marangi <ansuels...@gmail.com> wrote: > On Tue, Jan 10, 2023 at 11:06:52PM -0800, Brian Norris wrote: > > diff --git a/target/linux/ipq806x/base-files/etc/board.d/01_leds > > b/target/linux/ipq806x/base-files/etc/board.d/01_leds > > index 2b259b903614..80a337c6a4d4 100644 > > --- a/target/linux/ipq806x/base-files/etc/board.d/01_leds > > +++ b/target/linux/ipq806x/base-files/etc/board.d/01_leds > > @@ -9,6 +9,9 @@ board_config_update > > board=$(board_name) > > > > case "$board" in > > +asus,onhub) > > + ucidef_set_led_default "status" "STATUS" "LED_Green" "1" > > Can't we set this directly in the dt?
I suppose. I guess that's "linux,default-trigger"? Confusingly, there's also "default-state" in the common bindings, but it's only supported for a handful of drivers. Downside: IIUC, this will no longer reflect in the UCI configuration, and so it's less obvious how to override things. Especially when, like the TP-Link version, you have 18 (!!) LEDs (or, 6 x 3-color LEDs), and you have to guess as to which ones to override. But I can live with that if that's deemed better. > Also I think we should use a more > descriptive name. Sure. What do you think would be better? For ASUS, it's only a single (set of) LEDs, so maybe I could go with "{red,green,blue}:status" naming like I see on some others. But how about TP-Link? There are 6 x 3-color LEDs strung in a ring around the top. There's not really any particular function assigned to them, and there isn't much visual separation between them (they overlap in a sort of gradient). So, "{red,green,blue}:status{0..5}"? Or "{red,green,blue}:ring{0..5}"? There are some photos here, if you need inspiration: https://openwrt.org/inbox/toh/google/onhub_tp-link_tgr1900 https://www.exploitee.rs/index.php/Rooting_The_Google_OnHub > > + ;; > > buffalo,wxr-2533dhp) > > ucidef_set_led_wlan "wlan" "WLAN" "white:wireless" "phy0tpt" > > ucidef_set_led_switch "wan" "WAN" "white:internet" "switch0" "0x20" > > @@ -58,6 +61,14 @@ tplink,c2600) > > ucidef_set_led_switch "wan" "wan" "white:wan" "switch0" "0x20" > > ucidef_set_led_switch "lan" "lan" "white:lan" "switch0" "0x1e" > > ;; > > +tplink,onhub) > > + ucidef_set_led_default "led0_red" "LED0_Red" "LED0_Red" "1" > > + ucidef_set_led_default "led1_green" "LED1_Green" "LED1_Green" "1" > > + ucidef_set_led_default "led2_blue" "LED2_Blue" "LED2_Blue" "1" > > + ucidef_set_led_default "led3_red" "LED3_Red" "LED3_Red" "1" > > + ucidef_set_led_default "led4_green" "LED4_Green" "LED4_Green" "1" > > + ucidef_set_led_default "led5_blue" "LED5_Blue" "LED5_Blue" "1" > > + ;; > > Same here. > > Aside from these leds problem rest of the series looks OK. I have just > some concern about the changed name in patch 1 for downstream project > but we can live with that. Thanks, Brian _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel