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

From: Luke Jones
Date: Fri May 05 2023 - 19:44:20 EST



On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
On Fri, 5 May 2023, 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@xxxxxxxxxx>
---
.../ABI/testing/sysfs-platform-asus-wmi | 2 +-
drivers/platform/x86/asus-wmi.c | 132 ++++++++++++++++++
drivers/platform/x86/asus-wmi.h | 1 +
include/linux/platform_data/x86/asus-wmi.h | 4 +
4 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index a77a004a1baa..df9817c6233a 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -97,4 +97,4 @@ Contact: "Luke Jones" <luke@xxxxxxxxxx>
Description:
Enable an LCD response-time boost to reduce or remove ghosting:
* 0 - Disable,
- * 1 - Enable
+ * 1 - Enable
\ No newline at end of file

Spurious change?

Indeed it is. Not sure how that occurred.


diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1038dfdcdd32..0528eef02ef7 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -200,6 +200,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;
@@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
return 0;
}

+/* Screenpad backlight */
+
+static int read_screenpad_backlight_power(struct asus_wmi *asus)
+{
+ int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);

Please move this to own line because now you have the extra newline
in between the call and error handling.

I don't understand what you mean sorry. Remove the new line or:
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 == -ENODEV)
+ return err;

Just return 0. Or is there perhaps something wrong/missing here?

I thought the correct thing was to return any possible error state (here, anything less than 0 would be an error, right?)


+ else if (power < 0)
+ return power;
+
+ if (bd->props.power != power) {
+ if (power != FB_BLANK_UNBLANK) {
+ /* Only brightness can power it back on */

Only brightness > 0 can power the screen back on

+ ctrl_param = asus->driver->screenpad_brightness;

max(1, asus->driver->screenpad_brightness);

Don't forget to add the #include for it.

Oh, that's handy! Thank you.


+ /* Min 1 or the screen won't turn on */
+ if (ctrl_param == 0)
+ ctrl_param = 1;

Drop this.

Thanks to minmax.


+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
+ ctrl_param, NULL);

Align param.

Done.


+ } else {
+ /* Ensure brightness is stored to turn back on with */
+ asus->driver->screenpad_brightness = bd->props.brightness;
+ 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);

Why not store the brightness if powered off?

That's me being literal and short sighted. I've now moved:
```
/* Ensure brightness is stored to turn back on with */
asus->driver->screenpad_brightness = bd->props.brightness;
```
to below the conditional blocks.


+ }
+
+ 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 == -ENODEV)
+ power = FB_BLANK_UNBLANK;
+ else if (power < 0)
+ return power;
+
+ 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;
+
+ brightness = read_screenpad_brightness(bd);
+ if (brightness < 0)
+ return brightness;
+ /*
+ * 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.
+ */
+ else if (brightness < 60)

Since the other branch returns, else is unnecessary.

Good catch, thank you.

I'll submit V3 after we clarify the two points above that I'm confused by :)

Thank you for taking the time to review.