[PATCH RESEND v7 0/2] hwmon: add GPD devices sensor driver

[PATCH RESEND v7 0/2] hwmon: add GPD devices sensor driver

[Cryolitia PukNgae via B4 Relay]

Sensors driver for GPD Handhelds that expose fan reading and control
via hwmon sysfs.

Shenzhen GPD Technology Co., Ltd. manufactures a series of handheld
devices. This driver implements these functions through x86 port-mapped
IO.

Tested-by: Marcin Strągowski <marcin@stragowski.com>
Tested-by: someone5678 <someone5678.dev@gmail.com>
Tested-by: Justin Weiss <justin@justinweiss.com>
Tested-by: Antheas Kapenekakis <lkml@antheas.dev>
Tested-by: command_block <mtf@ik.me>
Tested-by: derjohn <himself@derjohn.de>
Tested-by: Crashdummyy <crashdummy1337@proton.me>

Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>

---
Additional explanation: Based on the concerns in the previous version
of the discussion about placing the driver in the x86 subsystem or the
hwmon subsystem, I currently do not see any intention from GPD to
integrate battery management into EC, and would prefer to keep the
driver in the hwmon subsystem until the hardware manufacturers actually
make something practical.

---
Changes in v7:
- Add support for GPD Duo
- Change email from cryolitia@gmail.com to cryolitia@uniontech.com
- Link to v6: https://lore.kernel.org/r/CAGwozwG13swYjCB6_Wm2h8a2CdHxam+2y=g1m42pynkKqqdDLg@mail.gmail.com

Changes in v6:
- fix: nullptr and label followed by a declaration
- cleanup: clean up code and rename some function
- format code
- dmi: add 2025 new GPD devices
- Link to v5: https://lore.kernel.org/r/20250211-gpd_fan-v5-0-608f4255f0e1@gmail.com

Changes in v5:
- Rebase on kernel 6.13
- Remove all value-cache related code
- Clean up code
- Link to v4: https://lore.kernel.org/r/20240718-gpd_fan-v4-0-116e5431a9fe@gmail.com

Changes in v4:
- Apply suggest by Krzysztof Kozlowski, thanks!
- Link to v3: https://lore.kernel.org/r/20240717-gpd_fan-v3-0-8d7efb1263b7@gmail.com

Changes in v3:
- Re-arrange code, thanks to Krzysztof Kozlowski, Guenter Roeck, Yao Zi!
- Link to v2: https://lore.kernel.org/r/20240717-gpd_fan-v2-0-f7b7e6b9f21b@gmail.com

Changes in v2:
- Improved documentation, thanks to Randy Dunlap!
- Link to v1: https://lore.kernel.org/r/20240716-gpd_fan-v1-0-34051dd71a06@gmail.com

---
Cryolitia PukNgae (2):
      hwmon: add GPD devices sensor driver
      hwmon: document: add gpd-fan

 Documentation/hwmon/gpd-fan.rst |  71 ++++
 Documentation/hwmon/index.rst   |   1 +
 MAINTAINERS                     |   7 +
 drivers/hwmon/Kconfig           |  10 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/gpd-fan.c         | 753 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 843 insertions(+)
---
base-commit: b19a97d57c15643494ac8bfaaa35e3ee472d41da
change-id: 20240716-gpd_fan-57f30923c884

Best regards,
-- 
Cryolitia PukNgae <cryolitia@uniontech.com>

[PATCH RESEND v7 1/2] hwmon: add GPD devices sensor driver

[Cryolitia PukNgae via B4 Relay]

From: Cryolitia PukNgae <cryolitia@uniontech.com>

Sensors driver for GPD Handhelds that expose fan reading and control via
hwmon sysfs.

Shenzhen GPD Technology Co., Ltd. manufactures a series of handheld
devices. This driver implements these functions through x86 port-mapped
IO.

Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
---
 MAINTAINERS             |   6 +
 drivers/hwmon/Kconfig   |  10 +
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/gpd-fan.c | 753 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 770 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index daf520a13bdf6a991c0160a96620f40308c29ee0..1deb9b817a37998828b6773d3dc8237c982d4bf9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10410,6 +10410,12 @@ F:	drivers/phy/samsung/phy-gs101-ufs.c
 F:	include/dt-bindings/clock/google,gs101.h
 K:	[gG]oogle.?[tT]ensor
 
+GPD FAN DRIVER
+M:	Cryolitia PukNgae <cryolitia@uniontech.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	drivers/hwmon/gpd-fan.c
+
 GPD POCKET FAN DRIVER
 M:	Hans de Goede <hansg@kernel.org>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9d28fcf7cd2a6f9e2f54694a717bd85ff4047b46..a552a5ced64d0fee2c80a5399ce9d1f0dbd7d763 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -769,6 +769,16 @@ config SENSORS_GL520SM
 	  This driver can also be built as a module. If so, the module
 	  will be called gl520sm.
 
+config SENSORS_GPD
+	tristate "GPD handhelds"
+	depends on X86
+	help
+	  If you say yes here you get support for fan readings and
+	  control over GPD handheld devices.
+
+	  Can also be built as a module. In that case it will be
+	  called gpd-fan.
+
 config SENSORS_G760A
 	tristate "GMT G760A"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index cd8bc4752b4dbf015c6eb46157626f4e8f87dfae..051981eb8a5089608e9eb351a1d5857805c728c8 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_SENSORS_GIGABYTE_WATERFORCE) += gigabyte_waterforce.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
+obj-$(CONFIG_SENSORS_GPD)	+= gpd-fan.o
 obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
 obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
 obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
new file mode 100644
index 0000000000000000000000000000000000000000..11409723cb2989569a33c4c0f1beceac073cb6e4
--- /dev/null
+++ b/drivers/hwmon/gpd-fan.c
@@ -0,0 +1,753 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/* Platform driver for GPD devices that expose fan control via hwmon sysfs.
+ *
+ * Fan control is provided via pwm interface in the range [0-255].
+ * Each model has a different range in the EC, the written value is scaled to
+ * accommodate for that.
+ *
+ * Based on this repo:
+ * https://github.com/Cryolitia/gpd-fan-driver
+ *
+ * Copyright (c) 2024 Cryolitia PukNgae
+ */
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/hwmon.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DRIVER_NAME "gpdfan"
+#define GPD_PWM_CTR_OFFSET 0x1841
+
+static char *gpd_fan_board = "";
+module_param(gpd_fan_board, charp, 0444);
+
+// EC read/write locker
+// Should never access EC at the same time, otherwise system down.
+static DEFINE_MUTEX(gpd_fan_lock);
+
+enum gpd_board {
+	win_mini,
+	win4_6800u,
+	win_max_2,
+	duo,
+};
+
+enum FAN_PWM_ENABLE {
+	DISABLE		= 0,
+	MANUAL		= 1,
+	AUTOMATIC	= 2,
+};
+
+static struct {
+	enum FAN_PWM_ENABLE pwm_enable;
+	u8 pwm_value;
+
+	const struct gpd_fan_drvdata *drvdata;
+} gpd_driver_priv;
+
+struct gpd_fan_drvdata {
+	const char *board_name; /* Board name for module param comparison */
+	const enum gpd_board board;
+
+	const u8 addr_port;
+	const u8 data_port;
+	const u16 manual_control_enable;
+	const u16 rpm_read;
+	const u16 pwm_write;
+	const u16 pwm_max;
+};
+
+static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
+	.board_name		= "win_mini",
+	.board			= win_mini,
+
+	.addr_port		= 0x4E,
+	.data_port		= 0x4F,
+	.manual_control_enable	= 0x047A,
+	.rpm_read		= 0x0478,
+	.pwm_write		= 0x047A,
+	.pwm_max		= 244,
+};
+
+static struct gpd_fan_drvdata gpd_duo_drvdata = {
+	.board_name		= "duo",
+	.board			= duo,
+
+	.addr_port		= 0x4E,
+	.data_port		= 0x4F,
+	.manual_control_enable	= 0x047A,
+	.rpm_read		= 0x0478,
+	.pwm_write		= 0x047A,
+	.pwm_max		= 244,
+};
+
+static struct gpd_fan_drvdata gpd_win4_drvdata = {
+	.board_name		= "win4",
+	.board			= win4_6800u,
+
+	.addr_port		= 0x2E,
+	.data_port		= 0x2F,
+	.manual_control_enable	= 0xC311,
+	.rpm_read		= 0xC880,
+	.pwm_write		= 0xC311,
+	.pwm_max		= 127,
+};
+
+static struct gpd_fan_drvdata gpd_wm2_drvdata = {
+	.board_name		= "wm2",
+	.board			= win_max_2,
+
+	.addr_port		= 0x4E,
+	.data_port		= 0x4F,
+	.manual_control_enable	= 0x0275,
+	.rpm_read		= 0x0218,
+	.pwm_write		= 0x1809,
+	.pwm_max		= 184,
+};
+
+static const struct dmi_system_id dmi_table[] = {
+	{
+		// GPD Win Mini
+		// GPD Win Mini with AMD Ryzen 8840U
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-01")
+		},
+		.driver_data = &gpd_win_mini_drvdata,
+	},
+	{
+		// GPD Win Mini
+		// GPD Win Mini with AMD Ryzen HX370
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-02")
+		},
+		.driver_data = &gpd_win_mini_drvdata,
+	},
+	{
+		// GPD Win Mini
+		// GPD Win Mini with AMD Ryzen HX370
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-02-L")
+		},
+		.driver_data = &gpd_win_mini_drvdata,
+	},
+	{
+		// GPD Win 4 with AMD Ryzen 6800U
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
+			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
+		},
+		.driver_data = &gpd_win4_drvdata,
+	},
+	{
+		// GPD Win 4 with Ryzen 7840U
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
+			DMI_MATCH(DMI_BOARD_VERSION, "Ver. 1.0"),
+		},
+		// Since 7840U, win4 uses the same drvdata as wm2
+		.driver_data = &gpd_wm2_drvdata,
+	},
+	{
+		// GPD Win 4 with Ryzen 7840U (another)
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
+			DMI_MATCH(DMI_BOARD_VERSION, "Ver.1.0"),
+		},
+		.driver_data = &gpd_wm2_drvdata,
+	},
+	{
+		// GPD Win Max 2 with Ryzen 6800U
+		// GPD Win Max 2 2023 with Ryzen 7840U
+		// GPD Win Max 2 2024 with Ryzen 8840U
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1619-04"),
+		},
+		.driver_data = &gpd_wm2_drvdata,
+	},
+	{
+		// GPD Win Max 2 with AMD Ryzen HX370
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1619-05"),
+		},
+		.driver_data = &gpd_wm2_drvdata,
+	},
+	{
+		// GPD Duo
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1622-01"),
+		},
+		.driver_data = &gpd_duo_drvdata,
+	},
+	{
+		// GPD Duo (another)
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1622-01-L"),
+		},
+		.driver_data = &gpd_duo_drvdata,
+	},
+	{
+		// GPD Pocket 4
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1628-04"),
+		},
+		.driver_data = &gpd_win_mini_drvdata,
+	},
+	{
+		// GPD Pocket 4 (another)
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G1628-04-L"),
+		},
+		.driver_data = &gpd_win_mini_drvdata,
+	},
+	{}
+};
+
+static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
+	&gpd_win_mini_drvdata, &gpd_win4_drvdata, &gpd_wm2_drvdata, NULL
+};
+
+/* Helper functions to handle EC read/write */
+static int gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset,
+			  u8 *val)
+{
+	int ret;
+	u16 addr_port = drvdata->addr_port;
+	u16 data_port = drvdata->data_port;
+
+	ret = mutex_lock_interruptible(&gpd_fan_lock);
+
+	if (ret)
+		return ret;
+
+	outb(0x2E, addr_port);
+	outb(0x11, data_port);
+	outb(0x2F, addr_port);
+	outb((u8)((offset >> 8) & 0xFF), data_port);
+
+	outb(0x2E, addr_port);
+	outb(0x10, data_port);
+	outb(0x2F, addr_port);
+	outb((u8)(offset & 0xFF), data_port);
+
+	outb(0x2E, addr_port);
+	outb(0x12, data_port);
+	outb(0x2F, addr_port);
+	*val = inb(data_port);
+
+	mutex_unlock(&gpd_fan_lock);
+	return 0;
+}
+
+static int gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset,
+			   u8 value)
+{
+	int ret;
+	u16 addr_port = drvdata->addr_port;
+	u16 data_port = drvdata->data_port;
+
+	ret = mutex_lock_interruptible(&gpd_fan_lock);
+
+	if (ret)
+		return ret;
+
+	outb(0x2E, addr_port);
+	outb(0x11, data_port);
+	outb(0x2F, addr_port);
+	outb((u8)((offset >> 8) & 0xFF), data_port);
+
+	outb(0x2E, addr_port);
+	outb(0x10, data_port);
+	outb(0x2F, addr_port);
+	outb((u8)(offset & 0xFF), data_port);
+
+	outb(0x2E, addr_port);
+	outb(0x12, data_port);
+	outb(0x2F, addr_port);
+	outb(value, data_port);
+
+	mutex_unlock(&gpd_fan_lock);
+	return 0;
+}
+
+static int gpd_generic_read_rpm(void)
+{
+	u8 high, low;
+	int ret;
+	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+
+	ret = gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
+	if (ret)
+		return ret;
+
+	ret = gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
+	if (ret)
+		return ret;
+
+	return (u16)high << 8 | low;
+}
+
+static int gpd_win4_read_rpm(void)
+{
+	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+	u8 pwm_ctr_reg;
+	int ret;
+
+	gpd_ecram_read(drvdata, GPD_PWM_CTR_OFFSET, &pwm_ctr_reg);
+
+	if (pwm_ctr_reg != 0x7F)
+		gpd_ecram_write(drvdata, GPD_PWM_CTR_OFFSET, 0x7F);
+
+	ret = gpd_generic_read_rpm();
+
+	if (ret < 0)
+		return ret;
+
+	if (ret == 0) {
+		// re-init EC
+		u8 chip_id;
+
+		gpd_ecram_read(drvdata, 0x2000, &chip_id);
+		if (chip_id == 0x55) {
+			u8 chip_ver;
+
+			if (gpd_ecram_read(drvdata, 0x1060, &chip_ver))
+				gpd_ecram_write(drvdata, 0x1060,
+						chip_ver | 0x80);
+		}
+	}
+
+	return ret;
+}
+
+static int gpd_wm2_read_rpm(void)
+{
+	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+
+	for (u16 pwm_ctr_offset = GPD_PWM_CTR_OFFSET;
+	     pwm_ctr_offset <= GPD_PWM_CTR_OFFSET + 2; pwm_ctr_offset++) {
+		u8 PWMCTR;
+
+		gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
+
+		if (PWMCTR != 0xB8)
+			gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
+	}
+
+	return gpd_generic_read_rpm();
+}
+
+// Read value for fan1_input
+static int gpd_read_rpm(void)
+{
+	switch (gpd_driver_priv.drvdata->board) {
+	case win_mini:
+	case duo:
+		return gpd_generic_read_rpm();
+	case win4_6800u:
+		return gpd_win4_read_rpm();
+	case win_max_2:
+		return gpd_wm2_read_rpm();
+	}
+
+	return 0;
+}
+
+static int gpd_wm2_read_pwm(void)
+{
+	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+	u8 var;
+	int ret = gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
+
+	if (ret < 0)
+		return ret;
+
+	return var * 255 / drvdata->pwm_max;
+}
+
+// Read value for pwm1
+static int gpd_read_pwm(void)
+{
+	switch (gpd_driver_priv.drvdata->board) {
+	case win_mini:
+	case duo:
+	case win4_6800u:
+		return gpd_driver_priv.pwm_value;
+	case win_max_2:
+		return gpd_wm2_read_pwm();
+	}
+	return 0;
+}
+
+static int gpd_generic_write_pwm(u8 val)
+{
+	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+	u8 pwm_reg;
+
+	// PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
+	pwm_reg = val * (drvdata->pwm_max - 1) / 255 + 1;
+	return gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
+}
+
+static int gpd_win_mini_write_pwm(u8 val)
+{
+	if (gpd_driver_priv.pwm_enable == MANUAL)
+		return gpd_generic_write_pwm(val);
+	else
+		return -EPERM;
+}
+
+static int gpd_duo_write_pwm_twice(u8 val)
+{
+	int ret;
+
+	ret = gpd_generic_write_pwm(val);
+
+	if (ret)
+		return ret;
+
+	return gpd_generic_write_pwm(val+1);
+}
+
+static int gpd_duo_write_pwm(u8 val)
+{
+	if (gpd_driver_priv.pwm_enable == MANUAL)
+		return gpd_duo_write_pwm_twice(val);
+	else
+		return -EPERM;
+}
+
+static int gpd_wm2_write_pwm(u8 val)
+{
+	if (gpd_driver_priv.pwm_enable != DISABLE)
+		return gpd_generic_write_pwm(val);
+	else
+		return -EPERM;
+}
+
+// Write value for pwm1
+static int gpd_write_pwm(u8 val)
+{
+	switch (gpd_driver_priv.drvdata->board) {
+	case win_mini:
+		return gpd_win_mini_write_pwm(val);
+	case duo:
+		return gpd_duo_write_pwm(val);
+	case win4_6800u:
+		return gpd_generic_write_pwm(val);
+	case win_max_2:
+		return gpd_wm2_write_pwm(val);
+	}
+
+	return 0;
+}
+
+static int gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
+{
+	const struct gpd_fan_drvdata *drvdata;
+
+	switch (pwm_enable) {
+	case DISABLE:
+		return gpd_generic_write_pwm(255);
+	case MANUAL:
+		return gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
+	case AUTOMATIC:
+		drvdata = gpd_driver_priv.drvdata;
+		return gpd_ecram_write(drvdata, drvdata->pwm_write, 0);
+	}
+
+	return 0;
+}
+
+static int gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
+{
+	const struct gpd_fan_drvdata *drvdata;
+
+	switch (pwm_enable) {
+	case DISABLE:
+		return gpd_duo_write_pwm_twice(255);
+	case MANUAL:
+		return gpd_duo_write_pwm_twice(gpd_driver_priv.pwm_value);
+	case AUTOMATIC:
+		drvdata = gpd_driver_priv.drvdata;
+		return gpd_ecram_write(drvdata, drvdata->pwm_write, 0);
+	}
+
+	return 0;
+}
+
+static int gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
+{
+	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+	int ret;
+
+	switch (enable) {
+	case DISABLE: {
+		ret = gpd_generic_write_pwm(255);
+
+		if (ret)
+			return ret;
+
+		return gpd_ecram_write(drvdata, drvdata->manual_control_enable,
+				       1);
+	}
+	case MANUAL: {
+		ret = gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
+
+		if (ret)
+			return ret;
+
+		return gpd_ecram_write(drvdata, drvdata->manual_control_enable,
+				       1);
+	}
+	case AUTOMATIC: {
+		ret = gpd_ecram_write(drvdata, drvdata->manual_control_enable,
+				      0);
+
+		return ret;
+	}
+	}
+
+	return 0;
+}
+
+// Write value for pwm1_enable
+static int gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
+{
+	switch (gpd_driver_priv.drvdata->board) {
+	case win_mini:
+	case win4_6800u:
+		return gpd_win_mini_set_pwm_enable(enable);
+	case duo:
+		return gpd_duo_set_pwm_enable(enable);
+	case win_max_2:
+		return gpd_wm2_set_pwm_enable(enable);
+	}
+
+	return 0;
+}
+
+static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
+					enum hwmon_sensor_types type, u32 attr,
+					__always_unused int channel)
+{
+	if (type == hwmon_fan && attr == hwmon_fan_input) {
+		return 0444;
+	} else if (type == hwmon_pwm) {
+		switch (attr) {
+		case hwmon_pwm_enable:
+		case hwmon_pwm_input:
+			return 0644;
+		default:
+			return 0;
+		}
+	}
+	return 0;
+}
+
+static int gpd_fan_hwmon_read(__always_unused struct device *dev,
+			      enum hwmon_sensor_types type, u32 attr,
+			      __always_unused int channel, long *val)
+{
+	if (type == hwmon_fan) {
+		if (attr == hwmon_fan_input) {
+			int ret = gpd_read_rpm();
+
+			if (ret < 0)
+				return ret;
+
+			*val = ret;
+			return 0;
+		}
+		return -EOPNOTSUPP;
+	} else if (type == hwmon_pwm) {
+		int ret;
+
+		switch (attr) {
+		case hwmon_pwm_enable:
+			*val = gpd_driver_priv.pwm_enable;
+			return 0;
+		case hwmon_pwm_input:
+			ret = gpd_read_pwm();
+
+			if (ret < 0)
+				return ret;
+
+			*val = ret;
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int gpd_fan_hwmon_write(__always_unused struct device *dev,
+			       enum hwmon_sensor_types type, u32 attr,
+			       __always_unused int channel, long val)
+{
+	u8 var;
+
+	if (type == hwmon_pwm) {
+		switch (attr) {
+		case hwmon_pwm_enable:
+			if (!in_range(val, 0, 3))
+				return -EINVAL;
+
+			gpd_driver_priv.pwm_enable = val;
+
+			return gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
+		case hwmon_pwm_input:
+			var = clamp_val(val, 0, 255);
+
+			gpd_driver_priv.pwm_value = var;
+
+			return gpd_write_pwm(var);
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops gpd_fan_ops = {
+	.is_visible = gpd_fan_hwmon_is_visible,
+	.read = gpd_fan_hwmon_read,
+	.write = gpd_fan_hwmon_write,
+};
+
+static const struct hwmon_channel_info *gpd_fan_hwmon_channel_info[] = {
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+	NULL
+};
+
+static struct hwmon_chip_info gpd_fan_chip_info = {
+	.ops = &gpd_fan_ops,
+	.info = gpd_fan_hwmon_channel_info
+};
+
+static int gpd_fan_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct resource *res;
+	const struct device *hwdev;
+	const struct resource *region;
+
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (IS_ERR(res))
+		return dev_err_probe(dev, PTR_ERR(res),
+				     "Failed to get platform resource\n");
+
+	region = devm_request_region(dev, res->start,
+				     resource_size(res), DRIVER_NAME);
+	if (IS_ERR(region))
+		return dev_err_probe(dev, PTR_ERR(region),
+				     "Failed to request region\n");
+
+	hwdev = devm_hwmon_device_register_with_info(dev,
+						     DRIVER_NAME,
+						     NULL,
+						     &gpd_fan_chip_info,
+						     NULL);
+	if (IS_ERR(hwdev))
+		return dev_err_probe(dev, PTR_ERR(region),
+				     "Failed to register hwmon device\n");
+
+	return 0;
+}
+
+static void gpd_fan_remove(__always_unused struct platform_device *pdev)
+{
+	gpd_driver_priv.pwm_enable = AUTOMATIC;
+	gpd_set_pwm_enable(AUTOMATIC);
+}
+
+static struct platform_driver gpd_fan_driver = {
+	.probe = gpd_fan_probe,
+	.remove = gpd_fan_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+static struct platform_device *gpd_fan_platform_device;
+
+static int __init gpd_fan_init(void)
+{
+	const struct gpd_fan_drvdata *match = NULL;
+
+	for (const struct gpd_fan_drvdata **p = gpd_module_drvdata; *p; p++) {
+		if (strcmp(gpd_fan_board, (*p)->board_name) == 0) {
+			match = *p;
+			break;
+		}
+	}
+
+	if (!match) {
+		const struct dmi_system_id *dmi_match =
+			dmi_first_match(dmi_table);
+		if (dmi_match)
+			match = dmi_match->driver_data;
+	}
+
+	if (!match)
+		return -ENODEV;
+
+	gpd_driver_priv.pwm_enable = AUTOMATIC;
+	gpd_driver_priv.pwm_value = 255;
+	gpd_driver_priv.drvdata = match;
+
+	struct resource gpd_fan_resources[] = {
+		{
+			.start = match->addr_port,
+			.end = match->data_port,
+			.flags = IORESOURCE_IO,
+		},
+	};
+
+	gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
+							 gpd_fan_probe,
+							 gpd_fan_resources,
+							 1, NULL, 0);
+
+	if (IS_ERR(gpd_fan_platform_device)) {
+		pr_warn("Failed to create platform device\n");
+		return PTR_ERR(gpd_fan_platform_device);
+	}
+
+	return 0;
+}
+
+static void __exit gpd_fan_exit(void)
+{
+	platform_device_unregister(gpd_fan_platform_device);
+	platform_driver_unregister(&gpd_fan_driver);
+}
+
+MODULE_DEVICE_TABLE(dmi, dmi_table);
+
+module_init(gpd_fan_init);
+module_exit(gpd_fan_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cryolitia PukNgae <cryolitia@uniontech.com>");
+MODULE_DESCRIPTION("GPD Devices fan control driver");

-- 
2.50.1

[PATCH RESEND v7 2/2] hwmon: document: add gpd-fan

[Cryolitia PukNgae via B4 Relay]

From: Cryolitia PukNgae <cryolitia@uniontech.com>

Add GPD fan driver document

Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
---
 Documentation/hwmon/gpd-fan.rst | 71 +++++++++++++++++++++++++++++++++++++++++
 Documentation/hwmon/index.rst   |  1 +
 MAINTAINERS                     |  1 +
 3 files changed, 73 insertions(+)

diff --git a/Documentation/hwmon/gpd-fan.rst b/Documentation/hwmon/gpd-fan.rst
new file mode 100644
index 0000000000000000000000000000000000000000..82f064c80aac485348f7c5179a9c4104fd6a4745
--- /dev/null
+++ b/Documentation/hwmon/gpd-fan.rst
@@ -0,0 +1,71 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver gpd-fan
+=========================
+
+Author:
+    - Cryolitia PukNgae <cryolitia@uniontech.com>
+
+Description
+------------
+
+Handheld devices from Shenzhen GPD Technology Co., Ltd. provide fan readings and fan control through
+their embedded controllers.
+
+Supported devices
+-----------------
+
+Currently the driver supports the following handhelds:
+
+ - GPD Win Mini (7840U)
+ - GPD Win Mini (8840U)
+ - GPD Win Mini (HX370)
+ - GPD Pocket 4
+ - GPD Duo
+ - GPD Win Max 2 (6800U)
+ - GPD Win Max 2 2023 (7840U)
+ - GPD Win Max 2 2024 (8840U)
+ - GPD Win Max 2 2025 (HX370)
+ - GPD Win 4 (6800U)
+ - GPD Win 4 (7840U)
+
+Module parameters
+-----------------
+
+gpd_fan_board
+  Force specific which module quirk should be used.
+  Use it like "gpd_fan_board=wm2".
+
+   - wm2
+       - GPD Win 4 (7840U)
+       - GPD Win Max 2 (6800U)
+       - GPD Win Max 2 2023 (7840U)
+       - GPD Win Max 2 2024 (8840U)
+       - GPD Win Max 2 2025 (HX370)
+   - win4
+       - GPD Win 4 (6800U)
+   - win_mini
+       - GPD Win Mini (7840U)
+       - GPD Win Mini (8840U)
+       - GPD Win Mini (HX370)
+       - GPD Pocket 4
+       - GPD Duo
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+fan1_input
+  Read Only. Reads current fan RPM.
+
+pwm1_enable
+  Read/Write. Enable manual fan control. Write "0" to disable control and run at
+  full speed. Write "1" to set to manual, write "2" to let the EC control decide
+  fan speed. Read this attribute to see current status.
+
+pwm1
+  Read/Write. Read this attribute to see current duty cycle in the range [0-255].
+  When pwm1_enable is set to "1" (manual) write any value in the range [0-255]
+  to set fan speed.
+
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index d292a86ac5da902cad02c1965c90f5de530489df..ce4419f064e1368740387af70af38a85cadd952d 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -82,6 +82,7 @@ Hardware Monitoring Kernel Drivers
    gigabyte_waterforce
    gsc-hwmon
    gl518sm
+   gpd-fan
    gxp-fan-ctrl
    hih6130
    hp-wmi-sensors
diff --git a/MAINTAINERS b/MAINTAINERS
index 1deb9b817a37998828b6773d3dc8237c982d4bf9..d5af3b7ab7a5fdc778b4032e5645fd9551223148 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10414,6 +10414,7 @@ GPD FAN DRIVER
 M:	Cryolitia PukNgae <cryolitia@uniontech.com>
 L:	linux-hwmon@vger.kernel.org
 S:	Maintained
+F:	Documentation/hwmon/gpd-fan.rst
 F:	drivers/hwmon/gpd-fan.c
 
 GPD POCKET FAN DRIVER

-- 
2.50.1

Re: [PATCH RESEND v7 1/2] hwmon: add GPD devices sensor driver

[Guenter Roeck]

On Wed, Aug 20, 2025 at 05:50:38PM +0800, Cryolitia PukNgae wrote:
> From: Cryolitia PukNgae <cryolitia@uniontech.com>
> 
> Sensors driver for GPD Handhelds that expose fan reading and control via
> hwmon sysfs.
> 
> Shenzhen GPD Technology Co., Ltd. manufactures a series of handheld
> devices. This driver implements these functions through x86 port-mapped
> IO.
> 
> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
> ---
>  MAINTAINERS             |   6 +
>  drivers/hwmon/Kconfig   |  10 +
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/gpd-fan.c | 753 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 770 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index daf520a13bdf6a991c0160a96620f40308c29ee0..1deb9b817a37998828b6773d3dc8237c982d4bf9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10410,6 +10410,12 @@ F:	drivers/phy/samsung/phy-gs101-ufs.c
>  F:	include/dt-bindings/clock/google,gs101.h
>  K:	[gG]oogle.?[tT]ensor
>  
> +GPD FAN DRIVER
> +M:	Cryolitia PukNgae <cryolitia@uniontech.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	drivers/hwmon/gpd-fan.c
> +
>  GPD POCKET FAN DRIVER
>  M:	Hans de Goede <hansg@kernel.org>
>  L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 9d28fcf7cd2a6f9e2f54694a717bd85ff4047b46..a552a5ced64d0fee2c80a5399ce9d1f0dbd7d763 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -769,6 +769,16 @@ config SENSORS_GL520SM
>  	  This driver can also be built as a module. If so, the module
>  	  will be called gl520sm.
>  
> +config SENSORS_GPD
> +	tristate "GPD handhelds"
> +	depends on X86
> +	help
> +	  If you say yes here you get support for fan readings and
> +	  control over GPD handheld devices.
> +
> +	  Can also be built as a module. In that case it will be
> +	  called gpd-fan.
> +
>  config SENSORS_G760A
>  	tristate "GMT G760A"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index cd8bc4752b4dbf015c6eb46157626f4e8f87dfae..051981eb8a5089608e9eb351a1d5857805c728c8 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_SENSORS_GIGABYTE_WATERFORCE) += gigabyte_waterforce.o
>  obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
>  obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
>  obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
> +obj-$(CONFIG_SENSORS_GPD)	+= gpd-fan.o
>  obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
>  obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
>  obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..11409723cb2989569a33c4c0f1beceac073cb6e4
> --- /dev/null
> +++ b/drivers/hwmon/gpd-fan.c
> @@ -0,0 +1,753 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/* Platform driver for GPD devices that expose fan control via hwmon sysfs.
> + *
> + * Fan control is provided via pwm interface in the range [0-255].
> + * Each model has a different range in the EC, the written value is scaled to
> + * accommodate for that.
> + *
> + * Based on this repo:
> + * https://github.com/Cryolitia/gpd-fan-driver
> + *
> + * Copyright (c) 2024 Cryolitia PukNgae
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "gpdfan"
> +#define GPD_PWM_CTR_OFFSET 0x1841
> +
> +static char *gpd_fan_board = "";
> +module_param(gpd_fan_board, charp, 0444);
> +
> +// EC read/write locker
> +// Should never access EC at the same time, otherwise system down.
> +static DEFINE_MUTEX(gpd_fan_lock);
> +
> +enum gpd_board {
> +	win_mini,
> +	win4_6800u,
> +	win_max_2,
> +	duo,
> +};
> +
> +enum FAN_PWM_ENABLE {
> +	DISABLE		= 0,
> +	MANUAL		= 1,
> +	AUTOMATIC	= 2,
> +};
> +
> +static struct {
> +	enum FAN_PWM_ENABLE pwm_enable;
> +	u8 pwm_value;
> +
> +	const struct gpd_fan_drvdata *drvdata;
> +} gpd_driver_priv;
> +
> +struct gpd_fan_drvdata {
> +	const char *board_name; /* Board name for module param comparison */
> +	const enum gpd_board board;
> +
> +	const u8 addr_port;
> +	const u8 data_port;
> +	const u16 manual_control_enable;
> +	const u16 rpm_read;
> +	const u16 pwm_write;
> +	const u16 pwm_max;
> +};
> +
> +static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
> +	.board_name		= "win_mini",
> +	.board			= win_mini,
> +
> +	.addr_port		= 0x4E,
> +	.data_port		= 0x4F,
> +	.manual_control_enable	= 0x047A,
> +	.rpm_read		= 0x0478,
> +	.pwm_write		= 0x047A,
> +	.pwm_max		= 244,
> +};
> +
> +static struct gpd_fan_drvdata gpd_duo_drvdata = {
> +	.board_name		= "duo",
> +	.board			= duo,
> +
> +	.addr_port		= 0x4E,
> +	.data_port		= 0x4F,
> +	.manual_control_enable	= 0x047A,
> +	.rpm_read		= 0x0478,
> +	.pwm_write		= 0x047A,
> +	.pwm_max		= 244,
> +};
> +
> +static struct gpd_fan_drvdata gpd_win4_drvdata = {
> +	.board_name		= "win4",
> +	.board			= win4_6800u,
> +
> +	.addr_port		= 0x2E,
> +	.data_port		= 0x2F,
> +	.manual_control_enable	= 0xC311,
> +	.rpm_read		= 0xC880,
> +	.pwm_write		= 0xC311,
> +	.pwm_max		= 127,
> +};
> +
> +static struct gpd_fan_drvdata gpd_wm2_drvdata = {
> +	.board_name		= "wm2",
> +	.board			= win_max_2,
> +
> +	.addr_port		= 0x4E,
> +	.data_port		= 0x4F,
> +	.manual_control_enable	= 0x0275,
> +	.rpm_read		= 0x0218,
> +	.pwm_write		= 0x1809,
> +	.pwm_max		= 184,
> +};
> +
> +static const struct dmi_system_id dmi_table[] = {
> +	{
> +		// GPD Win Mini
> +		// GPD Win Mini with AMD Ryzen 8840U
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-01")
> +		},
> +		.driver_data = &gpd_win_mini_drvdata,
> +	},
> +	{
> +		// GPD Win Mini
> +		// GPD Win Mini with AMD Ryzen HX370
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-02")
> +		},
> +		.driver_data = &gpd_win_mini_drvdata,
> +	},
> +	{
> +		// GPD Win Mini
> +		// GPD Win Mini with AMD Ryzen HX370
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-02-L")
> +		},
> +		.driver_data = &gpd_win_mini_drvdata,
> +	},
> +	{
> +		// GPD Win 4 with AMD Ryzen 6800U
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
> +		},
> +		.driver_data = &gpd_win4_drvdata,
> +	},
> +	{
> +		// GPD Win 4 with Ryzen 7840U
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
> +			DMI_MATCH(DMI_BOARD_VERSION, "Ver. 1.0"),
> +		},
> +		// Since 7840U, win4 uses the same drvdata as wm2
> +		.driver_data = &gpd_wm2_drvdata,
> +	},
> +	{
> +		// GPD Win 4 with Ryzen 7840U (another)
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
> +			DMI_MATCH(DMI_BOARD_VERSION, "Ver.1.0"),
> +		},
> +		.driver_data = &gpd_wm2_drvdata,
> +	},
> +	{
> +		// GPD Win Max 2 with Ryzen 6800U
> +		// GPD Win Max 2 2023 with Ryzen 7840U
> +		// GPD Win Max 2 2024 with Ryzen 8840U
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1619-04"),
> +		},
> +		.driver_data = &gpd_wm2_drvdata,
> +	},
> +	{
> +		// GPD Win Max 2 with AMD Ryzen HX370
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1619-05"),
> +		},
> +		.driver_data = &gpd_wm2_drvdata,
> +	},
> +	{
> +		// GPD Duo
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1622-01"),
> +		},
> +		.driver_data = &gpd_duo_drvdata,
> +	},
> +	{
> +		// GPD Duo (another)
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1622-01-L"),
> +		},
> +		.driver_data = &gpd_duo_drvdata,
> +	},
> +	{
> +		// GPD Pocket 4
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1628-04"),
> +		},
> +		.driver_data = &gpd_win_mini_drvdata,
> +	},
> +	{
> +		// GPD Pocket 4 (another)
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1628-04-L"),
> +		},
> +		.driver_data = &gpd_win_mini_drvdata,
> +	},
> +	{}
> +};
> +
> +static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
> +	&gpd_win_mini_drvdata, &gpd_win4_drvdata, &gpd_wm2_drvdata, NULL
> +};
> +
> +/* Helper functions to handle EC read/write */

Either all C++ comments, or all C comments, but not a mix of it.

> +static int gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset,
> +			  u8 *val)
> +{
> +	int ret;
> +	u16 addr_port = drvdata->addr_port;
> +	u16 data_port = drvdata->data_port;
> +
> +	ret = mutex_lock_interruptible(&gpd_fan_lock);
> +

Drop empty lines after assigning ret and before checking it.

Regarding the mutex, I am not sure if it provides the expected
protection. There are several read/read and read/write sequences
which may be called from multiple threads at the same time.
Those are not protected against each other.

> +	if (ret)
> +		return ret;
> +
> +	outb(0x2E, addr_port);
> +	outb(0x11, data_port);
> +	outb(0x2F, addr_port);
> +	outb((u8)((offset >> 8) & 0xFF), data_port);
> +
> +	outb(0x2E, addr_port);
> +	outb(0x10, data_port);
> +	outb(0x2F, addr_port);
> +	outb((u8)(offset & 0xFF), data_port);
> +
> +	outb(0x2E, addr_port);
> +	outb(0x12, data_port);
> +	outb(0x2F, addr_port);
> +	*val = inb(data_port);
> +
> +	mutex_unlock(&gpd_fan_lock);
> +	return 0;
> +}
> +
> +static int gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset,
> +			   u8 value)

drvdata is fixed. Why pass it to this function as argument but not to
other functions ?

> +{
> +	int ret;
> +	u16 addr_port = drvdata->addr_port;
> +	u16 data_port = drvdata->data_port;
> +
> +	ret = mutex_lock_interruptible(&gpd_fan_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	outb(0x2E, addr_port);
> +	outb(0x11, data_port);
> +	outb(0x2F, addr_port);
> +	outb((u8)((offset >> 8) & 0xFF), data_port);
> +
> +	outb(0x2E, addr_port);
> +	outb(0x10, data_port);
> +	outb(0x2F, addr_port);
> +	outb((u8)(offset & 0xFF), data_port);
> +
> +	outb(0x2E, addr_port);
> +	outb(0x12, data_port);
> +	outb(0x2F, addr_port);
> +	outb(value, data_port);
> +
> +	mutex_unlock(&gpd_fan_lock);
> +	return 0;
> +}
> +
> +static int gpd_generic_read_rpm(void)
> +{
> +	u8 high, low;
> +	int ret;
> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;

In general, ordering of variables in reverse christmas tree order is
preferred.

> +
> +	ret = gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
> +	if (ret)
> +		return ret;
> +
> +	ret = gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
> +	if (ret)
> +		return ret;
> +
> +	return (u16)high << 8 | low;
> +}
> +
> +static int gpd_win4_read_rpm(void)
> +{
> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> +	u8 pwm_ctr_reg;
> +	int ret;
> +
> +	gpd_ecram_read(drvdata, GPD_PWM_CTR_OFFSET, &pwm_ctr_reg);

The return value is checked in some places but not in others.
What if the function returned an error ? The content of pwm_ctr_reg
would then be undefined.

> +
> +	if (pwm_ctr_reg != 0x7F)
> +		gpd_ecram_write(drvdata, GPD_PWM_CTR_OFFSET, 0x7F);
> +
> +	ret = gpd_generic_read_rpm();
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret == 0) {
> +		// re-init EC
> +		u8 chip_id;
> +
> +		gpd_ecram_read(drvdata, 0x2000, &chip_id);
> +		if (chip_id == 0x55) {
> +			u8 chip_ver;
> +
> +			if (gpd_ecram_read(drvdata, 0x1060, &chip_ver))
> +				gpd_ecram_write(drvdata, 0x1060,
> +						chip_ver | 0x80);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int gpd_wm2_read_rpm(void)
> +{
> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> +
> +	for (u16 pwm_ctr_offset = GPD_PWM_CTR_OFFSET;
> +	     pwm_ctr_offset <= GPD_PWM_CTR_OFFSET + 2; pwm_ctr_offset++) {
> +		u8 PWMCTR;
> +
> +		gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
> +
> +		if (PWMCTR != 0xB8)
> +			gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
> +	}
> +
> +	return gpd_generic_read_rpm();
> +}
> +
> +// Read value for fan1_input
> +static int gpd_read_rpm(void)
> +{
> +	switch (gpd_driver_priv.drvdata->board) {
> +	case win_mini:
> +	case duo:
> +		return gpd_generic_read_rpm();
> +	case win4_6800u:
> +		return gpd_win4_read_rpm();
> +	case win_max_2:
> +		return gpd_wm2_read_rpm();
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpd_wm2_read_pwm(void)
> +{
> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> +	u8 var;
> +	int ret = gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return var * 255 / drvdata->pwm_max;

You might want to consider using DIV_ROUND_CLOSEST() here
to improve accuracy.

> +}
> +
> +// Read value for pwm1
> +static int gpd_read_pwm(void)
> +{
> +	switch (gpd_driver_priv.drvdata->board) {
> +	case win_mini:
> +	case duo:
> +	case win4_6800u:
> +		return gpd_driver_priv.pwm_value;

That will just return 255 for those boards, or a previously written
value. It does not return the current value as suggested in the
documentation (and most definitely not if the fan is in automatic
mode). That should be documented.

> +	case win_max_2:
> +		return gpd_wm2_read_pwm();
> +	}
> +	return 0;
> +}
> +
> +static int gpd_generic_write_pwm(u8 val)
> +{
> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> +	u8 pwm_reg;
> +
> +	// PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
> +	pwm_reg = val * (drvdata->pwm_max - 1) / 255 + 1;

DIV_ROUND_CLOSEST ?

> +	return gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
> +}
> +
> +static int gpd_win_mini_write_pwm(u8 val)
> +{
> +	if (gpd_driver_priv.pwm_enable == MANUAL)
> +		return gpd_generic_write_pwm(val);
> +	else
> +		return -EPERM;

else after return is unnecessary, and the error check should come first.

> +}
> +
> +static int gpd_duo_write_pwm_twice(u8 val)
> +{
> +	int ret;
> +
> +	ret = gpd_generic_write_pwm(val);
> +
> +	if (ret)
> +		return ret;
> +
> +	return gpd_generic_write_pwm(val+1);

CHECK: spaces preferred around that '+' (ctx:VxV)
#581: FILE: drivers/hwmon/gpd-fan.c:425:
+	return gpd_generic_write_pwm(val+1);
 	                                ^

> +}
> +
> +static int gpd_duo_write_pwm(u8 val)
> +{
> +	if (gpd_driver_priv.pwm_enable == MANUAL)
> +		return gpd_duo_write_pwm_twice(val);
> +	else
> +		return -EPERM;

else after return is unnecessary, and the error check should come first.

	if (gpd_driver_priv.pwm_enable != MANUAL)
		return -EPERM;

	return gpd_duo_write_pwm_twice(val);

> +}
> +
> +static int gpd_wm2_write_pwm(u8 val)
> +{
> +	if (gpd_driver_priv.pwm_enable != DISABLE)
> +		return gpd_generic_write_pwm(val);
> +	else
> +		return -EPERM;

Same as above.

> +}
> +
> +// Write value for pwm1
> +static int gpd_write_pwm(u8 val)
> +{
> +	switch (gpd_driver_priv.drvdata->board) {
> +	case win_mini:
> +		return gpd_win_mini_write_pwm(val);
> +	case duo:
> +		return gpd_duo_write_pwm(val);
> +	case win4_6800u:
> +		return gpd_generic_write_pwm(val);
> +	case win_max_2:
> +		return gpd_wm2_write_pwm(val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
> +{
> +	const struct gpd_fan_drvdata *drvdata;
> +
> +	switch (pwm_enable) {
> +	case DISABLE:
> +		return gpd_generic_write_pwm(255);
> +	case MANUAL:
> +		return gpd_generic_write_pwm(gpd_driver_priv.pwm_value);

This means setting the enable status to MANUAL will set the pwm
speed to the maximum unless a different speed is written first.
That includes win_max_2 even if the value was read before.
Is this really what is intended ?

> +	case AUTOMATIC:
> +		drvdata = gpd_driver_priv.drvdata;
> +		return gpd_ecram_write(drvdata, drvdata->pwm_write, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
> +{
> +	const struct gpd_fan_drvdata *drvdata;
> +
> +	switch (pwm_enable) {
> +	case DISABLE:
> +		return gpd_duo_write_pwm_twice(255);
> +	case MANUAL:
> +		return gpd_duo_write_pwm_twice(gpd_driver_priv.pwm_value);
> +	case AUTOMATIC:
> +		drvdata = gpd_driver_priv.drvdata;
> +		return gpd_ecram_write(drvdata, drvdata->pwm_write, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
> +{
> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> +	int ret;
> +
> +	switch (enable) {
> +	case DISABLE: {
> +		ret = gpd_generic_write_pwm(255);
> +
> +		if (ret)
> +			return ret;
> +
> +		return gpd_ecram_write(drvdata, drvdata->manual_control_enable,
> +				       1);
> +	}
> +	case MANUAL: {
> +		ret = gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
> +
> +		if (ret)
> +			return ret;
> +
> +		return gpd_ecram_write(drvdata, drvdata->manual_control_enable,
> +				       1);
> +	}
> +	case AUTOMATIC: {
> +		ret = gpd_ecram_write(drvdata, drvdata->manual_control_enable,
> +				      0);
> +
> +		return ret;
> +	}

Unnessary set of { }. Same for each case statement above.

> +	}
> +
> +	return 0;
> +}
> +
> +// Write value for pwm1_enable
> +static int gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
> +{
> +	switch (gpd_driver_priv.drvdata->board) {
> +	case win_mini:
> +	case win4_6800u:
> +		return gpd_win_mini_set_pwm_enable(enable);
> +	case duo:
> +		return gpd_duo_set_pwm_enable(enable);
> +	case win_max_2:
> +		return gpd_wm2_set_pwm_enable(enable);
> +	}
> +
> +	return 0;
> +}
> +
> +static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
> +					enum hwmon_sensor_types type, u32 attr,
> +					__always_unused int channel)
> +{
> +	if (type == hwmon_fan && attr == hwmon_fan_input) {
> +		return 0444;
> +	} else if (type == hwmon_pwm) {
> +		switch (attr) {
> +		case hwmon_pwm_enable:
> +		case hwmon_pwm_input:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int gpd_fan_hwmon_read(__always_unused struct device *dev,
> +			      enum hwmon_sensor_types type, u32 attr,
> +			      __always_unused int channel, long *val)
> +{
> +	if (type == hwmon_fan) {
> +		if (attr == hwmon_fan_input) {
> +			int ret = gpd_read_rpm();
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret;
> +			return 0;
> +		}
> +		return -EOPNOTSUPP;
> +	} else if (type == hwmon_pwm) {
> +		int ret;

ret is used in each branch. Declare it once at the beginning of the function.

> +
> +		switch (attr) {
> +		case hwmon_pwm_enable:
> +			*val = gpd_driver_priv.pwm_enable;
> +			return 0;
> +		case hwmon_pwm_input:
> +			ret = gpd_read_pwm();
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int gpd_fan_hwmon_write(__always_unused struct device *dev,
> +			       enum hwmon_sensor_types type, u32 attr,
> +			       __always_unused int channel, long val)
> +{
> +	u8 var;
> +
> +	if (type == hwmon_pwm) {
> +		switch (attr) {
> +		case hwmon_pwm_enable:
> +			if (!in_range(val, 0, 3))
> +				return -EINVAL;
> +
> +			gpd_driver_priv.pwm_enable = val;
> +
> +			return gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
> +		case hwmon_pwm_input:
> +			var = clamp_val(val, 0, 255);

pwm values need to be range checked, not clamped.

> +
> +			gpd_driver_priv.pwm_value = var;
> +
> +			return gpd_write_pwm(var);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops gpd_fan_ops = {
> +	.is_visible = gpd_fan_hwmon_is_visible,
> +	.read = gpd_fan_hwmon_read,
> +	.write = gpd_fan_hwmon_write,
> +};
> +
> +static const struct hwmon_channel_info *gpd_fan_hwmon_channel_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> +	NULL
> +};
> +
> +static struct hwmon_chip_info gpd_fan_chip_info = {
> +	.ops = &gpd_fan_ops,
> +	.info = gpd_fan_hwmon_channel_info
> +};
> +
> +static int gpd_fan_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct resource *res;
> +	const struct device *hwdev;
> +	const struct resource *region;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (IS_ERR(res))
> +		return dev_err_probe(dev, PTR_ERR(res),
> +				     "Failed to get platform resource\n");
> +
> +	region = devm_request_region(dev, res->start,
> +				     resource_size(res), DRIVER_NAME);
> +	if (IS_ERR(region))
> +		return dev_err_probe(dev, PTR_ERR(region),
> +				     "Failed to request region\n");
> +
> +	hwdev = devm_hwmon_device_register_with_info(dev,
> +						     DRIVER_NAME,
> +						     NULL,
> +						     &gpd_fan_chip_info,
> +						     NULL);
> +	if (IS_ERR(hwdev))
> +		return dev_err_probe(dev, PTR_ERR(region),
> +				     "Failed to register hwmon device\n");
> +
> +	return 0;
> +}
> +
> +static void gpd_fan_remove(__always_unused struct platform_device *pdev)
> +{
> +	gpd_driver_priv.pwm_enable = AUTOMATIC;
> +	gpd_set_pwm_enable(AUTOMATIC);
> +}
> +
> +static struct platform_driver gpd_fan_driver = {
> +	.probe = gpd_fan_probe,
> +	.remove = gpd_fan_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	},
> +};
> +
> +static struct platform_device *gpd_fan_platform_device;
> +
> +static int __init gpd_fan_init(void)
> +{
> +	const struct gpd_fan_drvdata *match = NULL;
> +
> +	for (const struct gpd_fan_drvdata **p = gpd_module_drvdata; *p; p++) {
> +		if (strcmp(gpd_fan_board, (*p)->board_name) == 0) {
> +			match = *p;
> +			break;
> +		}
> +	}
> +
> +	if (!match) {
> +		const struct dmi_system_id *dmi_match =
> +			dmi_first_match(dmi_table);
> +		if (dmi_match)
> +			match = dmi_match->driver_data;
> +	}
> +
> +	if (!match)
> +		return -ENODEV;
> +
> +	gpd_driver_priv.pwm_enable = AUTOMATIC;
> +	gpd_driver_priv.pwm_value = 255;

This means the value reported to the user will (for most boards)
be 255 until written, and not match reality.

> +	gpd_driver_priv.drvdata = match;
> +
> +	struct resource gpd_fan_resources[] = {
> +		{
> +			.start = match->addr_port,
> +			.end = match->data_port,
> +			.flags = IORESOURCE_IO,
> +		},
> +	};
> +
> +	gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
> +							 gpd_fan_probe,
> +							 gpd_fan_resources,
> +							 1, NULL, 0);
> +
> +	if (IS_ERR(gpd_fan_platform_device)) {
> +		pr_warn("Failed to create platform device\n");
> +		return PTR_ERR(gpd_fan_platform_device);
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit gpd_fan_exit(void)
> +{
> +	platform_device_unregister(gpd_fan_platform_device);
> +	platform_driver_unregister(&gpd_fan_driver);
> +}
> +
> +MODULE_DEVICE_TABLE(dmi, dmi_table);
> +
> +module_init(gpd_fan_init);
> +module_exit(gpd_fan_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cryolitia PukNgae <cryolitia@uniontech.com>");
> +MODULE_DESCRIPTION("GPD Devices fan control driver");

Re: [PATCH RESEND v7 1/2] hwmon: add GPD devices sensor driver

[Cryolitia PukNgae]

On 02/09/2025 23.38, Guenter Roeck wrote:
> On Wed, Aug 20, 2025 at 05:50:38PM +0800, Cryolitia PukNgae wrote:
>> From: Cryolitia PukNgae <cryolitia@uniontech.com>
>>
>> Sensors driver for GPD Handhelds that expose fan reading and control via
>> hwmon sysfs.
>>
>> Shenzhen GPD Technology Co., Ltd. manufactures a series of handheld
>> devices. This driver implements these functions through x86 port-mapped
>> IO.
>>
>> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
>> ---
>>  MAINTAINERS             |   6 +
>>  drivers/hwmon/Kconfig   |  10 +
>>  drivers/hwmon/Makefile  |   1 +
>>  drivers/hwmon/gpd-fan.c | 753 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 770 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index daf520a13bdf6a991c0160a96620f40308c29ee0..1deb9b817a37998828b6773d3dc8237c982d4bf9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10410,6 +10410,12 @@ F:	drivers/phy/samsung/phy-gs101-ufs.c
>>  F:	include/dt-bindings/clock/google,gs101.h
>>  K:	[gG]oogle.?[tT]ensor
>>  
>> +GPD FAN DRIVER
>> +M:	Cryolitia PukNgae <cryolitia@uniontech.com>
>> +L:	linux-hwmon@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/hwmon/gpd-fan.c
>> +
>>  GPD POCKET FAN DRIVER
>>  M:	Hans de Goede <hansg@kernel.org>
>>  L:	platform-driver-x86@vger.kernel.org
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 9d28fcf7cd2a6f9e2f54694a717bd85ff4047b46..a552a5ced64d0fee2c80a5399ce9d1f0dbd7d763 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -769,6 +769,16 @@ config SENSORS_GL520SM
>>  	  This driver can also be built as a module. If so, the module
>>  	  will be called gl520sm.
>>  
>> +config SENSORS_GPD
>> +	tristate "GPD handhelds"
>> +	depends on X86
>> +	help
>> +	  If you say yes here you get support for fan readings and
>> +	  control over GPD handheld devices.
>> +
>> +	  Can also be built as a module. In that case it will be
>> +	  called gpd-fan.
>> +
>>  config SENSORS_G760A
>>  	tristate "GMT G760A"
>>  	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index cd8bc4752b4dbf015c6eb46157626f4e8f87dfae..051981eb8a5089608e9eb351a1d5857805c728c8 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -88,6 +88,7 @@ obj-$(CONFIG_SENSORS_GIGABYTE_WATERFORCE) += gigabyte_waterforce.o
>>  obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
>>  obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
>>  obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
>> +obj-$(CONFIG_SENSORS_GPD)	+= gpd-fan.o
>>  obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
>>  obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
>>  obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
>> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..11409723cb2989569a33c4c0f1beceac073cb6e4
>> --- /dev/null
>> +++ b/drivers/hwmon/gpd-fan.c
>> @@ -0,0 +1,753 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +/* Platform driver for GPD devices that expose fan control via hwmon sysfs.
>> + *
>> + * Fan control is provided via pwm interface in the range [0-255].
>> + * Each model has a different range in the EC, the written value is scaled to
>> + * accommodate for that.
>> + *
>> + * Based on this repo:
>> + * https://github.com/Cryolitia/gpd-fan-driver
>> + *
>> + * Copyright (c) 2024 Cryolitia PukNgae
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/dmi.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/ioport.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define DRIVER_NAME "gpdfan"
>> +#define GPD_PWM_CTR_OFFSET 0x1841
>> +
>> +static char *gpd_fan_board = "";
>> +module_param(gpd_fan_board, charp, 0444);
>> +
>> +// EC read/write locker
>> +// Should never access EC at the same time, otherwise system down.
>> +static DEFINE_MUTEX(gpd_fan_lock);
>> +
>> +enum gpd_board {
>> +	win_mini,
>> +	win4_6800u,
>> +	win_max_2,
>> +	duo,
>> +};
>> +
>> +enum FAN_PWM_ENABLE {
>> +	DISABLE		= 0,
>> +	MANUAL		= 1,
>> +	AUTOMATIC	= 2,
>> +};
>> +
>> +static struct {
>> +	enum FAN_PWM_ENABLE pwm_enable;
>> +	u8 pwm_value;
>> +
>> +	const struct gpd_fan_drvdata *drvdata;
>> +} gpd_driver_priv;
>> +
>> +struct gpd_fan_drvdata {
>> +	const char *board_name; /* Board name for module param comparison */
>> +	const enum gpd_board board;
>> +
>> +	const u8 addr_port;
>> +	const u8 data_port;
>> +	const u16 manual_control_enable;
>> +	const u16 rpm_read;
>> +	const u16 pwm_write;
>> +	const u16 pwm_max;
>> +};
>> +
>> +static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
>> +	.board_name		= "win_mini",
>> +	.board			= win_mini,
>> +
>> +	.addr_port		= 0x4E,
>> +	.data_port		= 0x4F,
>> +	.manual_control_enable	= 0x047A,
>> +	.rpm_read		= 0x0478,
>> +	.pwm_write		= 0x047A,
>> +	.pwm_max		= 244,
>> +};
>> +
>> +static struct gpd_fan_drvdata gpd_duo_drvdata = {
>> +	.board_name		= "duo",
>> +	.board			= duo,
>> +
>> +	.addr_port		= 0x4E,
>> +	.data_port		= 0x4F,
>> +	.manual_control_enable	= 0x047A,
>> +	.rpm_read		= 0x0478,
>> +	.pwm_write		= 0x047A,
>> +	.pwm_max		= 244,
>> +};
>> +
>> +static struct gpd_fan_drvdata gpd_win4_drvdata = {
>> +	.board_name		= "win4",
>> +	.board			= win4_6800u,
>> +
>> +	.addr_port		= 0x2E,
>> +	.data_port		= 0x2F,
>> +	.manual_control_enable	= 0xC311,
>> +	.rpm_read		= 0xC880,
>> +	.pwm_write		= 0xC311,
>> +	.pwm_max		= 127,
>> +};
>> +
>> +static struct gpd_fan_drvdata gpd_wm2_drvdata = {
>> +	.board_name		= "wm2",
>> +	.board			= win_max_2,
>> +
>> +	.addr_port		= 0x4E,
>> +	.data_port		= 0x4F,
>> +	.manual_control_enable	= 0x0275,
>> +	.rpm_read		= 0x0218,
>> +	.pwm_write		= 0x1809,
>> +	.pwm_max		= 184,
>> +};
>> +
>> +static const struct dmi_system_id dmi_table[] = {
>> +	{
>> +		// GPD Win Mini
>> +		// GPD Win Mini with AMD Ryzen 8840U
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-01")
>> +		},
>> +		.driver_data = &gpd_win_mini_drvdata,
>> +	},
>> +	{
>> +		// GPD Win Mini
>> +		// GPD Win Mini with AMD Ryzen HX370
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-02")
>> +		},
>> +		.driver_data = &gpd_win_mini_drvdata,
>> +	},
>> +	{
>> +		// GPD Win Mini
>> +		// GPD Win Mini with AMD Ryzen HX370
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1617-02-L")
>> +		},
>> +		.driver_data = &gpd_win_mini_drvdata,
>> +	},
>> +	{
>> +		// GPD Win 4 with AMD Ryzen 6800U
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
>> +			DMI_MATCH(DMI_BOARD_VERSION, "Default string"),
>> +		},
>> +		.driver_data = &gpd_win4_drvdata,
>> +	},
>> +	{
>> +		// GPD Win 4 with Ryzen 7840U
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
>> +			DMI_MATCH(DMI_BOARD_VERSION, "Ver. 1.0"),
>> +		},
>> +		// Since 7840U, win4 uses the same drvdata as wm2
>> +		.driver_data = &gpd_wm2_drvdata,
>> +	},
>> +	{
>> +		// GPD Win 4 with Ryzen 7840U (another)
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"),
>> +			DMI_MATCH(DMI_BOARD_VERSION, "Ver.1.0"),
>> +		},
>> +		.driver_data = &gpd_wm2_drvdata,
>> +	},
>> +	{
>> +		// GPD Win Max 2 with Ryzen 6800U
>> +		// GPD Win Max 2 2023 with Ryzen 7840U
>> +		// GPD Win Max 2 2024 with Ryzen 8840U
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1619-04"),
>> +		},
>> +		.driver_data = &gpd_wm2_drvdata,
>> +	},
>> +	{
>> +		// GPD Win Max 2 with AMD Ryzen HX370
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1619-05"),
>> +		},
>> +		.driver_data = &gpd_wm2_drvdata,
>> +	},
>> +	{
>> +		// GPD Duo
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1622-01"),
>> +		},
>> +		.driver_data = &gpd_duo_drvdata,
>> +	},
>> +	{
>> +		// GPD Duo (another)
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1622-01-L"),
>> +		},
>> +		.driver_data = &gpd_duo_drvdata,
>> +	},
>> +	{
>> +		// GPD Pocket 4
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1628-04"),
>> +		},
>> +		.driver_data = &gpd_win_mini_drvdata,
>> +	},
>> +	{
>> +		// GPD Pocket 4 (another)
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "GPD"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "G1628-04-L"),
>> +		},
>> +		.driver_data = &gpd_win_mini_drvdata,
>> +	},
>> +	{}
>> +};
>> +
>> +static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
>> +	&gpd_win_mini_drvdata, &gpd_win4_drvdata, &gpd_wm2_drvdata, NULL
>> +};
>> +
>> +/* Helper functions to handle EC read/write */
> Either all C++ comments, or all C comments, but not a mix of it.
>
>> +static int gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset,
>> +			  u8 *val)
>> +{
>> +	int ret;
>> +	u16 addr_port = drvdata->addr_port;
>> +	u16 data_port = drvdata->data_port;
>> +
>> +	ret = mutex_lock_interruptible(&gpd_fan_lock);
>> +
> Drop empty lines after assigning ret and before checking it.
>
> Regarding the mutex, I am not sure if it provides the expected
> protection. There are several read/read and read/write sequences
> which may be called from multiple threads at the same time.
> Those are not protected against each other.
>
>> +	if (ret)
>> +		return ret;
>> +
>> +	outb(0x2E, addr_port);
>> +	outb(0x11, data_port);
>> +	outb(0x2F, addr_port);
>> +	outb((u8)((offset >> 8) & 0xFF), data_port);
>> +
>> +	outb(0x2E, addr_port);
>> +	outb(0x10, data_port);
>> +	outb(0x2F, addr_port);
>> +	outb((u8)(offset & 0xFF), data_port);
>> +
>> +	outb(0x2E, addr_port);
>> +	outb(0x12, data_port);
>> +	outb(0x2F, addr_port);
>> +	*val = inb(data_port);
>> +
>> +	mutex_unlock(&gpd_fan_lock);
>> +	return 0;
>> +}
>> +
>> +static int gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset,
>> +			   u8 value)
> drvdata is fixed. Why pass it to this function as argument but not to
> other functions ?
>
>> +{
>> +	int ret;
>> +	u16 addr_port = drvdata->addr_port;
>> +	u16 data_port = drvdata->data_port;
>> +
>> +	ret = mutex_lock_interruptible(&gpd_fan_lock);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	outb(0x2E, addr_port);
>> +	outb(0x11, data_port);
>> +	outb(0x2F, addr_port);
>> +	outb((u8)((offset >> 8) & 0xFF), data_port);
>> +
>> +	outb(0x2E, addr_port);
>> +	outb(0x10, data_port);
>> +	outb(0x2F, addr_port);
>> +	outb((u8)(offset & 0xFF), data_port);
>> +
>> +	outb(0x2E, addr_port);
>> +	outb(0x12, data_port);
>> +	outb(0x2F, addr_port);
>> +	outb(value, data_port);
>> +
>> +	mutex_unlock(&gpd_fan_lock);
>> +	return 0;
>> +}
>> +
>> +static int gpd_generic_read_rpm(void)
>> +{
>> +	u8 high, low;
>> +	int ret;
>> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> In general, ordering of variables in reverse christmas tree order is
> preferred.
>
>> +
>> +	ret = gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return (u16)high << 8 | low;
>> +}
>> +
>> +static int gpd_win4_read_rpm(void)
>> +{
>> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> +	u8 pwm_ctr_reg;
>> +	int ret;
>> +
>> +	gpd_ecram_read(drvdata, GPD_PWM_CTR_OFFSET, &pwm_ctr_reg);
> The return value is checked in some places but not in others.
> What if the function returned an error ? The content of pwm_ctr_reg
> would then be undefined.
>
>> +
>> +	if (pwm_ctr_reg != 0x7F)
>> +		gpd_ecram_write(drvdata, GPD_PWM_CTR_OFFSET, 0x7F);
>> +
>> +	ret = gpd_generic_read_rpm();
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret == 0) {
>> +		// re-init EC
>> +		u8 chip_id;
>> +
>> +		gpd_ecram_read(drvdata, 0x2000, &chip_id);
>> +		if (chip_id == 0x55) {
>> +			u8 chip_ver;
>> +
>> +			if (gpd_ecram_read(drvdata, 0x1060, &chip_ver))
>> +				gpd_ecram_write(drvdata, 0x1060,
>> +						chip_ver | 0x80);
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int gpd_wm2_read_rpm(void)
>> +{
>> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> +
>> +	for (u16 pwm_ctr_offset = GPD_PWM_CTR_OFFSET;
>> +	     pwm_ctr_offset <= GPD_PWM_CTR_OFFSET + 2; pwm_ctr_offset++) {
>> +		u8 PWMCTR;
>> +
>> +		gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
>> +
>> +		if (PWMCTR != 0xB8)
>> +			gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
>> +	}
>> +
>> +	return gpd_generic_read_rpm();
>> +}
>> +
>> +// Read value for fan1_input
>> +static int gpd_read_rpm(void)
>> +{
>> +	switch (gpd_driver_priv.drvdata->board) {
>> +	case win_mini:
>> +	case duo:
>> +		return gpd_generic_read_rpm();
>> +	case win4_6800u:
>> +		return gpd_win4_read_rpm();
>> +	case win_max_2:
>> +		return gpd_wm2_read_rpm();
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int gpd_wm2_read_pwm(void)
>> +{
>> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> +	u8 var;
>> +	int ret = gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return var * 255 / drvdata->pwm_max;
> You might want to consider using DIV_ROUND_CLOSEST() here
> to improve accuracy.
>
>> +}
>> +
>> +// Read value for pwm1
>> +static int gpd_read_pwm(void)
>> +{
>> +	switch (gpd_driver_priv.drvdata->board) {
>> +	case win_mini:
>> +	case duo:
>> +	case win4_6800u:
>> +		return gpd_driver_priv.pwm_value;
> That will just return 255 for those boards, or a previously written
> value. It does not return the current value as suggested in the
> documentation (and most definitely not if the fan is in automatic
> mode). That should be documented.

So far, only GPD Win Max series support to read out the pwm value. I will
documente it in the next version.

>> +	case win_max_2:
>> +		return gpd_wm2_read_pwm();
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int gpd_generic_write_pwm(u8 val)
>> +{
>> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> +	u8 pwm_reg;
>> +
>> +	// PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
>> +	pwm_reg = val * (drvdata->pwm_max - 1) / 255 + 1;
> DIV_ROUND_CLOSEST ?
>
>> +	return gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
>> +}
>> +
>> +static int gpd_win_mini_write_pwm(u8 val)
>> +{
>> +	if (gpd_driver_priv.pwm_enable == MANUAL)
>> +		return gpd_generic_write_pwm(val);
>> +	else
>> +		return -EPERM;
> else after return is unnecessary, and the error check should come first.
>
>> +}
>> +
>> +static int gpd_duo_write_pwm_twice(u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = gpd_generic_write_pwm(val);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	return gpd_generic_write_pwm(val+1);
> CHECK: spaces preferred around that '+' (ctx:VxV)
> #581: FILE: drivers/hwmon/gpd-fan.c:425:
> +	return gpd_generic_write_pwm(val+1);
>  	                                ^
>
>> +}
>> +
>> +static int gpd_duo_write_pwm(u8 val)
>> +{
>> +	if (gpd_driver_priv.pwm_enable == MANUAL)
>> +		return gpd_duo_write_pwm_twice(val);
>> +	else
>> +		return -EPERM;
> else after return is unnecessary, and the error check should come first.
>
> 	if (gpd_driver_priv.pwm_enable != MANUAL)
> 		return -EPERM;
>
> 	return gpd_duo_write_pwm_twice(val);
>
>> +}
>> +
>> +static int gpd_wm2_write_pwm(u8 val)
>> +{
>> +	if (gpd_driver_priv.pwm_enable != DISABLE)
>> +		return gpd_generic_write_pwm(val);
>> +	else
>> +		return -EPERM;
> Same as above.
>
>> +}
>> +
>> +// Write value for pwm1
>> +static int gpd_write_pwm(u8 val)
>> +{
>> +	switch (gpd_driver_priv.drvdata->board) {
>> +	case win_mini:
>> +		return gpd_win_mini_write_pwm(val);
>> +	case duo:
>> +		return gpd_duo_write_pwm(val);
>> +	case win4_6800u:
>> +		return gpd_generic_write_pwm(val);
>> +	case win_max_2:
>> +		return gpd_wm2_write_pwm(val);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>> +{
>> +	const struct gpd_fan_drvdata *drvdata;
>> +
>> +	switch (pwm_enable) {
>> +	case DISABLE:
>> +		return gpd_generic_write_pwm(255);
>> +	case MANUAL:
>> +		return gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
> This means setting the enable status to MANUAL will set the pwm
> speed to the maximum unless a different speed is written first.
> That includes win_max_2 even if the value was read before.
> Is this really what is intended ?
Thank you for raising this important question. After reconsidering the
design, I believe this approach is indeed intentional and justified for
this specific hardware platform.

GPD devices are extremely compact yet high-performance systems, and their
gaming handheld form factor means they're often subjected to rapidly
fluctuating workloads. The small thermal mass combined with high heat
generation creates a scenario where even minor CPU load changes can cause
temperature spikes of 10+ Kelvin within seconds—as I observed during
testing when briefly stopping the fan.

This behavior is designed with a specific safety philosophy: when userspace
announces it's taking manual control, we assume it has already prepared
proper thermal monitoring and fan curves. The brief moment of max fan
speed acts as a safety buffer until userspace writes its first intended
value—which should happen almost immediately if the userspace controller
is properly implemented. This prevents any gap in thermal protection or
frame drops caused by throttling if userspace fails to write a value
promptly after claiming control.
>> +	case AUTOMATIC:
>> +		drvdata = gpd_driver_priv.drvdata;
>> +		return gpd_ecram_write(drvdata, drvdata->pwm_write, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>> +{
>> +	const struct gpd_fan_drvdata *drvdata;
>> +
>> +	switch (pwm_enable) {
>> +	case DISABLE:
>> +		return gpd_duo_write_pwm_twice(255);
>> +	case MANUAL:
>> +		return gpd_duo_write_pwm_twice(gpd_driver_priv.pwm_value);
>> +	case AUTOMATIC:
>> +		drvdata = gpd_driver_priv.drvdata;
>> +		return gpd_ecram_write(drvdata, drvdata->pwm_write, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
>> +{
>> +	const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> +	int ret;
>> +
>> +	switch (enable) {
>> +	case DISABLE: {
>> +		ret = gpd_generic_write_pwm(255);
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		return gpd_ecram_write(drvdata, drvdata->manual_control_enable,
>> +				       1);
>> +	}
>> +	case MANUAL: {
>> +		ret = gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		return gpd_ecram_write(drvdata, drvdata->manual_control_enable,
>> +				       1);
>> +	}
>> +	case AUTOMATIC: {
>> +		ret = gpd_ecram_write(drvdata, drvdata->manual_control_enable,
>> +				      0);
>> +
>> +		return ret;
>> +	}
> Unnessary set of { }. Same for each case statement above.
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +// Write value for pwm1_enable
>> +static int gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
>> +{
>> +	switch (gpd_driver_priv.drvdata->board) {
>> +	case win_mini:
>> +	case win4_6800u:
>> +		return gpd_win_mini_set_pwm_enable(enable);
>> +	case duo:
>> +		return gpd_duo_set_pwm_enable(enable);
>> +	case win_max_2:
>> +		return gpd_wm2_set_pwm_enable(enable);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
>> +					enum hwmon_sensor_types type, u32 attr,
>> +					__always_unused int channel)
>> +{
>> +	if (type == hwmon_fan && attr == hwmon_fan_input) {
>> +		return 0444;
>> +	} else if (type == hwmon_pwm) {
>> +		switch (attr) {
>> +		case hwmon_pwm_enable:
>> +		case hwmon_pwm_input:
>> +			return 0644;
>> +		default:
>> +			return 0;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>> +			      enum hwmon_sensor_types type, u32 attr,
>> +			      __always_unused int channel, long *val)
>> +{
>> +	if (type == hwmon_fan) {
>> +		if (attr == hwmon_fan_input) {
>> +			int ret = gpd_read_rpm();
>> +
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			*val = ret;
>> +			return 0;
>> +		}
>> +		return -EOPNOTSUPP;
>> +	} else if (type == hwmon_pwm) {
>> +		int ret;
> ret is used in each branch. Declare it once at the beginning of the function.
>
>> +
>> +		switch (attr) {
>> +		case hwmon_pwm_enable:
>> +			*val = gpd_driver_priv.pwm_enable;
>> +			return 0;
>> +		case hwmon_pwm_input:
>> +			ret = gpd_read_pwm();
>> +
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			*val = ret;
>> +			return 0;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int gpd_fan_hwmon_write(__always_unused struct device *dev,
>> +			       enum hwmon_sensor_types type, u32 attr,
>> +			       __always_unused int channel, long val)
>> +{
>> +	u8 var;
>> +
>> +	if (type == hwmon_pwm) {
>> +		switch (attr) {
>> +		case hwmon_pwm_enable:
>> +			if (!in_range(val, 0, 3))
>> +				return -EINVAL;
>> +
>> +			gpd_driver_priv.pwm_enable = val;
>> +
>> +			return gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
>> +		case hwmon_pwm_input:
>> +			var = clamp_val(val, 0, 255);
> pwm values need to be range checked, not clamped.
>
>> +
>> +			gpd_driver_priv.pwm_value = var;
>> +
>> +			return gpd_write_pwm(var);
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops gpd_fan_ops = {
>> +	.is_visible = gpd_fan_hwmon_is_visible,
>> +	.read = gpd_fan_hwmon_read,
>> +	.write = gpd_fan_hwmon_write,
>> +};
>> +
>> +static const struct hwmon_channel_info *gpd_fan_hwmon_channel_info[] = {
>> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
>> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
>> +	NULL
>> +};
>> +
>> +static struct hwmon_chip_info gpd_fan_chip_info = {
>> +	.ops = &gpd_fan_ops,
>> +	.info = gpd_fan_hwmon_channel_info
>> +};
>> +
>> +static int gpd_fan_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	const struct resource *res;
>> +	const struct device *hwdev;
>> +	const struct resource *region;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> +	if (IS_ERR(res))
>> +		return dev_err_probe(dev, PTR_ERR(res),
>> +				     "Failed to get platform resource\n");
>> +
>> +	region = devm_request_region(dev, res->start,
>> +				     resource_size(res), DRIVER_NAME);
>> +	if (IS_ERR(region))
>> +		return dev_err_probe(dev, PTR_ERR(region),
>> +				     "Failed to request region\n");
>> +
>> +	hwdev = devm_hwmon_device_register_with_info(dev,
>> +						     DRIVER_NAME,
>> +						     NULL,
>> +						     &gpd_fan_chip_info,
>> +						     NULL);
>> +	if (IS_ERR(hwdev))
>> +		return dev_err_probe(dev, PTR_ERR(region),
>> +				     "Failed to register hwmon device\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static void gpd_fan_remove(__always_unused struct platform_device *pdev)
>> +{
>> +	gpd_driver_priv.pwm_enable = AUTOMATIC;
>> +	gpd_set_pwm_enable(AUTOMATIC);
>> +}
>> +
>> +static struct platform_driver gpd_fan_driver = {
>> +	.probe = gpd_fan_probe,
>> +	.remove = gpd_fan_remove,
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +	},
>> +};
>> +
>> +static struct platform_device *gpd_fan_platform_device;
>> +
>> +static int __init gpd_fan_init(void)
>> +{
>> +	const struct gpd_fan_drvdata *match = NULL;
>> +
>> +	for (const struct gpd_fan_drvdata **p = gpd_module_drvdata; *p; p++) {
>> +		if (strcmp(gpd_fan_board, (*p)->board_name) == 0) {
>> +			match = *p;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!match) {
>> +		const struct dmi_system_id *dmi_match =
>> +			dmi_first_match(dmi_table);
>> +		if (dmi_match)
>> +			match = dmi_match->driver_data;
>> +	}
>> +
>> +	if (!match)
>> +		return -ENODEV;
>> +
>> +	gpd_driver_priv.pwm_enable = AUTOMATIC;
>> +	gpd_driver_priv.pwm_value = 255;
> This means the value reported to the user will (for most boards)
> be 255 until written, and not match reality.
>
We can only read the pwm value on the GPD Win Max devices, I'll documente it.
>> +	gpd_driver_priv.drvdata = match;
>> +
>> +	struct resource gpd_fan_resources[] = {
>> +		{
>> +			.start = match->addr_port,
>> +			.end = match->data_port,
>> +			.flags = IORESOURCE_IO,
>> +		},
>> +	};
>> +
>> +	gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
>> +							 gpd_fan_probe,
>> +							 gpd_fan_resources,
>> +							 1, NULL, 0);
>> +
>> +	if (IS_ERR(gpd_fan_platform_device)) {
>> +		pr_warn("Failed to create platform device\n");
>> +		return PTR_ERR(gpd_fan_platform_device);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit gpd_fan_exit(void)
>> +{
>> +	platform_device_unregister(gpd_fan_platform_device);
>> +	platform_driver_unregister(&gpd_fan_driver);
>> +}
>> +
>> +MODULE_DEVICE_TABLE(dmi, dmi_table);
>> +
>> +module_init(gpd_fan_init);
>> +module_exit(gpd_fan_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Cryolitia PukNgae <cryolitia@uniontech.com>");
>> +MODULE_DESCRIPTION("GPD Devices fan control driver");

Re: [PATCH RESEND v7 1/2] hwmon: add GPD devices sensor driver

[Guenter Roeck]

On 9/3/25 01:33, Cryolitia PukNgae wrote:
> 
> On 02/09/2025 23.38, Guenter Roeck wrote:
>> On Wed, Aug 20, 2025 at 05:50:38PM +0800, Cryolitia PukNgae wrote:
>>> From: Cryolitia PukNgae <cryolitia@uniontech.com>
>>>
>>> Sensors driver for GPD Handhelds that expose fan reading and control via
>>> hwmon sysfs.
>>>
>>> Shenzhen GPD Technology Co., Ltd. manufactures a series of handheld
>>> devices. This driver implements these functions through x86 port-mapped
>>> IO.
>>>
>>> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
>>> ---

>>> +static int gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>>> +{
>>> +	const struct gpd_fan_drvdata *drvdata;
>>> +
>>> +	switch (pwm_enable) {
>>> +	case DISABLE:
>>> +		return gpd_generic_write_pwm(255);
>>> +	case MANUAL:
>>> +		return gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
>> This means setting the enable status to MANUAL will set the pwm
>> speed to the maximum unless a different speed is written first.
>> That includes win_max_2 even if the value was read before.
>> Is this really what is intended ?
> Thank you for raising this important question. After reconsidering the
> design, I believe this approach is indeed intentional and justified for
> this specific hardware platform.
> 
> GPD devices are extremely compact yet high-performance systems, and their
> gaming handheld form factor means they're often subjected to rapidly
> fluctuating workloads. The small thermal mass combined with high heat
> generation creates a scenario where even minor CPU load changes can cause
> temperature spikes of 10+ Kelvin within seconds—as I observed during
> testing when briefly stopping the fan.
> 
> This behavior is designed with a specific safety philosophy: when userspace
> announces it's taking manual control, we assume it has already prepared
> proper thermal monitoring and fan curves. The brief moment of max fan
> speed acts as a safety buffer until userspace writes its first intended
> value—which should happen almost immediately if the userspace controller
> is properly implemented. This prevents any gap in thermal protection or
> frame drops caused by throttling if userspace fails to write a value
> promptly after claiming control.

Please make sure to document this behavior.

Thanks,
Guenter