[PATCH v2 21/24] platform/x86: ideapad-laptop: add keyboard backlight control support

[PATCH v2 21/24] platform/x86: ideapad-laptop: add keyboard backlight control support

[Barnabás Pőcze]

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>

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);
-- 
2.30.0

Re: [PATCH v2 21/24] platform/x86: ideapad-laptop: add keyboard backlight control support

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 21/24] platform/x86: ideapad-laptop: add keyboard backlight control support

[Barnabás Pőcze]

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

Re: [PATCH v2 21/24] platform/x86: ideapad-laptop: add keyboard backlight control support

[Andy Shevchenko]

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.


-- 
With Best Regards,
Andy Shevchenko