diff mbox series

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

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

Commit Message

Luke Jones May 5, 2023, 4:30 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>
---
 .../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(-)

Comments

Ilpo Järvinen May 5, 2023, 1:08 p.m. UTC | #1 
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?

> 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.

> +
> +	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?

> +	else if (power < 0)
> +		return power;
> +
> +	if (bd->props.power != power) {
> +		if (power != FB_BLANK_UNBLANK) {
> +			/* Only brightness can power it back on */

Only brightness > 0 can power the screen back on

> +			ctrl_param = asus->driver->screenpad_brightness;

max(1, asus->driver->screenpad_brightness);

Don't forget to add the #include for it.

> +			/* Min 1 or the screen won't turn on */
> +			if (ctrl_param == 0)
> +				ctrl_param = 1;

Drop this.

> +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
> +							ctrl_param, NULL);

Align param.

> +		} else {
> +			/* Ensure brightness is stored to turn back on with */
> +			asus->driver->screenpad_brightness = bd->props.brightness;
> +			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);

Why not store the brightness if powered off?

> +	}
> +
> +	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 power, brightness;
> +
> +	power = read_screenpad_backlight_power(asus);
> +	if (power == -ENODEV)
> +		power = FB_BLANK_UNBLANK;
> +	else if (power < 0)
> +		return power;
> +
> +	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;
> +
> +	brightness = read_screenpad_brightness(bd);
> +	if (brightness < 0)
> +		return brightness;
> +	/*
> +	 * Counter an odd behaviour where default is set to < 13 if it was 0 on boot.
> +	 * 60 is subjective, but accepted as a good compromise to retain visibility.
> +	 */
> +	else if (brightness < 60)

Since the other branch returns, else is unnecessary.
Luke Jones May 5, 2023, 11:43 p.m. UTC | #2 
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?)

> 
>>  +	else if (power < 0)
>>  +		return power;
>>  +
>>  +	if (bd->props.power != power) {
>>  +		if (power != FB_BLANK_UNBLANK) {
>>  +			/* Only brightness can power it back on */
> 
> Only brightness > 0 can power the screen back on
> 
>>  +			ctrl_param = asus->driver->screenpad_brightness;
> 
> max(1, asus->driver->screenpad_brightness);
> 
> Don't forget to add the #include for it.

Oh, that's handy! Thank you.

> 
>>  +			/* Min 1 or the screen won't turn on */
>>  +			if (ctrl_param == 0)
>>  +				ctrl_param = 1;
> 
> Drop this.

Thanks to minmax.

> 
>>  +			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
>>  +							ctrl_param, NULL);
> 
> Align param.

Done.

> 
>>  +		} else {
>>  +			/* Ensure brightness is stored to turn back on with */
>>  +			asus->driver->screenpad_brightness = bd->props.brightness;
>>  +			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);
> 
> Why not store the brightness if powered off?

That's me being literal and short sighted. I've now moved:
```
/* Ensure brightness is stored to turn back on with */
asus->driver->screenpad_brightness = bd->props.brightness;
```
to below the conditional blocks.

> 
>>  +	}
>>  +
>>  +	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 power, brightness;
>>  +
>>  +	power = read_screenpad_backlight_power(asus);
>>  +	if (power == -ENODEV)
>>  +		power = FB_BLANK_UNBLANK;
>>  +	else if (power < 0)
>>  +		return power;
>>  +
>>  +	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;
>>  +
>>  +	brightness = read_screenpad_brightness(bd);
>>  +	if (brightness < 0)
>>  +		return brightness;
>>  +	/*
>>  +	 * Counter an odd behaviour where default is set to < 13 if it 
>> was 0 on boot.
>>  +	 * 60 is subjective, but accepted as a good compromise to retain 
>> visibility.
>>  +	 */
>>  +	else if (brightness < 60)
> 
> Since the other branch returns, else is unnecessary.

Good catch, thank you.

I'll submit V3 after we clarify the two points above that I'm confused 
by :)

Thank you for taking the time to review.

>
Guenter Roeck May 6, 2023, 1:30 a.m. UTC | #3 
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
Luke Jones May 6, 2023, 3:48 a.m. UTC | #4 
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?
Guenter Roeck May 6, 2023, 4:43 a.m. UTC | #5 
On 5/5/23 20:48, Luke Jones wrote:
> 
> 
> 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?
> 

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 error,
return it".
Changing the code as suggested above changes the semantics to
"If read_screenpad_backlight_power() returns an error, always return it".

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 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.

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.

Guenter
Luke Jones May 6, 2023, 8:09 a.m. UTC | #6 
On Fri, May 5 2023 at 21:43:44 -0700, Guenter Roeck 
<linux@roeck-us.net> wrote:
> On 5/5/23 20:48, Luke Jones wrote:
>> 
>> 
>> 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?
>> 
> 
> 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 
> error,
> return it".
> Changing the code as suggested above changes the semantics to
> "If read_screenpad_backlight_power() returns an error, always return 
> it".
> 
> 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 
> 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.
> 
> 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.
> 
> Guenter
> 


Thanks mate. I was working on this between my workload and getting a 
few users to test. My first priority was to get the thing working for 
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 
brightness WMI method exists, then it and the other functions should 
return all errors if the power WMI method fails at all since the device 
functionality is compromised. If there is a hardware change in this 
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 
existing backlight code was doing, I've now added a `goto 
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 
fair few patches in these days but they've been mostly simple things 
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.
Guenter Roeck May 6, 2023, 1:24 p.m. UTC | #7 
On 5/6/23 01:09, Luke Jones wrote:
> 
> 
> On Fri, May 5 2023 at 21:43:44 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 5/5/23 20:48, Luke Jones wrote:
>>>
>>>
>>> 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?
>>>
>>
>> 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 error,
>> return it".
>> Changing the code as suggested above changes the semantics to
>> "If read_screenpad_backlight_power() returns an error, always return it".
>>
>> 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 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.
>>
>> 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.
>>
>> Guenter
>>
> 
> 
> Thanks mate. I was working on this between my workload and getting a few users to test. My first priority was to get the thing working for 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 brightness WMI method exists, then it and the other functions should return all errors if the power WMI method fails at all since the device functionality is compromised. If there is a hardware change in this aspect we could revisit it then.
> 
That would suggest that asus_screenpad_init() should also abort on all errors
and not assign a power of 'FB_BLANK_UNBLANK' if it is unreadable.

>  > 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 existing backlight code was doing, I've now added a `goto 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 fair few patches in these days but they've been mostly simple things 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.
> 

Personally I'd first try to use devm_backlight_device_register().
As for error handling, the code would normally be something like

fail_screenpad:
	backlight_device_unregister(asus->screenpad_backlight_device);

or better

fail_screenpad:
	backlight_device_unregister(bd);

I don't know why you would want to check if asus->screenpad_backlight
is NULL because the code would never be called with it being NULL.

Note that I don't see why
	asus->screenpad_backlight_device = NULL;
in asus_screenpad_exit() would be necessary.

Guenter
Ilpo Järvinen May 8, 2023, 8:21 a.m. UTC | #8 
On Sat, 6 May 2023, 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>
> > >  ---

> > >  @@ -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);

Don't do:

int func()
{
	int ret = func();

	if (ret < 0)
		return ret;
	...
}

But do:

int func()
{
	int ret;

	ret = func();
	if (ret < 0)
		return ret;
	...
}

This keeps the error handling next to the actual call following the usual 
error handling pattern (natural logic grouping). The added clarity is well 
worth the one extra line required.

> > >  +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?)

I think this was covered already but here what I meant, use either:
	return 0;
because err is always 0 at that point, or:
	return power;
Depending on which is correct, I wasn't sure because you had this after 
it:

> > >  +	else if (power < 0)
> > >  +		return power;

...So I thought you might really intentionally wanted to return 0 there.
diff mbox series

Patch

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
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);
+
+	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;
+	else if (power < 0)
+		return power;
+
+	if (bd->props.power != power) {
+		if (power != FB_BLANK_UNBLANK) {
+			/* Only brightness can power it back on */
+			ctrl_param = asus->driver->screenpad_brightness;
+			/* Min 1 or the screen won't turn on */
+			if (ctrl_param == 0)
+				ctrl_param = 1;
+			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
+							ctrl_param, NULL);
+		} else {
+			/* Ensure brightness is stored to turn back on with */
+			asus->driver->screenpad_brightness = bd->props.brightness;
+			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);
+	}
+
+	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 power, brightness;
+
+	power = read_screenpad_backlight_power(asus);
+	if (power == -ENODEV)
+		power = FB_BLANK_UNBLANK;
+	else if (power < 0)
+		return power;
+
+	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;
+
+	brightness = read_screenpad_brightness(bd);
+	if (brightness < 0)
+		return brightness;
+	/*
+	 * Counter an odd behaviour where default is set to < 13 if it was 0 on boot.
+	 * 60 is subjective, but accepted as a good compromise to retain visibility.
+	 */
+	else if (brightness < 60)
+		brightness = 60;
+
+	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)
@@ -3823,6 +3947,13 @@  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)) {
+		pr_warn("Begin asus_screenpad_init");
+		err = asus_screenpad_init(asus);
+		if (err && err != -ENODEV)
+			goto fail_backlight;
+	}
+
 	if (asus_wmi_has_fnlock_key(asus)) {
 		asus->fnlock_locked = fnlock_default;
 		asus_wmi_fnlock_update(asus);
@@ -3844,6 +3975,7 @@  static int asus_wmi_add(struct platform_device *pdev)
 
 fail_wmi_handler:
 	asus_wmi_backlight_exit(asus);
+	asus_screenpad_exit(asus);
 fail_backlight:
 	asus_wmi_rfkill_exit(asus);
 fail_rfkill:
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 28234dc9fa6a..a2d94adb5c80 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