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

Commit Message

Werner Sembach Feb. 5, 2025, 4:19 p.m. UTC

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
fan curves.

Signed-off-by: Werner Sembach <[e-mail]>
---
 MAINTAINERS                                   |   6 +
 drivers/platform/x86/Kconfig                  |   2 +
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/tuxedo/Kbuild            |   6 +
 drivers/platform/x86/tuxedo/Kconfig           |   6 +
 drivers/platform/x86/tuxedo/nbxx/Kbuild       |   9 +
 drivers/platform/x86/tuxedo/nbxx/Kconfig      |  13 +
 .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.c         | 421 ++++++++++++++++++
 .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.h         |  14 +
 .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.c |  60 +++
 .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.h |  16 +
 .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.c |  58 +++
 .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.h |  84 ++++
 13 files changed, 698 insertions(+)
 create mode 100644 drivers/platform/x86/tuxedo/Kbuild
 create mode 100644 drivers/platform/x86/tuxedo/Kconfig
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
 create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h

Comments

Guenter Roeck Feb. 5, 2025, 6:01 p.m. UTC | #1

On 2/5/25 08:19, 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
> fan curves.
> 
> Signed-off-by: Werner Sembach <[e-mail]>
> ---
>   MAINTAINERS                                   |   6 +
>   drivers/platform/x86/Kconfig                  |   2 +
>   drivers/platform/x86/Makefile                 |   3 +
>   drivers/platform/x86/tuxedo/Kbuild            |   6 +
>   drivers/platform/x86/tuxedo/Kconfig           |   6 +
>   drivers/platform/x86/tuxedo/nbxx/Kbuild       |   9 +
>   drivers/platform/x86/tuxedo/nbxx/Kconfig      |  13 +
>   .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.c         | 421 ++++++++++++++++++
>   .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.h         |  14 +
>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.c |  60 +++
>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.h |  16 +
>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.c |  58 +++
>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.h |  84 ++++
>   13 files changed, 698 insertions(+)
>   create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>   create mode 100644 drivers/platform/x86/tuxedo/Kconfig
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0fa7c5728f1e6..2d3fe9de4e9cf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23916,6 +23916,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
>   F:	tools/power/x86/turbostat/
>   F:	tools/testing/selftests/turbostat/
>   
> +TUXEDO DRIVERS
> +M:	Werner Sembach <[e-mail]>
> +L:	[e-mail]
> +S:	Supported
> +F:	drivers/platform/x86/tuxedo/
> +
>   TW5864 VIDEO4LINUX DRIVER
>   M:	Bluecherry Maintainers <[e-mail]>
>   M:	Andrey Utkin <[e-mail]>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 0258dd879d64b..9b78a1255c08e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1199,3 +1199,5 @@ config P2SB
>   	  The main purpose of this library is to unhide P2SB device in case
>   	  firmware kept it hidden on some platforms in order to access devices
>   	  behind it.
> +
> +source "drivers/platform/x86/tuxedo/Kconfig"
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index e1b1429470674..1562dcd7ad9a5 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
>   
>   # SEL
>   obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
> +
> +# TUXEDO
> +obj-y					+= tuxedo/
> diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
> new file mode 100644
> index 0000000000000..0de6c7871db95
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/Kbuild
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +obj-y	+= nbxx/
> diff --git a/drivers/platform/x86/tuxedo/Kconfig b/drivers/platform/x86/tuxedo/Kconfig
> new file mode 100644
> index 0000000000000..82df7419cf29d
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +source "drivers/platform/x86/tuxedo/nb04/Kconfig"
> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
> new file mode 100644
> index 0000000000000..db8def8ffe8f6
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +tuxedo_nbxx_acpi_tuxi-y			:= acpi_tuxi_init.o
> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_util.o
> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_hwmon.o
> +obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)	+= tuxedo_nbxx_acpi_tuxi.o
> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
> new file mode 100644
> index 0000000000000..827c65c410fb2
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +config TUXEDO_NBXX_ACPI_TUXI
> +	tristate "TUXEDO NBxx ACPI TUXI Platform Driver"
> +	help
> +	  This driver implements the ACPI TUXI device found on some TUXEDO
> +	  Notebooks. Currently this consists only of the TFAN subdevice which is
> +	  implemented via hwmon.
> +
> +	  When compiled as a module it will be called tuxedo_nbxx_acpi_tuxi
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
> new file mode 100644
> index 0000000000000..8ebb2bfc19e20
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
> @@ -0,0 +1,421 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/hwmon.h>
> +#include <linux/workqueue.h>
> +
> +#include "acpi_tuxi_util.h"
> +
> +#include "acpi_tuxi_hwmon.h"
> +
> +struct tuxi_hwmon_driver_data_t {
> +	struct delayed_work work;
> +	struct device *hwmdev;
> +	u8 fan_count;
> +	const char **fan_types;
> +	u8 *temp_level;
> +	u8 *curr_speed;
> +	u8 *want_speed;
> +	u8 pwm_enabled;

Pretty much all those variables are not range checked when assigned.
I commented on one of them below, but it pretty much applies everywhere.
Overflows will have some interesting results.

> +};
> +
> +struct tuxi_temp_high_config_t {
> +	long temp;
> +	u8 min_speed;
> +};
> +
> +#define TUXI_SAFEGUARD_PERIOD 1000
> +
> +#define TUXI_PWM_FAN_ON_MIN_SPEED 0x40 // ~25%
> +
> +static struct tuxi_temp_high_config_t temp_levels[] = {
> +	{  80000, 0x4d }, // ~30%
> +	{  90000, 0x66 }, // ~40%
> +	{  95000, 0x99 }, // ~60%
> +	{ 100000, 0xff }, // 100%
> +	{ }
> +};
> +
> +static umode_t hwm_is_visible(const void * __always_unused data,
> +			      enum hwmon_sensor_types type,
> +			      u32 __always_unused attr,
> +			      int __always_unused channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return 0444;
> +	case hwmon_pwm:
> +		return 0644;
> +	case hwmon_temp:
> +		return 0444;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +		    int channel, long *val)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	int ret;
> +
> +	unsigned long long params[2];
> +	unsigned long long retval;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		params[0] = channel;
> +		ret = tuxi_tfan_method(pdev,
> +				       TUXI_TFAN_METHOD_GET_FAN_RPM,
> +				       params, 1, &retval);
> +		*val = retval;
> +		return ret;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			if (driver_data->pwm_enabled) {
> +				*val = driver_data->curr_speed[channel];
> +				return 0;
> +			}
> +			params[0] = channel;
> +			ret = tuxi_tfan_method(pdev,
> +					       TUXI_TFAN_METHOD_GET_FAN_SPEED,
> +					       params, 1, &retval);
> +			*val = retval;
> +			return ret;
> +		case hwmon_pwm_enable:
> +			*val = driver_data->pwm_enabled;
> +			return ret;
> +		}
> +		break;
> +	case hwmon_temp:
> +		params[0] = channel;
> +		ret = tuxi_tfan_method(pdev,
> +				       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
> +				       params, 1, &retval);
> +		*val = retval * 100 - 272000;
> +		return ret;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int hwm_read_string(struct device *dev,
> +			   enum hwmon_sensor_types __always_unused type,
> +			   u32 __always_unused attr, int channel,
> +			   const char **str)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +
> +	*str = driver_data->fan_types[channel];
> +
> +	return 0;
> +}
> +
> +static int write_speed(struct device *dev, int channel, long val)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	int ret;
> +
> +	unsigned long long params[2];
> +
> +	params[0] = channel;
> +
> +	u8 temp_level, min_speed;
> +
> +	temp_level = driver_data->temp_level[channel];
> +	if (temp_level) {
> +		min_speed = temp_levels[temp_level].min_speed;
> +		if (val < min_speed)
> +			val = min_speed;
> +	}
> +
> +	if (val < TUXI_PWM_FAN_ON_MIN_SPEED / 2)
> +		params[1] = 0;
> +	else if (val < TUXI_PWM_FAN_ON_MIN_SPEED)
> +		params[1] = TUXI_PWM_FAN_ON_MIN_SPEED;
> +	else
> +		params[1] = val;
> +
> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_SPEED,
> +			       params, 2, NULL);
> +	if (ret)
> +		return ret;
> +
> +	driver_data->curr_speed[channel] = val;
> +
> +	return 0;
> +}
> +
> +static int hwm_write(struct device *dev,
> +		     enum hwmon_sensor_types __always_unused type, u32 attr,
> +		     int channel, long val)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	unsigned int i;
> +	int ret;
> +
> +	unsigned long long params[2];
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (driver_data->pwm_enabled) {
> +			driver_data->want_speed[channel] = val;
> +			return write_speed(dev, channel, val);
> +		}
> +
> +		return 0;
> +	case hwmon_pwm_enable:
> +		params[0] = val;
> +		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE,
> +				       params, 1, NULL);
> +		if (ret)
> +			return ret;
> +
> +		driver_data->pwm_enabled = val;

val is not range checked. Since pwm_enabled is declared as u8,
writing a multiple of 256 will cause it to be 0. This may result
in unexpected behavior.

> +
> +		/* Activating PWM sets speed to 0. Alternativ design decision
> +		 * could be to keep the current value. This would require proper
> +		 * setting of driver_data->curr_speed for example.
> +		 */
> +		if (val)
> +			for (i = 0; i < driver_data->fan_count; ++i) {
> +				ret = write_speed(dev, i, 0);
> +				if (ret)
> +					return ret;
> +			}
> +
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops hwmops = {
> +	.is_visible = hwm_is_visible,
> +	.read = hwm_read,
> +	.read_string = hwm_read_string,
> +	.write = hwm_write,
> +};
> +
> +static struct hwmon_channel_info hwmcinfo_fan = {
> +	.type = hwmon_fan,
> +};
> +
> +static struct hwmon_channel_info hwmcinfo_pwm = {
> +	.type = hwmon_pwm,
> +};
> +
> +static struct hwmon_channel_info hwmcinfo_temp = {
> +	.type = hwmon_temp,
> +};
> +
> +static const struct hwmon_channel_info * const hwmcinfo[] = {
> +	&hwmcinfo_fan,
> +	&hwmcinfo_pwm,
> +	&hwmcinfo_temp,
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info hwminfo = {
> +	.ops = &hwmops,
> +	.info = hwmcinfo
> +};
> +
> +static void tuxi_periodic_hw_safeguard(struct work_struct *work)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data;
> +
> +	driver_data = container_of(work, struct tuxi_hwmon_driver_data_t,
> +				   work.work);
> +
> +	struct device *dev = driver_data->hwmdev;
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	unsigned int i, j;
> +
> +	unsigned long long params[2];
> +	unsigned long long retval;
> +
> +	long temp, temp_low, temp_high;
> +	u8 temp_level, min_speed, curr_speed, want_speed;
> +
> +	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);

The return value from tuxi_tfan_method() is checked elsewhere. If there is
an error, retval may contain a random value. The resulting behavior may be
unexpected.

> +		temp = retval * 100 - 272000;
> +
> +		for (j = 0; temp_levels[j].temp; ++j) {
> +			temp_low = j == 0 ? -272000 : temp_levels[j-1].temp;
> +			temp_high = temp_levels[j].temp;
> +			if (driver_data->temp_level[i] > j)
> +				temp_high -= 2000; // hysteresis
> +
> +			if (temp >= temp_low && temp < temp_high)
> +				driver_data->temp_level[i] = j;
> +		}
> +		if (temp >= temp_high)
> +			driver_data->temp_level[i] = j;
> +
> +		temp_level = driver_data->temp_level[i];
> +		min_speed = temp_level == 0 ?
> +			0 : temp_levels[temp_level-1].min_speed;
> +		curr_speed = driver_data->curr_speed[i];
> +		want_speed = driver_data->want_speed[i];
> +
> +		if (want_speed < min_speed) {
> +			if (curr_speed < min_speed)
> +				write_speed(dev, i, min_speed);
> +		} else if (curr_speed != want_speed)
> +			write_speed(dev, i, want_speed);
> +	}
> +
> +	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
> +}

This is not expected functionality of a hardware monitoring driver.
Hardware monmitoring drivers should not replicate userspace or
thermal subsystem functionality.

This would be unacceptable in drivers/hwmon/.

> +
> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data;
> +	int ret;
> +
> +	unsigned long long params[2];
> +	unsigned long long retval;
> +
> +	u32 *hwmcinfo_fan_config, *hwmcinfo_pwm_config, *hwmcinfo_temp_config;
> +
> +	/* The first version of TUXI TFAN didn't have the Get Fan Temperature
> +	 * method which is integral to this driver. So probe for existence and
> +	 * abort otherwise.
> +	 *
> +	 * The Get Fan Speed method is also missing in that version, but was
> +	 * added in the same version so it doesn't have to be probe separately.
> +	 */
> +	params[0] = 0;
> +	ret = tuxi_tfan_method(pdev,
> +			       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
> +			       params, 1, &retval);
> +	if (ret)
> +		return ret;
> +
> +	driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);
> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	/* Loading this module sets the fan mode to auto. Alternative design
> +	 * decision could be to keep the current value. This would require
> +	 * proper initialization of driver_data->curr_speed for example.
> +	 */
> +	params[0] = 0;
> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1,
> +			       NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL, 0,
> +			       &retval);
> +	if (ret)
> +		return ret;
> +	driver_data->fan_count = retval;
> +
> +	driver_data->temp_level = devm_kcalloc(&pdev->dev,
> +					       driver_data->fan_count,
> +					       sizeof(*driver_data->temp_level),
> +					       GFP_KERNEL);
> +	if (driver_data->temp_level == NULL)
> +		return -ENOMEM;
> +	driver_data->curr_speed = devm_kcalloc(&pdev->dev,
> +					       driver_data->fan_count,
> +					       sizeof(*driver_data->curr_speed),
> +					       GFP_KERNEL);
> +	if (driver_data->curr_speed == NULL)
> +		return -ENOMEM;
> +	driver_data->want_speed = devm_kcalloc(&pdev->dev,
> +					       driver_data->fan_count,
> +					       sizeof(*driver_data->want_speed),
> +					       GFP_KERNEL);
> +	if (driver_data->want_speed == NULL)
> +		return -ENOMEM;
> +	driver_data->fan_types = devm_kcalloc(&pdev->dev,
> +					      driver_data->fan_count,
> +					      sizeof(*driver_data->fan_types),
> +					      GFP_KERNEL);
> +	if (driver_data->fan_types == NULL)
> +		return -ENOMEM;
> +
> +	hwmcinfo_fan_config = devm_kcalloc(&pdev->dev,
> +					   driver_data->fan_count + 1,
> +					   sizeof(*hwmcinfo_fan_config),
> +					   GFP_KERNEL);
> +	if (hwmcinfo_fan_config == NULL)
> +		return -ENOMEM;
> +	hwmcinfo_pwm_config = devm_kcalloc(&pdev->dev,
> +					   driver_data->fan_count + 1,
> +					   sizeof(*hwmcinfo_pwm_config),
> +					   GFP_KERNEL);
> +	if (hwmcinfo_pwm_config == NULL)
> +		return -ENOMEM;
> +	hwmcinfo_temp_config = devm_kcalloc(&pdev->dev,
> +					    driver_data->fan_count + 1,
> +					    sizeof(*hwmcinfo_temp_config),
> +					    GFP_KERNEL);
> +	if (hwmcinfo_temp_config == NULL)
> +		return -ENOMEM;
> +
> +	for (unsigned long long i = 0; i < driver_data->fan_count; ++i) {

driver_data->fan_count is declaered as u8. Using an unsigned long long to iterate
over it seems to be a bit unusual.

> +		hwmcinfo_fan_config[i] = HWMON_F_INPUT | HWMON_F_LABEL;
> +		hwmcinfo_pwm_config[i] = HWMON_PWM_INPUT | HWMON_PWM_ENABLE;
> +		hwmcinfo_temp_config[i] = HWMON_T_INPUT | HWMON_T_LABEL;
> +
> +		params[0] = i;
> +		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TYPE,
> +				       params, 1, &retval);
> +		if (ret)
> +			return ret;
> +		else if (retval >= ARRAY_SIZE(tuxi_fan_type_labels))
> +			return -EOPNOTSUPP;
> +		driver_data->fan_types[i] = tuxi_fan_type_labels[retval];
> +	}
> +	hwmcinfo_fan.config = hwmcinfo_fan_config;
> +	hwmcinfo_pwm.config = hwmcinfo_pwm_config;
> +	hwmcinfo_temp.config = hwmcinfo_temp_config;
> +

Personally I think this is way too complicated. It would make much more sense
to assume a reasonable maximum (say, 16) and use fixed size arrays to access
the data. The is_visible function can then simply return 0 for larger channel
values if the total number of fans is less than the ones configured in the
channel information.

Also, as already mentioned, there is no range check of fan_count. This will
cause some oddities if the system ever claims to have 256+ fans.

> +	*hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
> +						       "tuxedo_nbxx_acpi_tuxi",
> +						       driver_data, &hwminfo,
> +						       NULL);
> +	if (PTR_ERR_OR_ZERO(*hwmdev))
> +		return PTR_ERR_OR_ZERO(*hwmdev);
> +
Why not just return hwmdev ?

> +	driver_data->hwmdev = *hwmdev;
> +
> +	INIT_DELAYED_WORK(&driver_data->work, tuxi_periodic_hw_safeguard);
> +	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tuxi_hwmon_add_devices);
> +
> +void tuxi_hwmon_remove_devices(struct device *hwmdev)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(hwmdev);
> +	struct acpi_device *pdev = to_acpi_device(hwmdev->parent);
> +
> +	unsigned long long params[2];
> +
> +	cancel_delayed_work_sync(&driver_data->work);
> +
> +	params[0] = 0;
> +	tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1, NULL);
> +}
> +EXPORT_SYMBOL(tuxi_hwmon_remove_devices);
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
> new file mode 100644
> index 0000000000000..248730fab8574
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
> +
> +#include <linux/acpi.h>
> +
> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev);
> +void tuxi_hwmon_remove_devices(struct device *hwmdev);
> +
> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
> new file mode 100644
> index 0000000000000..5e8edb458aba2
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +
> +#include "acpi_tuxi_hwmon.h"
> +
> +#include "acpi_tuxi_init.h"
> +
> +static const struct acpi_device_id acpi_device_ids[] = {
> +	{"TUXI0000", 0},
> +	{"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
> +
> +static int add(struct acpi_device *device)
> +{
> +	struct tuxi_driver_data_t *driver_data;
> +	acpi_status status;
> +
> +	driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);
> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	// Find subdevices
> +	status = acpi_get_handle(device->handle, "TFAN",
> +				 &driver_data->tfan_handle);
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
> +
> +	dev_set_drvdata(&device->dev, driver_data);
> +
> +	return tuxi_hwmon_add_devices(device, &driver_data->hwmdev);
> +}
> +
> +static void remove(struct acpi_device *device)
> +{
> +	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
> +
> +	tuxi_hwmon_remove_devices(driver_data->hwmdev);
> +}
> +
> +static struct acpi_driver acpi_driver = {
> +	.name = "tuxedo_nbxx_acpi_tuxi",
> +	.ids = acpi_device_ids,
> +	.ops = {
> +		.add = add,
> +		.remove = remove,
> +	},
> +};
> +
> +module_acpi_driver(acpi_driver);
> +
> +MODULE_DESCRIPTION("TUXEDO TUXI");
> +MODULE_AUTHOR("Werner Sembach <[e-mail]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
> new file mode 100644
> index 0000000000000..33388332a2328
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
> +
> +#include <linux/acpi.h>
> +
> +struct tuxi_driver_data_t {
> +	acpi_handle tfan_handle;
> +	struct device *hwmdev;
> +};
> +
> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
> 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 [e-mail]
> + */
> +
> +#include <linux/acpi.h>
> +
> +#include "acpi_tuxi_init.h"
> +
> +#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)
> +{
> +	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) {
> +			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);
> +	} else {
> +		status = acpi_evaluate_integer(handle, pathname, NULL, &data);
> +	}
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
> +
> +	if (retval)
> +		*retval = data;
> +
> +	return 0;
> +}
> +
> +int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
> +		     unsigned long long *params, u32 pcount,
> +		     unsigned long long *retval)
> +{
> +	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
> +
> +	return __acpi_eval_intarray_in_int_out(driver_data->tfan_handle, method,
> +					       params, pcount, retval);
> +}
> +EXPORT_SYMBOL(tuxi_tfan_method);
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
> new file mode 100644
> index 0000000000000..670fe02249d06
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
> +
> +#include <linux/acpi.h>
> +
> +/*
> + * Set fan speed target
> + *
> + * Set a specific fan speed (needs manual mode)
> + *
> + * Arg0: Fan index
> + * Arg1: Fan speed as a fraction of maximum speed (0-255)
> + */
> +#define TUXI_TFAN_METHOD_SET_FAN_SPEED		"SSPD"
> +
> +/*
> + * Get fan speed target
> + *
> + * Arg0: Fan index
> + * Returns: Current fan speed target a fraction of maximum speed (0-255)
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_SPEED		"GSPD"
> +
> +/*
> + * Get fans count
> + *
> + * Returns: Number of individually controllable fans
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_COUNT		"GCNT"
> +
> +/*
> + * Set fans mode
> + *
> + * Arg0: 0 = auto, 1 = manual
> + */
> +#define TUXI_TFAN_METHOD_SET_FAN_MODE		"SMOD"
> +
> +/*
> + * Get fans mode
> + *
> + * Returns: 0 = auto, 1 = manual
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_MODE		"GMOD"
> +
> +/*
> + * Get fan type/what the fan is pointed at
> + *
> + * Arg0: Fan index
> + * Returns: 0 = general, 1 = CPU, 2 = GPU
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_TYPE		"GTYP"
> +
> +static const char * const tuxi_fan_type_labels[] = {
> +	"general",
> +	"cpu",
> +	"gpu"
> +};
> +
> +/*
> + * Get fan temperature/temperature of what the fan is pointed at
> + *
> + * Arg0: Fan index
> + * Returns: Temperature sensor value in 10ths of degrees kelvin
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE	"GTMP"
> +
> +/*
> + * Get actual fan speed in RPM
> + *
> + * 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__

Werner Sembach Feb. 6, 2025, 9:28 a.m. UTC | #2

Hi,

Am 05.02.25 um 19:01 schrieb Guenter Roeck:
> On 2/5/25 08:19, 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
>> fan curves.
>>
>> Signed-off-by: Werner Sembach <[e-mail]>
>> ---
>>   MAINTAINERS                                   |   6 +
>>   drivers/platform/x86/Kconfig                  |   2 +
>>   drivers/platform/x86/Makefile                 |   3 +
>>   drivers/platform/x86/tuxedo/Kbuild            |   6 +
>>   drivers/platform/x86/tuxedo/Kconfig           |   6 +
>>   drivers/platform/x86/tuxedo/nbxx/Kbuild       |   9 +
>>   drivers/platform/x86/tuxedo/nbxx/Kconfig      |  13 +
>>   .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.c         | 421 ++++++++++++++++++
>>   .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.h         |  14 +
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.c |  60 +++
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.h |  16 +
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.c |  58 +++
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.h |  84 ++++
>>   13 files changed, 698 insertions(+)
>>   create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>>   create mode 100644 drivers/platform/x86/tuxedo/Kconfig
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0fa7c5728f1e6..2d3fe9de4e9cf 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23916,6 +23916,12 @@ T:    git 
>> git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
>>   F:    tools/power/x86/turbostat/
>>   F:    tools/testing/selftests/turbostat/
>>   +TUXEDO DRIVERS
>> +M:    Werner Sembach <[e-mail]>
>> +L:    [e-mail]
>> +S:    Supported
>> +F:    drivers/platform/x86/tuxedo/
>> +
>>   TW5864 VIDEO4LINUX DRIVER
>>   M:    Bluecherry Maintainers <[e-mail]>
>>   M:    Andrey Utkin <[e-mail]>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 0258dd879d64b..9b78a1255c08e 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1199,3 +1199,5 @@ config P2SB
>>         The main purpose of this library is to unhide P2SB device in case
>>         firmware kept it hidden on some platforms in order to access devices
>>         behind it.
>> +
>> +source "drivers/platform/x86/tuxedo/Kconfig"
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index e1b1429470674..1562dcd7ad9a5 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)        += 
>> winmate-fm07-keys.o
>>     # SEL
>>   obj-$(CONFIG_SEL3350_PLATFORM)        += sel3350-platform.o
>> +
>> +# TUXEDO
>> +obj-y                    += tuxedo/
>> diff --git a/drivers/platform/x86/tuxedo/Kbuild 
>> b/drivers/platform/x86/tuxedo/Kbuild
>> new file mode 100644
>> index 0000000000000..0de6c7871db95
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/Kbuild
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +obj-y    += nbxx/
>> diff --git a/drivers/platform/x86/tuxedo/Kconfig 
>> b/drivers/platform/x86/tuxedo/Kconfig
>> new file mode 100644
>> index 0000000000000..82df7419cf29d
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/Kconfig
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +source "drivers/platform/x86/tuxedo/nb04/Kconfig"
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild 
>> b/drivers/platform/x86/tuxedo/nbxx/Kbuild
>> new file mode 100644
>> index 0000000000000..db8def8ffe8f6
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +tuxedo_nbxx_acpi_tuxi-y            := acpi_tuxi_init.o
>> +tuxedo_nbxx_acpi_tuxi-y            += acpi_tuxi_util.o
>> +tuxedo_nbxx_acpi_tuxi-y            += acpi_tuxi_hwmon.o
>> +obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)    += tuxedo_nbxx_acpi_tuxi.o
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig 
>> b/drivers/platform/x86/tuxedo/nbxx/Kconfig
>> new file mode 100644
>> index 0000000000000..827c65c410fb2
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
>> @@ -0,0 +1,13 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +config TUXEDO_NBXX_ACPI_TUXI
>> +    tristate "TUXEDO NBxx ACPI TUXI Platform Driver"
>> +    help
>> +      This driver implements the ACPI TUXI device found on some TUXEDO
>> +      Notebooks. Currently this consists only of the TFAN subdevice which is
>> +      implemented via hwmon.
>> +
>> +      When compiled as a module it will be called tuxedo_nbxx_acpi_tuxi
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c 
>> b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>> new file mode 100644
>> index 0000000000000..8ebb2bfc19e20
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>> @@ -0,0 +1,421 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "acpi_tuxi_util.h"
>> +
>> +#include "acpi_tuxi_hwmon.h"
>> +
>> +struct tuxi_hwmon_driver_data_t {
>> +    struct delayed_work work;
>> +    struct device *hwmdev;
>> +    u8 fan_count;
>> +    const char **fan_types;
>> +    u8 *temp_level;
>> +    u8 *curr_speed;
>> +    u8 *want_speed;
>> +    u8 pwm_enabled;
>
> Pretty much all those variables are not range checked when assigned.
> I commented on one of them below, but it pretty much applies everywhere.
> Overflows will have some interesting results.

curr_speed comes from the firmware where I know what the max value is (255).

temp_level comes from the code itself and has a fixed max value (length of the 
hard coded temp_levels array + 1).

fan_count while technically not limited by definition will realworld never be 
anything big (realistically it will go up to about 4).

Would it be enough to document this?

pwm_enabled and want_speed see below.

>
>> +};
>> +
>> +struct tuxi_temp_high_config_t {
>> +    long temp;
>> +    u8 min_speed;
>> +};
>> +
>> +#define TUXI_SAFEGUARD_PERIOD 1000
>> +
>> +#define TUXI_PWM_FAN_ON_MIN_SPEED 0x40 // ~25%
>> +
>> +static struct tuxi_temp_high_config_t temp_levels[] = {
>> +    {  80000, 0x4d }, // ~30%
>> +    {  90000, 0x66 }, // ~40%
>> +    {  95000, 0x99 }, // ~60%
>> +    { 100000, 0xff }, // 100%
>> +    { }
>> +};
>> +
>> +static umode_t hwm_is_visible(const void * __always_unused data,
>> +                  enum hwmon_sensor_types type,
>> +                  u32 __always_unused attr,
>> +                  int __always_unused channel)
>> +{
>> +    switch (type) {
>> +    case hwmon_fan:
>> +        return 0444;
>> +    case hwmon_pwm:
>> +        return 0644;
>> +    case hwmon_temp:
>> +        return 0444;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +            int channel, long *val)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +    struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +    int ret;
>> +
>> +    unsigned long long params[2];
>> +    unsigned long long retval;
>> +
>> +    switch (type) {
>> +    case hwmon_fan:
>> +        params[0] = channel;
>> +        ret = tuxi_tfan_method(pdev,
>> +                       TUXI_TFAN_METHOD_GET_FAN_RPM,
>> +                       params, 1, &retval);
>> +        *val = retval;
>> +        return ret;
>> +    case hwmon_pwm:
>> +        switch (attr) {
>> +        case hwmon_pwm_input:
>> +            if (driver_data->pwm_enabled) {
>> +                *val = driver_data->curr_speed[channel];
>> +                return 0;
>> +            }
>> +            params[0] = channel;
>> +            ret = tuxi_tfan_method(pdev,
>> +                           TUXI_TFAN_METHOD_GET_FAN_SPEED,
>> +                           params, 1, &retval);
>> +            *val = retval;
>> +            return ret;
>> +        case hwmon_pwm_enable:
>> +            *val = driver_data->pwm_enabled;
>> +            return ret;
>> +        }
>> +        break;
>> +    case hwmon_temp:
>> +        params[0] = channel;
>> +        ret = tuxi_tfan_method(pdev,
>> +                       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>> +                       params, 1, &retval);
>> +        *val = retval * 100 - 272000;
>> +        return ret;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static int hwm_read_string(struct device *dev,
>> +               enum hwmon_sensor_types __always_unused type,
>> +               u32 __always_unused attr, int channel,
>> +               const char **str)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +
>> +    *str = driver_data->fan_types[channel];
>> +
>> +    return 0;
>> +}
>> +
>> +static int write_speed(struct device *dev, int channel, long val)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +    struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +    int ret;
>> +
>> +    unsigned long long params[2];
>> +
>> +    params[0] = channel;
>> +
>> +    u8 temp_level, min_speed;
>> +
>> +    temp_level = driver_data->temp_level[channel];
>> +    if (temp_level) {
>> +        min_speed = temp_levels[temp_level].min_speed;
>> +        if (val < min_speed)
>> +            val = min_speed;
>> +    }
>> +
>> +    if (val < TUXI_PWM_FAN_ON_MIN_SPEED / 2)
>> +        params[1] = 0;
>> +    else if (val < TUXI_PWM_FAN_ON_MIN_SPEED)
>> +        params[1] = TUXI_PWM_FAN_ON_MIN_SPEED;
>> +    else
>> +        params[1] = val;
>> +
>> +    ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_SPEED,
>> +                   params, 2, NULL);
>> +    if (ret)
>> +        return ret;
>> +
>> +    driver_data->curr_speed[channel] = val;
>> +
>> +    return 0;
>> +}
>> +
>> +static int hwm_write(struct device *dev,
>> +             enum hwmon_sensor_types __always_unused type, u32 attr,
>> +             int channel, long val)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +    struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +    unsigned int i;
>> +    int ret;
>> +
>> +    unsigned long long params[2];
>> +
>> +    switch (attr) {
>> +    case hwmon_pwm_input:
>> +        if (driver_data->pwm_enabled) {
>> +            driver_data->want_speed[channel] = val;
>> +            return write_speed(dev, channel, val);
>> +        }
>> +
>> +        return 0;
>> +    case hwmon_pwm_enable:
>> +        params[0] = val;
>> +        ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE,
>> +                       params, 1, NULL);
>> +        if (ret)
>> +            return ret;
>> +
>> +        driver_data->pwm_enabled = val;
>
> val is not range checked. Since pwm_enabled is declared as u8,
> writing a multiple of 256 will cause it to be 0. This may result
> in unexpected behavior.

I somehow assumed that pwm_enabled val can only be 1 or 0, will fix this brain 
fail in the next version.

Same goes for the pwm value which should only between 0 and 255.

>
>> +
>> +        /* Activating PWM sets speed to 0. Alternativ design decision
>> +         * could be to keep the current value. This would require proper
>> +         * setting of driver_data->curr_speed for example.
>> +         */
>> +        if (val)
>> +            for (i = 0; i < driver_data->fan_count; ++i) {
>> +                ret = write_speed(dev, i, 0);
>> +                if (ret)
>> +                    return ret;
>> +            }
>> +
>> +        return 0;
>> +    }
>> +
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops hwmops = {
>> +    .is_visible = hwm_is_visible,
>> +    .read = hwm_read,
>> +    .read_string = hwm_read_string,
>> +    .write = hwm_write,
>> +};
>> +
>> +static struct hwmon_channel_info hwmcinfo_fan = {
>> +    .type = hwmon_fan,
>> +};
>> +
>> +static struct hwmon_channel_info hwmcinfo_pwm = {
>> +    .type = hwmon_pwm,
>> +};
>> +
>> +static struct hwmon_channel_info hwmcinfo_temp = {
>> +    .type = hwmon_temp,
>> +};
>> +
>> +static const struct hwmon_channel_info * const hwmcinfo[] = {
>> +    &hwmcinfo_fan,
>> +    &hwmcinfo_pwm,
>> +    &hwmcinfo_temp,
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_chip_info hwminfo = {
>> +    .ops = &hwmops,
>> +    .info = hwmcinfo
>> +};
>> +
>> +static void tuxi_periodic_hw_safeguard(struct work_struct *work)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data;
>> +
>> +    driver_data = container_of(work, struct tuxi_hwmon_driver_data_t,
>> +                   work.work);
>> +
>> +    struct device *dev = driver_data->hwmdev;
>> +    struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +    unsigned int i, j;
>> +
>> +    unsigned long long params[2];
>> +    unsigned long long retval;
>> +
>> +    long temp, temp_low, temp_high;
>> +    u8 temp_level, min_speed, curr_speed, want_speed;
>> +
>> +    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);
>
> The return value from tuxi_tfan_method() is checked elsewhere. If there is
> an error, retval may contain a random value. The resulting behavior may be
> unexpected.
Thanks for spotting the missing error handling. I will come up with a safe fallback.
>
>> +        temp = retval * 100 - 272000;
>> +
>> +        for (j = 0; temp_levels[j].temp; ++j) {
>> +            temp_low = j == 0 ? -272000 : temp_levels[j-1].temp;
>> +            temp_high = temp_levels[j].temp;
>> +            if (driver_data->temp_level[i] > j)
>> +                temp_high -= 2000; // hysteresis
>> +
>> +            if (temp >= temp_low && temp < temp_high)
>> +                driver_data->temp_level[i] = j;
>> +        }
>> +        if (temp >= temp_high)
>> +            driver_data->temp_level[i] = j;
>> +
>> +        temp_level = driver_data->temp_level[i];
>> +        min_speed = temp_level == 0 ?
>> +            0 : temp_levels[temp_level-1].min_speed;
>> +        curr_speed = driver_data->curr_speed[i];
>> +        want_speed = driver_data->want_speed[i];
>> +
>> +        if (want_speed < min_speed) {
>> +            if (curr_speed < min_speed)
>> +                write_speed(dev, i, min_speed);
>> +        } else if (curr_speed != want_speed)
>> +            write_speed(dev, i, want_speed);
>> +    }
>> +
>> +    schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>> +}
>
> This is not expected functionality of a hardware monitoring driver.
> Hardware monmitoring drivers should not replicate userspace or
> thermal subsystem functionality.
>
> This would be unacceptable in drivers/hwmon/.

Problem is: The thermal subsystem doesn't do this either as far as I can tell.

See this: 
https://lore.kernel.org/all/[e-mail]/ 
and this: 
https://lore.kernel.org/all/[e-mail]/ 
thread.

The short version is: The Thermal subsystem always allows userspace to select 
the "userspace" governor which has no way for the kernel to enforce a minimum speed.

As far as I can tell the Thermal subsystem would require a new governor for the 
behavior i want to archive and more importantly, a way to restrict which 
governors userspace can select.

As to why I don't want grant userspace full control: The firmware is perfectly 
fine with accepting potentially mainboard frying settings (as mentioned in the 
cover letter) and the lowest level I can write code for is the kernel driver. So 
that's the location I need to prevent this.

Also hwmon is not purely a hardware monitoring, it also allows writing 
fanspeeds. Or did I miss something and this shouldn't actually be used?

>
>> +
>> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data;
>> +    int ret;
>> +
>> +    unsigned long long params[2];
>> +    unsigned long long retval;
>> +
>> +    u32 *hwmcinfo_fan_config, *hwmcinfo_pwm_config, *hwmcinfo_temp_config;
>> +
>> +    /* The first version of TUXI TFAN didn't have the Get Fan Temperature
>> +     * method which is integral to this driver. So probe for existence and
>> +     * abort otherwise.
>> +     *
>> +     * The Get Fan Speed method is also missing in that version, but was
>> +     * added in the same version so it doesn't have to be probe separately.
>> +     */
>> +    params[0] = 0;
>> +    ret = tuxi_tfan_method(pdev,
>> +                   TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>> +                   params, 1, &retval);
>> +    if (ret)
>> +        return ret;
>> +
>> +    driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
>> +                   GFP_KERNEL);
>> +    if (!driver_data)
>> +        return -ENOMEM;
>> +
>> +    /* Loading this module sets the fan mode to auto. Alternative design
>> +     * decision could be to keep the current value. This would require
>> +     * proper initialization of driver_data->curr_speed for example.
>> +     */
>> +    params[0] = 0;
>> +    ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1,
>> +                   NULL);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL, 0,
>> +                   &retval);
>> +    if (ret)
>> +        return ret;
>> +    driver_data->fan_count = retval;
>> +
>> +    driver_data->temp_level = devm_kcalloc(&pdev->dev,
>> +                           driver_data->fan_count,
>> +                           sizeof(*driver_data->temp_level),
>> +                           GFP_KERNEL);
>> +    if (driver_data->temp_level == NULL)
>> +        return -ENOMEM;
>> +    driver_data->curr_speed = devm_kcalloc(&pdev->dev,
>> +                           driver_data->fan_count,
>> +                           sizeof(*driver_data->curr_speed),
>> +                           GFP_KERNEL);
>> +    if (driver_data->curr_speed == NULL)
>> +        return -ENOMEM;
>> +    driver_data->want_speed = devm_kcalloc(&pdev->dev,
>> +                           driver_data->fan_count,
>> +                           sizeof(*driver_data->want_speed),
>> +                           GFP_KERNEL);
>> +    if (driver_data->want_speed == NULL)
>> +        return -ENOMEM;
>> +    driver_data->fan_types = devm_kcalloc(&pdev->dev,
>> +                          driver_data->fan_count,
>> +                          sizeof(*driver_data->fan_types),
>> +                          GFP_KERNEL);
>> +    if (driver_data->fan_types == NULL)
>> +        return -ENOMEM;
>> +
>> +    hwmcinfo_fan_config = devm_kcalloc(&pdev->dev,
>> +                       driver_data->fan_count + 1,
>> +                       sizeof(*hwmcinfo_fan_config),
>> +                       GFP_KERNEL);
>> +    if (hwmcinfo_fan_config == NULL)
>> +        return -ENOMEM;
>> +    hwmcinfo_pwm_config = devm_kcalloc(&pdev->dev,
>> +                       driver_data->fan_count + 1,
>> +                       sizeof(*hwmcinfo_pwm_config),
>> +                       GFP_KERNEL);
>> +    if (hwmcinfo_pwm_config == NULL)
>> +        return -ENOMEM;
>> +    hwmcinfo_temp_config = devm_kcalloc(&pdev->dev,
>> +                        driver_data->fan_count + 1,
>> +                        sizeof(*hwmcinfo_temp_config),
>> +                        GFP_KERNEL);
>> +    if (hwmcinfo_temp_config == NULL)
>> +        return -ENOMEM;
>> +
>> +    for (unsigned long long i = 0; i < driver_data->fan_count; ++i) {
>
> driver_data->fan_count is declaered as u8. Using an unsigned long long to iterate
> over it seems to be a bit unusual.
oversight, i initially iterated over the acpi retval directly and was just 
typematching it here, will fix in the next version
>
>> +        hwmcinfo_fan_config[i] = HWMON_F_INPUT | HWMON_F_LABEL;
>> +        hwmcinfo_pwm_config[i] = HWMON_PWM_INPUT | HWMON_PWM_ENABLE;
>> +        hwmcinfo_temp_config[i] = HWMON_T_INPUT | HWMON_T_LABEL;
>> +
>> +        params[0] = i;
>> +        ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TYPE,
>> +                       params, 1, &retval);
>> +        if (ret)
>> +            return ret;
>> +        else if (retval >= ARRAY_SIZE(tuxi_fan_type_labels))
>> +            return -EOPNOTSUPP;
>> +        driver_data->fan_types[i] = tuxi_fan_type_labels[retval];
>> +    }
>> +    hwmcinfo_fan.config = hwmcinfo_fan_config;
>> +    hwmcinfo_pwm.config = hwmcinfo_pwm_config;
>> +    hwmcinfo_temp.config = hwmcinfo_temp_config;
>> +
>
> Personally I think this is way too complicated. It would make much more sense
> to assume a reasonable maximum (say, 16) and use fixed size arrays to access
> the data. The is_visible function can then simply return 0 for larger channel
> values if the total number of fans is less than the ones configured in the
> channel information.
Didn't know it was possible to filter extra entries out completely with the 
is_visible function, thanks for the tip.
>
> Also, as already mentioned, there is no range check of fan_count. This will
> cause some oddities if the system ever claims to have 256+ fans.
Will not happen, but i guess a singular additional if in the init doesn't hurt, 
i can add it.
>
>> +    *hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
>> +                               "tuxedo_nbxx_acpi_tuxi",
>> +                               driver_data, &hwminfo,
>> +                               NULL);
>> +    if (PTR_ERR_OR_ZERO(*hwmdev))
>> +        return PTR_ERR_OR_ZERO(*hwmdev);
>> +
> Why not just return hwmdev ?
because if hwmon is NULL it is still an error, i have to look again at what 
actually is returned by PTR_ERR_OR_ZERO on zero.
>
>> +    driver_data->hwmdev = *hwmdev;
>> +
>> +    INIT_DELAYED_WORK(&driver_data->work, tuxi_periodic_hw_safeguard);
>> +    schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(tuxi_hwmon_add_devices);
>> +
>> +void tuxi_hwmon_remove_devices(struct device *hwmdev)
>> +{
>> +    struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(hwmdev);
>> +    struct acpi_device *pdev = to_acpi_device(hwmdev->parent);
>> +
>> +    unsigned long long params[2];
>> +
>> +    cancel_delayed_work_sync(&driver_data->work);
>> +
>> +    params[0] = 0;
>> +    tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1, NULL);
>> +}
>> +EXPORT_SYMBOL(tuxi_hwmon_remove_devices);
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h 
>> b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>> new file mode 100644
>> index 0000000000000..248730fab8574
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
>> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
>> +
>> +#include <linux/acpi.h>
>> +
>> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev);
>> +void tuxi_hwmon_remove_devices(struct device *hwmdev);
>> +
>> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c 
>> b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>> new file mode 100644
>> index 0000000000000..5e8edb458aba2
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>> @@ -0,0 +1,60 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/module.h>
>> +
>> +#include "acpi_tuxi_hwmon.h"
>> +
>> +#include "acpi_tuxi_init.h"
>> +
>> +static const struct acpi_device_id acpi_device_ids[] = {
>> +    {"TUXI0000", 0},
>> +    {"", 0}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
>> +
>> +static int add(struct acpi_device *device)
>> +{
>> +    struct tuxi_driver_data_t *driver_data;
>> +    acpi_status status;
>> +
>> +    driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
>> +                   GFP_KERNEL);
>> +    if (!driver_data)
>> +        return -ENOMEM;
>> +
>> +    // Find subdevices
>> +    status = acpi_get_handle(device->handle, "TFAN",
>> +                 &driver_data->tfan_handle);
>> +    if (ACPI_FAILURE(status))
>> +        return_ACPI_STATUS(status);
>> +
>> +    dev_set_drvdata(&device->dev, driver_data);
>> +
>> +    return tuxi_hwmon_add_devices(device, &driver_data->hwmdev);
>> +}
>> +
>> +static void remove(struct acpi_device *device)
>> +{
>> +    struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
>> +
>> +    tuxi_hwmon_remove_devices(driver_data->hwmdev);
>> +}
>> +
>> +static struct acpi_driver acpi_driver = {
>> +    .name = "tuxedo_nbxx_acpi_tuxi",
>> +    .ids = acpi_device_ids,
>> +    .ops = {
>> +        .add = add,
>> +        .remove = remove,
>> +    },
>> +};
>> +
>> +module_acpi_driver(acpi_driver);
>> +
>> +MODULE_DESCRIPTION("TUXEDO TUXI");
>> +MODULE_AUTHOR("Werner Sembach <[e-mail]>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h 
>> b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>> new file mode 100644
>> index 0000000000000..33388332a2328
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
>> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
>> +
>> +#include <linux/acpi.h>
>> +
>> +struct tuxi_driver_data_t {
>> +    acpi_handle tfan_handle;
>> +    struct device *hwmdev;
>> +};
>> +
>> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
>> 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 [e-mail]
>> + */
>> +
>> +#include <linux/acpi.h>
>> +
>> +#include "acpi_tuxi_init.h"
>> +
>> +#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)
>> +{
>> +    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) {
>> +            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);
>> +    } else {
>> +        status = acpi_evaluate_integer(handle, pathname, NULL, &data);
>> +    }
>> +    if (ACPI_FAILURE(status))
>> +        return_ACPI_STATUS(status);
>> +
>> +    if (retval)
>> +        *retval = data;
>> +
>> +    return 0;
>> +}
>> +
>> +int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
>> +             unsigned long long *params, u32 pcount,
>> +             unsigned long long *retval)
>> +{
>> +    struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
>> +
>> +    return __acpi_eval_intarray_in_int_out(driver_data->tfan_handle, method,
>> +                           params, pcount, retval);
>> +}
>> +EXPORT_SYMBOL(tuxi_tfan_method);
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h 
>> b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
>> new file mode 100644
>> index 0000000000000..670fe02249d06
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
>> @@ -0,0 +1,84 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
>> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
>> +
>> +#include <linux/acpi.h>
>> +
>> +/*
>> + * Set fan speed target
>> + *
>> + * Set a specific fan speed (needs manual mode)
>> + *
>> + * Arg0: Fan index
>> + * Arg1: Fan speed as a fraction of maximum speed (0-255)
>> + */
>> +#define TUXI_TFAN_METHOD_SET_FAN_SPEED        "SSPD"
>> +
>> +/*
>> + * Get fan speed target
>> + *
>> + * Arg0: Fan index
>> + * Returns: Current fan speed target a fraction of maximum speed (0-255)
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_SPEED        "GSPD"
>> +
>> +/*
>> + * Get fans count
>> + *
>> + * Returns: Number of individually controllable fans
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_COUNT        "GCNT"
>> +
>> +/*
>> + * Set fans mode
>> + *
>> + * Arg0: 0 = auto, 1 = manual
>> + */
>> +#define TUXI_TFAN_METHOD_SET_FAN_MODE        "SMOD"
>> +
>> +/*
>> + * Get fans mode
>> + *
>> + * Returns: 0 = auto, 1 = manual
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_MODE        "GMOD"
>> +
>> +/*
>> + * Get fan type/what the fan is pointed at
>> + *
>> + * Arg0: Fan index
>> + * Returns: 0 = general, 1 = CPU, 2 = GPU
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_TYPE        "GTYP"
>> +
>> +static const char * const tuxi_fan_type_labels[] = {
>> +    "general",
>> +    "cpu",
>> +    "gpu"
>> +};
>> +
>> +/*
>> + * Get fan temperature/temperature of what the fan is pointed at
>> + *
>> + * Arg0: Fan index
>> + * Returns: Temperature sensor value in 10ths of degrees kelvin
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE    "GTMP"
>> +
>> +/*
>> + * Get actual fan speed in RPM
>> + *
>> + * 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__
>
Thanks for the quick feedback so far.

Kind regards,

Werner

Ilpo Järvinen Feb. 6, 2025, 9:51 a.m. UTC | #3

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

> fan curves.
> 
> Signed-off-by: Werner Sembach <[e-mail]>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/platform/x86/Kconfig                  |   2 +
>  drivers/platform/x86/Makefile                 |   3 +
>  drivers/platform/x86/tuxedo/Kbuild            |   6 +
>  drivers/platform/x86/tuxedo/Kconfig           |   6 +
>  drivers/platform/x86/tuxedo/nbxx/Kbuild       |   9 +
>  drivers/platform/x86/tuxedo/nbxx/Kconfig      |  13 +
>  .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.c         | 421 ++++++++++++++++++
>  .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.h         |  14 +
>  .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.c |  60 +++
>  .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.h |  16 +
>  .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.c |  58 +++
>  .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.h |  84 ++++
>  13 files changed, 698 insertions(+)
>  create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>  create mode 100644 drivers/platform/x86/tuxedo/Kconfig
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
>  create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0fa7c5728f1e6..2d3fe9de4e9cf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23916,6 +23916,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
>  F:	tools/power/x86/turbostat/
>  F:	tools/testing/selftests/turbostat/
>  
> +TUXEDO DRIVERS
> +M:	Werner Sembach <[e-mail]>
> +L:	[e-mail]
> +S:	Supported
> +F:	drivers/platform/x86/tuxedo/
> +
>  TW5864 VIDEO4LINUX DRIVER
>  M:	Bluecherry Maintainers <[e-mail]>
>  M:	Andrey Utkin <[e-mail]>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 0258dd879d64b..9b78a1255c08e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1199,3 +1199,5 @@ config P2SB
>  	  The main purpose of this library is to unhide P2SB device in case
>  	  firmware kept it hidden on some platforms in order to access devices
>  	  behind it.
> +
> +source "drivers/platform/x86/tuxedo/Kconfig"
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index e1b1429470674..1562dcd7ad9a5 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
>  
>  # SEL
>  obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
> +
> +# TUXEDO
> +obj-y					+= tuxedo/
> diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
> new file mode 100644
> index 0000000000000..0de6c7871db95
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/Kbuild
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +obj-y	+= nbxx/
> diff --git a/drivers/platform/x86/tuxedo/Kconfig b/drivers/platform/x86/tuxedo/Kconfig
> new file mode 100644
> index 0000000000000..82df7419cf29d
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +source "drivers/platform/x86/tuxedo/nb04/Kconfig"
> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
> new file mode 100644
> index 0000000000000..db8def8ffe8f6
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +tuxedo_nbxx_acpi_tuxi-y			:= acpi_tuxi_init.o
> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_util.o
> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_hwmon.o
> +obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)	+= tuxedo_nbxx_acpi_tuxi.o
> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
> new file mode 100644
> index 0000000000000..827c65c410fb2
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# TUXEDO X86 Platform Specific Drivers
> +#
> +
> +config TUXEDO_NBXX_ACPI_TUXI
> +	tristate "TUXEDO NBxx ACPI TUXI Platform Driver"
> +	help
> +	  This driver implements the ACPI TUXI device found on some TUXEDO
> +	  Notebooks. Currently this consists only of the TFAN subdevice which is
> +	  implemented via hwmon.
> +
> +	  When compiled as a module it will be called tuxedo_nbxx_acpi_tuxi
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
> new file mode 100644
> index 0000000000000..8ebb2bfc19e20
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
> @@ -0,0 +1,421 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/hwmon.h>
> +#include <linux/workqueue.h>
> +
> +#include "acpi_tuxi_util.h"
> +
> +#include "acpi_tuxi_hwmon.h"
> +
> +struct tuxi_hwmon_driver_data_t {
> +	struct delayed_work work;
> +	struct device *hwmdev;
> +	u8 fan_count;
> +	const char **fan_types;
> +	u8 *temp_level;
> +	u8 *curr_speed;
> +	u8 *want_speed;
> +	u8 pwm_enabled;
> +};
> +
> +struct tuxi_temp_high_config_t {
> +	long temp;
> +	u8 min_speed;
> +};
> +
> +#define TUXI_SAFEGUARD_PERIOD 1000
> +
> +#define TUXI_PWM_FAN_ON_MIN_SPEED 0x40 // ~25%
> +
> +static struct tuxi_temp_high_config_t temp_levels[] = {
> +	{  80000, 0x4d }, // ~30%
> +	{  90000, 0x66 }, // ~40%
> +	{  95000, 0x99 }, // ~60%
> +	{ 100000, 0xff }, // 100%
> +	{ }
> +};
> +
> +static umode_t hwm_is_visible(const void * __always_unused data,
> +			      enum hwmon_sensor_types type,
> +			      u32 __always_unused attr,
> +			      int __always_unused channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return 0444;
> +	case hwmon_pwm:
> +		return 0644;
> +	case hwmon_temp:
> +		return 0444;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +		    int channel, long *val)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	int ret;
> +
> +	unsigned long long params[2];
> +	unsigned long long retval;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		params[0] = channel;
> +		ret = tuxi_tfan_method(pdev,
> +				       TUXI_TFAN_METHOD_GET_FAN_RPM,
> +				       params, 1, &retval);
> +		*val = retval;
> +		return ret;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			if (driver_data->pwm_enabled) {
> +				*val = driver_data->curr_speed[channel];
> +				return 0;
> +			}
> +			params[0] = channel;
> +			ret = tuxi_tfan_method(pdev,
> +					       TUXI_TFAN_METHOD_GET_FAN_SPEED,
> +					       params, 1, &retval);
> +			*val = retval;
> +			return ret;
> +		case hwmon_pwm_enable:
> +			*val = driver_data->pwm_enabled;
> +			return ret;
> +		}
> +		break;
> +	case hwmon_temp:
> +		params[0] = channel;
> +		ret = tuxi_tfan_method(pdev,
> +				       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
> +				       params, 1, &retval);
> +		*val = retval * 100 - 272000;
> +		return ret;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int hwm_read_string(struct device *dev,
> +			   enum hwmon_sensor_types __always_unused type,
> +			   u32 __always_unused attr, int channel,
> +			   const char **str)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +
> +	*str = driver_data->fan_types[channel];
> +
> +	return 0;
> +}
> +
> +static int write_speed(struct device *dev, int channel, long val)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	int ret;
> +
> +	unsigned long long params[2];
> +
> +	params[0] = channel;
> +
> +	u8 temp_level, min_speed;
> +
> +	temp_level = driver_data->temp_level[channel];
> +	if (temp_level) {
> +		min_speed = temp_levels[temp_level].min_speed;
> +		if (val < min_speed)
> +			val = min_speed;
> +	}
> +
> +	if (val < TUXI_PWM_FAN_ON_MIN_SPEED / 2)
> +		params[1] = 0;
> +	else if (val < TUXI_PWM_FAN_ON_MIN_SPEED)
> +		params[1] = TUXI_PWM_FAN_ON_MIN_SPEED;
> +	else
> +		params[1] = val;
> +
> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_SPEED,
> +			       params, 2, NULL);
> +	if (ret)
> +		return ret;
> +
> +	driver_data->curr_speed[channel] = val;
> +
> +	return 0;
> +}
> +
> +static int hwm_write(struct device *dev,
> +		     enum hwmon_sensor_types __always_unused type, u32 attr,
> +		     int channel, long val)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	unsigned int i;
> +	int ret;
> +
> +	unsigned long long params[2];
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (driver_data->pwm_enabled) {
> +			driver_data->want_speed[channel] = val;
> +			return write_speed(dev, channel, val);
> +		}
> +
> +		return 0;
> +	case hwmon_pwm_enable:
> +		params[0] = val;
> +		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE,
> +				       params, 1, NULL);
> +		if (ret)
> +			return ret;
> +
> +		driver_data->pwm_enabled = val;
> +
> +		/* Activating PWM sets speed to 0. Alternativ design decision

Alternative

> +		 * could be to keep the current value. This would require proper
> +		 * setting of driver_data->curr_speed for example.
> +		 */
> +		if (val)
> +			for (i = 0; i < driver_data->fan_count; ++i) {
> +				ret = write_speed(dev, i, 0);
> +				if (ret)
> +					return ret;
> +			}
> +
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops hwmops = {
> +	.is_visible = hwm_is_visible,
> +	.read = hwm_read,
> +	.read_string = hwm_read_string,
> +	.write = hwm_write,
> +};
> +
> +static struct hwmon_channel_info hwmcinfo_fan = {
> +	.type = hwmon_fan,
> +};
> +
> +static struct hwmon_channel_info hwmcinfo_pwm = {
> +	.type = hwmon_pwm,
> +};
> +
> +static struct hwmon_channel_info hwmcinfo_temp = {
> +	.type = hwmon_temp,
> +};
> +
> +static const struct hwmon_channel_info * const hwmcinfo[] = {
> +	&hwmcinfo_fan,
> +	&hwmcinfo_pwm,
> +	&hwmcinfo_temp,
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info hwminfo = {
> +	.ops = &hwmops,
> +	.info = hwmcinfo
> +};
> +
> +static void tuxi_periodic_hw_safeguard(struct work_struct *work)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data;
> +
> +	driver_data = container_of(work, struct tuxi_hwmon_driver_data_t,
> +				   work.work);

No, please don't mix code like this into the variable definitions.

If you have long type names and want to remain within 80 chars, you get to 
keep the pieces. I'll not be accept compromises in other aspects of coding 
style because of that self-inflicted pain. ;-)

I suggest you just rename the type to struct tuxi_hwmon_data and consider
disregarding 80 chars for the container_of() lines, container_of lines are 
just boilerplate with only little useful content so exceeding 80 chars is 
not going to make the code harder to read in general.

> +
> +	struct device *dev = driver_data->hwmdev;
> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
> +	unsigned int i, j;
> +
> +	unsigned long long params[2];
> +	unsigned long long retval;

Join these two to the same line.

> +
> +	long temp, temp_low, temp_high;
> +	u8 temp_level, min_speed, curr_speed, want_speed;

Order to reverse xmas-tree (to the extent you can).

Remove all empty lines within the variable definition block.

> +
> +	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 ?

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.

> +
> +			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.


> +
> +		temp_level = driver_data->temp_level[i];
> +		min_speed = temp_level == 0 ?
> +			0 : temp_levels[temp_level-1].min_speed;

I suggest you reverse logic and add the space around - operator.

> +		curr_speed = driver_data->curr_speed[i];
> +		want_speed = driver_data->want_speed[i];
> +
> +		if (want_speed < min_speed) {
> +			if (curr_speed < min_speed)
> +				write_speed(dev, i, min_speed);

I'd tend to think you can probably use max() to set want_speed and then 
do this once after it:

> +		} else if (curr_speed != want_speed)
> +			write_speed(dev, i, want_speed);
> +	}
> +
> +	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
> +}
> +
> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data;
> +	int ret;
> +
> +	unsigned long long params[2];
> +	unsigned long long retval;
> +
> +	u32 *hwmcinfo_fan_config, *hwmcinfo_pwm_config, *hwmcinfo_temp_config;

No empty lines like this, join params & retval to same line and order to 
reverse xmas tree.

> +
> +	/* The first version of TUXI TFAN didn't have the Get Fan Temperature
> +	 * method which is integral to this driver. So probe for existence and
> +	 * abort otherwise.
> +	 *
> +	 * The Get Fan Speed method is also missing in that version, but was
> +	 * added in the same version so it doesn't have to be probe separately.
> +	 */
> +	params[0] = 0;
> +	ret = tuxi_tfan_method(pdev,
> +			       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
> +			       params, 1, &retval);
> +	if (ret)
> +		return ret;
> +
> +	driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);
> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	/* Loading this module sets the fan mode to auto. Alternative design
> +	 * decision could be to keep the current value. This would require
> +	 * proper initialization of driver_data->curr_speed for example.
> +	 */
> +	params[0] = 0;
> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1,
> +			       NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL, 0,
> +			       &retval);
> +	if (ret)
> +		return ret;
> +	driver_data->fan_count = retval;
> +
> +	driver_data->temp_level = devm_kcalloc(&pdev->dev,
> +					       driver_data->fan_count,
> +					       sizeof(*driver_data->temp_level),
> +					       GFP_KERNEL);
> +	if (driver_data->temp_level == NULL)
> +		return -ENOMEM;
> +	driver_data->curr_speed = devm_kcalloc(&pdev->dev,
> +					       driver_data->fan_count,
> +					       sizeof(*driver_data->curr_speed),
> +					       GFP_KERNEL);
> +	if (driver_data->curr_speed == NULL)
> +		return -ENOMEM;
> +	driver_data->want_speed = devm_kcalloc(&pdev->dev,
> +					       driver_data->fan_count,
> +					       sizeof(*driver_data->want_speed),
> +					       GFP_KERNEL);
> +	if (driver_data->want_speed == NULL)
> +		return -ENOMEM;
> +	driver_data->fan_types = devm_kcalloc(&pdev->dev,
> +					      driver_data->fan_count,
> +					      sizeof(*driver_data->fan_types),
> +					      GFP_KERNEL);
> +	if (driver_data->fan_types == NULL)
> +		return -ENOMEM;
> +
> +	hwmcinfo_fan_config = devm_kcalloc(&pdev->dev,
> +					   driver_data->fan_count + 1,
> +					   sizeof(*hwmcinfo_fan_config),
> +					   GFP_KERNEL);
> +	if (hwmcinfo_fan_config == NULL)
> +		return -ENOMEM;
> +	hwmcinfo_pwm_config = devm_kcalloc(&pdev->dev,
> +					   driver_data->fan_count + 1,
> +					   sizeof(*hwmcinfo_pwm_config),
> +					   GFP_KERNEL);
> +	if (hwmcinfo_pwm_config == NULL)
> +		return -ENOMEM;
> +	hwmcinfo_temp_config = devm_kcalloc(&pdev->dev,
> +					    driver_data->fan_count + 1,
> +					    sizeof(*hwmcinfo_temp_config),
> +					    GFP_KERNEL);
> +	if (hwmcinfo_temp_config == NULL)
> +		return -ENOMEM;
> +
> +	for (unsigned long long i = 0; i < driver_data->fan_count; ++i) {
> +		hwmcinfo_fan_config[i] = HWMON_F_INPUT | HWMON_F_LABEL;
> +		hwmcinfo_pwm_config[i] = HWMON_PWM_INPUT | HWMON_PWM_ENABLE;
> +		hwmcinfo_temp_config[i] = HWMON_T_INPUT | HWMON_T_LABEL;
> +
> +		params[0] = i;
> +		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TYPE,
> +				       params, 1, &retval);
> +		if (ret)
> +			return ret;
> +		else if (retval >= ARRAY_SIZE(tuxi_fan_type_labels))
> +			return -EOPNOTSUPP;
> +		driver_data->fan_types[i] = tuxi_fan_type_labels[retval];
> +	}
> +	hwmcinfo_fan.config = hwmcinfo_fan_config;
> +	hwmcinfo_pwm.config = hwmcinfo_pwm_config;
> +	hwmcinfo_temp.config = hwmcinfo_temp_config;
> +
> +	*hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
> +						       "tuxedo_nbxx_acpi_tuxi",
> +						       driver_data, &hwminfo,
> +						       NULL);
> +	if (PTR_ERR_OR_ZERO(*hwmdev))
> +		return PTR_ERR_OR_ZERO(*hwmdev);
> +
> +	driver_data->hwmdev = *hwmdev;
> +
> +	INIT_DELAYED_WORK(&driver_data->work, tuxi_periodic_hw_safeguard);
> +	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tuxi_hwmon_add_devices);
> +
> +void tuxi_hwmon_remove_devices(struct device *hwmdev)
> +{
> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(hwmdev);
> +	struct acpi_device *pdev = to_acpi_device(hwmdev->parent);
> +
> +	unsigned long long params[2];
> +
> +	cancel_delayed_work_sync(&driver_data->work);
> +
> +	params[0] = 0;
> +	tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1, NULL);
> +}
> +EXPORT_SYMBOL(tuxi_hwmon_remove_devices);
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
> new file mode 100644
> index 0000000000000..248730fab8574
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
> +
> +#include <linux/acpi.h>
> +
> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev);
> +void tuxi_hwmon_remove_devices(struct device *hwmdev);
> +
> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
> new file mode 100644
> index 0000000000000..5e8edb458aba2
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +
> +#include "acpi_tuxi_hwmon.h"
> +
> +#include "acpi_tuxi_init.h"
> +
> +static const struct acpi_device_id acpi_device_ids[] = {
> +	{"TUXI0000", 0},
> +	{"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
> +
> +static int add(struct acpi_device *device)

Please add proper prefixes, never use too generic names for driver 
specific functions, even if they'd be static. Generic naming makes reading 
the code much harder than it needs to be as the prefix tells important 
information (that a function is _not_ a generic function).

> +{
> +	struct tuxi_driver_data_t *driver_data;
> +	acpi_status status;
> +
> +	driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);

I'd put this to a single line.

> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	// Find subdevices
> +	status = acpi_get_handle(device->handle, "TFAN",
> +				 &driver_data->tfan_handle);
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
> +
> +	dev_set_drvdata(&device->dev, driver_data);
> +
> +	return tuxi_hwmon_add_devices(device, &driver_data->hwmdev);
> +}
> +
> +static void remove(struct acpi_device *device)
> +{
> +	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
> +
> +	tuxi_hwmon_remove_devices(driver_data->hwmdev);
> +}
> +
> +static struct acpi_driver acpi_driver = {
> +	.name = "tuxedo_nbxx_acpi_tuxi",
> +	.ids = acpi_device_ids,
> +	.ops = {
> +		.add = add,
> +		.remove = remove,
> +	},
> +};
> +
> +module_acpi_driver(acpi_driver);
> +
> +MODULE_DESCRIPTION("TUXEDO TUXI");
> +MODULE_AUTHOR("Werner Sembach <[e-mail]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
> new file mode 100644
> index 0000000000000..33388332a2328
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
> +
> +#include <linux/acpi.h>
> +
> +struct tuxi_driver_data_t {
> +	acpi_handle tfan_handle;
> +	struct device *hwmdev;
> +};
> +
> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
> 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 [e-mail]
> + */
> +
> +#include <linux/acpi.h>
> +
> +#include "acpi_tuxi_init.h"
> +

Remove empty line but see first what I note below.

> +#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.

> +{
> +	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

> +			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.

> +	} 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.

> +	}
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
> +
> +	if (retval)
> +		*retval = data;
> +
> +	return 0;
> +}
> +
> +int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
> +		     unsigned long long *params, u32 pcount,
> +		     unsigned long long *retval)
> +{
> +	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
> +
> +	return __acpi_eval_intarray_in_int_out(driver_data->tfan_handle, method,
> +					       params, pcount, retval);
> +}
> +EXPORT_SYMBOL(tuxi_tfan_method);
> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
> new file mode 100644
> index 0000000000000..670fe02249d06
> --- /dev/null
> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
> + */
> +
> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
> +
> +#include <linux/acpi.h>
> +
> +/*
> + * Set fan speed target
> + *
> + * Set a specific fan speed (needs manual mode)
> + *
> + * Arg0: Fan index
> + * Arg1: Fan speed as a fraction of maximum speed (0-255)
> + */
> +#define TUXI_TFAN_METHOD_SET_FAN_SPEED		"SSPD"
> +
> +/*
> + * Get fan speed target
> + *
> + * Arg0: Fan index
> + * Returns: Current fan speed target a fraction of maximum speed (0-255)
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_SPEED		"GSPD"
> +
> +/*
> + * Get fans count
> + *
> + * Returns: Number of individually controllable fans
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_COUNT		"GCNT"
> +
> +/*
> + * Set fans mode
> + *
> + * Arg0: 0 = auto, 1 = manual
> + */
> +#define TUXI_TFAN_METHOD_SET_FAN_MODE		"SMOD"
> +
> +/*
> + * Get fans mode
> + *
> + * Returns: 0 = auto, 1 = manual
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_MODE		"GMOD"
> +
> +/*
> + * Get fan type/what the fan is pointed at
> + *
> + * Arg0: Fan index
> + * Returns: 0 = general, 1 = CPU, 2 = GPU
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_TYPE		"GTYP"
> +
> +static const char * const tuxi_fan_type_labels[] = {
> +	"general",
> +	"cpu",
> +	"gpu"
> +};
> +
> +/*
> + * Get fan temperature/temperature of what the fan is pointed at
> + *
> + * Arg0: Fan index
> + * Returns: Temperature sensor value in 10ths of degrees kelvin
> + */
> +#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE	"GTMP"
> +
> +/*
> + * Get actual fan speed in RPM
> + *
> + * 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.

Werner Sembach Feb. 6, 2025, 3:38 p.m. UTC | #4

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 <[e-mail]>
>> ---
>>   MAINTAINERS                                   |   6 +
>>   drivers/platform/x86/Kconfig                  |   2 +
>>   drivers/platform/x86/Makefile                 |   3 +
>>   drivers/platform/x86/tuxedo/Kbuild            |   6 +
>>   drivers/platform/x86/tuxedo/Kconfig           |   6 +
>>   drivers/platform/x86/tuxedo/nbxx/Kbuild       |   9 +
>>   drivers/platform/x86/tuxedo/nbxx/Kconfig      |  13 +
>>   .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.c         | 421 ++++++++++++++++++
>>   .../x86/tuxedo/nbxx/acpi_tuxi_hwmon.h         |  14 +
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.c |  60 +++
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_init.h |  16 +
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.c |  58 +++
>>   .../platform/x86/tuxedo/nbxx/acpi_tuxi_util.h |  84 ++++
>>   13 files changed, 698 insertions(+)
>>   create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>>   create mode 100644 drivers/platform/x86/tuxedo/Kconfig
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.c
>>   create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0fa7c5728f1e6..2d3fe9de4e9cf 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23916,6 +23916,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
>>   F:	tools/power/x86/turbostat/
>>   F:	tools/testing/selftests/turbostat/
>>   
>> +TUXEDO DRIVERS
>> +M:	Werner Sembach <[e-mail]>
>> +L:	[e-mail]
>> +S:	Supported
>> +F:	drivers/platform/x86/tuxedo/
>> +
>>   TW5864 VIDEO4LINUX DRIVER
>>   M:	Bluecherry Maintainers <[e-mail]>
>>   M:	Andrey Utkin <[e-mail]>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 0258dd879d64b..9b78a1255c08e 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1199,3 +1199,5 @@ config P2SB
>>   	  The main purpose of this library is to unhide P2SB device in case
>>   	  firmware kept it hidden on some platforms in order to access devices
>>   	  behind it.
>> +
>> +source "drivers/platform/x86/tuxedo/Kconfig"
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index e1b1429470674..1562dcd7ad9a5 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
>>   
>>   # SEL
>>   obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
>> +
>> +# TUXEDO
>> +obj-y					+= tuxedo/
>> diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
>> new file mode 100644
>> index 0000000000000..0de6c7871db95
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/Kbuild
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +obj-y	+= nbxx/
>> diff --git a/drivers/platform/x86/tuxedo/Kconfig b/drivers/platform/x86/tuxedo/Kconfig
>> new file mode 100644
>> index 0000000000000..82df7419cf29d
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/Kconfig
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +source "drivers/platform/x86/tuxedo/nb04/Kconfig"
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
>> new file mode 100644
>> index 0000000000000..db8def8ffe8f6
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +tuxedo_nbxx_acpi_tuxi-y			:= acpi_tuxi_init.o
>> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_util.o
>> +tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_hwmon.o
>> +obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)	+= tuxedo_nbxx_acpi_tuxi.o
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
>> new file mode 100644
>> index 0000000000000..827c65c410fb2
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
>> @@ -0,0 +1,13 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +#
>> +# TUXEDO X86 Platform Specific Drivers
>> +#
>> +
>> +config TUXEDO_NBXX_ACPI_TUXI
>> +	tristate "TUXEDO NBxx ACPI TUXI Platform Driver"
>> +	help
>> +	  This driver implements the ACPI TUXI device found on some TUXEDO
>> +	  Notebooks. Currently this consists only of the TFAN subdevice which is
>> +	  implemented via hwmon.
>> +
>> +	  When compiled as a module it will be called tuxedo_nbxx_acpi_tuxi
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>> new file mode 100644
>> index 0000000000000..8ebb2bfc19e20
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
>> @@ -0,0 +1,421 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "acpi_tuxi_util.h"
>> +
>> +#include "acpi_tuxi_hwmon.h"
>> +
>> +struct tuxi_hwmon_driver_data_t {
>> +	struct delayed_work work;
>> +	struct device *hwmdev;
>> +	u8 fan_count;
>> +	const char **fan_types;
>> +	u8 *temp_level;
>> +	u8 *curr_speed;
>> +	u8 *want_speed;
>> +	u8 pwm_enabled;
>> +};
>> +
>> +struct tuxi_temp_high_config_t {
>> +	long temp;
>> +	u8 min_speed;
>> +};
>> +
>> +#define TUXI_SAFEGUARD_PERIOD 1000
>> +
>> +#define TUXI_PWM_FAN_ON_MIN_SPEED 0x40 // ~25%
>> +
>> +static struct tuxi_temp_high_config_t temp_levels[] = {
>> +	{  80000, 0x4d }, // ~30%
>> +	{  90000, 0x66 }, // ~40%
>> +	{  95000, 0x99 }, // ~60%
>> +	{ 100000, 0xff }, // 100%
>> +	{ }
>> +};
>> +
>> +static umode_t hwm_is_visible(const void * __always_unused data,
>> +			      enum hwmon_sensor_types type,
>> +			      u32 __always_unused attr,
>> +			      int __always_unused channel)
>> +{
>> +	switch (type) {
>> +	case hwmon_fan:
>> +		return 0444;
>> +	case hwmon_pwm:
>> +		return 0644;
>> +	case hwmon_temp:
>> +		return 0444;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +		    int channel, long *val)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +	int ret;
>> +
>> +	unsigned long long params[2];
>> +	unsigned long long retval;
>> +
>> +	switch (type) {
>> +	case hwmon_fan:
>> +		params[0] = channel;
>> +		ret = tuxi_tfan_method(pdev,
>> +				       TUXI_TFAN_METHOD_GET_FAN_RPM,
>> +				       params, 1, &retval);
>> +		*val = retval;
>> +		return ret;
>> +	case hwmon_pwm:
>> +		switch (attr) {
>> +		case hwmon_pwm_input:
>> +			if (driver_data->pwm_enabled) {
>> +				*val = driver_data->curr_speed[channel];
>> +				return 0;
>> +			}
>> +			params[0] = channel;
>> +			ret = tuxi_tfan_method(pdev,
>> +					       TUXI_TFAN_METHOD_GET_FAN_SPEED,
>> +					       params, 1, &retval);
>> +			*val = retval;
>> +			return ret;
>> +		case hwmon_pwm_enable:
>> +			*val = driver_data->pwm_enabled;
>> +			return ret;
>> +		}
>> +		break;
>> +	case hwmon_temp:
>> +		params[0] = channel;
>> +		ret = tuxi_tfan_method(pdev,
>> +				       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>> +				       params, 1, &retval);
>> +		*val = retval * 100 - 272000;
>> +		return ret;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int hwm_read_string(struct device *dev,
>> +			   enum hwmon_sensor_types __always_unused type,
>> +			   u32 __always_unused attr, int channel,
>> +			   const char **str)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +
>> +	*str = driver_data->fan_types[channel];
>> +
>> +	return 0;
>> +}
>> +
>> +static int write_speed(struct device *dev, int channel, long val)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +	int ret;
>> +
>> +	unsigned long long params[2];
>> +
>> +	params[0] = channel;
>> +
>> +	u8 temp_level, min_speed;
>> +
>> +	temp_level = driver_data->temp_level[channel];
>> +	if (temp_level) {
>> +		min_speed = temp_levels[temp_level].min_speed;
>> +		if (val < min_speed)
>> +			val = min_speed;
>> +	}
>> +
>> +	if (val < TUXI_PWM_FAN_ON_MIN_SPEED / 2)
>> +		params[1] = 0;
>> +	else if (val < TUXI_PWM_FAN_ON_MIN_SPEED)
>> +		params[1] = TUXI_PWM_FAN_ON_MIN_SPEED;
>> +	else
>> +		params[1] = val;
>> +
>> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_SPEED,
>> +			       params, 2, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	driver_data->curr_speed[channel] = val;
>> +
>> +	return 0;
>> +}
>> +
>> +static int hwm_write(struct device *dev,
>> +		     enum hwmon_sensor_types __always_unused type, u32 attr,
>> +		     int channel, long val)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	unsigned long long params[2];
>> +
>> +	switch (attr) {
>> +	case hwmon_pwm_input:
>> +		if (driver_data->pwm_enabled) {
>> +			driver_data->want_speed[channel] = val;
>> +			return write_speed(dev, channel, val);
>> +		}
>> +
>> +		return 0;
>> +	case hwmon_pwm_enable:
>> +		params[0] = val;
>> +		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE,
>> +				       params, 1, NULL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		driver_data->pwm_enabled = val;
>> +
>> +		/* Activating PWM sets speed to 0. Alternativ design decision
> Alternative
thx for spotting
>
>> +		 * could be to keep the current value. This would require proper
>> +		 * setting of driver_data->curr_speed for example.
>> +		 */
>> +		if (val)
>> +			for (i = 0; i < driver_data->fan_count; ++i) {
>> +				ret = write_speed(dev, i, 0);
>> +				if (ret)
>> +					return ret;
>> +			}
>> +
>> +		return 0;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops hwmops = {
>> +	.is_visible = hwm_is_visible,
>> +	.read = hwm_read,
>> +	.read_string = hwm_read_string,
>> +	.write = hwm_write,
>> +};
>> +
>> +static struct hwmon_channel_info hwmcinfo_fan = {
>> +	.type = hwmon_fan,
>> +};
>> +
>> +static struct hwmon_channel_info hwmcinfo_pwm = {
>> +	.type = hwmon_pwm,
>> +};
>> +
>> +static struct hwmon_channel_info hwmcinfo_temp = {
>> +	.type = hwmon_temp,
>> +};
>> +
>> +static const struct hwmon_channel_info * const hwmcinfo[] = {
>> +	&hwmcinfo_fan,
>> +	&hwmcinfo_pwm,
>> +	&hwmcinfo_temp,
>> +	NULL
>> +};
>> +
>> +static const struct hwmon_chip_info hwminfo = {
>> +	.ops = &hwmops,
>> +	.info = hwmcinfo
>> +};
>> +
>> +static void tuxi_periodic_hw_safeguard(struct work_struct *work)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data;
>> +
>> +	driver_data = container_of(work, struct tuxi_hwmon_driver_data_t,
>> +				   work.work);
> No, please don't mix code like this into the variable definitions.
>
> If you have long type names and want to remain within 80 chars, you get to
> keep the pieces. I'll not be accept compromises in other aspects of coding
> style because of that self-inflicted pain. ;-)
>
> I suggest you just rename the type to struct tuxi_hwmon_data and consider
> disregarding 80 chars for the container_of() lines, container_of lines are
> just boilerplate with only little useful content so exceeding 80 chars is
> not going to make the code harder to read in general.
>
>> +
>> +	struct device *dev = driver_data->hwmdev;
>> +	struct acpi_device *pdev = to_acpi_device(dev->parent);
>> +	unsigned int i, j;
>> +
>> +	unsigned long long params[2];
>> +	unsigned long long retval;
> Join these two to the same line.
kk
>
>> +
>> +	long temp, temp_low, temp_high;
>> +	u8 temp_level, min_speed, curr_speed, want_speed;
> Order to reverse xmas-tree (to the extent you can).
>
> Remove all empty lines within the variable definition block.
kk
>
>> +
>> +	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.

>
> 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.
>
>
>> +
>> +		temp_level = driver_data->temp_level[i];
>> +		min_speed = temp_level == 0 ?
>> +			0 : temp_levels[temp_level-1].min_speed;
> I suggest you reverse logic and add the space around - operator.
kk
>
>> +		curr_speed = driver_data->curr_speed[i];
>> +		want_speed = driver_data->want_speed[i];
>> +
>> +		if (want_speed < min_speed) {
>> +			if (curr_speed < min_speed)
>> +				write_speed(dev, i, min_speed);
> I'd tend to think you can probably use max() to set want_speed and then
> do this once after it:
kk
>> +		} else if (curr_speed != want_speed)
>> +			write_speed(dev, i, want_speed);
>> +	}
>> +
>> +	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>> +}
>> +
>> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data;
>> +	int ret;
>> +
>> +	unsigned long long params[2];
>> +	unsigned long long retval;
>> +
>> +	u32 *hwmcinfo_fan_config, *hwmcinfo_pwm_config, *hwmcinfo_temp_config;
> No empty lines like this, join params & retval to same line and order to
> reverse xmas tree.
kk
>
>> +
>> +	/* The first version of TUXI TFAN didn't have the Get Fan Temperature
>> +	 * method which is integral to this driver. So probe for existence and
>> +	 * abort otherwise.
>> +	 *
>> +	 * The Get Fan Speed method is also missing in that version, but was
>> +	 * added in the same version so it doesn't have to be probe separately.
>> +	 */
>> +	params[0] = 0;
>> +	ret = tuxi_tfan_method(pdev,
>> +			       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>> +			       params, 1, &retval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
>> +				   GFP_KERNEL);
>> +	if (!driver_data)
>> +		return -ENOMEM;
>> +
>> +	/* Loading this module sets the fan mode to auto. Alternative design
>> +	 * decision could be to keep the current value. This would require
>> +	 * proper initialization of driver_data->curr_speed for example.
>> +	 */
>> +	params[0] = 0;
>> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1,
>> +			       NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL, 0,
>> +			       &retval);
>> +	if (ret)
>> +		return ret;
>> +	driver_data->fan_count = retval;
>> +
>> +	driver_data->temp_level = devm_kcalloc(&pdev->dev,
>> +					       driver_data->fan_count,
>> +					       sizeof(*driver_data->temp_level),
>> +					       GFP_KERNEL);
>> +	if (driver_data->temp_level == NULL)
>> +		return -ENOMEM;
>> +	driver_data->curr_speed = devm_kcalloc(&pdev->dev,
>> +					       driver_data->fan_count,
>> +					       sizeof(*driver_data->curr_speed),
>> +					       GFP_KERNEL);
>> +	if (driver_data->curr_speed == NULL)
>> +		return -ENOMEM;
>> +	driver_data->want_speed = devm_kcalloc(&pdev->dev,
>> +					       driver_data->fan_count,
>> +					       sizeof(*driver_data->want_speed),
>> +					       GFP_KERNEL);
>> +	if (driver_data->want_speed == NULL)
>> +		return -ENOMEM;
>> +	driver_data->fan_types = devm_kcalloc(&pdev->dev,
>> +					      driver_data->fan_count,
>> +					      sizeof(*driver_data->fan_types),
>> +					      GFP_KERNEL);
>> +	if (driver_data->fan_types == NULL)
>> +		return -ENOMEM;
>> +
>> +	hwmcinfo_fan_config = devm_kcalloc(&pdev->dev,
>> +					   driver_data->fan_count + 1,
>> +					   sizeof(*hwmcinfo_fan_config),
>> +					   GFP_KERNEL);
>> +	if (hwmcinfo_fan_config == NULL)
>> +		return -ENOMEM;
>> +	hwmcinfo_pwm_config = devm_kcalloc(&pdev->dev,
>> +					   driver_data->fan_count + 1,
>> +					   sizeof(*hwmcinfo_pwm_config),
>> +					   GFP_KERNEL);
>> +	if (hwmcinfo_pwm_config == NULL)
>> +		return -ENOMEM;
>> +	hwmcinfo_temp_config = devm_kcalloc(&pdev->dev,
>> +					    driver_data->fan_count + 1,
>> +					    sizeof(*hwmcinfo_temp_config),
>> +					    GFP_KERNEL);
>> +	if (hwmcinfo_temp_config == NULL)
>> +		return -ENOMEM;
>> +
>> +	for (unsigned long long i = 0; i < driver_data->fan_count; ++i) {
>> +		hwmcinfo_fan_config[i] = HWMON_F_INPUT | HWMON_F_LABEL;
>> +		hwmcinfo_pwm_config[i] = HWMON_PWM_INPUT | HWMON_PWM_ENABLE;
>> +		hwmcinfo_temp_config[i] = HWMON_T_INPUT | HWMON_T_LABEL;
>> +
>> +		params[0] = i;
>> +		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TYPE,
>> +				       params, 1, &retval);
>> +		if (ret)
>> +			return ret;
>> +		else if (retval >= ARRAY_SIZE(tuxi_fan_type_labels))
>> +			return -EOPNOTSUPP;
>> +		driver_data->fan_types[i] = tuxi_fan_type_labels[retval];
>> +	}
>> +	hwmcinfo_fan.config = hwmcinfo_fan_config;
>> +	hwmcinfo_pwm.config = hwmcinfo_pwm_config;
>> +	hwmcinfo_temp.config = hwmcinfo_temp_config;
>> +
>> +	*hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
>> +						       "tuxedo_nbxx_acpi_tuxi",
>> +						       driver_data, &hwminfo,
>> +						       NULL);
>> +	if (PTR_ERR_OR_ZERO(*hwmdev))
>> +		return PTR_ERR_OR_ZERO(*hwmdev);
>> +
>> +	driver_data->hwmdev = *hwmdev;
>> +
>> +	INIT_DELAYED_WORK(&driver_data->work, tuxi_periodic_hw_safeguard);
>> +	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(tuxi_hwmon_add_devices);
>> +
>> +void tuxi_hwmon_remove_devices(struct device *hwmdev)
>> +{
>> +	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(hwmdev);
>> +	struct acpi_device *pdev = to_acpi_device(hwmdev->parent);
>> +
>> +	unsigned long long params[2];
>> +
>> +	cancel_delayed_work_sync(&driver_data->work);
>> +
>> +	params[0] = 0;
>> +	tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1, NULL);
>> +}
>> +EXPORT_SYMBOL(tuxi_hwmon_remove_devices);
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>> new file mode 100644
>> index 0000000000000..248730fab8574
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
>> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
>> +
>> +#include <linux/acpi.h>
>> +
>> +int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev);
>> +void tuxi_hwmon_remove_devices(struct device *hwmdev);
>> +
>> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>> new file mode 100644
>> index 0000000000000..5e8edb458aba2
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
>> @@ -0,0 +1,60 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/module.h>
>> +
>> +#include "acpi_tuxi_hwmon.h"
>> +
>> +#include "acpi_tuxi_init.h"
>> +
>> +static const struct acpi_device_id acpi_device_ids[] = {
>> +	{"TUXI0000", 0},
>> +	{"", 0}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
>> +
>> +static int add(struct acpi_device *device)
> Please add proper prefixes, never use too generic names for driver
> specific functions, even if they'd be static. Generic naming makes reading
> the code much harder than it needs to be as the prefix tells important
> information (that a function is _not_ a generic function).
kk
>
>> +{
>> +	struct tuxi_driver_data_t *driver_data;
>> +	acpi_status status;
>> +
>> +	driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
>> +				   GFP_KERNEL);
> I'd put this to a single line.
kk
>
>> +	if (!driver_data)
>> +		return -ENOMEM;
>> +
>> +	// Find subdevices
>> +	status = acpi_get_handle(device->handle, "TFAN",
>> +				 &driver_data->tfan_handle);
>> +	if (ACPI_FAILURE(status))
>> +		return_ACPI_STATUS(status);
>> +
>> +	dev_set_drvdata(&device->dev, driver_data);
>> +
>> +	return tuxi_hwmon_add_devices(device, &driver_data->hwmdev);
>> +}
>> +
>> +static void remove(struct acpi_device *device)
>> +{
>> +	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
>> +
>> +	tuxi_hwmon_remove_devices(driver_data->hwmdev);
>> +}
>> +
>> +static struct acpi_driver acpi_driver = {
>> +	.name = "tuxedo_nbxx_acpi_tuxi",
>> +	.ids = acpi_device_ids,
>> +	.ops = {
>> +		.add = add,
>> +		.remove = remove,
>> +	},
>> +};
>> +
>> +module_acpi_driver(acpi_driver);
>> +
>> +MODULE_DESCRIPTION("TUXEDO TUXI");
>> +MODULE_AUTHOR("Werner Sembach <[e-mail]>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>> new file mode 100644
>> index 0000000000000..33388332a2328
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
>> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
>> +
>> +#include <linux/acpi.h>
>> +
>> +struct tuxi_driver_data_t {
>> +	acpi_handle tfan_handle;
>> +	struct device *hwmdev;
>> +};
>> +
>> +#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
>> 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 [e-mail]
>> + */
>> +
>> +#include <linux/acpi.h>
>> +
>> +#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.

also i don't know if ?-operator in a function call is good to read.

>
>> +	}
>> +	if (ACPI_FAILURE(status))
>> +		return_ACPI_STATUS(status);
>> +
>> +	if (retval)
>> +		*retval = data;
>> +
>> +	return 0;
>> +}
>> +
>> +int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
>> +		     unsigned long long *params, u32 pcount,
>> +		     unsigned long long *retval)
>> +{
>> +	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
>> +
>> +	return __acpi_eval_intarray_in_int_out(driver_data->tfan_handle, method,
>> +					       params, pcount, retval);
>> +}
>> +EXPORT_SYMBOL(tuxi_tfan_method);
>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
>> new file mode 100644
>> index 0000000000000..670fe02249d06
>> --- /dev/null
>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
>> @@ -0,0 +1,84 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2024-2025 Werner Sembach [e-mail]
>> + */
>> +
>> +#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
>> +#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
>> +
>> +#include <linux/acpi.h>
>> +
>> +/*
>> + * Set fan speed target
>> + *
>> + * Set a specific fan speed (needs manual mode)
>> + *
>> + * Arg0: Fan index
>> + * Arg1: Fan speed as a fraction of maximum speed (0-255)
>> + */
>> +#define TUXI_TFAN_METHOD_SET_FAN_SPEED		"SSPD"
>> +
>> +/*
>> + * Get fan speed target
>> + *
>> + * Arg0: Fan index
>> + * Returns: Current fan speed target a fraction of maximum speed (0-255)
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_SPEED		"GSPD"
>> +
>> +/*
>> + * Get fans count
>> + *
>> + * Returns: Number of individually controllable fans
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_COUNT		"GCNT"
>> +
>> +/*
>> + * Set fans mode
>> + *
>> + * Arg0: 0 = auto, 1 = manual
>> + */
>> +#define TUXI_TFAN_METHOD_SET_FAN_MODE		"SMOD"
>> +
>> +/*
>> + * Get fans mode
>> + *
>> + * Returns: 0 = auto, 1 = manual
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_MODE		"GMOD"
>> +
>> +/*
>> + * Get fan type/what the fan is pointed at
>> + *
>> + * Arg0: Fan index
>> + * Returns: 0 = general, 1 = CPU, 2 = GPU
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_TYPE		"GTYP"
>> +
>> +static const char * const tuxi_fan_type_labels[] = {
>> +	"general",
>> +	"cpu",
>> +	"gpu"
>> +};
>> +
>> +/*
>> + * Get fan temperature/temperature of what the fan is pointed at
>> + *
>> + * Arg0: Fan index
>> + * Returns: Temperature sensor value in 10ths of degrees kelvin
>> + */
>> +#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE	"GTMP"
>> +
>> +/*
>> + * Get actual fan speed in RPM
>> + *
>> + * 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.

Best regards,

Werner

Guenter Roeck Feb. 6, 2025, 6:57 p.m. UTC | #5

On Thu, Feb 06, 2025 at 10:28:01AM +0100, Werner Sembach wrote:

[ ... ]

> > > +        temp = retval * 100 - 272000;
> > > +
> > > +        for (j = 0; temp_levels[j].temp; ++j) {
> > > +            temp_low = j == 0 ? -272000 : temp_levels[j-1].temp;
> > > +            temp_high = temp_levels[j].temp;
> > > +            if (driver_data->temp_level[i] > j)
> > > +                temp_high -= 2000; // hysteresis
> > > +
> > > +            if (temp >= temp_low && temp < temp_high)
> > > +                driver_data->temp_level[i] = j;
> > > +        }
> > > +        if (temp >= temp_high)
> > > +            driver_data->temp_level[i] = j;
> > > +
> > > +        temp_level = driver_data->temp_level[i];
> > > +        min_speed = temp_level == 0 ?
> > > +            0 : temp_levels[temp_level-1].min_speed;
> > > +        curr_speed = driver_data->curr_speed[i];
> > > +        want_speed = driver_data->want_speed[i];
> > > +
> > > +        if (want_speed < min_speed) {
> > > +            if (curr_speed < min_speed)
> > > +                write_speed(dev, i, min_speed);
> > > +        } else if (curr_speed != want_speed)
> > > +            write_speed(dev, i, want_speed);
> > > +    }
> > > +
> > > +    schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
> > > +}
> > 
> > This is not expected functionality of a hardware monitoring driver.
> > Hardware monmitoring drivers should not replicate userspace or
> > thermal subsystem functionality.
> > 
> > This would be unacceptable in drivers/hwmon/.
> 
> Problem is: The thermal subsystem doesn't do this either as far as I can tell.
> 
> See this: https://lore.kernel.org/all/[e-mail]/
> and this: https://lore.kernel.org/all/[e-mail]/
> thread.
> 
> The short version is: The Thermal subsystem always allows userspace to
> select the "userspace" governor which has no way for the kernel to enforce a
> minimum speed.
> 
You can specify thermal parameters / limits using devicetree. Also, drivers
can always enforce value ranges.

> As far as I can tell the Thermal subsystem would require a new governor for
> the behavior i want to archive and more importantly, a way to restrict which
> governors userspace can select.
> 
> As to why I don't want grant userspace full control: The firmware is
> perfectly fine with accepting potentially mainboard frying settings (as
> mentioned in the cover letter) and the lowest level I can write code for is
> the kernel driver. So that's the location I need to prevent this.
> 

It is ok for the kernel to accept and enforce _limits_ (such as lower and upper
ranges for temperatures) when they are written. That is not what the code here
does.

> Also hwmon is not purely a hardware monitoring, it also allows writing
> fanspeeds. Or did I miss something and this shouldn't actually be used?
> 

If doesn't actively control fan speeds, though. It just tells the firmware what
the limits or target values are.

> > 
> > Personally I think this is way too complicated. It would make much more sense
> > to assume a reasonable maximum (say, 16) and use fixed size arrays to access
> > the data. The is_visible function can then simply return 0 for larger channel
> > values if the total number of fans is less than the ones configured in the
> > channel information.
> Didn't know it was possible to filter extra entries out completely with the
> is_visible function, thanks for the tip.
> > 
> > Also, as already mentioned, there is no range check of fan_count. This will
> > cause some oddities if the system ever claims to have 256+ fans.
> Will not happen, but i guess a singular additional if in the init doesn't
> hurt, i can add it.

You are making the assumption that the firmware always provides correct
values.

I fully agree that repeated range checks for in-kernel API functions are
useless. However, values should still be checked when a value enters
the kernel, either via userspace or via hardware, even more so if that value
is used to determine, like here, the amount of memory allocated. Or, worse,
if the value is reported as 32-bit value and written into an 8-byte variable.

> > 
> > > +    *hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
> > > +                               "tuxedo_nbxx_acpi_tuxi",
> > > +                               driver_data, &hwminfo,
> > > +                               NULL);
> > > +    if (PTR_ERR_OR_ZERO(*hwmdev))
> > > +        return PTR_ERR_OR_ZERO(*hwmdev);
> > > +
> > Why not just return hwmdev ?
> because if hwmon is NULL it is still an error, i have to look again at what
> actually is returned by PTR_ERR_OR_ZERO on zero.

That seems a bit philosophical. The caller would have to check for
PTR_ERR_OR_ZERO() instead of checking for < 0.

On a side note, the code now returns 0 if devm_hwmon_device_register_with_info()
returned NULL.  devm_hwmon_device_register_with_info() never returns NULL,
so that doesn't make a difference in practice, but, still, this should
at least use PTR_ERR().

Guenter

Werner Sembach Feb. 6, 2025, 10:55 p.m. UTC | #6

Am 06.02.25 um 19:57 schrieb Guenter Roeck:
> On Thu, Feb 06, 2025 at 10:28:01AM +0100, Werner Sembach wrote:
>
> [ ... ]
>
>>>> +        temp = retval * 100 - 272000;
>>>> +
>>>> +        for (j = 0; temp_levels[j].temp; ++j) {
>>>> +            temp_low = j == 0 ? -272000 : temp_levels[j-1].temp;
>>>> +            temp_high = temp_levels[j].temp;
>>>> +            if (driver_data->temp_level[i] > j)
>>>> +                temp_high -= 2000; // hysteresis
>>>> +
>>>> +            if (temp >= temp_low && temp < temp_high)
>>>> +                driver_data->temp_level[i] = j;
>>>> +        }
>>>> +        if (temp >= temp_high)
>>>> +            driver_data->temp_level[i] = j;
>>>> +
>>>> +        temp_level = driver_data->temp_level[i];
>>>> +        min_speed = temp_level == 0 ?
>>>> +            0 : temp_levels[temp_level-1].min_speed;
>>>> +        curr_speed = driver_data->curr_speed[i];
>>>> +        want_speed = driver_data->want_speed[i];
>>>> +
>>>> +        if (want_speed < min_speed) {
>>>> +            if (curr_speed < min_speed)
>>>> +                write_speed(dev, i, min_speed);
>>>> +        } else if (curr_speed != want_speed)
>>>> +            write_speed(dev, i, want_speed);
>>>> +    }
>>>> +
>>>> +    schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>>>> +}
>>> This is not expected functionality of a hardware monitoring driver.
>>> Hardware monmitoring drivers should not replicate userspace or
>>> thermal subsystem functionality.
>>>
>>> This would be unacceptable in drivers/hwmon/.
>> Problem is: The thermal subsystem doesn't do this either as far as I can tell.
>>
>> See this: https://lore.kernel.org/all/[e-mail]/
>> and this: https://lore.kernel.org/all/[e-mail]/
>> thread.
>>
>> The short version is: The Thermal subsystem always allows userspace to
>> select the "userspace" governor which has no way for the kernel to enforce a
>> minimum speed.
>>
> You can specify thermal parameters / limits using devicetree. Also, drivers
> can always enforce value ranges.

Sorry for my noob question: What do you mean with devicetree in x86 context?

I only want to enforce a value range at a certain temperature, if the 
device is cool, the fan can be turned off for example.

>
>> As far as I can tell the Thermal subsystem would require a new governor for
>> the behavior i want to archive and more importantly, a way to restrict which
>> governors userspace can select.
>>
>> As to why I don't want grant userspace full control: The firmware is
>> perfectly fine with accepting potentially mainboard frying settings (as
>> mentioned in the cover letter) and the lowest level I can write code for is
>> the kernel driver. So that's the location I need to prevent this.
>>
> It is ok for the kernel to accept and enforce _limits_ (such as lower and upper
> ranges for temperatures) when they are written. That is not what the code here
> does.

It conditionally enforces a minimum fanspeed.

So is the problem that hwmon drivers are only allowed to enforce 
unconditional limits?

>
>> Also hwmon is not purely a hardware monitoring, it also allows writing
>> fanspeeds. Or did I miss something and this shouldn't actually be used?
>>
> If doesn't actively control fan speeds, though. It just tells the firmware what
> the limits or target values are.
What is the difference if it tells the firmware a target fanspeed, which 
can be ignored by it, or a driver a target fanspeed, which can be 
ignored by it?
>
>>> Personally I think this is way too complicated. It would make much more sense
>>> to assume a reasonable maximum (say, 16) and use fixed size arrays to access
>>> the data. The is_visible function can then simply return 0 for larger channel
>>> values if the total number of fans is less than the ones configured in the
>>> channel information.
>> Didn't know it was possible to filter extra entries out completely with the
>> is_visible function, thanks for the tip.
>>> Also, as already mentioned, there is no range check of fan_count. This will
>>> cause some oddities if the system ever claims to have 256+ fans.
>> Will not happen, but i guess a singular additional if in the init doesn't
>> hurt, i can add it.
> You are making the assumption that the firmware always provides correct
> values.
>
> I fully agree that repeated range checks for in-kernel API functions are
> useless. However, values should still be checked when a value enters
> the kernel, either via userspace or via hardware, even more so if that value
> is used to determine, like here, the amount of memory allocated. Or, worse,
> if the value is reported as 32-bit value and written into an 8-byte variable.
ok
>
>>>> +    *hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
>>>> +                               "tuxedo_nbxx_acpi_tuxi",
>>>> +                               driver_data, &hwminfo,
>>>> +                               NULL);
>>>> +    if (PTR_ERR_OR_ZERO(*hwmdev))
>>>> +        return PTR_ERR_OR_ZERO(*hwmdev);
>>>> +
>>> Why not just return hwmdev ?
>> because if hwmon is NULL it is still an error, i have to look again at what
>> actually is returned by PTR_ERR_OR_ZERO on zero.
> That seems a bit philosophical. The caller would have to check for
> PTR_ERR_OR_ZERO() instead of checking for < 0.
>
> On a side note, the code now returns 0 if devm_hwmon_device_register_with_info()
> returned NULL.  devm_hwmon_device_register_with_info() never returns NULL,
> so that doesn't make a difference in practice, but, still, this should
> at least use PTR_ERR().
ok
>
> Guenter

Ilpo Järvinen Feb. 7, 2025, 11:53 a.m. UTC | #7

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 <[e-mail]>


> > > +	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.

> > 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 :-)).

But I see Guenther was against some parts of this so please don't take my 
style related comments as overruling his objections.


> > > 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 [e-mail]
> > > + */
> > > +
> > > +#include <linux/acpi.h>

Btw just noticed, #include at least for kcalloc/kfree is missing.

> > > +
> > > +#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.


> > > + * 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 ;-).

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).

Werner Sembach Feb. 7, 2025, 6:44 p.m. UTC | #8

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 <[e-mail]>
>
>>>> +	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 [e-mail]
>>>> + */
>>>> +
>>>> +#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).
>

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 0fa7c5728f1e6..2d3fe9de4e9cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23916,6 +23916,12 @@  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
 F:	tools/power/x86/turbostat/
 F:	tools/testing/selftests/turbostat/
 
+TUXEDO DRIVERS
+M:	Werner Sembach <[e-mail]>
+L:	[e-mail]
+S:	Supported
+F:	drivers/platform/x86/tuxedo/
+
 TW5864 VIDEO4LINUX DRIVER
 M:	Bluecherry Maintainers <[e-mail]>
 M:	Andrey Utkin <[e-mail]>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0258dd879d64b..9b78a1255c08e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1199,3 +1199,5 @@  config P2SB
 	  The main purpose of this library is to unhide P2SB device in case
 	  firmware kept it hidden on some platforms in order to access devices
 	  behind it.
+
+source "drivers/platform/x86/tuxedo/Kconfig"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e1b1429470674..1562dcd7ad9a5 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -153,3 +153,6 @@  obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
 
 # SEL
 obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
+
+# TUXEDO
+obj-y					+= tuxedo/
diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
new file mode 100644
index 0000000000000..0de6c7871db95
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/Kbuild
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+obj-y	+= nbxx/
diff --git a/drivers/platform/x86/tuxedo/Kconfig b/drivers/platform/x86/tuxedo/Kconfig
new file mode 100644
index 0000000000000..82df7419cf29d
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/Kconfig
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+source "drivers/platform/x86/tuxedo/nb04/Kconfig"
diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
new file mode 100644
index 0000000000000..db8def8ffe8f6
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
@@ -0,0 +1,9 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+tuxedo_nbxx_acpi_tuxi-y			:= acpi_tuxi_init.o
+tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_util.o
+tuxedo_nbxx_acpi_tuxi-y			+= acpi_tuxi_hwmon.o
+obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI)	+= tuxedo_nbxx_acpi_tuxi.o
diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
new file mode 100644
index 0000000000000..827c65c410fb2
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
@@ -0,0 +1,13 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+config TUXEDO_NBXX_ACPI_TUXI
+	tristate "TUXEDO NBxx ACPI TUXI Platform Driver"
+	help
+	  This driver implements the ACPI TUXI device found on some TUXEDO
+	  Notebooks. Currently this consists only of the TFAN subdevice which is
+	  implemented via hwmon.
+
+	  When compiled as a module it will be called tuxedo_nbxx_acpi_tuxi
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
new file mode 100644
index 0000000000000..8ebb2bfc19e20
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.c
@@ -0,0 +1,421 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024-2025 Werner Sembach [e-mail]
+ */
+
+#include <linux/array_size.h>
+#include <linux/hwmon.h>
+#include <linux/workqueue.h>
+
+#include "acpi_tuxi_util.h"
+
+#include "acpi_tuxi_hwmon.h"
+
+struct tuxi_hwmon_driver_data_t {
+	struct delayed_work work;
+	struct device *hwmdev;
+	u8 fan_count;
+	const char **fan_types;
+	u8 *temp_level;
+	u8 *curr_speed;
+	u8 *want_speed;
+	u8 pwm_enabled;
+};
+
+struct tuxi_temp_high_config_t {
+	long temp;
+	u8 min_speed;
+};
+
+#define TUXI_SAFEGUARD_PERIOD 1000
+
+#define TUXI_PWM_FAN_ON_MIN_SPEED 0x40 // ~25%
+
+static struct tuxi_temp_high_config_t temp_levels[] = {
+	{  80000, 0x4d }, // ~30%
+	{  90000, 0x66 }, // ~40%
+	{  95000, 0x99 }, // ~60%
+	{ 100000, 0xff }, // 100%
+	{ }
+};
+
+static umode_t hwm_is_visible(const void * __always_unused data,
+			      enum hwmon_sensor_types type,
+			      u32 __always_unused attr,
+			      int __always_unused channel)
+{
+	switch (type) {
+	case hwmon_fan:
+		return 0444;
+	case hwmon_pwm:
+		return 0644;
+	case hwmon_temp:
+		return 0444;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+		    int channel, long *val)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
+	struct acpi_device *pdev = to_acpi_device(dev->parent);
+	int ret;
+
+	unsigned long long params[2];
+	unsigned long long retval;
+
+	switch (type) {
+	case hwmon_fan:
+		params[0] = channel;
+		ret = tuxi_tfan_method(pdev,
+				       TUXI_TFAN_METHOD_GET_FAN_RPM,
+				       params, 1, &retval);
+		*val = retval;
+		return ret;
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			if (driver_data->pwm_enabled) {
+				*val = driver_data->curr_speed[channel];
+				return 0;
+			}
+			params[0] = channel;
+			ret = tuxi_tfan_method(pdev,
+					       TUXI_TFAN_METHOD_GET_FAN_SPEED,
+					       params, 1, &retval);
+			*val = retval;
+			return ret;
+		case hwmon_pwm_enable:
+			*val = driver_data->pwm_enabled;
+			return ret;
+		}
+		break;
+	case hwmon_temp:
+		params[0] = channel;
+		ret = tuxi_tfan_method(pdev,
+				       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
+				       params, 1, &retval);
+		*val = retval * 100 - 272000;
+		return ret;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int hwm_read_string(struct device *dev,
+			   enum hwmon_sensor_types __always_unused type,
+			   u32 __always_unused attr, int channel,
+			   const char **str)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
+
+	*str = driver_data->fan_types[channel];
+
+	return 0;
+}
+
+static int write_speed(struct device *dev, int channel, long val)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
+	struct acpi_device *pdev = to_acpi_device(dev->parent);
+	int ret;
+
+	unsigned long long params[2];
+
+	params[0] = channel;
+
+	u8 temp_level, min_speed;
+
+	temp_level = driver_data->temp_level[channel];
+	if (temp_level) {
+		min_speed = temp_levels[temp_level].min_speed;
+		if (val < min_speed)
+			val = min_speed;
+	}
+
+	if (val < TUXI_PWM_FAN_ON_MIN_SPEED / 2)
+		params[1] = 0;
+	else if (val < TUXI_PWM_FAN_ON_MIN_SPEED)
+		params[1] = TUXI_PWM_FAN_ON_MIN_SPEED;
+	else
+		params[1] = val;
+
+	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_SPEED,
+			       params, 2, NULL);
+	if (ret)
+		return ret;
+
+	driver_data->curr_speed[channel] = val;
+
+	return 0;
+}
+
+static int hwm_write(struct device *dev,
+		     enum hwmon_sensor_types __always_unused type, u32 attr,
+		     int channel, long val)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
+	struct acpi_device *pdev = to_acpi_device(dev->parent);
+	unsigned int i;
+	int ret;
+
+	unsigned long long params[2];
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		if (driver_data->pwm_enabled) {
+			driver_data->want_speed[channel] = val;
+			return write_speed(dev, channel, val);
+		}
+
+		return 0;
+	case hwmon_pwm_enable:
+		params[0] = val;
+		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE,
+				       params, 1, NULL);
+		if (ret)
+			return ret;
+
+		driver_data->pwm_enabled = val;
+
+		/* Activating PWM sets speed to 0. Alternativ design decision
+		 * could be to keep the current value. This would require proper
+		 * setting of driver_data->curr_speed for example.
+		 */
+		if (val)
+			for (i = 0; i < driver_data->fan_count; ++i) {
+				ret = write_speed(dev, i, 0);
+				if (ret)
+					return ret;
+			}
+
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops hwmops = {
+	.is_visible = hwm_is_visible,
+	.read = hwm_read,
+	.read_string = hwm_read_string,
+	.write = hwm_write,
+};
+
+static struct hwmon_channel_info hwmcinfo_fan = {
+	.type = hwmon_fan,
+};
+
+static struct hwmon_channel_info hwmcinfo_pwm = {
+	.type = hwmon_pwm,
+};
+
+static struct hwmon_channel_info hwmcinfo_temp = {
+	.type = hwmon_temp,
+};
+
+static const struct hwmon_channel_info * const hwmcinfo[] = {
+	&hwmcinfo_fan,
+	&hwmcinfo_pwm,
+	&hwmcinfo_temp,
+	NULL
+};
+
+static const struct hwmon_chip_info hwminfo = {
+	.ops = &hwmops,
+	.info = hwmcinfo
+};
+
+static void tuxi_periodic_hw_safeguard(struct work_struct *work)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data;
+
+	driver_data = container_of(work, struct tuxi_hwmon_driver_data_t,
+				   work.work);
+
+	struct device *dev = driver_data->hwmdev;
+	struct acpi_device *pdev = to_acpi_device(dev->parent);
+	unsigned int i, j;
+
+	unsigned long long params[2];
+	unsigned long long retval;
+
+	long temp, temp_low, temp_high;
+	u8 temp_level, min_speed, curr_speed, want_speed;
+
+	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;
+			temp_high = temp_levels[j].temp;
+			if (driver_data->temp_level[i] > j)
+				temp_high -= 2000; // hysteresis
+
+			if (temp >= temp_low && temp < temp_high)
+				driver_data->temp_level[i] = j;
+		}
+		if (temp >= temp_high)
+			driver_data->temp_level[i] = j;
+
+		temp_level = driver_data->temp_level[i];
+		min_speed = temp_level == 0 ?
+			0 : temp_levels[temp_level-1].min_speed;
+		curr_speed = driver_data->curr_speed[i];
+		want_speed = driver_data->want_speed[i];
+
+		if (want_speed < min_speed) {
+			if (curr_speed < min_speed)
+				write_speed(dev, i, min_speed);
+		} else if (curr_speed != want_speed)
+			write_speed(dev, i, want_speed);
+	}
+
+	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
+}
+
+int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data;
+	int ret;
+
+	unsigned long long params[2];
+	unsigned long long retval;
+
+	u32 *hwmcinfo_fan_config, *hwmcinfo_pwm_config, *hwmcinfo_temp_config;
+
+	/* The first version of TUXI TFAN didn't have the Get Fan Temperature
+	 * method which is integral to this driver. So probe for existence and
+	 * abort otherwise.
+	 *
+	 * The Get Fan Speed method is also missing in that version, but was
+	 * added in the same version so it doesn't have to be probe separately.
+	 */
+	params[0] = 0;
+	ret = tuxi_tfan_method(pdev,
+			       TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
+			       params, 1, &retval);
+	if (ret)
+		return ret;
+
+	driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
+				   GFP_KERNEL);
+	if (!driver_data)
+		return -ENOMEM;
+
+	/* Loading this module sets the fan mode to auto. Alternative design
+	 * decision could be to keep the current value. This would require
+	 * proper initialization of driver_data->curr_speed for example.
+	 */
+	params[0] = 0;
+	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1,
+			       NULL);
+	if (ret)
+		return ret;
+
+	ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL, 0,
+			       &retval);
+	if (ret)
+		return ret;
+	driver_data->fan_count = retval;
+
+	driver_data->temp_level = devm_kcalloc(&pdev->dev,
+					       driver_data->fan_count,
+					       sizeof(*driver_data->temp_level),
+					       GFP_KERNEL);
+	if (driver_data->temp_level == NULL)
+		return -ENOMEM;
+	driver_data->curr_speed = devm_kcalloc(&pdev->dev,
+					       driver_data->fan_count,
+					       sizeof(*driver_data->curr_speed),
+					       GFP_KERNEL);
+	if (driver_data->curr_speed == NULL)
+		return -ENOMEM;
+	driver_data->want_speed = devm_kcalloc(&pdev->dev,
+					       driver_data->fan_count,
+					       sizeof(*driver_data->want_speed),
+					       GFP_KERNEL);
+	if (driver_data->want_speed == NULL)
+		return -ENOMEM;
+	driver_data->fan_types = devm_kcalloc(&pdev->dev,
+					      driver_data->fan_count,
+					      sizeof(*driver_data->fan_types),
+					      GFP_KERNEL);
+	if (driver_data->fan_types == NULL)
+		return -ENOMEM;
+
+	hwmcinfo_fan_config = devm_kcalloc(&pdev->dev,
+					   driver_data->fan_count + 1,
+					   sizeof(*hwmcinfo_fan_config),
+					   GFP_KERNEL);
+	if (hwmcinfo_fan_config == NULL)
+		return -ENOMEM;
+	hwmcinfo_pwm_config = devm_kcalloc(&pdev->dev,
+					   driver_data->fan_count + 1,
+					   sizeof(*hwmcinfo_pwm_config),
+					   GFP_KERNEL);
+	if (hwmcinfo_pwm_config == NULL)
+		return -ENOMEM;
+	hwmcinfo_temp_config = devm_kcalloc(&pdev->dev,
+					    driver_data->fan_count + 1,
+					    sizeof(*hwmcinfo_temp_config),
+					    GFP_KERNEL);
+	if (hwmcinfo_temp_config == NULL)
+		return -ENOMEM;
+
+	for (unsigned long long i = 0; i < driver_data->fan_count; ++i) {
+		hwmcinfo_fan_config[i] = HWMON_F_INPUT | HWMON_F_LABEL;
+		hwmcinfo_pwm_config[i] = HWMON_PWM_INPUT | HWMON_PWM_ENABLE;
+		hwmcinfo_temp_config[i] = HWMON_T_INPUT | HWMON_T_LABEL;
+
+		params[0] = i;
+		ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TYPE,
+				       params, 1, &retval);
+		if (ret)
+			return ret;
+		else if (retval >= ARRAY_SIZE(tuxi_fan_type_labels))
+			return -EOPNOTSUPP;
+		driver_data->fan_types[i] = tuxi_fan_type_labels[retval];
+	}
+	hwmcinfo_fan.config = hwmcinfo_fan_config;
+	hwmcinfo_pwm.config = hwmcinfo_pwm_config;
+	hwmcinfo_temp.config = hwmcinfo_temp_config;
+
+	*hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
+						       "tuxedo_nbxx_acpi_tuxi",
+						       driver_data, &hwminfo,
+						       NULL);
+	if (PTR_ERR_OR_ZERO(*hwmdev))
+		return PTR_ERR_OR_ZERO(*hwmdev);
+
+	driver_data->hwmdev = *hwmdev;
+
+	INIT_DELAYED_WORK(&driver_data->work, tuxi_periodic_hw_safeguard);
+	schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
+
+	return 0;
+}
+EXPORT_SYMBOL(tuxi_hwmon_add_devices);
+
+void tuxi_hwmon_remove_devices(struct device *hwmdev)
+{
+	struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(hwmdev);
+	struct acpi_device *pdev = to_acpi_device(hwmdev->parent);
+
+	unsigned long long params[2];
+
+	cancel_delayed_work_sync(&driver_data->work);
+
+	params[0] = 0;
+	tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1, NULL);
+}
+EXPORT_SYMBOL(tuxi_hwmon_remove_devices);
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
new file mode 100644
index 0000000000000..248730fab8574
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_hwmon.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024-2025 Werner Sembach [e-mail]
+ */
+
+#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
+#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
+
+#include <linux/acpi.h>
+
+int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev);
+void tuxi_hwmon_remove_devices(struct device *hwmdev);
+
+#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_HWMON_H__
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
new file mode 100644
index 0000000000000..5e8edb458aba2
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.c
@@ -0,0 +1,60 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024-2025 Werner Sembach [e-mail]
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+
+#include "acpi_tuxi_hwmon.h"
+
+#include "acpi_tuxi_init.h"
+
+static const struct acpi_device_id acpi_device_ids[] = {
+	{"TUXI0000", 0},
+	{"", 0}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
+
+static int add(struct acpi_device *device)
+{
+	struct tuxi_driver_data_t *driver_data;
+	acpi_status status;
+
+	driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
+				   GFP_KERNEL);
+	if (!driver_data)
+		return -ENOMEM;
+
+	// Find subdevices
+	status = acpi_get_handle(device->handle, "TFAN",
+				 &driver_data->tfan_handle);
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	dev_set_drvdata(&device->dev, driver_data);
+
+	return tuxi_hwmon_add_devices(device, &driver_data->hwmdev);
+}
+
+static void remove(struct acpi_device *device)
+{
+	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
+
+	tuxi_hwmon_remove_devices(driver_data->hwmdev);
+}
+
+static struct acpi_driver acpi_driver = {
+	.name = "tuxedo_nbxx_acpi_tuxi",
+	.ids = acpi_device_ids,
+	.ops = {
+		.add = add,
+		.remove = remove,
+	},
+};
+
+module_acpi_driver(acpi_driver);
+
+MODULE_DESCRIPTION("TUXEDO TUXI");
+MODULE_AUTHOR("Werner Sembach <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
new file mode 100644
index 0000000000000..33388332a2328
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_init.h
@@ -0,0 +1,16 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024-2025 Werner Sembach [e-mail]
+ */
+
+#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
+#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
+
+#include <linux/acpi.h>
+
+struct tuxi_driver_data_t {
+	acpi_handle tfan_handle;
+	struct device *hwmdev;
+};
+
+#endif // __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_INIT_H__
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 [e-mail]
+ */
+
+#include <linux/acpi.h>
+
+#include "acpi_tuxi_init.h"
+
+#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)
+{
+	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) {
+			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);
+	} else {
+		status = acpi_evaluate_integer(handle, pathname, NULL, &data);
+	}
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	if (retval)
+		*retval = data;
+
+	return 0;
+}
+
+int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
+		     unsigned long long *params, u32 pcount,
+		     unsigned long long *retval)
+{
+	struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
+
+	return __acpi_eval_intarray_in_int_out(driver_data->tfan_handle, method,
+					       params, pcount, retval);
+}
+EXPORT_SYMBOL(tuxi_tfan_method);
diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
new file mode 100644
index 0000000000000..670fe02249d06
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi_util.h
@@ -0,0 +1,84 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024-2025 Werner Sembach [e-mail]
+ */
+
+#ifndef __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
+#define __PLATFORM_X86_TUXEDO_NBXX_ACPI_TUXI_UTIL_H__
+
+#include <linux/acpi.h>
+
+/*
+ * Set fan speed target
+ *
+ * Set a specific fan speed (needs manual mode)
+ *
+ * Arg0: Fan index
+ * Arg1: Fan speed as a fraction of maximum speed (0-255)
+ */
+#define TUXI_TFAN_METHOD_SET_FAN_SPEED		"SSPD"
+
+/*
+ * Get fan speed target
+ *
+ * Arg0: Fan index
+ * Returns: Current fan speed target a fraction of maximum speed (0-255)
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_SPEED		"GSPD"
+
+/*
+ * Get fans count
+ *
+ * Returns: Number of individually controllable fans
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_COUNT		"GCNT"
+
+/*
+ * Set fans mode
+ *
+ * Arg0: 0 = auto, 1 = manual
+ */
+#define TUXI_TFAN_METHOD_SET_FAN_MODE		"SMOD"
+
+/*
+ * Get fans mode
+ *
+ * Returns: 0 = auto, 1 = manual
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_MODE		"GMOD"
+
+/*
+ * Get fan type/what the fan is pointed at
+ *
+ * Arg0: Fan index
+ * Returns: 0 = general, 1 = CPU, 2 = GPU
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_TYPE		"GTYP"
+
+static const char * const tuxi_fan_type_labels[] = {
+	"general",
+	"cpu",
+	"gpu"
+};
+
+/*
+ * Get fan temperature/temperature of what the fan is pointed at
+ *
+ * Arg0: Fan index
+ * Returns: Temperature sensor value in 10ths of degrees kelvin
+ */
+#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE	"GTMP"
+
+/*
+ * Get actual fan speed in RPM
+ *
+ * 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__