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.
