| Message ID | 20221029120311.11152-7-erayorcunus@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add camera access keys, IdeaPad driver improvements | expand |
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
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) { + + 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;
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(-)