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:   Sat, 06 May 2023 20:09:26 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS
 screenpad
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Ilpo =?iso-8859-1?q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        acpi4asus-user@lists.sourceforge.net, hdegoede@redhat.com,
        corentin.chary@gmail.com, markgross@kernel.org, jdelvare@suse.com
Message-Id: <QB98UR.2KNZGZXO8NF12@ljones.dev>
In-Reply-To: <17fb02ff-e2d8-9c0b-3de6-670c82fee997@roeck-us.net>
References: <20230505043013.2622603-1-luke@ljones.dev>
        <20230505043013.2622603-2-luke@ljones.dev>
        <9f77e8fd-38fe-818f-2fee-ca3bf4243576@linux.intel.com>
        <TWL7UR.KE812U8BYMG8@ljones.dev>
        <f1f8ff7a-6f23-e284-b494-7df2f0dce1a4@roeck-us.net>
        <M8X7UR.MPEZYPQ0PU4F1@ljones.dev>
        <17fb02ff-e2d8-9c0b-3de6-670c82fee997@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Fri, May 5 2023 at 21:43:44 -0700, Guenter Roeck=20
<linux@roeck-us.net> wrote:
> On 5/5/23 20:48, Luke Jones wrote:
>>=20
>>=20
>> On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck=20
>> <linux@roeck-us.net> wrote:
>>> On 5/5/23 16:43, Luke Jones wrote:
>>>>=20
>>>>=20
>>>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo J=E4rvinen=20
>>>> <ilpo.jarvinen@linux.intel.com> wrote:
>>>>> On Fri, 5 May 2023, Luke D. Jones wrote:
>>>>>=20
>>>>>>  Add support for the WMI methods used to turn off and adjust the
>>>>>>  brightness of the secondary "screenpad" device found on some=20
>>>>>> high-end
>>>>>>  ASUS laptops like the GX650P series and others.
>>>>>>=20
>>>>>>  These methods are utilised in a new backlight device named:
>>>>>>  - asus_screenpad
>>>>>>=20
>>>>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>>>  ---
>>>>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>>>>   drivers/platform/x86/asus-wmi.c               | 132=20
>>>>>> ++++++++++++++++++
>>>>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>>>>=20
>>>>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi=20
>>>>>> 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@ljones.dev>
>>>>>>   Description:
>>>>>>           Enable an LCD response-time boost to reduce or remove=20
>>>>>> ghosting:
>>>>>>               * 0 - Disable,
>>>>>>  -            * 1 - Enable
>>>>>>  +            * 1 - Enable
>>>>>>  \ No newline at end of file
>>>>>=20
>>>>> Spurious change?
>>>>=20
>>>> Indeed it is. Not sure how that occurred.
>>>>=20
>>>>>=20
>>>>>>  diff --git a/drivers/platform/x86/asus-wmi.c=20
>>>>>> 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 {
>>>>>>=20
>>>>>>       struct input_dev *inputdev;
>>>>>>       struct backlight_device *backlight_device;
>>>>>>  +    struct backlight_device *screenpad_backlight_device;
>>>>>>       struct platform_device *platform_device;
>>>>>>=20
>>>>>>       struct led_classdev wlan_led;
>>>>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>>>       return 0;
>>>>>>   }
>>>>>>=20
>>>>>>  +/* Screenpad backlight */
>>>>>>  +
>>>>>>  +static int read_screenpad_backlight_power(struct asus_wmi=20
>>>>>> *asus)
>>>>>>  +{
>>>>>>  +    int ret =3D asus_wmi_get_devstate_simple(asus,=20
>>>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>>=20
>>>>> Please move this to own line because now you have the extra=20
>>>>> newline
>>>>> in between the call and error handling.
>>>>=20
>>>> I don't understand what you mean sorry. Remove the new line or:
>>>> int ret;
>>>> ret =3D asus_wmi_get_devstate_simple(asus,=20
>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>=20
>>>>>=20
>>>>>>  +
>>>>>>  +    if (ret < 0)
>>>>>>  +        return ret;
>>>>>>  +    /* 1 =3D=3D powered */
>>>>>>  +    return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>>>>  +}
>>>>>>  +
>>>>>>  +static int read_screenpad_brightness(struct backlight_device=20
>>>>>> *bd)
>>>>>>  +{
>>>>>>  +    struct asus_wmi *asus =3D bl_get_data(bd);
>>>>>>  +    u32 retval;
>>>>>>  +    int err;
>>>>>>  +
>>>>>>  +    err =3D read_screenpad_backlight_power(asus);
>>>>>>  +    if (err < 0)
>>>>>>  +        return err;
>>>>>>  +    /* The device brightness can only be read if powered, so=20
>>>>>> return stored */
>>>>>>  +    if (err =3D=3D FB_BLANK_POWERDOWN)
>>>>>>  +        return asus->driver->screenpad_brightness;
>>>>>>  +
>>>>>>  +    err =3D asus_wmi_get_devstate(asus,=20
>>>>>> 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=20
>>>>>> *bd)
>>>>>>  +{
>>>>>>  +    struct asus_wmi *asus =3D bl_get_data(bd);
>>>>>>  +    int power, err =3D 0;
>>>>>>  +    u32 ctrl_param;
>>>>>>  +
>>>>>>  +    power =3D read_screenpad_backlight_power(asus);
>>>>>>  +    if (power =3D=3D -ENODEV)
>>>>>>  +        return err;
>>>>>=20
>>>>> Just return 0. Or is there perhaps something wrong/missing here?
>>>>=20
>>>> I thought the correct thing was to return any possible error state=20
>>>> (here, anything less than 0 would be an error, right?)
>>>>=20
>>>=20
>>> Well, yes, but you are not returning an error. You are returning=20
>>> 'err'
>>> which is 0 at this point. So, at the very least, this code is (very)
>>> misleading since it suggests that it would return some error
>>> (as saved in the 'err' variable) when it doesn't.
>>>=20
>>> Guenter
>>>=20
>>=20
>> Oh! Right I see it now, I'm sorry, I just kept skipping over it=20
>> somehow.
>>=20
>> So I should change to:
>>      power =3D read_screenpad_backlight_power(asus);
>>      if (power < 0)
>>          return power;
>>=20
>> Is that acceptable?
>>=20
>=20
> That depends on what the code is supposed to do. Right now it is
> "If read_screenpad_backlight_power() returns -ENODEV, stop and return
> no error (let the rest of the code continue). If it returns another=20
> error,
> return it".
> Changing the code as suggested above changes the semantics to
> "If read_screenpad_backlight_power() returns an error, always return=20
> it".
>=20
> Looking at the patch, I don't have a definite answer.
> asus_screenpad_init() happily registers the backlight if
> read_screenpad_backlight_power() returns -ENODEV. One could argue that
> the other functions should thus handle that situation gracefully as=20
> well,
> but read_screenpad_brightness() does return -ENODEV in that situation.
> I think you should decide how you want to handle that case and handle
> it consistently across all functions.
>=20
> Either case, there is another problem in asus_screenpad_init():
> If read_screenpad_brightness() fails, the function returns an error,
> but does not unregister the backlight device.
>=20
> Guenter
>=20


Thanks mate. I was working on this between my workload and getting a=20
few users to test. My first priority was to get the thing working for=20
them and as such I didn't put much thought in to errors and consistency.

I think since `asus_screenpad_init()` is not called unless the=20
brightness WMI method exists, then it and the other functions should=20
return all errors if the power WMI method fails at all since the device=20
functionality is compromised. If there is a hardware change in this=20
aspect we could revisit it then.

 > Either case, there is another problem in asus_screenpad_init():
 > If read_screenpad_brightness() fails, the function returns an error,
 > but does not unregister the backlight device.

I wasn't entirely sure how to handle this. Mostly I followed what the=20
existing backlight code was doing, I've now added a `goto=20
fail_screenpad;` with:
fail_screenpad:
	if (asus->screenpad_backlight_device)
		asus_screenpad_exit(asus);

really I'm relying on others to guide me with this - I know I've got a=20
fair few patches in these days but they've been mostly simple things=20
except for TUF RGB, and I've had to learn a bunch of stuff as I go.

I'll await your response before I send in a V3.

Cheers,
Luke.