Messages in this thread | | | Date | Wed, 12 Jul 2023 10:21:33 +1200 | From | Luke Jones <> | Subject | Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for ASUS screenpad |
| |
On Tue, Jul 11 2023 at 11:42:25 +02:00:00, Hans de Goede <hdegoede@redhat.com> 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@ljones.dev> >>> >>> 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@redhat.com/ > > "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.
| |