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.
Regards,
Hans
