[PATCH v1 0/4] platform/x86: asus-wmi: fix screenpad backlight regression

[PATCH v1 0/4] platform/x86: asus-wmi: fix screenpad backlight regression

[Denis Benato]

Hi all,

I have touched ScreenPad handling code based on latest Luke's work
and in doing so I caused some regressions in the screenpad backlight
handling.

This patchset attempts to amend those regressions while also
improving the support based on feedbacks and suggestions
I have received.

The screenpad backlight regression introduced by 130d29c5627c
("platform/x86: asus-wmi: adjust screenpad power/brightness handling")
left the ScreenPad panel unusable on several models: the panel is
powered off at probe time or bl_power writes have inverted effects.
Patches 1 and 2 fix the regression, patch 3 makes the screenpad
backlight honor the core blank and suspend states and patch 4 removes
dead code left behind by the same rework.

I would ask everybody involved to test these changes again and
provide feedback.

Thanks to everyone who provided feedback and helped
identify the regressions, especially Hugo Baigue that also
provided values straight from the hardware and suggestions.

The use of AI here is to review the content and to research
relevant links, putting the correct tags at the end of
each patch and referencing the correct people,
the rest comes either from me or from Hugo.

Honestly I am a bit unsure on what older DUO models might
make of these changes, but it's impossible for them to work
worse than they are doing now due to me inverting
the backlight logic, so I'm willing to hear any brightness
misbehavior and to resolve them on a per-model basis if
this works well in newer laptops.

Best regards,
Denis Benato

Link: https://lore.kernel.org/regressions/178362762638.911488.8564892548331679884@eldamar.lan/ (X532FA regression, Debian #1141604)
Link: https://bugs.debian.org/1141604
Link: https://lore.kernel.org/all/CAO84+xJLVW_9_D9-8cwJYv_Vz+LK7nJqRaG-1P4_eeLrQmSiWw@mail.gmail.com/ (UX5400EA power state report)
Link: https://lore.kernel.org/all/CAO84+x+P2_xyHP89+nGVSMV6bL+dy0P9=vyEEfZGnFhv0hBNWw@mail.gmail.com/ (UX5400EA tests, brightness fix and DSDT analysis)
Link: https://lore.kernel.org/all/985b98f7-04a3-449b-ab6e-7f23e29e4b43@gmail.com/ (X532FA confirming patch 1 works)

Denis Benato (4):
  platform/x86: asus-wmi: fix unclear usage of bd->props.power
  platform/x86: asus-wmi: fix screenpad power state detection
  platform/x86: asus-wmi: use backlight_is_blank() for screenpad power
  platform/x86: asus-wmi: remove unused screenpad_brightness

 drivers/platform/x86/asus-wmi.c | 50 ++++++++++++++++-----------------
 drivers/platform/x86/asus-wmi.h |  1 -
 2 files changed, 24 insertions(+), 27 deletions(-)

-- 
2.47.3

[PATCH v1 1/4] platform/x86: asus-wmi: fix unclear usage of bd->props.power

[Denis Benato]

The bd->props.power is checked in parts of the driver correctly
comparing with BACKLIGHT_POWER_ON, while in others with a raw usage
of bd->props.power and !bd->props.power, moreover in certain checks
the logic has been inverted due to BACKLIGHT_POWER_ON being defined
as 0: fix both the wrong usage and the inconsistencies by using
proper comparisons.

Fixes: 130d29c5627c ("platform/x86: asus-wmi: adjust screenpad power/brightness handling")
Closes: https://lore.kernel.org/all/178362762638.911488.8564892548331679884@eldamar.lan/
Closes: https://lore.kernel.org/all/ea9c63d1-4776-49d5-9dc4-6c09498f99c9@linux.dev/
Tested-by: Hugo Baigue <hugobaigue2004@gmail.com>
Tested-by: Ponali <ponali2k@gmail.com>
Cc: stable@vger.kernel.org
Assisted-by: zcode:glm-5.3-flash
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-wmi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index b4fd2257bc8e..7382f2b38678 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4532,7 +4532,8 @@ static int update_screenpad_bl_status(struct backlight_device *bd)
 	u32 ctrl_param = bd->props.brightness;
 	int err = 0;
 
-	if (bd->props.power) {
+	switch (bd->props.power) {
+	case BACKLIGHT_POWER_ON:
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1, NULL);
 		if (err < 0)
 			return err;
@@ -4540,12 +4541,17 @@ static int update_screenpad_bl_status(struct backlight_device *bd)
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
 		if (err < 0)
 			return err;
-	}
+		break;
 
-	if (!bd->props.power) {
+	case BACKLIGHT_POWER_OFF:
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
 		if (err < 0)
 			return err;
+		break;
+
+	default:
+		pr_warn("Invalid screenpad backlight power state: %d\n", bd->props.power);
+		return -EINVAL;
 	}
 
 	return err;
-- 
2.47.3

[PATCH v1 2/4] platform/x86: asus-wmi: fix screenpad power state detection

[Denis Benato]

On some models, e.g. the UX5400EA, the screenpad power state cannot be
read back: DSTS(ASUS_WMI_DEVID_SCREENPAD_POWER) never sets
ASUS_WMI_DSTS_STATUS_BIT, so asus_wmi_get_devstate_simple() reports the
panel as powered off regardless of its real state.

The DSDT shows DSTS 0x00050031 (POWER) and DSTS 0x00050032 (LIGHT)
both read the same two-byte EC register, each returning a different
byte: byte 0 is a raw EC status byte, which is 0xa0 when the panel is
powered and 0x00 when it is off, and byte 1 is the brightness. Read
the power state from the low byte instead of
ASUS_WMI_DSTS_STATUS_BIT: firmware reporting the power through the
status bit is covered too, since ASUS_WMI_DSTS_STATUS_BIT lies inside
ASUS_WMI_DSTS_BRIGHTNESS_MASK.

Reuse read_screenpad_backlight_power() in asus_screenpad_init() in
place of the raw devstate read. This makes the brightness read
reachable on models like the UX5400EA, so mask the SCREENPAD_LIGHT
value with ASUS_WMI_DSTS_BRIGHTNESS_MASK before storing it: the
unmasked devstate (0x0001ffa0 when the panel is on) would otherwise be
exposed to userspace as an out-of-range brightness (max_brightness is
255) which systemd-backlight then persists.

While at it, pass asus_wmi_get_devstate() the u32 it expects.

Fixes: 130d29c5627c ("platform/x86: asus-wmi: adjust screenpad power/brightness handling")
Reported-by: Hugo Baigue <hugobaigue2004@gmail.com>
Closes: https://lore.kernel.org/all/CAO84+x+P2_xyHP89+nGVSMV6bL+dy0P9=vyEEfZGnFhv0hBNWw@mail.gmail.com/
Suggested-by: Hugo Baigue <hugobaigue2004@gmail.com>
Tested-by: Hugo Baigue <hugobaigue2004@gmail.com>
Cc: stable@vger.kernel.org
Assisted-by: zcode:glm-5.3-flash
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-wmi.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 7382f2b38678..ded1aa356cf3 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4498,13 +4498,20 @@ static int is_display_toggle(int code)
 
 static int read_screenpad_backlight_power(struct asus_wmi *asus)
 {
-	int ret;
+	int ret, retval;
 
-	ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
+	ret = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_POWER, &retval);
 	if (ret < 0)
 		return ret;
-	/* 1 == powered */
-	return ret ? BACKLIGHT_POWER_ON : BACKLIGHT_POWER_OFF;
+
+	/*
+	 * The firmware reports the panel power in the low byte of the
+	 * devstate as a raw EC status byte that is non-zero when the
+	 * panel is powered; other models report it through
+	 * ASUS_WMI_DSTS_STATUS_BIT, which lies inside the same mask.
+	 */
+	return (retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK) ?
+		BACKLIGHT_POWER_ON : BACKLIGHT_POWER_OFF;
 }
 
 static int read_screenpad_brightness(struct backlight_device *bd)
@@ -4568,16 +4575,17 @@ static int asus_screenpad_init(struct asus_wmi *asus)
 	struct backlight_device *bd;
 	struct backlight_properties props;
 	int err, power;
-	int brightness = 0;
+	u32 brightness = 0;
 
-	power = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
+	power = read_screenpad_backlight_power(asus);
 	if (power < 0)
 		return power;
 
-	if (power) {
+	if (power == BACKLIGHT_POWER_ON) {
 		err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT, &brightness);
 		if (err < 0)
 			return err;
+		brightness &= ASUS_WMI_DSTS_BRIGHTNESS_MASK;
 	}
 
 	memset(&props, 0, sizeof(struct backlight_properties));
-- 
2.47.3

[PATCH v1 3/4] platform/x86: asus-wmi: use backlight_is_blank() for screenpad power

[Denis Benato]

The screenpad backlight sets BL_CORE_SUSPENDRESUME, so the backlight
core calls update_status() on suspend, resume and fb blanking with
BL_CORE_SUSPENDED or BL_CORE_FBBLANK set in bd->props.state while
bd->props.power keeps its value: the switch on bd->props.power alone
ignored those flags, repowering the panel at suspend entry instead of
turning it off, and ignoring fb blank requests.

Replace the switch with backlight_is_blank(), which accounts for both
bd->props.power and bd->props.state: the panel is powered off whenever
the backlight is blank and powered on with backlight_get_brightness()
otherwise. Writing a power state other than BACKLIGHT_POWER_ON or
BACKLIGHT_POWER_OFF to bl_power now blanks the panel following the
core convention instead of warning and failing with -EINVAL.

The visible change is that the panel now actually powers off on
suspend and fb blank, and is restored on unblank and resume.

Suggested-by: Hugo Baigue <hugobaigue2004@gmail.com>
Closes: https://lore.kernel.org/all/CAO84+xJ9aW3pj3x8e9b5biWtnNd4EyH7A4Uy7aqBR4qZMDcVvg@mail.gmail.com/
Assisted-by: zcode:glm-5.3-flash
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-wmi.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ded1aa356cf3..e6f3a5c0dba9 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4536,32 +4536,18 @@ static int read_screenpad_brightness(struct backlight_device *bd)
 
 static int update_screenpad_bl_status(struct backlight_device *bd)
 {
-	u32 ctrl_param = bd->props.brightness;
-	int err = 0;
-
-	switch (bd->props.power) {
-	case BACKLIGHT_POWER_ON:
-		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1, NULL);
-		if (err < 0)
-			return err;
-
-		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
-		if (err < 0)
-			return err;
-		break;
+	int err;
 
-	case BACKLIGHT_POWER_OFF:
-		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
-		if (err < 0)
-			return err;
-		break;
+	if (backlight_is_blank(bd))
+		return asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER,
+					     0, NULL);
 
-	default:
-		pr_warn("Invalid screenpad backlight power state: %d\n", bd->props.power);
-		return -EINVAL;
-	}
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1, NULL);
+	if (err < 0)
+		return err;
 
-	return err;
+	return asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
+				     backlight_get_brightness(bd), NULL);
 }
 
 static const struct backlight_ops asus_screenpad_bl_ops = {
-- 
2.47.3

[PATCH v1 4/4] platform/x86: asus-wmi: remove unused screenpad_brightness

[Denis Benato]

Nothing reads asus->driver->screenpad_brightness anymore: the last
reader went away when the screenpad update path stopped relying on a
driver-side copy of the brightness, leaving only the write in
asus_screenpad_init(). ASUS_SCREENPAD_BRIGHT_DEFAULT became unused in
the same rework.

Drop the field from struct asus_wmi_driver, the write and the unused
define. Nothing in the current code depends on remembering the last
brightness while the panel is off; should a model turn up that needs
it (the original screenpad implementation did, and older DUO models
may behave differently from the hardware tested so far), the field can
be reintroduced then.

Assisted-by: zcode:glm-5.3-flash
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-wmi.c | 2 --
 drivers/platform/x86/asus-wmi.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index e6f3a5c0dba9..440b86549eb9 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -127,7 +127,6 @@ module_param(fnlock_default, bool, 0444);
 #define NVIDIA_TEMP_MAX		87
 
 #define ASUS_SCREENPAD_BRIGHT_MAX 255
-#define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
 
 #define ASUS_MINI_LED_MODE_MASK		0x03
 /* Standard modes for devices with only on/off */
@@ -4586,7 +4585,6 @@ static int asus_screenpad_init(struct asus_wmi *asus)
 	}
 
 	asus->screenpad_backlight_device = bd;
-	asus->driver->screenpad_brightness = brightness;
 	bd->props.brightness = brightness;
 	bd->props.power = power;
 	backlight_update_status(bd);
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index 5cd4392b964e..b45f81b38a47 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -57,7 +57,6 @@ struct quirk_entry {
 struct asus_wmi_driver {
 	int			brightness;
 	int			panel_power;
-	int			screenpad_brightness;
 	int			wlan_ctrl_by_user;
 
 	const char		*name;
-- 
2.47.3