On 9/20/20 5:28 PM, Marek Behun wrote:
On Sun, 20 Sep 2020 16:59:01 +0200
Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> wrote:
[...]
In case of my mt7601u dongle it looks like below:
/sys/class/leds/mt7601u-phy2$ cat trigger
none kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
kbd-ctrlllock kbd-ctrlrlock usb-gadget usb-host timer disk-activity
disk-read disk-write ide-disk mtd nand-disk heartbeat cpu cpu0 cpu1 cpu2
cpu3 cpu4 cpu5 cpu6 cpu7 panic pattern rfkill-any rfkill-none rfkill2
phy2rx phy2tx phy2assoc phy2radio [phy2tpt]
(This is another thing that is wrong: there should be only phy, or
wireless-phy trigger, and the mode (rx/tx/assoc/radio) and device
(phy0, phy1, ...) should be set via device_name file, as in netdev
trigger. Can we reimplement it and leave this ABI under configuration
option _LEAGACY?).
I agree.
IMO if LED is not physically integrated with any device, then it should
not be named after the device that is to be initially associated with
via trigger. This association can be later changed in userspace, which
will render the name invalid. And current associated device can be read
by reading triggers sysfs file, provided that the trigger conveys
that information like in case of presented above phy* triggers.
There are devices which have LEDs connected via a LED controller for
example via I2C bus, but the individual LEDs are dedicated (in the way
that there is an icon or text written on the device's case next to each
LED). In this case the trigger-source should be defined in device tree
in such a way that it aligns with the manufacturer's intended function
of the LED. And in this case I think the devicename part of the LED
should be derived from this trigger source.
Agreed about trigger source, but I'd rather not go for consulting LED
name with trigger source. Actually I was considering that back then,
but it turned out to be troublesome as if would have required
implementing that mechanism for associations with all subsystems.
And also you would need an intermediary layer to allow asynchronous
matching of LEDs with their trigger sources (something like
drivers/media/v4l2-core/v4l2-async.c). This would be an overkill.
Sure, if for example an ethernet PHY registers its LEDs, it can
hardcode init_data.devicename to "ethernet-phyN" or something like
that. But for LEDs on a generic LED controller...
I think we should get opinions from other people in this.
OTOH, a LED with devicename describing its physical location will
not change this location, even after changing the trigger
(or trigger source), thus it proves correct to have fixed devicename
section for the LED, but only if it is a part of some other pluggable
device.
[0]
https://lore.kernel.org/linux-leds/20190609190803.14815-27-jacek.anaszewski@xxxxxxxxx/
Marek
--
Best regards,
Jacek Anaszewski
