Hi John, On 1/10/23 14:26, John Martens wrote: > Dear Kernel devs, Mr. Hans de Goede, Mr. Ike Panhc, > > I am currently working on a driver for fan control, fan speed, temperature sensors, and power mode (platform profile) for Lenovo Legion Laptops. Switching iGPU/dGPU could also be possible. It is a port of the closed and open tools in Windows LenovoLegionToolkit, Vantage, LegionFanControl. I am testing it on different laptops with the help of a forum/chat and its working quite good. > > There is a README (https://github.com/johnfanv2/LenovoLegionLinux) and code (https://github.com/johnfanv2/LenovoLegionLinux/blob/main/kernel_module/legion-laptop.c). > > I would be interested to get your opinion. > > Questions > > Should this extend ideapad_laptop.c or a new file? > - pro: > - both access parts of the same hardware > - con: > - both files are already quite large > - it only works on Lenovo Legion laptops that have this > custom control firmware in the embedded controller (EC) > - there is almost no reuse of code As you say there is almost 0 code re-use so I don't think that adding this to ideapad_laptop.c is worth the trouble, ideapad_laptop.c already is big enough as is. I do see that you bind to the PNP0C09 or VPC2004 device. Since you poke the EC RAM and don't make any ACPI calls AFAICT it would be best to just create your own lenovo_legion platform_device to bind to, using platform_create_bundle() which will create a platform_device + platform_driver for you in one go. This will avoid conflicts with other drivers binding to the VPC2004 device such as ideapad_laptop. And you can then also specify the used superio mem or io-ports as resources (and claim then in your probe()) so that these properly show up in /proc/ioports and so that conflicts with other drivers also banging on these ports get detected. > Which method do you prefer writing to EC memory for older models? With ioremap or outb? > - To use ioremap one needs to get the start address. It is > different on Intel vs AMD. It is the same as a OperationRegion > in the ACPI tables, e.g. "OperationRegion (ERAX, SystemMemory, > 0xFE00D400, 0xFF)". However, I have found no kernel functions > to get the address (here 0xFE00D400) of a OperationRegion. > One could also hardcode it for each model/firmware. These addresses sometimes change at boot-time when different BIOS options are enabled/disabled so I would advice against hardcoding them. Have you looked in: sudo cat /proc/iomem To see if the range is perhaps known to the kernel somehow already ? > - alternative (which I am currently using) is sending commands > to IO ports 0x4E/0x4F (Super IO controller). I wonder if these ports are not already in use by some other driver causing possible conflicts. Are they not listed in "sudo cat /proc/ioports" / > Background > > The laptops come with an embedded controller (EC) from ITE. These usually come with a 3 point fan curve in ROM, but also can be flashed with a small additional custom program. Lenovo implemented implemented a 10 point fan curve. The program is also shipped with each EFI update. > > The fan curve can be edited by writing to some memory locations in the EC. These locations are > > The driver works by: > - directly writing/reading embedded controller memory > - older models (2020-2021): there are two possibilities > - the EC memory is already memory mapped, so one can > use ioremap > - one can use outb/inb and write sequenc of commands to > port 0x43, 0x4F (super IO ports) > - ideapad_laptop.c writes to some parts of EC memory > with ACPI methods VPCR, VPCW. However, these do not seem > to work in the memory region with the fan curve. Ok, so lets see of we can figure out a better way to get the EC memory address and if not I guess outb/inb it is. > - newer models (2022): these provide ACPI/WMI methods > setFanCurve/getFanCurve to write to these regions. However, I > have implemented that and have no models for testing >From the sound of it we should definitely use this new methods on these newer models, this should protect us against the EC RAM layout changing. This might be best done in separate driver, that depends on how much code we can re-use if we put support for the new models inside the same driver. Regards, Hans
