I guess I am late on the party on [1], just reviewed the series. Quite a nice series Given there is a class device for this now, it would make sense to me that "tunings" for each platform driver would go there Antheas [1] https://lore.kernel.org/all/20241206031918.1537-11-mario.limonciello@xxxxxxx/ On Sun, 29 Dec 2024 at 23:41, Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote: > > Hi Armin, > indeed you covered everything. > > I am a bit hesitant about binding sppt, fppt, and spl into those > interfaces as they need to be set in a very specific ordering and > rules. E.g., spl < sppt < fppt after setting tdp and before the fan > curve and after sleep maybe depending on device, after reboot maybe > after keybind (Legion L + Y) as well. Which is not what's expected by > the userspace programs consuming this interface. In addition, this > would expose them to perusing users where they might be confused. I > also know that its difficult by looking at a patch series to > understand the nature of these values. However, given my previous > email, you now have the full context you need to make a decision. > If you think it is appropriate, it is fine by me. > > I'd personally stick them next to platform_profile with a /name > discoverability mechanism similar to hwmon, where tuning > software can find them (something similar to Mario's RFC > that I linked above). Other settings such as the bios light that > interface is perfectly good for. > > As for the hardware limits. You are absolutely right, the ACPI eforces > none, incl. for Lenovo. And the quality is as you expect. For the > Legion Go, they are quite creative. They added a battery 80% > capacity limit by re-using the key value for booting from AC [1-2]. > They also used a weird ABI for the lighting interface to turn off > the suspend light for a good half of the BIOSes, then they fixed it > when they allowed to turn off the suspend light during sleep as well, > which caused that option to break in Legion Space for I want to say > two months. Nevertheless, nobody has broken a Legion Go yet > messing with those settings by e.g., overclocking. It also brings > into view that while the Legion Go uses a derived Legion bios it > has started diverging a bit as it has its own vendor software. > > So I would say that it is good that the other function has a discovery > mechanism and that gamezone has some bitmasks for that purpose as > well. It means that if we tap on them during probe, at least for > Legion laptops from the last 3 years, we can get pretty good support > from the get go. Before that, it is a mix of EC + WMI (see [3]). > > In regards to firmware limits, it is something I would not include in > the first patch series as it will just make this harder to merge, esp. > if there are laptops with wrong limits. Then there are issues with > overrides etc. I would advertise the limits through _min, _max so we > can figure this out later and I would not do a runtime WMI check, as > we have to run the check during probe anyway to populate sysfs, where > it is natural to cache the limits. > > FInally, if indeed the gamezone function is Legion specific, and the > key-value pairs of the Other function are legion specific, from a > stylistic perspective I would tend towards making the ABI of the > driver Legion specific and abstract away its WMI details. E.g., I'd > use the name legion-wmi for a combined driver instead of > lenovo-gamezone-wmi which would then not be useful if lenovo moves > past gamezone. And I'd make sure it only loads on legion laptops. I'm > not up to date on my WMI driver conventions, so this is just a > suggestion. > > Best, > Antheas > > [1] https://github.com/BartoszCichecki/LenovoLegionToolkit/blob/21c0e8ca8b98181a2dedbec1e436d695932a4b0f/LenovoLegionToolkit.Lib/Enums.cs#L72 > [2] https://github.com/hhd-dev/adjustor/blob/188ef6c3e4d7020f2110dd29df6d78847026d41e/src/adjustor/core/lenovo.py#L241 > [3] https://github.com/johnfanv2/LenovoLegionLinux
