Re: [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 4 Dec 2024, 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
> wmidev_* methods, let the WMI drivers present a single interface through
> this "alienfx operations".
> 
> 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 and makes use of
> non-deprecated WMI methods.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/platform/x86/dell/alienware-wmi.c | 110 ++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 34fb59a14bc0..043cde40de9a 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -411,8 +411,16 @@ struct alienfx_priv {
>  	u8 lighting_control_state;
>  };
>  
> +struct alienfx_ops {
> +	int (*upd_led)(struct alienfx_priv *priv, struct wmi_device *wdev,
> +		       u8 location);
> +	int (*upd_brightness)(struct alienfx_priv *priv, struct wmi_device *wdev,
> +			      u8 brightness);
> +};
> +
>  struct alienfx_platdata {
>  	struct wmi_device *wdev;
> +	struct alienfx_ops ops;
>  };
>  
>  static struct platform_profile_handler pp_handler;
> @@ -421,6 +429,32 @@ static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
>  static u8 interface;
>  static struct wmi_driver *preferred_wmi_driver;
>  
> +static acpi_status alienware_wmi_command(struct wmi_device *wdev, u32 method_id,
> +					 void *in_args, size_t in_size, u32 *out_data)
> +{
> +	acpi_status ret;
> +	union acpi_object *obj;

> +	struct acpi_buffer in = { in_size,  in_args };
> +	struct acpi_buffer out = {  ACPI_ALLOCATE_BUFFER, NULL };

Extra whitespace on both lines.

> +
> +	if (out_data) {
> +		ret = wmidev_evaluate_method(wdev, 0, method_id, &in, &out);
> +		if (ACPI_FAILURE(ret))
> +			goto out_free_ptr;
> +
> +		obj = (union acpi_object *) out.pointer;

The usual style for casts is without space but not an end of the world if 
you want to have the space there (in any case, it's not explicitly stated 
in coding style).

> +
> +		if (obj && obj->type == ACPI_TYPE_INTEGER)
> +			*out_data = (u32) obj->integer.value;
> +	} else {
> +		ret = wmidev_evaluate_method(wdev, 0, method_id, &in, NULL);
> +	}
> +
> +out_free_ptr:
> +	kfree(out.pointer);
> +	return ret;
> +}
> +
>  /*
>   * Helpers used for zone control
>   */
> @@ -1178,11 +1212,48 @@ static void alienfx_wmi_exit(struct wmi_device *wdev)
>  /*
>   * Legacy WMI device
>   */
> +static int legacy_wmi_update_led(struct alienfx_priv *priv,
> +				 struct wmi_device *wdev, u8 location)
> +{
> +	acpi_status status;
> +	struct acpi_buffer input;
> +	struct legacy_led_args legacy_args;

Mostly try to abide the reverse xmas tree order (but if there's 
dependency, feel free to violate it where it makes sense).

-- 
 i.

> +	legacy_args.colors = priv->colors[location];
> +	legacy_args.brightness = priv->global_brightness;
> +	legacy_args.state = priv->lighting_control_state;
> +
> +	input.length = sizeof(legacy_args);
> +	input.pointer = &legacy_args;
> +
> +	if (legacy_args.state == LEGACY_RUNNING)
> +		status = alienware_wmi_command(wdev, location + 1, &legacy_args,
> +					       sizeof(legacy_args), NULL);
> +	else
> +		status = wmi_evaluate_method(LEGACY_POWER_CONTROL_GUID, 0,
> +					     location + 1, &input, NULL);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int legacy_wmi_update_brightness(struct alienfx_priv *priv,
> +					struct wmi_device *wdev, u8 brightness)
> +{
> +	return legacy_wmi_update_led(priv, wdev, 0);
> +}
> +
>  static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
>  {
>  	int ret = 0;
>  	struct alienfx_platdata pdata = {
>  		.wdev = wdev,
> +		.ops = {
> +			.upd_led = legacy_wmi_update_led,
> +			.upd_brightness = legacy_wmi_update_brightness,
> +		},
>  	};
>  
>  	if (quirks->num_zones > 0)
> @@ -1219,11 +1290,50 @@ static struct wmi_driver alienware_legacy_wmi_driver = {
>  /*
>   * WMAX WMI device
>   */
> +static int wmax_wmi_update_led(struct alienfx_priv *priv,
> +			       struct wmi_device *wdev, u8 location)
> +{
> +	acpi_status status;
> +	struct wmax_led_args in_args = {
> +		.led_mask = 1 << location,
> +		.colors = priv->colors[location],
> +		.state = priv->lighting_control_state,
> +	};
> +
> +	status = alienware_wmi_command(wdev, WMAX_METHOD_ZONE_CONTROL,
> +				       &in_args, sizeof(in_args), NULL);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int wmax_wmi_update_brightness(struct alienfx_priv *priv,
> +				      struct wmi_device *wdev, u8 brightness)
> +{
> +	acpi_status status;
> +	struct wmax_brightness_args in_args = {
> +		.led_mask = 0xFF,
> +		.percentage = brightness,
> +	};
> +
> +	status = alienware_wmi_command(wdev, WMAX_METHOD_BRIGHTNESS, &in_args,
> +				       sizeof(in_args), NULL);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
>  static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
>  {
>  	int ret = 0;
>  	struct alienfx_platdata pdata = {
>  		.wdev = wdev,
> +		.ops = {
> +			.upd_led = wmax_wmi_update_led,
> +			.upd_brightness = wmax_wmi_update_brightness,
> +		},
>  	};
>  
>  	if (quirks->thermal)
>