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@xxxxxxxxx>
> ---
> 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
