* [PATCH v5 0/1] platform/x86: asus-wmi: add support for ASUS screenpad @ 2023-08-27 23:23 Luke D. Jones 2023-08-27 23:23 ` [PATCH v5 1/1] " Luke D. Jones 0 siblings, 1 reply; 3+ messages in thread From: Luke D. Jones @ 2023-08-27 23:23 UTC (permalink / raw) To: hdegoede; +Cc: corentin.chary, markgross, linux-kernel, 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: - V5 - Add minimum brightness to prevent screen from becoming invisible - Previous: https://lore.kernel.org/all/20230630041743.911303-1-luke@ljones.dev/ - V4 - Fix a null dereference that happened if the display was powered off and dev struct uninitialised yet - Previous: https://lore.kernel.org/all/20230620030033.55033-1-luke@ljones.dev/ - 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 | 131 +++++++++++++++++++++ drivers/platform/x86/asus-wmi.h | 1 + include/linux/platform_data/x86/asus-wmi.h | 4 + 3 files changed, 136 insertions(+) -- 2.41.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v5 1/1] platform/x86: asus-wmi: add support for ASUS screenpad 2023-08-27 23:23 [PATCH v5 0/1] platform/x86: asus-wmi: add support for ASUS screenpad Luke D. Jones @ 2023-08-27 23:23 ` Luke D. Jones 2023-08-28 10:15 ` Hans de Goede 0 siblings, 1 reply; 3+ messages in thread From: Luke D. Jones @ 2023-08-27 23:23 UTC (permalink / raw) To: hdegoede; +Cc: corentin.chary, markgross, linux-kernel, 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 | 131 +++++++++++++++++++++ drivers/platform/x86/asus-wmi.h | 1 + include/linux/platform_data/x86/asus-wmi.h | 4 + 3 files changed, 136 insertions(+) diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 13547e55ae82..2801c691133a 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> @@ -127,6 +128,9 @@ module_param(fnlock_default, bool, 0444); #define NVIDIA_TEMP_MIN 75 #define NVIDIA_TEMP_MAX 87 +#define ASUS_SCREENPAD_BRIGHT_MIN 20 +#define ASUS_SCREENPAD_BRIGHT_MAX 255 + static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL }; static int throttle_thermal_policy_write(struct asus_wmi *); @@ -212,6 +216,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; @@ -3776,6 +3781,123 @@ 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) - ASUS_SCREENPAD_BRIGHT_MIN; +} + +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(ASUS_SCREENPAD_BRIGHT_MIN, 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 + ASUS_SCREENPAD_BRIGHT_MIN; + 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 + ASUS_SCREENPAD_BRIGHT_MIN; + + 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 err, power; + int brightness = 0; + + power = read_screenpad_backlight_power(asus); + if (power < 0) + return power; + + if (power != FB_BLANK_POWERDOWN) { + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &brightness); + if (err < 0) + return err; + } + /* default to an acceptable min brightness on boot if too low */ + if (brightness < ASUS_SCREENPAD_BRIGHT_MIN) + brightness = ASUS_SCREENPAD_BRIGHT_MIN; + + memset(&props, 0, sizeof(struct backlight_properties)); + props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */ + props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX - ASUS_SCREENPAD_BRIGHT_MIN; + 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; + 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) @@ -4431,6 +4553,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); @@ -4454,6 +4582,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: @@ -4480,6 +4610,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 16e99a1c37fc..63e630276499 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.41.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v5 1/1] platform/x86: asus-wmi: add support for ASUS screenpad 2023-08-27 23:23 ` [PATCH v5 1/1] " Luke D. Jones @ 2023-08-28 10:15 ` Hans de Goede 0 siblings, 0 replies; 3+ messages in thread From: Hans de Goede @ 2023-08-28 10:15 UTC (permalink / raw) To: Luke D. Jones Cc: corentin.chary, markgross, linux-kernel, platform-driver-x86 Hi Luke, Thank you for your patch. For the next version please don't forget to Cc: platform-driver-x86@vger.kernel.org On 8/28/23 01:23, 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. You have quite a bit of useful extra info in your cover-letter, for the next version please add a copy of that info to your commit message. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > drivers/platform/x86/asus-wmi.c | 131 +++++++++++++++++++++ > drivers/platform/x86/asus-wmi.h | 1 + > include/linux/platform_data/x86/asus-wmi.h | 4 + > 3 files changed, 136 insertions(+) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 13547e55ae82..2801c691133a 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> > @@ -127,6 +128,9 @@ module_param(fnlock_default, bool, 0444); > #define NVIDIA_TEMP_MIN 75 > #define NVIDIA_TEMP_MAX 87 > > +#define ASUS_SCREENPAD_BRIGHT_MIN 20 Hmm, in the coverletter you talk about 15 as min value and 60 as default value when the value read back from the hw at probe() time is < min. I'm fine with using 20 instead of 15, but the whole "The compromise was to set the brightness to 60 which is a usable brightness if the module init brightness was under 15." thing from your cover letter seems to be missing. > +#define ASUS_SCREENPAD_BRIGHT_MAX 255 > + > static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL }; > > static int throttle_thermal_policy_write(struct asus_wmi *); > @@ -212,6 +216,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; > @@ -3776,6 +3781,123 @@ 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; You are missing a " - ASUS_SCREENPAD_BRIGHT_MIN" here (you do add it when storing values to asus->driver->screenpad_brightness in update_screenpad_bl_status()). > + > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval); > + if (err < 0) > + return err; > + > + return (retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK) - ASUS_SCREENPAD_BRIGHT_MIN; > +} > + > +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(ASUS_SCREENPAD_BRIGHT_MIN, asus->driver->screenpad_brightness); Not sure you need the max() here, I don't think you ever store a value below ASUS_SCREENPAD_BRIGHT_MIN in 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 + ASUS_SCREENPAD_BRIGHT_MIN; > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL); > + } > + Not sure if you should update screenpad_brightness here on errors ? Maybe add a "if (err == 0)" as condition ? > + /* Ensure brightness is stored to turn back on with */ > + asus->driver->screenpad_brightness = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN; So screenpad_brightness is never below ASUS_SCREENPAD_BRIGHT_MIN here. > + > + 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 err, power; > + int brightness = 0; > + > + power = read_screenpad_backlight_power(asus); > + if (power < 0) > + return power; > + > + if (power != FB_BLANK_POWERDOWN) { > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &brightness); > + if (err < 0) > + return err; > + } > + /* default to an acceptable min brightness on boot if too low */ > + if (brightness < ASUS_SCREENPAD_BRIGHT_MIN) > + brightness = ASUS_SCREENPAD_BRIGHT_MIN; brightness = ASUS_SCREENPAD_BRIGHT_DEFAULT; Instead (new define for "60") ? > + > + memset(&props, 0, sizeof(struct backlight_properties)); > + props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */ > + props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX - ASUS_SCREENPAD_BRIGHT_MIN; > + 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; > + asus->driver->screenpad_brightness = brightness; > + bd->props.brightness = brightness; And you've checked brightness against ASUS_SCREENPAD_BRIGHT_MIN above, so screenpad_brightness is never below ASUS_SCREENPAD_BRIGHT_MIN here either. Otherwise this looks good to me. Regards, Hans > + 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) > @@ -4431,6 +4553,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); > @@ -4454,6 +4582,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: > @@ -4480,6 +4610,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 16e99a1c37fc..63e630276499 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-28 10:17 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-27 23:23 [PATCH v5 0/1] platform/x86: asus-wmi: add support for ASUS screenpad Luke D. Jones 2023-08-27 23:23 ` [PATCH v5 1/1] " Luke D. Jones 2023-08-28 10:15 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).