add FnLock LED class device to ideapad laptops

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.

Hi Krzysztof,

Quoted 1 line · Krzysztof Kozlowski

Quoted 3 lines

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

Quoted 8 lines · Gergo Koteles

Hi Krzysztof,

Quoted 4 lines · Krzysztof Kozlowski

Do we really need to define all these possible LED functions? Please link to DTS user for this.

Quoted 4 lines

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.

Quoted 7 lines · Gergo Koteles

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.

Hi Krzysztof,

Quoted 14 lines · Krzysztof Kozlowski

Quoted 12 lines · Gergo Koteles

Hi Krzysztof,

Quoted 4 lines · Krzysztof Kozlowski

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.

Quoted 3 lines

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.

Hi Krzysztof,

Quoted 11 lines · Krzysztof Kozlowski

Quoted 7 lines · Gergo Koteles

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

Quoted 40 lines · Hans de Goede

Hi Krzysztof,

Quoted 11 lines · Krzysztof Kozlowski

Quoted 7 lines · Gergo Koteles

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

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

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.

Quoted 1 line

Quoted 5 lines

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

Quoted 12 lines

Quoted 3 lines

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.

Quoted 5 lines

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 :

Quoted 3 lines

Quoted 1 line

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

Quoted 14 lines · Hans de Goede

Quoted 14 lines

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 4 lines

Quoted 1 line

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.

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

drivers/platform/x86/ideapad-laptop.c +27 -11

index 901849810ce2..529df08af548 100644
@@ -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) {

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

drivers/platform/x86/ideapad-laptop.c +27 -11

index 901849810ce2..529df08af548 100644
@@ -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) {

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

drivers/platform/x86/ideapad-laptop.c +96 -1

index 529df08af548..8a5bef4eedfe 100644
@@ -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);

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(+)

Hi All,

This patch series adds a new LED_FUNCTION_FNLOCK define as "fnlock" and adds a new FnLock LED class device into the ideapad-laptop driver.

This helps to display FnLock LED status in OSD or other places.

Best regards,
Gergo

Gergo Koteles (3):
dt-bindings: leds: add LED_FUNCTION_FNLOCK platform/x86: ideapad-laptop: add fn_lock_get/set functions platform/x86: ideapad-laptop: add FnLock LED class device

 drivers/platform/x86/ideapad-laptop.c | 133 +++++++++++++++++++++++---
 include/dt-bindings/leds/common.h     |   1 +
 2 files changed, 123 insertions(+), 11 deletions(-)

base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f