On Wed, Nov 20, 2024 at 10:07:59PM +0100, Armin Wolf wrote: > Am 20.11.24 um 16:04 schrieb Kurt Borja: > > > On Wed, Nov 20, 2024 at 01:06:57PM +0100, Armin Wolf wrote: > > > Am 19.11.24 um 05:29 schrieb Kurt Borja: > > > > > > > On Mon, Nov 18, 2024 at 09:54:25PM -0600, Mario Limonciello wrote: > > > > > Loop Dell Client Kernel M/B for any comments. > > > > > > > > > > On 11/18/2024 21:47, Kurt Borja wrote: > > > > > > Hi! > > > > > > > > > > > > I'm planning on migrating the alienware-wmi driver to the new WMI > > > > > > interface, as it's currently using the deprecated one. > > > > > 🎉 > > > I like this :) > > > > > > > > > My plan is to: > > > > > > > > > > > > rename alienware-wmi.c -> alienware-wmi-base.c > > > > > > create alienware-wmi.h > > > > > > create alienware-wmi-legacy > > > > > > create alienware-wmi-wmax > > > > > > > > > > > > The last two files would not be independent modules, just includes for > > > > > > the base module. The base module would be in charge of initializing the > > > > > > platform driver plus the correct wmi_driver backend, but the wmi probes > > > > > > would register the platform device. This would be very similar to what > > > > > > other dell drivers already do. Aditionally I want to migrate everything > > > > > > to the state container design pattern. > > > > > > > > > > > > I would do this in such a way that the legacy and new code would be > > > > > > completely independent of each other (i.e. different state containters, > > > > > > dmi checks, etc). > > > > > As the original author of this driver when I was at Dell I'll add some > > > > > comments. > > > > > > > > > > The 'legacy' code was very narrowly focused for supporting a handful of > > > > > hardware specifically for lighting control. One was the original Alienware > > > > > steam machine, and then a few generations of the X51. > > > > > > > > > > I don't know how much of the driver continues to work on hardware since > > > > > then. Maybe Dell guys I added to CC can comment on how much of this has > > > > > stuck around over the years and keeps working. > > > > My guess is that none of it works on new models. The LEGACY wmi device > > > > is not longer included on new machines, as all lighting control is done > > > > through an EC and the WMAX device was repurposed to fan/thermal control. > > > > I say this based on exploring quite a few acpidumps and a couple RGB > > > > control Windows open source alternatives. > > > > > > > > > > Pros: > > > > > > - Modern interfaces and design patterns > > > > > > - This is compatible with Mario's upcoming platform profile changes as > > > > > > the WMAX device would hold a reference to the platform device > > > > > > - Would not break compatibility as legacy code is independent > > > > > > - Easier to understand and develop in the future > > > > > > > > > > > > Cons: > > > > > > - Initialy alienware-wmi-base.c would be almost completely legacy code, > > > > > > as new features don't require a platform device (yet), so > > > > > > alienware-wmi-base would basically just register the wmax wmi driver > > > > > > on newer machines > > > > > > - With this design users would not be able to completely exclude legacy > > > > > > code with CONFIG parameters > > > > > I wonder if you're better off just having the legacy driver as it's own > > > > > kernel object? If it only supports a handful of systems, most people won't > > > > > need it compiled. > > > > Yes! I'd like to do this but unfortunately some user space applications > > > > might depend on attributes being available to a platform device named > > > > "alienware-wmi". This is why I wanted to have a unified "alienware-wmi" > > > > platform driver. > > > > > > > > Thank you for your feedback! > > > It see, that is unfortunate. In this case having a single driver which handles the platform device > > > and calls the right initialization function from the other two files sounds like a good choice to me. > > I have another idea: > > > > rename alienware-wmi.c -> alienware-alienfx.c > > create alienware-wmi-wmax.c > > > > Both independent modules that can be included/excluded with config > > parameters, etc. alienware-alienfx would register the platform driver > > and the wmi legacy device driver, while alienware-wmi-wmax would do > > everything independently and only register an "alienware-wmi" platform > > device if loaded on a 'legacy' laptop model. > > > > Of course I'd make sure a platform device with the same name is not loaded > > twice. > > > > Something about this doesn't feel right but I don't know what it is. > > However it is less files and less memory footprint, because ideally, > > 'legacy' code would not even load on newer models. > > > > I thought about this because Mario said legacy interface is only > > supported by a handful of devices and it seems like a waste of resources > > to load unsupported interfaces. > > I think the WMI driver for the WMAX interface still has to always register the platform device > since nearly all custom attributes (deepsleep, etc) work on the WMAX interface. > > I would prefer to have a single driver which when using the legacy WMI interface will only provide > lighting features (correct me if the legacy WMI interface supports more features) but will provide > all features when using the WMAX WMI interface. Ack. > > The overhead of handling the legacy WMI interface should be acceptable since it only provides lighting > services, so a lot of the code can be shared with the lighting code for the WMAX WMI interface. > > Making support for the legacy WMI interface configurable when building the kernel also allows people to > trim the resource consumption should they not want legacy support. > > Thanks, > Armin Wolf I'll do it this way then. I'll send v1 next week or so. Thank you for your feedback! Kurt > > > > Thanks, > > > Armin Wolf > > > > > > > > > After this I want to add HWMON and sparse keymap capabilities to the > > > > > > wmax interface. > > > > > 🎉 > > > > > > > > > > > I'm sure there are things I'm not seeing so feedback is greatly > > > > > > appreciated! > > > > > > > > > > > > Regards, > > > > > > Kurt
