Messages in this thread | | | From | Jacek Anaszewski <> | Subject | Re: [PATCH 04/24] dt-bindings: leds: Add function and color properties | Date | Sun, 23 Dec 2018 21:11:48 +0100 |
| |
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 <pavel@ucw.cz> 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
| |