Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger
On 13 October 2016 at 09:44, Rafał Miłecki wrote: > From: Rafał Miłecki > > This makes init.d script handle existing UCI entries using the new > trigger. It also switches all targets to use its package. > > Signed-off-by: Rafał Miłecki Pushed with code moved out of switch for cleaner code sharing with usbport trigger. Also added comments to make it clear what sed commands are for. ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger
2016-10-17 12:10 GMT+02:00 Rafal Milecki : > On 10/16/2016 03:12 PM, Mathias Kresin wrote: >> >> 13.10.2016 09:44, Rafał Miłecki: >>> >>> diff --git a/package/base-files/files/etc/init.d/led >>> b/package/base-files/files/etc/init.d/led >>> index 79f2904..507dcbf 100755 >>> --- a/package/base-files/files/etc/init.d/led >>> +++ b/package/base-files/files/etc/init.d/led >>> @@ -47,6 +47,8 @@ load_led() { >>> echo 0 >/sys/class/leds/${sysfs}/brightness >>> >>> echo $trigger > /sys/class/leds/${sysfs}/trigger 2> /dev/null >>> +# Backward compatibility >>> +[ $trigger = "usbdev" ] && echo usbport > >>> /sys/class/leds/${sysfs}/trigger 2> /dev/null >>> ret="$?" >>> >>> [ $default = 1 ] && >>> @@ -72,9 +74,14 @@ load_led() { >>> ;; >>> >>> "usbdev") >>> -[ -n "$dev" ] && { >>> -echo $dev > /sys/class/leds/${sysfs}/device_name >>> -echo $interval > >>> /sys/class/leds/${sysfs}/activity_interval >>> +local usbport >>> + >>> +# Translate USB dev/port format of the old usbdev trigger >>> +usbport=$(echo "$dev" | sed -n >>> 's/^\([0-9]*\)-\([0-9]*\)$/usb\1-port\2/p') >>> +[ -z "$usbport" ] && usbport=$(echo "$dev" | sed -n >>> 's/\./-port/p') >> >> >> I'm not sure if I got the purpose of this sed call correctly. As far as I >> can see, it should fixup usb ports defined as "usb1.1". Via a quick grep I >> couldn't find anything like that used. > > > This second sed call is needed for ports of (internal) hubs. If you grep > targets > for ucidef_set_led_usbdev you can find e.g. > ucidef_set_led_usbdev "usb" "USB" "arduino:white:usb" "1-1.1" > ucidef_set_led_usbdev "usb1" "USB1" "tp-link:green:usb1" "1-1.1" > ucidef_set_led_usbdev "usb2" "USB2" "tp-link:green:usb2" "1-1.2" > > So e.g. 2-1 has to be translated into usb2-port1 > While 1-1.1 has to be translated into 1-1-port1 Thanks for the explanation. > > This is what I meant when I wrote: >> Now after coming back home I also tried it with extra hub and: >> ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-2.4" > > >> I rather would suggest to add a line which allows to use the new syntax >> instead: >> >> [ -z "$usbport" ] && usbport="${dev}" > > > For using new trigger and its syntax directly, I mean to add a new call like > ucidef_set_led_usbport. It should also support specifying more than 1 USB > port. > You can find my initial work on it in: > https://patchwork.ozlabs.org/patch/678014/ Ah okay. I was fooled by the fact that you old patch is marked as superseded in patchwork. I was the opinion that this patch is the one that supersedes the old one. Personally, I prefer to keep ucidef_set_led_usbdev() and to add the suggested changes. This way we don't need to touch all the board.d/leds files, possibly existing documentation remains valid and boards added in custom trees doesn't need an update as well. It would be possible to add a ucidef_set_led_usbport() function without removing ucidef_set_led_usbdev() to workaround this. But I'm not that addicted of having two function doing basically the same. But I guess it's just a matter of taste. Mathias ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger
On 10/16/2016 03:12 PM, Mathias Kresin wrote: 13.10.2016 09:44, Rafał Miłecki: diff --git a/package/base-files/files/etc/init.d/led b/package/base-files/files/etc/init.d/led index 79f2904..507dcbf 100755 --- a/package/base-files/files/etc/init.d/led +++ b/package/base-files/files/etc/init.d/led @@ -47,6 +47,8 @@ load_led() { echo 0 >/sys/class/leds/${sysfs}/brightness echo $trigger > /sys/class/leds/${sysfs}/trigger 2> /dev/null +# Backward compatibility +[ $trigger = "usbdev" ] && echo usbport > /sys/class/leds/${sysfs}/trigger 2> /dev/null ret="$?" [ $default = 1 ] && @@ -72,9 +74,14 @@ load_led() { ;; "usbdev") -[ -n "$dev" ] && { -echo $dev > /sys/class/leds/${sysfs}/device_name -echo $interval > /sys/class/leds/${sysfs}/activity_interval +local usbport + +# Translate USB dev/port format of the old usbdev trigger +usbport=$(echo "$dev" | sed -n 's/^\([0-9]*\)-\([0-9]*\)$/usb\1-port\2/p') +[ -z "$usbport" ] && usbport=$(echo "$dev" | sed -n 's/\./-port/p') I'm not sure if I got the purpose of this sed call correctly. As far as I can see, it should fixup usb ports defined as "usb1.1". Via a quick grep I couldn't find anything like that used. This second sed call is needed for ports of (internal) hubs. If you grep targets for ucidef_set_led_usbdev you can find e.g. ucidef_set_led_usbdev "usb" "USB" "arduino:white:usb" "1-1.1" ucidef_set_led_usbdev "usb1" "USB1" "tp-link:green:usb1" "1-1.1" ucidef_set_led_usbdev "usb2" "USB2" "tp-link:green:usb2" "1-1.2" So e.g. 2-1 has to be translated into usb2-port1 While 1-1.1 has to be translated into 1-1-port1 This is what I meant when I wrote: > Now after coming back home I also tried it with extra hub and: > ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-2.4" I rather would suggest to add a line which allows to use the new syntax instead: [ -z "$usbport" ] && usbport="${dev}" For using new trigger and its syntax directly, I mean to add a new call like ucidef_set_led_usbport. It should also support specifying more than 1 USB port. You can find my initial work on it in: https://patchwork.ozlabs.org/patch/678014/ + +[ -n "$usbport" ] && { +echo 1 > /sys/class/leds/${sysfs}/ports/$usbport } What about adding something that allows to add all or multiple usb ports (using the new syntax of course) to a single LED (which is thanks to your usbport trigger now possible): [ "$usbport" = "*" ] && usbport=$(ls /sys/class/leds/${sysfs}/ports/) for port in ${usbport}; do echo 1 > "/sys/class/leds/${sysfs}/ports/${port}" done I'm OK with that, it sounds like a good idea! I'd just rather wait for a new call implementation (ucidef_set_led_usbport?) instead adding more features to this old one. If there won't be any complains about this patchset, I'll push it soon and then I'll try to add support for specifying multiple ports. I'd like to see your patch then! ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger
13.10.2016 09:44, Rafał Miłecki: From: Rafał Miłecki This makes init.d script handle existing UCI entries using the new trigger. It also switches all targets to use its package. Signed-off-by: Rafał Miłecki --- package/base-files/files/etc/init.d/led| 13 +++-- target/linux/apm821xx/image/Makefile | 4 +- target/linux/apm821xx/nand/profiles/00-default.mk | 2 +- target/linux/apm821xx/sata/profiles/00-default.mk | 2 +- target/linux/ar71xx/generic/profiles/00-default.mk | 2 +- target/linux/ar71xx/image/generic.mk | 14 ++--- target/linux/ar71xx/image/legacy-devices.mk| 44 +++ target/linux/ar71xx/image/nand.mk | 8 +-- target/linux/ar71xx/image/tp-link.mk | 62 +++--- target/linux/ar71xx/nand/profiles/00-default.mk| 2 +- target/linux/brcm63xx/image/Makefile | 4 +- target/linux/ipq806x/Makefile | 2 +- target/linux/lantiq/image/Makefile | 36 ++--- target/linux/lantiq/image/tp-link.mk | 6 +-- target/linux/mcs814x/Makefile | 2 +- target/linux/mediatek/profiles/default.mk | 2 +- target/linux/mvebu/config-4.4 | 2 +- target/linux/oxnas/Makefile| 2 +- target/linux/ramips/image/mt7620.mk| 6 +-- target/linux/ramips/image/mt7621.mk| 18 +++ target/linux/ramips/image/mt7628.mk| 8 +-- target/linux/ramips/image/rt305x-legacy.mk | 12 ++--- target/linux/ramips/image/rt305x.mk| 18 +++ target/linux/ramips/mt7620/profiles/00-default.mk | 2 +- target/linux/ramips/mt7621/profiles/00-default.mk | 2 +- target/linux/ramips/mt7628/profiles/00-default.mk | 2 +- target/linux/ramips/mt7688/profiles/00-default.mk | 2 +- target/linux/ramips/rt305x/profiles/00-default.mk | 2 +- 28 files changed, 144 insertions(+), 137 deletions(-) diff --git a/package/base-files/files/etc/init.d/led b/package/base-files/files/etc/init.d/led index 79f2904..507dcbf 100755 --- a/package/base-files/files/etc/init.d/led +++ b/package/base-files/files/etc/init.d/led @@ -47,6 +47,8 @@ load_led() { echo 0 >/sys/class/leds/${sysfs}/brightness echo $trigger > /sys/class/leds/${sysfs}/trigger 2> /dev/null + # Backward compatibility + [ $trigger = "usbdev" ] && echo usbport > /sys/class/leds/${sysfs}/trigger 2> /dev/null ret="$?" [ $default = 1 ] && @@ -72,9 +74,14 @@ load_led() { ;; "usbdev") - [ -n "$dev" ] && { - echo $dev > /sys/class/leds/${sysfs}/device_name - echo $interval > /sys/class/leds/${sysfs}/activity_interval + local usbport + + # Translate USB dev/port format of the old usbdev trigger + usbport=$(echo "$dev" | sed -n 's/^\([0-9]*\)-\([0-9]*\)$/usb\1-port\2/p') + [ -z "$usbport" ] && usbport=$(echo "$dev" | sed -n 's/\./-port/p') I'm not sure if I got the purpose of this sed call correctly. As far as I can see, it should fixup usb ports defined as "usb1.1". Via a quick grep I couldn't find anything like that used. I rather would suggest to add a line which allows to use the new syntax instead: [ -z "$usbport" ] && usbport="${dev}" + + [ -n "$usbport" ] && { + echo 1 > /sys/class/leds/${sysfs}/ports/$usbport } What about adding something that allows to add all or multiple usb ports (using the new syntax of course) to a single LED (which is thanks to your usbport trigger now possible): [ "$usbport" = "*" ] && usbport=$(ls /sys/class/leds/${sysfs}/ports/) for port in ${usbport}; do echo 1 > "/sys/class/leds/${sysfs}/ports/${port}" done Mathias ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger
On 13 October 2016 at 09:44, Rafał Miłecki wrote: > From: Rafał Miłecki > > This makes init.d script handle existing UCI entries using the new > trigger. It also switches all targets to use its package. > > Signed-off-by: Rafał Miłecki Before sending this patchset I tested it with some simple: ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-1" Now after coming back home I also tried it with extra hub and: ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-2.4" Both ports were translated correctly and worked with usbdev as well as with the usbport. This lets me hope there won't be any regressions caused by this. ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev