* [PATCH] asus-wmi: fixup screenpad brightness
@ 2025-05-25 20:42 Luke Jones
2025-06-09 7:31 ` Ilpo Järvinen
0 siblings, 1 reply; 3+ messages in thread
From: Luke Jones @ 2025-05-25 20:42 UTC (permalink / raw)
To: linux-kernel; +Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, Luke Jones
Fix up some inconsistent behaviour involving the screenpad on some
ASUS laptops. This fixes:
- illogical screen off control (0/1 flipped depending on WMI state)
- bad brightness depending on the last screenpad power state
- incorrect brightness scaling
Signed-off-by: Luke Jones <luke@ljones.dev>
---
drivers/platform/x86/asus-wmi.c | 52 +++++++++++++--------------------
1 file changed, 21 insertions(+), 31 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index f52884e90759..cec509171971 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -123,7 +123,6 @@ 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
#define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
@@ -4239,43 +4238,37 @@ static int read_screenpad_brightness(struct backlight_device *bd)
return err;
/* The device brightness can only be read if powered, so return stored */
if (err == BACKLIGHT_POWER_OFF)
- return asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
+ return bd->props.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;
+ 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;
+ int err = 0;
u32 ctrl_param;
- power = read_screenpad_backlight_power(asus);
- if (power < 0)
- return power;
-
- if (bd->props.power != power) {
- if (power != BACKLIGHT_POWER_ON) {
- /* Only brightness > 0 can power it back on */
- ctrl_param = asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
- 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 == BACKLIGHT_POWER_ON) {
- /* Only set brightness if powered on or we get invalid/unsync state */
- ctrl_param = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
+ ctrl_param = bd->props.brightness;
+ if (ctrl_param >= 0 && bd->props.power) {
+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1,
+ NULL);
+ if (err < 0)
+ return err;
+ ctrl_param = bd->props.brightness;
err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
+ if (err < 0)
+ return err;
}
- /* Ensure brightness is stored to turn back on with */
- if (err == 0)
- asus->driver->screenpad_brightness = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
+ if (!bd->props.power) {
+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
+ if (err < 0)
+ return err;
+ }
return err;
}
@@ -4293,22 +4286,19 @@ static int asus_screenpad_init(struct asus_wmi *asus)
int err, power;
int brightness = 0;
- power = read_screenpad_backlight_power(asus);
+ power = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
if (power < 0)
return power;
- if (power != BACKLIGHT_POWER_OFF) {
+ if (power) {
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_DEFAULT;
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;
+ props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX;
bd = backlight_device_register("asus_screenpad",
&asus->platform_device->dev, asus,
&asus_screenpad_bl_ops, &props);
@@ -4319,7 +4309,7 @@ static int asus_screenpad_init(struct asus_wmi *asus)
asus->screenpad_backlight_device = bd;
asus->driver->screenpad_brightness = brightness;
- bd->props.brightness = brightness - ASUS_SCREENPAD_BRIGHT_MIN;
+ bd->props.brightness = brightness;
bd->props.power = power;
backlight_update_status(bd);
--
2.49.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] asus-wmi: fixup screenpad brightness
2025-05-25 20:42 [PATCH] asus-wmi: fixup screenpad brightness Luke Jones
@ 2025-06-09 7:31 ` Ilpo Järvinen
2025-06-10 1:20 ` Luke Jones
0 siblings, 1 reply; 3+ messages in thread
From: Ilpo Järvinen @ 2025-06-09 7:31 UTC (permalink / raw)
To: Luke Jones; +Cc: LKML, Hans de Goede, platform-driver-x86
On Sun, 25 May 2025, Luke Jones wrote:
> Fix up some inconsistent behaviour involving the screenpad on some
> ASUS laptops. This fixes:
> - illogical screen off control (0/1 flipped depending on WMI state)
> - bad brightness depending on the last screenpad power state
> - incorrect brightness scaling
Why did you put them all into a single patch? I understand there's some
overlap in lines they touch but if they can be separated, it would be
much better and I'd likely have much higher confidence on each change.
> Signed-off-by: Luke Jones <luke@ljones.dev>
> ---
> drivers/platform/x86/asus-wmi.c | 52 +++++++++++++--------------------
> 1 file changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index f52884e90759..cec509171971 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -123,7 +123,6 @@ 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
> #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>
> @@ -4239,43 +4238,37 @@ static int read_screenpad_brightness(struct backlight_device *bd)
> return err;
> /* The device brightness can only be read if powered, so return stored */
> if (err == BACKLIGHT_POWER_OFF)
> - return asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
> + return bd->props.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;
> + 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;
> + int err = 0;
> u32 ctrl_param;
>
> - power = read_screenpad_backlight_power(asus);
> - if (power < 0)
> - return power;
> -
> - if (bd->props.power != power) {
> - if (power != BACKLIGHT_POWER_ON) {
> - /* Only brightness > 0 can power it back on */
> - ctrl_param = asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
> - 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 == BACKLIGHT_POWER_ON) {
> - /* Only set brightness if powered on or we get invalid/unsync state */
> - ctrl_param = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
> + ctrl_param = bd->props.brightness;
> + if (ctrl_param >= 0 && bd->props.power) {
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1,
> + NULL);
> + if (err < 0)
> + return err;
> + ctrl_param = bd->props.brightness;
> err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
> + if (err < 0)
> + return err;
> }
>
> - /* Ensure brightness is stored to turn back on with */
> - if (err == 0)
> - asus->driver->screenpad_brightness = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
> + if (!bd->props.power) {
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
> + if (err < 0)
> + return err;
> + }
>
> return err;
> }
> @@ -4293,22 +4286,19 @@ static int asus_screenpad_init(struct asus_wmi *asus)
> int err, power;
> int brightness = 0;
>
> - power = read_screenpad_backlight_power(asus);
> + power = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
> if (power < 0)
> return power;
>
> - if (power != BACKLIGHT_POWER_OFF) {
> + if (power) {
> 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_DEFAULT;
>
> 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;
> + props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX;
> bd = backlight_device_register("asus_screenpad",
> &asus->platform_device->dev, asus,
> &asus_screenpad_bl_ops, &props);
> @@ -4319,7 +4309,7 @@ static int asus_screenpad_init(struct asus_wmi *asus)
>
> asus->screenpad_backlight_device = bd;
> asus->driver->screenpad_brightness = brightness;
> - bd->props.brightness = brightness - ASUS_SCREENPAD_BRIGHT_MIN;
> + bd->props.brightness = brightness;
> bd->props.power = power;
> backlight_update_status(bd);
>
>
--
i.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] asus-wmi: fixup screenpad brightness
2025-06-09 7:31 ` Ilpo Järvinen
@ 2025-06-10 1:20 ` Luke Jones
0 siblings, 0 replies; 3+ messages in thread
From: Luke Jones @ 2025-06-10 1:20 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: LKML, Hans de Goede, platform-driver-x86
On Mon, 9 Jun 2025, at 7:31 PM, Ilpo Järvinen wrote:
> On Sun, 25 May 2025, Luke Jones wrote:
>
>> Fix up some inconsistent behaviour involving the screenpad on some
>> ASUS laptops. This fixes:
>> - illogical screen off control (0/1 flipped depending on WMI state)
>> - bad brightness depending on the last screenpad power state
>> - incorrect brightness scaling
>
> Why did you put them all into a single patch? I understand there's some
> overlap in lines they touch but if they can be separated, it would be
> much better and I'd likely have much higher confidence on each change.
Because it wasn't easy or practical to try and fix just one part - the screenpad is quite funky in how it works (and extremely annoying) where trying to fix power made brightness behave out of sync, or fixing brightness made power appear to be randomly affecting brightness. In the end it was easier to steamroll the whole thing. It's now been quite thoroughly tested on an actual screenpad based laptop that asus sent me.
I'm not keen on sinking more in to this beyond fixes. I simply do not have the time this year.
>
>> Signed-off-by: Luke Jones <luke@ljones.dev>
>> ---
>> drivers/platform/x86/asus-wmi.c | 52 +++++++++++++--------------------
>> 1 file changed, 21 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index f52884e90759..cec509171971 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -123,7 +123,6 @@ 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
>> #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>>
>> @@ -4239,43 +4238,37 @@ static int read_screenpad_brightness(struct backlight_device *bd)
>> return err;
>> /* The device brightness can only be read if powered, so return stored */
>> if (err == BACKLIGHT_POWER_OFF)
>> - return asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
>> + return bd->props.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;
>> + 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;
>> + int err = 0;
>> u32 ctrl_param;
>>
>> - power = read_screenpad_backlight_power(asus);
>> - if (power < 0)
>> - return power;
>> -
>> - if (bd->props.power != power) {
>> - if (power != BACKLIGHT_POWER_ON) {
>> - /* Only brightness > 0 can power it back on */
>> - ctrl_param = asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
>> - 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 == BACKLIGHT_POWER_ON) {
>> - /* Only set brightness if powered on or we get invalid/unsync state */
>> - ctrl_param = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
>> + ctrl_param = bd->props.brightness;
>> + if (ctrl_param >= 0 && bd->props.power) {
>> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1,
>> + NULL);
>> + if (err < 0)
>> + return err;
>> + ctrl_param = bd->props.brightness;
>> err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
>> + if (err < 0)
>> + return err;
>> }
>>
>> - /* Ensure brightness is stored to turn back on with */
>> - if (err == 0)
>> - asus->driver->screenpad_brightness = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
>> + if (!bd->props.power) {
>> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
>> + if (err < 0)
>> + return err;
>> + }
>>
>> return err;
>> }
>> @@ -4293,22 +4286,19 @@ static int asus_screenpad_init(struct asus_wmi *asus)
>> int err, power;
>> int brightness = 0;
>>
>> - power = read_screenpad_backlight_power(asus);
>> + power = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>> if (power < 0)
>> return power;
>>
>> - if (power != BACKLIGHT_POWER_OFF) {
>> + if (power) {
>> 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_DEFAULT;
>>
>> 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;
>> + props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX;
>> bd = backlight_device_register("asus_screenpad",
>> &asus->platform_device->dev, asus,
>> &asus_screenpad_bl_ops, &props);
>> @@ -4319,7 +4309,7 @@ static int asus_screenpad_init(struct asus_wmi *asus)
>>
>> asus->screenpad_backlight_device = bd;
>> asus->driver->screenpad_brightness = brightness;
>> - bd->props.brightness = brightness - ASUS_SCREENPAD_BRIGHT_MIN;
>> + bd->props.brightness = brightness;
>> bd->props.power = power;
>> backlight_update_status(bd);
>>
>>
>
> --
> i.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-06-10 1:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-25 20:42 [PATCH] asus-wmi: fixup screenpad brightness Luke Jones
2025-06-09 7:31 ` Ilpo Järvinen
2025-06-10 1:20 ` Luke Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox