linux-kernel - Re: [PATCH 1/3] dt-bindings: leds: add LED_FUNCTION

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 3 Apr 2024 10:31:19 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Gergo Koteles <soyer@....hu>, Ike Panhc <ike.pan@...onical.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-leds@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: leds: add LED_FUNCTION_FNLOCK

Hi Krzysztof,

On 4/2/24 3:55 PM, Krzysztof Kozlowski wrote:
> On 02/04/2024 15:21, Gergo Koteles wrote:
>> Newer laptops have FnLock LED.
>>
>> Add a define for this very common function.
>>
>> Signed-off-by: Gergo Koteles <soyer@....hu>
>> ---
>>  include/dt-bindings/leds/common.h | 1 +
> 
> Do we really need to define all these possible LED functions? Please
> link to DTS user for this.

It is useful to have well established names for common
LED functions instead of having each driver come up
with its own name with slightly different spelling
for various fixed function LEDs.

This is even documented in:

Documentation/leds/leds-class.rst :

"""
LED Device Naming
=================

Is currently of the form:

        "devicename:color:function"

..

- function:
        one of LED_FUNCTION_* definitions from the header
        include/dt-bindings/leds/common.h.
"""

Note this even specifies these definitions should go into
include/dt-bindings/leds/common.h .

In this case there is no dts user (yet) only an in kernel
driver which wants to use a LED_FUNCTION_* define to
establish how to identify FN-lock LEDs going forward.

Since a lot of LED_FUNCTION_* defines happen to be used
in dts files these happen to live under include/dt-bindings/
but the dts files are not the only consumer of these defines (1).

IMHO having a hard this must be used in a dts file rule
is not helpful for these kinda files with defines shared
between dts and non dts cases.

If we were to follow this logic then any addition to

include/uapi/linux/input-event-codes.h

must have a dts user before being approved too ? Since
that file is included from include/dt-bindings/input/input.h ?

TL;DR: not only is this patch fine, this is actually
the correct place to add such a define according to
the docs in Documentation/leds/leds-class.rst :

Reviewed-by: Hans de Goede <hdegoede@...hat.com>

Regards,

Hans

1) These defines are also used in:

drivers/hid/hid-playstation.c
drivers/hid/hid-nintendo.c
drivers/platform/x86/ideapad-laptop.c
drivers/leds/leds-cht-wcove.c
drivers/leds/simple/simatic-ipc-leds.c
drivers/leds/simple/simatic-ipc-leds-gpio-core.c