[LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger

[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

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!