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);
>> +}
