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

[Mathias Kresin]

2016-10-17 12:10 GMT+02:00 Rafal Milecki <rafal at milecki.pl>:
> 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