diff mbox series

[v4,1/1] platform/x86: asus-wmi: add support for ASUS screenpad

Message ID 20230630041743.911303-2-luke@ljones.dev
State New
Headers
Series asus-wmi: add support for ASUS screenpad |

Commit Message

Luke Jones June 30, 2023, 4:17 a.m. UTC 
  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>
---
 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(+)

Comments

Hans de Goede July 4, 2023, 11:16 a.m. UTC | #1 
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 .

Regards,

Hans






> +
> +	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;
> +	asus->driver->screenpad_brightness = brightness;
> +	bd->props.brightness = brightness;
> +	bd->props.power = power;
> +	backlight_update_status(bd);
> +
> +	return 0;
> +}
> +
> +static void asus_screenpad_exit(struct asus_wmi *asus)
> +{
> +	backlight_device_unregister(asus->screenpad_backlight_device);
> +
> +	asus->screenpad_backlight_device = NULL;
> +}
> +
>  /* Fn-lock ********************************************************************/
>  
>  static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
> @@ -4504,6 +4623,12 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	} else if (asus->driver->quirks->wmi_backlight_set_devstate)
>  		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
>  
> +	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
> +		err = asus_screenpad_init(asus);
> +		if (err && err != -ENODEV)
> +			goto fail_screenpad;
> +	}
> +
>  	if (asus_wmi_has_fnlock_key(asus)) {
>  		asus->fnlock_locked = fnlock_default;
>  		asus_wmi_fnlock_update(asus);
> @@ -4527,6 +4652,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	asus_wmi_backlight_exit(asus);
>  fail_backlight:
>  	asus_wmi_rfkill_exit(asus);
> +fail_screenpad:
> +	asus_screenpad_exit(asus);
>  fail_rfkill:
>  	asus_wmi_led_exit(asus);
>  fail_leds:
> @@ -4553,6 +4680,7 @@ static int asus_wmi_remove(struct platform_device *device)
>  	asus = platform_get_drvdata(device);
>  	wmi_remove_notify_handler(asus->driver->event_guid);
>  	asus_wmi_backlight_exit(asus);
> +	asus_screenpad_exit(asus);
>  	asus_wmi_input_exit(asus);
>  	asus_wmi_led_exit(asus);
>  	asus_wmi_rfkill_exit(asus);
> diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
> index a478ebfd34df..5fbdd0eafa02 100644
> --- a/drivers/platform/x86/asus-wmi.h
> +++ b/drivers/platform/x86/asus-wmi.h
> @@ -57,6 +57,7 @@ struct quirk_entry {
>  struct asus_wmi_driver {
>  	int			brightness;
>  	int			panel_power;
> +	int			screenpad_brightness;
>  	int			wlan_ctrl_by_user;
>  
>  	const char		*name;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index d17ae2eb0f8d..61ba70b32846 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -58,6 +58,10 @@
>  #define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
>  #define ASUS_WMI_DEVID_LIGHT_SENSOR	0x00050022 /* ?? */
>  #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
> +/* This can only be used to disable the screen, not re-enable */
> +#define ASUS_WMI_DEVID_SCREENPAD_POWER	0x00050031
> +/* Writing a brightness re-enables the screen if disabled */
> +#define ASUS_WMI_DEVID_SCREENPAD_LIGHT	0x00050032
>  #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
>  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
>
Luke Jones July 6, 2023, 10:23 p.m. UTC | #2 
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.

Cheers,
Luke.



> 
> > +
> > +       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;
> > +       asus->driver->screenpad_brightness = brightness;
> > +       bd->props.brightness = brightness;
> > +       bd->props.power = power;
> > +       backlight_update_status(bd);
> > +
> > +       return 0;
> > +}
> > +
> > +static void asus_screenpad_exit(struct asus_wmi *asus)
> > +{
> > +       backlight_device_unregister(asus-
> > >screenpad_backlight_device);
> > +
> > +       asus->screenpad_backlight_device = NULL;
> > +}
> > +
> >  /* Fn-lock
> > *******************************************************************
> > */
> >  
> >  static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
> > @@ -4504,6 +4623,12 @@ static int asus_wmi_add(struct
> > platform_device *pdev)
> >         } else if (asus->driver->quirks-
> > >wmi_backlight_set_devstate)
> >                 err =
> > asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
> >  
> > +       if (asus_wmi_dev_is_present(asus,
> > ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
> > +               err = asus_screenpad_init(asus);
> > +               if (err && err != -ENODEV)
> > +                       goto fail_screenpad;
> > +       }
> > +
> >         if (asus_wmi_has_fnlock_key(asus)) {
> >                 asus->fnlock_locked = fnlock_default;
> >                 asus_wmi_fnlock_update(asus);
> > @@ -4527,6 +4652,8 @@ static int asus_wmi_add(struct
> > platform_device *pdev)
> >         asus_wmi_backlight_exit(asus);
> >  fail_backlight:
> >         asus_wmi_rfkill_exit(asus);
> > +fail_screenpad:
> > +       asus_screenpad_exit(asus);
> >  fail_rfkill:
> >         asus_wmi_led_exit(asus);
> >  fail_leds:
> > @@ -4553,6 +4680,7 @@ static int asus_wmi_remove(struct
> > platform_device *device)
> >         asus = platform_get_drvdata(device);
> >         wmi_remove_notify_handler(asus->driver->event_guid);
> >         asus_wmi_backlight_exit(asus);
> > +       asus_screenpad_exit(asus);
> >         asus_wmi_input_exit(asus);
> >         asus_wmi_led_exit(asus);
> >         asus_wmi_rfkill_exit(asus);
> > diff --git a/drivers/platform/x86/asus-wmi.h
> > b/drivers/platform/x86/asus-wmi.h
> > index a478ebfd34df..5fbdd0eafa02 100644
> > --- a/drivers/platform/x86/asus-wmi.h
> > +++ b/drivers/platform/x86/asus-wmi.h
> > @@ -57,6 +57,7 @@ struct quirk_entry {
> >  struct asus_wmi_driver {
> >         int                     brightness;
> >         int                     panel_power;
> > +       int                     screenpad_brightness;
> >         int                     wlan_ctrl_by_user;
> >  
> >         const char              *name;
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h
> > b/include/linux/platform_data/x86/asus-wmi.h
> > index d17ae2eb0f8d..61ba70b32846 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -58,6 +58,10 @@
> >  #define ASUS_WMI_DEVID_KBD_BACKLIGHT   0x00050021
> >  #define ASUS_WMI_DEVID_LIGHT_SENSOR    0x00050022 /* ?? */
> >  #define ASUS_WMI_DEVID_LIGHTBAR                0x00050025
> > +/* This can only be used to disable the screen, not re-enable */
> > +#define ASUS_WMI_DEVID_SCREENPAD_POWER 0x00050031
> > +/* Writing a brightness re-enables the screen if disabled */
> > +#define ASUS_WMI_DEVID_SCREENPAD_LIGHT 0x00050032
> >  #define ASUS_WMI_DEVID_FAN_BOOST_MODE  0x00110018
> >  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
> >  
>
Hans de Goede July 11, 2023, 9:42 a.m. UTC | #3 
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.

Regards,

Hans
Luke Jones July 11, 2023, 10:21 p.m. UTC | #4 
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.
Barnabás Pőcze July 12, 2023, 1:07 a.m. UTC | #5 
Hi


2023. július 11., kedd 11:42 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:

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


Sorry for hijacking this thread, but then how can I turn backlight off?
I quite liked how I was able to turn my laptop display (almost) completely off
with the brightness hotkeys / brightness slider in gnome-shell, and I was quite
annoyed when this was changed in gnome-settings-daemon and forced the minimum
brightness to be 1% of max_brightness.

(https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/merge_requests/17)

Also, "minimum brightness at which the display is still readable" is not really
well-defined, so it can (will) happen that the minimum brightness values don't match,
so it is theoretically possible that I cannot set both my laptop panel and external
monitor to the same desired brightness level. Or am I missing something?


Regards,
Barnabás Pőcze
Hans de Goede July 12, 2023, 6:44 p.m. UTC | #6 
Hi,

On 7/12/23 03:07, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2023. július 11., kedd 11:42 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:
> 
>> [...]
>>>>
>>>> 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.
>>
>> Regards,
>>
>> Hans
> 
> 
> Sorry for hijacking this thread, but then how can I turn backlight off?

Documentation/ABI/stable/sysfs-class-backlight

What:           /sys/class/backlight/<backlight>/bl_power
Date:           April 2005
KernelVersion:  2.6.12
Contact:        Richard Purdie <rpurdie@rpsys.net>
Description:
                Control BACKLIGHT power, values are FB_BLANK_* from fb.h

                 - FB_BLANK_UNBLANK (0)   : power on.
                 - FB_BLANK_POWERDOWN (4) : power off

Although it is better to actually disable video output on the connector,
this leads to much bigger power savings. Under X, this can typically be
done by hitting the lock-screen option, e.g. "Windows-logo-key + L" under
GNOME.

On the console you can do:

echo 1 > /sys/class/graphics/fb0/blank

To really put the panel in low power mode.



> I quite liked how I was able to turn my laptop display (almost) completely off
> with the brightness hotkeys / brightness slider in gnome-shell, and I was quite
> annoyed when this was changed in gnome-settings-daemon and forced the minimum
> brightness to be 1% of max_brightness.

Right, there are 2 problems with this:

1. Using brightness control to disable the backlight is not reliabl. Many
backlight control methods only go to some low setting not to completely off.
This differs from model laptop to model laptop. Also e.g. amdgpu and radeonhd
have always ensured that brightness never completely turns of the backlight.

The plan going forward is to try and consistently have 0 mean minimum
backlight and not backlight off, instead of the current some models 0 = off,
some models 0 is works fine in a not too brightly lit room.

2. Users sometimes turn of the backlight through e.g. the GNOME system menu
slider and then have no way to turn it back on (on devices without (working)
brightness hotkeys (they cannot use the slider since they can no longer see it)
This scenario is a real problem which used to result in quite a few bug reports.

> Also, "minimum brightness at which the display is still readable" is not really
> well-defined

True, as mentioned above the minimum might only be good enough to
e.g. read text in a somewhat low lit room, but typically it at
least leaves things visible enough to allow a user to change
the brightness to a better setting.

What we don't want is brightness settings so low that the backlight is
essentially off (does not even show any light in a fully dark room).

> so it can (will) happen that the minimum brightness values don't match,
> so it is theoretically possible that I cannot set both my laptop panel and external
> monitor to the same desired brightness level. Or am I missing something?

This already is the case, some monitors may not go as low (or high) as you want,
some laptops also already don't go as low as you want.

Expecting to be able to match monitor and laptop panel brightness at both
ends of the brightness range simply is not realistic.

Regards,

Hans
Barnabás Pőcze July 13, 2023, 3:57 p.m. UTC | #7 
Hi


2023. július 12., szerda 20:44 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:

> Hi,
> 
> On 7/12/23 03:07, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2023. július 11., kedd 11:42 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:
> >
> >> [...]
> >>>>
> >>>> 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.
> >>
> >> Regards,
> >>
> >> Hans
> >
> >
> > Sorry for hijacking this thread, but then how can I turn backlight off?
> 
> Documentation/ABI/stable/sysfs-class-backlight
> 
> What:           /sys/class/backlight/<backlight>/bl_power
> Date:           April 2005
> KernelVersion:  2.6.12
> Contact:        Richard Purdie <rpurdie@rpsys.net>
> Description:
>                 Control BACKLIGHT power, values are FB_BLANK_* from fb.h
> 
>                  - FB_BLANK_UNBLANK (0)   : power on.
>                  - FB_BLANK_POWERDOWN (4) : power off
> 
> Although it is better to actually disable video output on the connector,
> this leads to much bigger power savings. Under X, this can typically be
> done by hitting the lock-screen option, e.g. "Windows-logo-key + L" under
> GNOME.

Super+L locks the screen for me on GNOME, which is decidedly *not* what I want
to achieve. I just want to disable the backlight on the laptop panel without
any other changes whatsoever. Writing 4 to /sys/class/backlight/<backlight>/bl_power
does essentially what I want.


> 
> On the console you can do:
> 
> echo 1 > /sys/class/graphics/fb0/blank
> 
> To really put the panel in low power mode.

I never doubted it could still be done. I guess what I wanted to say is "convenience".
Using the brightness slider / hotkeys is very convenient, much more convenient than
messing with getting the right permissions to be able to write to
/sys/class/backlight/<backlight>/bl_power or /sys/class/graphics/fb0/blank.


> 
> 
> 
> > I quite liked how I was able to turn my laptop display (almost) completely off
> > with the brightness hotkeys / brightness slider in gnome-shell, and I was quite
> > annoyed when this was changed in gnome-settings-daemon and forced the minimum
> > brightness to be 1% of max_brightness.
> 
> Right, there are 2 problems with this:
> 
> 1. Using brightness control to disable the backlight is not reliabl. Many
> backlight control methods only go to some low setting not to completely off.
> This differs from model laptop to model laptop. Also e.g. amdgpu and radeonhd
> have always ensured that brightness never completely turns of the backlight.
> 
> The plan going forward is to try and consistently have 0 mean minimum
> backlight and not backlight off, instead of the current some models 0 = off,
> some models 0 is works fine in a not too brightly lit room.

That is good to know, I did not realize that was the case thanks to my intel_backlight
bias...


> 
> 2. Users sometimes turn of the backlight through e.g. the GNOME system menu
> slider and then have no way to turn it back on (on devices without (working)
> brightness hotkeys (they cannot use the slider since they can no longer see it)
> This scenario is a real problem which used to result in quite a few bug reports.

Of course I understand why that change was made in e.g. gnome-settings-daemon.
However, the same way gnome-tweaks has a switch for enabling volume levels greater
than 100%, there could be a switch for enabling the full brightness scale.


> 
> > Also, "minimum brightness at which the display is still readable" is not really
> > well-defined
> 
> True, as mentioned above the minimum might only be good enough to
> e.g. read text in a somewhat low lit room, but typically it at
> least leaves things visible enough to allow a user to change
> the brightness to a better setting.
> 
> What we don't want is brightness settings so low that the backlight is
> essentially off (does not even show any light in a fully dark room).
> 
> > so it can (will) happen that the minimum brightness values don't match,
> > so it is theoretically possible that I cannot set both my laptop panel and external
> > monitor to the same desired brightness level. Or am I missing something?
> 
> This already is the case, some monitors may not go as low (or high) as you want,
> some laptops also already don't go as low as you want.

Fair point, but restricting the range decreases the chances even more.


> 
> Expecting to be able to match monitor and laptop panel brightness at both
> ends of the brightness range simply is not realistic.
> [...]


In conclusion, I appreciate your answer, there were some things I did not know.
I suppose, after all, it might indeed not be a wise idea to shoehorn turning off
backlight into brightness control.


Regards,
Barnabás Pőcze
diff mbox series

Patch

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;
+
+	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;
+	asus->driver->screenpad_brightness = brightness;
+	bd->props.brightness = brightness;
+	bd->props.power = power;
+	backlight_update_status(bd);
+
+	return 0;
+}
+
+static void asus_screenpad_exit(struct asus_wmi *asus)
+{
+	backlight_device_unregister(asus->screenpad_backlight_device);
+
+	asus->screenpad_backlight_device = NULL;
+}
+
 /* Fn-lock ********************************************************************/
 
 static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
@@ -4504,6 +4623,12 @@  static int asus_wmi_add(struct platform_device *pdev)
 	} else if (asus->driver->quirks->wmi_backlight_set_devstate)
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
 
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
+		err = asus_screenpad_init(asus);
+		if (err && err != -ENODEV)
+			goto fail_screenpad;
+	}
+
 	if (asus_wmi_has_fnlock_key(asus)) {
 		asus->fnlock_locked = fnlock_default;
 		asus_wmi_fnlock_update(asus);
@@ -4527,6 +4652,8 @@  static int asus_wmi_add(struct platform_device *pdev)
 	asus_wmi_backlight_exit(asus);
 fail_backlight:
 	asus_wmi_rfkill_exit(asus);
+fail_screenpad:
+	asus_screenpad_exit(asus);
 fail_rfkill:
 	asus_wmi_led_exit(asus);
 fail_leds:
@@ -4553,6 +4680,7 @@  static int asus_wmi_remove(struct platform_device *device)
 	asus = platform_get_drvdata(device);
 	wmi_remove_notify_handler(asus->driver->event_guid);
 	asus_wmi_backlight_exit(asus);
+	asus_screenpad_exit(asus);
 	asus_wmi_input_exit(asus);
 	asus_wmi_led_exit(asus);
 	asus_wmi_rfkill_exit(asus);
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index a478ebfd34df..5fbdd0eafa02 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -57,6 +57,7 @@  struct quirk_entry {
 struct asus_wmi_driver {
 	int			brightness;
 	int			panel_power;
+	int			screenpad_brightness;
 	int			wlan_ctrl_by_user;
 
 	const char		*name;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index d17ae2eb0f8d..61ba70b32846 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -58,6 +58,10 @@ 
 #define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
 #define ASUS_WMI_DEVID_LIGHT_SENSOR	0x00050022 /* ?? */
 #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
+/* This can only be used to disable the screen, not re-enable */
+#define ASUS_WMI_DEVID_SCREENPAD_POWER	0x00050031
+/* Writing a brightness re-enables the screen if disabled */
+#define ASUS_WMI_DEVID_SCREENPAD_LIGHT	0x00050032
 #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
 #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075