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@xxxxxxxxx/
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
