* [RFC PATCH 01/21] alienware-wmi: Modify parse_rgb() signature
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
@ 2024-12-05 0:38 ` Kurt Borja
2024-12-05 0:38 ` [RFC PATCH 02/21] alienware-wmi: Move Lighting Control State Kurt Borja
` (20 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:38 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
parse_rgb() now takes struct color_platform instead of struct
platform_zone to support upcoming refactor.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 77465ed9b449..b3a73fc43b3c 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -434,7 +434,7 @@ static u8 global_brightness;
/*
* Helpers used for zone control
*/
-static int parse_rgb(const char *buf, struct platform_zone *zone)
+static int parse_rgb(const char *buf, struct color_platform *colors)
{
long unsigned int rgb;
int ret;
@@ -454,7 +454,7 @@ static int parse_rgb(const char *buf, struct platform_zone *zone)
repackager.package = rgb & 0x0f0f0f0f;
pr_debug("alienware-wmi: r: %d g:%d b: %d\n",
repackager.cp.red, repackager.cp.green, repackager.cp.blue);
- zone->colors = repackager.cp;
+ *colors = repackager.cp;
return 0;
}
@@ -538,7 +538,7 @@ static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
pr_err("alienware-wmi: invalid target zone\n");
return 1;
}
- ret = parse_rgb(buf, target_zone);
+ ret = parse_rgb(buf, &target_zone->colors);
if (ret)
return ret;
ret = alienware_update_led(target_zone);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 02/21] alienware-wmi: Move Lighting Control State
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
2024-12-05 0:38 ` [RFC PATCH 01/21] alienware-wmi: Modify parse_rgb() signature Kurt Borja
@ 2024-12-05 0:38 ` Kurt Borja
2024-12-05 0:39 ` [RFC PATCH 03/21] alienware-wmi: Remove unnecessary check at module exit Kurt Borja
` (19 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:38 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Place Lighting Control State logic next to other attributes of the same
sysfs group.
While at it, rename:
store_control_state() -> lighting_control_state_store()
show_control_state() -> lighting_control_state_show()
And use DEVICE_ATTR_RW() instead of DEVICE_ATTR().
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 73 ++++++++++++-----------
1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index b3a73fc43b3c..b1ac0e393180 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -545,6 +545,44 @@ static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
return ret ? ret : count;
}
+/*
+ * Lighting control state device attribute (Global)
+ */
+static ssize_t lighting_control_state_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ if (lighting_control_state == LEGACY_BOOTING)
+ return sysfs_emit(buf, "[booting] running suspend\n");
+ else if (lighting_control_state == LEGACY_SUSPEND)
+ return sysfs_emit(buf, "booting running [suspend]\n");
+ return sysfs_emit(buf, "booting [running] suspend\n");
+}
+
+static ssize_t lighting_control_state_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 val;
+
+ if (strcmp(buf, "booting\n") == 0)
+ val = LEGACY_BOOTING;
+ else if (strcmp(buf, "suspend\n") == 0)
+ val = LEGACY_SUSPEND;
+ else if (interface == LEGACY)
+ val = LEGACY_RUNNING;
+ else
+ val = WMAX_RUNNING;
+
+ lighting_control_state = val;
+ pr_debug("alienware-wmi: updated control state to %d\n",
+ lighting_control_state);
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(lighting_control_state);
+
/*
* LED Brightness (Global)
*/
@@ -589,41 +627,6 @@ static struct led_classdev global_led = {
.name = "alienware::global_brightness",
};
-/*
- * Lighting control state device attribute (Global)
- */
-static ssize_t show_control_state(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- if (lighting_control_state == LEGACY_BOOTING)
- return sysfs_emit(buf, "[booting] running suspend\n");
- else if (lighting_control_state == LEGACY_SUSPEND)
- return sysfs_emit(buf, "booting running [suspend]\n");
- return sysfs_emit(buf, "booting [running] suspend\n");
-}
-
-static ssize_t store_control_state(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- long unsigned int val;
- if (strcmp(buf, "booting\n") == 0)
- val = LEGACY_BOOTING;
- else if (strcmp(buf, "suspend\n") == 0)
- val = LEGACY_SUSPEND;
- else if (interface == LEGACY)
- val = LEGACY_RUNNING;
- else
- val = WMAX_RUNNING;
- lighting_control_state = val;
- pr_debug("alienware-wmi: updated control state to %d\n",
- lighting_control_state);
- return count;
-}
-
-static DEVICE_ATTR(lighting_control_state, 0644, show_control_state,
- store_control_state);
-
static int alienware_zone_init(struct platform_device *dev)
{
u8 zone;
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 03/21] alienware-wmi: Remove unnecessary check at module exit
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
2024-12-05 0:38 ` [RFC PATCH 01/21] alienware-wmi: Modify parse_rgb() signature Kurt Borja
2024-12-05 0:38 ` [RFC PATCH 02/21] alienware-wmi: Move Lighting Control State Kurt Borja
@ 2024-12-05 0:39 ` Kurt Borja
2024-12-05 0:39 ` [RFC PATCH 04/21] alienware-wmi: Improve sysfs groups creation Kurt Borja
` (18 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:39 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Module initialization fails if platform device fails to register so it's
always not NULL at exit.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index b1ac0e393180..115b3aa5637b 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1258,13 +1258,11 @@ module_init(alienware_wmi_init);
static void __exit alienware_wmi_exit(void)
{
- if (platform_device) {
- alienware_zone_exit(platform_device);
- remove_hdmi(platform_device);
- remove_thermal_profile();
- platform_device_unregister(platform_device);
- platform_driver_unregister(&platform_driver);
- }
+ alienware_zone_exit(platform_device);
+ remove_hdmi(platform_device);
+ remove_thermal_profile();
+ platform_device_unregister(platform_device);
+ platform_driver_unregister(&platform_driver);
}
module_exit(alienware_wmi_exit);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 04/21] alienware-wmi: Improve sysfs groups creation
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (2 preceding siblings ...)
2024-12-05 0:39 ` [RFC PATCH 03/21] alienware-wmi: Remove unnecessary check at module exit Kurt Borja
@ 2024-12-05 0:39 ` Kurt Borja
2024-12-05 0:40 ` [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation Kurt Borja
` (17 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:39 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
for each available feature. To accomplish this, helper create/remove
functions were called on module init, but they had the following
problems:
- Create helpers called remove helpers on failure, which in turn tried
to remove the sysfs group that failed to be created
- If group creation failed mid way, previous successfully created groups
were not cleaned up
- Module exit only removed hdmi_mux group
To improve this, drop all helpers and let the platform driver manage these
sysfs groups, while controlling visibility with their respective quirks.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 114 ++++++++--------------
1 file changed, 38 insertions(+), 76 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 115b3aa5637b..78bbb4ef4526 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -417,12 +417,6 @@ static struct platform_zone *zone_data;
static struct platform_profile_handler pp_handler;
static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
-static struct platform_driver platform_driver = {
- .driver = {
- .name = "alienware-wmi",
- }
-};
-
static struct attribute_group zone_attribute_group = {
.name = "rgb_zones",
};
@@ -803,6 +797,12 @@ static DEVICE_ATTR(cable, S_IRUGO, show_hdmi_cable, NULL);
static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
toggle_hdmi_source);
+static bool hdmi_group_visible(struct kobject *kobj)
+{
+ return quirks->hdmi_mux;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
+
static struct attribute *hdmi_attrs[] = {
&dev_attr_cable.attr,
&dev_attr_source.attr,
@@ -811,25 +811,10 @@ static struct attribute *hdmi_attrs[] = {
static const struct attribute_group hdmi_attribute_group = {
.name = "hdmi",
+ .is_visible = SYSFS_GROUP_VISIBLE(hdmi),
.attrs = hdmi_attrs,
};
-static void remove_hdmi(struct platform_device *dev)
-{
- if (quirks->hdmi_mux > 0)
- sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
-}
-
-static int create_hdmi(struct platform_device *dev)
-{
- int ret;
-
- ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
- if (ret)
- remove_hdmi(dev);
- return ret;
-}
-
/*
* Alienware GFX amplifier support
* - Currently supports reading cable status
@@ -858,6 +843,12 @@ static ssize_t show_amplifier_status(struct device *dev,
static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
+static bool amplifier_group_visible(struct kobject *kobj)
+{
+ return quirks->amplifier;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
+
static struct attribute *amplifier_attrs[] = {
&dev_attr_status.attr,
NULL,
@@ -865,25 +856,10 @@ static struct attribute *amplifier_attrs[] = {
static const struct attribute_group amplifier_attribute_group = {
.name = "amplifier",
+ .is_visible = SYSFS_GROUP_VISIBLE(amplifier),
.attrs = amplifier_attrs,
};
-static void remove_amplifier(struct platform_device *dev)
-{
- if (quirks->amplifier > 0)
- sysfs_remove_group(&dev->dev.kobj, &lifier_attribute_group);
-}
-
-static int create_amplifier(struct platform_device *dev)
-{
- int ret;
-
- ret = sysfs_create_group(&dev->dev.kobj, &lifier_attribute_group);
- if (ret)
- remove_amplifier(dev);
- return ret;
-}
-
/*
* Deep Sleep Control support
* - Modifies BIOS setting for deep sleep control allowing extra wakeup events
@@ -936,6 +912,12 @@ static ssize_t toggle_deepsleep(struct device *dev,
static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
+static bool deepsleep_group_visible(struct kobject *kobj)
+{
+ return quirks->deepslp;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
+
static struct attribute *deepsleep_attrs[] = {
&dev_attr_deepsleep.attr,
NULL,
@@ -943,25 +925,10 @@ static struct attribute *deepsleep_attrs[] = {
static const struct attribute_group deepsleep_attribute_group = {
.name = "deepsleep",
+ .is_visible = SYSFS_GROUP_VISIBLE(deepsleep),
.attrs = deepsleep_attrs,
};
-static void remove_deepsleep(struct platform_device *dev)
-{
- if (quirks->deepslp > 0)
- sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
-}
-
-static int create_deepsleep(struct platform_device *dev)
-{
- int ret;
-
- ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
- if (ret)
- remove_deepsleep(dev);
- return ret;
-}
-
/*
* Thermal Profile control
* - Provides thermal profile control through the Platform Profile API
@@ -1169,6 +1136,23 @@ static void remove_thermal_profile(void)
platform_profile_remove();
}
+/*
+ * Platform Driver
+ */
+static const struct attribute_group *alienfx_groups[] = {
+ &hdmi_attribute_group,
+ &lifier_attribute_group,
+ &deepsleep_attribute_group,
+ NULL
+};
+
+static struct platform_driver platform_driver = {
+ .driver = {
+ .name = "alienware-wmi",
+ .dev_groups = alienfx_groups,
+ },
+};
+
static int __init alienware_wmi_init(void)
{
int ret;
@@ -1208,24 +1192,6 @@ static int __init alienware_wmi_init(void)
if (ret)
goto fail_platform_device2;
- if (quirks->hdmi_mux > 0) {
- ret = create_hdmi(platform_device);
- if (ret)
- goto fail_prep_hdmi;
- }
-
- if (quirks->amplifier > 0) {
- ret = create_amplifier(platform_device);
- if (ret)
- goto fail_prep_amplifier;
- }
-
- if (quirks->deepslp > 0) {
- ret = create_deepsleep(platform_device);
- if (ret)
- goto fail_prep_deepsleep;
- }
-
if (quirks->thermal) {
ret = create_thermal_profile();
if (ret)
@@ -1242,9 +1208,6 @@ static int __init alienware_wmi_init(void)
alienware_zone_exit(platform_device);
remove_thermal_profile();
fail_prep_thermal_profile:
-fail_prep_deepsleep:
-fail_prep_amplifier:
-fail_prep_hdmi:
platform_device_del(platform_device);
fail_platform_device2:
platform_device_put(platform_device);
@@ -1259,7 +1222,6 @@ module_init(alienware_wmi_init);
static void __exit alienware_wmi_exit(void)
{
alienware_zone_exit(platform_device);
- remove_hdmi(platform_device);
remove_thermal_profile();
platform_device_unregister(platform_device);
platform_driver_unregister(&platform_driver);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (3 preceding siblings ...)
2024-12-05 0:39 ` [RFC PATCH 04/21] alienware-wmi: Improve sysfs groups creation Kurt Borja
@ 2024-12-05 0:40 ` Kurt Borja
2024-12-05 10:17 ` Ilpo Järvinen
2024-12-05 0:40 ` [RFC PATCH 06/21] alienware-wmi: Add state container and alienfx_probe() Kurt Borja
` (16 subsequent siblings)
21 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:40 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Define zone_attrs statically with the use of helper macros and
initialize the zone_attribute_group with driver's .dev_groups.
This makes match_zone() no longer needed, so drop it.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 137 ++++++++++------------
1 file changed, 60 insertions(+), 77 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 78bbb4ef4526..fa7bbbb07b86 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -378,7 +378,6 @@ struct color_platform {
struct platform_zone {
u8 location;
- struct device_attribute *attr;
struct color_platform colors;
};
@@ -411,16 +410,10 @@ struct wmax_u32_args {
};
static struct platform_device *platform_device;
-static struct device_attribute *zone_dev_attrs;
-static struct attribute **zone_attrs;
static struct platform_zone *zone_data;
static struct platform_profile_handler pp_handler;
static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
-static struct attribute_group zone_attribute_group = {
- .name = "rgb_zones",
-};
-
static u8 interface;
static u8 lighting_control_state;
static u8 global_brightness;
@@ -452,20 +445,6 @@ static int parse_rgb(const char *buf, struct color_platform *colors)
return 0;
}
-static struct platform_zone *match_zone(struct device_attribute *attr)
-{
- u8 zone;
-
- for (zone = 0; zone < quirks->num_zones; zone++) {
- if ((struct device_attribute *)zone_data[zone].attr == attr) {
- pr_debug("alienware-wmi: matched zone location: %d\n",
- zone_data[zone].location);
- return &zone_data[zone];
- }
- }
- return NULL;
-}
-
/*
* Individual RGB zone control
*/
@@ -510,12 +489,10 @@ static int alienware_update_led(struct platform_zone *zone)
}
static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
- char *buf)
+ char *buf, u8 location)
{
struct platform_zone *target_zone;
- target_zone = match_zone(attr);
- if (target_zone == NULL)
- return sprintf(buf, "red: -1, green: -1, blue: -1\n");
+ target_zone = &zone_data[location];
return sprintf(buf, "red: %d, green: %d, blue: %d\n",
target_zone->colors.red,
target_zone->colors.green, target_zone->colors.blue);
@@ -523,15 +500,11 @@ static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
}
static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
+ const char *buf, size_t count, u8 location)
{
struct platform_zone *target_zone;
int ret;
- target_zone = match_zone(attr);
- if (target_zone == NULL) {
- pr_err("alienware-wmi: invalid target zone\n");
- return 1;
- }
+ target_zone = &zone_data[location];
ret = parse_rgb(buf, &target_zone->colors);
if (ret)
return ret;
@@ -539,6 +512,32 @@ static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
return ret ? ret : count;
}
+#define ALIENWARE_ZONE_SHOW_FUNC(_num) \
+ static ssize_t zone0##_num##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+ { \
+ return zone_show(dev, attr, buf, _num); \
+ }
+
+#define ALIENWARE_ZONE_STORE_FUNC(_num) \
+ static ssize_t zone0##_num##_store(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+ { \
+ return zone_set(dev, attr, buf, count, _num); \
+ }
+
+#define ALIENWARE_ZONE_ATTR(_num) \
+ ALIENWARE_ZONE_SHOW_FUNC(_num) \
+ ALIENWARE_ZONE_STORE_FUNC(_num) \
+ static DEVICE_ATTR_RW(zone0##_num)
+
+ALIENWARE_ZONE_ATTR(0);
+ALIENWARE_ZONE_ATTR(1);
+ALIENWARE_ZONE_ATTR(2);
+ALIENWARE_ZONE_ATTR(3);
+
/*
* Lighting control state device attribute (Global)
*/
@@ -577,6 +576,33 @@ static ssize_t lighting_control_state_store(struct device *dev,
static DEVICE_ATTR_RW(lighting_control_state);
+static umode_t zone_attr_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ return n < quirks->num_zones + 1 ? 0644 : 0;
+}
+
+static bool zone_group_visible(struct kobject *kobj)
+{
+ return quirks->num_zones > 0;
+}
+DEFINE_SYSFS_GROUP_VISIBLE(zone);
+
+static struct attribute *zone_attrs[] = {
+ &dev_attr_lighting_control_state.attr,
+ &dev_attr_zone00.attr,
+ &dev_attr_zone01.attr,
+ &dev_attr_zone02.attr,
+ &dev_attr_zone03.attr,
+ NULL
+};
+
+static struct attribute_group zone_attribute_group = {
+ .name = "rgb_zones",
+ .is_visible = SYSFS_GROUP_VISIBLE(zone),
+ .attrs = zone_attrs,
+};
+
/*
* LED Brightness (Global)
*/
@@ -624,7 +650,6 @@ static struct led_classdev global_led = {
static int alienware_zone_init(struct platform_device *dev)
{
u8 zone;
- char *name;
if (interface == WMAX) {
lighting_control_state = WMAX_RUNNING;
@@ -634,65 +659,22 @@ static int alienware_zone_init(struct platform_device *dev)
global_led.max_brightness = 0x0F;
global_brightness = global_led.max_brightness;
- /*
- * - zone_dev_attrs num_zones + 1 is for individual zones and then
- * null terminated
- * - zone_attrs num_zones + 2 is for all attrs in zone_dev_attrs +
- * the lighting control + null terminated
- * - zone_data num_zones is for the distinct zones
- */
- zone_dev_attrs =
- kcalloc(quirks->num_zones + 1, sizeof(struct device_attribute),
- GFP_KERNEL);
- if (!zone_dev_attrs)
- return -ENOMEM;
-
- zone_attrs =
- kcalloc(quirks->num_zones + 2, sizeof(struct attribute *),
- GFP_KERNEL);
- if (!zone_attrs)
- return -ENOMEM;
-
zone_data =
kcalloc(quirks->num_zones, sizeof(struct platform_zone),
GFP_KERNEL);
if (!zone_data)
return -ENOMEM;
- for (zone = 0; zone < quirks->num_zones; zone++) {
- name = kasprintf(GFP_KERNEL, "zone%02hhX", zone);
- if (name == NULL)
- return 1;
- sysfs_attr_init(&zone_dev_attrs[zone].attr);
- zone_dev_attrs[zone].attr.name = name;
- zone_dev_attrs[zone].attr.mode = 0644;
- zone_dev_attrs[zone].show = zone_show;
- zone_dev_attrs[zone].store = zone_set;
+ for (zone = 0; zone < 4; zone++)
zone_data[zone].location = zone;
- zone_attrs[zone] = &zone_dev_attrs[zone].attr;
- zone_data[zone].attr = &zone_dev_attrs[zone];
- }
- zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
- zone_attribute_group.attrs = zone_attrs;
-
- led_classdev_register(&dev->dev, &global_led);
- return sysfs_create_group(&dev->dev.kobj, &zone_attribute_group);
+ return led_classdev_register(&dev->dev, &global_led);
}
static void alienware_zone_exit(struct platform_device *dev)
{
- u8 zone;
-
- sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group);
led_classdev_unregister(&global_led);
- if (zone_dev_attrs) {
- for (zone = 0; zone < quirks->num_zones; zone++)
- kfree(zone_dev_attrs[zone].attr.name);
- }
- kfree(zone_dev_attrs);
kfree(zone_data);
- kfree(zone_attrs);
}
static acpi_status alienware_wmax_command(void *in_args, size_t in_size,
@@ -1140,6 +1122,7 @@ static void remove_thermal_profile(void)
* Platform Driver
*/
static const struct attribute_group *alienfx_groups[] = {
+ &zone_attribute_group,
&hdmi_attribute_group,
&lifier_attribute_group,
&deepsleep_attribute_group,
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation
2024-12-05 0:40 ` [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation Kurt Borja
@ 2024-12-05 10:17 ` Ilpo Järvinen
2024-12-05 12:48 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 10:17 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Wed, 4 Dec 2024, Kurt Borja wrote:
> Define zone_attrs statically with the use of helper macros and
> initialize the zone_attribute_group with driver's .dev_groups.
>
> This makes match_zone() no longer needed, so drop it.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> drivers/platform/x86/dell/alienware-wmi.c | 137 ++++++++++------------
> 1 file changed, 60 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 78bbb4ef4526..fa7bbbb07b86 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -378,7 +378,6 @@ struct color_platform {
>
> struct platform_zone {
> u8 location;
> - struct device_attribute *attr;
> struct color_platform colors;
> };
>
> @@ -411,16 +410,10 @@ struct wmax_u32_args {
> };
>
> static struct platform_device *platform_device;
> -static struct device_attribute *zone_dev_attrs;
> -static struct attribute **zone_attrs;
> static struct platform_zone *zone_data;
> static struct platform_profile_handler pp_handler;
> static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
>
> -static struct attribute_group zone_attribute_group = {
> - .name = "rgb_zones",
> -};
> -
> static u8 interface;
> static u8 lighting_control_state;
> static u8 global_brightness;
> @@ -452,20 +445,6 @@ static int parse_rgb(const char *buf, struct color_platform *colors)
> return 0;
> }
>
> -static struct platform_zone *match_zone(struct device_attribute *attr)
> -{
> - u8 zone;
> -
> - for (zone = 0; zone < quirks->num_zones; zone++) {
> - if ((struct device_attribute *)zone_data[zone].attr == attr) {
> - pr_debug("alienware-wmi: matched zone location: %d\n",
> - zone_data[zone].location);
> - return &zone_data[zone];
> - }
> - }
> - return NULL;
> -}
> -
> /*
> * Individual RGB zone control
> */
> @@ -510,12 +489,10 @@ static int alienware_update_led(struct platform_zone *zone)
> }
>
> static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
> - char *buf)
> + char *buf, u8 location)
> {
> struct platform_zone *target_zone;
> - target_zone = match_zone(attr);
> - if (target_zone == NULL)
> - return sprintf(buf, "red: -1, green: -1, blue: -1\n");
> + target_zone = &zone_data[location];
> return sprintf(buf, "red: %d, green: %d, blue: %d\n",
> target_zone->colors.red,
> target_zone->colors.green, target_zone->colors.blue);
> @@ -523,15 +500,11 @@ static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
> }
>
> static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t count)
> + const char *buf, size_t count, u8 location)
> {
> struct platform_zone *target_zone;
> int ret;
> - target_zone = match_zone(attr);
> - if (target_zone == NULL) {
> - pr_err("alienware-wmi: invalid target zone\n");
> - return 1;
> - }
> + target_zone = &zone_data[location];
> ret = parse_rgb(buf, &target_zone->colors);
> if (ret)
> return ret;
> @@ -539,6 +512,32 @@ static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
> return ret ? ret : count;
> }
>
> +#define ALIENWARE_ZONE_SHOW_FUNC(_num) \
> + static ssize_t zone0##_num##_show(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> + { \
> + return zone_show(dev, attr, buf, _num); \
> + }
> +
> +#define ALIENWARE_ZONE_STORE_FUNC(_num) \
> + static ssize_t zone0##_num##_store(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t count) \
> + { \
> + return zone_set(dev, attr, buf, count, _num); \
> + }
> +
> +#define ALIENWARE_ZONE_ATTR(_num) \
> + ALIENWARE_ZONE_SHOW_FUNC(_num) \
> + ALIENWARE_ZONE_STORE_FUNC(_num) \
> + static DEVICE_ATTR_RW(zone0##_num)
> +
> +ALIENWARE_ZONE_ATTR(0);
> +ALIENWARE_ZONE_ATTR(1);
> +ALIENWARE_ZONE_ATTR(2);
> +ALIENWARE_ZONE_ATTR(3);
> +
> /*
> * Lighting control state device attribute (Global)
> */
> @@ -577,6 +576,33 @@ static ssize_t lighting_control_state_store(struct device *dev,
>
> static DEVICE_ATTR_RW(lighting_control_state);
>
> +static umode_t zone_attr_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + return n < quirks->num_zones + 1 ? 0644 : 0;
> +}
> +
> +static bool zone_group_visible(struct kobject *kobj)
> +{
> + return quirks->num_zones > 0;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(zone);
> +
> +static struct attribute *zone_attrs[] = {
> + &dev_attr_lighting_control_state.attr,
> + &dev_attr_zone00.attr,
> + &dev_attr_zone01.attr,
> + &dev_attr_zone02.attr,
> + &dev_attr_zone03.attr,
> + NULL
> +};
> +
> +static struct attribute_group zone_attribute_group = {
> + .name = "rgb_zones",
> + .is_visible = SYSFS_GROUP_VISIBLE(zone),
> + .attrs = zone_attrs,
> +};
> +
> /*
> * LED Brightness (Global)
> */
> @@ -624,7 +650,6 @@ static struct led_classdev global_led = {
> static int alienware_zone_init(struct platform_device *dev)
> {
> u8 zone;
> - char *name;
>
> if (interface == WMAX) {
> lighting_control_state = WMAX_RUNNING;
> @@ -634,65 +659,22 @@ static int alienware_zone_init(struct platform_device *dev)
> global_led.max_brightness = 0x0F;
> global_brightness = global_led.max_brightness;
>
> - /*
> - * - zone_dev_attrs num_zones + 1 is for individual zones and then
> - * null terminated
> - * - zone_attrs num_zones + 2 is for all attrs in zone_dev_attrs +
> - * the lighting control + null terminated
> - * - zone_data num_zones is for the distinct zones
> - */
> - zone_dev_attrs =
> - kcalloc(quirks->num_zones + 1, sizeof(struct device_attribute),
> - GFP_KERNEL);
> - if (!zone_dev_attrs)
> - return -ENOMEM;
> -
> - zone_attrs =
> - kcalloc(quirks->num_zones + 2, sizeof(struct attribute *),
> - GFP_KERNEL);
> - if (!zone_attrs)
> - return -ENOMEM;
> -
> zone_data =
> kcalloc(quirks->num_zones, sizeof(struct platform_zone),
> GFP_KERNEL);
> if (!zone_data)
> return -ENOMEM;
>
> - for (zone = 0; zone < quirks->num_zones; zone++) {
> - name = kasprintf(GFP_KERNEL, "zone%02hhX", zone);
> - if (name == NULL)
> - return 1;
> - sysfs_attr_init(&zone_dev_attrs[zone].attr);
> - zone_dev_attrs[zone].attr.name = name;
> - zone_dev_attrs[zone].attr.mode = 0644;
> - zone_dev_attrs[zone].show = zone_show;
> - zone_dev_attrs[zone].store = zone_set;
> + for (zone = 0; zone < 4; zone++)
You allocate quirks->num_zones entries to zone_data above but use a
literal here?
--
i.
> zone_data[zone].location = zone;
> - zone_attrs[zone] = &zone_dev_attrs[zone].attr;
> - zone_data[zone].attr = &zone_dev_attrs[zone];
> - }
> - zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
> - zone_attribute_group.attrs = zone_attrs;
> -
> - led_classdev_register(&dev->dev, &global_led);
>
> - return sysfs_create_group(&dev->dev.kobj, &zone_attribute_group);
> + return led_classdev_register(&dev->dev, &global_led);
> }
>
> static void alienware_zone_exit(struct platform_device *dev)
> {
> - u8 zone;
> -
> - sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group);
> led_classdev_unregister(&global_led);
> - if (zone_dev_attrs) {
> - for (zone = 0; zone < quirks->num_zones; zone++)
> - kfree(zone_dev_attrs[zone].attr.name);
> - }
> - kfree(zone_dev_attrs);
> kfree(zone_data);
> - kfree(zone_attrs);
> }
>
> static acpi_status alienware_wmax_command(void *in_args, size_t in_size,
> @@ -1140,6 +1122,7 @@ static void remove_thermal_profile(void)
> * Platform Driver
> */
> static const struct attribute_group *alienfx_groups[] = {
> + &zone_attribute_group,
> &hdmi_attribute_group,
> &lifier_attribute_group,
> &deepsleep_attribute_group,
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation
2024-12-05 10:17 ` Ilpo Järvinen
@ 2024-12-05 12:48 ` Kurt Borja
2024-12-05 13:18 ` Ilpo Järvinen
0 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 12:48 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 12:17:01PM +0200, Ilpo Järvinen wrote:
> On Wed, 4 Dec 2024, Kurt Borja wrote:
>
> > Define zone_attrs statically with the use of helper macros and
> > initialize the zone_attribute_group with driver's .dev_groups.
> >
> > This makes match_zone() no longer needed, so drop it.
> >
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> > drivers/platform/x86/dell/alienware-wmi.c | 137 ++++++++++------------
> > 1 file changed, 60 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index 78bbb4ef4526..fa7bbbb07b86 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -378,7 +378,6 @@ struct color_platform {
> >
> > struct platform_zone {
> > u8 location;
> > - struct device_attribute *attr;
> > struct color_platform colors;
> > };
> >
> > @@ -411,16 +410,10 @@ struct wmax_u32_args {
> > };
> >
> > static struct platform_device *platform_device;
> > -static struct device_attribute *zone_dev_attrs;
> > -static struct attribute **zone_attrs;
> > static struct platform_zone *zone_data;
> > static struct platform_profile_handler pp_handler;
> > static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
> >
> > -static struct attribute_group zone_attribute_group = {
> > - .name = "rgb_zones",
> > -};
> > -
> > static u8 interface;
> > static u8 lighting_control_state;
> > static u8 global_brightness;
> > @@ -452,20 +445,6 @@ static int parse_rgb(const char *buf, struct color_platform *colors)
> > return 0;
> > }
> >
> > -static struct platform_zone *match_zone(struct device_attribute *attr)
> > -{
> > - u8 zone;
> > -
> > - for (zone = 0; zone < quirks->num_zones; zone++) {
> > - if ((struct device_attribute *)zone_data[zone].attr == attr) {
> > - pr_debug("alienware-wmi: matched zone location: %d\n",
> > - zone_data[zone].location);
> > - return &zone_data[zone];
> > - }
> > - }
> > - return NULL;
> > -}
> > -
> > /*
> > * Individual RGB zone control
> > */
> > @@ -510,12 +489,10 @@ static int alienware_update_led(struct platform_zone *zone)
> > }
> >
> > static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
> > - char *buf)
> > + char *buf, u8 location)
> > {
> > struct platform_zone *target_zone;
> > - target_zone = match_zone(attr);
> > - if (target_zone == NULL)
> > - return sprintf(buf, "red: -1, green: -1, blue: -1\n");
> > + target_zone = &zone_data[location];
> > return sprintf(buf, "red: %d, green: %d, blue: %d\n",
> > target_zone->colors.red,
> > target_zone->colors.green, target_zone->colors.blue);
> > @@ -523,15 +500,11 @@ static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
> > }
> >
> > static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
> > - const char *buf, size_t count)
> > + const char *buf, size_t count, u8 location)
> > {
> > struct platform_zone *target_zone;
> > int ret;
> > - target_zone = match_zone(attr);
> > - if (target_zone == NULL) {
> > - pr_err("alienware-wmi: invalid target zone\n");
> > - return 1;
> > - }
> > + target_zone = &zone_data[location];
> > ret = parse_rgb(buf, &target_zone->colors);
> > if (ret)
> > return ret;
> > @@ -539,6 +512,32 @@ static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
> > return ret ? ret : count;
> > }
> >
> > +#define ALIENWARE_ZONE_SHOW_FUNC(_num) \
> > + static ssize_t zone0##_num##_show(struct device *dev, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > + { \
> > + return zone_show(dev, attr, buf, _num); \
> > + }
> > +
> > +#define ALIENWARE_ZONE_STORE_FUNC(_num) \
> > + static ssize_t zone0##_num##_store(struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, size_t count) \
> > + { \
> > + return zone_set(dev, attr, buf, count, _num); \
> > + }
> > +
> > +#define ALIENWARE_ZONE_ATTR(_num) \
> > + ALIENWARE_ZONE_SHOW_FUNC(_num) \
> > + ALIENWARE_ZONE_STORE_FUNC(_num) \
> > + static DEVICE_ATTR_RW(zone0##_num)
> > +
> > +ALIENWARE_ZONE_ATTR(0);
> > +ALIENWARE_ZONE_ATTR(1);
> > +ALIENWARE_ZONE_ATTR(2);
> > +ALIENWARE_ZONE_ATTR(3);
> > +
> > /*
> > * Lighting control state device attribute (Global)
> > */
> > @@ -577,6 +576,33 @@ static ssize_t lighting_control_state_store(struct device *dev,
> >
> > static DEVICE_ATTR_RW(lighting_control_state);
> >
> > +static umode_t zone_attr_visible(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > +{
> > + return n < quirks->num_zones + 1 ? 0644 : 0;
> > +}
> > +
> > +static bool zone_group_visible(struct kobject *kobj)
> > +{
> > + return quirks->num_zones > 0;
> > +}
> > +DEFINE_SYSFS_GROUP_VISIBLE(zone);
> > +
> > +static struct attribute *zone_attrs[] = {
> > + &dev_attr_lighting_control_state.attr,
> > + &dev_attr_zone00.attr,
> > + &dev_attr_zone01.attr,
> > + &dev_attr_zone02.attr,
> > + &dev_attr_zone03.attr,
> > + NULL
> > +};
> > +
> > +static struct attribute_group zone_attribute_group = {
> > + .name = "rgb_zones",
> > + .is_visible = SYSFS_GROUP_VISIBLE(zone),
> > + .attrs = zone_attrs,
> > +};
> > +
> > /*
> > * LED Brightness (Global)
> > */
> > @@ -624,7 +650,6 @@ static struct led_classdev global_led = {
> > static int alienware_zone_init(struct platform_device *dev)
> > {
> > u8 zone;
> > - char *name;
> >
> > if (interface == WMAX) {
> > lighting_control_state = WMAX_RUNNING;
> > @@ -634,65 +659,22 @@ static int alienware_zone_init(struct platform_device *dev)
> > global_led.max_brightness = 0x0F;
> > global_brightness = global_led.max_brightness;
> >
> > - /*
> > - * - zone_dev_attrs num_zones + 1 is for individual zones and then
> > - * null terminated
> > - * - zone_attrs num_zones + 2 is for all attrs in zone_dev_attrs +
> > - * the lighting control + null terminated
> > - * - zone_data num_zones is for the distinct zones
> > - */
> > - zone_dev_attrs =
> > - kcalloc(quirks->num_zones + 1, sizeof(struct device_attribute),
> > - GFP_KERNEL);
> > - if (!zone_dev_attrs)
> > - return -ENOMEM;
> > -
> > - zone_attrs =
> > - kcalloc(quirks->num_zones + 2, sizeof(struct attribute *),
> > - GFP_KERNEL);
> > - if (!zone_attrs)
> > - return -ENOMEM;
> > -
> > zone_data =
> > kcalloc(quirks->num_zones, sizeof(struct platform_zone),
> > GFP_KERNEL);
> > if (!zone_data)
> > return -ENOMEM;
> >
> > - for (zone = 0; zone < quirks->num_zones; zone++) {
> > - name = kasprintf(GFP_KERNEL, "zone%02hhX", zone);
> > - if (name == NULL)
> > - return 1;
> > - sysfs_attr_init(&zone_dev_attrs[zone].attr);
> > - zone_dev_attrs[zone].attr.name = name;
> > - zone_dev_attrs[zone].attr.mode = 0644;
> > - zone_dev_attrs[zone].show = zone_show;
> > - zone_dev_attrs[zone].store = zone_set;
> > + for (zone = 0; zone < 4; zone++)
>
> You allocate quirks->num_zones entries to zone_data above but use a
> literal here?
I did this because quirks->num_zones controlls only visibility now. I
didn't feel comfortable leaving an out of bounds access on zone_show()
and zone_set() when they do `zone_data[location]`.
Still those out of bounds accesses are hidden from user-space (right?) and
alienware_wmi_init() is getting dropped anyway so I should just leave it
as zone < quirks->num_zones.
>
> --
> i.
>
>
> > zone_data[zone].location = zone;
> > - zone_attrs[zone] = &zone_dev_attrs[zone].attr;
> > - zone_data[zone].attr = &zone_dev_attrs[zone];
> > - }
> > - zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr;
> > - zone_attribute_group.attrs = zone_attrs;
> > -
> > - led_classdev_register(&dev->dev, &global_led);
> >
> > - return sysfs_create_group(&dev->dev.kobj, &zone_attribute_group);
> > + return led_classdev_register(&dev->dev, &global_led);
> > }
> >
> > static void alienware_zone_exit(struct platform_device *dev)
> > {
> > - u8 zone;
> > -
> > - sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group);
> > led_classdev_unregister(&global_led);
> > - if (zone_dev_attrs) {
> > - for (zone = 0; zone < quirks->num_zones; zone++)
> > - kfree(zone_dev_attrs[zone].attr.name);
> > - }
> > - kfree(zone_dev_attrs);
> > kfree(zone_data);
> > - kfree(zone_attrs);
> > }
> >
> > static acpi_status alienware_wmax_command(void *in_args, size_t in_size,
> > @@ -1140,6 +1122,7 @@ static void remove_thermal_profile(void)
> > * Platform Driver
> > */
> > static const struct attribute_group *alienfx_groups[] = {
> > + &zone_attribute_group,
> > &hdmi_attribute_group,
> > &lifier_attribute_group,
> > &deepsleep_attribute_group,
> >
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation
2024-12-05 12:48 ` Kurt Borja
@ 2024-12-05 13:18 ` Ilpo Järvinen
2024-12-05 13:34 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 13:18 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]
On Thu, 5 Dec 2024, Kurt Borja wrote:
> On Thu, Dec 05, 2024 at 12:17:01PM +0200, Ilpo Järvinen wrote:
> > On Wed, 4 Dec 2024, Kurt Borja wrote:
> >
> > > Define zone_attrs statically with the use of helper macros and
> > > initialize the zone_attribute_group with driver's .dev_groups.
> > >
> > > This makes match_zone() no longer needed, so drop it.
> > >
> > > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > > zone_data =
> > > kcalloc(quirks->num_zones, sizeof(struct platform_zone),
> > > GFP_KERNEL);
You kcalloc() zone_data here for quirks->num_zones entries....
> > > - for (zone = 0; zone < quirks->num_zones; zone++) {
> > > - name = kasprintf(GFP_KERNEL, "zone%02hhX", zone);
> > > - if (name == NULL)
> > > - return 1;
> > > - sysfs_attr_init(&zone_dev_attrs[zone].attr);
> > > - zone_dev_attrs[zone].attr.name = name;
> > > - zone_dev_attrs[zone].attr.mode = 0644;
> > > - zone_dev_attrs[zone].show = zone_show;
> > > - zone_dev_attrs[zone].store = zone_set;
> > > + for (zone = 0; zone < 4; zone++)
> > > zone_data[zone].location = zone;
> >
> > You allocate quirks->num_zones entries to zone_data above but use a
> > literal here?
>
> I did this because quirks->num_zones controlls only visibility now.
This kind of information would be useful to put into the commit message!
It does not control only visibility, see the kcalloc() code above. Did you
mean to alter the alloc too but forgot?
> I didn't feel comfortable leaving an out of bounds access on zone_show()
> and zone_set() when they do `zone_data[location]`.
>
> Still those out of bounds accesses are hidden from user-space (right?) and
> alienware_wmi_init() is getting dropped anyway so I should just leave it
> as zone < quirks->num_zones.
The assignment within this loop will write out of bounds if
quirks->num_zones is less than 4.
--
i.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation
2024-12-05 13:18 ` Ilpo Järvinen
@ 2024-12-05 13:34 ` Kurt Borja
0 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 13:34 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 03:18:20PM +0200, Ilpo Järvinen wrote:
> On Thu, 5 Dec 2024, Kurt Borja wrote:
>
> > On Thu, Dec 05, 2024 at 12:17:01PM +0200, Ilpo Järvinen wrote:
> > > On Wed, 4 Dec 2024, Kurt Borja wrote:
> > >
> > > > Define zone_attrs statically with the use of helper macros and
> > > > initialize the zone_attribute_group with driver's .dev_groups.
> > > >
> > > > This makes match_zone() no longer needed, so drop it.
> > > >
> > > > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>
> > > > zone_data =
> > > > kcalloc(quirks->num_zones, sizeof(struct platform_zone),
> > > > GFP_KERNEL);
>
> You kcalloc() zone_data here for quirks->num_zones entries....
Totally missed this, thanks.
>
> > > > - for (zone = 0; zone < quirks->num_zones; zone++) {
> > > > - name = kasprintf(GFP_KERNEL, "zone%02hhX", zone);
> > > > - if (name == NULL)
> > > > - return 1;
> > > > - sysfs_attr_init(&zone_dev_attrs[zone].attr);
> > > > - zone_dev_attrs[zone].attr.name = name;
> > > > - zone_dev_attrs[zone].attr.mode = 0644;
> > > > - zone_dev_attrs[zone].show = zone_show;
> > > > - zone_dev_attrs[zone].store = zone_set;
> > > > + for (zone = 0; zone < 4; zone++)
> > > > zone_data[zone].location = zone;
> > >
> > > You allocate quirks->num_zones entries to zone_data above but use a
> > > literal here?
> >
> > I did this because quirks->num_zones controlls only visibility now.
>
> This kind of information would be useful to put into the commit message!
>
> It does not control only visibility, see the kcalloc() code above. Did you
> mean to alter the alloc too but forgot?
Yes I did not pay much attention when modifying this function. I'll fix
it.
>
> > I didn't feel comfortable leaving an out of bounds access on zone_show()
> > and zone_set() when they do `zone_data[location]`.
> >
> > Still those out of bounds accesses are hidden from user-space (right?) and
> > alienware_wmi_init() is getting dropped anyway so I should just leave it
> > as zone < quirks->num_zones.
>
> The assignment within this loop will write out of bounds if
> quirks->num_zones is less than 4.
>
> --
> i.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 06/21] alienware-wmi: Add state container and alienfx_probe()
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (4 preceding siblings ...)
2024-12-05 0:40 ` [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation Kurt Borja
@ 2024-12-05 0:40 ` Kurt Borja
2024-12-05 0:40 ` [RFC PATCH 07/21] alienware-wmi: Migrate to state container pattern Kurt Borja
` (15 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:40 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Add a state container and initialize it on the new alienfx_probe().
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 35 +++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index fa7bbbb07b86..ea848937b579 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -409,6 +409,14 @@ struct wmax_u32_args {
u8 arg3;
};
+struct alienfx_priv {
+ struct platform_device *pdev;
+ struct led_classdev global_led;
+ struct color_platform colors[4];
+ u8 global_brightness;
+ u8 lighting_control_state;
+};
+
static struct platform_device *platform_device;
static struct platform_zone *zone_data;
static struct platform_profile_handler pp_handler;
@@ -1121,6 +1129,32 @@ static void remove_thermal_profile(void)
/*
* Platform Driver
*/
+static int alienfx_probe(struct platform_device *pdev)
+{
+ struct alienfx_priv *priv;
+ struct led_classdev *leds;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ platform_set_drvdata(pdev, priv);
+
+ priv->pdev = pdev;
+
+ if (interface == WMAX)
+ priv->lighting_control_state = WMAX_RUNNING;
+ else if (interface == LEGACY)
+ priv->lighting_control_state = LEGACY_RUNNING;
+
+ leds = &priv->global_led;
+ leds->name = "alienware::global_brightness";
+ leds->brightness_set = global_led_set;
+ leds->brightness_get = global_led_get;
+ leds->max_brightness = 0x0F;
+
+ priv->global_brightness = priv->global_led.max_brightness;
+
+ return 0;
+}
+
static const struct attribute_group *alienfx_groups[] = {
&zone_attribute_group,
&hdmi_attribute_group,
@@ -1134,6 +1168,7 @@ static struct platform_driver platform_driver = {
.name = "alienware-wmi",
.dev_groups = alienfx_groups,
},
+ .probe = alienfx_probe,
};
static int __init alienware_wmi_init(void)
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 07/21] alienware-wmi: Migrate to state container pattern
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (5 preceding siblings ...)
2024-12-05 0:40 ` [RFC PATCH 06/21] alienware-wmi: Add state container and alienfx_probe() Kurt Borja
@ 2024-12-05 0:40 ` Kurt Borja
2024-12-05 0:41 ` [RFC PATCH 08/21] alienware-wmi: Add WMI Drivers Kurt Borja
` (14 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:40 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Migrate all led control functions to use the platform_device state
container.
Additionally move the led_classdev registration to the platform driver
probe and make it device managed.
Drop alienware_zone_init() and alienware_zone_exit() because it's no
longer needed.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 123 +++++++++-------------
1 file changed, 47 insertions(+), 76 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index ea848937b579..e5e4a7b4e9ca 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -376,11 +376,6 @@ struct color_platform {
u8 red;
} __packed;
-struct platform_zone {
- u8 location;
- struct color_platform colors;
-};
-
struct wmax_brightness_args {
u32 led_mask;
u32 percentage;
@@ -418,13 +413,10 @@ struct alienfx_priv {
};
static struct platform_device *platform_device;
-static struct platform_zone *zone_data;
static struct platform_profile_handler pp_handler;
static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
static u8 interface;
-static u8 lighting_control_state;
-static u8 global_brightness;
/*
* Helpers used for zone control
@@ -456,7 +448,7 @@ static int parse_rgb(const char *buf, struct color_platform *colors)
/*
* Individual RGB zone control
*/
-static int alienware_update_led(struct platform_zone *zone)
+static int alienware_update_led(struct alienfx_priv *priv, u8 location)
{
int method_id;
acpi_status status;
@@ -465,25 +457,25 @@ static int alienware_update_led(struct platform_zone *zone)
struct legacy_led_args legacy_args;
struct wmax_led_args wmax_basic_args;
if (interface == WMAX) {
- wmax_basic_args.led_mask = 1 << zone->location;
- wmax_basic_args.colors = zone->colors;
- wmax_basic_args.state = lighting_control_state;
+ wmax_basic_args.led_mask = 1 << location;
+ wmax_basic_args.colors = priv->colors[location];
+ wmax_basic_args.state = priv->lighting_control_state;
guid = WMAX_CONTROL_GUID;
method_id = WMAX_METHOD_ZONE_CONTROL;
input.length = sizeof(wmax_basic_args);
input.pointer = &wmax_basic_args;
} else {
- legacy_args.colors = zone->colors;
- legacy_args.brightness = global_brightness;
+ legacy_args.colors = priv->colors[location];
+ legacy_args.brightness = priv->global_brightness;
legacy_args.state = 0;
- if (lighting_control_state == LEGACY_BOOTING ||
- lighting_control_state == LEGACY_SUSPEND) {
+ if (priv->lighting_control_state == LEGACY_BOOTING ||
+ priv->lighting_control_state == LEGACY_SUSPEND) {
guid = LEGACY_POWER_CONTROL_GUID;
- legacy_args.state = lighting_control_state;
+ legacy_args.state = priv->lighting_control_state;
} else
guid = LEGACY_CONTROL_GUID;
- method_id = zone->location + 1;
+ method_id = location + 1;
input.length = sizeof(legacy_args);
input.pointer = &legacy_args;
@@ -499,24 +491,33 @@ static int alienware_update_led(struct platform_zone *zone)
static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
char *buf, u8 location)
{
- struct platform_zone *target_zone;
- target_zone = &zone_data[location];
+ struct alienfx_priv *priv;
+ struct color_platform *colors;
+
+ priv = dev_get_drvdata(dev);
+ colors = &priv->colors[location];
+
return sprintf(buf, "red: %d, green: %d, blue: %d\n",
- target_zone->colors.red,
- target_zone->colors.green, target_zone->colors.blue);
+ colors->red, colors->green, colors->blue);
}
static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count, u8 location)
{
- struct platform_zone *target_zone;
+ struct alienfx_priv *priv;
+ struct color_platform *colors;
int ret;
- target_zone = &zone_data[location];
- ret = parse_rgb(buf, &target_zone->colors);
+
+ priv = dev_get_drvdata(dev);
+
+ colors = &priv->colors[location];
+ ret = parse_rgb(buf, colors);
if (ret)
return ret;
- ret = alienware_update_led(target_zone);
+
+ ret = alienware_update_led(priv, location);
+
return ret ? ret : count;
}
@@ -553,9 +554,13 @@ static ssize_t lighting_control_state_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- if (lighting_control_state == LEGACY_BOOTING)
+ struct alienfx_priv *priv;
+
+ priv = dev_get_drvdata(dev);
+
+ if (priv->lighting_control_state == LEGACY_BOOTING)
return sysfs_emit(buf, "[booting] running suspend\n");
- else if (lighting_control_state == LEGACY_SUSPEND)
+ else if (priv->lighting_control_state == LEGACY_SUSPEND)
return sysfs_emit(buf, "booting running [suspend]\n");
return sysfs_emit(buf, "booting [running] suspend\n");
}
@@ -564,8 +569,11 @@ static ssize_t lighting_control_state_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
+ struct alienfx_priv *priv;
u8 val;
+ priv = dev_get_drvdata(dev);
+
if (strcmp(buf, "booting\n") == 0)
val = LEGACY_BOOTING;
else if (strcmp(buf, "suspend\n") == 0)
@@ -575,9 +583,9 @@ static ssize_t lighting_control_state_store(struct device *dev,
else
val = WMAX_RUNNING;
- lighting_control_state = val;
+ priv->lighting_control_state = val;
pr_debug("alienware-wmi: updated control state to %d\n",
- lighting_control_state);
+ priv->lighting_control_state);
return count;
}
@@ -634,55 +642,26 @@ static int wmax_brightness(int brightness)
static void global_led_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
+ struct alienfx_priv *priv;
int ret;
- global_brightness = brightness;
+
+ priv = container_of(led_cdev, struct alienfx_priv, global_led);
+ priv->global_brightness = brightness;
if (interface == WMAX)
ret = wmax_brightness(brightness);
else
- ret = alienware_update_led(&zone_data[0]);
+ ret = alienware_update_led(priv, 0);
if (ret)
pr_err("LED brightness update failed\n");
}
static enum led_brightness global_led_get(struct led_classdev *led_cdev)
{
- return global_brightness;
-}
-
-static struct led_classdev global_led = {
- .brightness_set = global_led_set,
- .brightness_get = global_led_get,
- .name = "alienware::global_brightness",
-};
-
-static int alienware_zone_init(struct platform_device *dev)
-{
- u8 zone;
-
- if (interface == WMAX) {
- lighting_control_state = WMAX_RUNNING;
- } else if (interface == LEGACY) {
- lighting_control_state = LEGACY_RUNNING;
- }
- global_led.max_brightness = 0x0F;
- global_brightness = global_led.max_brightness;
-
- zone_data =
- kcalloc(quirks->num_zones, sizeof(struct platform_zone),
- GFP_KERNEL);
- if (!zone_data)
- return -ENOMEM;
-
- for (zone = 0; zone < 4; zone++)
- zone_data[zone].location = zone;
+ struct alienfx_priv *priv;
- return led_classdev_register(&dev->dev, &global_led);
-}
+ priv = container_of(led_cdev, struct alienfx_priv, global_led);
-static void alienware_zone_exit(struct platform_device *dev)
-{
- led_classdev_unregister(&global_led);
- kfree(zone_data);
+ return priv->global_brightness;
}
static acpi_status alienware_wmax_command(void *in_args, size_t in_size,
@@ -1152,7 +1131,7 @@ static int alienfx_probe(struct platform_device *pdev)
priv->global_brightness = priv->global_led.max_brightness;
- return 0;
+ return devm_led_classdev_register(&pdev->dev, &priv->global_led);
}
static const struct attribute_group *alienfx_groups[] = {
@@ -1216,15 +1195,8 @@ static int __init alienware_wmi_init(void)
goto fail_prep_thermal_profile;
}
- ret = alienware_zone_init(platform_device);
- if (ret)
- goto fail_prep_zones;
-
return 0;
-fail_prep_zones:
- alienware_zone_exit(platform_device);
- remove_thermal_profile();
fail_prep_thermal_profile:
platform_device_del(platform_device);
fail_platform_device2:
@@ -1239,7 +1211,6 @@ module_init(alienware_wmi_init);
static void __exit alienware_wmi_exit(void)
{
- alienware_zone_exit(platform_device);
remove_thermal_profile();
platform_device_unregister(platform_device);
platform_driver_unregister(&platform_driver);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 08/21] alienware-wmi: Add WMI Drivers
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (6 preceding siblings ...)
2024-12-05 0:40 ` [RFC PATCH 07/21] alienware-wmi: Migrate to state container pattern Kurt Borja
@ 2024-12-05 0:41 ` Kurt Borja
2024-12-05 0:41 ` [RFC PATCH 09/21] alienware-wmi: Initialize WMI drivers Kurt Borja
` (13 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:41 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Add WMI drivers for LEGACY and WMAX devices. On hosts that support the
AlienFX interface, the corresponding platform device will be registered
as a bundle on the WMI driver's probe and will "pass" a WMI device
pointer to the platform device as platform specific data.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 115 +++++++++++++++++++++-
1 file changed, 113 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index e5e4a7b4e9ca..38e9cb80ae69 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);
@@ -412,6 +411,10 @@ struct alienfx_priv {
u8 lighting_control_state;
};
+struct alienfx_platdata {
+ struct wmi_device *wdev;
+};
+
static struct platform_device *platform_device;
static struct platform_profile_handler pp_handler;
static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
@@ -1150,6 +1153,114 @@ static struct platform_driver platform_driver = {
.probe = alienfx_probe,
};
+static int alienfx_wmi_init(struct alienfx_platdata *pdata)
+{
+ struct platform_device *pdev;
+
+ pdev = platform_create_bundle(&platform_driver, alienfx_probe, NULL, 0,
+ pdata, sizeof(*pdata));
+
+ dev_set_drvdata(&pdata->wdev->dev, pdev);
+
+ return PTR_ERR_OR_ZERO(pdev);
+}
+
+static void alienfx_wmi_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 device
+ */
+static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ int ret = 0;
+ struct alienfx_platdata pdata = {
+ .wdev = wdev,
+ };
+
+ if (quirks->num_zones > 0)
+ ret = alienfx_wmi_init(&pdata);
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void legacy_wmi_remove(struct wmi_device *wdev)
+{
+ if (quirks->num_zones > 0)
+ alienfx_wmi_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,
+};
+
+/*
+ * WMAX WMI device
+ */
+static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ int ret = 0;
+ struct alienfx_platdata pdata = {
+ .wdev = wdev,
+ };
+
+ if (quirks->thermal)
+ ret = create_thermal_profile();
+ else if (quirks->num_zones > 0)
+ ret = alienfx_wmi_init(&pdata);
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void wmax_wmi_remove(struct wmi_device *wdev)
+{
+ if (quirks->thermal)
+ remove_thermal_profile();
+ else if (quirks->num_zones > 0)
+ alienfx_wmi_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_wmi_init(void)
{
int ret;
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 09/21] alienware-wmi: Initialize WMI drivers
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (7 preceding siblings ...)
2024-12-05 0:41 ` [RFC PATCH 08/21] alienware-wmi: Add WMI Drivers Kurt Borja
@ 2024-12-05 0:41 ` Kurt Borja
2024-12-05 0:42 ` [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata Kurt Borja
` (12 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:41 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Usually, laptop models only support one of the two WMI devices, however,
to avoid the risk of registering two platform devices with the same
name, the WMAX device driver is always preferred.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 48 ++++-------------------
1 file changed, 8 insertions(+), 40 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 38e9cb80ae69..34fb59a14bc0 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -415,11 +415,11 @@ struct alienfx_platdata {
struct wmi_device *wdev;
};
-static struct platform_device *platform_device;
static struct platform_profile_handler pp_handler;
static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
static u8 interface;
+static struct wmi_driver *preferred_wmi_driver;
/*
* Helpers used for zone control
@@ -1263,15 +1263,12 @@ static struct wmi_driver alienware_wmax_wmi_driver = {
static int __init alienware_wmi_init(void)
{
- int ret;
-
- if (wmi_has_guid(LEGACY_CONTROL_GUID))
- interface = LEGACY;
- else if (wmi_has_guid(WMAX_CONTROL_GUID))
+ if (wmi_has_guid(WMAX_CONTROL_GUID)) {
interface = WMAX;
- else {
- pr_warn("alienware-wmi: No known WMI GUID found\n");
- return -ENODEV;
+ preferred_wmi_driver = &alienware_wmax_wmi_driver;
+ } else {
+ interface = LEGACY;
+ preferred_wmi_driver = &alienware_legacy_wmi_driver;
}
dmi_check_system(alienware_quirks);
@@ -1288,43 +1285,14 @@ 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;
- }
- ret = platform_device_add(platform_device);
- if (ret)
- goto fail_platform_device2;
-
- if (quirks->thermal) {
- ret = create_thermal_profile();
- if (ret)
- goto fail_prep_thermal_profile;
- }
-
- 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;
+ return wmi_driver_register(preferred_wmi_driver);
}
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);
+ wmi_driver_unregister(preferred_wmi_driver);
}
module_exit(alienware_wmi_exit);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (8 preceding siblings ...)
2024-12-05 0:41 ` [RFC PATCH 09/21] alienware-wmi: Initialize WMI drivers Kurt Borja
@ 2024-12-05 0:42 ` Kurt Borja
2024-12-05 11:05 ` Ilpo Järvinen
2024-12-05 0:43 ` [RFC PATCH 11/21] alienware-wmi: Refactor LED control methods Kurt Borja
` (11 subsequent siblings)
21 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:42 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
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 };
+
+ 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;
+
+ 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;
+
+ 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)
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata
2024-12-05 0:42 ` [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata Kurt Borja
@ 2024-12-05 11:05 ` Ilpo Järvinen
2024-12-05 12:50 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 11:05 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
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)
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata
2024-12-05 11:05 ` Ilpo Järvinen
@ 2024-12-05 12:50 ` Kurt Borja
0 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 12:50 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 01:05:50PM +0200, Ilpo Järvinen wrote:
> 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.
Ack.
>
> > +
> > + 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).
Just personal preference, but I'll stick with the usual 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).
Ack.
>
> --
> 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)
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 11/21] alienware-wmi: Refactor LED control methods
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (9 preceding siblings ...)
2024-12-05 0:42 ` [RFC PATCH 10/21] alienware-wmi: Add alienfx OPs to platdata Kurt Borja
@ 2024-12-05 0:43 ` Kurt Borja
2024-12-05 0:43 ` [RFC PATCH 12/21] alienware-wmi: Refactor hdmi, amplifier, deepslp Kurt Borja
` (10 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:43 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Drop alienware_update_led() and wmax_brightness() in favor of upd_led()
and upd_brightness() provided by device's platdata.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 70 +++--------------------
1 file changed, 8 insertions(+), 62 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 043cde40de9a..93287bbffb97 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -485,46 +485,6 @@ static int parse_rgb(const char *buf, struct color_platform *colors)
/*
* Individual RGB zone control
*/
-static int alienware_update_led(struct alienfx_priv *priv, u8 location)
-{
- int method_id;
- acpi_status status;
- char *guid;
- struct acpi_buffer input;
- struct legacy_led_args legacy_args;
- struct wmax_led_args wmax_basic_args;
- if (interface == WMAX) {
- wmax_basic_args.led_mask = 1 << location;
- wmax_basic_args.colors = priv->colors[location];
- wmax_basic_args.state = priv->lighting_control_state;
- guid = WMAX_CONTROL_GUID;
- method_id = WMAX_METHOD_ZONE_CONTROL;
-
- input.length = sizeof(wmax_basic_args);
- input.pointer = &wmax_basic_args;
- } else {
- legacy_args.colors = priv->colors[location];
- legacy_args.brightness = priv->global_brightness;
- legacy_args.state = 0;
- if (priv->lighting_control_state == LEGACY_BOOTING ||
- priv->lighting_control_state == LEGACY_SUSPEND) {
- guid = LEGACY_POWER_CONTROL_GUID;
- legacy_args.state = priv->lighting_control_state;
- } else
- guid = LEGACY_CONTROL_GUID;
- method_id = location + 1;
-
- input.length = sizeof(legacy_args);
- input.pointer = &legacy_args;
- }
- pr_debug("alienware-wmi: guid %s method %d\n", guid, method_id);
-
- status = wmi_evaluate_method(guid, 0, method_id, &input, NULL);
- if (ACPI_FAILURE(status))
- pr_err("alienware-wmi: zone set failure: %u\n", status);
- return ACPI_FAILURE(status);
-}
-
static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
char *buf, u8 location)
{
@@ -543,17 +503,19 @@ static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count, u8 location)
{
struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
struct color_platform *colors;
int ret;
priv = dev_get_drvdata(dev);
+ pdata = dev_get_platdata(dev);
colors = &priv->colors[location];
ret = parse_rgb(buf, colors);
if (ret)
return ret;
- ret = alienware_update_led(priv, location);
+ ret = pdata->ops.upd_led(priv, pdata->wdev, location);
return ret ? ret : count;
}
@@ -659,35 +621,19 @@ static struct attribute_group zone_attribute_group = {
/*
* LED Brightness (Global)
*/
-static int wmax_brightness(int brightness)
-{
- acpi_status status;
- struct acpi_buffer input;
- struct wmax_brightness_args args = {
- .led_mask = 0xFF,
- .percentage = brightness,
- };
- input.length = sizeof(args);
- input.pointer = &args;
- status = wmi_evaluate_method(WMAX_CONTROL_GUID, 0,
- WMAX_METHOD_BRIGHTNESS, &input, NULL);
- if (ACPI_FAILURE(status))
- pr_err("alienware-wmi: brightness set failure: %u\n", status);
- return ACPI_FAILURE(status);
-}
-
static void global_led_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
int ret;
priv = container_of(led_cdev, struct alienfx_priv, global_led);
+ pdata = dev_get_platdata(&priv->pdev->dev);
+
priv->global_brightness = brightness;
- if (interface == WMAX)
- ret = wmax_brightness(brightness);
- else
- ret = alienware_update_led(priv, 0);
+
+ ret = pdata->ops.upd_brightness(priv, pdata->wdev, brightness);
if (ret)
pr_err("LED brightness update failed\n");
}
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 12/21] alienware-wmi: Refactor hdmi, amplifier, deepslp
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (10 preceding siblings ...)
2024-12-05 0:43 ` [RFC PATCH 11/21] alienware-wmi: Refactor LED control methods Kurt Borja
@ 2024-12-05 0:43 ` Kurt Borja
2024-12-05 0:44 ` [RFC PATCH 13/21] alienware-wmi: Add a state container for AWCC Kurt Borja
` (9 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:43 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Refactor show/store methods for hdmi, amplifier, deepslp sysfs groups to
use alienware_wmi_command() instead of alienware_wmax_command() which
uses deprecated WMI methods.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 51 ++++++++++++++++-------
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 93287bbffb97..8a21342cabd6 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -682,14 +682,18 @@ static acpi_status alienware_wmax_command(void *in_args, size_t in_size,
static ssize_t show_hdmi_cable(struct device *dev,
struct device_attribute *attr, char *buf)
{
+ struct alienfx_platdata *pdata;
acpi_status status;
u32 out_data;
struct wmax_basic_args in_args = {
.arg = 0,
};
- status =
- alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_HDMI_CABLE, &out_data);
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_CABLE,
+ &in_args, sizeof(in_args), &out_data);
+
if (ACPI_SUCCESS(status)) {
if (out_data == 0)
return sysfs_emit(buf, "[unconnected] connected unknown\n");
@@ -703,14 +707,17 @@ static ssize_t show_hdmi_cable(struct device *dev,
static ssize_t show_hdmi_source(struct device *dev,
struct device_attribute *attr, char *buf)
{
+ struct alienfx_platdata *pdata;
acpi_status status;
u32 out_data;
struct wmax_basic_args in_args = {
.arg = 0,
};
- status =
- alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_HDMI_STATUS, &out_data);
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_STATUS,
+ &in_args, sizeof(in_args), &out_data);
if (ACPI_SUCCESS(status)) {
if (out_data == 1)
@@ -726,8 +733,12 @@ static ssize_t toggle_hdmi_source(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
+ struct alienfx_platdata *pdata;
acpi_status status;
struct wmax_basic_args args;
+
+ pdata = dev_get_platdata(dev);
+
if (strcmp(buf, "gpu\n") == 0)
args.arg = 1;
else if (strcmp(buf, "input\n") == 0)
@@ -736,8 +747,8 @@ static ssize_t toggle_hdmi_source(struct device *dev,
args.arg = 3;
pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
- status = alienware_wmax_command(&args, sizeof(args),
- WMAX_METHOD_HDMI_SOURCE, NULL);
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_SOURCE,
+ &args, sizeof(args), NULL);
if (ACPI_FAILURE(status))
pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
@@ -775,14 +786,17 @@ static const struct attribute_group hdmi_attribute_group = {
static ssize_t show_amplifier_status(struct device *dev,
struct device_attribute *attr, char *buf)
{
+ struct alienfx_platdata *pdata;
acpi_status status;
u32 out_data;
struct wmax_basic_args in_args = {
.arg = 0,
};
- status =
- alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_AMPLIFIER_CABLE, &out_data);
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_AMPLIFIER_CABLE,
+ &in_args, sizeof(in_args), &out_data);
if (ACPI_SUCCESS(status)) {
if (out_data == 0)
return sysfs_emit(buf, "[unconnected] connected unknown\n");
@@ -819,13 +833,17 @@ static const struct attribute_group amplifier_attribute_group = {
static ssize_t show_deepsleep_status(struct device *dev,
struct device_attribute *attr, char *buf)
{
+ struct alienfx_platdata *pdata;
acpi_status status;
u32 out_data;
struct wmax_basic_args in_args = {
.arg = 0,
};
- status = alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_DEEP_SLEEP_STATUS, &out_data);
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_STATUS,
+ &in_args, sizeof(in_args), &out_data);
if (ACPI_SUCCESS(status)) {
if (out_data == 0)
return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
@@ -842,9 +860,12 @@ static ssize_t toggle_deepsleep(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
+ struct alienfx_platdata *pdata;
acpi_status status;
struct wmax_basic_args args;
+ pdata = dev_get_platdata(dev);
+
if (strcmp(buf, "disabled\n") == 0)
args.arg = 0;
else if (strcmp(buf, "s5\n") == 0)
@@ -853,8 +874,8 @@ static ssize_t toggle_deepsleep(struct device *dev,
args.arg = 2;
pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
- status = alienware_wmax_command(&args, sizeof(args),
- WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_CONTROL,
+ &args, sizeof(args), NULL);
if (ACPI_FAILURE(status))
pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 13/21] alienware-wmi: Add a state container for AWCC
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (11 preceding siblings ...)
2024-12-05 0:43 ` [RFC PATCH 12/21] alienware-wmi: Refactor hdmi, amplifier, deepslp Kurt Borja
@ 2024-12-05 0:44 ` Kurt Borja
2024-12-05 0:44 ` [RFC PATCH 14/21] alienware-wmi: Migrate thermal methods to wmidev Kurt Borja
` (8 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:44 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Refactor all "platform_profile" methods to use the newly defined
awcc_priv state container instead of global variables.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 39 +++++++++++++++--------
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 8a21342cabd6..01414c2d9565 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -403,6 +403,12 @@ struct wmax_u32_args {
u8 arg3;
};
+struct awcc_priv {
+ struct wmi_device *wdev;
+ struct platform_profile_handler pp_handler;
+ enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
+};
+
struct alienfx_priv {
struct platform_device *pdev;
struct led_classdev global_led;
@@ -423,9 +429,6 @@ struct alienfx_platdata {
struct alienfx_ops ops;
};
-static struct platform_profile_handler pp_handler;
-static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
-
static u8 interface;
static struct wmi_driver *preferred_wmi_driver;
@@ -1028,6 +1031,10 @@ static int thermal_profile_get(struct platform_profile_handler *pprof,
static int thermal_profile_set(struct platform_profile_handler *pprof,
enum platform_profile_option profile)
{
+ struct awcc_priv *priv;
+
+ priv = container_of(pprof, struct awcc_priv, pp_handler);
+
if (quirks->gmode) {
u32 gmode_status;
int ret;
@@ -1048,11 +1055,12 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
}
}
- return wmax_thermal_control(supported_thermal_profiles[profile]);
+ return wmax_thermal_control(priv->supported_thermal_profiles[profile]);
}
-static int create_thermal_profile(void)
+static int create_thermal_profile(struct wmi_device *wdev)
{
+ struct awcc_priv *priv;
u32 out_data;
u8 sys_desc[4];
u32 first_mode;
@@ -1060,6 +1068,9 @@ static int create_thermal_profile(void)
enum platform_profile_option profile;
int ret;
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+ dev_set_drvdata(&wdev->dev, priv);
+
ret = wmax_thermal_information(WMAX_OPERATION_SYS_DESCRIPTION,
0, (u32 *) &sys_desc);
if (ret < 0)
@@ -1082,25 +1093,25 @@ static int create_thermal_profile(void)
mode = out_data & WMAX_THERMAL_MODE_MASK;
profile = wmax_mode_to_platform_profile[mode];
- supported_thermal_profiles[profile] = out_data;
+ priv->supported_thermal_profiles[profile] = out_data;
- set_bit(profile, pp_handler.choices);
+ set_bit(profile, priv->pp_handler.choices);
}
- if (bitmap_empty(pp_handler.choices, PLATFORM_PROFILE_LAST))
+ if (bitmap_empty(priv->pp_handler.choices, PLATFORM_PROFILE_LAST))
return -ENODEV;
if (quirks->gmode) {
- supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
+ priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
WMAX_THERMAL_MODE_GMODE;
- set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->pp_handler.choices);
}
- pp_handler.profile_get = thermal_profile_get;
- pp_handler.profile_set = thermal_profile_set;
+ priv->pp_handler.profile_get = thermal_profile_get;
+ priv->pp_handler.profile_set = thermal_profile_set;
- return platform_profile_register(&pp_handler);
+ return platform_profile_register(&priv->pp_handler);
}
static void remove_thermal_profile(void)
@@ -1304,7 +1315,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
};
if (quirks->thermal)
- ret = create_thermal_profile();
+ ret = create_thermal_profile(wdev);
else if (quirks->num_zones > 0)
ret = alienfx_wmi_init(&pdata);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 14/21] alienware-wmi: Migrate thermal methods to wmidev
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (12 preceding siblings ...)
2024-12-05 0:44 ` [RFC PATCH 13/21] alienware-wmi: Add a state container for AWCC Kurt Borja
@ 2024-12-05 0:44 ` Kurt Borja
2024-12-05 0:44 ` [RFC PATCH 15/21] alienware-wmi: Refactor sysfs visibility methods Kurt Borja
` (7 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:44 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Drop alienware_wmax_command() and refactor "platform_profile" methods to
use alienware_wmi_command() instead.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 71 ++++++++---------------
1 file changed, 25 insertions(+), 46 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 01414c2d9565..18a15dcf90a3 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -650,34 +650,6 @@ static enum led_brightness global_led_get(struct led_classdev *led_cdev)
return priv->global_brightness;
}
-static acpi_status alienware_wmax_command(void *in_args, size_t in_size,
- u32 command, u32 *out_data)
-{
- acpi_status status;
- union acpi_object *obj;
- struct acpi_buffer input;
- struct acpi_buffer output;
-
- input.length = in_size;
- input.pointer = in_args;
- if (out_data) {
- output.length = ACPI_ALLOCATE_BUFFER;
- output.pointer = NULL;
- status = wmi_evaluate_method(WMAX_CONTROL_GUID, 0,
- command, &input, &output);
- if (ACPI_SUCCESS(status)) {
- obj = (union acpi_object *)output.pointer;
- if (obj && obj->type == ACPI_TYPE_INTEGER)
- *out_data = (u32)obj->integer.value;
- }
- kfree(output.pointer);
- } else {
- status = wmi_evaluate_method(WMAX_CONTROL_GUID, 0,
- command, &input, NULL);
- }
- return status;
-}
-
/*
* The HDMI mux sysfs node indicates the status of the HDMI input mux.
* It can toggle between standard system GPU output and HDMI input.
@@ -932,7 +904,8 @@ static bool is_wmax_thermal_code(u32 code)
return false;
}
-static int wmax_thermal_information(u8 operation, u8 arg, u32 *out_data)
+static int wmax_thermal_information(struct wmi_device *wdev, u8 operation,
+ u8 arg, u32 *out_data)
{
acpi_status status;
struct wmax_u32_args in_args = {
@@ -942,9 +915,8 @@ static int wmax_thermal_information(u8 operation, u8 arg, u32 *out_data)
.arg3 = 0,
};
- status = alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_THERMAL_INFORMATION,
- out_data);
+ status = alienware_wmi_command(wdev, WMAX_METHOD_THERMAL_INFORMATION,
+ &in_args, sizeof(in_args), out_data);
if (ACPI_FAILURE(status))
return -EIO;
@@ -955,7 +927,7 @@ static int wmax_thermal_information(u8 operation, u8 arg, u32 *out_data)
return 0;
}
-static int wmax_thermal_control(u8 profile)
+static int wmax_thermal_control(struct wmi_device *wdev, u8 profile)
{
acpi_status status;
struct wmax_u32_args in_args = {
@@ -966,9 +938,8 @@ static int wmax_thermal_control(u8 profile)
};
u32 out_data;
- status = alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_THERMAL_CONTROL,
- &out_data);
+ status = alienware_wmi_command(wdev, WMAX_METHOD_THERMAL_CONTROL,
+ &in_args, sizeof(in_args), &out_data);
if (ACPI_FAILURE(status))
return -EIO;
@@ -979,7 +950,8 @@ static int wmax_thermal_control(u8 profile)
return 0;
}
-static int wmax_game_shift_status(u8 operation, u32 *out_data)
+static int wmax_game_shift_status(struct wmi_device *wdev, u8 operation,
+ u32 *out_data)
{
acpi_status status;
struct wmax_u32_args in_args = {
@@ -989,9 +961,8 @@ static int wmax_game_shift_status(u8 operation, u32 *out_data)
.arg3 = 0,
};
- status = alienware_wmax_command(&in_args, sizeof(in_args),
- WMAX_METHOD_GAME_SHIFT_STATUS,
- out_data);
+ status = alienware_wmi_command(wdev, WMAX_METHOD_GAME_SHIFT_STATUS,
+ &in_args, sizeof(in_args), out_data);
if (ACPI_FAILURE(status))
return -EIO;
@@ -1005,10 +976,13 @@ static int wmax_game_shift_status(u8 operation, u32 *out_data)
static int thermal_profile_get(struct platform_profile_handler *pprof,
enum platform_profile_option *profile)
{
+ struct awcc_priv *priv;
u32 out_data;
int ret;
- ret = wmax_thermal_information(WMAX_OPERATION_CURRENT_PROFILE,
+ priv = container_of(pprof, struct awcc_priv, pp_handler);
+
+ ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_CURRENT_PROFILE,
0, &out_data);
if (ret < 0)
@@ -1039,7 +1013,8 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
u32 gmode_status;
int ret;
- ret = wmax_game_shift_status(WMAX_OPERATION_GET_GAME_SHIFT_STATUS,
+ ret = wmax_game_shift_status(priv->wdev,
+ WMAX_OPERATION_GET_GAME_SHIFT_STATUS,
&gmode_status);
if (ret < 0)
@@ -1047,7 +1022,8 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
if ((profile == PLATFORM_PROFILE_PERFORMANCE && !gmode_status) ||
(profile != PLATFORM_PROFILE_PERFORMANCE && gmode_status)) {
- ret = wmax_game_shift_status(WMAX_OPERATION_TOGGLE_GAME_SHIFT,
+ ret = wmax_game_shift_status(priv->wdev,
+ WMAX_OPERATION_TOGGLE_GAME_SHIFT,
&gmode_status);
if (ret < 0)
@@ -1055,7 +1031,8 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
}
}
- return wmax_thermal_control(priv->supported_thermal_profiles[profile]);
+ return wmax_thermal_control(priv->wdev,
+ priv->supported_thermal_profiles[profile]);
}
static int create_thermal_profile(struct wmi_device *wdev)
@@ -1071,7 +1048,9 @@ static int create_thermal_profile(struct wmi_device *wdev)
priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
dev_set_drvdata(&wdev->dev, priv);
- ret = wmax_thermal_information(WMAX_OPERATION_SYS_DESCRIPTION,
+ priv->wdev = wdev;
+
+ ret = wmax_thermal_information(wdev, WMAX_OPERATION_SYS_DESCRIPTION,
0, (u32 *) &sys_desc);
if (ret < 0)
return ret;
@@ -1079,7 +1058,7 @@ static int create_thermal_profile(struct wmi_device *wdev)
first_mode = sys_desc[0] + sys_desc[1];
for (u32 i = 0; i < sys_desc[3]; i++) {
- ret = wmax_thermal_information(WMAX_OPERATION_LIST_IDS,
+ ret = wmax_thermal_information(wdev, WMAX_OPERATION_LIST_IDS,
i + first_mode, &out_data);
if (ret == -EIO)
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 15/21] alienware-wmi: Refactor sysfs visibility methods
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (13 preceding siblings ...)
2024-12-05 0:44 ` [RFC PATCH 14/21] alienware-wmi: Migrate thermal methods to wmidev Kurt Borja
@ 2024-12-05 0:44 ` Kurt Borja
2024-12-05 0:45 ` [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata Kurt Borja
` (6 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:44 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
All sysfs visibility methods relied on the `quirks` global variable. To
avoid this let the WMI drivers set this information as platform specific
data, and refactor visibility methods accordingly.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 52 ++++++++++++++++++++---
1 file changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 18a15dcf90a3..25e0139ed78c 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -427,6 +427,10 @@ struct alienfx_ops {
struct alienfx_platdata {
struct wmi_device *wdev;
struct alienfx_ops ops;
+ u8 num_zones;
+ bool hdmi_mux;
+ bool amplifier;
+ bool deepslp;
};
static u8 interface;
@@ -597,12 +601,24 @@ static DEVICE_ATTR_RW(lighting_control_state);
static umode_t zone_attr_visible(struct kobject *kobj,
struct attribute *attr, int n)
{
- return n < quirks->num_zones + 1 ? 0644 : 0;
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return n < pdata->num_zones + 1 ? 0644 : 0;
}
static bool zone_group_visible(struct kobject *kobj)
{
- return quirks->num_zones > 0;
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->num_zones > 0;
}
DEFINE_SYSFS_GROUP_VISIBLE(zone);
@@ -737,7 +753,13 @@ static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
static bool hdmi_group_visible(struct kobject *kobj)
{
- return quirks->hdmi_mux;
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->hdmi_mux;
}
DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
@@ -786,7 +808,13 @@ static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
static bool amplifier_group_visible(struct kobject *kobj)
{
- return quirks->amplifier;
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->amplifier;
}
DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
@@ -862,7 +890,13 @@ static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_d
static bool deepsleep_group_visible(struct kobject *kobj)
{
- return quirks->deepslp;
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->deepslp;
}
DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
@@ -1211,6 +1245,10 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
.upd_led = legacy_wmi_update_led,
.upd_brightness = legacy_wmi_update_brightness,
},
+ .num_zones = quirks->num_zones,
+ .hdmi_mux = quirks->hdmi_mux,
+ .amplifier = quirks->amplifier,
+ .deepslp = quirks->deepslp,
};
if (quirks->num_zones > 0)
@@ -1291,6 +1329,10 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
.upd_led = wmax_wmi_update_led,
.upd_brightness = wmax_wmi_update_brightness,
},
+ .num_zones = quirks->num_zones,
+ .hdmi_mux = quirks->hdmi_mux,
+ .amplifier = quirks->amplifier,
+ .deepslp = quirks->deepslp,
};
if (quirks->thermal)
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (14 preceding siblings ...)
2024-12-05 0:44 ` [RFC PATCH 15/21] alienware-wmi: Refactor sysfs visibility methods Kurt Borja
@ 2024-12-05 0:45 ` Kurt Borja
2024-12-05 11:32 ` Ilpo Järvinen
2024-12-05 0:46 ` [RFC PATCH 17/21] alienware-wmi: Drop thermal methods dependency on quirks Kurt Borja
` (5 subsequent siblings)
21 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:45 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Both WMI devices have a different "RUNNING" control state code. Make the
WMI drivers decide which code to use, and refactor sysfs methods
accordingly.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 25e0139ed78c..fa21a50d66bd 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -431,6 +431,7 @@ struct alienfx_platdata {
bool hdmi_mux;
bool amplifier;
bool deepslp;
+ u8 running_code;
};
static u8 interface;
@@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
const char *buf, size_t count)
{
struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
u8 val;
priv = dev_get_drvdata(dev);
+ pdata = dev_get_platdata(dev);
if (strcmp(buf, "booting\n") == 0)
val = LEGACY_BOOTING;
else if (strcmp(buf, "suspend\n") == 0)
val = LEGACY_SUSPEND;
- else if (interface == LEGACY)
- val = LEGACY_RUNNING;
else
- val = WMAX_RUNNING;
+ val = pdata->running_code;
priv->lighting_control_state = val;
pr_debug("alienware-wmi: updated control state to %d\n",
@@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
.hdmi_mux = quirks->hdmi_mux,
.amplifier = quirks->amplifier,
.deepslp = quirks->deepslp,
+ .running_code = LEGACY_RUNNING,
};
if (quirks->num_zones > 0)
@@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
.hdmi_mux = quirks->hdmi_mux,
.amplifier = quirks->amplifier,
.deepslp = quirks->deepslp,
+ .running_code = WMAX_RUNNING,
};
if (quirks->thermal)
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata
2024-12-05 0:45 ` [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata Kurt Borja
@ 2024-12-05 11:32 ` Ilpo Järvinen
2024-12-05 13:10 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 11:32 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Wed, 4 Dec 2024, Kurt Borja wrote:
> Both WMI devices have a different "RUNNING" control state code. Make the
> WMI drivers decide which code to use, and refactor sysfs methods
> accordingly.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 25e0139ed78c..fa21a50d66bd 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -431,6 +431,7 @@ struct alienfx_platdata {
> bool hdmi_mux;
> bool amplifier;
> bool deepslp;
> + u8 running_code;
> };
>
> static u8 interface;
> @@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
> const char *buf, size_t count)
> {
> struct alienfx_priv *priv;
> + struct alienfx_platdata *pdata;
> u8 val;
>
> priv = dev_get_drvdata(dev);
> + pdata = dev_get_platdata(dev);
>
> if (strcmp(buf, "booting\n") == 0)
> val = LEGACY_BOOTING;
> else if (strcmp(buf, "suspend\n") == 0)
> val = LEGACY_SUSPEND;
> - else if (interface == LEGACY)
> - val = LEGACY_RUNNING;
> else
> - val = WMAX_RUNNING;
> + val = pdata->running_code;
>
> priv->lighting_control_state = val;
> pr_debug("alienware-wmi: updated control state to %d\n",
> @@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> .hdmi_mux = quirks->hdmi_mux,
> .amplifier = quirks->amplifier,
> .deepslp = quirks->deepslp,
> + .running_code = LEGACY_RUNNING,
> };
>
> if (quirks->num_zones > 0)
> @@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> .hdmi_mux = quirks->hdmi_mux,
> .amplifier = quirks->amplifier,
> .deepslp = quirks->deepslp,
> + .running_code = WMAX_RUNNING,
> };
>
> if (quirks->thermal)
>
I've not taken extensive look at interdependent changes in the series but
while reviewing patch 20/21 I noticed that alienfx_probe() changed there
from:
- if (interface == WMAX)
- priv->lighting_control_state = WMAX_RUNNING;
- else if (interface == LEGACY)
- priv->lighting_control_state = LEGACY_RUNNING;
to:
+ priv->lighting_control_state = pdata->running_code;
Somehow, it felt odd and then looking this patch 16, it felt even odder.
Perhaps there's a good reason my review that didn't go deep enough failed
to uncover but please check if this is an indication that something is a
miss in the series.
--
i.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata
2024-12-05 11:32 ` Ilpo Järvinen
@ 2024-12-05 13:10 ` Kurt Borja
2024-12-05 14:06 ` Ilpo Järvinen
0 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 13:10 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 01:32:46PM +0200, Ilpo Järvinen wrote:
> On Wed, 4 Dec 2024, Kurt Borja wrote:
>
> > Both WMI devices have a different "RUNNING" control state code. Make the
> > WMI drivers decide which code to use, and refactor sysfs methods
> > accordingly.
> >
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> > drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index 25e0139ed78c..fa21a50d66bd 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -431,6 +431,7 @@ struct alienfx_platdata {
> > bool hdmi_mux;
> > bool amplifier;
> > bool deepslp;
> > + u8 running_code;
> > };
> >
> > static u8 interface;
> > @@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
> > const char *buf, size_t count)
> > {
> > struct alienfx_priv *priv;
> > + struct alienfx_platdata *pdata;
> > u8 val;
> >
> > priv = dev_get_drvdata(dev);
> > + pdata = dev_get_platdata(dev);
> >
> > if (strcmp(buf, "booting\n") == 0)
> > val = LEGACY_BOOTING;
> > else if (strcmp(buf, "suspend\n") == 0)
> > val = LEGACY_SUSPEND;
> > - else if (interface == LEGACY)
> > - val = LEGACY_RUNNING;
> > else
> > - val = WMAX_RUNNING;
> > + val = pdata->running_code;
> >
> > priv->lighting_control_state = val;
> > pr_debug("alienware-wmi: updated control state to %d\n",
> > @@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> > .hdmi_mux = quirks->hdmi_mux,
> > .amplifier = quirks->amplifier,
> > .deepslp = quirks->deepslp,
> > + .running_code = LEGACY_RUNNING,
> > };
> >
> > if (quirks->num_zones > 0)
> > @@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> > .hdmi_mux = quirks->hdmi_mux,
> > .amplifier = quirks->amplifier,
> > .deepslp = quirks->deepslp,
> > + .running_code = WMAX_RUNNING,
> > };
> >
> > if (quirks->thermal)
> >
>
> I've not taken extensive look at interdependent changes in the series but
> while reviewing patch 20/21 I noticed that alienfx_probe() changed there
> from:
>
> - if (interface == WMAX)
> - priv->lighting_control_state = WMAX_RUNNING;
> - else if (interface == LEGACY)
> - priv->lighting_control_state = LEGACY_RUNNING;
>
> to:
>
> + priv->lighting_control_state = pdata->running_code;
This was a workaround. I forgot to add this change in this patch. Also I'll
move this earlier in the series.
>
> Somehow, it felt odd and then looking this patch 16, it felt even odder.
The reason behind this is that I don't want AlienFX related methods
depending on global variables like `interface` so I can split the file
cleanly.
> Perhaps there's a good reason my review that didn't go deep enough failed
> to uncover but please check if this is an indication that something is a
> miss in the series.
>
> --
> i.
Thank you so much for commenting on the series!
Also, what do you think about the general approach? I was a bit unsure
about the whole "platdata" stuff, as not many drivers use it in this
subsystem.
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata
2024-12-05 13:10 ` Kurt Borja
@ 2024-12-05 14:06 ` Ilpo Järvinen
2024-12-07 2:10 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 14:06 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
[-- Attachment #1: Type: text/plain, Size: 4270 bytes --]
On Thu, 5 Dec 2024, Kurt Borja wrote:
> On Thu, Dec 05, 2024 at 01:32:46PM +0200, Ilpo Järvinen wrote:
> > On Wed, 4 Dec 2024, Kurt Borja wrote:
> >
> > > Both WMI devices have a different "RUNNING" control state code. Make the
> > > WMI drivers decide which code to use, and refactor sysfs methods
> > > accordingly.
> > >
> > > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > > ---
> > > drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
> > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > > index 25e0139ed78c..fa21a50d66bd 100644
> > > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > > @@ -431,6 +431,7 @@ struct alienfx_platdata {
> > > bool hdmi_mux;
> > > bool amplifier;
> > > bool deepslp;
> > > + u8 running_code;
> > > };
> > >
> > > static u8 interface;
> > > @@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
> > > const char *buf, size_t count)
> > > {
> > > struct alienfx_priv *priv;
> > > + struct alienfx_platdata *pdata;
> > > u8 val;
> > >
> > > priv = dev_get_drvdata(dev);
> > > + pdata = dev_get_platdata(dev);
> > >
> > > if (strcmp(buf, "booting\n") == 0)
> > > val = LEGACY_BOOTING;
> > > else if (strcmp(buf, "suspend\n") == 0)
> > > val = LEGACY_SUSPEND;
> > > - else if (interface == LEGACY)
> > > - val = LEGACY_RUNNING;
> > > else
> > > - val = WMAX_RUNNING;
> > > + val = pdata->running_code;
> > >
> > > priv->lighting_control_state = val;
> > > pr_debug("alienware-wmi: updated control state to %d\n",
> > > @@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> > > .hdmi_mux = quirks->hdmi_mux,
> > > .amplifier = quirks->amplifier,
> > > .deepslp = quirks->deepslp,
> > > + .running_code = LEGACY_RUNNING,
> > > };
> > >
> > > if (quirks->num_zones > 0)
> > > @@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> > > .hdmi_mux = quirks->hdmi_mux,
> > > .amplifier = quirks->amplifier,
> > > .deepslp = quirks->deepslp,
> > > + .running_code = WMAX_RUNNING,
> > > };
> > >
> > > if (quirks->thermal)
> > >
> >
> > I've not taken extensive look at interdependent changes in the series but
> > while reviewing patch 20/21 I noticed that alienfx_probe() changed there
> > from:
> >
> > - if (interface == WMAX)
> > - priv->lighting_control_state = WMAX_RUNNING;
> > - else if (interface == LEGACY)
> > - priv->lighting_control_state = LEGACY_RUNNING;
> >
> > to:
> >
> > + priv->lighting_control_state = pdata->running_code;
>
> This was a workaround. I forgot to add this change in this patch. Also I'll
> move this earlier in the series.
>
> >
> > Somehow, it felt odd and then looking this patch 16, it felt even odder.
>
> The reason behind this is that I don't want AlienFX related methods
> depending on global variables like `interface` so I can split the file
> cleanly.
Just to make sure we don't talk past each other, I didn't mean the code
itself felt odd, just the patch it was changed in. So if your plan was to
have it in this patch to begin with but you noticed the problem while
doing the other change and put it "temporarily" there and forgot to move
it into correct place, no oddity problem then. :-)
> > Perhaps there's a good reason my review that didn't go deep enough failed
> > to uncover but please check if this is an indication that something is a
> > miss in the series.
> >
>
> Thank you so much for commenting on the series!
>
> Also, what do you think about the general approach? I was a bit unsure
> about the whole "platdata" stuff, as not many drivers use it in this
> subsystem.
I'm sorry but I've not come across it much so I'm not sure if my opinion
is that valuable.
What I can say for sure though is that the wide-spread use of global-level
variables in pdx86 drivers is definitely not setting a good example.
--
i.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata
2024-12-05 14:06 ` Ilpo Järvinen
@ 2024-12-07 2:10 ` Kurt Borja
0 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-07 2:10 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 04:06:23PM +0200, Ilpo Järvinen wrote:
> On Thu, 5 Dec 2024, Kurt Borja wrote:
>
> > On Thu, Dec 05, 2024 at 01:32:46PM +0200, Ilpo Järvinen wrote:
> > > On Wed, 4 Dec 2024, Kurt Borja wrote:
> > >
> > > > Both WMI devices have a different "RUNNING" control state code. Make the
> > > > WMI drivers decide which code to use, and refactor sysfs methods
> > > > accordingly.
> > > >
> > > > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > > > ---
> > > > drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
> > > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > > > index 25e0139ed78c..fa21a50d66bd 100644
> > > > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > > > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > > > @@ -431,6 +431,7 @@ struct alienfx_platdata {
> > > > bool hdmi_mux;
> > > > bool amplifier;
> > > > bool deepslp;
> > > > + u8 running_code;
> > > > };
> > > >
> > > > static u8 interface;
> > > > @@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
> > > > const char *buf, size_t count)
> > > > {
> > > > struct alienfx_priv *priv;
> > > > + struct alienfx_platdata *pdata;
> > > > u8 val;
> > > >
> > > > priv = dev_get_drvdata(dev);
> > > > + pdata = dev_get_platdata(dev);
> > > >
> > > > if (strcmp(buf, "booting\n") == 0)
> > > > val = LEGACY_BOOTING;
> > > > else if (strcmp(buf, "suspend\n") == 0)
> > > > val = LEGACY_SUSPEND;
> > > > - else if (interface == LEGACY)
> > > > - val = LEGACY_RUNNING;
> > > > else
> > > > - val = WMAX_RUNNING;
> > > > + val = pdata->running_code;
> > > >
> > > > priv->lighting_control_state = val;
> > > > pr_debug("alienware-wmi: updated control state to %d\n",
> > > > @@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> > > > .hdmi_mux = quirks->hdmi_mux,
> > > > .amplifier = quirks->amplifier,
> > > > .deepslp = quirks->deepslp,
> > > > + .running_code = LEGACY_RUNNING,
> > > > };
> > > >
> > > > if (quirks->num_zones > 0)
> > > > @@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> > > > .hdmi_mux = quirks->hdmi_mux,
> > > > .amplifier = quirks->amplifier,
> > > > .deepslp = quirks->deepslp,
> > > > + .running_code = WMAX_RUNNING,
> > > > };
> > > >
> > > > if (quirks->thermal)
> > > >
> > >
> > > I've not taken extensive look at interdependent changes in the series but
> > > while reviewing patch 20/21 I noticed that alienfx_probe() changed there
> > > from:
> > >
> > > - if (interface == WMAX)
> > > - priv->lighting_control_state = WMAX_RUNNING;
> > > - else if (interface == LEGACY)
> > > - priv->lighting_control_state = LEGACY_RUNNING;
> > >
> > > to:
> > >
> > > + priv->lighting_control_state = pdata->running_code;
> >
> > This was a workaround. I forgot to add this change in this patch. Also I'll
> > move this earlier in the series.
> >
> > >
> > > Somehow, it felt odd and then looking this patch 16, it felt even odder.
> >
> > The reason behind this is that I don't want AlienFX related methods
> > depending on global variables like `interface` so I can split the file
> > cleanly.
>
> Just to make sure we don't talk past each other, I didn't mean the code
> itself felt odd, just the patch it was changed in. So if your plan was to
> have it in this patch to begin with but you noticed the problem while
> doing the other change and put it "temporarily" there and forgot to move
> it into correct place, no oddity problem then. :-)
No worries, I explained my intention just in case it was confusing.
Thank you for clarifying :)
>
> > > Perhaps there's a good reason my review that didn't go deep enough failed
> > > to uncover but please check if this is an indication that something is a
> > > miss in the series.
> > >
> >
> > Thank you so much for commenting on the series!
> >
> > Also, what do you think about the general approach? I was a bit unsure
> > about the whole "platdata" stuff, as not many drivers use it in this
> > subsystem.
>
> I'm sorry but I've not come across it much so I'm not sure if my opinion
> is that valuable.
>
> What I can say for sure though is that the wide-spread use of global-level
> variables in pdx86 drivers is definitely not setting a good example.
In that case I'll stick with it for now!
~ Kurt
>
> --
> i.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 17/21] alienware-wmi: Drop thermal methods dependency on quirks
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (15 preceding siblings ...)
2024-12-05 0:45 ` [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata Kurt Borja
@ 2024-12-05 0:46 ` Kurt Borja
2024-12-05 11:14 ` Ilpo Järvinen
2024-12-05 0:46 ` [RFC PATCH 18/21] platform-x86: Add header file for alienware-wmi Kurt Borja
` (4 subsequent siblings)
21 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:46 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Make the `gmode` condition on AWCC WMI devices part of the device's
state container, and refactor "platform_profile" methods accordingly.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index fa21a50d66bd..7137995d5983 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -407,6 +407,7 @@ struct awcc_priv {
struct wmi_device *wdev;
struct platform_profile_handler pp_handler;
enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
+ bool has_gmode;
};
struct alienfx_priv {
@@ -1044,7 +1045,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
priv = container_of(pprof, struct awcc_priv, pp_handler);
- if (quirks->gmode) {
+ if (priv->has_gmode) {
u32 gmode_status;
int ret;
@@ -1070,7 +1071,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
priv->supported_thermal_profiles[profile]);
}
-static int create_thermal_profile(struct wmi_device *wdev)
+static int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
{
struct awcc_priv *priv;
u32 out_data;
@@ -1115,7 +1116,8 @@ static int create_thermal_profile(struct wmi_device *wdev)
if (bitmap_empty(priv->pp_handler.choices, PLATFORM_PROFILE_LAST))
return -ENODEV;
- if (quirks->gmode) {
+ if (has_gmode) {
+ priv->has_gmode = true;
priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
WMAX_THERMAL_MODE_GMODE;
@@ -1130,8 +1132,7 @@ static int create_thermal_profile(struct wmi_device *wdev)
static void remove_thermal_profile(void)
{
- if (quirks->thermal)
- platform_profile_remove();
+ platform_profile_remove();
}
/*
@@ -1339,7 +1340,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
};
if (quirks->thermal)
- ret = create_thermal_profile(wdev);
+ ret = create_thermal_profile(wdev, quirks->gmode);
else if (quirks->num_zones > 0)
ret = alienfx_wmi_init(&pdata);
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH 17/21] alienware-wmi: Drop thermal methods dependency on quirks
2024-12-05 0:46 ` [RFC PATCH 17/21] alienware-wmi: Drop thermal methods dependency on quirks Kurt Borja
@ 2024-12-05 11:14 ` Ilpo Järvinen
2024-12-05 12:56 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 11:14 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Wed, 4 Dec 2024, Kurt Borja wrote:
> Make the `gmode` condition on AWCC WMI devices part of the device's
> state container, and refactor "platform_profile" methods accordingly.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> drivers/platform/x86/dell/alienware-wmi.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index fa21a50d66bd..7137995d5983 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -407,6 +407,7 @@ struct awcc_priv {
> struct wmi_device *wdev;
> struct platform_profile_handler pp_handler;
> enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
> + bool has_gmode;
> };
>
> struct alienfx_priv {
> @@ -1044,7 +1045,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
>
> priv = container_of(pprof, struct awcc_priv, pp_handler);
>
> - if (quirks->gmode) {
> + if (priv->has_gmode) {
> u32 gmode_status;
> int ret;
>
> @@ -1070,7 +1071,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
> priv->supported_thermal_profiles[profile]);
> }
>
> -static int create_thermal_profile(struct wmi_device *wdev)
> +static int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
> {
> struct awcc_priv *priv;
> u32 out_data;
> @@ -1115,7 +1116,8 @@ static int create_thermal_profile(struct wmi_device *wdev)
> if (bitmap_empty(priv->pp_handler.choices, PLATFORM_PROFILE_LAST))
> return -ENODEV;
>
> - if (quirks->gmode) {
> + if (has_gmode) {
> + priv->has_gmode = true;
> priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
> WMAX_THERMAL_MODE_GMODE;
>
> @@ -1130,8 +1132,7 @@ static int create_thermal_profile(struct wmi_device *wdev)
>
> static void remove_thermal_profile(void)
> {
> - if (quirks->thermal)
> - platform_profile_remove();
> + platform_profile_remove();
Did you intend to make this change in this patch?
I'm unable to confirm why the if () could now be removed and how it is
related to other parts of this patch.
> }
>
> /*
> @@ -1339,7 +1340,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> };
>
> if (quirks->thermal)
> - ret = create_thermal_profile(wdev);
> + ret = create_thermal_profile(wdev, quirks->gmode);
> else if (quirks->num_zones > 0)
> ret = alienfx_wmi_init(&pdata);
>
>
--
i.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 17/21] alienware-wmi: Drop thermal methods dependency on quirks
2024-12-05 11:14 ` Ilpo Järvinen
@ 2024-12-05 12:56 ` Kurt Borja
0 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 12:56 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 01:14:43PM +0200, Ilpo Järvinen wrote:
> On Wed, 4 Dec 2024, Kurt Borja wrote:
>
> > Make the `gmode` condition on AWCC WMI devices part of the device's
> > state container, and refactor "platform_profile" methods accordingly.
> >
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> > drivers/platform/x86/dell/alienware-wmi.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index fa21a50d66bd..7137995d5983 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -407,6 +407,7 @@ struct awcc_priv {
> > struct wmi_device *wdev;
> > struct platform_profile_handler pp_handler;
> > enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
> > + bool has_gmode;
> > };
> >
> > struct alienfx_priv {
> > @@ -1044,7 +1045,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
> >
> > priv = container_of(pprof, struct awcc_priv, pp_handler);
> >
> > - if (quirks->gmode) {
> > + if (priv->has_gmode) {
> > u32 gmode_status;
> > int ret;
> >
> > @@ -1070,7 +1071,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
> > priv->supported_thermal_profiles[profile]);
> > }
> >
> > -static int create_thermal_profile(struct wmi_device *wdev)
> > +static int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
> > {
> > struct awcc_priv *priv;
> > u32 out_data;
> > @@ -1115,7 +1116,8 @@ static int create_thermal_profile(struct wmi_device *wdev)
> > if (bitmap_empty(priv->pp_handler.choices, PLATFORM_PROFILE_LAST))
> > return -ENODEV;
> >
> > - if (quirks->gmode) {
> > + if (has_gmode) {
> > + priv->has_gmode = true;
> > priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
> > WMAX_THERMAL_MODE_GMODE;
> >
> > @@ -1130,8 +1132,7 @@ static int create_thermal_profile(struct wmi_device *wdev)
> >
> > static void remove_thermal_profile(void)
> > {
> > - if (quirks->thermal)
> > - platform_profile_remove();
> > + platform_profile_remove();
>
> Did you intend to make this change in this patch?
>
> I'm unable to confirm why the if () could now be removed and how it is
> related to other parts of this patch.
This is indeed should not be part of this patch.
In patch 08/21 wmax_wmi_remove() checks for quirks->thermal before
calling remove_thermal_profile() so the this check is safe to drop.
>
> > }
> >
> > /*
> > @@ -1339,7 +1340,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> > };
> >
> > if (quirks->thermal)
> > - ret = create_thermal_profile(wdev);
> > + ret = create_thermal_profile(wdev, quirks->gmode);
> > else if (quirks->num_zones > 0)
> > ret = alienfx_wmi_init(&pdata);
> >
> >
>
> --
> i.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 18/21] platform-x86: Add header file for alienware-wmi
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (16 preceding siblings ...)
2024-12-05 0:46 ` [RFC PATCH 17/21] alienware-wmi: Drop thermal methods dependency on quirks Kurt Borja
@ 2024-12-05 0:46 ` Kurt Borja
2024-12-05 0:47 ` [RFC PATCH 19/21] platform-x86: Rename alienare-wmi Kurt Borja
` (3 subsequent siblings)
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:46 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
In order to split the alienware-wmi module, add a header file for all
shared assets.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/alienware-wmi.c | 106 ++-----------------
drivers/platform/x86/dell/alienware-wmi.h | 119 ++++++++++++++++++++++
2 files changed, 126 insertions(+), 99 deletions(-)
create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 7137995d5983..4165eb0d0bf5 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -16,19 +16,8 @@
#include <linux/dmi.h>
#include <linux/leds.h>
#include <linux/wmi.h>
+#include "alienware-wmi.h"
-#define LEGACY_CONTROL_GUID "A90597CE-A997-11DA-B012-B622A1EF5492"
-#define LEGACY_POWER_CONTROL_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
-#define WMAX_CONTROL_GUID "A70591CE-A997-11DA-B012-B622A1EF5492"
-
-#define WMAX_METHOD_HDMI_SOURCE 0x1
-#define WMAX_METHOD_HDMI_STATUS 0x2
-#define WMAX_METHOD_BRIGHTNESS 0x3
-#define WMAX_METHOD_ZONE_CONTROL 0x4
-#define WMAX_METHOD_HDMI_CABLE 0x5
-#define WMAX_METHOD_AMPLIFIER_CABLE 0x6
-#define WMAX_METHOD_DEEP_SLEEP_CONTROL 0x0B
-#define WMAX_METHOD_DEEP_SLEEP_STATUS 0x0C
#define WMAX_METHOD_THERMAL_INFORMATION 0x14
#define WMAX_METHOD_THERMAL_CONTROL 0x15
#define WMAX_METHOD_GAME_SHIFT_STATUS 0x25
@@ -54,18 +43,6 @@ enum INTERFACE_FLAGS {
WMAX,
};
-enum LEGACY_CONTROL_STATES {
- LEGACY_RUNNING = 1,
- LEGACY_BOOTING = 0,
- LEGACY_SUSPEND = 3,
-};
-
-enum WMAX_CONTROL_STATES {
- WMAX_RUNNING = 0xFF,
- WMAX_BOOTING = 0,
- WMAX_SUSPEND = 3,
-};
-
enum WMAX_THERMAL_INFORMATION_OPERATIONS {
WMAX_OPERATION_SYS_DESCRIPTION = 0x02,
WMAX_OPERATION_LIST_IDS = 0x03,
@@ -86,20 +63,6 @@ enum WMAX_THERMAL_TABLES {
WMAX_THERMAL_TABLE_USTT = 0xA0,
};
-enum wmax_thermal_mode {
- THERMAL_MODE_USTT_BALANCED,
- THERMAL_MODE_USTT_BALANCED_PERFORMANCE,
- THERMAL_MODE_USTT_COOL,
- THERMAL_MODE_USTT_QUIET,
- THERMAL_MODE_USTT_PERFORMANCE,
- THERMAL_MODE_USTT_LOW_POWER,
- THERMAL_MODE_BASIC_QUIET,
- THERMAL_MODE_BASIC_BALANCED,
- THERMAL_MODE_BASIC_BALANCED_PERFORMANCE,
- THERMAL_MODE_BASIC_PERFORMANCE,
- THERMAL_MODE_LAST,
-};
-
static const enum platform_profile_option wmax_mode_to_platform_profile[THERMAL_MODE_LAST] = {
[THERMAL_MODE_USTT_BALANCED] = PLATFORM_PROFILE_BALANCED,
[THERMAL_MODE_USTT_BALANCED_PERFORMANCE] = PLATFORM_PROFILE_BALANCED_PERFORMANCE,
@@ -369,33 +332,10 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
{}
};
-struct color_platform {
- u8 blue;
- u8 green;
- u8 red;
-} __packed;
-
-struct wmax_brightness_args {
- u32 led_mask;
- u32 percentage;
-};
-
struct wmax_basic_args {
u8 arg;
};
-struct legacy_led_args {
- struct color_platform colors;
- u8 brightness;
- u8 state;
-} __packed;
-
-struct wmax_led_args {
- u32 led_mask;
- struct color_platform colors;
- u8 state;
-} __packed;
-
struct wmax_u32_args {
u8 operation;
u8 arg1;
@@ -403,43 +343,11 @@ struct wmax_u32_args {
u8 arg3;
};
-struct awcc_priv {
- struct wmi_device *wdev;
- struct platform_profile_handler pp_handler;
- enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
- bool has_gmode;
-};
-
-struct alienfx_priv {
- struct platform_device *pdev;
- struct led_classdev global_led;
- struct color_platform colors[4];
- u8 global_brightness;
- 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;
- u8 num_zones;
- bool hdmi_mux;
- bool amplifier;
- bool deepslp;
- u8 running_code;
-};
-
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 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;
@@ -1071,7 +979,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
priv->supported_thermal_profiles[profile]);
}
-static int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
+int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
{
struct awcc_priv *priv;
u32 out_data;
@@ -1130,7 +1038,7 @@ static int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
return platform_profile_register(&priv->pp_handler);
}
-static void remove_thermal_profile(void)
+void remove_thermal_profile(void)
{
platform_profile_remove();
}
@@ -1180,7 +1088,7 @@ static struct platform_driver platform_driver = {
.probe = alienfx_probe,
};
-static int alienfx_wmi_init(struct alienfx_platdata *pdata)
+int alienfx_wmi_init(struct alienfx_platdata *pdata)
{
struct platform_device *pdev;
@@ -1192,7 +1100,7 @@ static int alienfx_wmi_init(struct alienfx_platdata *pdata)
return PTR_ERR_OR_ZERO(pdev);
}
-static void alienfx_wmi_exit(struct wmi_device *wdev)
+void alienfx_wmi_exit(struct wmi_device *wdev)
{
struct platform_device *pdev;
diff --git a/drivers/platform/x86/dell/alienware-wmi.h b/drivers/platform/x86/dell/alienware-wmi.h
new file mode 100644
index 000000000000..1c12b8e330e2
--- /dev/null
+++ b/drivers/platform/x86/dell/alienware-wmi.h
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Alienware AlienFX control
+ *
+ * Copyright (C) 2014 Dell Inc <Dell.Client.Kernel@dell.com>
+ */
+
+#ifndef _ALIENWARE_WMI_H_
+#define _ALIENWARE_WMI_H_
+
+#include <linux/leds.h>
+#include <linux/platform_device.h>
+#include <linux/platform_profile.h>
+#include <linux/wmi.h>
+
+#define LEGACY_CONTROL_GUID "A90597CE-A997-11DA-B012-B622A1EF5492"
+#define LEGACY_POWER_CONTROL_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+#define WMAX_CONTROL_GUID "A70591CE-A997-11DA-B012-B622A1EF5492"
+
+#define WMAX_METHOD_HDMI_SOURCE 0x1
+#define WMAX_METHOD_HDMI_STATUS 0x2
+#define WMAX_METHOD_BRIGHTNESS 0x3
+#define WMAX_METHOD_ZONE_CONTROL 0x4
+#define WMAX_METHOD_HDMI_CABLE 0x5
+#define WMAX_METHOD_AMPLIFIER_CABLE 0x6
+#define WMAX_METHOD_DEEP_SLEEP_CONTROL 0x0B
+#define WMAX_METHOD_DEEP_SLEEP_STATUS 0x0C
+
+enum LEGACY_CONTROL_STATES {
+ LEGACY_RUNNING = 1,
+ LEGACY_BOOTING = 0,
+ LEGACY_SUSPEND = 3,
+};
+
+enum WMAX_CONTROL_STATES {
+ WMAX_RUNNING = 0xFF,
+ WMAX_BOOTING = 0,
+ WMAX_SUSPEND = 3,
+};
+
+enum wmax_thermal_mode {
+ THERMAL_MODE_USTT_BALANCED,
+ THERMAL_MODE_USTT_BALANCED_PERFORMANCE,
+ THERMAL_MODE_USTT_COOL,
+ THERMAL_MODE_USTT_QUIET,
+ THERMAL_MODE_USTT_PERFORMANCE,
+ THERMAL_MODE_USTT_LOW_POWER,
+ THERMAL_MODE_BASIC_QUIET,
+ THERMAL_MODE_BASIC_BALANCED,
+ THERMAL_MODE_BASIC_BALANCED_PERFORMANCE,
+ THERMAL_MODE_BASIC_PERFORMANCE,
+ THERMAL_MODE_LAST,
+};
+
+struct color_platform {
+ u8 blue;
+ u8 green;
+ u8 red;
+} __packed;
+
+struct legacy_led_args {
+ struct color_platform colors;
+ u8 brightness;
+ u8 state;
+} __packed;
+
+struct wmax_led_args {
+ u32 led_mask;
+ struct color_platform colors;
+ u8 state;
+} __packed;
+
+struct wmax_brightness_args {
+ u32 led_mask;
+ u32 percentage;
+};
+
+struct alienfx_priv {
+ struct platform_device *pdev;
+ struct led_classdev global_led;
+ struct color_platform colors[4];
+ u8 global_brightness;
+ 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;
+ u8 num_zones;
+ bool hdmi_mux;
+ bool amplifier;
+ bool deepslp;
+ u8 running_code;
+};
+
+struct awcc_priv {
+ struct wmi_device *wdev;
+ bool has_gmode;
+ struct platform_profile_handler pp_handler;
+ enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
+};
+
+acpi_status alienware_wmi_command(struct wmi_device *wdev, u32 method_id,
+ void *in_args, size_t in_size, u32 *out_data);
+
+int alienfx_wmi_init(struct alienfx_platdata *pdata);
+void alienfx_wmi_exit(struct wmi_device *wdev);
+
+int create_thermal_profile(struct wmi_device *wdev, bool has_gmode);
+void remove_thermal_profile(void);
+
+#endif
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 19/21] platform-x86: Rename alienare-wmi
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (17 preceding siblings ...)
2024-12-05 0:46 ` [RFC PATCH 18/21] platform-x86: Add header file for alienware-wmi Kurt Borja
@ 2024-12-05 0:47 ` Kurt Borja
2024-12-05 11:16 ` Ilpo Järvinen
2024-12-05 0:47 ` [RFC PATCH 20/21] platform-x86: Split the alienware-wmi module Kurt Borja
` (2 subsequent siblings)
21 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:47 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
In order to eventually split the module:
Rename alienware-wmi.c -> alienware-wmi-base.c
And modify MAINTAINERS accordingly.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
MAINTAINERS | 3 ++-
drivers/platform/x86/dell/Makefile | 1 +
.../x86/dell/{alienware-wmi.c => alienware-wmi-base.c} | 0
3 files changed, 3 insertions(+), 1 deletion(-)
rename drivers/platform/x86/dell/{alienware-wmi.c => alienware-wmi-base.c} (100%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3809931b9240..a4d02074dd91 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -787,7 +787,8 @@ ALIENWARE WMI DRIVER
L: Dell.Client.Kernel@dell.com
S: Maintained
F: Documentation/wmi/devices/alienware-wmi.rst
-F: drivers/platform/x86/dell/alienware-wmi.c
+F: drivers/platform/x86/dell/alienware-wmi-*
+F: drivers/platform/x86/dell/alienware-wmi.*
ALLEGRO DVT VIDEO IP CORE DRIVER
M: Michael Tretter <m.tretter@pengutronix.de>
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index 79d60f1bf4c1..5e7496aeb070 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -5,6 +5,7 @@
#
obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
+alienware-wmi-objs := alienware-wmi-base.o
obj-$(CONFIG_DCDBAS) += dcdbas.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi-base.c
similarity index 100%
rename from drivers/platform/x86/dell/alienware-wmi.c
rename to drivers/platform/x86/dell/alienware-wmi-base.c
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH 19/21] platform-x86: Rename alienare-wmi
2024-12-05 0:47 ` [RFC PATCH 19/21] platform-x86: Rename alienare-wmi Kurt Borja
@ 2024-12-05 11:16 ` Ilpo Järvinen
2024-12-05 12:57 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Ilpo Järvinen @ 2024-12-05 11:16 UTC (permalink / raw)
To: Kurt Borja
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Wed, 4 Dec 2024, Kurt Borja wrote:
> In order to eventually split the module:
>
> Rename alienware-wmi.c -> alienware-wmi-base.c
>
> And modify MAINTAINERS accordingly.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> MAINTAINERS | 3 ++-
> drivers/platform/x86/dell/Makefile | 1 +
> .../x86/dell/{alienware-wmi.c => alienware-wmi-base.c} | 0
> 3 files changed, 3 insertions(+), 1 deletion(-)
> rename drivers/platform/x86/dell/{alienware-wmi.c => alienware-wmi-base.c} (100%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3809931b9240..a4d02074dd91 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -787,7 +787,8 @@ ALIENWARE WMI DRIVER
> L: Dell.Client.Kernel@dell.com
> S: Maintained
> F: Documentation/wmi/devices/alienware-wmi.rst
> -F: drivers/platform/x86/dell/alienware-wmi.c
> +F: drivers/platform/x86/dell/alienware-wmi-*
> +F: drivers/platform/x86/dell/alienware-wmi.*
Why not F: drivers/platform/x86/dell/alienware-wmi*
?
> ALLEGRO DVT VIDEO IP CORE DRIVER
> M: Michael Tretter <m.tretter@pengutronix.de>
> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
> index 79d60f1bf4c1..5e7496aeb070 100644
> --- a/drivers/platform/x86/dell/Makefile
> +++ b/drivers/platform/x86/dell/Makefile
> @@ -5,6 +5,7 @@
> #
>
> obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
> +alienware-wmi-objs := alienware-wmi-base.o
> obj-$(CONFIG_DCDBAS) += dcdbas.o
> obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
> obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi-base.c
> similarity index 100%
> rename from drivers/platform/x86/dell/alienware-wmi.c
> rename to drivers/platform/x86/dell/alienware-wmi-base.c
>
--
i.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 19/21] platform-x86: Rename alienare-wmi
2024-12-05 11:16 ` Ilpo Järvinen
@ 2024-12-05 12:57 ` Kurt Borja
0 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 12:57 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Dell.Client.Kernel, Hans de Goede, LKML, mario.limonciello,
platform-driver-x86, w_armin
On Thu, Dec 05, 2024 at 01:16:59PM +0200, Ilpo Järvinen wrote:
> On Wed, 4 Dec 2024, Kurt Borja wrote:
>
> > In order to eventually split the module:
> >
> > Rename alienware-wmi.c -> alienware-wmi-base.c
> >
> > And modify MAINTAINERS accordingly.
> >
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> > MAINTAINERS | 3 ++-
> > drivers/platform/x86/dell/Makefile | 1 +
> > .../x86/dell/{alienware-wmi.c => alienware-wmi-base.c} | 0
> > 3 files changed, 3 insertions(+), 1 deletion(-)
> > rename drivers/platform/x86/dell/{alienware-wmi.c => alienware-wmi-base.c} (100%)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3809931b9240..a4d02074dd91 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -787,7 +787,8 @@ ALIENWARE WMI DRIVER
> > L: Dell.Client.Kernel@dell.com
> > S: Maintained
> > F: Documentation/wmi/devices/alienware-wmi.rst
> > -F: drivers/platform/x86/dell/alienware-wmi.c
> > +F: drivers/platform/x86/dell/alienware-wmi-*
> > +F: drivers/platform/x86/dell/alienware-wmi.*
>
> Why not F: drivers/platform/x86/dell/alienware-wmi*
Of course, my bad!
>
> ?
>
> > ALLEGRO DVT VIDEO IP CORE DRIVER
> > M: Michael Tretter <m.tretter@pengutronix.de>
> > diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
> > index 79d60f1bf4c1..5e7496aeb070 100644
> > --- a/drivers/platform/x86/dell/Makefile
> > +++ b/drivers/platform/x86/dell/Makefile
> > @@ -5,6 +5,7 @@
> > #
> >
> > obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
> > +alienware-wmi-objs := alienware-wmi-base.o
> > obj-$(CONFIG_DCDBAS) += dcdbas.o
> > obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
> > obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi-base.c
> > similarity index 100%
> > rename from drivers/platform/x86/dell/alienware-wmi.c
> > rename to drivers/platform/x86/dell/alienware-wmi-base.c
> >
>
> --
> i.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 20/21] platform-x86: Split the alienware-wmi module
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (18 preceding siblings ...)
2024-12-05 0:47 ` [RFC PATCH 19/21] platform-x86: Rename alienare-wmi Kurt Borja
@ 2024-12-05 0:47 ` Kurt Borja
2024-12-05 0:48 ` [RFC PATCH 21/21] platform-x86: Add config entries to alienware-wmi Kurt Borja
2024-12-06 23:26 ` [RFC PATCH 00/21] alienware-wmi driver rework Armin Wolf
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:47 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Split the alienware-wmi module into three files, which correspond to the
different interfaces this module exposed. The new structure roughly
works like this:
- alienware-wmi-base.c: Manages DMI quirks and will initialize the
preferred WMI driver
- alienware-wmi-alienfx.c: Manages the sysfs interface exposed by the
"alienware-wmi" platform device
- alienware-wmi-awcc.c: Manages thermal control methods present on new
Alienware and Dell devices
These three parts already worked completely independently, so the split
is done seamlessly by copying and pasting.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/Makefile | 2 +
.../platform/x86/dell/alienware-wmi-alienfx.c | 531 ++++++++++++
.../platform/x86/dell/alienware-wmi-awcc.c | 282 +++++++
.../platform/x86/dell/alienware-wmi-base.c | 794 ------------------
4 files changed, 815 insertions(+), 794 deletions(-)
create mode 100644 drivers/platform/x86/dell/alienware-wmi-alienfx.c
create mode 100644 drivers/platform/x86/dell/alienware-wmi-awcc.c
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index 5e7496aeb070..54a592fd6ae6 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -6,6 +6,8 @@
obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
alienware-wmi-objs := alienware-wmi-base.o
+alienware-wmi-y += alienware-wmi-alienfx.o
+alienware-wmi-y += alienware-wmi-awcc.o
obj-$(CONFIG_DCDBAS) += dcdbas.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
diff --git a/drivers/platform/x86/dell/alienware-wmi-alienfx.c b/drivers/platform/x86/dell/alienware-wmi-alienfx.c
new file mode 100644
index 000000000000..d0291d1ba9bb
--- /dev/null
+++ b/drivers/platform/x86/dell/alienware-wmi-alienfx.c
@@ -0,0 +1,531 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Alienware AlienFX control
+ *
+ * Copyright (C) 2014 Dell Inc <Dell.Client.Kernel@dell.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/wmi.h>
+#include "alienware-wmi.h"
+
+struct wmax_basic_args {
+ u8 arg;
+};
+
+/*
+ * Helpers used for zone control
+ */
+static int parse_rgb(const char *buf, struct color_platform *colors)
+{
+ long unsigned int rgb;
+ int ret;
+ union color_union {
+ struct color_platform cp;
+ int package;
+ } repackager;
+
+ ret = kstrtoul(buf, 16, &rgb);
+ if (ret)
+ return ret;
+
+ /* RGB triplet notation is 24-bit hexadecimal */
+ if (rgb > 0xFFFFFF)
+ return -EINVAL;
+
+ repackager.package = rgb & 0x0f0f0f0f;
+ pr_debug("alienware-wmi: r: %d g:%d b: %d\n",
+ repackager.cp.red, repackager.cp.green, repackager.cp.blue);
+ *colors = repackager.cp;
+ return 0;
+}
+
+/*
+ * Individual RGB zone control
+ */
+static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
+ char *buf, u8 location)
+{
+ struct alienfx_priv *priv;
+ struct color_platform *colors;
+
+ priv = dev_get_drvdata(dev);
+ colors = &priv->colors[location];
+
+ return sprintf(buf, "red: %d, green: %d, blue: %d\n",
+ colors->red, colors->green, colors->blue);
+
+}
+
+static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count, u8 location)
+{
+ struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
+ struct color_platform *colors;
+ int ret;
+
+ priv = dev_get_drvdata(dev);
+ pdata = dev_get_platdata(dev);
+
+ colors = &priv->colors[location];
+ ret = parse_rgb(buf, colors);
+ if (ret)
+ return ret;
+
+ ret = pdata->ops.upd_led(priv, pdata->wdev, location);
+
+ return ret ? ret : count;
+}
+
+#define ALIENWARE_ZONE_SHOW_FUNC(_num) \
+ static ssize_t zone0##_num##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+ { \
+ return zone_show(dev, attr, buf, _num); \
+ }
+
+#define ALIENWARE_ZONE_STORE_FUNC(_num) \
+ static ssize_t zone0##_num##_store(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+ { \
+ return zone_set(dev, attr, buf, count, _num); \
+ }
+
+#define ALIENWARE_ZONE_ATTR(_num) \
+ ALIENWARE_ZONE_SHOW_FUNC(_num) \
+ ALIENWARE_ZONE_STORE_FUNC(_num) \
+ static DEVICE_ATTR_RW(zone0##_num)
+
+ALIENWARE_ZONE_ATTR(0);
+ALIENWARE_ZONE_ATTR(1);
+ALIENWARE_ZONE_ATTR(2);
+ALIENWARE_ZONE_ATTR(3);
+
+/*
+ * Lighting control state device attribute (Global)
+ */
+static ssize_t lighting_control_state_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct alienfx_priv *priv;
+
+ priv = dev_get_drvdata(dev);
+
+ if (priv->lighting_control_state == LEGACY_BOOTING)
+ return sysfs_emit(buf, "[booting] running suspend\n");
+ else if (priv->lighting_control_state == LEGACY_SUSPEND)
+ return sysfs_emit(buf, "booting running [suspend]\n");
+ return sysfs_emit(buf, "booting [running] suspend\n");
+}
+
+static ssize_t lighting_control_state_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
+ u8 val;
+
+ priv = dev_get_drvdata(dev);
+ pdata = dev_get_platdata(dev);
+
+ if (strcmp(buf, "booting\n") == 0)
+ val = LEGACY_BOOTING;
+ else if (strcmp(buf, "suspend\n") == 0)
+ val = LEGACY_SUSPEND;
+ else
+ val = pdata->running_code;
+
+ priv->lighting_control_state = val;
+ pr_debug("alienware-wmi: updated control state to %d\n",
+ priv->lighting_control_state);
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(lighting_control_state);
+
+static umode_t zone_attr_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return n < pdata->num_zones + 1 ? 0644 : 0;
+}
+
+static bool zone_group_visible(struct kobject *kobj)
+{
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->num_zones > 0;
+}
+DEFINE_SYSFS_GROUP_VISIBLE(zone);
+
+static struct attribute *zone_attrs[] = {
+ &dev_attr_lighting_control_state.attr,
+ &dev_attr_zone00.attr,
+ &dev_attr_zone01.attr,
+ &dev_attr_zone02.attr,
+ &dev_attr_zone03.attr,
+ NULL
+};
+
+static struct attribute_group zone_attribute_group = {
+ .name = "rgb_zones",
+ .is_visible = SYSFS_GROUP_VISIBLE(zone),
+ .attrs = zone_attrs,
+};
+
+/*
+ * LED Brightness (Global)
+ */
+static void global_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
+ int ret;
+
+ priv = container_of(led_cdev, struct alienfx_priv, global_led);
+ pdata = dev_get_platdata(&priv->pdev->dev);
+
+ priv->global_brightness = brightness;
+
+ ret = pdata->ops.upd_brightness(priv, pdata->wdev, brightness);
+ if (ret)
+ pr_err("LED brightness update failed\n");
+}
+
+static enum led_brightness global_led_get(struct led_classdev *led_cdev)
+{
+ struct alienfx_priv *priv;
+
+ priv = container_of(led_cdev, struct alienfx_priv, global_led);
+
+ return priv->global_brightness;
+}
+
+/*
+ * The HDMI mux sysfs node indicates the status of the HDMI input mux.
+ * It can toggle between standard system GPU output and HDMI input.
+ */
+static ssize_t show_hdmi_cable(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct alienfx_platdata *pdata;
+ acpi_status status;
+ u32 out_data;
+ struct wmax_basic_args in_args = {
+ .arg = 0,
+ };
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_CABLE,
+ &in_args, sizeof(in_args), &out_data);
+
+ if (ACPI_SUCCESS(status)) {
+ if (out_data == 0)
+ return sysfs_emit(buf, "[unconnected] connected unknown\n");
+ else if (out_data == 1)
+ return sysfs_emit(buf, "unconnected [connected] unknown\n");
+ }
+ pr_err("alienware-wmi: unknown HDMI cable status: %d\n", status);
+ return sysfs_emit(buf, "unconnected connected [unknown]\n");
+}
+
+static ssize_t show_hdmi_source(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct alienfx_platdata *pdata;
+ acpi_status status;
+ u32 out_data;
+ struct wmax_basic_args in_args = {
+ .arg = 0,
+ };
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_STATUS,
+ &in_args, sizeof(in_args), &out_data);
+
+ if (ACPI_SUCCESS(status)) {
+ if (out_data == 1)
+ return sysfs_emit(buf, "[input] gpu unknown\n");
+ else if (out_data == 2)
+ return sysfs_emit(buf, "input [gpu] unknown\n");
+ }
+ pr_err("alienware-wmi: unknown HDMI source status: %u\n", status);
+ return sysfs_emit(buf, "input gpu [unknown]\n");
+}
+
+static ssize_t toggle_hdmi_source(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct alienfx_platdata *pdata;
+ acpi_status status;
+ struct wmax_basic_args args;
+
+ pdata = dev_get_platdata(dev);
+
+ if (strcmp(buf, "gpu\n") == 0)
+ args.arg = 1;
+ else if (strcmp(buf, "input\n") == 0)
+ args.arg = 2;
+ else
+ args.arg = 3;
+ pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_SOURCE,
+ &args, sizeof(args), NULL);
+
+ if (ACPI_FAILURE(status))
+ pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
+ status);
+ return count;
+}
+
+static DEVICE_ATTR(cable, S_IRUGO, show_hdmi_cable, NULL);
+static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
+ toggle_hdmi_source);
+
+static bool hdmi_group_visible(struct kobject *kobj)
+{
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->hdmi_mux;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
+
+static struct attribute *hdmi_attrs[] = {
+ &dev_attr_cable.attr,
+ &dev_attr_source.attr,
+ NULL,
+};
+
+static const struct attribute_group hdmi_attribute_group = {
+ .name = "hdmi",
+ .is_visible = SYSFS_GROUP_VISIBLE(hdmi),
+ .attrs = hdmi_attrs,
+};
+
+/*
+ * Alienware GFX amplifier support
+ * - Currently supports reading cable status
+ * - Leaving expansion room to possibly support dock/undock events later
+ */
+static ssize_t show_amplifier_status(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct alienfx_platdata *pdata;
+ acpi_status status;
+ u32 out_data;
+ struct wmax_basic_args in_args = {
+ .arg = 0,
+ };
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_AMPLIFIER_CABLE,
+ &in_args, sizeof(in_args), &out_data);
+ if (ACPI_SUCCESS(status)) {
+ if (out_data == 0)
+ return sysfs_emit(buf, "[unconnected] connected unknown\n");
+ else if (out_data == 1)
+ return sysfs_emit(buf, "unconnected [connected] unknown\n");
+ }
+ pr_err("alienware-wmi: unknown amplifier cable status: %d\n", status);
+ return sysfs_emit(buf, "unconnected connected [unknown]\n");
+}
+
+static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
+
+static bool amplifier_group_visible(struct kobject *kobj)
+{
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->amplifier;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
+
+static struct attribute *amplifier_attrs[] = {
+ &dev_attr_status.attr,
+ NULL,
+};
+
+static const struct attribute_group amplifier_attribute_group = {
+ .name = "amplifier",
+ .is_visible = SYSFS_GROUP_VISIBLE(amplifier),
+ .attrs = amplifier_attrs,
+};
+
+/*
+ * Deep Sleep Control support
+ * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
+ */
+static ssize_t show_deepsleep_status(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct alienfx_platdata *pdata;
+ acpi_status status;
+ u32 out_data;
+ struct wmax_basic_args in_args = {
+ .arg = 0,
+ };
+
+ pdata = dev_get_platdata(dev);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_STATUS,
+ &in_args, sizeof(in_args), &out_data);
+ if (ACPI_SUCCESS(status)) {
+ if (out_data == 0)
+ return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
+ else if (out_data == 1)
+ return sysfs_emit(buf, "disabled [s5] s5_s4\n");
+ else if (out_data == 2)
+ return sysfs_emit(buf, "disabled s5 [s5_s4]\n");
+ }
+ pr_err("alienware-wmi: unknown deep sleep status: %d\n", status);
+ return sysfs_emit(buf, "disabled s5 s5_s4 [unknown]\n");
+}
+
+static ssize_t toggle_deepsleep(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct alienfx_platdata *pdata;
+ acpi_status status;
+ struct wmax_basic_args args;
+
+ pdata = dev_get_platdata(dev);
+
+ if (strcmp(buf, "disabled\n") == 0)
+ args.arg = 0;
+ else if (strcmp(buf, "s5\n") == 0)
+ args.arg = 1;
+ else
+ args.arg = 2;
+ pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
+
+ status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_CONTROL,
+ &args, sizeof(args), NULL);
+
+ if (ACPI_FAILURE(status))
+ pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
+ status);
+ return count;
+}
+
+static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
+
+static bool deepsleep_group_visible(struct kobject *kobj)
+{
+ struct device *dev;
+ struct alienfx_platdata *pdata;
+
+ dev = container_of(kobj, struct device, kobj);
+ pdata = dev_get_platdata(dev);
+
+ return pdata->deepslp;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
+
+static struct attribute *deepsleep_attrs[] = {
+ &dev_attr_deepsleep.attr,
+ NULL,
+};
+
+static const struct attribute_group deepsleep_attribute_group = {
+ .name = "deepsleep",
+ .is_visible = SYSFS_GROUP_VISIBLE(deepsleep),
+ .attrs = deepsleep_attrs,
+};
+
+/*
+ * Platform Driver
+ */
+static int alienfx_probe(struct platform_device *pdev)
+{
+ struct alienfx_priv *priv;
+ struct alienfx_platdata *pdata;
+ struct led_classdev *leds;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ platform_set_drvdata(pdev, priv);
+
+ priv->pdev = pdev;
+
+ pdata = dev_get_platdata(&pdev->dev);
+
+ priv->lighting_control_state = pdata->running_code;
+
+ leds = &priv->global_led;
+ leds->name = "alienware::global_brightness";
+ leds->brightness_set = global_led_set;
+ leds->brightness_get = global_led_get;
+ leds->max_brightness = 0x0F;
+
+ priv->global_brightness = priv->global_led.max_brightness;
+
+ return devm_led_classdev_register(&pdev->dev, &priv->global_led);
+}
+
+static const struct attribute_group *alienfx_groups[] = {
+ &zone_attribute_group,
+ &hdmi_attribute_group,
+ &lifier_attribute_group,
+ &deepsleep_attribute_group,
+ NULL
+};
+
+static struct platform_driver platform_driver = {
+ .driver = {
+ .name = "alienware-wmi",
+ .dev_groups = alienfx_groups,
+ },
+ .probe = alienfx_probe,
+};
+
+int alienfx_wmi_init(struct alienfx_platdata *pdata)
+{
+ struct platform_device *pdev;
+
+ pdev = platform_create_bundle(&platform_driver, alienfx_probe, NULL, 0,
+ pdata, sizeof(*pdata));
+
+ dev_set_drvdata(&pdata->wdev->dev, pdev);
+
+ return PTR_ERR_OR_ZERO(pdev);
+}
+
+void alienfx_wmi_exit(struct wmi_device *wdev)
+{
+ struct platform_device *pdev;
+
+ pdev = dev_get_drvdata(&wdev->dev);
+
+ platform_device_unregister(pdev);
+ platform_driver_unregister(&platform_driver);
+}
diff --git a/drivers/platform/x86/dell/alienware-wmi-awcc.c b/drivers/platform/x86/dell/alienware-wmi-awcc.c
new file mode 100644
index 000000000000..115ef8436ae3
--- /dev/null
+++ b/drivers/platform/x86/dell/alienware-wmi-awcc.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Alienware AlienFX control
+ *
+ * Copyright (C) 2024 Kurt Borja <kuurtb@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/platform_device.h>
+#include <linux/platform_profile.h>
+#include <linux/wmi.h>
+#include "alienware-wmi.h"
+
+#define WMAX_METHOD_THERMAL_INFORMATION 0x14
+#define WMAX_METHOD_THERMAL_CONTROL 0x15
+#define WMAX_METHOD_GAME_SHIFT_STATUS 0x25
+
+#define WMAX_THERMAL_MODE_GMODE 0xAB
+
+#define WMAX_FAILURE_CODE 0xFFFFFFFF
+#define WMAX_THERMAL_TABLE_MASK GENMASK(7, 4)
+#define WMAX_THERMAL_MODE_MASK GENMASK(3, 0)
+#define WMAX_SENSOR_ID_MASK BIT(8)
+
+enum WMAX_THERMAL_INFORMATION_OPERATIONS {
+ WMAX_OPERATION_SYS_DESCRIPTION = 0x02,
+ WMAX_OPERATION_LIST_IDS = 0x03,
+ WMAX_OPERATION_CURRENT_PROFILE = 0x0B,
+};
+
+enum WMAX_THERMAL_CONTROL_OPERATIONS {
+ WMAX_OPERATION_ACTIVATE_PROFILE = 0x01,
+};
+
+enum WMAX_GAME_SHIFT_STATUS_OPERATIONS {
+ WMAX_OPERATION_TOGGLE_GAME_SHIFT = 0x01,
+ WMAX_OPERATION_GET_GAME_SHIFT_STATUS = 0x02,
+};
+
+enum WMAX_THERMAL_TABLES {
+ WMAX_THERMAL_TABLE_BASIC = 0x90,
+ WMAX_THERMAL_TABLE_USTT = 0xA0,
+};
+
+static const enum platform_profile_option wmax_mode_to_platform_profile[THERMAL_MODE_LAST] = {
+ [THERMAL_MODE_USTT_BALANCED] = PLATFORM_PROFILE_BALANCED,
+ [THERMAL_MODE_USTT_BALANCED_PERFORMANCE] = PLATFORM_PROFILE_BALANCED_PERFORMANCE,
+ [THERMAL_MODE_USTT_COOL] = PLATFORM_PROFILE_COOL,
+ [THERMAL_MODE_USTT_QUIET] = PLATFORM_PROFILE_QUIET,
+ [THERMAL_MODE_USTT_PERFORMANCE] = PLATFORM_PROFILE_PERFORMANCE,
+ [THERMAL_MODE_USTT_LOW_POWER] = PLATFORM_PROFILE_LOW_POWER,
+ [THERMAL_MODE_BASIC_QUIET] = PLATFORM_PROFILE_QUIET,
+ [THERMAL_MODE_BASIC_BALANCED] = PLATFORM_PROFILE_BALANCED,
+ [THERMAL_MODE_BASIC_BALANCED_PERFORMANCE] = PLATFORM_PROFILE_BALANCED_PERFORMANCE,
+ [THERMAL_MODE_BASIC_PERFORMANCE] = PLATFORM_PROFILE_PERFORMANCE,
+};
+
+struct wmax_u32_args {
+ u8 operation;
+ u8 arg1;
+ u8 arg2;
+ u8 arg3;
+};
+
+/*
+ * Thermal Profile control
+ * - Provides thermal profile control through the Platform Profile API
+ */
+static bool is_wmax_thermal_code(u32 code)
+{
+ if (code & WMAX_SENSOR_ID_MASK)
+ return false;
+
+ if ((code & WMAX_THERMAL_MODE_MASK) >= THERMAL_MODE_LAST)
+ return false;
+
+ if ((code & WMAX_THERMAL_TABLE_MASK) == WMAX_THERMAL_TABLE_BASIC &&
+ (code & WMAX_THERMAL_MODE_MASK) >= THERMAL_MODE_BASIC_QUIET)
+ return true;
+
+ if ((code & WMAX_THERMAL_TABLE_MASK) == WMAX_THERMAL_TABLE_USTT &&
+ (code & WMAX_THERMAL_MODE_MASK) <= THERMAL_MODE_USTT_LOW_POWER)
+ return true;
+
+ return false;
+}
+
+static int wmax_thermal_information(struct wmi_device *wdev, u8 operation,
+ u8 arg, u32 *out_data)
+{
+ acpi_status status;
+ struct wmax_u32_args in_args = {
+ .operation = operation,
+ .arg1 = arg,
+ .arg2 = 0,
+ .arg3 = 0,
+ };
+
+ status = alienware_wmi_command(wdev, WMAX_METHOD_THERMAL_INFORMATION,
+ &in_args, sizeof(in_args), out_data);
+
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ if (*out_data == WMAX_FAILURE_CODE)
+ return -EBADRQC;
+
+ return 0;
+}
+
+static int wmax_thermal_control(struct wmi_device *wdev, u8 profile)
+{
+ acpi_status status;
+ struct wmax_u32_args in_args = {
+ .operation = WMAX_OPERATION_ACTIVATE_PROFILE,
+ .arg1 = profile,
+ .arg2 = 0,
+ .arg3 = 0,
+ };
+ u32 out_data;
+
+ status = alienware_wmi_command(wdev, WMAX_METHOD_THERMAL_CONTROL,
+ &in_args, sizeof(in_args), &out_data);
+
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ if (out_data == WMAX_FAILURE_CODE)
+ return -EBADRQC;
+
+ return 0;
+}
+
+static int wmax_game_shift_status(struct wmi_device *wdev, u8 operation,
+ u32 *out_data)
+{
+ acpi_status status;
+ struct wmax_u32_args in_args = {
+ .operation = operation,
+ .arg1 = 0,
+ .arg2 = 0,
+ .arg3 = 0,
+ };
+
+ status = alienware_wmi_command(wdev, WMAX_METHOD_GAME_SHIFT_STATUS,
+ &in_args, sizeof(in_args), out_data);
+
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ if (*out_data == WMAX_FAILURE_CODE)
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
+static int thermal_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ struct awcc_priv *priv;
+ u32 out_data;
+ int ret;
+
+ priv = container_of(pprof, struct awcc_priv, pp_handler);
+
+ ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_CURRENT_PROFILE,
+ 0, &out_data);
+
+ if (ret < 0)
+ return ret;
+
+ if (out_data == WMAX_THERMAL_MODE_GMODE) {
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ return 0;
+ }
+
+ if (!is_wmax_thermal_code(out_data))
+ return -ENODATA;
+
+ out_data &= WMAX_THERMAL_MODE_MASK;
+ *profile = wmax_mode_to_platform_profile[out_data];
+
+ return 0;
+}
+
+static int thermal_profile_set(struct platform_profile_handler *pprof,
+ enum platform_profile_option profile)
+{
+ struct awcc_priv *priv;
+
+ priv = container_of(pprof, struct awcc_priv, pp_handler);
+
+ if (priv->has_gmode) {
+ u32 gmode_status;
+ int ret;
+
+ ret = wmax_game_shift_status(priv->wdev,
+ WMAX_OPERATION_GET_GAME_SHIFT_STATUS,
+ &gmode_status);
+
+ if (ret < 0)
+ return ret;
+
+ if ((profile == PLATFORM_PROFILE_PERFORMANCE && !gmode_status) ||
+ (profile != PLATFORM_PROFILE_PERFORMANCE && gmode_status)) {
+ ret = wmax_game_shift_status(priv->wdev,
+ WMAX_OPERATION_TOGGLE_GAME_SHIFT,
+ &gmode_status);
+
+ if (ret < 0)
+ return ret;
+ }
+ }
+
+ return wmax_thermal_control(priv->wdev,
+ priv->supported_thermal_profiles[profile]);
+}
+
+int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
+{
+ struct awcc_priv *priv;
+ u32 out_data;
+ u8 sys_desc[4];
+ u32 first_mode;
+ enum wmax_thermal_mode mode;
+ enum platform_profile_option profile;
+ int ret;
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+ dev_set_drvdata(&wdev->dev, priv);
+
+ priv->wdev = wdev;
+
+ ret = wmax_thermal_information(wdev, WMAX_OPERATION_SYS_DESCRIPTION,
+ 0, (u32 *) &sys_desc);
+ if (ret < 0)
+ return ret;
+
+ first_mode = sys_desc[0] + sys_desc[1];
+
+ for (u32 i = 0; i < sys_desc[3]; i++) {
+ ret = wmax_thermal_information(wdev, WMAX_OPERATION_LIST_IDS,
+ i + first_mode, &out_data);
+
+ if (ret == -EIO)
+ return ret;
+
+ if (ret == -EBADRQC)
+ break;
+
+ if (!is_wmax_thermal_code(out_data))
+ continue;
+
+ mode = out_data & WMAX_THERMAL_MODE_MASK;
+ profile = wmax_mode_to_platform_profile[mode];
+ priv->supported_thermal_profiles[profile] = out_data;
+
+ set_bit(profile, priv->pp_handler.choices);
+ }
+
+ if (bitmap_empty(priv->pp_handler.choices, PLATFORM_PROFILE_LAST))
+ return -ENODEV;
+
+ if (has_gmode) {
+ priv->has_gmode = true;
+ priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
+ WMAX_THERMAL_MODE_GMODE;
+
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->pp_handler.choices);
+ }
+
+ priv->pp_handler.profile_get = thermal_profile_get;
+ priv->pp_handler.profile_set = thermal_profile_set;
+
+ return platform_profile_register(&priv->pp_handler);
+}
+
+void remove_thermal_profile(void)
+{
+ platform_profile_remove();
+}
diff --git a/drivers/platform/x86/dell/alienware-wmi-base.c b/drivers/platform/x86/dell/alienware-wmi-base.c
index 4165eb0d0bf5..7cc6bb3dc0d7 100644
--- a/drivers/platform/x86/dell/alienware-wmi-base.c
+++ b/drivers/platform/x86/dell/alienware-wmi-base.c
@@ -8,24 +8,12 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/acpi.h>
-#include <linux/bitfield.h>
-#include <linux/bits.h>
#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/platform_profile.h>
#include <linux/dmi.h>
-#include <linux/leds.h>
#include <linux/wmi.h>
#include "alienware-wmi.h"
-#define WMAX_METHOD_THERMAL_INFORMATION 0x14
-#define WMAX_METHOD_THERMAL_CONTROL 0x15
-#define WMAX_METHOD_GAME_SHIFT_STATUS 0x25
-
-#define WMAX_THERMAL_MODE_GMODE 0xAB
-
-#define WMAX_FAILURE_CODE 0xFFFFFFFF
-
MODULE_AUTHOR("Mario Limonciello <mario.limonciello@outlook.com>");
MODULE_DESCRIPTION("Alienware special feature control");
MODULE_LICENSE("GPL");
@@ -43,39 +31,6 @@ enum INTERFACE_FLAGS {
WMAX,
};
-enum WMAX_THERMAL_INFORMATION_OPERATIONS {
- WMAX_OPERATION_SYS_DESCRIPTION = 0x02,
- WMAX_OPERATION_LIST_IDS = 0x03,
- WMAX_OPERATION_CURRENT_PROFILE = 0x0B,
-};
-
-enum WMAX_THERMAL_CONTROL_OPERATIONS {
- WMAX_OPERATION_ACTIVATE_PROFILE = 0x01,
-};
-
-enum WMAX_GAME_SHIFT_STATUS_OPERATIONS {
- WMAX_OPERATION_TOGGLE_GAME_SHIFT = 0x01,
- WMAX_OPERATION_GET_GAME_SHIFT_STATUS = 0x02,
-};
-
-enum WMAX_THERMAL_TABLES {
- WMAX_THERMAL_TABLE_BASIC = 0x90,
- WMAX_THERMAL_TABLE_USTT = 0xA0,
-};
-
-static const enum platform_profile_option wmax_mode_to_platform_profile[THERMAL_MODE_LAST] = {
- [THERMAL_MODE_USTT_BALANCED] = PLATFORM_PROFILE_BALANCED,
- [THERMAL_MODE_USTT_BALANCED_PERFORMANCE] = PLATFORM_PROFILE_BALANCED_PERFORMANCE,
- [THERMAL_MODE_USTT_COOL] = PLATFORM_PROFILE_COOL,
- [THERMAL_MODE_USTT_QUIET] = PLATFORM_PROFILE_QUIET,
- [THERMAL_MODE_USTT_PERFORMANCE] = PLATFORM_PROFILE_PERFORMANCE,
- [THERMAL_MODE_USTT_LOW_POWER] = PLATFORM_PROFILE_LOW_POWER,
- [THERMAL_MODE_BASIC_QUIET] = PLATFORM_PROFILE_QUIET,
- [THERMAL_MODE_BASIC_BALANCED] = PLATFORM_PROFILE_BALANCED,
- [THERMAL_MODE_BASIC_BALANCED_PERFORMANCE] = PLATFORM_PROFILE_BALANCED_PERFORMANCE,
- [THERMAL_MODE_BASIC_PERFORMANCE] = PLATFORM_PROFILE_PERFORMANCE,
-};
-
struct quirk_entry {
u8 num_zones;
u8 hdmi_mux;
@@ -332,17 +287,6 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
{}
};
-struct wmax_basic_args {
- u8 arg;
-};
-
-struct wmax_u32_args {
- u8 operation;
- u8 arg1;
- u8 arg2;
- u8 arg3;
-};
-
static u8 interface;
static struct wmi_driver *preferred_wmi_driver;
@@ -372,744 +316,6 @@ acpi_status alienware_wmi_command(struct wmi_device *wdev, u32 method_id,
return ret;
}
-/*
- * Helpers used for zone control
- */
-static int parse_rgb(const char *buf, struct color_platform *colors)
-{
- long unsigned int rgb;
- int ret;
- union color_union {
- struct color_platform cp;
- int package;
- } repackager;
-
- ret = kstrtoul(buf, 16, &rgb);
- if (ret)
- return ret;
-
- /* RGB triplet notation is 24-bit hexadecimal */
- if (rgb > 0xFFFFFF)
- return -EINVAL;
-
- repackager.package = rgb & 0x0f0f0f0f;
- pr_debug("alienware-wmi: r: %d g:%d b: %d\n",
- repackager.cp.red, repackager.cp.green, repackager.cp.blue);
- *colors = repackager.cp;
- return 0;
-}
-
-/*
- * Individual RGB zone control
- */
-static ssize_t zone_show(struct device *dev, struct device_attribute *attr,
- char *buf, u8 location)
-{
- struct alienfx_priv *priv;
- struct color_platform *colors;
-
- priv = dev_get_drvdata(dev);
- colors = &priv->colors[location];
-
- return sprintf(buf, "red: %d, green: %d, blue: %d\n",
- colors->red, colors->green, colors->blue);
-
-}
-
-static ssize_t zone_set(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count, u8 location)
-{
- struct alienfx_priv *priv;
- struct alienfx_platdata *pdata;
- struct color_platform *colors;
- int ret;
-
- priv = dev_get_drvdata(dev);
- pdata = dev_get_platdata(dev);
-
- colors = &priv->colors[location];
- ret = parse_rgb(buf, colors);
- if (ret)
- return ret;
-
- ret = pdata->ops.upd_led(priv, pdata->wdev, location);
-
- return ret ? ret : count;
-}
-
-#define ALIENWARE_ZONE_SHOW_FUNC(_num) \
- static ssize_t zone0##_num##_show(struct device *dev, \
- struct device_attribute *attr, \
- char *buf) \
- { \
- return zone_show(dev, attr, buf, _num); \
- }
-
-#define ALIENWARE_ZONE_STORE_FUNC(_num) \
- static ssize_t zone0##_num##_store(struct device *dev, \
- struct device_attribute *attr, \
- const char *buf, size_t count) \
- { \
- return zone_set(dev, attr, buf, count, _num); \
- }
-
-#define ALIENWARE_ZONE_ATTR(_num) \
- ALIENWARE_ZONE_SHOW_FUNC(_num) \
- ALIENWARE_ZONE_STORE_FUNC(_num) \
- static DEVICE_ATTR_RW(zone0##_num)
-
-ALIENWARE_ZONE_ATTR(0);
-ALIENWARE_ZONE_ATTR(1);
-ALIENWARE_ZONE_ATTR(2);
-ALIENWARE_ZONE_ATTR(3);
-
-/*
- * Lighting control state device attribute (Global)
- */
-static ssize_t lighting_control_state_show(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct alienfx_priv *priv;
-
- priv = dev_get_drvdata(dev);
-
- if (priv->lighting_control_state == LEGACY_BOOTING)
- return sysfs_emit(buf, "[booting] running suspend\n");
- else if (priv->lighting_control_state == LEGACY_SUSPEND)
- return sysfs_emit(buf, "booting running [suspend]\n");
- return sysfs_emit(buf, "booting [running] suspend\n");
-}
-
-static ssize_t lighting_control_state_store(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct alienfx_priv *priv;
- struct alienfx_platdata *pdata;
- u8 val;
-
- priv = dev_get_drvdata(dev);
- pdata = dev_get_platdata(dev);
-
- if (strcmp(buf, "booting\n") == 0)
- val = LEGACY_BOOTING;
- else if (strcmp(buf, "suspend\n") == 0)
- val = LEGACY_SUSPEND;
- else
- val = pdata->running_code;
-
- priv->lighting_control_state = val;
- pr_debug("alienware-wmi: updated control state to %d\n",
- priv->lighting_control_state);
-
- return count;
-}
-
-static DEVICE_ATTR_RW(lighting_control_state);
-
-static umode_t zone_attr_visible(struct kobject *kobj,
- struct attribute *attr, int n)
-{
- struct device *dev;
- struct alienfx_platdata *pdata;
-
- dev = container_of(kobj, struct device, kobj);
- pdata = dev_get_platdata(dev);
-
- return n < pdata->num_zones + 1 ? 0644 : 0;
-}
-
-static bool zone_group_visible(struct kobject *kobj)
-{
- struct device *dev;
- struct alienfx_platdata *pdata;
-
- dev = container_of(kobj, struct device, kobj);
- pdata = dev_get_platdata(dev);
-
- return pdata->num_zones > 0;
-}
-DEFINE_SYSFS_GROUP_VISIBLE(zone);
-
-static struct attribute *zone_attrs[] = {
- &dev_attr_lighting_control_state.attr,
- &dev_attr_zone00.attr,
- &dev_attr_zone01.attr,
- &dev_attr_zone02.attr,
- &dev_attr_zone03.attr,
- NULL
-};
-
-static struct attribute_group zone_attribute_group = {
- .name = "rgb_zones",
- .is_visible = SYSFS_GROUP_VISIBLE(zone),
- .attrs = zone_attrs,
-};
-
-/*
- * LED Brightness (Global)
- */
-static void global_led_set(struct led_classdev *led_cdev,
- enum led_brightness brightness)
-{
- struct alienfx_priv *priv;
- struct alienfx_platdata *pdata;
- int ret;
-
- priv = container_of(led_cdev, struct alienfx_priv, global_led);
- pdata = dev_get_platdata(&priv->pdev->dev);
-
- priv->global_brightness = brightness;
-
- ret = pdata->ops.upd_brightness(priv, pdata->wdev, brightness);
- if (ret)
- pr_err("LED brightness update failed\n");
-}
-
-static enum led_brightness global_led_get(struct led_classdev *led_cdev)
-{
- struct alienfx_priv *priv;
-
- priv = container_of(led_cdev, struct alienfx_priv, global_led);
-
- return priv->global_brightness;
-}
-
-/*
- * The HDMI mux sysfs node indicates the status of the HDMI input mux.
- * It can toggle between standard system GPU output and HDMI input.
- */
-static ssize_t show_hdmi_cable(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct alienfx_platdata *pdata;
- acpi_status status;
- u32 out_data;
- struct wmax_basic_args in_args = {
- .arg = 0,
- };
-
- pdata = dev_get_platdata(dev);
-
- status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_CABLE,
- &in_args, sizeof(in_args), &out_data);
-
- if (ACPI_SUCCESS(status)) {
- if (out_data == 0)
- return sysfs_emit(buf, "[unconnected] connected unknown\n");
- else if (out_data == 1)
- return sysfs_emit(buf, "unconnected [connected] unknown\n");
- }
- pr_err("alienware-wmi: unknown HDMI cable status: %d\n", status);
- return sysfs_emit(buf, "unconnected connected [unknown]\n");
-}
-
-static ssize_t show_hdmi_source(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct alienfx_platdata *pdata;
- acpi_status status;
- u32 out_data;
- struct wmax_basic_args in_args = {
- .arg = 0,
- };
-
- pdata = dev_get_platdata(dev);
-
- status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_STATUS,
- &in_args, sizeof(in_args), &out_data);
-
- if (ACPI_SUCCESS(status)) {
- if (out_data == 1)
- return sysfs_emit(buf, "[input] gpu unknown\n");
- else if (out_data == 2)
- return sysfs_emit(buf, "input [gpu] unknown\n");
- }
- pr_err("alienware-wmi: unknown HDMI source status: %u\n", status);
- return sysfs_emit(buf, "input gpu [unknown]\n");
-}
-
-static ssize_t toggle_hdmi_source(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct alienfx_platdata *pdata;
- acpi_status status;
- struct wmax_basic_args args;
-
- pdata = dev_get_platdata(dev);
-
- if (strcmp(buf, "gpu\n") == 0)
- args.arg = 1;
- else if (strcmp(buf, "input\n") == 0)
- args.arg = 2;
- else
- args.arg = 3;
- pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
-
- status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_HDMI_SOURCE,
- &args, sizeof(args), NULL);
-
- if (ACPI_FAILURE(status))
- pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
- status);
- return count;
-}
-
-static DEVICE_ATTR(cable, S_IRUGO, show_hdmi_cable, NULL);
-static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
- toggle_hdmi_source);
-
-static bool hdmi_group_visible(struct kobject *kobj)
-{
- struct device *dev;
- struct alienfx_platdata *pdata;
-
- dev = container_of(kobj, struct device, kobj);
- pdata = dev_get_platdata(dev);
-
- return pdata->hdmi_mux;
-}
-DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
-
-static struct attribute *hdmi_attrs[] = {
- &dev_attr_cable.attr,
- &dev_attr_source.attr,
- NULL,
-};
-
-static const struct attribute_group hdmi_attribute_group = {
- .name = "hdmi",
- .is_visible = SYSFS_GROUP_VISIBLE(hdmi),
- .attrs = hdmi_attrs,
-};
-
-/*
- * Alienware GFX amplifier support
- * - Currently supports reading cable status
- * - Leaving expansion room to possibly support dock/undock events later
- */
-static ssize_t show_amplifier_status(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct alienfx_platdata *pdata;
- acpi_status status;
- u32 out_data;
- struct wmax_basic_args in_args = {
- .arg = 0,
- };
-
- pdata = dev_get_platdata(dev);
-
- status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_AMPLIFIER_CABLE,
- &in_args, sizeof(in_args), &out_data);
- if (ACPI_SUCCESS(status)) {
- if (out_data == 0)
- return sysfs_emit(buf, "[unconnected] connected unknown\n");
- else if (out_data == 1)
- return sysfs_emit(buf, "unconnected [connected] unknown\n");
- }
- pr_err("alienware-wmi: unknown amplifier cable status: %d\n", status);
- return sysfs_emit(buf, "unconnected connected [unknown]\n");
-}
-
-static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
-
-static bool amplifier_group_visible(struct kobject *kobj)
-{
- struct device *dev;
- struct alienfx_platdata *pdata;
-
- dev = container_of(kobj, struct device, kobj);
- pdata = dev_get_platdata(dev);
-
- return pdata->amplifier;
-}
-DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
-
-static struct attribute *amplifier_attrs[] = {
- &dev_attr_status.attr,
- NULL,
-};
-
-static const struct attribute_group amplifier_attribute_group = {
- .name = "amplifier",
- .is_visible = SYSFS_GROUP_VISIBLE(amplifier),
- .attrs = amplifier_attrs,
-};
-
-/*
- * Deep Sleep Control support
- * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
- */
-static ssize_t show_deepsleep_status(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct alienfx_platdata *pdata;
- acpi_status status;
- u32 out_data;
- struct wmax_basic_args in_args = {
- .arg = 0,
- };
-
- pdata = dev_get_platdata(dev);
-
- status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_STATUS,
- &in_args, sizeof(in_args), &out_data);
- if (ACPI_SUCCESS(status)) {
- if (out_data == 0)
- return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
- else if (out_data == 1)
- return sysfs_emit(buf, "disabled [s5] s5_s4\n");
- else if (out_data == 2)
- return sysfs_emit(buf, "disabled s5 [s5_s4]\n");
- }
- pr_err("alienware-wmi: unknown deep sleep status: %d\n", status);
- return sysfs_emit(buf, "disabled s5 s5_s4 [unknown]\n");
-}
-
-static ssize_t toggle_deepsleep(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct alienfx_platdata *pdata;
- acpi_status status;
- struct wmax_basic_args args;
-
- pdata = dev_get_platdata(dev);
-
- if (strcmp(buf, "disabled\n") == 0)
- args.arg = 0;
- else if (strcmp(buf, "s5\n") == 0)
- args.arg = 1;
- else
- args.arg = 2;
- pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
-
- status = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_CONTROL,
- &args, sizeof(args), NULL);
-
- if (ACPI_FAILURE(status))
- pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
- status);
- return count;
-}
-
-static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
-
-static bool deepsleep_group_visible(struct kobject *kobj)
-{
- struct device *dev;
- struct alienfx_platdata *pdata;
-
- dev = container_of(kobj, struct device, kobj);
- pdata = dev_get_platdata(dev);
-
- return pdata->deepslp;
-}
-DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
-
-static struct attribute *deepsleep_attrs[] = {
- &dev_attr_deepsleep.attr,
- NULL,
-};
-
-static const struct attribute_group deepsleep_attribute_group = {
- .name = "deepsleep",
- .is_visible = SYSFS_GROUP_VISIBLE(deepsleep),
- .attrs = deepsleep_attrs,
-};
-
-/*
- * Thermal Profile control
- * - Provides thermal profile control through the Platform Profile API
- */
-#define WMAX_THERMAL_TABLE_MASK GENMASK(7, 4)
-#define WMAX_THERMAL_MODE_MASK GENMASK(3, 0)
-#define WMAX_SENSOR_ID_MASK BIT(8)
-
-static bool is_wmax_thermal_code(u32 code)
-{
- if (code & WMAX_SENSOR_ID_MASK)
- return false;
-
- if ((code & WMAX_THERMAL_MODE_MASK) >= THERMAL_MODE_LAST)
- return false;
-
- if ((code & WMAX_THERMAL_TABLE_MASK) == WMAX_THERMAL_TABLE_BASIC &&
- (code & WMAX_THERMAL_MODE_MASK) >= THERMAL_MODE_BASIC_QUIET)
- return true;
-
- if ((code & WMAX_THERMAL_TABLE_MASK) == WMAX_THERMAL_TABLE_USTT &&
- (code & WMAX_THERMAL_MODE_MASK) <= THERMAL_MODE_USTT_LOW_POWER)
- return true;
-
- return false;
-}
-
-static int wmax_thermal_information(struct wmi_device *wdev, u8 operation,
- u8 arg, u32 *out_data)
-{
- acpi_status status;
- struct wmax_u32_args in_args = {
- .operation = operation,
- .arg1 = arg,
- .arg2 = 0,
- .arg3 = 0,
- };
-
- status = alienware_wmi_command(wdev, WMAX_METHOD_THERMAL_INFORMATION,
- &in_args, sizeof(in_args), out_data);
-
- if (ACPI_FAILURE(status))
- return -EIO;
-
- if (*out_data == WMAX_FAILURE_CODE)
- return -EBADRQC;
-
- return 0;
-}
-
-static int wmax_thermal_control(struct wmi_device *wdev, u8 profile)
-{
- acpi_status status;
- struct wmax_u32_args in_args = {
- .operation = WMAX_OPERATION_ACTIVATE_PROFILE,
- .arg1 = profile,
- .arg2 = 0,
- .arg3 = 0,
- };
- u32 out_data;
-
- status = alienware_wmi_command(wdev, WMAX_METHOD_THERMAL_CONTROL,
- &in_args, sizeof(in_args), &out_data);
-
- if (ACPI_FAILURE(status))
- return -EIO;
-
- if (out_data == WMAX_FAILURE_CODE)
- return -EBADRQC;
-
- return 0;
-}
-
-static int wmax_game_shift_status(struct wmi_device *wdev, u8 operation,
- u32 *out_data)
-{
- acpi_status status;
- struct wmax_u32_args in_args = {
- .operation = operation,
- .arg1 = 0,
- .arg2 = 0,
- .arg3 = 0,
- };
-
- status = alienware_wmi_command(wdev, WMAX_METHOD_GAME_SHIFT_STATUS,
- &in_args, sizeof(in_args), out_data);
-
- if (ACPI_FAILURE(status))
- return -EIO;
-
- if (*out_data == WMAX_FAILURE_CODE)
- return -EOPNOTSUPP;
-
- return 0;
-}
-
-static int thermal_profile_get(struct platform_profile_handler *pprof,
- enum platform_profile_option *profile)
-{
- struct awcc_priv *priv;
- u32 out_data;
- int ret;
-
- priv = container_of(pprof, struct awcc_priv, pp_handler);
-
- ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_CURRENT_PROFILE,
- 0, &out_data);
-
- if (ret < 0)
- return ret;
-
- if (out_data == WMAX_THERMAL_MODE_GMODE) {
- *profile = PLATFORM_PROFILE_PERFORMANCE;
- return 0;
- }
-
- if (!is_wmax_thermal_code(out_data))
- return -ENODATA;
-
- out_data &= WMAX_THERMAL_MODE_MASK;
- *profile = wmax_mode_to_platform_profile[out_data];
-
- return 0;
-}
-
-static int thermal_profile_set(struct platform_profile_handler *pprof,
- enum platform_profile_option profile)
-{
- struct awcc_priv *priv;
-
- priv = container_of(pprof, struct awcc_priv, pp_handler);
-
- if (priv->has_gmode) {
- u32 gmode_status;
- int ret;
-
- ret = wmax_game_shift_status(priv->wdev,
- WMAX_OPERATION_GET_GAME_SHIFT_STATUS,
- &gmode_status);
-
- if (ret < 0)
- return ret;
-
- if ((profile == PLATFORM_PROFILE_PERFORMANCE && !gmode_status) ||
- (profile != PLATFORM_PROFILE_PERFORMANCE && gmode_status)) {
- ret = wmax_game_shift_status(priv->wdev,
- WMAX_OPERATION_TOGGLE_GAME_SHIFT,
- &gmode_status);
-
- if (ret < 0)
- return ret;
- }
- }
-
- return wmax_thermal_control(priv->wdev,
- priv->supported_thermal_profiles[profile]);
-}
-
-int create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
-{
- struct awcc_priv *priv;
- u32 out_data;
- u8 sys_desc[4];
- u32 first_mode;
- enum wmax_thermal_mode mode;
- enum platform_profile_option profile;
- int ret;
-
- priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
- dev_set_drvdata(&wdev->dev, priv);
-
- priv->wdev = wdev;
-
- ret = wmax_thermal_information(wdev, WMAX_OPERATION_SYS_DESCRIPTION,
- 0, (u32 *) &sys_desc);
- if (ret < 0)
- return ret;
-
- first_mode = sys_desc[0] + sys_desc[1];
-
- for (u32 i = 0; i < sys_desc[3]; i++) {
- ret = wmax_thermal_information(wdev, WMAX_OPERATION_LIST_IDS,
- i + first_mode, &out_data);
-
- if (ret == -EIO)
- return ret;
-
- if (ret == -EBADRQC)
- break;
-
- if (!is_wmax_thermal_code(out_data))
- continue;
-
- mode = out_data & WMAX_THERMAL_MODE_MASK;
- profile = wmax_mode_to_platform_profile[mode];
- priv->supported_thermal_profiles[profile] = out_data;
-
- set_bit(profile, priv->pp_handler.choices);
- }
-
- if (bitmap_empty(priv->pp_handler.choices, PLATFORM_PROFILE_LAST))
- return -ENODEV;
-
- if (has_gmode) {
- priv->has_gmode = true;
- priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
- WMAX_THERMAL_MODE_GMODE;
-
- set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->pp_handler.choices);
- }
-
- priv->pp_handler.profile_get = thermal_profile_get;
- priv->pp_handler.profile_set = thermal_profile_set;
-
- return platform_profile_register(&priv->pp_handler);
-}
-
-void remove_thermal_profile(void)
-{
- platform_profile_remove();
-}
-
-/*
- * Platform Driver
- */
-static int alienfx_probe(struct platform_device *pdev)
-{
- struct alienfx_priv *priv;
- struct led_classdev *leds;
-
- priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
- platform_set_drvdata(pdev, priv);
-
- priv->pdev = pdev;
-
- if (interface == WMAX)
- priv->lighting_control_state = WMAX_RUNNING;
- else if (interface == LEGACY)
- priv->lighting_control_state = LEGACY_RUNNING;
-
- leds = &priv->global_led;
- leds->name = "alienware::global_brightness";
- leds->brightness_set = global_led_set;
- leds->brightness_get = global_led_get;
- leds->max_brightness = 0x0F;
-
- priv->global_brightness = priv->global_led.max_brightness;
-
- return devm_led_classdev_register(&pdev->dev, &priv->global_led);
-}
-
-static const struct attribute_group *alienfx_groups[] = {
- &zone_attribute_group,
- &hdmi_attribute_group,
- &lifier_attribute_group,
- &deepsleep_attribute_group,
- NULL
-};
-
-static struct platform_driver platform_driver = {
- .driver = {
- .name = "alienware-wmi",
- .dev_groups = alienfx_groups,
- },
- .probe = alienfx_probe,
-};
-
-int alienfx_wmi_init(struct alienfx_platdata *pdata)
-{
- struct platform_device *pdev;
-
- pdev = platform_create_bundle(&platform_driver, alienfx_probe, NULL, 0,
- pdata, sizeof(*pdata));
-
- dev_set_drvdata(&pdata->wdev->dev, pdev);
-
- return PTR_ERR_OR_ZERO(pdev);
-}
-
-void alienfx_wmi_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 device
*/
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [RFC PATCH 21/21] platform-x86: Add config entries to alienware-wmi
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (19 preceding siblings ...)
2024-12-05 0:47 ` [RFC PATCH 20/21] platform-x86: Split the alienware-wmi module Kurt Borja
@ 2024-12-05 0:48 ` Kurt Borja
2024-12-06 23:26 ` [RFC PATCH 00/21] alienware-wmi driver rework Armin Wolf
21 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-05 0:48 UTC (permalink / raw)
To: kuurtb
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello, platform-driver-x86, w_armin
Add config entries to conditionally compile alienware-wmi-alienfx.c and
alienware-wmi-awcc.c.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/dell/Kconfig | 25 ++++++++++++++++++-----
drivers/platform/x86/dell/Makefile | 8 ++++----
drivers/platform/x86/dell/alienware-wmi.h | 22 ++++++++++++++++++++
3 files changed, 46 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index 2dddafb3f7fa..fcc7f7d28ac1 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -18,16 +18,31 @@ config ALIENWARE_WMI
tristate "Alienware Special feature control"
default m
depends on ACPI
+ depends on ACPI_WMI
+ help
+ This is a driver for controlling Alienware WMI driven
+ features.
+
+config ALIENWARE_ALIENFX
+ bool "Alienware AlienFX backend"
+ default y
depends on LEDS_CLASS
depends on NEW_LEDS
- depends on ACPI_WMI
- select ACPI_PLATFORM_PROFILE
+ depends on ALIENWARE_WMI
help
- This is a driver for controlling Alienware BIOS driven
- features. It exposes an interface for controlling the AlienFX
- zones on Alienware machines that don't contain a dedicated AlienFX
+ It exposes an interface for controlling the AlienFX zones on
+ Alienware machines that don't contain a dedicated AlienFX
USB MCU such as the X51 and X51-R2.
+config ALIENWARE_AWCC
+ bool "Alienware AWCC backend"
+ default y
+ depends on ALIENWARE_WMI
+ select ACPI_PLATFORM_PROFILE
+ help
+ Provides thermal control features on Alienware and Dell's
+ GSeries laptops with a WMI device with UID "AWCC".
+
config DCDBAS
tristate "Dell Systems Management Base Driver"
default m
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index 54a592fd6ae6..6153cff5538f 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -4,10 +4,10 @@
# Dell x86 Platform-Specific Drivers
#
-obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
-alienware-wmi-objs := alienware-wmi-base.o
-alienware-wmi-y += alienware-wmi-alienfx.o
-alienware-wmi-y += alienware-wmi-awcc.o
+obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
+alienware-wmi-objs := alienware-wmi-base.o
+alienware-wmi-$(CONFIG_ALIENWARE_ALIENFX) += alienware-wmi-alienfx.o
+alienware-wmi-$(CONFIG_ALIENWARE_AWCC) += alienware-wmi-awcc.o
obj-$(CONFIG_DCDBAS) += dcdbas.o
obj-$(CONFIG_DELL_LAPTOP) += dell-laptop.o
obj-$(CONFIG_DELL_RBTN) += dell-rbtn.o
diff --git a/drivers/platform/x86/dell/alienware-wmi.h b/drivers/platform/x86/dell/alienware-wmi.h
index 1c12b8e330e2..16624c824828 100644
--- a/drivers/platform/x86/dell/alienware-wmi.h
+++ b/drivers/platform/x86/dell/alienware-wmi.h
@@ -110,10 +110,32 @@ struct awcc_priv {
acpi_status alienware_wmi_command(struct wmi_device *wdev, u32 method_id,
void *in_args, size_t in_size, u32 *out_data);
+#if IS_ENABLED(CONFIG_ALIENWARE_ALIENFX)
int alienfx_wmi_init(struct alienfx_platdata *pdata);
void alienfx_wmi_exit(struct wmi_device *wdev);
+#else
+int inline alienfx_wmi_init(struct alienfx_platdata *pdata)
+{
+ return 0;
+}
+
+void inline alienfx_wmi_exit(struct wmi_device *wdev)
+{
+}
+#endif
+#if IS_ENABLED(CONFIG_ALIENWARE_AWCC)
int create_thermal_profile(struct wmi_device *wdev, bool has_gmode);
void remove_thermal_profile(void);
+#else
+int inline create_thermal_profile(struct wmi_device *wdev, bool has_gmode)
+{
+ return 0;
+}
+
+void inline remove_thermal_profile(void)
+{
+}
+#endif
#endif
--
2.47.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH 00/21] alienware-wmi driver rework
2024-12-05 0:27 [RFC PATCH 00/21] alienware-wmi driver rework Kurt Borja
` (20 preceding siblings ...)
2024-12-05 0:48 ` [RFC PATCH 21/21] platform-x86: Add config entries to alienware-wmi Kurt Borja
@ 2024-12-06 23:26 ` Armin Wolf
2024-12-07 1:59 ` Kurt Borja
21 siblings, 1 reply; 40+ messages in thread
From: Armin Wolf @ 2024-12-06 23:26 UTC (permalink / raw)
To: Kurt Borja, platform-driver-x86
Cc: Dell.Client.Kernel, hdegoede, ilpo.jarvinen, linux-kernel,
mario.limonciello
Am 05.12.24 um 01:27 schrieb Kurt Borja:
> Hi :)
>
> This series are a follow-up to this discussion [1], in which I proposed
> migrating the alienware-wmi driver to use:
>
> 1. State container driver model
> 2. Modern WMI driver design
> 3. Drop use of deprecated WMI methods
>
> Of course, this was much harder than expected to do cleanly. Main
> problem was that this driver "drives" two completely different devices
> (I'm not referring to the WMI devices, which also happen to be two).
>
> Throughout these series we will call these devices AlienFX and AWCC.
>
> As a preamble
> =============
>
> AlienFX exposes a LED, hdmi, amplifier and deepsleep interface to
> userspace through a platform device named "alienware-wmi". Historically
> this driver handled this by leveraging on two WMI devices as a backend.
> This devices named LEGACY and WMAX were very similar, the only
> difference was that WMAX had more features, but share all features
> LEGACY had. Although it's a stretch, it could be argued this WMI devices
> are the "same", just different GUID.
>
> Later Dell repurposed the WMAX WMI device to serve as a thermal control
> interface for all newer "gaming" laptops. This new WMAX device has an
> ACPI UID = "AWCC" (I discovered this recently). So it could also be
> argued that old WMAX and AWCC WMAX are not the same device, just same
> GUID.
>
> This drivers manages all these features using deprecated WMI methods.
I think there is a misunderstanding here.
The WMAX WMI device is identical with the AWCC WMI device, only the UID might be different.
The reason why the thermal control WMI methods are not available on older WMAX devices is
that Dell seemed to have introduced this WMI methods after the usual WMAX WMI methods.
Because of this i advise against splitting WMAX (LED, attributes, ...) and AWCC functionality
into separate files.
> Approach I took for the rework
> ==============================
>
> Parts 1-7 sort of containerize all AlienFX functionality under the
> "alienware-wmi" platform driver so WMI drivers can prepare and register
> a matching platform device from the probe.
>
> Parts 8-12 create and register two WMI drivers for the LEGACY and WMAX
> devices respectively. The code for these probes is VERY similar and
> all "differences" are passed to the platform device via platform
> specific data (platdata). Also AlienFX functionality is refactored to
> use non-deprecated WMI methods.
>
> Parts 13-17 migrate all AWCC methods to use non-deprecated WMI methods
> and the state container driver model.
>
> Parts 18-21 I splitted the alienware-wmi.c module into the different
> features this driver manages.
>
> alienware-wmi-base.c is in charge of initializing WMI drivers and
> define some platform specific data, like operations (Part 10 for more
> info). alienware-wmi-alienfx.c has all AlienFX functionality and
> alienware-wmi-awcc.c has all AWCC functionality.
I would rather split the drivers into:
- alienware-wmi-legacy, which handles the LEGACY WMI device and registers a alienware-wmi platform device
- alienware-wmi-wmax, which handles the modern WMAX WMI device and also registers a alienware-wmi platform device
- alienware-wmi-base, which provides a driver for the alienware-wmi platform device
This of course only works if the LEGACY WMI device and the WMAX WMI device are newer both present at the same time,
in this case alienware-wmi-legacy could use wmi_has_guid() as a band aid check to avoid probing if a WMAX WMI device
is present.
Using the platform_data mechanism to decouple the alienware-wmi device driver from the underlying hardware implementation
should be fine IMHO.
> Coments
> =======
>
> This is still kind of a draft, but I did some testing and it works!
>
> Of course I will do thorough testing and cleanup when I send the
> non-RFC version. I just want to get some comments on the general
> approach before proceeding further.
>
> I think this is quite messy in it's current state so I apollogize.
>
> @Mario Limonciello: I included the reviews you gave me on [2]. I
> included some of those patches here, and dropped the ones that did not
> make sense with this design. As this is another series let me know if
> you want me to drop the tags!
>
> @Armin Wolf: I don't like the amount of files I made. As the maintainer
> of the wmi module, what do you think about making two independent
> modules, one for AlienFX and one for AWCC. In order to not register two
> drivers for the WMAX device the module init would check if the "AWCC"
> UID is present.
I know of at least one device which support both AWCC thermal control and
WMAX LED control, so splitting the WMAX device driver like this could cause
problems.
Like i said before, you should view the WMAX WMI device as having different
capabilities (= WMI methods) depending of the machine the kernel is running on.
If a capability is available (currently determined via quirks), the driver should
do the necessary things to handle it.
As a side note: i am currently exploring if we can decode the WMI BMOF buffers inside
the kernel, so that in the far future we can remove those quirks and automatically detect
which methods are available. But this will take a long time, so it has nothing to do with
this patch series.
I will take a look at the other patches tomorrow.
Thanks,
Armin Wolf
>
> The approach for that would be basically the same, and I think the
> series would change very little.
>
> I would like this a lot because I still think old and new WMAX devices
> are different, but I couldn't find another example of where an OEM
> repurposed a WMI device.
>
> @Everyone: I know this is VERY long. Thank you so much for your time in
> advance!
>
> This series were made on top of the 'for-next' branch:
>
> Commit c712e8fd9bf4 ("MAINTAINERS: Change AMD PMC driver status to "Supported"")
>
> ~ Kurt
>
> [1] https://lore.kernel.org/platform-driver-x86/6m66cuivkzhcsvpjv4nunjyddqhr42bmjdhptu4bqm6rm7fvxf@qjwove4hg6gb/T/#u
> [2] https://lore.kernel.org/platform-driver-x86/20241120163834.6446-3-kuurtb@gmail.com/
>
> Kurt Borja (21):
> alienware-wmi: Modify parse_rgb() signature
> alienware-wmi: Move Lighting Control State
> alienware-wmi: Remove unnecessary check at module exit
> alienware-wmi: Improve sysfs groups creation
> alienware-wmi: Refactor rgb-zones sysfs group creation
> alienware-wmi: Add state container and alienfx_probe()
> alienware-wmi: Migrate to state container pattern
> alienware-wmi: Add WMI Drivers
> alienware-wmi: Initialize WMI drivers
> alienware-wmi: Add alienfx OPs to platdata
> alienware-wmi: Refactor LED control methods
> alienware-wmi: Refactor hdmi, amplifier, deepslp
> alienware-wmi: Add a state container for AWCC
> alienware-wmi: Migrate thermal methods to wmidev
> alienware-wmi: Refactor sysfs visibility methods
> alienware-wmi: Make running control state part of platdata
> alienware-wmi: Drop thermal methods dependency on quirks
> platform-x86: Add header file for alienware-wmi
> platform-x86: Rename alienare-wmi
> platform-x86: Split the alienware-wmi module
> platform-x86: Add config entries to alienware-wmi
>
> MAINTAINERS | 3 +-
> drivers/platform/x86/dell/Kconfig | 25 +-
> drivers/platform/x86/dell/Makefile | 5 +-
> .../platform/x86/dell/alienware-wmi-alienfx.c | 531 +++++++
> .../platform/x86/dell/alienware-wmi-awcc.c | 282 ++++
> .../platform/x86/dell/alienware-wmi-base.c | 525 +++++++
> drivers/platform/x86/dell/alienware-wmi.c | 1267 -----------------
> drivers/platform/x86/dell/alienware-wmi.h | 141 ++
> 8 files changed, 1505 insertions(+), 1274 deletions(-)
> create mode 100644 drivers/platform/x86/dell/alienware-wmi-alienfx.c
> create mode 100644 drivers/platform/x86/dell/alienware-wmi-awcc.c
> create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
> delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
> create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 00/21] alienware-wmi driver rework
2024-12-06 23:26 ` [RFC PATCH 00/21] alienware-wmi driver rework Armin Wolf
@ 2024-12-07 1:59 ` Kurt Borja
2024-12-07 3:20 ` Armin Wolf
0 siblings, 1 reply; 40+ messages in thread
From: Kurt Borja @ 2024-12-07 1:59 UTC (permalink / raw)
To: Armin Wolf
Cc: platform-driver-x86, Dell.Client.Kernel, hdegoede, ilpo.jarvinen,
linux-kernel, mario.limonciello
On Sat, Dec 07, 2024 at 12:26:20AM +0100, Armin Wolf wrote:
> Am 05.12.24 um 01:27 schrieb Kurt Borja:
>
> > Hi :)
> >
> > This series are a follow-up to this discussion [1], in which I proposed
> > migrating the alienware-wmi driver to use:
> >
> > 1. State container driver model
> > 2. Modern WMI driver design
> > 3. Drop use of deprecated WMI methods
> >
> > Of course, this was much harder than expected to do cleanly. Main
> > problem was that this driver "drives" two completely different devices
> > (I'm not referring to the WMI devices, which also happen to be two).
> >
> > Throughout these series we will call these devices AlienFX and AWCC.
> >
> > As a preamble
> > =============
> >
> > AlienFX exposes a LED, hdmi, amplifier and deepsleep interface to
> > userspace through a platform device named "alienware-wmi". Historically
> > this driver handled this by leveraging on two WMI devices as a backend.
> > This devices named LEGACY and WMAX were very similar, the only
> > difference was that WMAX had more features, but share all features
> > LEGACY had. Although it's a stretch, it could be argued this WMI devices
> > are the "same", just different GUID.
> >
> > Later Dell repurposed the WMAX WMI device to serve as a thermal control
> > interface for all newer "gaming" laptops. This new WMAX device has an
> > ACPI UID = "AWCC" (I discovered this recently). So it could also be
> > argued that old WMAX and AWCC WMAX are not the same device, just same
> > GUID.
> >
> > This drivers manages all these features using deprecated WMI methods.
>
> I think there is a misunderstanding here.
>
> The WMAX WMI device is identical with the AWCC WMI device, only the UID might be different.
> The reason why the thermal control WMI methods are not available on older WMAX devices is
> that Dell seemed to have introduced this WMI methods after the usual WMAX WMI methods.
>
> Because of this i advise against splitting WMAX (LED, attributes, ...) and AWCC functionality
> into separate files.
By examining the ACPI tables of the devices that support the AWCC
functionality, I realized none of the newer devices support the LED
interface.
At the time I added quirks for those devices I assigned `num_zones = 2`,
because I mimicked the default behavior of the driver, which was
assigning quirk_unknown to devices not listed on the DMI table.
This is of course my bad, but fortunately in all these cases the WMAX
device returned an error code safely.
I can send a fix for this, but it would require a bit of refactoring of
the init function, which I think would cause merge conflicts if we end
up reworking this driver. Also we don't know "FOR SURE" which devices
don't support the LED interface, although I'm pretty sure it comes down
to the UID of the device, but it's just a guess in the end.
Thoughts on sending a fix? I believe leaving zones is pretty harmless in
the end.
I would love to have advice from Dell on this too. Hopefully they'll
get back at us at some point. Any time now...
>
> > Approach I took for the rework
> > ==============================
> >
> > Parts 1-7 sort of containerize all AlienFX functionality under the
> > "alienware-wmi" platform driver so WMI drivers can prepare and register
> > a matching platform device from the probe.
> >
> > Parts 8-12 create and register two WMI drivers for the LEGACY and WMAX
> > devices respectively. The code for these probes is VERY similar and
> > all "differences" are passed to the platform device via platform
> > specific data (platdata). Also AlienFX functionality is refactored to
> > use non-deprecated WMI methods.
> >
> > Parts 13-17 migrate all AWCC methods to use non-deprecated WMI methods
> > and the state container driver model.
> >
> > Parts 18-21 I splitted the alienware-wmi.c module into the different
> > features this driver manages.
> >
> > alienware-wmi-base.c is in charge of initializing WMI drivers and
> > define some platform specific data, like operations (Part 10 for more
> > info). alienware-wmi-alienfx.c has all AlienFX functionality and
> > alienware-wmi-awcc.c has all AWCC functionality.
>
> I would rather split the drivers into:
>
> - alienware-wmi-legacy, which handles the LEGACY WMI device and registers a alienware-wmi platform device
>
> - alienware-wmi-wmax, which handles the modern WMAX WMI device and also registers a alienware-wmi platform device
>
> - alienware-wmi-base, which provides a driver for the alienware-wmi platform device
If you don't change your mind with the information given above, I'm ok
with this. That's why I splitted the driver at the end of the series :p
>
> This of course only works if the LEGACY WMI device and the WMAX WMI device are newer both present at the same time,
> in this case alienware-wmi-legacy could use wmi_has_guid() as a band aid check to avoid probing if a WMAX WMI device
> is present.
>
> Using the platform_data mechanism to decouple the alienware-wmi device driver from the underlying hardware implementation
> should be fine IMHO.
This is good to know!
>
> > Coments
> > =======
> >
> > This is still kind of a draft, but I did some testing and it works!
> >
> > Of course I will do thorough testing and cleanup when I send the
> > non-RFC version. I just want to get some comments on the general
> > approach before proceeding further.
> >
> > I think this is quite messy in it's current state so I apollogize.
> >
> > @Mario Limonciello: I included the reviews you gave me on [2]. I
> > included some of those patches here, and dropped the ones that did not
> > make sense with this design. As this is another series let me know if
> > you want me to drop the tags!
> >
> > @Armin Wolf: I don't like the amount of files I made. As the maintainer
> > of the wmi module, what do you think about making two independent
> > modules, one for AlienFX and one for AWCC. In order to not register two
> > drivers for the WMAX device the module init would check if the "AWCC"
> > UID is present.
>
> I know of at least one device which support both AWCC thermal control and
> WMAX LED control, so splitting the WMAX device driver like this could cause
> problems.
>
> Like i said before, you should view the WMAX WMI device as having different
> capabilities (= WMI methods) depending of the machine the kernel is running on.
Yes, it's really unfortunate Dell didn't make a new device for the
thermal methods.
>
> If a capability is available (currently determined via quirks), the driver should
> do the necessary things to handle it.
>
> As a side note: i am currently exploring if we can decode the WMI BMOF buffers inside
> the kernel, so that in the far future we can remove those quirks and automatically detect
> which methods are available. But this will take a long time, so it has nothing to do with
> this patch series.
This would be an awesome feature! Will you implement Pali Rohar's decoder?
I'll be sure to make the necessary improvements once is done.
>
> I will take a look at the other patches tomorrow.
Thank you very much!
~ Kurt
>
> Thanks,
> Armin Wolf
>
> >
> > The approach for that would be basically the same, and I think the
> > series would change very little.
> >
> > I would like this a lot because I still think old and new WMAX devices
> > are different, but I couldn't find another example of where an OEM
> > repurposed a WMI device.
> >
> > @Everyone: I know this is VERY long. Thank you so much for your time in
> > advance!
> >
> > This series were made on top of the 'for-next' branch:
> >
> > Commit c712e8fd9bf4 ("MAINTAINERS: Change AMD PMC driver status to "Supported"")
> >
> > ~ Kurt
> >
> > [1] https://lore.kernel.org/platform-driver-x86/6m66cuivkzhcsvpjv4nunjyddqhr42bmjdhptu4bqm6rm7fvxf@qjwove4hg6gb/T/#u
> > [2] https://lore.kernel.org/platform-driver-x86/20241120163834.6446-3-kuurtb@gmail.com/
> >
> > Kurt Borja (21):
> > alienware-wmi: Modify parse_rgb() signature
> > alienware-wmi: Move Lighting Control State
> > alienware-wmi: Remove unnecessary check at module exit
> > alienware-wmi: Improve sysfs groups creation
> > alienware-wmi: Refactor rgb-zones sysfs group creation
> > alienware-wmi: Add state container and alienfx_probe()
> > alienware-wmi: Migrate to state container pattern
> > alienware-wmi: Add WMI Drivers
> > alienware-wmi: Initialize WMI drivers
> > alienware-wmi: Add alienfx OPs to platdata
> > alienware-wmi: Refactor LED control methods
> > alienware-wmi: Refactor hdmi, amplifier, deepslp
> > alienware-wmi: Add a state container for AWCC
> > alienware-wmi: Migrate thermal methods to wmidev
> > alienware-wmi: Refactor sysfs visibility methods
> > alienware-wmi: Make running control state part of platdata
> > alienware-wmi: Drop thermal methods dependency on quirks
> > platform-x86: Add header file for alienware-wmi
> > platform-x86: Rename alienare-wmi
> > platform-x86: Split the alienware-wmi module
> > platform-x86: Add config entries to alienware-wmi
> >
> > MAINTAINERS | 3 +-
> > drivers/platform/x86/dell/Kconfig | 25 +-
> > drivers/platform/x86/dell/Makefile | 5 +-
> > .../platform/x86/dell/alienware-wmi-alienfx.c | 531 +++++++
> > .../platform/x86/dell/alienware-wmi-awcc.c | 282 ++++
> > .../platform/x86/dell/alienware-wmi-base.c | 525 +++++++
> > drivers/platform/x86/dell/alienware-wmi.c | 1267 -----------------
> > drivers/platform/x86/dell/alienware-wmi.h | 141 ++
> > 8 files changed, 1505 insertions(+), 1274 deletions(-)
> > create mode 100644 drivers/platform/x86/dell/alienware-wmi-alienfx.c
> > create mode 100644 drivers/platform/x86/dell/alienware-wmi-awcc.c
> > create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
> > delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
> > create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
> >
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 00/21] alienware-wmi driver rework
2024-12-07 1:59 ` Kurt Borja
@ 2024-12-07 3:20 ` Armin Wolf
2024-12-07 3:47 ` Kurt Borja
0 siblings, 1 reply; 40+ messages in thread
From: Armin Wolf @ 2024-12-07 3:20 UTC (permalink / raw)
To: Kurt Borja
Cc: platform-driver-x86, Dell.Client.Kernel, hdegoede, ilpo.jarvinen,
linux-kernel, mario.limonciello
Am 07.12.24 um 02:59 schrieb Kurt Borja:
> On Sat, Dec 07, 2024 at 12:26:20AM +0100, Armin Wolf wrote:
>> Am 05.12.24 um 01:27 schrieb Kurt Borja:
>>
>>> Hi :)
>>>
>>> This series are a follow-up to this discussion [1], in which I proposed
>>> migrating the alienware-wmi driver to use:
>>>
>>> 1. State container driver model
>>> 2. Modern WMI driver design
>>> 3. Drop use of deprecated WMI methods
>>>
>>> Of course, this was much harder than expected to do cleanly. Main
>>> problem was that this driver "drives" two completely different devices
>>> (I'm not referring to the WMI devices, which also happen to be two).
>>>
>>> Throughout these series we will call these devices AlienFX and AWCC.
>>>
>>> As a preamble
>>> =============
>>>
>>> AlienFX exposes a LED, hdmi, amplifier and deepsleep interface to
>>> userspace through a platform device named "alienware-wmi". Historically
>>> this driver handled this by leveraging on two WMI devices as a backend.
>>> This devices named LEGACY and WMAX were very similar, the only
>>> difference was that WMAX had more features, but share all features
>>> LEGACY had. Although it's a stretch, it could be argued this WMI devices
>>> are the "same", just different GUID.
>>>
>>> Later Dell repurposed the WMAX WMI device to serve as a thermal control
>>> interface for all newer "gaming" laptops. This new WMAX device has an
>>> ACPI UID = "AWCC" (I discovered this recently). So it could also be
>>> argued that old WMAX and AWCC WMAX are not the same device, just same
>>> GUID.
>>>
>>> This drivers manages all these features using deprecated WMI methods.
>> I think there is a misunderstanding here.
>>
>> The WMAX WMI device is identical with the AWCC WMI device, only the UID might be different.
>> The reason why the thermal control WMI methods are not available on older WMAX devices is
>> that Dell seemed to have introduced this WMI methods after the usual WMAX WMI methods.
>>
>> Because of this i advise against splitting WMAX (LED, attributes, ...) and AWCC functionality
>> into separate files.
> By examining the ACPI tables of the devices that support the AWCC
> functionality, I realized none of the newer devices support the LED
> interface.
You are right, i misread the decoded bmof buffer xd.
> At the time I added quirks for those devices I assigned `num_zones = 2`,
> because I mimicked the default behavior of the driver, which was
> assigning quirk_unknown to devices not listed on the DMI table.
>
> This is of course my bad, but fortunately in all these cases the WMAX
> device returned an error code safely.
>
> I can send a fix for this, but it would require a bit of refactoring of
> the init function, which I think would cause merge conflicts if we end
> up reworking this driver. Also we don't know "FOR SURE" which devices
> don't support the LED interface, although I'm pretty sure it comes down
> to the UID of the device, but it's just a guess in the end.
If you do not know for sure which devices _you_ added support the LED interface, then
i would prefer to remove the "num_zones = 2" quirk from those devices for now.
> Thoughts on sending a fix? I believe leaving zones is pretty harmless in
> the end.
Please send a fix for your quirk entries, so we can avoid forgetting this little detail.
> I would love to have advice from Dell on this too. Hopefully they'll
> get back at us at some point. Any time now...
>
>>> Approach I took for the rework
>>> ==============================
>>>
>>> Parts 1-7 sort of containerize all AlienFX functionality under the
>>> "alienware-wmi" platform driver so WMI drivers can prepare and register
>>> a matching platform device from the probe.
>>>
>>> Parts 8-12 create and register two WMI drivers for the LEGACY and WMAX
>>> devices respectively. The code for these probes is VERY similar and
>>> all "differences" are passed to the platform device via platform
>>> specific data (platdata). Also AlienFX functionality is refactored to
>>> use non-deprecated WMI methods.
>>>
>>> Parts 13-17 migrate all AWCC methods to use non-deprecated WMI methods
>>> and the state container driver model.
>>>
>>> Parts 18-21 I splitted the alienware-wmi.c module into the different
>>> features this driver manages.
>>>
>>> alienware-wmi-base.c is in charge of initializing WMI drivers and
>>> define some platform specific data, like operations (Part 10 for more
>>> info). alienware-wmi-alienfx.c has all AlienFX functionality and
>>> alienware-wmi-awcc.c has all AWCC functionality.
>> I would rather split the drivers into:
>>
>> - alienware-wmi-legacy, which handles the LEGACY WMI device and registers a alienware-wmi platform device
>>
>> - alienware-wmi-wmax, which handles the modern WMAX WMI device and also registers a alienware-wmi platform device
>>
>> - alienware-wmi-base, which provides a driver for the alienware-wmi platform device
> If you don't change your mind with the information given above, I'm ok
> with this. That's why I splitted the driver at the end of the series :p
I did not change my mind.
I can understand that most devices either support the original WMAX WMI methods or the AWCC WMI methods,
but from a technical point of view it is still the same device. Also Dell could combine both sets of WMI methods
in a future device, and i would prefer being prepared for that.
We can still split alienware-wmi-wmax into multiple files (which get linked together) later should the source code
of it get too big in the future.
Also having a separate alienware-wmi-legacy would allow users to disable this driver when building the kernel.
>> This of course only works if the LEGACY WMI device and the WMAX WMI device are newer both present at the same time,
>> in this case alienware-wmi-legacy could use wmi_has_guid() as a band aid check to avoid probing if a WMAX WMI device
>> is present.
>>
>> Using the platform_data mechanism to decouple the alienware-wmi device driver from the underlying hardware implementation
>> should be fine IMHO.
> This is good to know!
>
>>> Coments
>>> =======
>>>
>>> This is still kind of a draft, but I did some testing and it works!
>>>
>>> Of course I will do thorough testing and cleanup when I send the
>>> non-RFC version. I just want to get some comments on the general
>>> approach before proceeding further.
>>>
>>> I think this is quite messy in it's current state so I apollogize.
>>>
>>> @Mario Limonciello: I included the reviews you gave me on [2]. I
>>> included some of those patches here, and dropped the ones that did not
>>> make sense with this design. As this is another series let me know if
>>> you want me to drop the tags!
>>>
>>> @Armin Wolf: I don't like the amount of files I made. As the maintainer
>>> of the wmi module, what do you think about making two independent
>>> modules, one for AlienFX and one for AWCC. In order to not register two
>>> drivers for the WMAX device the module init would check if the "AWCC"
>>> UID is present.
>> I know of at least one device which support both AWCC thermal control and
>> WMAX LED control, so splitting the WMAX device driver like this could cause
>> problems.
>>
>> Like i said before, you should view the WMAX WMI device as having different
>> capabilities (= WMI methods) depending of the machine the kernel is running on.
> Yes, it's really unfortunate Dell didn't make a new device for the
> thermal methods.
I agree, sadly this god object architecture is very common with some hardware manufactures :(
>
>> If a capability is available (currently determined via quirks), the driver should
>> do the necessary things to handle it.
>>
>> As a side note: i am currently exploring if we can decode the WMI BMOF buffers inside
>> the kernel, so that in the far future we can remove those quirks and automatically detect
>> which methods are available. But this will take a long time, so it has nothing to do with
>> this patch series.
> This would be an awesome feature! Will you implement Pali Rohar's decoder?
> I'll be sure to make the necessary improvements once is done.
I will base my work on Pali Rohars decoder, but sadly the source code is quite convoluted, so i will
need to do some reverse-engineering.
The decompression alogrithm is already finished:
https://github.com/Wer-Wolf/libdeds
Thanks,
Armin Wolf
>
>> I will take a look at the other patches tomorrow.
> Thank you very much!
>
> ~ Kurt
>
>> Thanks,
>> Armin Wolf
>>
>>> The approach for that would be basically the same, and I think the
>>> series would change very little.
>>>
>>> I would like this a lot because I still think old and new WMAX devices
>>> are different, but I couldn't find another example of where an OEM
>>> repurposed a WMI device.
>>>
>>> @Everyone: I know this is VERY long. Thank you so much for your time in
>>> advance!
>>>
>>> This series were made on top of the 'for-next' branch:
>>>
>>> Commit c712e8fd9bf4 ("MAINTAINERS: Change AMD PMC driver status to "Supported"")
>>>
>>> ~ Kurt
>>>
>>> [1] https://lore.kernel.org/platform-driver-x86/6m66cuivkzhcsvpjv4nunjyddqhr42bmjdhptu4bqm6rm7fvxf@qjwove4hg6gb/T/#u
>>> [2] https://lore.kernel.org/platform-driver-x86/20241120163834.6446-3-kuurtb@gmail.com/
>>>
>>> Kurt Borja (21):
>>> alienware-wmi: Modify parse_rgb() signature
>>> alienware-wmi: Move Lighting Control State
>>> alienware-wmi: Remove unnecessary check at module exit
>>> alienware-wmi: Improve sysfs groups creation
>>> alienware-wmi: Refactor rgb-zones sysfs group creation
>>> alienware-wmi: Add state container and alienfx_probe()
>>> alienware-wmi: Migrate to state container pattern
>>> alienware-wmi: Add WMI Drivers
>>> alienware-wmi: Initialize WMI drivers
>>> alienware-wmi: Add alienfx OPs to platdata
>>> alienware-wmi: Refactor LED control methods
>>> alienware-wmi: Refactor hdmi, amplifier, deepslp
>>> alienware-wmi: Add a state container for AWCC
>>> alienware-wmi: Migrate thermal methods to wmidev
>>> alienware-wmi: Refactor sysfs visibility methods
>>> alienware-wmi: Make running control state part of platdata
>>> alienware-wmi: Drop thermal methods dependency on quirks
>>> platform-x86: Add header file for alienware-wmi
>>> platform-x86: Rename alienare-wmi
>>> platform-x86: Split the alienware-wmi module
>>> platform-x86: Add config entries to alienware-wmi
>>>
>>> MAINTAINERS | 3 +-
>>> drivers/platform/x86/dell/Kconfig | 25 +-
>>> drivers/platform/x86/dell/Makefile | 5 +-
>>> .../platform/x86/dell/alienware-wmi-alienfx.c | 531 +++++++
>>> .../platform/x86/dell/alienware-wmi-awcc.c | 282 ++++
>>> .../platform/x86/dell/alienware-wmi-base.c | 525 +++++++
>>> drivers/platform/x86/dell/alienware-wmi.c | 1267 -----------------
>>> drivers/platform/x86/dell/alienware-wmi.h | 141 ++
>>> 8 files changed, 1505 insertions(+), 1274 deletions(-)
>>> create mode 100644 drivers/platform/x86/dell/alienware-wmi-alienfx.c
>>> create mode 100644 drivers/platform/x86/dell/alienware-wmi-awcc.c
>>> create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
>>> delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
>>> create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
>>>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH 00/21] alienware-wmi driver rework
2024-12-07 3:20 ` Armin Wolf
@ 2024-12-07 3:47 ` Kurt Borja
0 siblings, 0 replies; 40+ messages in thread
From: Kurt Borja @ 2024-12-07 3:47 UTC (permalink / raw)
To: Armin Wolf
Cc: platform-driver-x86, Dell.Client.Kernel, hdegoede, ilpo.jarvinen,
linux-kernel, mario.limonciello
On Sat, Dec 07, 2024 at 04:20:09AM +0100, Armin Wolf wrote:
> Am 07.12.24 um 02:59 schrieb Kurt Borja:
>
> > On Sat, Dec 07, 2024 at 12:26:20AM +0100, Armin Wolf wrote:
> > > Am 05.12.24 um 01:27 schrieb Kurt Borja:
> > >
> > > > Hi :)
> > > >
> > > > This series are a follow-up to this discussion [1], in which I proposed
> > > > migrating the alienware-wmi driver to use:
> > > >
> > > > 1. State container driver model
> > > > 2. Modern WMI driver design
> > > > 3. Drop use of deprecated WMI methods
> > > >
> > > > Of course, this was much harder than expected to do cleanly. Main
> > > > problem was that this driver "drives" two completely different devices
> > > > (I'm not referring to the WMI devices, which also happen to be two).
> > > >
> > > > Throughout these series we will call these devices AlienFX and AWCC.
> > > >
> > > > As a preamble
> > > > =============
> > > >
> > > > AlienFX exposes a LED, hdmi, amplifier and deepsleep interface to
> > > > userspace through a platform device named "alienware-wmi". Historically
> > > > this driver handled this by leveraging on two WMI devices as a backend.
> > > > This devices named LEGACY and WMAX were very similar, the only
> > > > difference was that WMAX had more features, but share all features
> > > > LEGACY had. Although it's a stretch, it could be argued this WMI devices
> > > > are the "same", just different GUID.
> > > >
> > > > Later Dell repurposed the WMAX WMI device to serve as a thermal control
> > > > interface for all newer "gaming" laptops. This new WMAX device has an
> > > > ACPI UID = "AWCC" (I discovered this recently). So it could also be
> > > > argued that old WMAX and AWCC WMAX are not the same device, just same
> > > > GUID.
> > > >
> > > > This drivers manages all these features using deprecated WMI methods.
> > > I think there is a misunderstanding here.
> > >
> > > The WMAX WMI device is identical with the AWCC WMI device, only the UID might be different.
> > > The reason why the thermal control WMI methods are not available on older WMAX devices is
> > > that Dell seemed to have introduced this WMI methods after the usual WMAX WMI methods.
> > >
> > > Because of this i advise against splitting WMAX (LED, attributes, ...) and AWCC functionality
> > > into separate files.
> > By examining the ACPI tables of the devices that support the AWCC
> > functionality, I realized none of the newer devices support the LED
> > interface.
>
> You are right, i misread the decoded bmof buffer xd.
>
> > At the time I added quirks for those devices I assigned `num_zones = 2`,
> > because I mimicked the default behavior of the driver, which was
> > assigning quirk_unknown to devices not listed on the DMI table.
> >
> > This is of course my bad, but fortunately in all these cases the WMAX
> > device returned an error code safely.
> >
> > I can send a fix for this, but it would require a bit of refactoring of
> > the init function, which I think would cause merge conflicts if we end
> > up reworking this driver. Also we don't know "FOR SURE" which devices
> > don't support the LED interface, although I'm pretty sure it comes down
> > to the UID of the device, but it's just a guess in the end.
>
> If you do not know for sure which devices _you_ added support the LED interface, then
> i would prefer to remove the "num_zones = 2" quirk from those devices for now.
>
> > Thoughts on sending a fix? I believe leaving zones is pretty harmless in
> > the end.
>
> Please send a fix for your quirk entries, so we can avoid forgetting this little detail.
Sure! I'll fix this weekend.
>
> > I would love to have advice from Dell on this too. Hopefully they'll
> > get back at us at some point. Any time now...
> >
> > > > Approach I took for the rework
> > > > ==============================
> > > >
> > > > Parts 1-7 sort of containerize all AlienFX functionality under the
> > > > "alienware-wmi" platform driver so WMI drivers can prepare and register
> > > > a matching platform device from the probe.
> > > >
> > > > Parts 8-12 create and register two WMI drivers for the LEGACY and WMAX
> > > > devices respectively. The code for these probes is VERY similar and
> > > > all "differences" are passed to the platform device via platform
> > > > specific data (platdata). Also AlienFX functionality is refactored to
> > > > use non-deprecated WMI methods.
> > > >
> > > > Parts 13-17 migrate all AWCC methods to use non-deprecated WMI methods
> > > > and the state container driver model.
> > > >
> > > > Parts 18-21 I splitted the alienware-wmi.c module into the different
> > > > features this driver manages.
> > > >
> > > > alienware-wmi-base.c is in charge of initializing WMI drivers and
> > > > define some platform specific data, like operations (Part 10 for more
> > > > info). alienware-wmi-alienfx.c has all AlienFX functionality and
> > > > alienware-wmi-awcc.c has all AWCC functionality.
> > > I would rather split the drivers into:
> > >
> > > - alienware-wmi-legacy, which handles the LEGACY WMI device and registers a alienware-wmi platform device
> > >
> > > - alienware-wmi-wmax, which handles the modern WMAX WMI device and also registers a alienware-wmi platform device
> > >
> > > - alienware-wmi-base, which provides a driver for the alienware-wmi platform device
> > If you don't change your mind with the information given above, I'm ok
> > with this. That's why I splitted the driver at the end of the series :p
>
> I did not change my mind.
>
> I can understand that most devices either support the original WMAX WMI methods or the AWCC WMI methods,
> but from a technical point of view it is still the same device. Also Dell could combine both sets of WMI methods
> in a future device, and i would prefer being prepared for that.
>
> We can still split alienware-wmi-wmax into multiple files (which get linked together) later should the source code
> of it get too big in the future.
>
> Also having a separate alienware-wmi-legacy would allow users to disable this driver when building the kernel.
Makes sense, I'll split it that way in the next iteration.
>
> > > This of course only works if the LEGACY WMI device and the WMAX WMI device are newer both present at the same time,
> > > in this case alienware-wmi-legacy could use wmi_has_guid() as a band aid check to avoid probing if a WMAX WMI device
> > > is present.
> > >
> > > Using the platform_data mechanism to decouple the alienware-wmi device driver from the underlying hardware implementation
> > > should be fine IMHO.
> > This is good to know!
> >
> > > > Coments
> > > > =======
> > > >
> > > > This is still kind of a draft, but I did some testing and it works!
> > > >
> > > > Of course I will do thorough testing and cleanup when I send the
> > > > non-RFC version. I just want to get some comments on the general
> > > > approach before proceeding further.
> > > >
> > > > I think this is quite messy in it's current state so I apollogize.
> > > >
> > > > @Mario Limonciello: I included the reviews you gave me on [2]. I
> > > > included some of those patches here, and dropped the ones that did not
> > > > make sense with this design. As this is another series let me know if
> > > > you want me to drop the tags!
> > > >
> > > > @Armin Wolf: I don't like the amount of files I made. As the maintainer
> > > > of the wmi module, what do you think about making two independent
> > > > modules, one for AlienFX and one for AWCC. In order to not register two
> > > > drivers for the WMAX device the module init would check if the "AWCC"
> > > > UID is present.
> > > I know of at least one device which support both AWCC thermal control and
> > > WMAX LED control, so splitting the WMAX device driver like this could cause
> > > problems.
> > >
> > > Like i said before, you should view the WMAX WMI device as having different
> > > capabilities (= WMI methods) depending of the machine the kernel is running on.
> > Yes, it's really unfortunate Dell didn't make a new device for the
> > thermal methods.
>
> I agree, sadly this god object architecture is very common with some hardware manufactures :(
>
> >
> > > If a capability is available (currently determined via quirks), the driver should
> > > do the necessary things to handle it.
> > >
> > > As a side note: i am currently exploring if we can decode the WMI BMOF buffers inside
> > > the kernel, so that in the far future we can remove those quirks and automatically detect
> > > which methods are available. But this will take a long time, so it has nothing to do with
> > > this patch series.
> > This would be an awesome feature! Will you implement Pali Rohar's decoder?
> > I'll be sure to make the necessary improvements once is done.
>
> I will base my work on Pali Rohars decoder, but sadly the source code is quite convoluted, so i will
> need to do some reverse-engineering.
>
> The decompression alogrithm is already finished:
>
> https://github.com/Wer-Wolf/libdeds
Wow this is MUCH more readable now, nice work! I remember pulling gdb
just to get a general idea of what was going on. The parsing code should
be able to be simplified, but it is indeed very convoluted.
>
> Thanks,
> Armin Wolf
>
> >
> > > I will take a look at the other patches tomorrow.
> > Thank you very much!
> >
> > ~ Kurt
> >
> > > Thanks,
> > > Armin Wolf
> > >
> > > > The approach for that would be basically the same, and I think the
> > > > series would change very little.
> > > >
> > > > I would like this a lot because I still think old and new WMAX devices
> > > > are different, but I couldn't find another example of where an OEM
> > > > repurposed a WMI device.
> > > >
> > > > @Everyone: I know this is VERY long. Thank you so much for your time in
> > > > advance!
> > > >
> > > > This series were made on top of the 'for-next' branch:
> > > >
> > > > Commit c712e8fd9bf4 ("MAINTAINERS: Change AMD PMC driver status to "Supported"")
> > > >
> > > > ~ Kurt
> > > >
> > > > [1] https://lore.kernel.org/platform-driver-x86/6m66cuivkzhcsvpjv4nunjyddqhr42bmjdhptu4bqm6rm7fvxf@qjwove4hg6gb/T/#u
> > > > [2] https://lore.kernel.org/platform-driver-x86/20241120163834.6446-3-kuurtb@gmail.com/
> > > >
> > > > Kurt Borja (21):
> > > > alienware-wmi: Modify parse_rgb() signature
> > > > alienware-wmi: Move Lighting Control State
> > > > alienware-wmi: Remove unnecessary check at module exit
> > > > alienware-wmi: Improve sysfs groups creation
> > > > alienware-wmi: Refactor rgb-zones sysfs group creation
> > > > alienware-wmi: Add state container and alienfx_probe()
> > > > alienware-wmi: Migrate to state container pattern
> > > > alienware-wmi: Add WMI Drivers
> > > > alienware-wmi: Initialize WMI drivers
> > > > alienware-wmi: Add alienfx OPs to platdata
> > > > alienware-wmi: Refactor LED control methods
> > > > alienware-wmi: Refactor hdmi, amplifier, deepslp
> > > > alienware-wmi: Add a state container for AWCC
> > > > alienware-wmi: Migrate thermal methods to wmidev
> > > > alienware-wmi: Refactor sysfs visibility methods
> > > > alienware-wmi: Make running control state part of platdata
> > > > alienware-wmi: Drop thermal methods dependency on quirks
> > > > platform-x86: Add header file for alienware-wmi
> > > > platform-x86: Rename alienare-wmi
> > > > platform-x86: Split the alienware-wmi module
> > > > platform-x86: Add config entries to alienware-wmi
> > > >
> > > > MAINTAINERS | 3 +-
> > > > drivers/platform/x86/dell/Kconfig | 25 +-
> > > > drivers/platform/x86/dell/Makefile | 5 +-
> > > > .../platform/x86/dell/alienware-wmi-alienfx.c | 531 +++++++
> > > > .../platform/x86/dell/alienware-wmi-awcc.c | 282 ++++
> > > > .../platform/x86/dell/alienware-wmi-base.c | 525 +++++++
> > > > drivers/platform/x86/dell/alienware-wmi.c | 1267 -----------------
> > > > drivers/platform/x86/dell/alienware-wmi.h | 141 ++
> > > > 8 files changed, 1505 insertions(+), 1274 deletions(-)
> > > > create mode 100644 drivers/platform/x86/dell/alienware-wmi-alienfx.c
> > > > create mode 100644 drivers/platform/x86/dell/alienware-wmi-awcc.c
> > > > create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
> > > > delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
> > > > create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
> > > >
^ permalink raw reply [flat|nested] 40+ messages in thread