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

[Christian Marangi]

On Thu, Jan 12, 2023 at 09:35:03AM -0800, Brian Norris wrote:
> 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.

If the idea is to keep them turned on, default-trigger and set it to
always on but strange that default-state is not supported...

> 
> 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.

It's not needed to have an UCI conf reflecting the state... If someone
needs to configure the led the current configuration is ignored anyway
as it will be changed to what the user wants.

> 
> > 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.

Yes the pattern is color:function... Ideally everything should be
handled with standard linux binding instead of declaring a label...

So using function-enumerator, color and function.
If something is not here [1] then label are required

[1] https://elixir.bootlin.com/linux/latest/source/include/dt-bindings/leds/common.h

> 
> 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
> 

I would use function_status for everything... function enumerator and
color... 

> > > +     ;;
> > >  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

-- 
	Ansuel