Hi, Before diving into a full review of the driver I thought it would be good to first catch up with the email thread sofar. I agree with everything discussed so far. One clarifying remark below: On 2/15/23 22:27, Nikita Kravets wrote: <snip> >> This will start poking the embedded controller when the module is loaded, regardless of the platform. I am not sure that is desirable. > > It only reads though, can it cause any harm? I have seen cases where some weird (i2c) hw reacts to reads as if they are writes. But since this is using the standardized ACPI EC access routines I believe that doing reads should generally speaking be safe. Also it seems that atm the module must always be loaded manually ? I don't see any MODULE_ALIAS or MODULE_DEVICE_TABLE entries in the driver to make it auto-load. I think this should get a dmi_system_id tables with known MSI DMI_SYS_VENDOR() matches in there + a MODULE_DEVICE_TABLE() pointing to the dmi_system_id table to have the driver auto-load on MSI systems. I think what we should do here is: 1. Get the module in the mainline kernel 2. Do a blogpost asking MSI laptop users to test 3. Assuming testing does not show any regressions / issues (it will likely show lots of unsupported fw versions) add the MODULE_DEVICE_TABLE(). Regards, Hans
