| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 06 May 2023 11:43:41 +1200
From: Luke Jones <luke@...nes.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
acpi4asus-user@...ts.sourceforge.net, hdegoede@...hat.com,
corentin.chary@...il.com, markgross@...nel.org, jdelvare@...e.com,
linux@...ck-us.net
Subject: Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS
screenpad
On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> 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@...nes.dev>
>> ---
>> .../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@...nes.dev>
>> 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.
>
Powered by blists - more mailing lists