Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i5ec1447f:Fastmail
Date:   Wed, 12 Jul 2023 10:21:33 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for ASUS
 screenpad
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     corentin.chary@gmail.com, acpi4asus-user@lists.sourceforge.net,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hwmon@vger.kernel.org, markgross@kernel.org,
        jdelvare@suse.com, linux@roeck-us.net
Message-Id: <XRKNXR.QG8FQ0SYV74U2@ljones.dev>
In-Reply-To: <350cfb54-f435-4482-5a40-18d4358bf747@redhat.com>
References: <20230630041743.911303-1-luke@ljones.dev>
        <20230630041743.911303-2-luke@ljones.dev>
        <974093b4-5dac-dc29-8f86-304eb5c6c19b@redhat.com>
        <39e6044b960c1099ecc34d76f78aad12496b23e2.camel@ljones.dev>
        <350cfb54-f435-4482-5a40-18d4358bf747@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk


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.