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

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Marek Behun <marek.behun@nic.cz>
Cc: Adrian Schmutzler <freifunk@adrianschmutzler.de>,
	linux-leds@vger.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: leds: add LED_FUNCTION for wlan2g/wlan5g
Date: Sun, 20 Sep 2020 19:44:59 +0200	[thread overview]
Message-ID: <8a14bce6-ae0d-d714-b431-786b2696bd88@gmail.com> (raw)
In-Reply-To: <20200920172848.3e49d613@nic.cz>

On 9/20/20 5:28 PM, Marek Behun wrote:
> On Sun, 20 Sep 2020 16:59:01 +0200
> Jacek Anaszewski <jacek.anaszewski@gmail.com> 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@gmail.com/
>>
> 
> Marek
> 

-- 
Best regards,
Jacek Anaszewski