Message-ID: <4a5e6ff0-7783-46f0-b0af-78f85d555ac8@tuxedocomputers.com>
Date: Fri, 7 Feb 2025 19:44:41 +0100
From: Werner Sembach <wse@...edocomputers.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>, ukleinek@...nel.org,
 jdelvare@...e.com, linux@...ck-us.net, LKML <linux-kernel@...r.kernel.org>,
 platform-driver-x86@...r.kernel.org, linux-pwm@...r.kernel.org,
 linux-hwmon@...r.kernel.org
Subject: Re: [RFC PATCH 1/1] platform/x86/tuxedo: Implement TUXEDO TUXI ACPI
 TFAN via hwmon


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@...edocomputers.com>
>
>>>> +	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@...edocomputers.com
>>>> + */
>>>> +
>>>> +#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).
>

Powered by blists - more mailing lists