[PATCH v3 0/1] asus-wmi: add support for ASUS screenpad

[PATCH v3 0/1] asus-wmi: add support for ASUS screenpad

[Luke D. Jones]

Adds support for the screenpad(-plus) found on a few ASUS laptops that have a main 16:9 or 16:10 screen and a shorter screen below the main but above the keyboard.
The support consists of:
- On off control
- Setting brightness from 0-255

There are some small quirks with this device when considering only the raw WMI methods:
1. The Off method can only switch the device off
2. Changing the brightness turns the device back on
3. To turn the device back on the brightness must be > 1
4. When the device is off the brightness can't be changed (so it is stored by the driver if device is off).
5. Booting with a value of 0 brightness (retained by bios) means the bios will set a value of > 0, < 15 which is far too dim and was unexpected by testers. The compromise was to set the brightness to 60 which is a usable brightness if the module init brightness was under 15.
6. When the device is off it is "unplugged"

All of the above points are addressed within the patch to create a good user experience and keep within user expectations.

Changelog:
- V3
  - Refactor error handling in all functions
- V2
  - Complete refactor to use as a backlight device

Luke D. Jones (1):
  platform/x86: asus-wmi: add support for ASUS screenpad

 drivers/platform/x86/asus-wmi.c            | 129 +++++++++++++++++++++
 drivers/platform/x86/asus-wmi.h            |   1 +
 include/linux/platform_data/x86/asus-wmi.h |   4 +
 3 files changed, 134 insertions(+)

-- 
2.40.1

[PATCH v3 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke D. Jones]

Add support for the WMI methods used to turn off and adjust the
brightness of the secondary "screenpad" device found on some high-end
ASUS laptops like the GX650P series and others.

These methods are utilised in a new backlight device named asus_screenpad.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 129 +++++++++++++++++++++
 drivers/platform/x86/asus-wmi.h            |   1 +
 include/linux/platform_data/x86/asus-wmi.h |   4 +
 3 files changed, 134 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 62cee13f5576..c87fbd81d658 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -25,6 +25,7 @@
 #include <linux/input/sparse-keymap.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pci_hotplug.h>
@@ -212,6 +213,7 @@ struct asus_wmi {
 
 	struct input_dev *inputdev;
 	struct backlight_device *backlight_device;
+	struct backlight_device *screenpad_backlight_device;
 	struct platform_device *platform_device;
 
 	struct led_classdev wlan_led;
@@ -3839,6 +3841,124 @@ static int is_display_toggle(int code)
 	return 0;
 }
 
+/* Screenpad backlight *******************************************************/
+
+static int read_screenpad_backlight_power(struct asus_wmi *asus)
+{
+	int ret;
+
+	ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
+	if (ret < 0)
+		return ret;
+	/* 1 == powered */
+	return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
+}
+
+static int read_screenpad_brightness(struct backlight_device *bd)
+{
+	struct asus_wmi *asus = bl_get_data(bd);
+	u32 retval;
+	int err;
+
+	err = read_screenpad_backlight_power(asus);
+	if (err < 0)
+		return err;
+	/* The device brightness can only be read if powered, so return stored */
+	if (err == FB_BLANK_POWERDOWN)
+		return asus->driver->screenpad_brightness;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
+	if (err < 0)
+		return err;
+
+	return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
+}
+
+static int update_screenpad_bl_status(struct backlight_device *bd)
+{
+	struct asus_wmi *asus = bl_get_data(bd);
+	int power, err = 0;
+	u32 ctrl_param;
+
+	power = read_screenpad_backlight_power(asus);
+	if (power < 0)
+		return power;
+
+	if (bd->props.power != power) {
+		if (power != FB_BLANK_UNBLANK) {
+			/* Only brightness > 0 can power it back on */
+			ctrl_param = max(1, asus->driver->screenpad_brightness);
+			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
+						    ctrl_param, NULL);
+		} else {
+			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
+		}
+	} else if (power == FB_BLANK_UNBLANK) {
+		/* Only set brightness if powered on or we get invalid/unsync state */
+		ctrl_param = bd->props.brightness;
+		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
+	}
+
+	/* Ensure brightness is stored to turn back on with */
+	asus->driver->screenpad_brightness = bd->props.brightness;
+
+	return err;
+}
+
+static const struct backlight_ops asus_screenpad_bl_ops = {
+	.get_brightness = read_screenpad_brightness,
+	.update_status = update_screenpad_bl_status,
+	.options = BL_CORE_SUSPENDRESUME,
+};
+
+static int asus_screenpad_init(struct asus_wmi *asus)
+{
+	struct backlight_device *bd;
+	struct backlight_properties props;
+	int power, brightness;
+
+	power = read_screenpad_backlight_power(asus);
+	if (power < 0)
+		return power;
+
+	brightness = read_screenpad_brightness(bd);
+	if (brightness < 0)
+		return brightness;
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */
+	props.max_brightness = 255;
+	bd = backlight_device_register("asus_screenpad",
+				       &asus->platform_device->dev, asus,
+				       &asus_screenpad_bl_ops, &props);
+	if (IS_ERR(bd)) {
+		pr_err("Could not register backlight device\n");
+		return PTR_ERR(bd);
+	}
+
+	asus->screenpad_backlight_device = bd;
+	/*
+	 * Counter an odd behaviour where default is set to < 13 if it was 0 on boot.
+	 * 60 is subjective, but accepted as a good compromise to retain visibility.
+	 */
+	if (brightness < 60)
+		brightness = 60;
+
+	asus->driver->screenpad_brightness = brightness;
+	bd->props.brightness = brightness;
+	bd->props.power = power;
+	backlight_update_status(bd);
+
+	return 0;
+}
+
+static void asus_screenpad_exit(struct asus_wmi *asus)
+{
+	backlight_device_unregister(asus->screenpad_backlight_device);
+
+	asus->screenpad_backlight_device = NULL;
+}
+
 /* Fn-lock ********************************************************************/
 
 static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
@@ -4504,6 +4624,12 @@ static int asus_wmi_add(struct platform_device *pdev)
 	} else if (asus->driver->quirks->wmi_backlight_set_devstate)
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
 
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
+		err = asus_screenpad_init(asus);
+		if (err && err != -ENODEV)
+			goto fail_screenpad;
+	}
+
 	if (asus_wmi_has_fnlock_key(asus)) {
 		asus->fnlock_locked = fnlock_default;
 		asus_wmi_fnlock_update(asus);
@@ -4527,6 +4653,8 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus_wmi_backlight_exit(asus);
 fail_backlight:
 	asus_wmi_rfkill_exit(asus);
+fail_screenpad:
+	asus_screenpad_exit(asus);
 fail_rfkill:
 	asus_wmi_led_exit(asus);
 fail_leds:
@@ -4553,6 +4681,7 @@ static int asus_wmi_remove(struct platform_device *device)
 	asus = platform_get_drvdata(device);
 	wmi_remove_notify_handler(asus->driver->event_guid);
 	asus_wmi_backlight_exit(asus);
+	asus_screenpad_exit(asus);
 	asus_wmi_input_exit(asus);
 	asus_wmi_led_exit(asus);
 	asus_wmi_rfkill_exit(asus);
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index a478ebfd34df..5fbdd0eafa02 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -57,6 +57,7 @@ struct quirk_entry {
 struct asus_wmi_driver {
 	int			brightness;
 	int			panel_power;
+	int			screenpad_brightness;
 	int			wlan_ctrl_by_user;
 
 	const char		*name;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index d17ae2eb0f8d..61ba70b32846 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -58,6 +58,10 @@
 #define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
 #define ASUS_WMI_DEVID_LIGHT_SENSOR	0x00050022 /* ?? */
 #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
+/* This can only be used to disable the screen, not re-enable */
+#define ASUS_WMI_DEVID_SCREENPAD_POWER	0x00050031
+/* Writing a brightness re-enables the screen if disabled */
+#define ASUS_WMI_DEVID_SCREENPAD_LIGHT	0x00050032
 #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
 #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
 
-- 
2.40.1

Re: [PATCH v3 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[kernel test robot]

Hi Luke,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.4-rc7 next-20230620]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luke-D-Jones/platform-x86-asus-wmi-add-support-for-ASUS-screenpad/20230620-110305
base:   linus/master
patch link:    https://lore.kernel.org/r/20230620030033.55033-2-luke%40ljones.dev
patch subject: [PATCH v3 1/1] platform/x86: asus-wmi: add support for ASUS screenpad
config: x86_64-randconfig-a001-20230620 (https://download.01.org/0day-ci/archive/20230621/202306210717.OFFdFt8Q-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230621/202306210717.OFFdFt8Q-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306210717.OFFdFt8Q-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/asus-wmi.c:3293:41: warning: variable 'bd' is uninitialized when used here [-Wuninitialized]
           brightness = read_screenpad_brightness(bd);
                                                  ^~
   drivers/platform/x86/asus-wmi.c:3285:29: note: initialize the variable 'bd' to silence this warning
           struct backlight_device *bd;
                                      ^
                                       = NULL
   1 warning generated.


vim +/bd +3293 drivers/platform/x86/asus-wmi.c

  3282	
  3283	static int asus_screenpad_init(struct asus_wmi *asus)
  3284	{
  3285		struct backlight_device *bd;
  3286		struct backlight_properties props;
  3287		int power, brightness;
  3288	
  3289		power = read_screenpad_backlight_power(asus);
  3290		if (power < 0)
  3291			return power;
  3292	
> 3293		brightness = read_screenpad_brightness(bd);
  3294		if (brightness < 0)
  3295			return brightness;
  3296	
  3297		memset(&props, 0, sizeof(struct backlight_properties));
  3298		props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */
  3299		props.max_brightness = 255;
  3300		bd = backlight_device_register("asus_screenpad",
  3301					       &asus->platform_device->dev, asus,
  3302					       &asus_screenpad_bl_ops, &props);
  3303		if (IS_ERR(bd)) {
  3304			pr_err("Could not register backlight device\n");
  3305			return PTR_ERR(bd);
  3306		}
  3307	
  3308		asus->screenpad_backlight_device = bd;
  3309		/*
  3310		 * Counter an odd behaviour where default is set to < 13 if it was 0 on boot.
  3311		 * 60 is subjective, but accepted as a good compromise to retain visibility.
  3312		 */
  3313		if (brightness < 60)
  3314			brightness = 60;
  3315	
  3316		asus->driver->screenpad_brightness = brightness;
  3317		bd->props.brightness = brightness;
  3318		bd->props.power = power;
  3319		backlight_update_status(bd);
  3320	
  3321		return 0;
  3322	}
  3323	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones]

On Tue, 2023-06-20 at 15:00 +1200, Luke D. Jones wrote:
> Add support for the WMI methods used to turn off and adjust the
> brightness of the secondary "screenpad" device found on some high-end
> ASUS laptops like the GX650P series and others.
> 
> These methods are utilised in a new backlight device named
> asus_screenpad.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 129
> +++++++++++++++++++++
>  drivers/platform/x86/asus-wmi.h            |   1 +
>  include/linux/platform_data/x86/asus-wmi.h |   4 +
>  3 files changed, 134 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c
> b/drivers/platform/x86/asus-wmi.c
> index 62cee13f5576..c87fbd81d658 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -25,6 +25,7 @@
>  #include <linux/input/sparse-keymap.h>
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/pci_hotplug.h>
> @@ -212,6 +213,7 @@ struct asus_wmi {
>  
>         struct input_dev *inputdev;
>         struct backlight_device *backlight_device;
> +       struct backlight_device *screenpad_backlight_device;
>         struct platform_device *platform_device;
>  
>         struct led_classdev wlan_led;
> @@ -3839,6 +3841,124 @@ static int is_display_toggle(int code)
>         return 0;
>  }
>  
> +/* Screenpad backlight
> *******************************************************/
> +
> +static int read_screenpad_backlight_power(struct asus_wmi *asus)
> +{
> +       int ret;
> +
> +       ret = asus_wmi_get_devstate_simple(asus,
> ASUS_WMI_DEVID_SCREENPAD_POWER);
> +       if (ret < 0)
> +               return ret;
> +       /* 1 == powered */
> +       return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
> +}
> +
> +static int read_screenpad_brightness(struct backlight_device *bd)
> +{
> +       struct asus_wmi *asus = bl_get_data(bd);
> +       u32 retval;
> +       int err;
> +
> +       err = read_screenpad_backlight_power(asus);
> +       if (err < 0)
> +               return err;
> +       /* The device brightness can only be read if powered, so
> return stored */
> +       if (err == FB_BLANK_POWERDOWN)
> +               return asus->driver->screenpad_brightness;
> +
> +       err = asus_wmi_get_devstate(asus,
> ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
> +       if (err < 0)
> +               return err;
> +
> +       return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
> +}
> +
> +static int update_screenpad_bl_status(struct backlight_device *bd)
> +{
> +       struct asus_wmi *asus = bl_get_data(bd);
> +       int power, err = 0;
> +       u32 ctrl_param;
> +
> +       power = read_screenpad_backlight_power(asus);
> +       if (power < 0)
> +               return power;
> +
> +       if (bd->props.power != power) {
> +               if (power != FB_BLANK_UNBLANK) {
> +                       /* Only brightness > 0 can power it back on
> */
> +                       ctrl_param = max(1, asus->driver-
> >screenpad_brightness);
> +                       err =
> asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
> +                                                   ctrl_param,
> NULL);
> +               } else {
> +                       err =
> asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
> +               }
> +       } else if (power == FB_BLANK_UNBLANK) {
> +               /* Only set brightness if powered on or we get
> invalid/unsync state */
> +               ctrl_param = bd->props.brightness;
> +               err =
> asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param,
> NULL);
> +       }
> +
> +       /* Ensure brightness is stored to turn back on with */
> +       asus->driver->screenpad_brightness = bd->props.brightness;
> +
> +       return err;
> +}
> +
> +static const struct backlight_ops asus_screenpad_bl_ops = {
> +       .get_brightness = read_screenpad_brightness,
> +       .update_status = update_screenpad_bl_status,
> +       .options = BL_CORE_SUSPENDRESUME,
> +};
> +
> +static int asus_screenpad_init(struct asus_wmi *asus)
> +{
> +       struct backlight_device *bd;
> +       struct backlight_properties props;
> +       int power, brightness;
> +
> +       power = read_screenpad_backlight_power(asus);
> +       if (power < 0)
> +               return power;
> +
> +       brightness = read_screenpad_brightness(bd);
> +       if (brightness < 0)
> +               return brightness;

I have made a mistake here. If power is off then
read_screenpad_backlight_power() tries to return a stored brightness
from the dev struct but this is uninitialised at this point.

I will submit v4.

> +
> +       memset(&props, 0, sizeof(struct backlight_properties));
> +       props.type = BACKLIGHT_RAW; /* ensure this bd is last to be
> picked */
> +       props.max_brightness = 255;
> +       bd = backlight_device_register("asus_screenpad",
> +                                      &asus->platform_device->dev,
> asus,
> +                                      &asus_screenpad_bl_ops,
> &props);
> +       if (IS_ERR(bd)) {
> +               pr_err("Could not register backlight device\n");
> +               return PTR_ERR(bd);
> +       }
> +
> +       asus->screenpad_backlight_device = bd;
> +       /*
> +        * Counter an odd behaviour where default is set to < 13 if
> it was 0 on boot.
> +        * 60 is subjective, but accepted as a good compromise to
> retain visibility.
> +        */
> +       if (brightness < 60)
> +               brightness = 60;
> +
> +       asus->driver->screenpad_brightness = brightness;
> +       bd->props.brightness = brightness;
> +       bd->props.power = power;
> +       backlight_update_status(bd);
> +
> +       return 0;
> +}
> +
> +static void asus_screenpad_exit(struct asus_wmi *asus)
> +{
> +       backlight_device_unregister(asus-
> >screenpad_backlight_device);
> +
> +       asus->screenpad_backlight_device = NULL;
> +}
> +
>  /* Fn-lock
> ********************************************************************/
>  
>  static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
> @@ -4504,6 +4624,12 @@ static int asus_wmi_add(struct platform_device
> *pdev)
>         } else if (asus->driver->quirks->wmi_backlight_set_devstate)
>                 err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT,
> 2, NULL);
>  
> +       if (asus_wmi_dev_is_present(asus,
> ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
> +               err = asus_screenpad_init(asus);
> +               if (err && err != -ENODEV)
> +                       goto fail_screenpad;
> +       }
> +
>         if (asus_wmi_has_fnlock_key(asus)) {
>                 asus->fnlock_locked = fnlock_default;
>                 asus_wmi_fnlock_update(asus);
> @@ -4527,6 +4653,8 @@ static int asus_wmi_add(struct platform_device
> *pdev)
>         asus_wmi_backlight_exit(asus);
>  fail_backlight:
>         asus_wmi_rfkill_exit(asus);
> +fail_screenpad:
> +       asus_screenpad_exit(asus);
>  fail_rfkill:
>         asus_wmi_led_exit(asus);
>  fail_leds:
> @@ -4553,6 +4681,7 @@ static int asus_wmi_remove(struct
> platform_device *device)
>         asus = platform_get_drvdata(device);
>         wmi_remove_notify_handler(asus->driver->event_guid);
>         asus_wmi_backlight_exit(asus);
> +       asus_screenpad_exit(asus);
>         asus_wmi_input_exit(asus);
>         asus_wmi_led_exit(asus);
>         asus_wmi_rfkill_exit(asus);
> diff --git a/drivers/platform/x86/asus-wmi.h
> b/drivers/platform/x86/asus-wmi.h
> index a478ebfd34df..5fbdd0eafa02 100644
> --- a/drivers/platform/x86/asus-wmi.h
> +++ b/drivers/platform/x86/asus-wmi.h
> @@ -57,6 +57,7 @@ struct quirk_entry {
>  struct asus_wmi_driver {
>         int                     brightness;
>         int                     panel_power;
> +       int                     screenpad_brightness;
>         int                     wlan_ctrl_by_user;
>  
>         const char              *name;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h
> b/include/linux/platform_data/x86/asus-wmi.h
> index d17ae2eb0f8d..61ba70b32846 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -58,6 +58,10 @@
>  #define ASUS_WMI_DEVID_KBD_BACKLIGHT   0x00050021
>  #define ASUS_WMI_DEVID_LIGHT_SENSOR    0x00050022 /* ?? */
>  #define ASUS_WMI_DEVID_LIGHTBAR                0x00050025
> +/* This can only be used to disable the screen, not re-enable */
> +#define ASUS_WMI_DEVID_SCREENPAD_POWER 0x00050031
> +/* Writing a brightness re-enables the screen if disabled */
> +#define ASUS_WMI_DEVID_SCREENPAD_LIGHT 0x00050032
>  #define ASUS_WMI_DEVID_FAN_BOOST_MODE  0x00110018
>  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
>