[v2,6/7] platform/x86: ideapad-laptop: Keyboard backlight support for more IdeaPads
Commit Message
IdeaPads with HALS_KBD_BL_SUPPORT_BIT have full keyboard light support,
and they send an event via ACPI on light state change. Whereas some
IdeaPads that don't have this bit set, i.e. 520-15ikb, 330-17ich and
5 (15), don't send an event, yet they still support switching keyboard
light via KBLO object on DSDT. Detect these IdeaPads with searching for
KBLO object, set their kbd_bl_partial to true and register led device
for them. Tested on 520-15ikb.
Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
---
drivers/platform/x86/ideapad-laptop.c | 79 ++++++++++++++++++++++++---
1 file changed, 70 insertions(+), 9 deletions(-)
Comments
Hi Eray,
On 10/29/22 14:03, Eray Orçunus wrote:
> IdeaPads with HALS_KBD_BL_SUPPORT_BIT have full keyboard light support,
> and they send an event via ACPI on light state change. Whereas some
> IdeaPads that don't have this bit set, i.e. 520-15ikb, 330-17ich and
> 5 (15), don't send an event, yet they still support switching keyboard
> light via KBLO object on DSDT. Detect these IdeaPads with searching for
> KBLO object, set their kbd_bl_partial to true and register led device
> for them. Tested on 520-15ikb.
>
> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> ---
> drivers/platform/x86/ideapad-laptop.c | 79 ++++++++++++++++++++++++---
> 1 file changed, 70 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index e8c088e7a53d..b34fbc4d741c 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -149,6 +149,7 @@ struct ideapad_private {
> bool fn_lock : 1;
> bool hw_rfkill_switch : 1;
> bool kbd_bl : 1;
> + bool kbd_bl_partial : 1;
> bool cam_ctrl_via_ec : 1;
> bool touchpad_ctrl_via_ec : 1;
> bool usb_charging : 1;
> @@ -157,6 +158,9 @@ struct ideapad_private {
> bool initialized;
> struct led_classdev led;
> unsigned int last_brightness;
> + /* Below are used only if kbd_bl_partial is set */
> + acpi_handle lfcm_mutex;
> + acpi_handle kblo_obj;
> } kbd_bl;
> };
>
> @@ -1300,19 +1304,52 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
> backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY);
> }
>
> +#define IDEAPAD_ACPI_MUTEX_TIMEOUT 1500
> +
> /*
> * keyboard backlight
> */
> static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> {
> - unsigned long hals;
> + unsigned long ret_val;
> int err;
> + acpi_status status;
>
> - err = eval_hals(priv->adev->handle, &hals);
> + /*
> + * Some IdeaPads with partially implemented keyboard lights don't give
> + * us the light state on HALS_KBD_BL_STATE_BIT in the return value of HALS,
> + * i.e. 5 (15) and 330-17ich. Fortunately we know how to gather it.
> + * Even if it won't work, we will still give HALS a try, because
> + * some IdeaPads with kbd_bl_partial, i.e. 520-15ikb,
> + * correctly sets HALS_KBD_BL_STATE_BIT in HALS return value.
> + */
> +
> + if (priv->features.kbd_bl_partial &&
> + priv->kbd_bl.lfcm_mutex != NULL && priv->kbd_bl.kblo_obj != NULL) {
IMHO it would be better to only set kbd_bl_partial when both handles
are not NULL, then you can drop the handle checks here.
> +
> + status = acpi_acquire_mutex(priv->kbd_bl.lfcm_mutex, NULL,
> + IDEAPAD_ACPI_MUTEX_TIMEOUT);
> +
> + if (ACPI_SUCCESS(status)) {
This code now ends up still going through the normal kbd-bl path
when it fails to acquire the mutex.
Instead it should do:
if (ACPI_FAILURE(status))
return -EIO;
And then have the rest of the code one indentation level less
deep.
> + err = eval_int(priv->kbd_bl.kblo_obj, NULL, &ret_val);
> +
> + status = acpi_release_mutex(priv->kbd_bl.lfcm_mutex, NULL);
> + if (ACPI_FAILURE(status))
> + dev_err(&priv->platform_device->dev,
> + "Failed to release LFCM mutex");
I'm pretty sure that the ACPI core will already log an error if things
fail, I would change this to just a single line:
acpi_release_mutex(priv->kbd_bl.lfcm_mutex, NULL);
> +
> + if (err)
> + return err;
> +
> + return !!ret_val;
!!ret_val turns it into a boolean, does that mean it is always either on
or off with no level in between ?
> + }
> + }
> +
> + err = eval_hals(priv->adev->handle, &ret_val);
> if (err)
> return err;
>
> - return !!test_bit(HALS_KBD_BL_STATE_BIT, &hals);
> + return !!test_bit(HALS_KBD_BL_STATE_BIT, &ret_val);
> }
>
> static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
> @@ -1329,7 +1366,8 @@ static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned
> if (err)
> return err;
>
> - priv->kbd_bl.last_brightness = brightness;
> + if (!priv->features.kbd_bl_partial)
> + priv->kbd_bl.last_brightness = brightness;
>
> return 0;
> }
I don't understand this change, you change ideapad_kbd_bl_brightness_get()
to do an int eval of KBLO, but here you now still do a
exec_sals(SALS_KBD_BL_ON / SALS_KBD_BL_OFF) ?
Also there is no reason not to update last_brightness here ...
> @@ -1349,6 +1387,9 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> if (!priv->kbd_bl.initialized)
> return;
>
> + if (priv->features.kbd_bl_partial)
> + return;
> +
Why? If we do happen to get a notify on one of these devices and
the brightness has changed, then it would be good to let userspace
know and if never get a notify then this function won't run so
we don't need the if.
> brightness = ideapad_kbd_bl_brightness_get(priv);
> if (brightness < 0)
> return;
> @@ -1371,17 +1412,20 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> if (WARN_ON(priv->kbd_bl.initialized))
> return -EEXIST;
>
> - brightness = ideapad_kbd_bl_brightness_get(priv);
> - if (brightness < 0)
> - return brightness;
> + /* IdeaPads with kbd_bl_partial don't have keyboard backlight event */
> + if (!priv->features.kbd_bl_partial) {
> + brightness = ideapad_kbd_bl_brightness_get(priv);
> + if (brightness < 0)
> + return brightness;
>
> - priv->kbd_bl.last_brightness = brightness;
> + priv->kbd_bl.last_brightness = brightness;
> + priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED;
> + }
Again no need to for the if here. Setting last_brightness and advertising
LED_BRIGHT_HW_CHANGED unconditionally won't hurt and not making this difference
keeps the code simpler.
>
> priv->kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
> priv->kbd_bl.led.max_brightness = 1;
> priv->kbd_bl.led.brightness_get = ideapad_kbd_bl_led_cdev_brightness_get;
> priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
> - priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED;
>
> err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
> if (err)
> @@ -1594,8 +1638,25 @@ static void ideapad_check_features(struct ideapad_private *priv)
> if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val))
> priv->features.fn_lock = true;
>
> + /*
> + * IdeaPads with HALS_KBD_BL_SUPPORT_BIT have full keyboard
> + * light support, and they send an event via ACPI on light
> + * state change. Whereas some IdeaPads, at least 520-15ikb
> + * and 5 (15), don't send an event, yet they still have
> + * KBLO object. In this case, set kbd_bl_partial to true
> + * and cache the LFCM mutex, it might be useful while
> + * getting the brightness.
> + */
> +
> if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val))
> priv->features.kbd_bl = true;
> + else if (ACPI_SUCCESS(acpi_get_handle(handle, "^KBLO",
> + &priv->kbd_bl.kblo_obj))) {
As mentioned above I would change this to:
else if (ACPI_SUCCESS(acpi_get_handle(handle, "^KBLO",
&priv->kbd_bl.kblo_obj)) &&
ACPI_SUCCESS(acpi_get_handle(handle, "^LFCM",
&priv->kbd_bl.lfcm_mutex))) {
> + priv->features.kbd_bl = true;
> + priv->features.kbd_bl_partial = true;
> + (void)acpi_get_handle(handle, "^LFCM",
> + &priv->kbd_bl.lfcm_mutex);
And then drop the acpi_get_handle() here.
> + }
>
> if (test_bit(HALS_USB_CHARGING_SUPPORT_BIT, &val))
> priv->features.usb_charging = true;
Regards,
Hans
@@ -149,6 +149,7 @@ struct ideapad_private {
bool fn_lock : 1;
bool hw_rfkill_switch : 1;
bool kbd_bl : 1;
+ bool kbd_bl_partial : 1;
bool cam_ctrl_via_ec : 1;
bool touchpad_ctrl_via_ec : 1;
bool usb_charging : 1;
@@ -157,6 +158,9 @@ struct ideapad_private {
bool initialized;
struct led_classdev led;
unsigned int last_brightness;
+ /* Below are used only if kbd_bl_partial is set */
+ acpi_handle lfcm_mutex;
+ acpi_handle kblo_obj;
} kbd_bl;
};
@@ -1300,19 +1304,52 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY);
}
+#define IDEAPAD_ACPI_MUTEX_TIMEOUT 1500
+
/*
* keyboard backlight
*/
static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
{
- unsigned long hals;
+ unsigned long ret_val;
int err;
+ acpi_status status;
- err = eval_hals(priv->adev->handle, &hals);
+ /*
+ * Some IdeaPads with partially implemented keyboard lights don't give
+ * us the light state on HALS_KBD_BL_STATE_BIT in the return value of HALS,
+ * i.e. 5 (15) and 330-17ich. Fortunately we know how to gather it.
+ * Even if it won't work, we will still give HALS a try, because
+ * some IdeaPads with kbd_bl_partial, i.e. 520-15ikb,
+ * correctly sets HALS_KBD_BL_STATE_BIT in HALS return value.
+ */
+
+ if (priv->features.kbd_bl_partial &&
+ priv->kbd_bl.lfcm_mutex != NULL && priv->kbd_bl.kblo_obj != NULL) {
+
+ status = acpi_acquire_mutex(priv->kbd_bl.lfcm_mutex, NULL,
+ IDEAPAD_ACPI_MUTEX_TIMEOUT);
+
+ if (ACPI_SUCCESS(status)) {
+ err = eval_int(priv->kbd_bl.kblo_obj, NULL, &ret_val);
+
+ status = acpi_release_mutex(priv->kbd_bl.lfcm_mutex, NULL);
+ if (ACPI_FAILURE(status))
+ dev_err(&priv->platform_device->dev,
+ "Failed to release LFCM mutex");
+
+ if (err)
+ return err;
+
+ return !!ret_val;
+ }
+ }
+
+ err = eval_hals(priv->adev->handle, &ret_val);
if (err)
return err;
- return !!test_bit(HALS_KBD_BL_STATE_BIT, &hals);
+ return !!test_bit(HALS_KBD_BL_STATE_BIT, &ret_val);
}
static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
@@ -1329,7 +1366,8 @@ static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned
if (err)
return err;
- priv->kbd_bl.last_brightness = brightness;
+ if (!priv->features.kbd_bl_partial)
+ priv->kbd_bl.last_brightness = brightness;
return 0;
}
@@ -1349,6 +1387,9 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
if (!priv->kbd_bl.initialized)
return;
+ if (priv->features.kbd_bl_partial)
+ return;
+
brightness = ideapad_kbd_bl_brightness_get(priv);
if (brightness < 0)
return;
@@ -1371,17 +1412,20 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
if (WARN_ON(priv->kbd_bl.initialized))
return -EEXIST;
- brightness = ideapad_kbd_bl_brightness_get(priv);
- if (brightness < 0)
- return brightness;
+ /* IdeaPads with kbd_bl_partial don't have keyboard backlight event */
+ if (!priv->features.kbd_bl_partial) {
+ brightness = ideapad_kbd_bl_brightness_get(priv);
+ if (brightness < 0)
+ return brightness;
- priv->kbd_bl.last_brightness = brightness;
+ priv->kbd_bl.last_brightness = brightness;
+ priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED;
+ }
priv->kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
priv->kbd_bl.led.max_brightness = 1;
priv->kbd_bl.led.brightness_get = ideapad_kbd_bl_led_cdev_brightness_get;
priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
- priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED;
err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
if (err)
@@ -1594,8 +1638,25 @@ static void ideapad_check_features(struct ideapad_private *priv)
if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val))
priv->features.fn_lock = true;
+ /*
+ * IdeaPads with HALS_KBD_BL_SUPPORT_BIT have full keyboard
+ * light support, and they send an event via ACPI on light
+ * state change. Whereas some IdeaPads, at least 520-15ikb
+ * and 5 (15), don't send an event, yet they still have
+ * KBLO object. In this case, set kbd_bl_partial to true
+ * and cache the LFCM mutex, it might be useful while
+ * getting the brightness.
+ */
+
if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val))
priv->features.kbd_bl = true;
+ else if (ACPI_SUCCESS(acpi_get_handle(handle, "^KBLO",
+ &priv->kbd_bl.kblo_obj))) {
+ priv->features.kbd_bl = true;
+ priv->features.kbd_bl_partial = true;
+ (void)acpi_get_handle(handle, "^LFCM",
+ &priv->kbd_bl.lfcm_mutex);
+ }
if (test_bit(HALS_USB_CHARGING_SUPPORT_BIT, &val))
priv->features.usb_charging = true;