| Message ID | 20210113182016.166049-22-pobrn@protonmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control | expand |
On Wed, Jan 13, 2021 at 8:25 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > On certain models it is possible to control/query the keyboard backlight > via the SALS/HALS ACPI methods. Add support for that, and register an LED > class device to expose this functionality. > Tested on: Lenovo YOGA 520-14IKB 80X8 ... > + struct { > + bool initialized; > + struct led_classdev led; > + unsigned int last_brightness; > + } kbd_bl; Data structure without sense. ... > +static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv) > +{ > + unsigned long hals; > + int err; > + > + err = eval_hals(priv->adev->handle, &hals); > + if (err) > + return err; > + > + return test_bit(HALS_KBD_BL_STATE_BIT, &hals); On some architectures IIRC it returns long (or unsigned long). Is it a problem? > +} ... > +static int ideapad_kbd_bl_init(struct ideapad_private *priv) > +{ > + int err; > + > + if (!priv->features.kbd_bl) > + return -ENODEV; > + > + err = ideapad_kbd_bl_brightness_get(priv); > + if (err < 0) Just to be sure, what positive code means here? > + return err; > + > + priv->kbd_bl.last_brightness = err; > + > + 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) > + return err; > + > + priv->kbd_bl.initialized = true; > + > + return 0; > +} ... > if (acpi_has_method(handle, "HALS") && acpi_has_method(handle, "SALS")) { > - if (!eval_hals(handle, &val)) > + if (!eval_hals(handle, &val)) { > if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val)) > priv->features.fn_lock = true; > + > + if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val)) > + priv->features.kbd_bl = true; > + } Okay, now I understand which you had separated conditionals (you may ignore that comment).
Hi Thanks for the review. 2021. január 16., szombat 21:07 keltezéssel, Andy Shevchenko írta: > On Wed, Jan 13, 2021 at 8:25 PM Barnabás Pőcze wrote: > > > > On certain models it is possible to control/query the keyboard backlight > > via the SALS/HALS ACPI methods. Add support for that, and register an LED > > class device to expose this functionality. > > Tested on: Lenovo YOGA 520-14IKB 80X8 > > ... > > > + struct { > > + bool initialized; > > + struct led_classdev led; > > + unsigned int last_brightness; > > + } kbd_bl; > > Data structure without sense. > It makes a lot of sense to me, it groups the members which are concerned with keyboard backlight control. Do you have any specific issues with it? And what would you suggest instead? > ... > > > +static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv) > > +{ > > + unsigned long hals; > > + int err; > > + > > + err = eval_hals(priv->adev->handle, &hals); > > + if (err) > > + return err; > > + > > + return test_bit(HALS_KBD_BL_STATE_BIT, &hals); > > On some architectures IIRC it returns long (or unsigned long). Is it a problem? > Potentially it is, but since it's an x86 platform driver, I failed to consider other architectures. Nonetheless, I will fix this in the next version. > > +} > > ... > > > +static int ideapad_kbd_bl_init(struct ideapad_private *priv) > > +{ > > + int err; > > + > > + if (!priv->features.kbd_bl) > > + return -ENODEV; > > + > > + err = ideapad_kbd_bl_brightness_get(priv); > > > + if (err < 0) > > Just to be sure, what positive code means here? > Non-zero return value is the brightness, negative return value is an error code. I realize the name `err` is arguably not be the best, but if I were to rename it to `brightness`, then the `led_classdev_register()` call would look odd in my opinion, and I wanted to avoid introducing two variables. But I think I'll do just that in the next version. > > + return err; > > + > > + priv->kbd_bl.last_brightness = err; > > + > > + 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) > > + return err; > > + > > + priv->kbd_bl.initialized = true; > > + > > + return 0; > > +} > [...] Thanks, Barnabás Pőcze
On Sun, Jan 17, 2021 at 12:44 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > 2021. január 16., szombat 21:07 keltezéssel, Andy Shevchenko írta: > > On Wed, Jan 13, 2021 at 8:25 PM Barnabás Pőcze wrote: ... > > > + struct { > > > + bool initialized; > > > + struct led_classdev led; > > > + unsigned int last_brightness; > > > + } kbd_bl; > > > > Data structure without sense. > It makes a lot of sense to me, it groups the members which are concerned with > keyboard backlight control. Do you have any specific issues with it? And > what would you suggest instead? Oops, I somehow misread it as similar to one with boolean bit fields. Sorry, please, disregard my comment.
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 91e6176cdfbd..6192bf42a87d 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -624,6 +624,8 @@ config IDEAPAD_LAPTOP depends on BACKLIGHT_CLASS_DEVICE depends on ACPI_VIDEO || ACPI_VIDEO = n depends on ACPI_WMI || ACPI_WMI = n + select NEW_LEDS + select LEDS_CLASS select INPUT_SPARSEKMAP help This is a driver for Lenovo IdeaPad netbooks contains drivers for diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index e1ab5c2e982f..7a0dbafe5dfc 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -21,6 +21,7 @@ #include <linux/input/sparse-keymap.h> #include <linux/jiffies.h> #include <linux/kernel.h> +#include <linux/leds.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/rfkill.h> @@ -30,6 +31,8 @@ #include <acpi/video.h> +#include <dt-bindings/leds/common.h> + #define IDEAPAD_RFKILL_DEV_NUM (3) #if IS_ENABLED(CONFIG_ACPI_WMI) @@ -57,12 +60,16 @@ enum { }; enum { + HALS_KBD_BL_SUPPORT_BIT = 4, + HALS_KBD_BL_STATE_BIT = 5, HALS_FNLOCK_SUPPORT_BIT = 9, HALS_FNLOCK_STATE_BIT = 10, HALS_HOTKEYS_PRIMARY_BIT = 11, }; enum { + SALS_KBD_BL_ON = 0x8, + SALS_KBD_BL_OFF = 0x9, SALS_FNLOCK_ON = 0xe, SALS_FNLOCK_OFF = 0xf, }; @@ -114,8 +121,14 @@ struct ideapad_private { fan_mode : 1, touchpad_ctrl_via_ec : 1, conservation_mode : 1, - fn_lock : 1; + fn_lock : 1, + kbd_bl : 1; } features; + struct { + bool initialized; + struct led_classdev led; + unsigned int last_brightness; + } kbd_bl; }; static bool no_bt_rfkill; @@ -937,6 +950,105 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv) backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY); } +/* + * keyboard backlight + */ +static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv) +{ + unsigned long hals; + int err; + + err = eval_hals(priv->adev->handle, &hals); + if (err) + return err; + + return test_bit(HALS_KBD_BL_STATE_BIT, &hals); +} + +static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev) +{ + struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led); + + return ideapad_kbd_bl_brightness_get(priv); +} + +static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness) +{ + int err; + + err = eval_sals(priv->adev->handle, + brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF); + if (err) + return err; + + priv->kbd_bl.last_brightness = brightness; + + return 0; +} + +static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led); + + return ideapad_kbd_bl_brightness_set(priv, brightness); +} + +static void ideapad_kbd_bl_notify(struct ideapad_private *priv) +{ + int brightness; + + if (!priv->kbd_bl.initialized) + return; + + brightness = ideapad_kbd_bl_brightness_get(priv); + if (brightness < 0) + return; + + if (brightness == priv->kbd_bl.last_brightness) + return; + + priv->kbd_bl.last_brightness = brightness; + + led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness); +} + +static int ideapad_kbd_bl_init(struct ideapad_private *priv) +{ + int err; + + if (!priv->features.kbd_bl) + return -ENODEV; + + err = ideapad_kbd_bl_brightness_get(priv); + if (err < 0) + return err; + + priv->kbd_bl.last_brightness = err; + + 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) + return err; + + priv->kbd_bl.initialized = true; + + return 0; +} + +static void ideapad_kbd_bl_exit(struct ideapad_private *priv) +{ + if (!priv->kbd_bl.initialized) + return; + + led_classdev_unregister(&priv->kbd_bl.led); +} + /* * module init/exit */ @@ -1006,8 +1118,9 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data) * Some IdeaPads report event 1 every ~20 * seconds while on battery power; some * report this when changing to/from tablet - * mode. Squelch this event. + * mode. */ + ideapad_kbd_bl_notify(priv); break; default: dev_warn(&priv->platform_device->dev, @@ -1068,9 +1181,13 @@ static void ideapad_check_features(struct ideapad_private *priv) priv->features.conservation_mode = true; if (acpi_has_method(handle, "HALS") && acpi_has_method(handle, "SALS")) { - if (!eval_hals(handle, &val)) + if (!eval_hals(handle, &val)) { if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val)) priv->features.fn_lock = true; + + if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val)) + priv->features.kbd_bl = true; + } } } @@ -1110,6 +1227,11 @@ static int ideapad_acpi_add(struct platform_device *pdev) if (err) goto input_failed; + err = ideapad_kbd_bl_init(priv); + if (err && err != -ENODEV) + dev_warn(&pdev->dev, + "Could not register keyboard backlight led: %d\n", err); + /* * 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. @@ -1174,6 +1296,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_kbd_bl_exit(priv); ideapad_input_exit(priv); input_failed: @@ -1201,6 +1324,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev) for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++) ideapad_unregister_rfkill(priv, i); + ideapad_kbd_bl_exit(priv); ideapad_input_exit(priv); ideapad_debugfs_exit(priv); ideapad_sysfs_exit(priv);
On certain models it is possible to control/query the keyboard backlight via the SALS/HALS ACPI methods. Add support for that, and register an LED class device to expose this functionality. Tested on: Lenovo YOGA 520-14IKB 80X8 Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>