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

>