diff mbox series

[1/3] dt-bindings: leds: add LED_FUNCTION_FNLOCK

Message ID 8ac95e85a53dc0b8cce1e27fc1cab6d19221543b.1712063200.git.soyer@irl.hu
State New
Headers show
Series [1/3] dt-bindings: leds: add LED_FUNCTION_FNLOCK | expand

Commit Message

Gergo Koteles April 2, 2024, 1:21 p.m. UTC
Newer laptops have FnLock LED.

Add a define for this very common function.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 include/dt-bindings/leds/common.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Gergo Koteles April 2, 2024, 2:36 p.m. UTC | #1
Hi Krzysztof,

On Tue, 2024-04-02 at 15:55 +0200, Krzysztof Kozlowski wrote:
> 
> Do we really need to define all these possible LED functions? Please
> link to DTS user for this.
> 

I think for userspace it's easier to support an LED with a specified
name than to use various sysfs attributes. LED devices are easy to find
because they available are in the /sys/class/leds/ directory.
So I think it's a good thing to define LED names somewhere.

J Luke missed this LED from /sys/class/leds/, that's where the idea
came from. The scrollock, numlock, capslock and kbd_backlight LEDs are
already exported.

https://github.com/tomsom/yoga-linux/issues/58#issuecomment-2029926094

Best regards,
Gergo
Krzysztof Kozlowski April 2, 2024, 6:08 p.m. UTC | #2
On 02/04/2024 16:36, Gergo Koteles wrote:
> Hi Krzysztof,
> 
> On Tue, 2024-04-02 at 15:55 +0200, Krzysztof Kozlowski wrote:
>>
>> Do we really need to define all these possible LED functions? Please
>> link to DTS user for this.
>>
> 
> I think for userspace it's easier to support an LED with a specified
> name than to use various sysfs attributes. LED devices are easy to find
> because they available are in the /sys/class/leds/ directory.
> So I think it's a good thing to define LED names somewhere.

You did not add anything for user-space, but DT bindings. We do not keep
here anything for user-space.

> 
> J Luke missed this LED from /sys/class/leds/, that's where the idea
> came from. The scrollock, numlock, capslock and kbd_backlight LEDs are
> already exported.
> 
> https://github.com/tomsom/yoga-linux/issues/58#issuecomment-2029926094


I see there sysfs, so what does it have to do with bindings?

Again, please link to in-tree or in-review DTS.

Best regards,
Krzysztof
Gergo Koteles April 2, 2024, 6:50 p.m. UTC | #3
Hi Krzysztof,

On Tue, 2024-04-02 at 20:08 +0200, Krzysztof Kozlowski wrote:
> On 02/04/2024 16:36, Gergo Koteles wrote:
> > Hi Krzysztof,
> > 
> > On Tue, 2024-04-02 at 15:55 +0200, Krzysztof Kozlowski wrote:
> > > 
> > > Do we really need to define all these possible LED functions? Please
> > > link to DTS user for this.
> > > 
> > 
> > I think for userspace it's easier to support an LED with a specified
> > name than to use various sysfs attributes. LED devices are easy to find
> > because they available are in the /sys/class/leds/ directory.
> > So I think it's a good thing to define LED names somewhere.
> 
> You did not add anything for user-space, but DT bindings. We do not keep
> here anything for user-space.
> 

The LED_FUNCTION_KBD_BACKLIGHT confused me. Ok, this shouldn't be here,
I will remove it from v2.

Thanks,
Gergo
Hans de Goede April 3, 2024, 8:31 a.m. UTC | #4
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@irl.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@redhat.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
Hans de Goede April 3, 2024, 8:33 a.m. UTC | #5
Hi George,

On 4/2/24 8:50 PM, Gergo Koteles wrote:
> Hi Krzysztof,
> 
> On Tue, 2024-04-02 at 20:08 +0200, Krzysztof Kozlowski wrote:
>> On 02/04/2024 16:36, Gergo Koteles wrote:
>>> Hi Krzysztof,
>>>
>>> On Tue, 2024-04-02 at 15:55 +0200, Krzysztof Kozlowski wrote:
>>>>
>>>> Do we really need to define all these possible LED functions? Please
>>>> link to DTS user for this.
>>>>
>>>
>>> I think for userspace it's easier to support an LED with a specified
>>> name than to use various sysfs attributes. LED devices are easy to find
>>> because they available are in the /sys/class/leds/ directory.
>>> So I think it's a good thing to define LED names somewhere.
>>
>> You did not add anything for user-space, but DT bindings. We do not keep
>> here anything for user-space.
>>
> 
> The LED_FUNCTION_KBD_BACKLIGHT confused me. Ok, this shouldn't be here,
> I will remove it from v2.

I don't believe that is necessary, see my direct reply to Krzysztof first
email about this. According to Documentation/leds/leds-class.rst
you did exactly the right thing.

Also thank you for your interesting contribution. I have only briefly
looked over your other 2 patches, but I like the concept.

I'll hopefully have time to do a full review coming Monday.

Regards,

Hans
Krzysztof Kozlowski April 3, 2024, 8:36 a.m. UTC | #6
On 03/04/2024 10:31, Hans de Goede wrote:
> 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@irl.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.

Ack, reasonable.

> 
> 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).

Yes, but if there was no DTS consumer at all, then it is not a binding,
so it should not go to include/dt-bindings.

> 
> 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 ?

Wait, that's UAPI :) and we just share the constants. That's kind of
special case, but I get what you mean.

> 
> 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@redhat.com>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Hans de Goede April 3, 2024, 8:39 a.m. UTC | #7
Hi,

On 4/3/24 10:36 AM, Krzysztof Kozlowski wrote:
> On 03/04/2024 10:31, Hans de Goede wrote:
>> 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@irl.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.
> 
> Ack, reasonable.
> 
>>
>> 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).
> 
> Yes, but if there was no DTS consumer at all, then it is not a binding,
> so it should not go to include/dt-bindings.
> 
>>
>> 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 ?
> 
> Wait, that's UAPI :) and we just share the constants. That's kind of
> special case, but I get what you mean.
> 
>>
>> 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@redhat.com>
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks. Is it ok for me to merge this through the pdx86
tree (once I've reviewed the other 2 patches) ?

Regards,

Hans
Krzysztof Kozlowski April 3, 2024, 8:46 a.m. UTC | #8
On 03/04/2024 10:39, Hans de Goede wrote:
>>>
>>> must have a dts user before being approved too ? Since
>>> that file is included from include/dt-bindings/input/input.h ?
>>
>> Wait, that's UAPI :) and we just share the constants. That's kind of
>> special case, but I get what you mean.
>>
>>>
>>> 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@redhat.com>
>>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Thanks. Is it ok for me to merge this through the pdx86
> tree (once I've reviewed the other 2 patches) ?

You need to sync (ack) with LED folks, because by default this should go
via LED subsystem.

Best regards,
Krzysztof
Hans de Goede April 3, 2024, 8:51 a.m. UTC | #9
Hi,

On 4/3/24 10:46 AM, Krzysztof Kozlowski wrote:
> On 03/04/2024 10:39, Hans de Goede wrote:
>>>>
>>>> must have a dts user before being approved too ? Since
>>>> that file is included from include/dt-bindings/input/input.h ?
>>>
>>> Wait, that's UAPI :) and we just share the constants. That's kind of
>>> special case, but I get what you mean.
>>>
>>>>
>>>> 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@redhat.com>
>>>
>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> Thanks. Is it ok for me to merge this through the pdx86
>> tree (once I've reviewed the other 2 patches) ?
> 
> You need to sync (ack) with LED folks, because by default this should go
> via LED subsystem.

Ok, will do.

Pavel, Lee can you give me an ack for merging this through the pdx86 tree?

Regards,

Hans
Lee Jones April 11, 2024, 7:13 a.m. UTC | #10
On Tue, 02 Apr 2024, Gergo Koteles wrote:

> Newer laptops have FnLock LED.
> 
> Add a define for this very common function.
> 
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> ---
>  include/dt-bindings/leds/common.h | 1 +
>  1 file changed, 1 insertion(+)

Hans,

You can take this in via the x86 tree, but please capitalise the first
letter of the subject line when doing so.

 dt-bindings: leds: Add LED_FUNCTION_FNLOCK

Acked-by: Lee Jones <lee@kernel.org>
diff mbox series

Patch

diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index ecea167930d9..d7c980bdf383 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -46,6 +46,7 @@ 
 #define LED_FUNCTION_CAPSLOCK "capslock"
 #define LED_FUNCTION_SCROLLLOCK "scrolllock"
 #define LED_FUNCTION_NUMLOCK "numlock"
+#define LED_FUNCTION_FNLOCK "fnlock"
 /*   Obsolete equivalents: "tpacpi::thinklight" (IBM/Lenovo Thinkpads),
      "lp5523:kb{1,2,3,4,5,6}" (Nokia N900) */
 #define LED_FUNCTION_KBD_BACKLIGHT "kbd_backlight"