On 11/12/2024 10:43, Werner Sembach wrote:
Am 12.11.24 um 17:31 schrieb Armin Wolf:
Am 12.11.24 um 16:10 schrieb Werner Sembach:
Am 12.11.24 um 13:51 schrieb Armin Wolf:
Am 12.11.24 um 13:42 schrieb Werner Sembach:
Hi,
Am 12.11.24 um 13:01 schrieb Armin Wolf:
Am 12.11.24 um 12:52 schrieb Werner Sembach:
Hi,
quick learning question: Why is wmi_bus_type not exported unlike,
for
example, acpi_bus_type, and platform_bus_type?
Wanted to use bus_find_device_by_name in an acpi driver that might
need additional infos from a wmi interface that might or might
not be
present.
Kind regards,
Werner Sembach
What kind of information do you have in mind? wmi_bus_type is not
being exported for historic reasons, i can change that if necessary.
It's for the tuxedo-drivers part for the Sirius 16 Gen 1 & 2 which has
a slow wmi and a quick acpi interface, however the quick acpi
interface can not get the max rpm of the cooling fans, but the wmi
interface can.
Thing is for the acpi driver we might plan an earlier upstream date
and it might get multi-odm support, while the wmi interface is and
stays odm specific. So my idea was to only couple both drivers in a
dynamic way using bus_find_device_by_name.
Interesting, how is the ACPI interface not ODM specific? Can you
elaborate a bit on how the ACPI and the WMI interfaces work?
We have an ODM that was willing to include ACPI code by us in their
BIOS blob and we hope that in the future we can carry that API over to
other ODMs for future TUXEDO devices.
In pseudocode that API looks like this:
v1:
void SMOD(bool mode): Toggle firmware controlled fans vs manually (aka
via the commands below) controlled fans
bool GMOD(): Get current SMOD setting
int GCNT(): Get number of fans
enum GTYP(int index): Returns "CPU-fan" or "GPU-fan"
void SSPD(int index, int value): Set fan speed target as a fraction of
max speed
int GSPD(int index): Get current fan speed target as a fraction of max
speed
v2 same as v1 but with added:
int GRPM(int index): Get current actual fan speed in revolutions per
minute
int GTMP(int index): Get temperature of thing fan with respective
index is pointed at (CPU or GPU die, see GTYP)
Like I said, what is missing is a "Get Max RPM" function even in v2,
which we might add a future iteration, but, well this bios is now out
in the wild. However these released devices have a "get info" function
in the wmi code which returns the v2 infos and the max rpm.
I want to write the code in a way that it probes the acpi interface
for function existence and wherever something is missing tries to fall
back to infos gathered from the wmi interface, but that one is
implemented in a stand alone module (the tuxedo_nb04_* stuff in
tuxedo-drivers) and I would like to keep it that way in honor of KISS.
My plan is that the first time max rpm is pulled the acpi driver uses
bus_find_device_* to get the wmi device, if present, and pulls max rpm
from the driver data there and copies it over to it's own driver data.
If not possible it returns a dummy value or falls back to another
method. Maybe a hard coded list of max rpm values, currently only 2
devices have the new interface, so it wouldn't be a long list.
Directly going to the hard coded list is our current fallback plan,
but it is not an elegant solution as the info is actually there, if
you know what i mean?
Kind regards,
Werner
I see, we once had a similar case with the dell-wmi driver, see commit
f97e058cfe80 ("platform/x86: wmi: Don't allow drivers to get each
other's GUIDs"):
The only driver using this was dell-wmi, and it really was a hack.
The driver was getting a data attribute from another driver
and this
type of action should not be encouraged.
Rather drivers that need to interact with one another should pass
data back and forth via exported functions.
I would be quite unhappy with drivers interacting with WMI devices
without a proper WMI driver, but i can see your point here.
Maybe we can keep the retrieval of the fanX_max values out of the
kernel? I propose the following:
- have a driver for your generic ACPI interface
- have a driver for the WMI interface (with fanX_max hwmon attributes)
The driver for the generic ACPI interface exposes the fan speed
controls as pwmX attributes if the interface does not support
the "Get Max RPM" function. The userspace application in this case
searches for the hwmon chip exposed by the WMI driver and
reads the fanX_max attributes there. Then the application can convert
the target fan speed into values for the pwmX attributes
itself.
If the ACPI interface however supports the "Get Max RPM" function,
then it exposes fanX_max and fanX_target hwmon attributes
themself and the userspace application uses them directly.
This would keep the kernel drivers simple.
I suppose you provide some sort of OEM application for fan control?
Yes we do.
It would be a little bit harder for 3rd parties wanting to implement the
hwmon interface then and i'm also thinking of implementing it in the
thermal subsystem (albeit i did not do a proper readup on that system
yet so maybe it wont work).
In this case having the fanX_max handling here also gives
you full control and allows you to stay independent from the kernel
development in that regard. For example you might decide
independently to hardcode the max. RPM values on some machines (like
the two new machines) or use a different hwmon chip on
some other machine.
Aside from that, i really like your approach with having a generic
ACPI interface, it can potentially save you a lot of work
once you have the initial driver.
Knock on wood that the generic acpi interface works out ^^.
If you're comfortable sharing - is the "ACPI interface" just a wrapper
for an EC interface? Is that EC interface stable? As you're developing
directly for Linux, there is also the "possibility" to just interact
with the EC directly.
That being said if there is an ACPI abstraction already it is nice that
you can tell about it's presence using the ACPI ID reserved for this
device interface.
