Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for ASUS screenpad
From: Luke Jones
Date: Tue Jul 11 2023 - 18:22:02 EST
On Tue, Jul 11 2023 at 11:42:25 +02:00:00, Hans de Goede
<hdegoede@xxxxxxxxxx> wrote:
Hi,
On 7/7/23 00:23, Luke Jones wrote:
On Tue, 2023-07-04 at 13:16 +0200, Hans de Goede wrote:
Hi Luke,
On 6/30/23 06:17, 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>
Thank you for your work on this. I have one small change request
and then v5 should be ready for merging, see me inline comment
below.
---
drivers/platform/x86/asus-wmi.c | 128
+++++++++++++++++++++
drivers/platform/x86/asus-wmi.h | 1 +
include/linux/platform_data/x86/asus-wmi.h | 4 +
3 files changed, 133 insertions(+)
diff --git a/drivers/platform/x86/asus-wmi.c
b/drivers/platform/x86/asus-wmi.c
index 62cee13f5576..967c92ceb041 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -25,6 +25,7 @@
#include <linux/input/sparse-keymap.h>
#include <linux/kernel.h>
#include <linux/leds.h>
+#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
@@ -212,6 +213,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;
@@ -3839,6 +3841,123 @@ static int is_display_toggle(int code)
return 0;
}
+/* Screenpad backlight
*******************************************************/
+
+static int read_screenpad_backlight_power(struct asus_wmi *asus)
+{
+ 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 < 0)
+ return power;
+
+ if (bd->props.power != power) {
+ if (power != FB_BLANK_UNBLANK) {
+ /* Only brightness > 0 can power it back
on
*/
+ ctrl_param = max(1, asus->driver-
screenpad_brightness);
+ 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 == 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);
+ }
+
+ /* Ensure brightness is stored to turn back on with */
+ asus->driver->screenpad_brightness = bd->props.brightness;
+
+ 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 err, power;
+ int brightness = 0;
+
+ power = read_screenpad_backlight_power(asus);
+ if (power < 0)
+ return power;
+
+ if (power != FB_BLANK_POWERDOWN) {
+ 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 < 60)
+ brightness = 60;
If settings below 60 are no good, then the correct way to handle
this is to limit the range to 0 - (255-60) and add / substract
60 when setting / getting the brightness.
E.g. do something like this:
#define SCREENPAD_MIN_BRIGHTNESS 60
#define SCREENPAD_MAX_BRIGHTNESS 255
props.max_brightness = SCREENPAD_MAX_BRIGHTNESS -
SCREENPAD_MIN_BRIGHTNESS;
And in update_screenpad_bl_status() do:
ctrl_param = bd->props.brightness +
SCREENPAD_MIN_BRIGHTNESS;
And when reading the brightness substract SCREENPAD_MIN_BRIGHTNESS,
clamping to a minimum value of 0.
This avoids a dead-zone in the brightness range between 0-60 .
Hi Hans, I think this is the wrong thing to do.
The initial point of that first `brightness = 60;` is only to set
it to
an acceptable brightness on boot. We don't want to prevent the user
from going below that brightness at all - this is just to ensure the
screen is visible on boot if the brightness was under that value,
and
it is usually only under that value if it was set to off before
shutdown/reboot.
It's not to try and put the range between 60-255, it's just to make
the
screen visible on boot if it was off previously. The folks who have
tested this have found that this is the desired behaviour they
expect.
I see.
So if I understand things correctly then 60 is a good default,
but the screen can go darker and still be usable.
But 1 leads to an unusable screen, so we still need
a SCREENPAD_MIN_BRIGHTNESS to avoid the screen being able
to go so dark that it is no longer usable and then still
move the range a bit, but just not by 60, but by some
other number (something in the 10-30 range I guess?)
Combined with adding a:
#define SCREENPAD_DEFAULT_BRIGHTNESS 60
And at boot when the read back brightness <
SCREENPAD_MIN_BRIGHTNESS set it to SCREENPAD_DEFAULT_BRIGHTNESS.
We really want to avoid users to be able to set an unusable
low brightness level. As mentioned in the new panel brightness
API proposal:
https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@xxxxxxxxxx/
"3. The meaning of 0 is not clearly defined, it can be either off,
or minimum brightness at which the display is still readable
(in a low light environment)"
and the plan going forward is to:
"Unlike the /sys/class/backlight/foo/brightness this brightness
property
has a clear definition for the value 0. The kernel must ensure that 0
means minimum brightness (so 0 should _never_ turn the backlight off).
If necessary the kernel must enforce a minimum value by adding
an offset to the value seen in the property to ensure this behavior."
So I really want to see this new backlight driver implement the
new planned behavior for 0 from the start, with 0 meaning minimum
*usable* brightness.
Hi Hans, yeah okay that makes sense. I'll get on to it.
Cheers,
Luke.