Re: [RFC PATCH 1/1] platform/x86/tuxedo: Implement TUXEDO TUXI ACPI TFAN via hwmon

From: Werner Sembach
Date: Fri Feb 07 2025 - 13:48:34 EST


Am 07.02.25 um 12:53 schrieb Ilpo Järvinen:
On Thu, 6 Feb 2025, Werner Sembach wrote:
Am 06.02.25 um 10:51 schrieb Ilpo Järvinen:
On Wed, 5 Feb 2025, Werner Sembach wrote:

The TUXEDO Sirius 16 Gen1 & Gen2 have the custom TUXEDO Interface (TUXI)
ACPI interface which currently consists of the TFAN device. This has ACPI
functions to control the built in fans and monitor fan speeds and CPU and
GPU temprature.

This driver implements this TFAN device via the hwmon subsystem with an
added temprature check that ensure a minimum fanspeed at certain
tempratures. This allows userspace controlled, but hardware safe, custom
temperatures
thx for spotting
fan curves.

Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>

+ for (i = 0; i < driver_data->fan_count; ++i) {
+ params[0] = i;
+ tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
+ params, 1, &retval);
+ temp = retval * 100 - 272000;
+
+ for (j = 0; temp_levels[j].temp; ++j) {
+ temp_low = j == 0 ? -272000 : temp_levels[j-1].temp;
Please add a define for 272000 magic, or do you actually want to use one
of the _kelvin conversion functions in linux/units.h ?
I just realized that it should be 273000.

Using the conversion functions would make it more complicated because the ec
pretends to return to a 10th degree precision but actually only return to a
full degree precission.

So i would need to cut of the last digit, convert and then readd it. When i do
it directly in the code i can just use 273000 instead of 273150 and just
ignore the last digits.
Fine, but add a local define for it then with a comment about the
precision compared with the generic define/conversion functions.
ack

Missing spaces around - operator.

+ temp_high = temp_levels[j].temp;
+ if (driver_data->temp_level[i] > j)
+ temp_high -= 2000; // hysteresis
2 * MILLIDEGREE_PER_DEGREE ?

Use define for it so you can place HYSTERESIS into its name and forgo the
comment.
kk
+
+ if (temp >= temp_low && temp < temp_high)
+ driver_data->temp_level[i] = j;
+ }
+ if (temp >= temp_high)
+ driver_data->temp_level[i] = j;
This loop should be in a helper I think. Naming it reasonably would also
make it easier to understand what the loop does.
only place i use it, i could just add a comment, but i can also do it in a
separate function.
I know it's the only user but what the loop does is relatively complex,
and requires a few variables, etc. Is relatively self-contained
algorithmically.

Splitting into two functions, both functions could be more focused and
clear on their intent. Cleverly naming the helper function such that it
explain what happens in it, can often help to avoid the need to add any
comments (comments may be needed at times, but when we can avoid one
there's one place less to get out-of-sync with the code, which tends to
happen with comments :-)).
ack

But I see Guenther was against some parts of this so please don't take my
style related comments as overruling his objections.
yes v2 will come once i know what i should implement


diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
new file mode 100644
index 0000000000000..292b739a161e7
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024-2025 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
+ */
+
+#include <linux/acpi.h>
Btw just noticed, #include at least for kcalloc/kfree is missing.
ack

+
+#include "acpi_tuxi_init.h"
+
Remove empty line but see first what I note below.
kk
+#include "acpi_tuxi_util.h"
+
+static int __acpi_eval_intarray_in_int_out(acpi_handle handle,
+ acpi_string pathname,
+ unsigned long long *params,
+ u32 pcount,
+ unsigned long long *retval)
There's only single caller of this function, so I question the need for
using an utility function.
It's in preparation for if the TUXI device get another subdevice besides TFAN.

Currently nothing is planed but i though this doesn't hurt.

+{
+ struct acpi_object_list arguments;
+ unsigned long long data;
+ acpi_status status;
+
+ if (pcount > 0) {
+ pr_debug("Params:\n");
+
+ arguments.count = pcount;
+ arguments.pointer = kcalloc(pcount,
sizeof(*arguments.pointer),
+ GFP_KERNEL);
+ for (int i = 0; i < pcount; ++i) {
unsigned int
kk
+ pr_debug("%llu\n", params[i]);
+
+ arguments.pointer[i].type = ACPI_TYPE_INTEGER;
+ arguments.pointer[i].integer.value = params[i];
+ }
+ status = acpi_evaluate_integer(handle, pathname, &arguments,
+ &data);
+ kfree(arguments.pointer);
You can use cleanup.h to handle freeing.
will look into it

+ } else {
+ status = acpi_evaluate_integer(handle, pathname, NULL, &data);
This call should be on the main level. You can use ?: operator for the
only parameter you're changing for it between the currently diverging
code paths.
then the kcalloc call happens every time even if it is not required.
No it won't, you'd allocate only if pcount > 0 (in a similar block as
now):

#include <linux/cleanup.h>
#include <linux/slab.h>
...

union acpi_object __free(kfree) *obj = NULL;

if (pcount > 0)
obj = kcalloc(...);

arguments.count = ...;
arguments.pointer = obj;
...
}

status = acpi_evaluate_integer(handle, pathname,
pcount ? arguments : NULL, &data);
if (ACPI_FAILURE(status))
...

__free() will handle kfree(obj) for you, you don't call kfree() manually.

also i don't know if ?-operator in a function call is good to read.
It is much better than duplicating almost the same call, by using ?: it
is obvious that only single parameter is being altered, whereas on split
calls, the code reader has to do the compare.
ok


+ * Arg0: Fan index
+ * Returns: Speed sensor value in revolutions per minute
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_RPM "GRPM"
+
+int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
+ unsigned long long *params, u32 pcount,
+ unsigned long long *retval);
+
+#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__

What is the reason for splitting this into so many files? Are there going
to be other users of the code that is split into separate files? For the
init/deinit code, surely not.

It will be considerably harder to track call chains, etc. when the
function cannot be found in the same file so you better provide a really
good reason for going so extreme with the split.
Same as above: in preparation for the future if there is another TUXI
subdevice other then TFAN.

Also to section of the hwmon logic as I might want to reuse it for other odms
in the future albeit it would then need to get passed the acpi-write function
in a dynamic way.

And imho it not harder to follow over different files, there is a lot of
external function references anyway, so having something setup to
automatically jump to a function definition in a different file is already
required to quickly parse the code.
For library type APIs, one usually doesn't read those functions. I'm
talking about functions within the driver. For well-structured and
well-named code, jumping all over the place not a requirement at all
because the interfaces that cross file boundaries are well architected and
rest is self-contained and self-explanatory. I see you started to defend
the suboptimal split with everybody does that argument ;-).

It is sectioned off in a logical way.

The _util file contains a helper function that you don't need to know the implementation details for the logic of the driver itself and the _init file just has boilerplate code except the singular acpi_get_handle line.


Your references to "future" sound quite vague, if there are no immediate
plans for such drivers to exist, I'd just do such rearranging of code when
the supposed other drivers actually happens (which often is never).