Hi Pavel,

Thanks for the review.

On 11/11/2018 12:31 PM, Pavel Machek wrote:
> Hi!
>
>> Add common LED function definitions for use in Device Tree.
>> The function names were extracted from existing dts files
>> after eliminating oddities.
>
> Thanks for doing this.
>
>> diff --git a/include/dt-bindings/leds/functions.h b/include/dt-bindings/leds/functions.h
>> new file mode 100644
>> index 0000000..3f94e09
>> --- /dev/null
>> +++ b/include/dt-bindings/leds/functions.h
>> @@ -0,0 +1,101 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>
>> +#define LED_FUNCTION_2G "2g"
>> +#define LED_FUNCTION_ACTIVITY "activity"
>> +#define LED_FUNCTION_ADSL "adsl"
>> +#define LED_FUNCTION_ALARM "alarm"
>> +#define LED_FUNCTION_ALIVE "alive"
>> +#define LED_FUNCTION_ALL "all"
>> +#define LED_FUNCTION_APP "app"
>> +#define LED_FUNCTION_AUX "aux"
>> +#define LED_FUNCTION_BACKUP "backup"
>> +#define LED_FUNCTION_BACKLIGHT "backlight"
>> +#define LED_FUNCTION_BACKLIGHT_CLUSTER "backlight_cluster"
>
> Sounds like one of backlight and backlight_cluster should be deprecated?

I think so.

>> +#define LED_FUNCTION_BEEP "beep"
>> +#define LED_FUNCTION_BLUETOOTH "bluetooth"
>> +#define LED_FUNCTION_BOOT "boot"
>> +#define LED_FUNCTION_BOTTOM "bottom"
>
> I believe some of these should get comments. For example what does
> "all" and "bottom" LEDs do?

It would be best to ask the author, but for now,
I'd just remove all of "bottom", "up", "left", "right"
etc.

>> +#define LED_FUNCTION_BRICK_STATUS "brick-status"
>> +#define LED_FUNCTION_CEL "cel"
>> +#define LED_FUNCTION_CEL_PWR "cel-pwr"
>> +#define LED_FUNCTION_CEL_RESET "cel-reset"
>> +#define LED_FUNCTION_CHRG "chrg"
>> +#define LED_FUNCTION_COM "com"
>> +#define LED_FUNCTION_COPY "copy"
>> +#define LED_FUNCTION_CPU "cpu"
>> +#define LED_FUNCTION_DEBUG "debug"
>> +#define LED_FUNCTION_DIA "dia"
>
> What does this one do?

I'd opt for something like "diagnostics", but this is a blind shot.
Let's remove it then and, and let people add more meaningful
definition in case it is needed.

>> +#define LED_FUNCTION_DISK "disk"
>
> We have disk, hd, hdd and sata. Deprecate some?

Disk should be the best choice, especially given that we have
identically named trigger.

>> +#define LED_FUNCTION_DISPLAY_CLUSTER "display_cluster"
>> +#define LED_FUNCTION_DOWN "down"
>> +#define LED_FUNCTION_DSL "dsl"
>> +#define LED_FUNCTION_ENOCEAN "enocean"
>> +#define LED_FUNCTION_ENTER "enter"
>> +#define LED_FUNCTION_ERROR "error"
>> +#define LED_FUNCTION_ESATA "esata"
>> +#define LED_FUNCTION_ETHERNET_STATUS "ethernet-status"
>> +#define LED_FUNCTION_FAIL "fail"
>> +#define LED_FUNCTION_FAULT "fault"
>> +#define LED_FUNCTION_FLASH "flash"
>> +#define LED_FUNCTION_FRONT "front"
>> +#define LED_FUNCTION_FUNC "func"
>> +#define LED_FUNCTION_GPIO "gpio"
>> +#define LED_FUNCTION_GSM "gsm"
>
>> +#define LED_FUNCTION_HD "hd"
>> +#define LED_FUNCTION_HDD "hdd"
>> +#define LED_FUNCTION_HDDERR "hdderr"
>
> Ok, I'll

Hmm?

>
>> +#define LED_FUNCTION_HEALTH "health"
>> +#define LED_FUNCTION_HEARTBEAT "heartbeat"
>
> Sounds same as alive, deprecate alive?

Heartbeat may be designated specifically for registration
for events from the trigger with the same name.

>> +#define LED_FUNCTION_WIFI "wifi"
>> +#define LED_FUNCTION_WIRELESS "wireless"
>> +#define LED_FUNCTION_WLAN "wlan"
>
> Same as wifi and wireless, I guess, deprecate some?

I'd remove "wireless" and "wlan".

> I'll touch same subject in another email.
> Pavel
>

--
Best regards,
Jacek Anaszewski

2018-11-11 20:04:23

by Jacek Anaszewski

[permalink] [raw]

Subject: Re: [PATCH 02/24] leds: core: Add support for composing LED class device names

Hi Pavel,

On 11/11/2018 01:02 PM, Pavel Machek wrote:
> Hi!
>
>> Add public led_compose_name() API for composing LED class device
>> name basing on fwnode_handle data. The function composes device name
>> according to either a new <color:function> pattern or the legacy
>> <devicename:color:function> pattern. The decision on using the
>> particular pattern is made basing on whether fwnode contains new
>> "function" and "color" properties, or the legacy "label" proeprty.
>>
>> Backwards compatibility with in-driver hard-coded LED class device
>> names is assured thanks to the default_desc argument.
>>
>> In case none of the aformentioned properties was found, then, for OF
>> nodes, the node name is adopted for LED class device name.
>>
>
>> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
>> index 836cb16..e9009c4 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -43,7 +43,7 @@ LED Device Naming
>>
>> Is currently of the form:
>>
>> -"devicename:colour:function"
>> +"colour:function"
>>
>
> I don't think we want to do it in all cases.
>
> So, on my cellphone seeing lp5523:green:led is indeed not useful.
>
> But on notebook with usb keyboard attached, you need to keep the
> devicename to be able to distinguish capslock on internal keyboard and
> capslock on first USB keyboard and capslock on second USB keyboard.
>
> Taking look at the list of functions, here's stuff like "hdd" and
> "hdderr". I assume the first means hdd activity... If we can do it, it
> would be nicest to have "sda:green:activity" and maybe
> "sda:red:error". For a raid array with 8 drives...
>
> For example I have a router running linux. It has 4 lan ports, with
> correspondings LED, and an wan led.
>
> Having "green:lan1" to "green:lan4" and "green:wan" plus
> "red:wanerror" would work, but I'd really preffer
> "eth0:green:link"... "adsl0:green:link", "adsl0:red:error".
>
> There are now phones with flashes on both main and selfie
> cameras. Again, knowing which device is which is important. As is
> knowing which display is controlled by particular backlight.

It's overcomplicating the naming again. In every case you can tweak
the function name to eth0_link, eth1_link etc.

--
Best regards,
Jacek Anaszewski

2018-11-11 20:04:56

by Jacek Anaszewski

[permalink] [raw]

Subject: Re: [PATCH 05/24] dt-bindings: sc27xx-blt: Add function and color properties

Hi Pavel.

On 11/11/2018 03:29 PM, Pavel Machek wrote:
> On Tue 2018-11-06 23:07:13, Jacek Anaszewski wrote:
>> Refer to new "function" and "color" properties and mark "label"
>> as deprecated.
>>
>> Signed-off-by: Jacek Anaszewski <[email protected]>
>> Cc: Xiaotong Lu <[email protected]>
>> Cc: Baolin Wang <[email protected]>
>
> Don't you want to keep both color and label? New dts on old kernel
> should still work...?
>
> Ok, I see these are only examples....

Right, this is only an example, and we'd rather want to encourage
people to switch to using new LED naming.

>> @@ -25,17 +27,17 @@ led-controller@200 {
>> reg = <0x200>;
>>
>> led@0 {
>> - label = "red";
>> + color = "red";
>> reg = <0x0>;
>> };
>>
>> led@1 {
>> - label = "green";
>> + color = "green";
>> reg = <0x1>;
>> };
>>
>> led@2 {
>> - label = "blue";
>> + color = "blue";
>> reg = <0x2>;
>> };
>> };
>

--
Best regards,
Jacek Anaszewski

2018-11-11 20:18:49

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH 02/24] leds: core: Add support for composing LED class device names

Hi!

> >> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
> >> index 836cb16..e9009c4 100644
> >> --- a/Documentation/leds/leds-class.txt
> >> +++ b/Documentation/leds/leds-class.txt
> >> @@ -43,7 +43,7 @@ LED Device Naming
> >>
> >> Is currently of the form:
> >>
> >> -"devicename:colour:function"
> >> +"colour:function"
> >>
> >
> > I don't think we want to do it in all cases.
> >
> > So, on my cellphone seeing lp5523:green:led is indeed not useful.
> >
> > But on notebook with usb keyboard attached, you need to keep the
> > devicename to be able to distinguish capslock on internal keyboard and
> > capslock on first USB keyboard and capslock on second USB keyboard.
> >
> > Taking look at the list of functions, here's stuff like "hdd" and
> > "hdderr". I assume the first means hdd activity... If we can do it, it
> > would be nicest to have "sda:green:activity" and maybe
> > "sda:red:error". For a raid array with 8 drives...
> >
> > For example I have a router running linux. It has 4 lan ports, with
> > correspondings LED, and an wan led.
> >
> > Having "green:lan1" to "green:lan4" and "green:wan" plus
> > "red:wanerror" would work, but I'd really preffer
> > "eth0:green:link"... "adsl0:green:link", "adsl0:red:error".
> >
> > There are now phones with flashes on both main and selfie
> > cameras. Again, knowing which device is which is important. As is
> > knowing which display is controlled by particular backlight.
>
> It's overcomplicating the naming again. In every case you can tweak
> the function name to eth0_link, eth1_link etc.

Well, but in such case it would be good to keep existing scheme.

My system looks like this:

input16::capslock tpacpi::bay_active tpacpi::standby
input16::numlock tpacpi::dock_active tpacpi::thinklight
input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage
input5::capslock tpacpi::dock_status1 tpacpi::unknown_led
input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2
input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3

I agree that we should get rid of "tpacpi:" part in some cases. But
it does not make sense to get rid of "input16:" part -- it tells you
if the LED is on USB or on built-in keyboard.

Ideally, tpacpi::thinklight would be input5::frontlight (as it is
frontlight for the keyboard).

Yes we should simplify, but it still needs to work in all cases.

Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.60 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-11-11 20:22:58

On Mon 2018-11-12 21:11:32, Jacek Anaszewski wrote:
> On 11/12/2018 08:05 PM, Pavel Machek wrote:
> > Hi!
> >
> >>>>> My system looks like this:
> >>>>>
> >>>>> input16::capslock tpacpi::bay_active tpacpi::standby
> >>>>> input16::numlock tpacpi::dock_active tpacpi::thinklight
> >>>>> input16::scrolllock tpacpi::dock_batt tpacpi::thinkvantage
> >>>>> input5::capslock tpacpi::dock_status1 tpacpi::unknown_led
> >>>>> input5::numlock tpacpi::dock_status2 tpacpi::unknown_led2
> >>>>> input5::scrolllock tpacpi:green:batt tpacpi::unknown_led3
> >
> >>> But it is not just for backwards compatibility. See my examples above,
> >>> it is needed to tell which device the LED is asociated with, and it is
> >>> absolutely required for USB devices (for example).
> >>
> >> For USB devices there is already ledtrig-usbport available, which
> >> provides sysfs interface for defining and reading the usb ports,
> >> the status of which the LED indicates. Since the USB devices can be
> >> attached/removed dynamically, it would be impossible to reflect
> >> the associations in the LED class device name.
> >
> > I'm not talking USB activity. I'm talking USB devices with LEDs on
> > them, like for example keyboards.
> >
> > Please take a look at example above. input16::numlock ;
> > input5::numlock . You absolutely need device part.
>
> It would be redundant since there is "device" symbolic link
> created in given LED class device sysfs directory, pointing to the
> corresponding input device directory, like in case of my USB
> keyboard:

You are right I forgot about the device symlink, and it partly helps
with the USB keyboard case...

But you still need the device part. Sysfs will not like two
directories named "::numlock".

> #/sys/class/leds/input5::scrolllock$ ls -l
> total 0
> -rw-r--r-- 1 root root 4096 Nov 12 20:22 brightness
> lrwxrwxrwx 1 root root 0 Nov 12 20:22 device -> ../../input5
> -r--r--r-- 1 root root 4096 Nov 12 20:22 max_brightness
> drwxr-xr-x 2 root root 0 Nov 12 20:22 power
> lrwxrwxrwx 1 root root 0 Nov 12 20:04 subsystem ->
> ../../../../../../../../../../../class/leds
> -rw-r--r-- 1 root root 4096 Nov 12 20:22 trigger

> > Because userspace needs that information?
> >
> > Say you have raid array, with "error" leds for each drive (your list
> > already contains "hdderr"). Now userland detects problem with hdparm
> > on /dev/sdi. It would like to turn on corresponding LED.
> >
> > How do you propose we do that?
>
> Similarly as in case of USB keyboard.

Not really, I'm afraid. Hard drives have no red LEDs on them (and the
LED would not be visible, anyway) so the "device" symlink in such case
would point to some kind of i2c LED controller.

Eventually we'll need to have two devices for each LED. "Controller
this is on" -- in device symlink and "device this talks about".

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (3.03 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-11-12 22:12:25

Hi!

> > >> Basically every single device could have a LED associated with it
> > >> ("activity"). Would doing it like this mean we'd have to modify every
> > >> single driver to parse leds / led-names properties?
> > >
> > > Normally, that's how properties like this would work. A driver is also
> > > what knows how the leds should function.
> >
> > This is not true in case of associations where LED controller is
> > an independent device, as in Pavel's example [0].
>
> I'm not really following how the HDD example is different. The parent
> of an LED would be the controller. The link to the led node would be
> in the disk controller node. Though maybe if things get complicated
> enough, we'd want to describe the drives or drive bays in DT.

We were talking case where the LED is on GPIO somewhere. And yes,
drive bays would be similar.

And.. often you have a single LED for multiple harddrives, too. Fun
:-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (1.07 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-12-12 10:02:13

On Wed 2018-12-12 07:56:20, Rob Herring wrote:
> On Wed, Dec 12, 2018 at 3:59 AM Pavel Machek <[email protected]> wrote:
> >
> > Hi!
> >
> > > > We would also probably need different DT properties for different
> > > > types of devices, since e.g. for network case the network interface
> > > > name would fit better for the LED name, than the phy name,
> > > > and we would need to know what type of device name we're going
> > > > to look for.
> > > >
> > > > Pavel gave following examples:
> > > >
> > > > eth0:green:link
> > > > adsl0:green:link
> > > > adsl0:red:error
> > > >
> > > > So we would have e.g.:
> > > >
> > > > associated-vl42-device = <&camera1>;
> > > > associated-network-device = <&phy1>;
> > > > associated-block-device = <&phy1>;
> > >
> > > Variable property names are kind of a pain to parse.
> > >
> > > Perhaps when LEDs are associated with a device, we shouldn't care
> > > within the context of the LED subsystem what the name is. The
> > > association is more important and if you have that exposed, then you
> > > don't really need to care what the name is. You still have to deal
> > > with a device with more than 1 LED, but that becomes a problem local
> > > to that device.
> > >
> > > What I'm getting at is following a more standard binding pattern of
> > > providers and consumers like we have for gpios, clocks, etc. So we'd
> > > have something like this:
> > >
> > > ethernet {
> > > ...
> > > leds = <&green_led>, <&red_led>;
> > > led-names = "link", "err";
> > > };
> > >
> > > We can still support defining LED names as we've done, but we don't
> > > have to come up with some elaborate naming convention that covers
> > > every single case.
> >
> > I see that it would be more consistent with how gpios work, but I'm
> > afraid this does not fit LEDs properly.
> >
> > With power LED, you want to be able to say "this is just on". Some
> > poeple like heartbeat, and have LED for that. There may be LED for
> > "disk activity", meaning activity on any harddrive. And there may be
> > activity LED for specific disk.
> >
> > Only in the last case it would be suitable to have LED reference as a
> > child of an device...
>
> Right. For all the other cases, why do you need any link with a
> device? What we have today is sufficient and can continue to be
> supported. A link to a device is only used if the led is associated
> with a particular device.

Right, so the link is only needed in some cases. So unlike your
example, we would have:

led1 { led-meaning = "heartbeat"; }
led2 { led-meaning = "link"; }
led3 { led-meaning = "err"; }
led4 { led-meaning = "activity"; what-activity = "all_harddisks"; }
ethernet { leds = < &led2, &led3 >; }

Dunno. I'd expect all the LED description in the LED node, not part of
the description there and back link from the device...
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.98 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-12-21 16:12:50

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 04/24] dt-bindings: leds: Add function and color properties

On Tue, Dec 11, 2018 at 6:27 PM Rob Herring <[email protected]> wrote:
> On Sat, Dec 1, 2018 at 3:17 PM Jacek Anaszewski
> <[email protected]> wrote:

> > This is not true in case of associations where LED controller is
> > an independent device, as in Pavel's example [0].
>
> I'm not really following how the HDD example is different. The parent
> of an LED would be the controller. The link to the led node would be
> in the disk controller node. Though maybe if things get complicated
> enough, we'd want to describe the drives or drive bays in DT.

Not talking about this in specific, but I will need to add DT bindings
for ATA master/slave, in order to add a temperature zone
for each drive (one sensor in each drive) in my in-progress
hwmon-over-SCSI-emulation-ATA-sensors such that:

ata-controller {
ata-master {
};
ata-slave {
};
};

I haven't looked closer at this topology yet, but we definatelt
need nodes for each physical connector out of an ATA
master, somehow, so they can act as a sensor/temperature
zone.

Yours,
Linus Walleij

2018-12-23 20:13:53

by Jacek Anaszewski

[permalink] [raw]

Subject: Re: [PATCH 04/24] dt-bindings: leds: Add function and color properties

On 12/12/18 7:32 PM, Pavel Machek wrote:
> On Wed 2018-12-12 07:56:20, Rob Herring wrote:
>> On Wed, Dec 12, 2018 at 3:59 AM Pavel Machek <[email protected]> wrote:
>>>
>>> Hi!
>>>
>>>>> We would also probably need different DT properties for different
>>>>> types of devices, since e.g. for network case the network interface
>>>>> name would fit better for the LED name, than the phy name,
>>>>> and we would need to know what type of device name we're going
>>>>> to look for.
>>>>>
>>>>> Pavel gave following examples:
>>>>>
>>>>> eth0:green:link
>>>>> adsl0:green:link
>>>>> adsl0:red:error
>>>>>
>>>>> So we would have e.g.:
>>>>>
>>>>> associated-vl42-device = <&camera1>;
>>>>> associated-network-device = <&phy1>;
>>>>> associated-block-device = <&phy1>;
>>>>
>>>> Variable property names are kind of a pain to parse.
>>>>
>>>> Perhaps when LEDs are associated with a device, we shouldn't care
>>>> within the context of the LED subsystem what the name is. The
>>>> association is more important and if you have that exposed, then you
>>>> don't really need to care what the name is. You still have to deal
>>>> with a device with more than 1 LED, but that becomes a problem local
>>>> to that device.
>>>>
>>>> What I'm getting at is following a more standard binding pattern of
>>>> providers and consumers like we have for gpios, clocks, etc. So we'd
>>>> have something like this:
>>>>
>>>> ethernet {
>>>> ...
>>>> leds = <&green_led>, <&red_led>;
>>>> led-names = "link", "err";
>>>> };
>>>>
>>>> We can still support defining LED names as we've done, but we don't
>>>> have to come up with some elaborate naming convention that covers
>>>> every single case.
>>>
>>> I see that it would be more consistent with how gpios work, but I'm
>>> afraid this does not fit LEDs properly.
>>>
>>> With power LED, you want to be able to say "this is just on". Some
>>> poeple like heartbeat, and have LED for that. There may be LED for
>>> "disk activity", meaning activity on any harddrive. And there may be
>>> activity LED for specific disk.
>>>
>>> Only in the last case it would be suitable to have LED reference as a
>>> child of an device...
>>
>> Right. For all the other cases, why do you need any link with a
>> device? What we have today is sufficient and can continue to be
>> supported. A link to a device is only used if the led is associated
>> with a particular device.
>
> Right, so the link is only needed in some cases. So unlike your
> example, we would have:
>
> led1 { led-meaning = "heartbeat"; }
> led2 { led-meaning = "link"; }
> led3 { led-meaning = "err"; }
> led4 { led-meaning = "activity"; what-activity = "all_harddisks"; }
> ethernet { leds = < &led2, &led3 >; }
>
> Dunno. I'd expect all the LED description in the LED node, not part of
> the description there and back link from the device

Driven by the feeling that it's all going in a wrong direction
I've done a bit more analysis - please refer below to my conclusions.

We already have a means for associating LEDs connected to standalone LED
controllers with other devices - this is LED trigger mechanism.
The association can be discovered by reading "trigger" sysfs file.

Device drivers can even create their own custom triggers, comprising
device node name or phy name in their names, which are known only during
driver probing, which allows to better describe the association than the
generic ones.

When it comes to devices with built-in LEDs - let's check how network
devices handle their LEDs. I've looked into ralink wireless driver:

drivers/net/wireless/ralink/rt2x00/rt2x00leds.c

It registers its LEDs in rt2x00leds_register(),
and composes names according to the following pattern:

snprintf(name, sizeof(name), "%s-%s::assoc",
rt2x00dev->ops->name, phy_name);

snprintf(name, sizeof(name), "%s-%s::quality",
rt2x00dev->ops->name, phy_name);

snprintf(name, sizeof(name), "%s-%s::radio",
rt2x00dev->ops->name, phy_name);

which results in the following LED names for wifi dongle:

rt73usb-phy16::assoc ->
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/leds/rt73usb-phy16::assoc
rt73usb-phy16::quality ->
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/leds/rt73usb-phy16::quality
rt73usb-phy16::radio ->
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/leds/rt73usb-phy16::radio

As it is shown, each entry under /sys/class/CLASS_NAME is a symbolic
link to the class device sub-directory located in the parent device
directory. In this case, even if we hadn't phy name in the LED name,
we could retrieve it by traversing parent device directory in the
sysfs:

../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/ieee80211/phy16/

We have also available additional device and driver specific info
in the uevent file:

../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/uevent:DEVTYPE=usb_interface
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/uevent:DRIVER=rt73usb
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/uevent:PRODUCT=148f/2573/1
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/uevent:TYPE=0/0/0
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/uevent:INTERFACE=255/255/255
../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8:1.0/uevent:MODALIAS=usb:v148Fp2573d0001dc00dsc00dp00icFFiscFFipFFin00

In addition to that the driver registers also its own triggers,
that associate the LEDs it exposes with the particular phy and the
function: phy16rx phy16tx phy16assoc phy16radio.

This is the pattern to be followed also in case user wants to
associate standalone LEDs with the device. Driver of a device
that lacks onboard LEDs would have to register suitable trigger
and that's it.

In case of keyboard LEDs we have similar symlinks:

input0::capslock ->
../../devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.0/0003:1C4F:0002.0001/input/input0/input0::capslock
input0::numlock ->
../../devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.0/0003:1C4F:0002.0001/input/input0/input0::numlock
input0::scrolllock ->
../../devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.0/0003:1C4F:0002.0001/input/input0/input0::scrolllock

#cat
/sys/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.0/0003:1C4F:0002.0001/uevent

DRIVER=hid-generic
HID_ID=0003:00001C4F:00000002
HID_NAME=SIGMACHIP USB Keyboard
HID_PHYS=usb-0000:00:14.0-3/input0
HID_UNIQ=
MODALIAS=hid:b0003g0001v00001C4Fp00000002

Having all the above it seems that we can safely remove devicename
section from LED class device naming pattern. Also no other links
in DT will be needed if we will entirely rely on LED trigger mechanism.

--
Best regards,
Jacek Anaszewski