| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4aa2e668-aa50-202f-6ce-3d4cc1209b@linux.intel.com>
Date: Sun, 27 Aug 2023 16:31:43 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Stuart Hayhurst <stuart.a.hayhurst@...il.com>
cc: LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org,
Mark Gross <markgross@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Ike Panhc <ike.pan@...onical.com>
Subject: Re: [PATCH v2] platform/x86: ideapad-laptop: Add support for keyboard
backlights using KBLC ACPI symbol
On Sat, 26 Aug 2023, Stuart Hayhurst wrote:
> Newer Lenovo laptops seem to use the KBLC symbol to control the backlight
> Add support for handling the keyboard backlight on these devices
>
> Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@...il.com>
> ---
>
> v1 -> v2:
> - Replace keyboard identification hex literals with defines
> - Use a helper macro for checking a keyboard type is tristate
> - Reworked ideapad_kbd_bl_brightness_set
> - Reworked ideapad_kbd_bl_brightness_get
> - Clean up newlines and stray change
> - Use GENMASK, FIELD_GET and FIELD_PUT instead of manual masking and shifting
> - Correct format specifiers for new warnings
Thanks, looks better already. A few follow up comments still below.
> ---
> drivers/platform/x86/ideapad-laptop.c | 113 ++++++++++++++++++++++++--
> 1 file changed, 107 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index d2fee9a3e239..6149852bf27f 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -10,6 +10,7 @@
>
> #include <linux/acpi.h>
> #include <linux/backlight.h>
> +#include <linux/bitfield.h>
> #include <linux/bitops.h>
> #include <linux/bug.h>
> #include <linux/debugfs.h>
> @@ -85,6 +86,32 @@ enum {
> SALS_FNLOCK_OFF = 0xf,
> };
>
> +/*
> + * These correspond to the number of supported states - 1
> + * Future keyboard types may need a new system, if there's a collision
> + * KBD_BL_TRISTATE_AUTO has no way to report or set the auto state
> + * so it effectively has 3 states, but needs to handle 4
> + */
> +enum {
> + KBD_BL_STANDARD = 1,
> + KBD_BL_TRISTATE = 2,
> + KBD_BL_TRISTATE_AUTO = 3,
> +};
> +
> +#define KBD_BL_QUERY_TYPE 0x1
> +#define KBD_BL_TRISTATE_TYPE 0x5
> +#define KBD_BL_TRISTATE_AUTO_TYPE 0x7
> +
> +#define KBD_BL_COMMAND_GET 0x2
> +#define KBD_BL_COMMAND_SET 0x3
> +
> +#define KBD_BL_GET_BRIGHTNESS_MASK GENMASK(15, 0)
> +#define KBD_BL_SET_BRIGHTNESS_MASK GENMASK(19, 16)
> +#define KBD_BL_SET_TYPE_MASK GENMASK(7, 4)
You can use _MASK if you want but it usually adds little to no value only
increasing the line numbers.
> +#define CHECK_KBD_BL_TRISTATE(TYPE) (TYPE == KBD_BL_TRISTATE || \
> + TYPE == KBD_BL_TRISTATE_AUTO)
Do a static function instead, there's nothing that requires macro for
this.
> +
> struct ideapad_dytc_priv {
> enum platform_profile_option current_profile;
> struct platform_profile_handler pprof;
> @@ -122,6 +149,7 @@ struct ideapad_private {
> } features;
> struct {
> bool initialized;
> + int type;
> struct led_classdev led;
> unsigned int last_brightness;
> } kbd_bl;
> @@ -242,6 +270,16 @@ static int exec_sals(acpi_handle handle, unsigned long arg)
> return exec_simple_method(handle, "SALS", arg);
> }
>
> +static int exec_kblc(acpi_handle handle, unsigned long arg)
> +{
> + return exec_simple_method(handle, "KBLC", arg);
> +}
> +
> +static int eval_kblc(acpi_handle handle, unsigned long cmd, unsigned long *res)
> +{
> + return eval_int_with_arg(handle, "KBLC", cmd, res);
> +}
> +
> static int eval_dytc(acpi_handle handle, unsigned long cmd, unsigned long *res)
> {
> return eval_int_with_arg(handle, "DYTC", cmd, res);
> @@ -1272,14 +1310,39 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
> */
> static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> {
> - unsigned long hals;
> + unsigned long value;
> int err;
>
> - err = eval_hals(priv->adev->handle, &hals);
> + if (CHECK_KBD_BL_TRISTATE(priv->kbd_bl.type)) {
> + err = eval_kblc(priv->adev->handle,
> + (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
Use FIELD_PREP(KBL_BL_GET_TYPE, priv->kbd_bl.type) here too.
Or ... Do GET and SET happen to share the field structure here in which
case you could name it as KBD_BL_COMMAND_TYPE and not one for set and get?
> + &value);
> +
> + if (err)
> + return err;
> +
> + /* Convert returned value to brightness level */
> + value = FIELD_GET(KBD_BL_GET_BRIGHTNESS_MASK, value) >> 1;
Why is this >> 1 here? Is it that the least-significant bit is not part of
the field? (In which case you want take that into account in the field's
GENMASK()).
--
i.
> +
> + /* Off, low or high */
> + if (value <= priv->kbd_bl.led.max_brightness)
> + return value;
> +
> + /* Auto, report as off */
> + if (value == priv->kbd_bl.led.max_brightness + 1)
> + return 0;
> +
> + /* Unknown value */
> + dev_warn(&priv->platform_device->dev,
> + "Unknown keyboard backlight value: %lu", value);
> + return -EINVAL;
> + }
> +
> + err = eval_hals(priv->adev->handle, &value);
> if (err)
> return err;
>
> - return !!test_bit(HALS_KBD_BL_STATE_BIT, &hals);
> + return !!test_bit(HALS_KBD_BL_STATE_BIT, &value);
> }
>
> static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
> @@ -1291,7 +1354,21 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
>
> static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> {
> - int err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> + int err;
> + unsigned long value;
> + int type = priv->kbd_bl.type;
> +
> + if (CHECK_KBD_BL_TRISTATE(type)) {
> + if (brightness > priv->kbd_bl.led.max_brightness)
> + return -EINVAL;
> +
> + value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS_MASK, brightness) |
> + FIELD_PREP(KBD_BL_SET_TYPE_MASK, type) |
> + KBD_BL_COMMAND_SET;
> + err = exec_kblc(priv->adev->handle, value);
> + } else {
> + err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> + }
>
> if (err)
> return err;
> @@ -1344,8 +1421,13 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>
> priv->kbd_bl.last_brightness = brightness;
>
> + if (CHECK_KBD_BL_TRISTATE(priv->kbd_bl.type)) {
> + priv->kbd_bl.led.max_brightness = 2;
> + } else {
> + priv->kbd_bl.led.max_brightness = 1;
> + }
> +
> 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;
> @@ -1456,6 +1538,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> case 2:
> ideapad_backlight_notify_power(priv);
> break;
> + case 12:
> case 1:
> /*
> * Some IdeaPads report event 1 every ~20
> @@ -1557,13 +1640,31 @@ static void ideapad_check_features(struct ideapad_private *priv)
> if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val))
> priv->features.fn_lock = true;
>
> - if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val))
> + if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val)) {
> priv->features.kbd_bl = true;
> + priv->kbd_bl.type = KBD_BL_STANDARD;
> + }
>
> if (test_bit(HALS_USB_CHARGING_SUPPORT_BIT, &val))
> priv->features.usb_charging = true;
> }
> }
> +
> + if (acpi_has_method(handle, "KBLC")) {
> + if (!eval_kblc(priv->adev->handle, KBD_BL_QUERY_TYPE, &val)) {
> + if (val == KBD_BL_TRISTATE_TYPE) {
> + priv->features.kbd_bl = true;
> + priv->kbd_bl.type = KBD_BL_TRISTATE;
> + } else if (val == KBD_BL_TRISTATE_AUTO_TYPE) {
> + priv->features.kbd_bl = true;
> + priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
> + } else {
> + dev_warn(&priv->platform_device->dev,
> + "Unknown keyboard type: %lu",
> + val);
> + }
> + }
> + }
> }
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
>
Powered by blists - more mailing lists