[PATCH v3 7/7] ipq806x: Initial TP-Link and ASUS OnHub support

[Brian Norris]

Hi Christian,

On Thu, Jan 12, 2023 at 6:34 AM Christian Marangi <ansuelsmth at 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