linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh@kernel.org>
Cc: Linux LED Subsystem <linux-leds@vger.kernel.org>,
	devicetree@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Baolin Wang <baolin.wang@linaro.org>,
	Daniel Mack <daniel@zonque.org>, Dan Murphy <dmurphy@ti.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Oleh Kravchenko <oleg@kaa.org.ua>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Simon Shields <simon@lineageos.org>
Subject: Re: [PATCH 04/24] dt-bindings: leds: Add function and color properties
Date: Sun, 23 Dec 2018 21:11:48 +0100	[thread overview]
Message-ID: <5084ad0d-8673-8151-d676-b5c60dd510ee@gmail.com> (raw)
In-Reply-To: <20181212183253.GA16578@amd>

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

  reply	other threads:[~2018-12-23 20:11 UTC|newest]

Thread overview: 105+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-06 22:07 [PATCH 00/24] Add generic support for composing LED class device name Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 01/24] leds: class: Improve LED and LED flash class registration API Jacek Anaszewski
2018-11-07  6:55   ` Baolin Wang
2018-11-07 20:51     ` Jacek Anaszewski
2018-11-08 17:50   ` Dan Murphy
2018-11-08 20:47     ` Jacek Anaszewski
2018-11-11 11:31   ` Pavel Machek
2018-11-06 22:07 ` [PATCH 02/24] leds: core: Add support for composing LED class device names Jacek Anaszewski
2018-11-07  7:20   ` Baolin Wang
2018-11-08 20:47     ` Jacek Anaszewski
2018-11-09  2:35       ` Baolin Wang
2018-11-08 18:06   ` Dan Murphy
2018-11-08 20:48     ` Jacek Anaszewski
2018-11-11 12:02   ` Pavel Machek
2018-11-11 20:02     ` Jacek Anaszewski
2018-11-11 20:16       ` Pavel Machek
2018-11-11 21:14         ` Jacek Anaszewski
2018-11-12  0:01           ` Vesa Jääskeläinen
2018-11-12 15:59             ` Jacek Anaszewski
2018-11-12 21:25             ` Linus Walleij
2018-11-12 22:11               ` LEDs on USB keyboards was " Pavel Machek
2018-11-12 10:35           ` Pavel Machek
2018-11-12 16:01             ` Jacek Anaszewski
2018-11-12 19:05               ` Pavel Machek
2018-11-12 20:11                 ` Jacek Anaszewski
2018-11-12 22:06                   ` Pavel Machek
2018-11-13 20:57                     ` Jacek Anaszewski
2018-11-13 21:38                     ` Geert Uytterhoeven
2018-11-13 21:50                       ` Pavel Machek
2018-11-12 21:23     ` Linus Walleij
2018-11-13 19:55       ` Jacek Anaszewski
2018-11-15  9:10         ` Linus Walleij
2018-11-06 22:07 ` [PATCH 03/24] leds: dt-bindings: Add LED_FUNCTION definitions Jacek Anaszewski
2018-11-07  8:36   ` Vokáč Michal
2018-11-07 19:28     ` Jacek Anaszewski
2018-11-08 15:13   ` Rob Herring
2018-11-08 21:03     ` Jacek Anaszewski
2018-11-11 11:31   ` Pavel Machek
2018-11-11 20:02     ` Jacek Anaszewski
2018-11-11 20:20       ` Pavel Machek
2018-11-11 21:16         ` Jacek Anaszewski
2018-11-12  0:25   ` Vesa Jääskeläinen
2018-11-12 16:01     ` Jacek Anaszewski
2018-11-15  9:16   ` Linus Walleij
2018-11-06 22:07 ` [PATCH 04/24] dt-bindings: leds: Add function and color properties Jacek Anaszewski
2018-11-08 18:00   ` Dan Murphy
2018-11-08 20:47     ` Jacek Anaszewski
2018-11-08 21:08       ` Dan Murphy
2018-11-09 20:00         ` Jacek Anaszewski
2018-11-09  8:32   ` Vesa Jääskeläinen
2018-11-09 20:42     ` Jacek Anaszewski
2018-11-10 17:19       ` Vesa Jääskeläinen
2018-11-12 16:02         ` Jacek Anaszewski
2018-11-13  7:10           ` Vesa Jääskeläinen
2018-11-13 19:02             ` Jacek Anaszewski
     [not found]   ` <5bea0eb8.1c69fb81.6b921.80e6@mx.google.com>
2018-11-13 20:57     ` Jacek Anaszewski
2018-11-27 20:37       ` Jacek Anaszewski
2018-11-30 14:38         ` Rob Herring
2018-11-30 21:08           ` Pavel Machek
2018-11-30 22:19             ` Rob Herring
2018-12-01 21:17               ` Jacek Anaszewski
2018-12-11 17:27                 ` Rob Herring
2018-12-11 22:44                   ` Pavel Machek
2018-12-21 10:12                   ` Linus Walleij
2018-12-12  9:59           ` Pavel Machek
2018-12-12 13:56             ` Rob Herring
2018-12-12 18:32               ` Pavel Machek
2018-12-23 20:11                 ` Jacek Anaszewski [this message]
2018-11-06 22:07 ` [PATCH 05/24] dt-bindings: sc27xx-blt: " Jacek Anaszewski
2018-11-11 14:29   ` Pavel Machek
2018-11-11 20:03     ` Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 06/24] leds: sc27xx-blt: Use led_compose_name() Jacek Anaszewski
2018-11-07  7:22   ` Baolin Wang
2018-11-11 14:31   ` Pavel Machek
2018-11-06 22:07 ` [PATCH 07/24] dt-bindings: lt3593: Add function and color properties Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 08/24] leds: lt3593: Use led_compose_name() Jacek Anaszewski
2018-11-07 19:37   ` Daniel Mack
2018-11-07 19:49     ` Jacek Anaszewski
2018-11-08  8:33       ` Daniel Mack
2018-11-06 22:07 ` [PATCH 09/24] dt-bindings: lp8860: Add function and color properties Jacek Anaszewski
2018-11-08 18:01   ` Dan Murphy
2018-11-06 22:07 ` [PATCH 10/24] leds: lp8860: Use led_compose_name() Jacek Anaszewski
2018-11-08 18:16   ` Dan Murphy
2018-11-08 20:48     ` Jacek Anaszewski
2018-11-12 14:05       ` Dan Murphy
2018-11-06 22:07 ` [PATCH 11/24] dt-bindings: lm3692x: Add function and color properties Jacek Anaszewski
2018-11-08 18:12   ` Dan Murphy
2018-11-06 22:07 ` [PATCH 12/24] leds: lm3692x: Use led_compose_name() Jacek Anaszewski
2018-11-08 18:14   ` Dan Murphy
2018-11-08 20:48     ` Jacek Anaszewski
2018-11-08 21:11       ` Dan Murphy
2018-11-11 14:31   ` Pavel Machek
2018-11-06 22:07 ` [PATCH 13/24] dt-bindings: lm36010: Add function and color properties Jacek Anaszewski
2018-11-08 18:15   ` Dan Murphy
2018-11-06 22:07 ` [PATCH 14/24] leds: lm3601x: Use led_compose_name() Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 15/24] dt-bindings: cr0014114: Add function and color properties Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 16/24] leds: cr0014114: Use led_compose_name() Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 17/24] dt-bindings: aat1290: Add function and color properties Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 18/24] leds: aat1290: Use led_compose_name() Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 19/24] dt-bindings: as3645a: Add function and color properties Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 20/24] leds: as3645a: Use led_compose_name() Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 21/24] dt-bindings: leds-gpio: Add function and color properties Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 22/24] leds: gpio: Use led_compose_name() Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 23/24] dt-bindings: an30259a: Add function and color properties Jacek Anaszewski
2018-11-06 22:07 ` [PATCH 24/24] leds: an30259a: Use led_compose_name() Jacek Anaszewski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5084ad0d-8673-8151-d676-b5c60dd510ee@gmail.com \
    --to=jacek.anaszewski@gmail.com \
    --cc=baolin.wang@linaro.org \
    --cc=daniel@zonque.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=oleg@kaa.org.ua \
    --cc=pavel@ucw.cz \
    --cc=robh@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=simon@lineageos.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).