Re: [PATCH v10 04/14] platform/x86: alienware-wmi: Refactor LED control methods

["Kurt Borja" <kuurtb@gmail.com>]

On Tue Feb 11, 2025 at 11:33 AM -05, Andy Shevchenko wrote:
> On Fri, Feb 07, 2025 at 10:46:00AM -0500, Kurt Borja wrote:
>> Both WMI devices handled by this module specify a distinct interface for
>> LED control. Previously this module handled this by dynamically adapting
>> arguments passed to wmi_evaluate_method() based on the `interface`
>> global variable.
>> 
>> To avoid the use of global variables, and enable the migration to
>> non-deprecated WMI methods, let the WMI drivers define upd_led and
>> upd_brightness operations, which completely replace
>> alienware_update_led() and wmax_brightness().
>> 
>> Also define alienware_wmi_command(), which serves as a wrapper for
>> wmidev_evaluate_method(). This new method is very similar to
>> alienware_wmax_command() but is WMI device agnostic.
>
> ...
>
>> +static int alienware_wmi_command(struct wmi_device *wdev, u32 method_id,
>> +				 void *in_args, size_t in_size, u32 *out_data)
>> +{
>> +	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
>> +	struct acpi_buffer in = {in_size, in_args};
>> +	acpi_status ret;
>> +
>> +	ret = wmidev_evaluate_method(wdev, 0, method_id, &in, out_data ? &out : NULL);
>> +	if (ACPI_FAILURE(ret))
>> +		return -EIO;
>> +
>> +	union acpi_object *obj __free(kfree) = out.pointer;
>
> Actually we have ACPI_FREE(), but it's not big deal (as of today) to use kfree().
>
>> +	if (out_data) {
>> +		if (obj && obj->type == ACPI_TYPE_INTEGER)
>> +			*out_data = (u32)obj->integer.value;
>
> Unneeded casting.

Ack.

>
>> +		else
>> +			return -ENOMSG;
>> +	}
>> +
>> +	return 0;
>> +}
>
> ...
>
>> +	if (legacy_args.state != LEGACY_RUNNING) {
>
> With inverted conditional and duplicated line it will all look better.

Ack.

>
>> +		legacy_args.state = priv->lighting_control_state;
>> +
>> +		input.length = sizeof(legacy_args);
>> +		input.pointer = &legacy_args;
>> +
>> +		status = wmi_evaluate_method(LEGACY_POWER_CONTROL_GUID, 0,
>> +					     location + 1, &input, NULL);
>> +		if (ACPI_FAILURE(status))
>> +			return -EIO;
>> +
>> +		return 0;
>> +	}
>> +
>> +	return alienware_wmi_command(wdev, location + 1, &legacy_args,
>> +				     sizeof(legacy_args), NULL);
>
> ...
>
>> +static int wmax_wmi_update_brightness(struct alienfx_priv *priv,
>> +				      struct wmi_device *wdev, u8 brightness)
>> +{
>> +	struct wmax_brightness_args in_args = {
>> +		.led_mask = 0xFF,
>
> GENMASK()?

Ack.

>
>> +		.percentage = brightness,
>> +	};
>> +
>> +	return alienware_wmi_command(wdev, WMAX_METHOD_BRIGHTNESS, &in_args,
>> +				     sizeof(in_args), NULL);
>> +}