LinuxLists.cc - [PATCH 0/3] add FnLock LED class device to ideapad laptops

[permalink] [raw]

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

Newer laptops have FnLock LED.

Add a define for this very common function.

Signed-off-by: Gergo Koteles <[email protected]>
---
include/dt-bindings/leds/common.h | 1 +
1 file changed, 1 insertion(+)

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"
--
2.44.0

2024-04-02 13:21:54

[permalink] [raw]

Subject: [PATCH 2/3] platform/x86: ideapad-laptop: add fn_lock_get/set functions

The FnLock is retrieved and set in several places in the code.

Move details into ideapad_fn_lock_get and ideapad_fn_lock_set functions.

Signed-off-by: Gergo Koteles <[email protected]>
---
drivers/platform/x86/ideapad-laptop.c | 38 +++++++++++++++++++--------
1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 901849810ce2..529df08af548 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -513,11 +513,8 @@ static ssize_t fan_mode_store(struct device *dev,

static DEVICE_ATTR_RW(fan_mode);

-static ssize_t fn_lock_show(struct device *dev,
- struct device_attribute *attr,
- char *buf)
+static int ideapad_fn_lock_get(struct ideapad_private *priv)
{
- struct ideapad_private *priv = dev_get_drvdata(dev);
unsigned long hals;
int err;

@@ -525,7 +522,27 @@ static ssize_t fn_lock_show(struct device *dev,
if (err)
return err;

- return sysfs_emit(buf, "%d\n", !!test_bit(HALS_FNLOCK_STATE_BIT, &hals));
+ return !!test_bit(HALS_FNLOCK_STATE_BIT, &hals);
+}
+
+static int ideapad_fn_lock_set(struct ideapad_private *priv, bool state)
+{
+ return exec_sals(priv->adev->handle,
+ state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
+}
+
+static ssize_t fn_lock_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ideapad_private *priv = dev_get_drvdata(dev);
+ int brightness;
+
+ brightness = ideapad_fn_lock_get(priv);
+ if (brightness < 0)
+ return brightness;
+
+ return sysfs_emit(buf, "%d\n", brightness);
}

static ssize_t fn_lock_store(struct device *dev,
@@ -540,7 +557,7 @@ static ssize_t fn_lock_store(struct device *dev,
if (err)
return err;

- err = exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
+ err = ideapad_fn_lock_set(priv, state);
if (err)
return err;

@@ -1709,7 +1726,6 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
{
struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
struct ideapad_private *priv;
- unsigned long result;

mutex_lock(&ideapad_shared_mutex);

@@ -1722,11 +1738,11 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
ideapad_input_report(priv, 128);
break;
case IDEAPAD_WMI_EVENT_FN_KEYS:
- if (priv->features.set_fn_lock_led &&
- !eval_hals(priv->adev->handle, &result)) {
- bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
+ if (priv->features.set_fn_lock_led) {
+ int brightness = ideapad_fn_lock_get(priv);

- exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
+ if (brightness >= 0)
+ ideapad_fn_lock_set(priv, brightness);
}

if (data->type != ACPI_TYPE_INTEGER) {
--
2.44.0

2024-04-02 13:21:59

[permalink] [raw]

Subject: [PATCH 3/3] platform/x86: ideapad-laptop: add FnLock LED class device

Some Ideapad/Yoga Laptops have an FnLock LED in the Esc key.

Expose Fnlock as an LED class device for easier OSD support.

Signed-off-by: Gergo Koteles <[email protected]>
---
drivers/platform/x86/ideapad-laptop.c | 97 ++++++++++++++++++++++++++-
1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 529df08af548..8a5bef4eedfe 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -152,6 +152,11 @@ struct ideapad_private {
struct led_classdev led;
unsigned int last_brightness;
} kbd_bl;
+ struct {
+ bool initialized;
+ struct led_classdev led;
+ unsigned int last_brightness;
+ } fn_lock;
};

static bool no_bt_rfkill;
@@ -531,6 +536,19 @@ static int ideapad_fn_lock_set(struct ideapad_private *priv, bool state)
state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
}

+static void ideapad_fn_lock_led_notify(struct ideapad_private *priv, int brightness)
+{
+ if (!priv->fn_lock.initialized)
+ return;
+
+ if (brightness == priv->fn_lock.last_brightness)
+ return;
+
+ priv->fn_lock.last_brightness = brightness;
+
+ led_classdev_notify_brightness_hw_changed(&priv->fn_lock.led, brightness);
+}
+
static ssize_t fn_lock_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -561,6 +579,8 @@ static ssize_t fn_lock_store(struct device *dev,
if (err)
return err;

+ ideapad_fn_lock_led_notify(priv, state);
+
return count;
}

@@ -1479,6 +1499,65 @@ static void ideapad_kbd_bl_exit(struct ideapad_private *priv)
led_classdev_unregister(&priv->kbd_bl.led);
}

+/*
+ * FnLock LED
+ */
+static enum led_brightness ideapad_fn_lock_led_cdev_get(struct led_classdev *led_cdev)
+{
+ struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, fn_lock.led);
+
+ return ideapad_fn_lock_get(priv);
+}
+
+static int ideapad_fn_lock_led_cdev_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, fn_lock.led);
+
+ return ideapad_fn_lock_set(priv, brightness);
+}
+
+static int ideapad_fn_lock_led_init(struct ideapad_private *priv)
+{
+ int brightness, err;
+
+ if (!priv->features.fn_lock)
+ return -ENODEV;
+
+ if (WARN_ON(priv->fn_lock.initialized))
+ return -EEXIST;
+
+ priv->fn_lock.led.max_brightness = 1;
+
+ brightness = ideapad_fn_lock_get(priv);
+ if (brightness < 0)
+ return brightness;
+
+ priv->fn_lock.last_brightness = brightness;
+ priv->fn_lock.led.name = "platform::" LED_FUNCTION_FNLOCK;
+ priv->fn_lock.led.brightness_get = ideapad_fn_lock_led_cdev_get;
+ priv->fn_lock.led.brightness_set_blocking = ideapad_fn_lock_led_cdev_set;
+ priv->fn_lock.led.flags = LED_BRIGHT_HW_CHANGED;
+
+ err = led_classdev_register(&priv->platform_device->dev, &priv->fn_lock.led);
+ if (err)
+ return err;
+
+ priv->fn_lock.initialized = true;
+
+ return 0;
+}
+
+static void ideapad_fn_lock_led_exit(struct ideapad_private *priv)
+{
+ if (!priv->fn_lock.initialized)
+ return;
+
+ priv->fn_lock.initialized = false;
+
+ led_classdev_unregister(&priv->fn_lock.led);
+}
+
/*
* module init/exit
*/
@@ -1741,8 +1820,10 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
if (priv->features.set_fn_lock_led) {
int brightness = ideapad_fn_lock_get(priv);

- if (brightness >= 0)
+ if (brightness >= 0) {
ideapad_fn_lock_set(priv, brightness);
+ ideapad_fn_lock_led_notify(priv, brightness);
+ }
}

if (data->type != ACPI_TYPE_INTEGER) {
@@ -1754,6 +1835,10 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
data->integer.value);

+ /* 0x02 FnLock, 0x03 Esc */
+ if (data->integer.value == 0x02 || data->integer.value == 0x03)
+ ideapad_fn_lock_led_notify(priv, data->integer.value == 0x02);
+
ideapad_input_report(priv,
data->integer.value | IDEAPAD_WMI_KEY);

@@ -1847,6 +1932,14 @@ static int ideapad_acpi_add(struct platform_device *pdev)
dev_info(&pdev->dev, "Keyboard backlight control not available\n");
}

+ err = ideapad_fn_lock_led_init(priv);
+ if (err) {
+ if (err != -ENODEV)
+ dev_warn(&pdev->dev, "Could not set up FnLock LED: %d\n", err);
+ else
+ dev_info(&pdev->dev, "FnLock control not available\n");
+ }
+
/*
* On some models without a hw-switch (the yoga 2 13 at least)
* VPCCMD_W_RF must be explicitly set to 1 for the wifi to work.
@@ -1903,6 +1996,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
ideapad_unregister_rfkill(priv, i);

+ ideapad_fn_lock_led_exit(priv);
ideapad_kbd_bl_exit(priv);
ideapad_input_exit(priv);

@@ -1930,6 +2024,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev)
for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
ideapad_unregister_rfkill(priv, i);

+ ideapad_fn_lock_led_exit(priv);
ideapad_kbd_bl_exit(priv);
ideapad_input_exit(priv);
ideapad_debugfs_exit(priv);
--
2.44.0

2024-04-02 13:56:14

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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

2024-04-02 15:12:26

[permalink] [raw]

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

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

2024-04-02 18:08:46

[permalink] [raw]

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

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

2024-04-02 18:57:58

[permalink] [raw]

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

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

2024-04-03 08:36:00

[permalink] [raw]

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

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

2024-04-03 08:37:16

[permalink] [raw]

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

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 <[email protected]>
>>> ---
>>> 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 <[email protected]>

Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2024-04-03 08:40:28

[permalink] [raw]

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

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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>
> Acked-by: Krzysztof Kozlowski <[email protected]>

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

Regards,

Hans

2024-04-03 08:47:14

[permalink] [raw]

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

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 <[email protected]>
>>
>> Acked-by: Krzysztof Kozlowski <[email protected]>
>
> 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

2024-04-03 08:51:37

[permalink] [raw]

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

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 <[email protected]>
>>>
>>> Acked-by: Krzysztof Kozlowski <[email protected]>
>>
>> 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

2024-04-03 09:04:05

[permalink] [raw]

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 <[email protected]>
>> ---
>> 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 <[email protected]>

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

2024-04-08 15:49:39

[permalink] [raw]

Subject: Re: [PATCH 2/3] platform/x86: ideapad-laptop: add fn_lock_get/set functions

Hi,

On 4/2/24 3:21 PM, Gergo Koteles wrote:
> The FnLock is retrieved and set in several places in the code.
>
> Move details into ideapad_fn_lock_get and ideapad_fn_lock_set functions.
>
> Signed-off-by: Gergo Koteles <[email protected]>

> ---
> drivers/platform/x86/ideapad-laptop.c | 38 +++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 901849810ce2..529df08af548 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -513,11 +513,8 @@ static ssize_t fan_mode_store(struct device *dev,
>
> static DEVICE_ATTR_RW(fan_mode);
>
> -static ssize_t fn_lock_show(struct device *dev,
> - struct device_attribute *attr,
> - char *buf)
> +static int ideapad_fn_lock_get(struct ideapad_private *priv)
> {
> - struct ideapad_private *priv = dev_get_drvdata(dev);
> unsigned long hals;
> int err;
>
> @@ -525,7 +522,27 @@ static ssize_t fn_lock_show(struct device *dev,
> if (err)
> return err;
>
> - return sysfs_emit(buf, "%d\n", !!test_bit(HALS_FNLOCK_STATE_BIT, &hals));
> + return !!test_bit(HALS_FNLOCK_STATE_BIT, &hals);
> +}
> +
> +static int ideapad_fn_lock_set(struct ideapad_private *priv, bool state)
> +{
> + return exec_sals(priv->adev->handle,
> + state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> +}
> +
> +static ssize_t fn_lock_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ideapad_private *priv = dev_get_drvdata(dev);
> + int brightness;
> +
> + brightness = ideapad_fn_lock_get(priv);
> + if (brightness < 0)
> + return brightness;
> +
> + return sysfs_emit(buf, "%d\n", brightness);
> }
>
> static ssize_t fn_lock_store(struct device *dev,
> @@ -540,7 +557,7 @@ static ssize_t fn_lock_store(struct device *dev,
> if (err)
> return err;
>
> - err = exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> + err = ideapad_fn_lock_set(priv, state);
> if (err)
> return err;
>
> @@ -1709,7 +1726,6 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> {
> struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
> struct ideapad_private *priv;
> - unsigned long result;
>
> mutex_lock(&ideapad_shared_mutex);
>
> @@ -1722,11 +1738,11 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> ideapad_input_report(priv, 128);
> break;
> case IDEAPAD_WMI_EVENT_FN_KEYS:
> - if (priv->features.set_fn_lock_led &&
> - !eval_hals(priv->adev->handle, &result)) {
> - bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> + if (priv->features.set_fn_lock_led) {
> + int brightness = ideapad_fn_lock_get(priv);
>
> - exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> + if (brightness >= 0)
> + ideapad_fn_lock_set(priv, brightness);
> }
>
> if (data->type != ACPI_TYPE_INTEGER) {

2024-04-08 15:50:10

[permalink] [raw]

Subject: Re: [PATCH 2/3] platform/x86: ideapad-laptop: add fn_lock_get/set functions

<sorry about the previous empty email, I hit send too soon>

Hi,

On 4/2/24 3:21 PM, Gergo Koteles wrote:
> The FnLock is retrieved and set in several places in the code.
>
> Move details into ideapad_fn_lock_get and ideapad_fn_lock_set functions.
>
> Signed-off-by: Gergo Koteles <[email protected]>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <[email protected]>

Regards,

Hans

> ---
> drivers/platform/x86/ideapad-laptop.c | 38 +++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 901849810ce2..529df08af548 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -513,11 +513,8 @@ static ssize_t fan_mode_store(struct device *dev,
>
> static DEVICE_ATTR_RW(fan_mode);
>
> -static ssize_t fn_lock_show(struct device *dev,
> - struct device_attribute *attr,
> - char *buf)
> +static int ideapad_fn_lock_get(struct ideapad_private *priv)
> {
> - struct ideapad_private *priv = dev_get_drvdata(dev);
> unsigned long hals;
> int err;
>
> @@ -525,7 +522,27 @@ static ssize_t fn_lock_show(struct device *dev,
> if (err)
> return err;
>
> - return sysfs_emit(buf, "%d\n", !!test_bit(HALS_FNLOCK_STATE_BIT, &hals));
> + return !!test_bit(HALS_FNLOCK_STATE_BIT, &hals);
> +}
> +
> +static int ideapad_fn_lock_set(struct ideapad_private *priv, bool state)
> +{
> + return exec_sals(priv->adev->handle,
> + state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> +}
> +
> +static ssize_t fn_lock_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ideapad_private *priv = dev_get_drvdata(dev);
> + int brightness;
> +
> + brightness = ideapad_fn_lock_get(priv);
> + if (brightness < 0)
> + return brightness;
> +
> + return sysfs_emit(buf, "%d\n", brightness);
> }
>
> static ssize_t fn_lock_store(struct device *dev,
> @@ -540,7 +557,7 @@ static ssize_t fn_lock_store(struct device *dev,
> if (err)
> return err;
>
> - err = exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> + err = ideapad_fn_lock_set(priv, state);
> if (err)
> return err;
>
> @@ -1709,7 +1726,6 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> {
> struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
> struct ideapad_private *priv;
> - unsigned long result;
>
> mutex_lock(&ideapad_shared_mutex);
>
> @@ -1722,11 +1738,11 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> ideapad_input_report(priv, 128);
> break;
> case IDEAPAD_WMI_EVENT_FN_KEYS:
> - if (priv->features.set_fn_lock_led &&
> - !eval_hals(priv->adev->handle, &result)) {
> - bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> + if (priv->features.set_fn_lock_led) {
> + int brightness = ideapad_fn_lock_get(priv);
>
> - exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> + if (brightness >= 0)
> + ideapad_fn_lock_set(priv, brightness);
> }
>
> if (data->type != ACPI_TYPE_INTEGER) {

2024-04-08 15:51:12

[permalink] [raw]

Subject: Re: [PATCH 3/3] platform/x86: ideapad-laptop: add FnLock LED class device

Hi,

On 4/2/24 3:21 PM, Gergo Koteles wrote:
> Some Ideapad/Yoga Laptops have an FnLock LED in the Esc key.
>
> Expose Fnlock as an LED class device for easier OSD support.
>
> Signed-off-by: Gergo Koteles <[email protected]>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <[email protected]>

Regards,

Hans

> ---
> drivers/platform/x86/ideapad-laptop.c | 97 ++++++++++++++++++++++++++-
> 1 file changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 529df08af548..8a5bef4eedfe 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -152,6 +152,11 @@ struct ideapad_private {
> struct led_classdev led;
> unsigned int last_brightness;
> } kbd_bl;
> + struct {
> + bool initialized;
> + struct led_classdev led;
> + unsigned int last_brightness;
> + } fn_lock;
> };
>
> static bool no_bt_rfkill;
> @@ -531,6 +536,19 @@ static int ideapad_fn_lock_set(struct ideapad_private *priv, bool state)
> state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> }
>
> +static void ideapad_fn_lock_led_notify(struct ideapad_private *priv, int brightness)
> +{
> + if (!priv->fn_lock.initialized)
> + return;
> +
> + if (brightness == priv->fn_lock.last_brightness)
> + return;
> +
> + priv->fn_lock.last_brightness = brightness;
> +
> + led_classdev_notify_brightness_hw_changed(&priv->fn_lock.led, brightness);
> +}
> +
> static ssize_t fn_lock_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -561,6 +579,8 @@ static ssize_t fn_lock_store(struct device *dev,
> if (err)
> return err;
>
> + ideapad_fn_lock_led_notify(priv, state);
> +
> return count;
> }
>
> @@ -1479,6 +1499,65 @@ static void ideapad_kbd_bl_exit(struct ideapad_private *priv)
> led_classdev_unregister(&priv->kbd_bl.led);
> }
>
> +/*
> + * FnLock LED
> + */
> +static enum led_brightness ideapad_fn_lock_led_cdev_get(struct led_classdev *led_cdev)
> +{
> + struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, fn_lock.led);
> +
> + return ideapad_fn_lock_get(priv);
> +}
> +
> +static int ideapad_fn_lock_led_cdev_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, fn_lock.led);
> +
> + return ideapad_fn_lock_set(priv, brightness);
> +}
> +
> +static int ideapad_fn_lock_led_init(struct ideapad_private *priv)
> +{
> + int brightness, err;
> +
> + if (!priv->features.fn_lock)
> + return -ENODEV;
> +
> + if (WARN_ON(priv->fn_lock.initialized))
> + return -EEXIST;
> +
> + priv->fn_lock.led.max_brightness = 1;
> +
> + brightness = ideapad_fn_lock_get(priv);
> + if (brightness < 0)
> + return brightness;
> +
> + priv->fn_lock.last_brightness = brightness;
> + priv->fn_lock.led.name = "platform::" LED_FUNCTION_FNLOCK;
> + priv->fn_lock.led.brightness_get = ideapad_fn_lock_led_cdev_get;
> + priv->fn_lock.led.brightness_set_blocking = ideapad_fn_lock_led_cdev_set;
> + priv->fn_lock.led.flags = LED_BRIGHT_HW_CHANGED;
> +
> + err = led_classdev_register(&priv->platform_device->dev, &priv->fn_lock.led);
> + if (err)
> + return err;
> +
> + priv->fn_lock.initialized = true;
> +
> + return 0;
> +}
> +
> +static void ideapad_fn_lock_led_exit(struct ideapad_private *priv)
> +{
> + if (!priv->fn_lock.initialized)
> + return;
> +
> + priv->fn_lock.initialized = false;
> +
> + led_classdev_unregister(&priv->fn_lock.led);
> +}
> +
> /*
> * module init/exit
> */
> @@ -1741,8 +1820,10 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> if (priv->features.set_fn_lock_led) {
> int brightness = ideapad_fn_lock_get(priv);
>
> - if (brightness >= 0)
> + if (brightness >= 0) {
> ideapad_fn_lock_set(priv, brightness);
> + ideapad_fn_lock_led_notify(priv, brightness);
> + }
> }
>
> if (data->type != ACPI_TYPE_INTEGER) {
> @@ -1754,6 +1835,10 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
> data->integer.value);
>
> + /* 0x02 FnLock, 0x03 Esc */
> + if (data->integer.value == 0x02 || data->integer.value == 0x03)
> + ideapad_fn_lock_led_notify(priv, data->integer.value == 0x02);
> +
> ideapad_input_report(priv,
> data->integer.value | IDEAPAD_WMI_KEY);
>
> @@ -1847,6 +1932,14 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> dev_info(&pdev->dev, "Keyboard backlight control not available\n");
> }
>
> + err = ideapad_fn_lock_led_init(priv);
> + if (err) {
> + if (err != -ENODEV)
> + dev_warn(&pdev->dev, "Could not set up FnLock LED: %d\n", err);
> + else
> + dev_info(&pdev->dev, "FnLock control not available\n");
> + }
> +
> /*
> * On some models without a hw-switch (the yoga 2 13 at least)
> * VPCCMD_W_RF must be explicitly set to 1 for the wifi to work.
> @@ -1903,6 +1996,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
> ideapad_unregister_rfkill(priv, i);
>
> + ideapad_fn_lock_led_exit(priv);
> ideapad_kbd_bl_exit(priv);
> ideapad_input_exit(priv);
>
> @@ -1930,6 +2024,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev)
> for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
> ideapad_unregister_rfkill(priv, i);
>
> + ideapad_fn_lock_led_exit(priv);
> ideapad_kbd_bl_exit(priv);
> ideapad_input_exit(priv);
> ideapad_debugfs_exit(priv);

2024-04-11 07:30:37

by Lee Jones

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

--
Lee Jones [李琼斯]

2024-04-15 13:41:02