asus-wmi: fixup screenpad brightness

[PATCH] asus-wmi: fixup screenpad brightness

Posted by Luke Jones 6 months, 3 weeks ago

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

Re: [PATCH] asus-wmi: fixup screenpad brightness

Posted by Ilpo Järvinen 6 months, 1 week ago

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.

Re: [PATCH] asus-wmi: fixup screenpad brightness

Posted by Luke Jones 6 months, 1 week ago

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.