Re: [PATCH v2 1/2] dt-bindings: leds: add LED_FUNCTION for wlan2g/wlan5g

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Marek,

On 9/19/20 10:31 PM, Marek Behun wrote:
> On Sat, 19 Sep 2020 21:24:26 +0200
> Adrian Schmutzler <freifunk@adrianschmutzler.de> wrote:
> 
>> Many consumer "routers" have dedicated LEDs for specific WiFi bands,
>> e.g. one for 2.4 GHz and one for 5 GHz. These LEDs specifically
>> indicate the state of the relevant band, so the latter should be
>> included in the function name. LED_FUNCTION_WLAN will remain for
>> general cases or when the LED is used for more than one band.
>>
>> This essentially is equivalent to how we use LED_FUNCTION_LAN and
>> LED_FUNCTION_WAN instead of just having LED_FUNCTION_ETHERNET.
> 
> Dont. If you want the LED name to inform the user about the WiFi
> device it is being triggered on, it instead should come from the
> devicename part:
>    "wlan0:blue:activity"
> 
> In fact the function should not be "wlan" (nor "wlan2g" or "wlan5g", but
> "activity".
> 
> I am going to try to work on this subsystem so that if trigger source
> of the LED is set to a WiFi device and function is set to activity, the
> LED shall blink on wifi activity.
> 
> This way we can also avoid using the `linux,default-trigger` property
> in favour of `function`, i.e. if I have:
> 
>     wlan0: wifi@12300 {
>       compatible = "some-wifi";
>       #trigger-source-cells = <0>;
>     }
> 
>     led {
>       color = <LED_COLOR_ID_BLUE>;
>       function = LED_FUNCTION_ACTIVITY;
>       trigger-sources = <&wlan0>;
>     };
> 
> Than this will automatically name the LED as
>    wlan0:blue:activity
> and if the corresponding trigger is available, it should set the
> trigger even if no `linux,default-trigger` property is present.

Since wlan0 is DT label, then you will probably not be able to retrieve
its name in the driver but only a pointer to the struct device_node
associated with the label. And actually from the userspace user POV
this name is not too informative. What is informative is
unique identifier of the wlan device present in the system, associated
with the LED.

And wlan drivers broadly use wiphy_name() to get phy identifier and
use it for composing associated LED device name.

This way we get LED name in the form (here for Mediatek wlan dongle):

mt7601u-phy0

This is not in alignment with LED naming pattern and there also are
other variations for different drivers in
drivers/net/wireless, so it would be good to standardize that.

While implementing LED core support for LED name composition I created
also a script for validating LED name and printing LED hardware
related information so that could be a good starting point.

The script is in the tree:

tools/leds/get_led_device_info.sh

and for said Mediatek dongle it gives following output:

<quote>

$./tools/leds/get_led_device_info.sh /sys/class/leds/mt7601u-phy0

#####################################
# LED class device hardware details #
#####################################

bus:			usb
idVendor:		148f
idProduct:		7601
manufacturer:		MediaTek
product:		802.11 n WLAN
driver:			mt7601u

####################################
# LED class device name validation #
####################################

":" delimiter not detected.	[ FAILED ]

</quote>

And for the LEDs exposed by USB keyboard it prints below stuff:

<quote>

$./tools/leds/get_led_device_info.sh /sys/class/leds/input1\:\:capslock

#####################################
# LED class device hardware details #
#####################################

bus:			usb
idVendor:		046d
idProduct:		c31c
manufacturer:		Logitech
product:		USB Keyboard
driver:			usbhid
associated input node:	input1

####################################
# LED class device name validation #
####################################

devicename :	input1               [ OK ]
function   :	capslock             [ OK ]     Matching definition: 
LED_FUNCTION_CAPSLOCK

</quoute>

The script currently detects LEDs associations only with wireless and
input devices.

-- 
Best regards,
Jacek Anaszewski