| Message ID | 8ac95e85a53dc0b8cce1e27fc1cab6d19221543b.1712063200.git.soyer@irl.hu (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | add FnLock LED class device to ideapad laptops | expand |
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. Best regards, Krzysztof
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
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
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
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
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
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
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
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
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
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 --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"
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(+)