| Message ID | 1544481988-30032-2-git-send-email-ynezz@true.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Mathias Kresin |
| Headers | show |
| Series | ath79: Add support for Ubiquiti Nanostation M (XW) | expand |
10/12/2018 23:46, Petr Štetiar: > Currently there is no LED signalization for various system states > implemented in diag.sh, so this patch adds support for it. > > Tested-by: Joe Ayers <ae6xe@arrl.net> > Signed-off-by: Petr Štetiar <ynezz@true.cz> > --- > target/linux/ath79/dts/ar9342_ubnt_xw.dtsi | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi b/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi > index b104bc6..742df11 100644 > --- a/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi > +++ b/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi > @@ -9,15 +9,22 @@ > compatible = "ubnt,xw", "qca,ar9342"; > model = "Ubiquiti Networks XW board"; > > + aliases { > + led-boot = &boot; > + led-failsafe = &failsafe; > + led-running = &boot; > + led-upgrade = &upgrade; > + }; > + > gpio-leds { > compatible = "gpio-leds"; > > - link1 { > + upgrade: link1 { > label = "ubnt:red:link1"; > gpios = <&gpio 11 GPIO_ACTIVE_LOW>; > }; > > - link2 { > + failsafe: link2 { > label = "ubnt:orange:link2"; > gpios = <&gpio 16 GPIO_ACTIVE_LOW>; > }; > @@ -27,7 +34,7 @@ > gpios = <&gpio 13 GPIO_ACTIVE_LOW>; > }; > > - link4 { > + boot: link4 { > label = "ubnt:green:link4"; > gpios = <&gpio 14 GPIO_ACTIVE_LOW>; > }; > Aren't these LEDs some kind of singal strength indicator? If so, they shouldn't be used to indicate a running system. I'm fine to temporary hijack the LEDs to indicate boot, failsafe and upgrade. But better use the same LED for all of these (link1)? Mathias
Mathias Kresin <dev@kresin.me> [2018-12-12 13:22:08]: > > --- a/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi > > +++ b/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi > > @@ -9,15 +9,22 @@ > > compatible = "ubnt,xw", "qca,ar9342"; > > model = "Ubiquiti Networks XW board"; > > + aliases { > > + led-boot = &boot; > > + led-failsafe = &failsafe; > > + led-running = &boot; > > + led-upgrade = &upgrade; > > + }; > > + > > gpio-leds { > > compatible = "gpio-leds"; > > - link1 { > > + upgrade: link1 { > > label = "ubnt:red:link1"; > > gpios = <&gpio 11 GPIO_ACTIVE_LOW>; > > }; > > - link2 { > > + failsafe: link2 { > > label = "ubnt:orange:link2"; > > gpios = <&gpio 16 GPIO_ACTIVE_LOW>; > > }; > > @@ -27,7 +34,7 @@ > > gpios = <&gpio 13 GPIO_ACTIVE_LOW>; > > }; > > - link4 { > > + boot: link4 { > > label = "ubnt:green:link4"; > > gpios = <&gpio 14 GPIO_ACTIVE_LOW>; > > }; > > > > Aren't these LEDs some kind of singal strength indicator? If so, they > shouldn't be used to indicate a running system. I'm fine to temporary hijack > the LEDs to indicate boot, failsafe and upgrade. But better use the same LED > for all of these (link1)? I've simply thought, that while having so many LEDs available, it would be better for UX (User Experience). Orange to signal failsafe mode, red to signal ongoing upgrade. Using just one green LED to signal all the states might be confusing and since we're using those LEDs only in cases when it's requested by the user (failsafe/upgrade), I don't see it as a big deal to hijack them for better UX as the RSSI won't be probably working at that time anyway (not tested this scenario). -- ynezz -- ynezz
12/12/2018 13:46, Petr Štetiar: > Mathias Kresin <dev@kresin.me> [2018-12-12 13:22:08]: > >>> --- a/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi >>> +++ b/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi >>> @@ -9,15 +9,22 @@ >>> compatible = "ubnt,xw", "qca,ar9342"; >>> model = "Ubiquiti Networks XW board"; >>> + aliases { >>> + led-boot = &boot; >>> + led-failsafe = &failsafe; >>> + led-running = &boot; >>> + led-upgrade = &upgrade; >>> + }; >>> + >>> gpio-leds { >>> compatible = "gpio-leds"; >>> - link1 { >>> + upgrade: link1 { >>> label = "ubnt:red:link1"; >>> gpios = <&gpio 11 GPIO_ACTIVE_LOW>; >>> }; >>> - link2 { >>> + failsafe: link2 { >>> label = "ubnt:orange:link2"; >>> gpios = <&gpio 16 GPIO_ACTIVE_LOW>; >>> }; >>> @@ -27,7 +34,7 @@ >>> gpios = <&gpio 13 GPIO_ACTIVE_LOW>; >>> }; >>> - link4 { >>> + boot: link4 { >>> label = "ubnt:green:link4"; >>> gpios = <&gpio 14 GPIO_ACTIVE_LOW>; >>> }; >>> >> >> Aren't these LEDs some kind of singal strength indicator? If so, they >> shouldn't be used to indicate a running system. I'm fine to temporary hijack >> the LEDs to indicate boot, failsafe and upgrade. But better use the same LED >> for all of these (link1)? > > I've simply thought, that while having so many LEDs available, it would be > better for UX (User Experience). Orange to signal failsafe mode, red to signal > ongoing upgrade. Using just one green LED to signal all the states might be > confusing and since we're using those LEDs only in cases when it's requested > by the user (failsafe/upgrade), I don't see it as a big deal to hijack them > for better UX as the RSSI won't be probably working at that time anyway > (not tested this scenario). We had the discussion dozen times. The LEDs have a defined function on these boards. Stick to this function. Using different link LEDs might be obvious to you, every one else need to have a look at the dts, to see which state is indicated by which misused led. Mathias
diff --git a/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi b/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi index b104bc6..742df11 100644 --- a/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi +++ b/target/linux/ath79/dts/ar9342_ubnt_xw.dtsi @@ -9,15 +9,22 @@ compatible = "ubnt,xw", "qca,ar9342"; model = "Ubiquiti Networks XW board"; + aliases { + led-boot = &boot; + led-failsafe = &failsafe; + led-running = &boot; + led-upgrade = &upgrade; + }; + gpio-leds { compatible = "gpio-leds"; - link1 { + upgrade: link1 { label = "ubnt:red:link1"; gpios = <&gpio 11 GPIO_ACTIVE_LOW>; }; - link2 { + failsafe: link2 { label = "ubnt:orange:link2"; gpios = <&gpio 16 GPIO_ACTIVE_LOW>; }; @@ -27,7 +34,7 @@ gpios = <&gpio 13 GPIO_ACTIVE_LOW>; }; - link4 { + boot: link4 { label = "ubnt:green:link4"; gpios = <&gpio 14 GPIO_ACTIVE_LOW>; };