Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

[Luke Jones <luke@ljones.dev>]

On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck 
<linux@roeck-us.net> wrote:
> On 5/5/23 16:43, Luke Jones wrote:
>> 
>> 
>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen 
>> <ilpo.jarvinen@linux.intel.com> wrote:
>>> On Fri, 5 May 2023, 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>
>>>>  ---
>>>>   .../ABI/testing/sysfs-platform-asus-wmi       |   2 +-
>>>>   drivers/platform/x86/asus-wmi.c               | 132 
>>>> ++++++++++++++++++
>>>>   drivers/platform/x86/asus-wmi.h               |   1 +
>>>>   include/linux/platform_data/x86/asus-wmi.h    |   4 +
>>>>   4 files changed, 138 insertions(+), 1 deletion(-)
>>>> 
>>>>  diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi 
>>>> 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 
>>>> ghosting:
>>>>               * 0 - Disable,
>>>>  -            * 1 - Enable
>>>>  +            * 1 - Enable
>>>>  \ No newline at end of file
>>> 
>>> Spurious change?
>> 
>> Indeed it is. Not sure how that occurred.
>> 
>>> 
>>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>>> 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 {
>>>> 
>>>>       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;
>>>>  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>       return 0;
>>>>   }
>>>> 
>>>>  +/* Screenpad backlight */
>>>>  +
>>>>  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>>>  +{
>>>>  +    int ret = asus_wmi_get_devstate_simple(asus, 
>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>> 
>>> Please move this to own line because now you have the extra newline
>>> in between the call and error handling.
>> 
>> I don't understand what you mean sorry. Remove the new line or:
>> 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 == -ENODEV)
>>>>  +        return err;
>>> 
>>> Just return 0. Or is there perhaps something wrong/missing here?
>> 
>> I thought the correct thing was to return any possible error state 
>> (here, anything less than 0 would be an error, right?)
>> 
> 
> Well, yes, but you are not returning an error. You are returning '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.
> 
> Guenter
> 

Oh! Right I see it now, I'm sorry, I just kept skipping over it somehow.

So I should change to:
	power = read_screenpad_backlight_power(asus);
	if (power < 0)
		return power;

Is that acceptable?