Re: [PATCH v2 08/20] alienware-wmi: Add WMI Drivers

[Armin Wolf <W_Armin@gmx.de>]

Am 29.12.24 um 20:44 schrieb Kurt Borja:

> Add WMI drivers for LEGACY and WMAX devices.
>
> This involves moving platform driver and device registration to a helper
> function, which is now called from the driver's preferred WMI device
> driver probe. However this is only done if !quirks->thermal because
> newer WMAX interface doesn't support any of the features exposed by this
> device.
>
> Only one driver is registered on module initialization to prevent
> registering duplicate platform driver and device.
>
> Additionally, create_thermal_profile() now takes wmi_device * instead of
> platform_device *.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/platform/x86/dell/alienware-wmi.c | 195 +++++++++++++++++-----
>   1 file changed, 155 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 88d4046ed45f..87a7997579c9 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -15,6 +15,7 @@
>   #include <linux/platform_profile.h>
>   #include <linux/dmi.h>
>   #include <linux/leds.h>
> +#include <linux/wmi.h>
>
>   #define LEGACY_CONTROL_GUID		"A90597CE-A997-11DA-B012-B622A1EF5492"
>   #define LEGACY_POWER_CONTROL_GUID	"A80593CE-A997-11DA-B012-B622A1EF5492"
> @@ -39,8 +40,6 @@
>   MODULE_AUTHOR("Mario Limonciello <mario.limonciello@outlook.com>");
>   MODULE_DESCRIPTION("Alienware special feature control");
>   MODULE_LICENSE("GPL");
> -MODULE_ALIAS("wmi:" LEGACY_CONTROL_GUID);
> -MODULE_ALIAS("wmi:" WMAX_CONTROL_GUID);
>
>   static bool force_platform_profile;
>   module_param_unsafe(force_platform_profile, bool, 0);
> @@ -413,7 +412,10 @@ struct alienfx_priv {
>   	u8 lighting_control_state;
>   };
>
> -static struct platform_device *platform_device;
> +struct alienfx_platdata {
> +	struct wmi_device *wdev;
> +};
> +
>   static struct platform_profile_handler pp_handler;
>   static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
>
> @@ -1072,7 +1074,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
>   	return wmax_thermal_control(supported_thermal_profiles[profile]);
>   }
>
> -static int create_thermal_profile(struct platform_device *platform_device)
> +static int create_thermal_profile(struct wmi_device *wdev)
>   {
>   	enum platform_profile_option profile;
>   	enum wmax_thermal_mode mode;
> @@ -1121,7 +1123,7 @@ static int create_thermal_profile(struct platform_device *platform_device)
>   	pp_handler.profile_get = thermal_profile_get;
>   	pp_handler.profile_set = thermal_profile_set;
>   	pp_handler.name = "alienware-wmi";
> -	pp_handler.dev = &platform_device->dev;
> +	pp_handler.dev = &wdev->dev;
>
>   	return platform_profile_register(&pp_handler);
>   }
> @@ -1164,6 +1166,10 @@ static int alienfx_probe(struct platform_device *pdev)
>
>   static const struct attribute_group *alienfx_groups[] = {
>   	&zone_attribute_group,
> +	NULL
> +};
> +
> +static const struct attribute_group *wmax_alienfx_groups[] = {
>   	&hdmi_attribute_group,
>   	&amplifier_attribute_group,
>   	&deepsleep_attribute_group,
> @@ -1179,19 +1185,143 @@ static struct platform_driver platform_driver = {
>   	.probe = alienfx_probe,
>   };
>
> -static int __init alienware_wmi_init(void)
> +static int alienware_alienfx_setup(struct alienfx_platdata *pdata)
>   {
> +	struct platform_device *pdev;
>   	int ret;
>
> -	if (wmi_has_guid(LEGACY_CONTROL_GUID))
> -		interface = LEGACY;
> -	else if (wmi_has_guid(WMAX_CONTROL_GUID))
> -		interface = WMAX;
> -	else {
> -		pr_warn("alienware-wmi: No known WMI GUID found\n");
> -		return -ENODEV;
> +	ret = platform_driver_register(&platform_driver);
> +	if (ret < 0)
> +		return ret;
> +
> +	pdev = platform_device_register_data(NULL, "alienware-wmi",
> +					     PLATFORM_DEVID_NONE, pdata,
> +					     sizeof(*pdata));
> +
> +	if (IS_ERR(pdev)) {
> +		platform_driver_unregister(&platform_driver);
> +		return PTR_ERR(pdev);
> +	}
> +
> +	dev_set_drvdata(&pdata->wdev->dev, pdev);
> +
> +	return 0;
> +}
> +
> +static void alienware_alienfx_exit(struct wmi_device *wdev)
> +{
> +	struct platform_device *pdev = dev_get_drvdata(&wdev->dev);
> +
> +	platform_device_unregister(pdev);
> +	platform_driver_unregister(&platform_driver);
> +}
> +
> +/*
> + * Legacy WMI driver
> + */
> +static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct alienfx_platdata pdata = {
> +		.wdev = wdev,
> +	};
> +
> +	return alienware_alienfx_setup(&pdata);
> +}
> +
> +static void legacy_wmi_remove(struct wmi_device *wdev)
> +{
> +	alienware_alienfx_exit(wdev);
> +}
> +
> +static struct wmi_device_id alienware_legacy_device_id_table[] = {
> +	{ LEGACY_CONTROL_GUID, NULL },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(wmi, alienware_legacy_device_id_table);
> +
> +static struct wmi_driver alienware_legacy_wmi_driver = {
> +	.driver = {
> +		.name = "alienware-wmi-alienfx",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.id_table = alienware_legacy_device_id_table,
> +	.probe = legacy_wmi_probe,
> +	.remove = legacy_wmi_remove,
> +};
> +
> +static int __init alienware_legacy_wmi_init(void)
> +{
> +	return wmi_driver_register(&alienware_legacy_wmi_driver);
> +}
> +
> +static void __exit alienware_legacy_wmi_exit(void)
> +{
> +	wmi_driver_unregister(&alienware_legacy_wmi_driver);
> +}
> +
> +/*
> + * WMAX WMI driver
> + */
> +static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct alienfx_platdata pdata = {
> +		.wdev = wdev,
> +	};
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	if (quirks->thermal) {
> +		ret = create_thermal_profile(wdev);
> +	} else {
> +		ret = alienware_alienfx_setup(&pdata);
> +		if (ret < 0)
> +			return ret;
> +
> +		pdev = dev_get_drvdata(&wdev->dev);
> +		ret = device_add_groups(&pdev->dev, wmax_alienfx_groups);

If device_add_groups() fails you need to call alienware_alienfx_exit().

>   	}
>
> +	return ret;
> +}
> +
> +static void wmax_wmi_remove(struct wmi_device *wdev)
> +{
> +	if (quirks->thermal)
> +		remove_thermal_profile();
> +	else
> +		alienware_alienfx_exit(wdev);

Missing call to device_remove_groups().

> +}
> +
> +static struct wmi_device_id alienware_wmax_device_id_table[] = {
> +	{ WMAX_CONTROL_GUID, NULL },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(wmi, alienware_wmax_device_id_table);
> +
> +static struct wmi_driver alienware_wmax_wmi_driver = {
> +	.driver = {
> +		.name = "alienware-wmi-wmax",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.id_table = alienware_wmax_device_id_table,
> +	.probe = wmax_wmi_probe,
> +	.remove = wmax_wmi_remove,
> +};
> +
> +static int __init alienware_wmax_wmi_init(void)
> +{
> +	return wmi_driver_register(&alienware_wmax_wmi_driver);
> +}
> +
> +static void __exit alienware_wmax_wmi_exit(void)
> +{
> +	wmi_driver_unregister(&alienware_wmax_wmi_driver);
> +}
> +
> +static int __init alienware_wmi_init(void)
> +{
> +	int ret;
> +
>   	dmi_check_system(alienware_quirks);
>   	if (quirks == NULL)
>   		quirks = &quirk_unknown;
> @@ -1206,43 +1336,28 @@ static int __init alienware_wmi_init(void)
>   			pr_warn("force_gmode requires platform profile support\n");
>   	}
>
> -	ret = platform_driver_register(&platform_driver);
> -	if (ret)
> -		goto fail_platform_driver;
> -	platform_device = platform_device_alloc("alienware-wmi", PLATFORM_DEVID_NONE);
> -	if (!platform_device) {
> -		ret = -ENOMEM;
> -		goto fail_platform_device1;
> +	if (wmi_has_guid(WMAX_CONTROL_GUID)) {
> +		interface = WMAX;
> +		ret = alienware_wmax_wmi_init();
> +	} else {
> +		interface = LEGACY;
> +		ret = alienware_legacy_wmi_init();
>   	}
> -	ret = platform_device_add(platform_device);
> -	if (ret)
> -		goto fail_platform_device2;
>
> -	if (quirks->thermal) {
> -		ret = create_thermal_profile(platform_device);
> -		if (ret)
> -			goto fail_prep_thermal_profile;
> -	}
> +	if (ret < 0)
> +		return ret;
>
>   	return 0;

Please just return ret here.

Thanks,
Armin Wolf

> -
> -fail_prep_thermal_profile:
> -	platform_device_del(platform_device);
> -fail_platform_device2:
> -	platform_device_put(platform_device);
> -fail_platform_device1:
> -	platform_driver_unregister(&platform_driver);
> -fail_platform_driver:
> -	return ret;
>   }
>
>   module_init(alienware_wmi_init);
>
>   static void __exit alienware_wmi_exit(void)
>   {
> -	remove_thermal_profile();
> -	platform_device_unregister(platform_device);
> -	platform_driver_unregister(&platform_driver);
> +	if (interface == WMAX)
> +		alienware_wmax_wmi_exit();
> +	else
> +		alienware_legacy_wmi_exit();
>   }
>
>   module_exit(alienware_wmi_exit);