* [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
@ 2023-08-25 12:29 Stuart Hayhurst
2023-08-25 13:01 ` Ilpo Järvinen
0 siblings, 1 reply; 9+ messages in thread
From: Stuart Hayhurst @ 2023-08-25 12:29 UTC (permalink / raw)
Cc: Stuart Hayhurst, platform-driver-x86, Mark Gross, Hans de Goede,
Ike Panhc
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@gmail.com>
---
This has been tested on both new types of keyboard backlight being supported.
KBD_BL_TRISTATE_AUTO is used for keyboards that support automatic brightness.
AUTO is reported as '0' with this patch, as it doesn't fit numerically, I'm not sure how else to
report AUTO, hopefully someone has some insight :)
---
drivers/platform/x86/ideapad-laptop.c | 107 ++++++++++++++++++++++++--
1 file changed, 100 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index d2fee9a3e239..0e4cdd471a4a 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -85,6 +85,21 @@ 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_COMMAND_GET 0x2
+#define KBD_BL_COMMAND_SET 0x3
+
struct ideapad_dytc_priv {
enum platform_profile_option current_profile;
struct platform_profile_handler pprof;
@@ -122,6 +137,7 @@ struct ideapad_private {
} features;
struct {
bool initialized;
+ int type;
struct led_classdev led;
unsigned int last_brightness;
} kbd_bl;
@@ -242,6 +258,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 +1298,42 @@ 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 (priv->kbd_bl.type == KBD_BL_TRISTATE ||
+ priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
+ err = eval_kblc(priv->adev->handle,
+ (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
+ &value);
+
+ if (err)
+ return err;
+
+ /* Convert returned value to brightness level */
+ value = (value & 0xFFFF) >> 1;
+
+ if (value >= 0 && value <= 2) {
+ /* Off, low or high */
+ return value;
+ } else if (value == 3) {
+ /* Auto, report as off */
+ return 0;
+ } else {
+ /* Unknown value */
+ dev_warn(&priv->platform_device->dev,
+ "Unknown keyboard backlight value: %i",
+ 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,13 +1345,27 @@ 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 (type == KBD_BL_TRISTATE ||
+ type == KBD_BL_TRISTATE_AUTO) {
+ if (brightness >= 0 && brightness <= 2) {
+ value = (brightness << 16) | (type << 4) | KBD_BL_COMMAND_SET;
+ } else {
+ return -EINVAL;
+ }
+
+ 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;
priv->kbd_bl.last_brightness = brightness;
-
return 0;
}
@@ -1344,8 +1412,14 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
priv->kbd_bl.last_brightness = brightness;
+ if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
+ priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
+ 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 +1530,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 +1632,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, 0x1, &val)) {
+ if (val == 0x5) {
+ priv->features.kbd_bl = true;
+ priv->kbd_bl.type = KBD_BL_TRISTATE;
+ } else if (val == 0x7) {
+ priv->features.kbd_bl = true;
+ priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
+ } else {
+ dev_warn(&priv->platform_device->dev,
+ "Unknown keyboard type: %i",
+ val);
+ }
+ }
+ }
}
#if IS_ENABLED(CONFIG_ACPI_WMI)
--
2.40.1.521.gf1e218fcd8
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-25 12:29 [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol Stuart Hayhurst
@ 2023-08-25 13:01 ` Ilpo Järvinen
2023-08-25 16:42 ` Stuart
0 siblings, 1 reply; 9+ messages in thread
From: Ilpo Järvinen @ 2023-08-25 13:01 UTC (permalink / raw)
To: Stuart Hayhurst; +Cc: platform-driver-x86, Mark Gross, Hans de Goede, Ike Panhc
On Fri, 25 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@gmail.com>
> ---
>
> This has been tested on both new types of keyboard backlight being supported.
> KBD_BL_TRISTATE_AUTO is used for keyboards that support automatic brightness.
> AUTO is reported as '0' with this patch, as it doesn't fit numerically, I'm not sure how else to
> report AUTO, hopefully someone has some insight :)
>
> ---
> drivers/platform/x86/ideapad-laptop.c | 107 ++++++++++++++++++++++++--
> 1 file changed, 100 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index d2fee9a3e239..0e4cdd471a4a 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -85,6 +85,21 @@ 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_COMMAND_GET 0x2
> +#define KBD_BL_COMMAND_SET 0x3
> +
> struct ideapad_dytc_priv {
> enum platform_profile_option current_profile;
> struct platform_profile_handler pprof;
> @@ -122,6 +137,7 @@ struct ideapad_private {
> } features;
> struct {
> bool initialized;
> + int type;
> struct led_classdev led;
> unsigned int last_brightness;
> } kbd_bl;
> @@ -242,6 +258,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 +1298,42 @@ 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 (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
> + err = eval_kblc(priv->adev->handle,
> + (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
> + &value);
> +
> + if (err)
> + return err;
> +
> + /* Convert returned value to brightness level */
> + value = (value & 0xFFFF) >> 1;
You should define a field for thiswith GENMASK() and use FIELD_GET()
instead of manual masking and shifting.
> +
> + if (value >= 0 && value <= 2) {
How can unsigned long be < 0??
If that 2 is the same as priv->kbd_bl.led.max_brightness, it would make
sense to use it rather than literal.
> + /* Off, low or high */
> + return value;
> + } else if (value == 3) {
> + /* Auto, report as off */
> + return 0;
> + } else {
Since those blocks above return, the elses are unnecessary.
> + /* Unknown value */
> + dev_warn(&priv->platform_device->dev,
> + "Unknown keyboard backlight value: %i",
> + value);
> + return -EINVAL;
> + }
> + }
> +
> +
Remove one of the newlines.
> + 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,13 +1345,27 @@ 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 (type == KBD_BL_TRISTATE ||
> + type == KBD_BL_TRISTATE_AUTO) {
> + if (brightness >= 0 && brightness <= 2) {
Brightness is unsigned int so no need for < 0 check.
Reverse the logic here:
if (brightness > 2)
return -EINVAL;
...as it avoid the need to use else.
Consider using .max_brightness here too.
> + value = (brightness << 16) | (type << 4) | KBD_BL_COMMAND_SET;
There would also be a readability benefit for these if you define these
as fields and use FIELD_PREP().
> + } else {
> + return -EINVAL;
> + }
> +
> + 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;
>
> priv->kbd_bl.last_brightness = brightness;
> -
Stray change.
> return 0;
> }
>
> @@ -1344,8 +1412,14 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>
> priv->kbd_bl.last_brightness = brightness;
>
> + if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
Please add a helper for this check as it seems to appear multiple times in
this patch.
> + 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 +1530,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> case 2:
> ideapad_backlight_notify_power(priv);
> break;
> + case 12:
Could these bits too be named with defines, it would be helpful for those
reading the code?
(If you can add the names to all these other bits too, it should be put
into own patch and not into this one.)
> case 1:
> /*
> * Some IdeaPads report event 1 every ~20
> @@ -1557,13 +1632,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, 0x1, &val)) {
> + if (val == 0x5) {
> + priv->features.kbd_bl = true;
> + priv->kbd_bl.type = KBD_BL_TRISTATE;
> + } else if (val == 0x7) {
Name these three literals with defines.
> + priv->features.kbd_bl = true;
> + priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
> + } else {
> + dev_warn(&priv->platform_device->dev,
> + "Unknown keyboard type: %i",
> + val);
> + }
> + }
> + }
> }
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
>
--
i.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-25 13:01 ` Ilpo Järvinen
@ 2023-08-25 16:42 ` Stuart
2023-08-26 10:07 ` Hans de Goede
2023-08-27 13:22 ` [PATCH 1/1] " Ilpo Järvinen
0 siblings, 2 replies; 9+ messages in thread
From: Stuart @ 2023-08-25 16:42 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: platform-driver-x86, Mark Gross, Hans de Goede, Ike Panhc
Thanks for the quick review, I've addressed most of the comments for
V2, do you want that submitted in this thread, or should I create a
new one?
> Could these bits too be named with defines, it would be helpful for those
> reading the code?
> (If you can add the names to all these other bits too, it should be put
> into own patch and not into this one.)
Sorry, I have no idea about the other events. I can do this one if
you'd like, or leave a comment for the future if you'd rather they all
be done together.
On Fri, Aug 25, 2023 at 2:01 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 25 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@gmail.com>
> > ---
> >
> > This has been tested on both new types of keyboard backlight being supported.
> > KBD_BL_TRISTATE_AUTO is used for keyboards that support automatic brightness.
> > AUTO is reported as '0' with this patch, as it doesn't fit numerically, I'm not sure how else to
> > report AUTO, hopefully someone has some insight :)
> >
> > ---
> > drivers/platform/x86/ideapad-laptop.c | 107 ++++++++++++++++++++++++--
> > 1 file changed, 100 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> > index d2fee9a3e239..0e4cdd471a4a 100644
> > --- a/drivers/platform/x86/ideapad-laptop.c
> > +++ b/drivers/platform/x86/ideapad-laptop.c
> > @@ -85,6 +85,21 @@ 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_COMMAND_GET 0x2
> > +#define KBD_BL_COMMAND_SET 0x3
> > +
> > struct ideapad_dytc_priv {
> > enum platform_profile_option current_profile;
> > struct platform_profile_handler pprof;
> > @@ -122,6 +137,7 @@ struct ideapad_private {
> > } features;
> > struct {
> > bool initialized;
> > + int type;
> > struct led_classdev led;
> > unsigned int last_brightness;
> > } kbd_bl;
> > @@ -242,6 +258,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 +1298,42 @@ 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 (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> > + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
> > + err = eval_kblc(priv->adev->handle,
> > + (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
> > + &value);
> > +
> > + if (err)
> > + return err;
> > +
> > + /* Convert returned value to brightness level */
> > + value = (value & 0xFFFF) >> 1;
>
> You should define a field for thiswith GENMASK() and use FIELD_GET()
> instead of manual masking and shifting.
>
> > +
> > + if (value >= 0 && value <= 2) {
>
> How can unsigned long be < 0??
>
> If that 2 is the same as priv->kbd_bl.led.max_brightness, it would make
> sense to use it rather than literal.
>
> > + /* Off, low or high */
> > + return value;
> > + } else if (value == 3) {
> > + /* Auto, report as off */
> > + return 0;
> > + } else {
>
> Since those blocks above return, the elses are unnecessary.
>
> > + /* Unknown value */
> > + dev_warn(&priv->platform_device->dev,
> > + "Unknown keyboard backlight value: %i",
> > + value);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > +
>
> Remove one of the newlines.
>
> > + 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,13 +1345,27 @@ 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 (type == KBD_BL_TRISTATE ||
> > + type == KBD_BL_TRISTATE_AUTO) {
> > + if (brightness >= 0 && brightness <= 2) {
>
> Brightness is unsigned int so no need for < 0 check.
>
> Reverse the logic here:
>
> if (brightness > 2)
> return -EINVAL;
>
> ...as it avoid the need to use else.
>
> Consider using .max_brightness here too.
>
> > + value = (brightness << 16) | (type << 4) | KBD_BL_COMMAND_SET;
>
> There would also be a readability benefit for these if you define these
> as fields and use FIELD_PREP().
>
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + 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;
> >
> > priv->kbd_bl.last_brightness = brightness;
> > -
>
> Stray change.
>
> > return 0;
> > }
> >
> > @@ -1344,8 +1412,14 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> >
> > priv->kbd_bl.last_brightness = brightness;
> >
> > + if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> > + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
>
> Please add a helper for this check as it seems to appear multiple times in
> this patch.
>
> > + 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 +1530,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> > case 2:
> > ideapad_backlight_notify_power(priv);
> > break;
> > + case 12:
>
> Could these bits too be named with defines, it would be helpful for those
> reading the code?
>
> (If you can add the names to all these other bits too, it should be put
> into own patch and not into this one.)
>
> > case 1:
> > /*
> > * Some IdeaPads report event 1 every ~20
> > @@ -1557,13 +1632,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, 0x1, &val)) {
> > + if (val == 0x5) {
> > + priv->features.kbd_bl = true;
> > + priv->kbd_bl.type = KBD_BL_TRISTATE;
> > + } else if (val == 0x7) {
>
> Name these three literals with defines.
>
> > + priv->features.kbd_bl = true;
> > + priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
> > + } else {
> > + dev_warn(&priv->platform_device->dev,
> > + "Unknown keyboard type: %i",
> > + val);
> > + }
> > + }
> > + }
> > }
> >
> > #if IS_ENABLED(CONFIG_ACPI_WMI)
> >
>
> --
> i.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-25 16:42 ` Stuart
@ 2023-08-26 10:07 ` Hans de Goede
2023-08-26 10:35 ` [PATCH v2] " Stuart Hayhurst
2023-08-27 13:22 ` [PATCH 1/1] " Ilpo Järvinen
1 sibling, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2023-08-26 10:07 UTC (permalink / raw)
To: Stuart, Ilpo Järvinen; +Cc: platform-driver-x86, Mark Gross, Ike Panhc
Hi Stuart,
On 8/25/23 18:42, Stuart wrote:
> Thanks for the quick review, I've addressed most of the comments for
> V2, do you want that submitted in this thread, or should I create a
> new one?
Either way is fine, just pick which way you prefer.
Regards,
Hans
>> Could these bits too be named with defines, it would be helpful for those
>> reading the code?
>
>> (If you can add the names to all these other bits too, it should be put
>> into own patch and not into this one.)
>
> Sorry, I have no idea about the other events. I can do this one if
> you'd like, or leave a comment for the future if you'd rather they all
> be done together.
>
> On Fri, Aug 25, 2023 at 2:01 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
>>
>> On Fri, 25 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@gmail.com>
>>> ---
>>>
>>> This has been tested on both new types of keyboard backlight being supported.
>>> KBD_BL_TRISTATE_AUTO is used for keyboards that support automatic brightness.
>>> AUTO is reported as '0' with this patch, as it doesn't fit numerically, I'm not sure how else to
>>> report AUTO, hopefully someone has some insight :)
>>>
>>> ---
>>> drivers/platform/x86/ideapad-laptop.c | 107 ++++++++++++++++++++++++--
>>> 1 file changed, 100 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>> index d2fee9a3e239..0e4cdd471a4a 100644
>>> --- a/drivers/platform/x86/ideapad-laptop.c
>>> +++ b/drivers/platform/x86/ideapad-laptop.c
>>> @@ -85,6 +85,21 @@ 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_COMMAND_GET 0x2
>>> +#define KBD_BL_COMMAND_SET 0x3
>>> +
>>> struct ideapad_dytc_priv {
>>> enum platform_profile_option current_profile;
>>> struct platform_profile_handler pprof;
>>> @@ -122,6 +137,7 @@ struct ideapad_private {
>>> } features;
>>> struct {
>>> bool initialized;
>>> + int type;
>>> struct led_classdev led;
>>> unsigned int last_brightness;
>>> } kbd_bl;
>>> @@ -242,6 +258,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 +1298,42 @@ 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 (priv->kbd_bl.type == KBD_BL_TRISTATE ||
>>> + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
>>> + err = eval_kblc(priv->adev->handle,
>>> + (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
>>> + &value);
>>> +
>>> + if (err)
>>> + return err;
>>> +
>>> + /* Convert returned value to brightness level */
>>> + value = (value & 0xFFFF) >> 1;
>>
>> You should define a field for thiswith GENMASK() and use FIELD_GET()
>> instead of manual masking and shifting.
>>
>>> +
>>> + if (value >= 0 && value <= 2) {
>>
>> How can unsigned long be < 0??
>>
>> If that 2 is the same as priv->kbd_bl.led.max_brightness, it would make
>> sense to use it rather than literal.
>>
>>> + /* Off, low or high */
>>> + return value;
>>> + } else if (value == 3) {
>>> + /* Auto, report as off */
>>> + return 0;
>>> + } else {
>>
>> Since those blocks above return, the elses are unnecessary.
>>
>>> + /* Unknown value */
>>> + dev_warn(&priv->platform_device->dev,
>>> + "Unknown keyboard backlight value: %i",
>>> + value);
>>> + return -EINVAL;
>>> + }
>>> + }
>>> +
>>> +
>>
>> Remove one of the newlines.
>>
>>> + 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,13 +1345,27 @@ 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 (type == KBD_BL_TRISTATE ||
>>> + type == KBD_BL_TRISTATE_AUTO) {
>>> + if (brightness >= 0 && brightness <= 2) {
>>
>> Brightness is unsigned int so no need for < 0 check.
>>
>> Reverse the logic here:
>>
>> if (brightness > 2)
>> return -EINVAL;
>>
>> ...as it avoid the need to use else.
>>
>> Consider using .max_brightness here too.
>>
>>> + value = (brightness << 16) | (type << 4) | KBD_BL_COMMAND_SET;
>>
>> There would also be a readability benefit for these if you define these
>> as fields and use FIELD_PREP().
>>
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + 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;
>>>
>>> priv->kbd_bl.last_brightness = brightness;
>>> -
>>
>> Stray change.
>>
>>> return 0;
>>> }
>>>
>>> @@ -1344,8 +1412,14 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>>>
>>> priv->kbd_bl.last_brightness = brightness;
>>>
>>> + if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
>>> + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
>>
>> Please add a helper for this check as it seems to appear multiple times in
>> this patch.
>>
>>> + 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 +1530,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>> case 2:
>>> ideapad_backlight_notify_power(priv);
>>> break;
>>> + case 12:
>>
>> Could these bits too be named with defines, it would be helpful for those
>> reading the code?
>>
>> (If you can add the names to all these other bits too, it should be put
>> into own patch and not into this one.)
>>
>>> case 1:
>>> /*
>>> * Some IdeaPads report event 1 every ~20
>>> @@ -1557,13 +1632,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, 0x1, &val)) {
>>> + if (val == 0x5) {
>>> + priv->features.kbd_bl = true;
>>> + priv->kbd_bl.type = KBD_BL_TRISTATE;
>>> + } else if (val == 0x7) {
>>
>> Name these three literals with defines.
>>
>>> + priv->features.kbd_bl = true;
>>> + priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
>>> + } else {
>>> + dev_warn(&priv->platform_device->dev,
>>> + "Unknown keyboard type: %i",
>>> + val);
>>> + }
>>> + }
>>> + }
>>> }
>>>
>>> #if IS_ENABLED(CONFIG_ACPI_WMI)
>>>
>>
>> --
>> i.
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-26 10:07 ` Hans de Goede
@ 2023-08-26 10:35 ` Stuart Hayhurst
2023-08-27 13:31 ` Ilpo Järvinen
0 siblings, 1 reply; 9+ messages in thread
From: Stuart Hayhurst @ 2023-08-26 10:35 UTC (permalink / raw)
To: linux-kernel
Cc: Stuart Hayhurst, platform-driver-x86, Mark Gross, Hans de Goede,
Ike Panhc, ilpo.jarvinen
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@gmail.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
---
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)
+
+#define CHECK_KBD_BL_TRISTATE(TYPE) (TYPE == KBD_BL_TRISTATE || \
+ TYPE == KBD_BL_TRISTATE_AUTO)
+
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,
+ &value);
+
+ if (err)
+ return err;
+
+ /* Convert returned value to brightness level */
+ value = FIELD_GET(KBD_BL_GET_BRIGHTNESS_MASK, value) >> 1;
+
+ /* 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)
--
2.40.1.521.gf1e218fcd8
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-25 16:42 ` Stuart
2023-08-26 10:07 ` Hans de Goede
@ 2023-08-27 13:22 ` Ilpo Järvinen
1 sibling, 0 replies; 9+ messages in thread
From: Ilpo Järvinen @ 2023-08-27 13:22 UTC (permalink / raw)
To: Stuart; +Cc: platform-driver-x86, Mark Gross, Hans de Goede, Ike Panhc
[-- Attachment #1: Type: text/plain, Size: 10701 bytes --]
On Fri, 25 Aug 2023, Stuart wrote:
> Thanks for the quick review, I've addressed most of the comments for
> V2, do you want that submitted in this thread, or should I create a
> new one?
>
> > Could these bits too be named with defines, it would be helpful for those
> > reading the code?
>
> > (If you can add the names to all these other bits too, it should be put
> > into own patch and not into this one.)
>
> Sorry, I have no idea about the other events. I can do this one if
> you'd like, or leave a comment for the future if you'd rather they all
> be done together.
No worries, I assumed you might not know about them.
Lets just have the named define for this and leave the others as they are
(hopefully one day we can get them named as well).
--
i.
> On Fri, Aug 25, 2023 at 2:01 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Fri, 25 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@gmail.com>
> > > ---
> > >
> > > This has been tested on both new types of keyboard backlight being supported.
> > > KBD_BL_TRISTATE_AUTO is used for keyboards that support automatic brightness.
> > > AUTO is reported as '0' with this patch, as it doesn't fit numerically, I'm not sure how else to
> > > report AUTO, hopefully someone has some insight :)
> > >
> > > ---
> > > drivers/platform/x86/ideapad-laptop.c | 107 ++++++++++++++++++++++++--
> > > 1 file changed, 100 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> > > index d2fee9a3e239..0e4cdd471a4a 100644
> > > --- a/drivers/platform/x86/ideapad-laptop.c
> > > +++ b/drivers/platform/x86/ideapad-laptop.c
> > > @@ -85,6 +85,21 @@ 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_COMMAND_GET 0x2
> > > +#define KBD_BL_COMMAND_SET 0x3
> > > +
> > > struct ideapad_dytc_priv {
> > > enum platform_profile_option current_profile;
> > > struct platform_profile_handler pprof;
> > > @@ -122,6 +137,7 @@ struct ideapad_private {
> > > } features;
> > > struct {
> > > bool initialized;
> > > + int type;
> > > struct led_classdev led;
> > > unsigned int last_brightness;
> > > } kbd_bl;
> > > @@ -242,6 +258,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 +1298,42 @@ 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 (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> > > + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
> > > + err = eval_kblc(priv->adev->handle,
> > > + (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
> > > + &value);
> > > +
> > > + if (err)
> > > + return err;
> > > +
> > > + /* Convert returned value to brightness level */
> > > + value = (value & 0xFFFF) >> 1;
> >
> > You should define a field for thiswith GENMASK() and use FIELD_GET()
> > instead of manual masking and shifting.
> >
> > > +
> > > + if (value >= 0 && value <= 2) {
> >
> > How can unsigned long be < 0??
> >
> > If that 2 is the same as priv->kbd_bl.led.max_brightness, it would make
> > sense to use it rather than literal.
> >
> > > + /* Off, low or high */
> > > + return value;
> > > + } else if (value == 3) {
> > > + /* Auto, report as off */
> > > + return 0;
> > > + } else {
> >
> > Since those blocks above return, the elses are unnecessary.
> >
> > > + /* Unknown value */
> > > + dev_warn(&priv->platform_device->dev,
> > > + "Unknown keyboard backlight value: %i",
> > > + value);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > +
> >
> > Remove one of the newlines.
> >
> > > + 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,13 +1345,27 @@ 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 (type == KBD_BL_TRISTATE ||
> > > + type == KBD_BL_TRISTATE_AUTO) {
> > > + if (brightness >= 0 && brightness <= 2) {
> >
> > Brightness is unsigned int so no need for < 0 check.
> >
> > Reverse the logic here:
> >
> > if (brightness > 2)
> > return -EINVAL;
> >
> > ...as it avoid the need to use else.
> >
> > Consider using .max_brightness here too.
> >
> > > + value = (brightness << 16) | (type << 4) | KBD_BL_COMMAND_SET;
> >
> > There would also be a readability benefit for these if you define these
> > as fields and use FIELD_PREP().
> >
> > > + } else {
> > > + return -EINVAL;
> > > + }
> > > +
> > > + 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;
> > >
> > > priv->kbd_bl.last_brightness = brightness;
> > > -
> >
> > Stray change.
> >
> > > return 0;
> > > }
> > >
> > > @@ -1344,8 +1412,14 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> > >
> > > priv->kbd_bl.last_brightness = brightness;
> > >
> > > + if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> > > + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
> >
> > Please add a helper for this check as it seems to appear multiple times in
> > this patch.
> >
> > > + 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 +1530,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> > > case 2:
> > > ideapad_backlight_notify_power(priv);
> > > break;
> > > + case 12:
> >
> > Could these bits too be named with defines, it would be helpful for those
> > reading the code?
> >
> > (If you can add the names to all these other bits too, it should be put
> > into own patch and not into this one.)
> >
> > > case 1:
> > > /*
> > > * Some IdeaPads report event 1 every ~20
> > > @@ -1557,13 +1632,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, 0x1, &val)) {
> > > + if (val == 0x5) {
> > > + priv->features.kbd_bl = true;
> > > + priv->kbd_bl.type = KBD_BL_TRISTATE;
> > > + } else if (val == 0x7) {
> >
> > Name these three literals with defines.
> >
> > > + priv->features.kbd_bl = true;
> > > + priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
> > > + } else {
> > > + dev_warn(&priv->platform_device->dev,
> > > + "Unknown keyboard type: %i",
> > > + val);
> > > + }
> > > + }
> > > + }
> > > }
> > >
> > > #if IS_ENABLED(CONFIG_ACPI_WMI)
> > >
> >
> > --
> > i.
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-26 10:35 ` [PATCH v2] " Stuart Hayhurst
@ 2023-08-27 13:31 ` Ilpo Järvinen
2023-08-27 16:19 ` [PATCH v3] " Stuart Hayhurst
0 siblings, 1 reply; 9+ messages in thread
From: Ilpo Järvinen @ 2023-08-27 13:31 UTC (permalink / raw)
To: Stuart Hayhurst
Cc: LKML, platform-driver-x86, Mark Gross, Hans de Goede, Ike Panhc
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@gmail.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)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-27 13:31 ` Ilpo Järvinen
@ 2023-08-27 16:19 ` Stuart Hayhurst
2023-08-28 8:52 ` Hans de Goede
0 siblings, 1 reply; 9+ messages in thread
From: Stuart Hayhurst @ 2023-08-27 16:19 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: Stuart Hayhurst, LKML, platform-driver-x86, Mark Gross,
Hans de Goede, Ike Panhc
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@gmail.com>
---
Thanks, applied your feedback
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
v2 -> v3:
- Use named define for keyboard brightness event
- Replaced CHECK_KBD_BL_TRISTATE macro with static function
- Dropped _MASK suffix for bit masking
- Use FIELD_PREP for the get command too
- Ignore least significant bit in KBD__BL_GET_BRIGHTNESS mask
---
drivers/platform/x86/ideapad-laptop.c | 118 ++++++++++++++++++++++++--
1 file changed, 112 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index d2fee9a3e239..4a19a396fc3b 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,31 @@ 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_COMMAND_TYPE GENMASK(7, 4)
+
+#define KBD_BL_GET_BRIGHTNESS GENMASK(15, 1)
+#define KBD_BL_SET_BRIGHTNESS GENMASK(19, 16)
+
+#define KBD_BL_KBLC_CHANGED_EVENT 12
+
struct ideapad_dytc_priv {
enum platform_profile_option current_profile;
struct platform_profile_handler pprof;
@@ -122,6 +148,7 @@ struct ideapad_private {
} features;
struct {
bool initialized;
+ int type;
struct led_classdev led;
unsigned int last_brightness;
} kbd_bl;
@@ -242,6 +269,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);
@@ -1270,16 +1307,47 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
/*
* keyboard backlight
*/
+static int ideapad_kbd_bl_check_tristate(int type)
+{
+ return (type == KBD_BL_TRISTATE) || (type == KBD_BL_TRISTATE_AUTO);
+}
+
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 (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type)) {
+ err = eval_kblc(priv->adev->handle,
+ FIELD_PREP(KBD_BL_COMMAND_TYPE, priv->kbd_bl.type) |
+ KBD_BL_COMMAND_GET,
+ &value);
+
+ if (err)
+ return err;
+
+ /* Convert returned value to brightness level */
+ value = FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
+
+ /* 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 +1359,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 (ideapad_kbd_bl_check_tristate(type)) {
+ if (brightness > priv->kbd_bl.led.max_brightness)
+ return -EINVAL;
+
+ value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, brightness) |
+ FIELD_PREP(KBD_BL_COMMAND_TYPE, 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 +1426,13 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
priv->kbd_bl.last_brightness = brightness;
+ if (ideapad_kbd_bl_check_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 +1543,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
case 2:
ideapad_backlight_notify_power(priv);
break;
+ case KBD_BL_KBLC_CHANGED_EVENT:
case 1:
/*
* Some IdeaPads report event 1 every ~20
@@ -1557,13 +1645,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)
--
2.40.1.521.gf1e218fcd8
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol
2023-08-27 16:19 ` [PATCH v3] " Stuart Hayhurst
@ 2023-08-28 8:52 ` Hans de Goede
0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2023-08-28 8:52 UTC (permalink / raw)
To: Stuart Hayhurst, ilpo.jarvinen
Cc: LKML, platform-driver-x86, Mark Gross, Ike Panhc
Hi Stuart,
On 8/27/23 18:19, 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@gmail.com>
Thank you for your patch and thank you Ilpo for the reviews.
This looks good to me now. The merge window for 6.6 opened yesterday,
so the timing of this is a bit unfortunate. Since I know that
various users have actually been asking about this functionality
and since this seems a reasonably isolated patch I've decided
to still merge this now (just in time for 6.6).
I hope I'm not going to regret this :) In the worse case we
can always revert this and try again for 6.7 .
I've applied this patch to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the current
merge-window.
Regards,
Hans
> ---
>
> Thanks, applied your feedback
>
> 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
>
> v2 -> v3:
> - Use named define for keyboard brightness event
> - Replaced CHECK_KBD_BL_TRISTATE macro with static function
> - Dropped _MASK suffix for bit masking
> - Use FIELD_PREP for the get command too
> - Ignore least significant bit in KBD__BL_GET_BRIGHTNESS mask
>
> ---
> drivers/platform/x86/ideapad-laptop.c | 118 ++++++++++++++++++++++++--
> 1 file changed, 112 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index d2fee9a3e239..4a19a396fc3b 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,31 @@ 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_COMMAND_TYPE GENMASK(7, 4)
> +
> +#define KBD_BL_GET_BRIGHTNESS GENMASK(15, 1)
> +#define KBD_BL_SET_BRIGHTNESS GENMASK(19, 16)
> +
> +#define KBD_BL_KBLC_CHANGED_EVENT 12
> +
> struct ideapad_dytc_priv {
> enum platform_profile_option current_profile;
> struct platform_profile_handler pprof;
> @@ -122,6 +148,7 @@ struct ideapad_private {
> } features;
> struct {
> bool initialized;
> + int type;
> struct led_classdev led;
> unsigned int last_brightness;
> } kbd_bl;
> @@ -242,6 +269,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);
> @@ -1270,16 +1307,47 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
> /*
> * keyboard backlight
> */
> +static int ideapad_kbd_bl_check_tristate(int type)
> +{
> + return (type == KBD_BL_TRISTATE) || (type == KBD_BL_TRISTATE_AUTO);
> +}
> +
> 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 (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type)) {
> + err = eval_kblc(priv->adev->handle,
> + FIELD_PREP(KBD_BL_COMMAND_TYPE, priv->kbd_bl.type) |
> + KBD_BL_COMMAND_GET,
> + &value);
> +
> + if (err)
> + return err;
> +
> + /* Convert returned value to brightness level */
> + value = FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
> +
> + /* 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 +1359,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 (ideapad_kbd_bl_check_tristate(type)) {
> + if (brightness > priv->kbd_bl.led.max_brightness)
> + return -EINVAL;
> +
> + value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, brightness) |
> + FIELD_PREP(KBD_BL_COMMAND_TYPE, 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 +1426,13 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>
> priv->kbd_bl.last_brightness = brightness;
>
> + if (ideapad_kbd_bl_check_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 +1543,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> case 2:
> ideapad_backlight_notify_power(priv);
> break;
> + case KBD_BL_KBLC_CHANGED_EVENT:
> case 1:
> /*
> * Some IdeaPads report event 1 every ~20
> @@ -1557,13 +1645,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)
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-08-28 8:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-25 12:29 [PATCH 1/1] platform/x86: ideapad-laptop: Add support for keyboard backlights using KBLC ACPI symbol Stuart Hayhurst
2023-08-25 13:01 ` Ilpo Järvinen
2023-08-25 16:42 ` Stuart
2023-08-26 10:07 ` Hans de Goede
2023-08-26 10:35 ` [PATCH v2] " Stuart Hayhurst
2023-08-27 13:31 ` Ilpo Järvinen
2023-08-27 16:19 ` [PATCH v3] " Stuart Hayhurst
2023-08-28 8:52 ` Hans de Goede
2023-08-27 13:22 ` [PATCH 1/1] " Ilpo Järvinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.