Message-ID: <6a99fe08-f1eb-4101-99bf-db5fa7364680@gmx.de>
Date: Sun, 29 Dec 2024 01:26:36 +0100
From: Armin Wolf <W_Armin@....de>
To: Kurt Borja <kuurtb@...il.com>
Cc: platform-driver-x86@...r.kernel.org, ilpo.jarvinen@...ux.intel.com,
 mario.limonciello@....com, hdegoede@...hat.com,
 linux-kernel@...r.kernel.org, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH 08/20] alienware-wmi: Add WMI Drivers

Am 27.12.24 um 05:44 schrieb Kurt Borja:

> On Fri, Dec 27, 2024 at 04:21:14AM +0100, Armin Wolf wrote:
>> Am 21.12.24 um 06:59 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.
>>>
>>> Aditionally, create_thermal_profile() now takes wmi_device * instead of
>>> platform_device *.
>> Since the HDMI, Amplifier and deepslp groups depend on the WMAX interface i would
>> register them inside wmax_wmi_probe() using device_add_groups() (you might need to
>> rework alienware_alienfx_setup() for that).
>>
>> In the end the only code shared between both drivers should be related to the LED handling.
>> Everything else AFAIK depends on the WMAX interface and should therefore be inside the
>> WMAX driver.
> Unfortunately this breaks old user-space interface, as the path to this
> attributes would change :(. This is why I can't even set a parent dev
> for the platform_device.
>
> I only know of one project which interefaces with this driver [1], and
> it does use the /sys/devices/platform/alienware-wmi path. Maybe I could
> send a pull req, but it would still break old unknown projects and
> scripts.
>
> [1] https://crates.io/crates/alienware

I meant that you use device_add_groups() on the platform device, not on the WMI device itself.
Since alienware_alienfx_setup() stores a pointer to the platform device using dev_set_drvdata() the WMI driver
can use dev_get_drvdata() to access the platform device.

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
>>
>>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>>> ---
>>>    drivers/platform/x86/dell/alienware-wmi.c | 186 +++++++++++++++++-----
>>>    1 file changed, 146 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
>>> index 4b68d64bd742..f2f6842e27e6 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@...look.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);
>>> @@ -412,7 +411,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];
>>>
>>> @@ -1048,7 +1050,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;
>>> @@ -1097,7 +1099,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);
>>>    }
>>> @@ -1153,19 +1155,138 @@ 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;
>>> +
>>> +	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,
>>> +	};
>>> +	int ret = 0;
>>> +
>>> +	if (quirks->thermal)
>>> +		ret = create_thermal_profile(wdev);
>>> +	else
>>> +		ret = alienware_alienfx_setup(&pdata);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void wmax_wmi_remove(struct wmi_device *wdev)
>>> +{
>>> +	if (quirks->thermal)
>>> +		remove_thermal_profile();
>>> +	else
>>> +		alienware_alienfx_exit(wdev);
>>> +}
>>> +
>>> +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;
>>> @@ -1180,43 +1301,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;
>>> -
>>> -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);

Powered by blists - more mailing lists