Message-ID: 
 <CAGwozwF2EZpChf+eCn5pWy5_ctJ_qUgTrARgiBaW2hWk1CgX4A@mail.gmail.com>
Date: Mon, 30 Dec 2024 14:41:02 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Armin Wolf <W_Armin@....de>
Cc: Derek John Clark <derekjohn.clark@...il.com>, corbet@....net,
 hdegoede@...hat.com,
	ilpo.jarvinen@...ux.intel.com, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, luke@...nes.dev, mpearson-lenovo@...ebb.ca,
	nijs1@...ovo.com, pgriffais@...vesoftware.com,
	platform-driver-x86@...r.kernel.org, shaohz1@...ovo.com, superm1@...nel.org,
	zhangzx36@...ovo.com, johnfanv2@...il.com, codyit@...il.com
Subject: Re: [PATCH 0/1] platform/x86: Add Lenovo Legion WMI Drivers

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@amd.com/

On Sun, 29 Dec 2024 at 23:41, Antheas Kapenekakis <lkml@...heas.dev> 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

Powered by blists - more mailing lists