* [PATCH 0/3] cleanups in hwmon gpd-fan
@ 2026-04-05 8:40 Pei Xiao
2026-04-05 8:40 ` [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv Pei Xiao
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Pei Xiao @ 2026-04-05 8:40 UTC (permalink / raw)
To: cryolitia, linux, linux-hwmon, linux-kernel; +Cc: Pei Xiao
hi Maintainer,
Some cleanups in gpd-fan driver.
But I have no machine to test this patches, only have compile test.
Please have a look and test those!
Thanks!
Pei Xiao (3):
hwmon: (gpd-fan): remove global variable gpd_driver_priv
hwmon: (gpd-fan): upgrade log level from warn to err for platform
device creation failure
hwmon: (gpd-fan): rename enum constants to uppercase as per kernel
coding style
drivers/hwmon/gpd-fan.c | 273 +++++++++++++++++++++-------------------
1 file changed, 147 insertions(+), 126 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv
2026-04-05 8:40 [PATCH 0/3] cleanups in hwmon gpd-fan Pei Xiao
@ 2026-04-05 8:40 ` Pei Xiao
2026-05-22 5:50 ` Pei Xiao
2026-04-05 8:40 ` [PATCH 2/3] hwmon: (gpd-fan): upgrade log level from warn to err for platform device creation failure Pei Xiao
2026-04-05 8:40 ` [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style Pei Xiao
2 siblings, 1 reply; 9+ messages in thread
From: Pei Xiao @ 2026-04-05 8:40 UTC (permalink / raw)
To: cryolitia, linux, linux-hwmon, linux-kernel; +Cc: Pei Xiao
Replace the global state gpd_driver_priv with per-device private data
(struct gpd_fan_data) allocated in probe. This allows the driver to
support multiple instances in the future and aligns with kernel best
practices.
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
drivers/hwmon/gpd-fan.c | 211 ++++++++++++++++++++++------------------
1 file changed, 116 insertions(+), 95 deletions(-)
diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
index 80de5f20781e..5a9d07cd29ab 100644
--- a/drivers/hwmon/gpd-fan.c
+++ b/drivers/hwmon/gpd-fan.c
@@ -40,12 +40,11 @@ enum FAN_PWM_ENABLE {
AUTOMATIC = 2,
};
-static struct {
+struct gpd_fan_data {
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
@@ -249,10 +248,10 @@ static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
};
// Helper functions to handle EC read/write
-static void gpd_ecram_read(u16 offset, u8 *val)
+static void gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 *val)
{
- u16 addr_port = gpd_driver_priv.drvdata->addr_port;
- u16 data_port = gpd_driver_priv.drvdata->data_port;
+ u16 addr_port = drvdata->addr_port;
+ u16 data_port = drvdata->data_port;
outb(0x2E, addr_port);
outb(0x11, data_port);
@@ -270,10 +269,10 @@ static void gpd_ecram_read(u16 offset, u8 *val)
*val = inb(data_port);
}
-static void gpd_ecram_write(u16 offset, u8 value)
+static void gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 value)
{
- u16 addr_port = gpd_driver_priv.drvdata->addr_port;
- u16 data_port = gpd_driver_priv.drvdata->data_port;
+ u16 addr_port = drvdata->addr_port;
+ u16 data_port = drvdata->data_port;
outb(0x2E, addr_port);
outb(0x11, data_port);
@@ -291,198 +290,198 @@ static void gpd_ecram_write(u16 offset, u8 value)
outb(value, data_port);
}
-static int gpd_generic_read_rpm(void)
+static int gpd_generic_read_rpm(struct gpd_fan_data *data)
{
- const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+ const struct gpd_fan_drvdata *drvdata = data->drvdata;
u8 high, low;
- gpd_ecram_read(drvdata->rpm_read, &high);
- gpd_ecram_read(drvdata->rpm_read + 1, &low);
+ gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
+ gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
return (u16)high << 8 | low;
}
-static int gpd_wm2_read_rpm(void)
+static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
{
+ const struct gpd_fan_drvdata *drvdata = data->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(pwm_ctr_offset, &PWMCTR);
+ gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
if (PWMCTR != 0xB8)
- gpd_ecram_write(pwm_ctr_offset, 0xB8);
+ gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
}
- return gpd_generic_read_rpm();
+ return gpd_generic_read_rpm(data);
}
// Read value for fan1_input
-static int gpd_read_rpm(void)
+static int gpd_read_rpm(struct gpd_fan_data *data)
{
- switch (gpd_driver_priv.drvdata->board) {
+ switch (data->drvdata->board) {
case win4_6800u:
case win_mini:
case duo:
case mpc2:
- return gpd_generic_read_rpm();
+ return gpd_generic_read_rpm(data);
case win_max_2:
- return gpd_wm2_read_rpm();
+ return gpd_wm2_read_rpm(data);
}
return 0;
}
-static int gpd_wm2_read_pwm(void)
+static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
{
- const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+ const struct gpd_fan_drvdata *drvdata = data->drvdata;
u8 var;
- gpd_ecram_read(drvdata->pwm_write, &var);
+ gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
// Match gpd_generic_write_pwm(u8) below
return DIV_ROUND_CLOSEST((var - 1) * 255, (drvdata->pwm_max - 1));
}
// Read value for pwm1
-static int gpd_read_pwm(void)
+static int gpd_read_pwm(struct gpd_fan_data *data)
{
- switch (gpd_driver_priv.drvdata->board) {
+ switch (data->drvdata->board) {
case win_mini:
case duo:
case win4_6800u:
case mpc2:
- switch (gpd_driver_priv.pwm_enable) {
+ switch (data->pwm_enable) {
case DISABLE:
return 255;
case MANUAL:
- return gpd_driver_priv.pwm_value;
+ return data->pwm_value;
case AUTOMATIC:
return -EOPNOTSUPP;
}
break;
case win_max_2:
- return gpd_wm2_read_pwm();
+ return gpd_wm2_read_pwm(data);
}
return 0;
}
// PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
-static inline u8 gpd_cast_pwm_range(u8 val)
+static inline u8 gpd_cast_pwm_range(const struct gpd_fan_drvdata *drvdata, u8 val)
{
- const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
-
return DIV_ROUND_CLOSEST(val * (drvdata->pwm_max - 1), 255) + 1;
}
-static void gpd_generic_write_pwm(u8 val)
+static void gpd_generic_write_pwm(struct gpd_fan_data *data, u8 val)
{
- const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+ const struct gpd_fan_drvdata *drvdata = data->drvdata;
u8 pwm_reg;
- pwm_reg = gpd_cast_pwm_range(val);
- gpd_ecram_write(drvdata->pwm_write, pwm_reg);
+ pwm_reg = gpd_cast_pwm_range(drvdata, val);
+ gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
}
-static void gpd_duo_write_pwm(u8 val)
+static void gpd_duo_write_pwm(struct gpd_fan_data *data, u8 val)
{
- const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+ const struct gpd_fan_drvdata *drvdata = data->drvdata;
u8 pwm_reg;
- pwm_reg = gpd_cast_pwm_range(val);
- gpd_ecram_write(drvdata->pwm_write, pwm_reg);
- gpd_ecram_write(drvdata->pwm_write + 1, pwm_reg);
+ pwm_reg = gpd_cast_pwm_range(drvdata, val);
+ gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
+ gpd_ecram_write(drvdata, drvdata->pwm_write + 1, pwm_reg);
}
// Write value for pwm1
-static int gpd_write_pwm(u8 val)
+static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
{
- if (gpd_driver_priv.pwm_enable != MANUAL)
+ if (data->pwm_enable != MANUAL)
return -EPERM;
- switch (gpd_driver_priv.drvdata->board) {
+ switch (data->drvdata->board) {
case duo:
- gpd_duo_write_pwm(val);
+ gpd_duo_write_pwm(data, val);
break;
case win_mini:
case win4_6800u:
case win_max_2:
case mpc2:
- gpd_generic_write_pwm(val);
+ gpd_generic_write_pwm(data, val);
break;
}
return 0;
}
-static void gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
+static void gpd_win_mini_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
{
switch (pwm_enable) {
case DISABLE:
- gpd_generic_write_pwm(255);
+ gpd_generic_write_pwm(data, 255);
break;
case MANUAL:
- gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
+ gpd_generic_write_pwm(data, data->pwm_value);
break;
case AUTOMATIC:
- gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
+ gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
break;
}
}
-static void gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
+static void gpd_duo_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
{
switch (pwm_enable) {
case DISABLE:
- gpd_duo_write_pwm(255);
+ gpd_duo_write_pwm(data, 255);
break;
case MANUAL:
- gpd_duo_write_pwm(gpd_driver_priv.pwm_value);
+ gpd_duo_write_pwm(data, data->pwm_value);
break;
case AUTOMATIC:
- gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
+ gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
break;
}
}
-static void gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
+static void gpd_wm2_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
{
- const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
+ const struct gpd_fan_drvdata *drvdata = data->drvdata;
switch (enable) {
case DISABLE:
- gpd_generic_write_pwm(255);
- gpd_ecram_write(drvdata->manual_control_enable, 1);
+ gpd_generic_write_pwm(data, 255);
+ gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
break;
case MANUAL:
- gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
- gpd_ecram_write(drvdata->manual_control_enable, 1);
+ gpd_generic_write_pwm(data, data->pwm_value);
+ gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
break;
case AUTOMATIC:
- gpd_ecram_write(drvdata->manual_control_enable, 0);
+ gpd_ecram_write(drvdata, drvdata->manual_control_enable, 0);
break;
}
}
// Write value for pwm1_enable
-static void gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
+static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
{
if (enable == MANUAL)
// Set pwm_value to max firstly when switching to manual mode, in
// consideration of device safety.
- gpd_driver_priv.pwm_value = 255;
+ data->pwm_value = 255;
- switch (gpd_driver_priv.drvdata->board) {
+ switch (data->drvdata->board) {
case win_mini:
case win4_6800u:
case mpc2:
- gpd_win_mini_set_pwm_enable(enable);
+ gpd_win_mini_set_pwm_enable(data, enable);
break;
case duo:
- gpd_duo_set_pwm_enable(enable);
+ gpd_duo_set_pwm_enable(data, enable);
break;
case win_max_2:
- gpd_wm2_set_pwm_enable(enable);
+ gpd_wm2_set_pwm_enable(data, enable);
break;
}
}
@@ -505,15 +504,16 @@ static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
return 0;
}
-static int gpd_fan_hwmon_read(__always_unused struct device *dev,
+static int gpd_fan_hwmon_read(struct device *dev,
enum hwmon_sensor_types type, u32 attr,
__always_unused int channel, long *val)
{
+ struct gpd_fan_data *data = dev_get_drvdata(dev);
int ret;
if (type == hwmon_fan) {
if (attr == hwmon_fan_input) {
- ret = gpd_read_rpm();
+ ret = gpd_read_rpm(data);
if (ret < 0)
return ret;
@@ -524,10 +524,10 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
} else if (type == hwmon_pwm) {
switch (attr) {
case hwmon_pwm_enable:
- *val = gpd_driver_priv.pwm_enable;
+ *val = data->pwm_enable;
return 0;
case hwmon_pwm_input:
- ret = gpd_read_pwm();
+ ret = gpd_read_pwm(data);
if (ret < 0)
return ret;
@@ -540,27 +540,29 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
return -EOPNOTSUPP;
}
-static int gpd_fan_hwmon_write(__always_unused struct device *dev,
+static int gpd_fan_hwmon_write(struct device *dev,
enum hwmon_sensor_types type, u32 attr,
__always_unused int channel, long val)
{
+ struct gpd_fan_data *data = dev_get_drvdata(dev);
+
if (type == hwmon_pwm) {
switch (attr) {
case hwmon_pwm_enable:
if (!in_range(val, 0, 3))
return -EINVAL;
- gpd_driver_priv.pwm_enable = val;
+ data->pwm_enable = val;
- gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
+ gpd_set_pwm_enable(data, data->pwm_enable);
return 0;
case hwmon_pwm_input:
if (!in_range(val, 0, 256))
return -EINVAL;
- gpd_driver_priv.pwm_value = val;
+ data->pwm_value = val;
- return gpd_write_pwm(val);
+ return gpd_write_pwm(data, val);
}
}
@@ -584,26 +586,27 @@ static struct hwmon_chip_info gpd_fan_chip_info = {
.info = gpd_fan_hwmon_channel_info
};
-static void gpd_win4_init_ec(void)
+static void gpd_win4_init_ec(struct gpd_fan_data *data)
{
+ const struct gpd_fan_drvdata *drvdata = data->drvdata;
u8 chip_id, chip_ver;
- gpd_ecram_read(0x2000, &chip_id);
+ gpd_ecram_read(drvdata, 0x2000, &chip_id);
if (chip_id == 0x55) {
- gpd_ecram_read(0x1060, &chip_ver);
- gpd_ecram_write(0x1060, chip_ver | 0x80);
+ gpd_ecram_read(drvdata, 0x1060, &chip_ver);
+ gpd_ecram_write(drvdata, 0x1060, chip_ver | 0x80);
}
}
-static void gpd_init_ec(void)
+static void gpd_init_ec(struct gpd_fan_data *data)
{
// The buggy firmware won't initialize EC properly on boot.
// Before its initialization, reading RPM will always return 0,
// and writing PWM will have no effect.
// Initialize it manually on driver load.
- if (gpd_driver_priv.drvdata->board == win4_6800u)
- gpd_win4_init_ec();
+ if (data->drvdata->board == win4_6800u)
+ gpd_win4_init_ec(data);
}
static int gpd_fan_probe(struct platform_device *pdev)
@@ -611,7 +614,9 @@ static int gpd_fan_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
const struct resource *region;
const struct resource *res;
- const struct device *hwdev;
+ struct device *hwdev;
+ struct gpd_fan_data *data;
+ const struct gpd_fan_drvdata **match_ptr;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (!res)
@@ -624,24 +629,42 @@ static int gpd_fan_probe(struct platform_device *pdev)
return dev_err_probe(dev, -EBUSY,
"Failed to request region\n");
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ match_ptr = dev_get_platdata(dev);
+ if (!match_ptr)
+ return -EINVAL;
+ data->drvdata = *match_ptr;
+
+ data->pwm_enable = AUTOMATIC;
+ data->pwm_value = 255;
+
+ dev_set_drvdata(dev, data);
+
+ gpd_init_ec(data);
+
hwdev = devm_hwmon_device_register_with_info(dev,
DRIVER_NAME,
- NULL,
+ data,
&gpd_fan_chip_info,
NULL);
if (IS_ERR(hwdev))
return dev_err_probe(dev, PTR_ERR(hwdev),
"Failed to register hwmon device\n");
- gpd_init_ec();
-
return 0;
}
-static void gpd_fan_remove(__always_unused struct platform_device *pdev)
+static void gpd_fan_remove(struct platform_device *pdev)
{
- gpd_driver_priv.pwm_enable = AUTOMATIC;
- gpd_set_pwm_enable(AUTOMATIC);
+ struct gpd_fan_data *data = dev_get_drvdata(&pdev->dev);
+
+ if (data) {
+ data->pwm_enable = AUTOMATIC;
+ gpd_set_pwm_enable(data, AUTOMATIC);
+ }
}
static struct platform_driver gpd_fan_driver = {
@@ -668,6 +691,7 @@ static int __init gpd_fan_init(void)
if (!match) {
const struct dmi_system_id *dmi_match =
dmi_first_match(dmi_table);
+
if (dmi_match)
match = dmi_match->driver_data;
}
@@ -675,10 +699,6 @@ static int __init gpd_fan_init(void)
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,
@@ -690,7 +710,8 @@ static int __init gpd_fan_init(void)
gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
gpd_fan_probe,
gpd_fan_resources,
- 1, NULL, 0);
+ 1,
+ &match, sizeof(match));
if (IS_ERR(gpd_fan_platform_device)) {
pr_warn("Failed to create platform device\n");
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] hwmon: (gpd-fan): upgrade log level from warn to err for platform device creation failure
2026-04-05 8:40 [PATCH 0/3] cleanups in hwmon gpd-fan Pei Xiao
2026-04-05 8:40 ` [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv Pei Xiao
@ 2026-04-05 8:40 ` Pei Xiao
2026-04-05 8:40 ` [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style Pei Xiao
2 siblings, 0 replies; 9+ messages in thread
From: Pei Xiao @ 2026-04-05 8:40 UTC (permalink / raw)
To: cryolitia, linux, linux-hwmon, linux-kernel; +Cc: Pei Xiao
When platform_create_bundle() fails, the error is fatal and prevents the
driver from loading. Use pr_err() instead of pr_warn() to clearly indicate
a critical failure.
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
drivers/hwmon/gpd-fan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
index 5a9d07cd29ab..7df46027177d 100644
--- a/drivers/hwmon/gpd-fan.c
+++ b/drivers/hwmon/gpd-fan.c
@@ -714,7 +714,7 @@ static int __init gpd_fan_init(void)
&match, sizeof(match));
if (IS_ERR(gpd_fan_platform_device)) {
- pr_warn("Failed to create platform device\n");
+ pr_err("Failed to create platform device\n");
return PTR_ERR(gpd_fan_platform_device);
}
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style
2026-04-05 8:40 [PATCH 0/3] cleanups in hwmon gpd-fan Pei Xiao
2026-04-05 8:40 ` [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv Pei Xiao
2026-04-05 8:40 ` [PATCH 2/3] hwmon: (gpd-fan): upgrade log level from warn to err for platform device creation failure Pei Xiao
@ 2026-04-05 8:40 ` Pei Xiao
2026-04-05 13:53 ` Guenter Roeck
2 siblings, 1 reply; 9+ messages in thread
From: Pei Xiao @ 2026-04-05 8:40 UTC (permalink / raw)
To: cryolitia, linux, linux-hwmon, linux-kernel; +Cc: Pei Xiao
Kernel coding style requires enum labels to be capitalized. Convert all
enum gpd_board constants from lowercase to uppercase.
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
drivers/hwmon/gpd-fan.c | 62 ++++++++++++++++++++---------------------
1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
index 7df46027177d..404155dd97bb 100644
--- a/drivers/hwmon/gpd-fan.c
+++ b/drivers/hwmon/gpd-fan.c
@@ -27,11 +27,11 @@ static char *gpd_fan_board = "";
module_param(gpd_fan_board, charp, 0444);
enum gpd_board {
- win_mini,
- win4_6800u,
- win_max_2,
- duo,
- mpc2,
+ GPD_WIN_MINI,
+ GPD_WIN4_6800U,
+ GPD_WIN_MAX_2,
+ GPD_DUO,
+ GPD_MPC2,
};
enum FAN_PWM_ENABLE {
@@ -60,7 +60,7 @@ struct gpd_fan_drvdata {
static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
.board_name = "win_mini",
- .board = win_mini,
+ .board = GPD_WIN_MINI,
.addr_port = 0x4E,
.data_port = 0x4F,
@@ -72,7 +72,7 @@ static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
static struct gpd_fan_drvdata gpd_duo_drvdata = {
.board_name = "duo",
- .board = duo,
+ .board = GPD_DUO,
.addr_port = 0x4E,
.data_port = 0x4F,
@@ -84,7 +84,7 @@ static struct gpd_fan_drvdata gpd_duo_drvdata = {
static struct gpd_fan_drvdata gpd_win4_drvdata = {
.board_name = "win4",
- .board = win4_6800u,
+ .board = GPD_WIN4_6800U,
.addr_port = 0x2E,
.data_port = 0x2F,
@@ -96,7 +96,7 @@ static struct gpd_fan_drvdata gpd_win4_drvdata = {
static struct gpd_fan_drvdata gpd_wm2_drvdata = {
.board_name = "wm2",
- .board = win_max_2,
+ .board = GPD_WIN_MAX_2,
.addr_port = 0x4E,
.data_port = 0x4F,
@@ -108,7 +108,7 @@ static struct gpd_fan_drvdata gpd_wm2_drvdata = {
static struct gpd_fan_drvdata gpd_mpc2_drvdata = {
.board_name = "mpc2",
- .board = mpc2,
+ .board = GPD_MPC2,
.addr_port = 0x4E,
.data_port = 0x4F,
@@ -322,12 +322,12 @@ static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
static int gpd_read_rpm(struct gpd_fan_data *data)
{
switch (data->drvdata->board) {
- case win4_6800u:
- case win_mini:
- case duo:
- case mpc2:
+ case GPD_WIN4_6800U:
+ case GPD_WIN_MINI:
+ case GPD_DUO:
+ case GPD_MPC2:
return gpd_generic_read_rpm(data);
- case win_max_2:
+ case GPD_WIN_MAX_2:
return gpd_wm2_read_rpm(data);
}
@@ -349,10 +349,10 @@ static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
static int gpd_read_pwm(struct gpd_fan_data *data)
{
switch (data->drvdata->board) {
- case win_mini:
- case duo:
- case win4_6800u:
- case mpc2:
+ case GPD_WIN_MINI:
+ case GPD_DUO:
+ case GPD_WIN4_6800U:
+ case GPD_MPC2:
switch (data->pwm_enable) {
case DISABLE:
return 255;
@@ -362,7 +362,7 @@ static int gpd_read_pwm(struct gpd_fan_data *data)
return -EOPNOTSUPP;
}
break;
- case win_max_2:
+ case GPD_WIN_MAX_2:
return gpd_wm2_read_pwm(data);
}
return 0;
@@ -400,13 +400,13 @@ static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
return -EPERM;
switch (data->drvdata->board) {
- case duo:
+ case GPD_DUO:
gpd_duo_write_pwm(data, val);
break;
- case win_mini:
- case win4_6800u:
- case win_max_2:
- case mpc2:
+ case GPD_WIN_MINI:
+ case GPD_WIN4_6800U:
+ case GPD_WIN_MAX_2:
+ case GPD_MPC2:
gpd_generic_write_pwm(data, val);
break;
}
@@ -472,15 +472,15 @@ static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE en
data->pwm_value = 255;
switch (data->drvdata->board) {
- case win_mini:
- case win4_6800u:
- case mpc2:
+ case GPD_WIN_MINI:
+ case GPD_WIN4_6800U:
+ case GPD_MPC2:
gpd_win_mini_set_pwm_enable(data, enable);
break;
- case duo:
+ case GPD_DUO:
gpd_duo_set_pwm_enable(data, enable);
break;
- case win_max_2:
+ case GPD_WIN_MAX_2:
gpd_wm2_set_pwm_enable(data, enable);
break;
}
@@ -605,7 +605,7 @@ static void gpd_init_ec(struct gpd_fan_data *data)
// Before its initialization, reading RPM will always return 0,
// and writing PWM will have no effect.
// Initialize it manually on driver load.
- if (data->drvdata->board == win4_6800u)
+ if (data->drvdata->board == GPD_WIN4_6800U)
gpd_win4_init_ec(data);
}
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style
2026-04-05 8:40 ` [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style Pei Xiao
@ 2026-04-05 13:53 ` Guenter Roeck
2026-04-06 0:58 ` Pei Xiao
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2026-04-05 13:53 UTC (permalink / raw)
To: Pei Xiao, cryolitia, linux-hwmon, linux-kernel
On 4/5/26 01:40, Pei Xiao wrote:
> Kernel coding style requires enum labels to be capitalized. Convert all
> enum gpd_board constants from lowercase to uppercase.
>
While this may be the case, the hardware monitoring subsystem traditionally
uses lowercase for chip/board/system types. Changing that would be a
tremendous task, disrupting, confusing, create a flurry of patches, make
back-ports more difficult, and it would be completely unnecessary.
NACK.
Guenter
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
> drivers/hwmon/gpd-fan.c | 62 ++++++++++++++++++++---------------------
> 1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
> index 7df46027177d..404155dd97bb 100644
> --- a/drivers/hwmon/gpd-fan.c
> +++ b/drivers/hwmon/gpd-fan.c
> @@ -27,11 +27,11 @@ static char *gpd_fan_board = "";
> module_param(gpd_fan_board, charp, 0444);
>
> enum gpd_board {
> - win_mini,
> - win4_6800u,
> - win_max_2,
> - duo,
> - mpc2,
> + GPD_WIN_MINI,
> + GPD_WIN4_6800U,
> + GPD_WIN_MAX_2,
> + GPD_DUO,
> + GPD_MPC2,
> };
>
> enum FAN_PWM_ENABLE {
> @@ -60,7 +60,7 @@ struct gpd_fan_drvdata {
>
> static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
> .board_name = "win_mini",
> - .board = win_mini,
> + .board = GPD_WIN_MINI,
>
> .addr_port = 0x4E,
> .data_port = 0x4F,
> @@ -72,7 +72,7 @@ static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
>
> static struct gpd_fan_drvdata gpd_duo_drvdata = {
> .board_name = "duo",
> - .board = duo,
> + .board = GPD_DUO,
>
> .addr_port = 0x4E,
> .data_port = 0x4F,
> @@ -84,7 +84,7 @@ static struct gpd_fan_drvdata gpd_duo_drvdata = {
>
> static struct gpd_fan_drvdata gpd_win4_drvdata = {
> .board_name = "win4",
> - .board = win4_6800u,
> + .board = GPD_WIN4_6800U,
>
> .addr_port = 0x2E,
> .data_port = 0x2F,
> @@ -96,7 +96,7 @@ static struct gpd_fan_drvdata gpd_win4_drvdata = {
>
> static struct gpd_fan_drvdata gpd_wm2_drvdata = {
> .board_name = "wm2",
> - .board = win_max_2,
> + .board = GPD_WIN_MAX_2,
>
> .addr_port = 0x4E,
> .data_port = 0x4F,
> @@ -108,7 +108,7 @@ static struct gpd_fan_drvdata gpd_wm2_drvdata = {
>
> static struct gpd_fan_drvdata gpd_mpc2_drvdata = {
> .board_name = "mpc2",
> - .board = mpc2,
> + .board = GPD_MPC2,
>
> .addr_port = 0x4E,
> .data_port = 0x4F,
> @@ -322,12 +322,12 @@ static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
> static int gpd_read_rpm(struct gpd_fan_data *data)
> {
> switch (data->drvdata->board) {
> - case win4_6800u:
> - case win_mini:
> - case duo:
> - case mpc2:
> + case GPD_WIN4_6800U:
> + case GPD_WIN_MINI:
> + case GPD_DUO:
> + case GPD_MPC2:
> return gpd_generic_read_rpm(data);
> - case win_max_2:
> + case GPD_WIN_MAX_2:
> return gpd_wm2_read_rpm(data);
> }
>
> @@ -349,10 +349,10 @@ static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
> static int gpd_read_pwm(struct gpd_fan_data *data)
> {
> switch (data->drvdata->board) {
> - case win_mini:
> - case duo:
> - case win4_6800u:
> - case mpc2:
> + case GPD_WIN_MINI:
> + case GPD_DUO:
> + case GPD_WIN4_6800U:
> + case GPD_MPC2:
> switch (data->pwm_enable) {
> case DISABLE:
> return 255;
> @@ -362,7 +362,7 @@ static int gpd_read_pwm(struct gpd_fan_data *data)
> return -EOPNOTSUPP;
> }
> break;
> - case win_max_2:
> + case GPD_WIN_MAX_2:
> return gpd_wm2_read_pwm(data);
> }
> return 0;
> @@ -400,13 +400,13 @@ static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
> return -EPERM;
>
> switch (data->drvdata->board) {
> - case duo:
> + case GPD_DUO:
> gpd_duo_write_pwm(data, val);
> break;
> - case win_mini:
> - case win4_6800u:
> - case win_max_2:
> - case mpc2:
> + case GPD_WIN_MINI:
> + case GPD_WIN4_6800U:
> + case GPD_WIN_MAX_2:
> + case GPD_MPC2:
> gpd_generic_write_pwm(data, val);
> break;
> }
> @@ -472,15 +472,15 @@ static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE en
> data->pwm_value = 255;
>
> switch (data->drvdata->board) {
> - case win_mini:
> - case win4_6800u:
> - case mpc2:
> + case GPD_WIN_MINI:
> + case GPD_WIN4_6800U:
> + case GPD_MPC2:
> gpd_win_mini_set_pwm_enable(data, enable);
> break;
> - case duo:
> + case GPD_DUO:
> gpd_duo_set_pwm_enable(data, enable);
> break;
> - case win_max_2:
> + case GPD_WIN_MAX_2:
> gpd_wm2_set_pwm_enable(data, enable);
> break;
> }
> @@ -605,7 +605,7 @@ static void gpd_init_ec(struct gpd_fan_data *data)
> // Before its initialization, reading RPM will always return 0,
> // and writing PWM will have no effect.
> // Initialize it manually on driver load.
> - if (data->drvdata->board == win4_6800u)
> + if (data->drvdata->board == GPD_WIN4_6800U)
> gpd_win4_init_ec(data);
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style
2026-04-05 13:53 ` Guenter Roeck
@ 2026-04-06 0:58 ` Pei Xiao
0 siblings, 0 replies; 9+ messages in thread
From: Pei Xiao @ 2026-04-06 0:58 UTC (permalink / raw)
To: Guenter Roeck, cryolitia, linux-hwmon, linux-kernel
在 2026/4/5 21:53, Guenter Roeck 写道:
> On 4/5/26 01:40, Pei Xiao wrote:
>> Kernel coding style requires enum labels to be capitalized. Convert all
>> enum gpd_board constants from lowercase to uppercase.
>>
>
> While this may be the case, the hardware monitoring subsystem traditionally
> uses lowercase for chip/board/system types. Changing that would be a
> tremendous task, disrupting, confusing, create a flurry of patches, make
> back-ports more difficult, and it would be completely unnecessary.
>
Yes, follow history. This will waste everyone's time.
> NACK.
>
> Guenter
>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>> drivers/hwmon/gpd-fan.c | 62 ++++++++++++++++++++---------------------
>> 1 file changed, 31 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
>> index 7df46027177d..404155dd97bb 100644
>> --- a/drivers/hwmon/gpd-fan.c
>> +++ b/drivers/hwmon/gpd-fan.c
>> @@ -27,11 +27,11 @@ static char *gpd_fan_board = "";
>> module_param(gpd_fan_board, charp, 0444);
>> enum gpd_board {
>> - win_mini,
>> - win4_6800u,
>> - win_max_2,
>> - duo,
>> - mpc2,
>> + GPD_WIN_MINI,
>> + GPD_WIN4_6800U,
>> + GPD_WIN_MAX_2,
>> + GPD_DUO,
>> + GPD_MPC2,
>> };
>> enum FAN_PWM_ENABLE {
>> @@ -60,7 +60,7 @@ struct gpd_fan_drvdata {
>> static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
>> .board_name = "win_mini",
>> - .board = win_mini,
>> + .board = GPD_WIN_MINI,
>> .addr_port = 0x4E,
>> .data_port = 0x4F,
>> @@ -72,7 +72,7 @@ static struct gpd_fan_drvdata gpd_win_mini_drvdata = {
>> static struct gpd_fan_drvdata gpd_duo_drvdata = {
>> .board_name = "duo",
>> - .board = duo,
>> + .board = GPD_DUO,
>> .addr_port = 0x4E,
>> .data_port = 0x4F,
>> @@ -84,7 +84,7 @@ static struct gpd_fan_drvdata gpd_duo_drvdata = {
>> static struct gpd_fan_drvdata gpd_win4_drvdata = {
>> .board_name = "win4",
>> - .board = win4_6800u,
>> + .board = GPD_WIN4_6800U,
>> .addr_port = 0x2E,
>> .data_port = 0x2F,
>> @@ -96,7 +96,7 @@ static struct gpd_fan_drvdata gpd_win4_drvdata = {
>> static struct gpd_fan_drvdata gpd_wm2_drvdata = {
>> .board_name = "wm2",
>> - .board = win_max_2,
>> + .board = GPD_WIN_MAX_2,
>> .addr_port = 0x4E,
>> .data_port = 0x4F,
>> @@ -108,7 +108,7 @@ static struct gpd_fan_drvdata gpd_wm2_drvdata = {
>> static struct gpd_fan_drvdata gpd_mpc2_drvdata = {
>> .board_name = "mpc2",
>> - .board = mpc2,
>> + .board = GPD_MPC2,
>> .addr_port = 0x4E,
>> .data_port = 0x4F,
>> @@ -322,12 +322,12 @@ static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
>> static int gpd_read_rpm(struct gpd_fan_data *data)
>> {
>> switch (data->drvdata->board) {
>> - case win4_6800u:
>> - case win_mini:
>> - case duo:
>> - case mpc2:
>> + case GPD_WIN4_6800U:
>> + case GPD_WIN_MINI:
>> + case GPD_DUO:
>> + case GPD_MPC2:
>> return gpd_generic_read_rpm(data);
>> - case win_max_2:
>> + case GPD_WIN_MAX_2:
>> return gpd_wm2_read_rpm(data);
>> }
>> @@ -349,10 +349,10 @@ static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
>> static int gpd_read_pwm(struct gpd_fan_data *data)
>> {
>> switch (data->drvdata->board) {
>> - case win_mini:
>> - case duo:
>> - case win4_6800u:
>> - case mpc2:
>> + case GPD_WIN_MINI:
>> + case GPD_DUO:
>> + case GPD_WIN4_6800U:
>> + case GPD_MPC2:
>> switch (data->pwm_enable) {
>> case DISABLE:
>> return 255;
>> @@ -362,7 +362,7 @@ static int gpd_read_pwm(struct gpd_fan_data *data)
>> return -EOPNOTSUPP;
>> }
>> break;
>> - case win_max_2:
>> + case GPD_WIN_MAX_2:
>> return gpd_wm2_read_pwm(data);
>> }
>> return 0;
>> @@ -400,13 +400,13 @@ static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
>> return -EPERM;
>> switch (data->drvdata->board) {
>> - case duo:
>> + case GPD_DUO:
>> gpd_duo_write_pwm(data, val);
>> break;
>> - case win_mini:
>> - case win4_6800u:
>> - case win_max_2:
>> - case mpc2:
>> + case GPD_WIN_MINI:
>> + case GPD_WIN4_6800U:
>> + case GPD_WIN_MAX_2:
>> + case GPD_MPC2:
>> gpd_generic_write_pwm(data, val);
>> break;
>> }
>> @@ -472,15 +472,15 @@ static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE en
>> data->pwm_value = 255;
>> switch (data->drvdata->board) {
>> - case win_mini:
>> - case win4_6800u:
>> - case mpc2:
>> + case GPD_WIN_MINI:
>> + case GPD_WIN4_6800U:
>> + case GPD_MPC2:
>> gpd_win_mini_set_pwm_enable(data, enable);
>> break;
>> - case duo:
>> + case GPD_DUO:
>> gpd_duo_set_pwm_enable(data, enable);
>> break;
>> - case win_max_2:
>> + case GPD_WIN_MAX_2:
>> gpd_wm2_set_pwm_enable(data, enable);
>> break;
>> }
>> @@ -605,7 +605,7 @@ static void gpd_init_ec(struct gpd_fan_data *data)
>> // Before its initialization, reading RPM will always return 0,
>> // and writing PWM will have no effect.
>> // Initialize it manually on driver load.
>> - if (data->drvdata->board == win4_6800u)
>> + if (data->drvdata->board == GPD_WIN4_6800U)
>> gpd_win4_init_ec(data);
>> }
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv
2026-04-05 8:40 ` [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv Pei Xiao
@ 2026-05-22 5:50 ` Pei Xiao
2026-05-22 13:52 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Pei Xiao @ 2026-05-22 5:50 UTC (permalink / raw)
To: cryolitia, linux, linux-hwmon, linux-kernel
After all, using the global variable |gpd_driver_priv|
does not comply with driver development specifications.
Friendly ping.
Pei.
Thanks!
在 2026/4/5 16:40, Pei Xiao 写道:
> Replace the global state gpd_driver_priv with per-device private data
> (struct gpd_fan_data) allocated in probe. This allows the driver to
> support multiple instances in the future and aligns with kernel best
> practices.
>
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
> drivers/hwmon/gpd-fan.c | 211 ++++++++++++++++++++++------------------
> 1 file changed, 116 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
> index 80de5f20781e..5a9d07cd29ab 100644
> --- a/drivers/hwmon/gpd-fan.c
> +++ b/drivers/hwmon/gpd-fan.c
> @@ -40,12 +40,11 @@ enum FAN_PWM_ENABLE {
> AUTOMATIC = 2,
> };
>
> -static struct {
> +struct gpd_fan_data {
> 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
> @@ -249,10 +248,10 @@ static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
> };
>
> // Helper functions to handle EC read/write
> -static void gpd_ecram_read(u16 offset, u8 *val)
> +static void gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 *val)
> {
> - u16 addr_port = gpd_driver_priv.drvdata->addr_port;
> - u16 data_port = gpd_driver_priv.drvdata->data_port;
> + u16 addr_port = drvdata->addr_port;
> + u16 data_port = drvdata->data_port;
>
> outb(0x2E, addr_port);
> outb(0x11, data_port);
> @@ -270,10 +269,10 @@ static void gpd_ecram_read(u16 offset, u8 *val)
> *val = inb(data_port);
> }
>
> -static void gpd_ecram_write(u16 offset, u8 value)
> +static void gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 value)
> {
> - u16 addr_port = gpd_driver_priv.drvdata->addr_port;
> - u16 data_port = gpd_driver_priv.drvdata->data_port;
> + u16 addr_port = drvdata->addr_port;
> + u16 data_port = drvdata->data_port;
>
> outb(0x2E, addr_port);
> outb(0x11, data_port);
> @@ -291,198 +290,198 @@ static void gpd_ecram_write(u16 offset, u8 value)
> outb(value, data_port);
> }
>
> -static int gpd_generic_read_rpm(void)
> +static int gpd_generic_read_rpm(struct gpd_fan_data *data)
> {
> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
> u8 high, low;
>
> - gpd_ecram_read(drvdata->rpm_read, &high);
> - gpd_ecram_read(drvdata->rpm_read + 1, &low);
> + gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
> + gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
>
> return (u16)high << 8 | low;
> }
>
> -static int gpd_wm2_read_rpm(void)
> +static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
> {
> + const struct gpd_fan_drvdata *drvdata = data->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(pwm_ctr_offset, &PWMCTR);
> + gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
>
> if (PWMCTR != 0xB8)
> - gpd_ecram_write(pwm_ctr_offset, 0xB8);
> + gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
> }
>
> - return gpd_generic_read_rpm();
> + return gpd_generic_read_rpm(data);
> }
>
> // Read value for fan1_input
> -static int gpd_read_rpm(void)
> +static int gpd_read_rpm(struct gpd_fan_data *data)
> {
> - switch (gpd_driver_priv.drvdata->board) {
> + switch (data->drvdata->board) {
> case win4_6800u:
> case win_mini:
> case duo:
> case mpc2:
> - return gpd_generic_read_rpm();
> + return gpd_generic_read_rpm(data);
> case win_max_2:
> - return gpd_wm2_read_rpm();
> + return gpd_wm2_read_rpm(data);
> }
>
> return 0;
> }
>
> -static int gpd_wm2_read_pwm(void)
> +static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
> {
> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
> u8 var;
>
> - gpd_ecram_read(drvdata->pwm_write, &var);
> + gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
>
> // Match gpd_generic_write_pwm(u8) below
> return DIV_ROUND_CLOSEST((var - 1) * 255, (drvdata->pwm_max - 1));
> }
>
> // Read value for pwm1
> -static int gpd_read_pwm(void)
> +static int gpd_read_pwm(struct gpd_fan_data *data)
> {
> - switch (gpd_driver_priv.drvdata->board) {
> + switch (data->drvdata->board) {
> case win_mini:
> case duo:
> case win4_6800u:
> case mpc2:
> - switch (gpd_driver_priv.pwm_enable) {
> + switch (data->pwm_enable) {
> case DISABLE:
> return 255;
> case MANUAL:
> - return gpd_driver_priv.pwm_value;
> + return data->pwm_value;
> case AUTOMATIC:
> return -EOPNOTSUPP;
> }
> break;
> case win_max_2:
> - return gpd_wm2_read_pwm();
> + return gpd_wm2_read_pwm(data);
> }
> return 0;
> }
>
> // PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
> -static inline u8 gpd_cast_pwm_range(u8 val)
> +static inline u8 gpd_cast_pwm_range(const struct gpd_fan_drvdata *drvdata, u8 val)
> {
> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> -
> return DIV_ROUND_CLOSEST(val * (drvdata->pwm_max - 1), 255) + 1;
> }
>
> -static void gpd_generic_write_pwm(u8 val)
> +static void gpd_generic_write_pwm(struct gpd_fan_data *data, u8 val)
> {
> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
> u8 pwm_reg;
>
> - pwm_reg = gpd_cast_pwm_range(val);
> - gpd_ecram_write(drvdata->pwm_write, pwm_reg);
> + pwm_reg = gpd_cast_pwm_range(drvdata, val);
> + gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
> }
>
> -static void gpd_duo_write_pwm(u8 val)
> +static void gpd_duo_write_pwm(struct gpd_fan_data *data, u8 val)
> {
> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
> u8 pwm_reg;
>
> - pwm_reg = gpd_cast_pwm_range(val);
> - gpd_ecram_write(drvdata->pwm_write, pwm_reg);
> - gpd_ecram_write(drvdata->pwm_write + 1, pwm_reg);
> + pwm_reg = gpd_cast_pwm_range(drvdata, val);
> + gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
> + gpd_ecram_write(drvdata, drvdata->pwm_write + 1, pwm_reg);
> }
>
> // Write value for pwm1
> -static int gpd_write_pwm(u8 val)
> +static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
> {
> - if (gpd_driver_priv.pwm_enable != MANUAL)
> + if (data->pwm_enable != MANUAL)
> return -EPERM;
>
> - switch (gpd_driver_priv.drvdata->board) {
> + switch (data->drvdata->board) {
> case duo:
> - gpd_duo_write_pwm(val);
> + gpd_duo_write_pwm(data, val);
> break;
> case win_mini:
> case win4_6800u:
> case win_max_2:
> case mpc2:
> - gpd_generic_write_pwm(val);
> + gpd_generic_write_pwm(data, val);
> break;
> }
>
> return 0;
> }
>
> -static void gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
> +static void gpd_win_mini_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
> {
> switch (pwm_enable) {
> case DISABLE:
> - gpd_generic_write_pwm(255);
> + gpd_generic_write_pwm(data, 255);
> break;
> case MANUAL:
> - gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
> + gpd_generic_write_pwm(data, data->pwm_value);
> break;
> case AUTOMATIC:
> - gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
> + gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
> break;
> }
> }
>
> -static void gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
> +static void gpd_duo_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
> {
> switch (pwm_enable) {
> case DISABLE:
> - gpd_duo_write_pwm(255);
> + gpd_duo_write_pwm(data, 255);
> break;
> case MANUAL:
> - gpd_duo_write_pwm(gpd_driver_priv.pwm_value);
> + gpd_duo_write_pwm(data, data->pwm_value);
> break;
> case AUTOMATIC:
> - gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
> + gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
> break;
> }
> }
>
> -static void gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
> +static void gpd_wm2_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
> {
> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>
> switch (enable) {
> case DISABLE:
> - gpd_generic_write_pwm(255);
> - gpd_ecram_write(drvdata->manual_control_enable, 1);
> + gpd_generic_write_pwm(data, 255);
> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
> break;
> case MANUAL:
> - gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
> - gpd_ecram_write(drvdata->manual_control_enable, 1);
> + gpd_generic_write_pwm(data, data->pwm_value);
> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
> break;
> case AUTOMATIC:
> - gpd_ecram_write(drvdata->manual_control_enable, 0);
> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 0);
> break;
> }
> }
>
> // Write value for pwm1_enable
> -static void gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
> +static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
> {
> if (enable == MANUAL)
> // Set pwm_value to max firstly when switching to manual mode, in
> // consideration of device safety.
> - gpd_driver_priv.pwm_value = 255;
> + data->pwm_value = 255;
>
> - switch (gpd_driver_priv.drvdata->board) {
> + switch (data->drvdata->board) {
> case win_mini:
> case win4_6800u:
> case mpc2:
> - gpd_win_mini_set_pwm_enable(enable);
> + gpd_win_mini_set_pwm_enable(data, enable);
> break;
> case duo:
> - gpd_duo_set_pwm_enable(enable);
> + gpd_duo_set_pwm_enable(data, enable);
> break;
> case win_max_2:
> - gpd_wm2_set_pwm_enable(enable);
> + gpd_wm2_set_pwm_enable(data, enable);
> break;
> }
> }
> @@ -505,15 +504,16 @@ static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
> return 0;
> }
>
> -static int gpd_fan_hwmon_read(__always_unused struct device *dev,
> +static int gpd_fan_hwmon_read(struct device *dev,
> enum hwmon_sensor_types type, u32 attr,
> __always_unused int channel, long *val)
> {
> + struct gpd_fan_data *data = dev_get_drvdata(dev);
> int ret;
>
> if (type == hwmon_fan) {
> if (attr == hwmon_fan_input) {
> - ret = gpd_read_rpm();
> + ret = gpd_read_rpm(data);
>
> if (ret < 0)
> return ret;
> @@ -524,10 +524,10 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
> } else if (type == hwmon_pwm) {
> switch (attr) {
> case hwmon_pwm_enable:
> - *val = gpd_driver_priv.pwm_enable;
> + *val = data->pwm_enable;
> return 0;
> case hwmon_pwm_input:
> - ret = gpd_read_pwm();
> + ret = gpd_read_pwm(data);
>
> if (ret < 0)
> return ret;
> @@ -540,27 +540,29 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
> return -EOPNOTSUPP;
> }
>
> -static int gpd_fan_hwmon_write(__always_unused struct device *dev,
> +static int gpd_fan_hwmon_write(struct device *dev,
> enum hwmon_sensor_types type, u32 attr,
> __always_unused int channel, long val)
> {
> + struct gpd_fan_data *data = dev_get_drvdata(dev);
> +
> if (type == hwmon_pwm) {
> switch (attr) {
> case hwmon_pwm_enable:
> if (!in_range(val, 0, 3))
> return -EINVAL;
>
> - gpd_driver_priv.pwm_enable = val;
> + data->pwm_enable = val;
>
> - gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
> + gpd_set_pwm_enable(data, data->pwm_enable);
> return 0;
> case hwmon_pwm_input:
> if (!in_range(val, 0, 256))
> return -EINVAL;
>
> - gpd_driver_priv.pwm_value = val;
> + data->pwm_value = val;
>
> - return gpd_write_pwm(val);
> + return gpd_write_pwm(data, val);
> }
> }
>
> @@ -584,26 +586,27 @@ static struct hwmon_chip_info gpd_fan_chip_info = {
> .info = gpd_fan_hwmon_channel_info
> };
>
> -static void gpd_win4_init_ec(void)
> +static void gpd_win4_init_ec(struct gpd_fan_data *data)
> {
> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
> u8 chip_id, chip_ver;
>
> - gpd_ecram_read(0x2000, &chip_id);
> + gpd_ecram_read(drvdata, 0x2000, &chip_id);
>
> if (chip_id == 0x55) {
> - gpd_ecram_read(0x1060, &chip_ver);
> - gpd_ecram_write(0x1060, chip_ver | 0x80);
> + gpd_ecram_read(drvdata, 0x1060, &chip_ver);
> + gpd_ecram_write(drvdata, 0x1060, chip_ver | 0x80);
> }
> }
>
> -static void gpd_init_ec(void)
> +static void gpd_init_ec(struct gpd_fan_data *data)
> {
> // The buggy firmware won't initialize EC properly on boot.
> // Before its initialization, reading RPM will always return 0,
> // and writing PWM will have no effect.
> // Initialize it manually on driver load.
> - if (gpd_driver_priv.drvdata->board == win4_6800u)
> - gpd_win4_init_ec();
> + if (data->drvdata->board == win4_6800u)
> + gpd_win4_init_ec(data);
> }
>
> static int gpd_fan_probe(struct platform_device *pdev)
> @@ -611,7 +614,9 @@ static int gpd_fan_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> const struct resource *region;
> const struct resource *res;
> - const struct device *hwdev;
> + struct device *hwdev;
> + struct gpd_fan_data *data;
> + const struct gpd_fan_drvdata **match_ptr;
>
> res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> if (!res)
> @@ -624,24 +629,42 @@ static int gpd_fan_probe(struct platform_device *pdev)
> return dev_err_probe(dev, -EBUSY,
> "Failed to request region\n");
>
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + match_ptr = dev_get_platdata(dev);
> + if (!match_ptr)
> + return -EINVAL;
> + data->drvdata = *match_ptr;
> +
> + data->pwm_enable = AUTOMATIC;
> + data->pwm_value = 255;
> +
> + dev_set_drvdata(dev, data);
> +
> + gpd_init_ec(data);
> +
> hwdev = devm_hwmon_device_register_with_info(dev,
> DRIVER_NAME,
> - NULL,
> + data,
> &gpd_fan_chip_info,
> NULL);
> if (IS_ERR(hwdev))
> return dev_err_probe(dev, PTR_ERR(hwdev),
> "Failed to register hwmon device\n");
>
> - gpd_init_ec();
> -
> return 0;
> }
>
> -static void gpd_fan_remove(__always_unused struct platform_device *pdev)
> +static void gpd_fan_remove(struct platform_device *pdev)
> {
> - gpd_driver_priv.pwm_enable = AUTOMATIC;
> - gpd_set_pwm_enable(AUTOMATIC);
> + struct gpd_fan_data *data = dev_get_drvdata(&pdev->dev);
> +
> + if (data) {
> + data->pwm_enable = AUTOMATIC;
> + gpd_set_pwm_enable(data, AUTOMATIC);
> + }
> }
>
> static struct platform_driver gpd_fan_driver = {
> @@ -668,6 +691,7 @@ static int __init gpd_fan_init(void)
> if (!match) {
> const struct dmi_system_id *dmi_match =
> dmi_first_match(dmi_table);
> +
> if (dmi_match)
> match = dmi_match->driver_data;
> }
> @@ -675,10 +699,6 @@ static int __init gpd_fan_init(void)
> 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,
> @@ -690,7 +710,8 @@ static int __init gpd_fan_init(void)
> gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
> gpd_fan_probe,
> gpd_fan_resources,
> - 1, NULL, 0);
> + 1,
> + &match, sizeof(match));
>
> if (IS_ERR(gpd_fan_platform_device)) {
> pr_warn("Failed to create platform device\n");
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv
2026-05-22 5:50 ` Pei Xiao
@ 2026-05-22 13:52 ` Guenter Roeck
2026-05-27 8:06 ` Pei Xiao
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2026-05-22 13:52 UTC (permalink / raw)
To: Pei Xiao, cryolitia, linux-hwmon, linux-kernel
On 5/21/26 22:50, Pei Xiao wrote:
>
> After all, using the global variable |gpd_driver_priv|
> does not comply with driver development specifications.
>
I had marked this patch "changes requested", though I don't recall
the reason. Comment inline. Please fix and resubmit, together with
the next patch of the series but excluding the third patch.
Thanks,
Guenter
> Friendly ping.
>
> Pei.
> Thanks!
> 在 2026/4/5 16:40, Pei Xiao 写道:
>> Replace the global state gpd_driver_priv with per-device private data
>> (struct gpd_fan_data) allocated in probe. This allows the driver to
>> support multiple instances in the future and aligns with kernel best
>> practices.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>> drivers/hwmon/gpd-fan.c | 211 ++++++++++++++++++++++------------------
>> 1 file changed, 116 insertions(+), 95 deletions(-)
>>
>> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
>> index 80de5f20781e..5a9d07cd29ab 100644
>> --- a/drivers/hwmon/gpd-fan.c
>> +++ b/drivers/hwmon/gpd-fan.c
>> @@ -40,12 +40,11 @@ enum FAN_PWM_ENABLE {
>> AUTOMATIC = 2,
>> };
>>
>> -static struct {
>> +struct gpd_fan_data {
>> 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
>> @@ -249,10 +248,10 @@ static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
>> };
>>
>> // Helper functions to handle EC read/write
>> -static void gpd_ecram_read(u16 offset, u8 *val)
>> +static void gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 *val)
>> {
>> - u16 addr_port = gpd_driver_priv.drvdata->addr_port;
>> - u16 data_port = gpd_driver_priv.drvdata->data_port;
>> + u16 addr_port = drvdata->addr_port;
>> + u16 data_port = drvdata->data_port;
>>
>> outb(0x2E, addr_port);
>> outb(0x11, data_port);
>> @@ -270,10 +269,10 @@ static void gpd_ecram_read(u16 offset, u8 *val)
>> *val = inb(data_port);
>> }
>>
>> -static void gpd_ecram_write(u16 offset, u8 value)
>> +static void gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 value)
>> {
>> - u16 addr_port = gpd_driver_priv.drvdata->addr_port;
>> - u16 data_port = gpd_driver_priv.drvdata->data_port;
>> + u16 addr_port = drvdata->addr_port;
>> + u16 data_port = drvdata->data_port;
>>
>> outb(0x2E, addr_port);
>> outb(0x11, data_port);
>> @@ -291,198 +290,198 @@ static void gpd_ecram_write(u16 offset, u8 value)
>> outb(value, data_port);
>> }
>>
>> -static int gpd_generic_read_rpm(void)
>> +static int gpd_generic_read_rpm(struct gpd_fan_data *data)
>> {
>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>> u8 high, low;
>>
>> - gpd_ecram_read(drvdata->rpm_read, &high);
>> - gpd_ecram_read(drvdata->rpm_read + 1, &low);
>> + gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
>> + gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
>>
>> return (u16)high << 8 | low;
>> }
>>
>> -static int gpd_wm2_read_rpm(void)
>> +static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
>> {
>> + const struct gpd_fan_drvdata *drvdata = data->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(pwm_ctr_offset, &PWMCTR);
>> + gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
>>
>> if (PWMCTR != 0xB8)
>> - gpd_ecram_write(pwm_ctr_offset, 0xB8);
>> + gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
>> }
>>
>> - return gpd_generic_read_rpm();
>> + return gpd_generic_read_rpm(data);
>> }
>>
>> // Read value for fan1_input
>> -static int gpd_read_rpm(void)
>> +static int gpd_read_rpm(struct gpd_fan_data *data)
>> {
>> - switch (gpd_driver_priv.drvdata->board) {
>> + switch (data->drvdata->board) {
>> case win4_6800u:
>> case win_mini:
>> case duo:
>> case mpc2:
>> - return gpd_generic_read_rpm();
>> + return gpd_generic_read_rpm(data);
>> case win_max_2:
>> - return gpd_wm2_read_rpm();
>> + return gpd_wm2_read_rpm(data);
>> }
>>
>> return 0;
>> }
>>
>> -static int gpd_wm2_read_pwm(void)
>> +static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
>> {
>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>> u8 var;
>>
>> - gpd_ecram_read(drvdata->pwm_write, &var);
>> + gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
>>
>> // Match gpd_generic_write_pwm(u8) below
>> return DIV_ROUND_CLOSEST((var - 1) * 255, (drvdata->pwm_max - 1));
>> }
>>
>> // Read value for pwm1
>> -static int gpd_read_pwm(void)
>> +static int gpd_read_pwm(struct gpd_fan_data *data)
>> {
>> - switch (gpd_driver_priv.drvdata->board) {
>> + switch (data->drvdata->board) {
>> case win_mini:
>> case duo:
>> case win4_6800u:
>> case mpc2:
>> - switch (gpd_driver_priv.pwm_enable) {
>> + switch (data->pwm_enable) {
>> case DISABLE:
>> return 255;
>> case MANUAL:
>> - return gpd_driver_priv.pwm_value;
>> + return data->pwm_value;
>> case AUTOMATIC:
>> return -EOPNOTSUPP;
>> }
>> break;
>> case win_max_2:
>> - return gpd_wm2_read_pwm();
>> + return gpd_wm2_read_pwm(data);
>> }
>> return 0;
>> }
>>
>> // PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
>> -static inline u8 gpd_cast_pwm_range(u8 val)
>> +static inline u8 gpd_cast_pwm_range(const struct gpd_fan_drvdata *drvdata, u8 val)
>> {
>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> -
>> return DIV_ROUND_CLOSEST(val * (drvdata->pwm_max - 1), 255) + 1;
>> }
>>
>> -static void gpd_generic_write_pwm(u8 val)
>> +static void gpd_generic_write_pwm(struct gpd_fan_data *data, u8 val)
>> {
>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>> u8 pwm_reg;
>>
>> - pwm_reg = gpd_cast_pwm_range(val);
>> - gpd_ecram_write(drvdata->pwm_write, pwm_reg);
>> + pwm_reg = gpd_cast_pwm_range(drvdata, val);
>> + gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
>> }
>>
>> -static void gpd_duo_write_pwm(u8 val)
>> +static void gpd_duo_write_pwm(struct gpd_fan_data *data, u8 val)
>> {
>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>> u8 pwm_reg;
>>
>> - pwm_reg = gpd_cast_pwm_range(val);
>> - gpd_ecram_write(drvdata->pwm_write, pwm_reg);
>> - gpd_ecram_write(drvdata->pwm_write + 1, pwm_reg);
>> + pwm_reg = gpd_cast_pwm_range(drvdata, val);
>> + gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
>> + gpd_ecram_write(drvdata, drvdata->pwm_write + 1, pwm_reg);
>> }
>>
>> // Write value for pwm1
>> -static int gpd_write_pwm(u8 val)
>> +static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
>> {
>> - if (gpd_driver_priv.pwm_enable != MANUAL)
>> + if (data->pwm_enable != MANUAL)
>> return -EPERM;
>>
>> - switch (gpd_driver_priv.drvdata->board) {
>> + switch (data->drvdata->board) {
>> case duo:
>> - gpd_duo_write_pwm(val);
>> + gpd_duo_write_pwm(data, val);
>> break;
>> case win_mini:
>> case win4_6800u:
>> case win_max_2:
>> case mpc2:
>> - gpd_generic_write_pwm(val);
>> + gpd_generic_write_pwm(data, val);
>> break;
>> }
>>
>> return 0;
>> }
>>
>> -static void gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>> +static void gpd_win_mini_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
>> {
>> switch (pwm_enable) {
>> case DISABLE:
>> - gpd_generic_write_pwm(255);
>> + gpd_generic_write_pwm(data, 255);
>> break;
>> case MANUAL:
>> - gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
>> + gpd_generic_write_pwm(data, data->pwm_value);
>> break;
>> case AUTOMATIC:
>> - gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
>> + gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
>> break;
>> }
>> }
>>
>> -static void gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>> +static void gpd_duo_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
>> {
>> switch (pwm_enable) {
>> case DISABLE:
>> - gpd_duo_write_pwm(255);
>> + gpd_duo_write_pwm(data, 255);
>> break;
>> case MANUAL:
>> - gpd_duo_write_pwm(gpd_driver_priv.pwm_value);
>> + gpd_duo_write_pwm(data, data->pwm_value);
>> break;
>> case AUTOMATIC:
>> - gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
>> + gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
>> break;
>> }
>> }
>>
>> -static void gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
>> +static void gpd_wm2_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
>> {
>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>
>> switch (enable) {
>> case DISABLE:
>> - gpd_generic_write_pwm(255);
>> - gpd_ecram_write(drvdata->manual_control_enable, 1);
>> + gpd_generic_write_pwm(data, 255);
>> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
>> break;
>> case MANUAL:
>> - gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
>> - gpd_ecram_write(drvdata->manual_control_enable, 1);
>> + gpd_generic_write_pwm(data, data->pwm_value);
>> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
>> break;
>> case AUTOMATIC:
>> - gpd_ecram_write(drvdata->manual_control_enable, 0);
>> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 0);
>> break;
>> }
>> }
>>
>> // Write value for pwm1_enable
>> -static void gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
>> +static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
>> {
>> if (enable == MANUAL)
>> // Set pwm_value to max firstly when switching to manual mode, in
>> // consideration of device safety.
>> - gpd_driver_priv.pwm_value = 255;
>> + data->pwm_value = 255;
>>
>> - switch (gpd_driver_priv.drvdata->board) {
>> + switch (data->drvdata->board) {
>> case win_mini:
>> case win4_6800u:
>> case mpc2:
>> - gpd_win_mini_set_pwm_enable(enable);
>> + gpd_win_mini_set_pwm_enable(data, enable);
>> break;
>> case duo:
>> - gpd_duo_set_pwm_enable(enable);
>> + gpd_duo_set_pwm_enable(data, enable);
>> break;
>> case win_max_2:
>> - gpd_wm2_set_pwm_enable(enable);
>> + gpd_wm2_set_pwm_enable(data, enable);
>> break;
>> }
>> }
>> @@ -505,15 +504,16 @@ static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
>> return 0;
>> }
>>
>> -static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>> +static int gpd_fan_hwmon_read(struct device *dev,
>> enum hwmon_sensor_types type, u32 attr,
>> __always_unused int channel, long *val)
>> {
>> + struct gpd_fan_data *data = dev_get_drvdata(dev);
>> int ret;
>>
>> if (type == hwmon_fan) {
>> if (attr == hwmon_fan_input) {
>> - ret = gpd_read_rpm();
>> + ret = gpd_read_rpm(data);
>>
>> if (ret < 0)
>> return ret;
>> @@ -524,10 +524,10 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>> } else if (type == hwmon_pwm) {
>> switch (attr) {
>> case hwmon_pwm_enable:
>> - *val = gpd_driver_priv.pwm_enable;
>> + *val = data->pwm_enable;
>> return 0;
>> case hwmon_pwm_input:
>> - ret = gpd_read_pwm();
>> + ret = gpd_read_pwm(data);
>>
>> if (ret < 0)
>> return ret;
>> @@ -540,27 +540,29 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>> return -EOPNOTSUPP;
>> }
>>
>> -static int gpd_fan_hwmon_write(__always_unused struct device *dev,
>> +static int gpd_fan_hwmon_write(struct device *dev,
>> enum hwmon_sensor_types type, u32 attr,
>> __always_unused int channel, long val)
>> {
>> + struct gpd_fan_data *data = dev_get_drvdata(dev);
>> +
>> if (type == hwmon_pwm) {
>> switch (attr) {
>> case hwmon_pwm_enable:
>> if (!in_range(val, 0, 3))
>> return -EINVAL;
>>
>> - gpd_driver_priv.pwm_enable = val;
>> + data->pwm_enable = val;
>>
>> - gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
>> + gpd_set_pwm_enable(data, data->pwm_enable);
>> return 0;
>> case hwmon_pwm_input:
>> if (!in_range(val, 0, 256))
>> return -EINVAL;
>>
>> - gpd_driver_priv.pwm_value = val;
>> + data->pwm_value = val;
>>
>> - return gpd_write_pwm(val);
>> + return gpd_write_pwm(data, val);
>> }
>> }
>>
>> @@ -584,26 +586,27 @@ static struct hwmon_chip_info gpd_fan_chip_info = {
>> .info = gpd_fan_hwmon_channel_info
>> };
>>
>> -static void gpd_win4_init_ec(void)
>> +static void gpd_win4_init_ec(struct gpd_fan_data *data)
>> {
>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>> u8 chip_id, chip_ver;
>>
>> - gpd_ecram_read(0x2000, &chip_id);
>> + gpd_ecram_read(drvdata, 0x2000, &chip_id);
>>
>> if (chip_id == 0x55) {
>> - gpd_ecram_read(0x1060, &chip_ver);
>> - gpd_ecram_write(0x1060, chip_ver | 0x80);
>> + gpd_ecram_read(drvdata, 0x1060, &chip_ver);
>> + gpd_ecram_write(drvdata, 0x1060, chip_ver | 0x80);
>> }
>> }
>>
>> -static void gpd_init_ec(void)
>> +static void gpd_init_ec(struct gpd_fan_data *data)
>> {
>> // The buggy firmware won't initialize EC properly on boot.
>> // Before its initialization, reading RPM will always return 0,
>> // and writing PWM will have no effect.
>> // Initialize it manually on driver load.
>> - if (gpd_driver_priv.drvdata->board == win4_6800u)
>> - gpd_win4_init_ec();
>> + if (data->drvdata->board == win4_6800u)
>> + gpd_win4_init_ec(data);
>> }
>>
>> static int gpd_fan_probe(struct platform_device *pdev)
>> @@ -611,7 +614,9 @@ static int gpd_fan_probe(struct platform_device *pdev)
>> struct device *dev = &pdev->dev;
>> const struct resource *region;
>> const struct resource *res;
>> - const struct device *hwdev;
>> + struct device *hwdev;
>> + struct gpd_fan_data *data;
>> + const struct gpd_fan_drvdata **match_ptr;
>>
>> res = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> if (!res)
>> @@ -624,24 +629,42 @@ static int gpd_fan_probe(struct platform_device *pdev)
>> return dev_err_probe(dev, -EBUSY,
>> "Failed to request region\n");
>>
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + match_ptr = dev_get_platdata(dev);
>> + if (!match_ptr)
>> + return -EINVAL;
>> + data->drvdata = *match_ptr;
>> +
>> + data->pwm_enable = AUTOMATIC;
>> + data->pwm_value = 255;
>> +
>> + dev_set_drvdata(dev, data);
>> +
>> + gpd_init_ec(data);
>> +
>> hwdev = devm_hwmon_device_register_with_info(dev,
>> DRIVER_NAME,
>> - NULL,
>> + data,
>> &gpd_fan_chip_info,
>> NULL);
>> if (IS_ERR(hwdev))
>> return dev_err_probe(dev, PTR_ERR(hwdev),
>> "Failed to register hwmon device\n");
>>
>> - gpd_init_ec();
>> -
This suspiciously looks like a bug fix which is not mentioned in the
commit description. Either explain why it is now needed before hwmon
registration and why that was not the case before, or submit as separate
bug fix.
>> return 0;
>> }
>>
>> -static void gpd_fan_remove(__always_unused struct platform_device *pdev)
>> +static void gpd_fan_remove(struct platform_device *pdev)
>> {
>> - gpd_driver_priv.pwm_enable = AUTOMATIC;
>> - gpd_set_pwm_enable(AUTOMATIC);
>> + struct gpd_fan_data *data = dev_get_drvdata(&pdev->dev);
>> +
>> + if (data) {
>> + data->pwm_enable = AUTOMATIC;
>> + gpd_set_pwm_enable(data, AUTOMATIC);
>> + }
>> }
>>
>> static struct platform_driver gpd_fan_driver = {
>> @@ -668,6 +691,7 @@ static int __init gpd_fan_init(void)
>> if (!match) {
>> const struct dmi_system_id *dmi_match =
>> dmi_first_match(dmi_table);
>> +
>> if (dmi_match)
>> match = dmi_match->driver_data;
>> }
>> @@ -675,10 +699,6 @@ static int __init gpd_fan_init(void)
>> 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,
>> @@ -690,7 +710,8 @@ static int __init gpd_fan_init(void)
>> gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
>> gpd_fan_probe,
>> gpd_fan_resources,
>> - 1, NULL, 0);
>> + 1,
>> + &match, sizeof(match));
Why &match instead of just match ? match is a pointer to driver_data which itself
is a pointer. Passing a pointer to a pointer just to dereference it in the probe
function doesn't make sense to me.
>>
>> if (IS_ERR(gpd_fan_platform_device)) {
>> pr_warn("Failed to create platform device\n");
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv
2026-05-22 13:52 ` Guenter Roeck
@ 2026-05-27 8:06 ` Pei Xiao
0 siblings, 0 replies; 9+ messages in thread
From: Pei Xiao @ 2026-05-27 8:06 UTC (permalink / raw)
To: Guenter Roeck, cryolitia, linux-hwmon, linux-kernel
在 2026/5/22 21:52, Guenter Roeck 写道:
> On 5/21/26 22:50, Pei Xiao wrote:
>>
>> After all, using the global variable |gpd_driver_priv|
>> does not comply with driver development specifications.
>>
>
> I had marked this patch "changes requested", though I don't recall
> the reason. Comment inline. Please fix and resubmit, together with
> the next patch of the series but excluding the third patch.
>
> Thanks,
> Guenter
>
>> Friendly ping.
>>
>> Pei.
>> Thanks!
>> 在 2026/4/5 16:40, Pei Xiao 写道:
>>> Replace the global state gpd_driver_priv with per-device private data
>>> (struct gpd_fan_data) allocated in probe. This allows the driver to
>>> support multiple instances in the future and aligns with kernel best
>>> practices.
>>>
>>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>>> ---
>>> drivers/hwmon/gpd-fan.c | 211 ++++++++++++++++++++++------------------
>>> 1 file changed, 116 insertions(+), 95 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
>>> index 80de5f20781e..5a9d07cd29ab 100644
>>> --- a/drivers/hwmon/gpd-fan.c
>>> +++ b/drivers/hwmon/gpd-fan.c
>>> @@ -40,12 +40,11 @@ enum FAN_PWM_ENABLE {
>>> AUTOMATIC = 2,
>>> };
>>> -static struct {
>>> +struct gpd_fan_data {
>>> 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
>>> @@ -249,10 +248,10 @@ static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
>>> };
>>> // Helper functions to handle EC read/write
>>> -static void gpd_ecram_read(u16 offset, u8 *val)
>>> +static void gpd_ecram_read(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 *val)
>>> {
>>> - u16 addr_port = gpd_driver_priv.drvdata->addr_port;
>>> - u16 data_port = gpd_driver_priv.drvdata->data_port;
>>> + u16 addr_port = drvdata->addr_port;
>>> + u16 data_port = drvdata->data_port;
>>> outb(0x2E, addr_port);
>>> outb(0x11, data_port);
>>> @@ -270,10 +269,10 @@ static void gpd_ecram_read(u16 offset, u8 *val)
>>> *val = inb(data_port);
>>> }
>>> -static void gpd_ecram_write(u16 offset, u8 value)
>>> +static void gpd_ecram_write(const struct gpd_fan_drvdata *drvdata, u16 offset, u8 value)
>>> {
>>> - u16 addr_port = gpd_driver_priv.drvdata->addr_port;
>>> - u16 data_port = gpd_driver_priv.drvdata->data_port;
>>> + u16 addr_port = drvdata->addr_port;
>>> + u16 data_port = drvdata->data_port;
>>> outb(0x2E, addr_port);
>>> outb(0x11, data_port);
>>> @@ -291,198 +290,198 @@ static void gpd_ecram_write(u16 offset, u8 value)
>>> outb(value, data_port);
>>> }
>>> -static int gpd_generic_read_rpm(void)
>>> +static int gpd_generic_read_rpm(struct gpd_fan_data *data)
>>> {
>>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>> u8 high, low;
>>> - gpd_ecram_read(drvdata->rpm_read, &high);
>>> - gpd_ecram_read(drvdata->rpm_read + 1, &low);
>>> + gpd_ecram_read(drvdata, drvdata->rpm_read, &high);
>>> + gpd_ecram_read(drvdata, drvdata->rpm_read + 1, &low);
>>> return (u16)high << 8 | low;
>>> }
>>> -static int gpd_wm2_read_rpm(void)
>>> +static int gpd_wm2_read_rpm(struct gpd_fan_data *data)
>>> {
>>> + const struct gpd_fan_drvdata *drvdata = data->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(pwm_ctr_offset, &PWMCTR);
>>> + gpd_ecram_read(drvdata, pwm_ctr_offset, &PWMCTR);
>>> if (PWMCTR != 0xB8)
>>> - gpd_ecram_write(pwm_ctr_offset, 0xB8);
>>> + gpd_ecram_write(drvdata, pwm_ctr_offset, 0xB8);
>>> }
>>> - return gpd_generic_read_rpm();
>>> + return gpd_generic_read_rpm(data);
>>> }
>>> // Read value for fan1_input
>>> -static int gpd_read_rpm(void)
>>> +static int gpd_read_rpm(struct gpd_fan_data *data)
>>> {
>>> - switch (gpd_driver_priv.drvdata->board) {
>>> + switch (data->drvdata->board) {
>>> case win4_6800u:
>>> case win_mini:
>>> case duo:
>>> case mpc2:
>>> - return gpd_generic_read_rpm();
>>> + return gpd_generic_read_rpm(data);
>>> case win_max_2:
>>> - return gpd_wm2_read_rpm();
>>> + return gpd_wm2_read_rpm(data);
>>> }
>>> return 0;
>>> }
>>> -static int gpd_wm2_read_pwm(void)
>>> +static int gpd_wm2_read_pwm(struct gpd_fan_data *data)
>>> {
>>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>> u8 var;
>>> - gpd_ecram_read(drvdata->pwm_write, &var);
>>> + gpd_ecram_read(drvdata, drvdata->pwm_write, &var);
>>> // Match gpd_generic_write_pwm(u8) below
>>> return DIV_ROUND_CLOSEST((var - 1) * 255, (drvdata->pwm_max - 1));
>>> }
>>> // Read value for pwm1
>>> -static int gpd_read_pwm(void)
>>> +static int gpd_read_pwm(struct gpd_fan_data *data)
>>> {
>>> - switch (gpd_driver_priv.drvdata->board) {
>>> + switch (data->drvdata->board) {
>>> case win_mini:
>>> case duo:
>>> case win4_6800u:
>>> case mpc2:
>>> - switch (gpd_driver_priv.pwm_enable) {
>>> + switch (data->pwm_enable) {
>>> case DISABLE:
>>> return 255;
>>> case MANUAL:
>>> - return gpd_driver_priv.pwm_value;
>>> + return data->pwm_value;
>>> case AUTOMATIC:
>>> return -EOPNOTSUPP;
>>> }
>>> break;
>>> case win_max_2:
>>> - return gpd_wm2_read_pwm();
>>> + return gpd_wm2_read_pwm(data);
>>> }
>>> return 0;
>>> }
>>> // PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it.
>>> -static inline u8 gpd_cast_pwm_range(u8 val)
>>> +static inline u8 gpd_cast_pwm_range(const struct gpd_fan_drvdata *drvdata, u8 val)
>>> {
>>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>>> -
>>> return DIV_ROUND_CLOSEST(val * (drvdata->pwm_max - 1), 255) + 1;
>>> }
>>> -static void gpd_generic_write_pwm(u8 val)
>>> +static void gpd_generic_write_pwm(struct gpd_fan_data *data, u8 val)
>>> {
>>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>> u8 pwm_reg;
>>> - pwm_reg = gpd_cast_pwm_range(val);
>>> - gpd_ecram_write(drvdata->pwm_write, pwm_reg);
>>> + pwm_reg = gpd_cast_pwm_range(drvdata, val);
>>> + gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
>>> }
>>> -static void gpd_duo_write_pwm(u8 val)
>>> +static void gpd_duo_write_pwm(struct gpd_fan_data *data, u8 val)
>>> {
>>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>> u8 pwm_reg;
>>> - pwm_reg = gpd_cast_pwm_range(val);
>>> - gpd_ecram_write(drvdata->pwm_write, pwm_reg);
>>> - gpd_ecram_write(drvdata->pwm_write + 1, pwm_reg);
>>> + pwm_reg = gpd_cast_pwm_range(drvdata, val);
>>> + gpd_ecram_write(drvdata, drvdata->pwm_write, pwm_reg);
>>> + gpd_ecram_write(drvdata, drvdata->pwm_write + 1, pwm_reg);
>>> }
>>> // Write value for pwm1
>>> -static int gpd_write_pwm(u8 val)
>>> +static int gpd_write_pwm(struct gpd_fan_data *data, u8 val)
>>> {
>>> - if (gpd_driver_priv.pwm_enable != MANUAL)
>>> + if (data->pwm_enable != MANUAL)
>>> return -EPERM;
>>> - switch (gpd_driver_priv.drvdata->board) {
>>> + switch (data->drvdata->board) {
>>> case duo:
>>> - gpd_duo_write_pwm(val);
>>> + gpd_duo_write_pwm(data, val);
>>> break;
>>> case win_mini:
>>> case win4_6800u:
>>> case win_max_2:
>>> case mpc2:
>>> - gpd_generic_write_pwm(val);
>>> + gpd_generic_write_pwm(data, val);
>>> break;
>>> }
>>> return 0;
>>> }
>>> -static void gpd_win_mini_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>>> +static void gpd_win_mini_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
>>> {
>>> switch (pwm_enable) {
>>> case DISABLE:
>>> - gpd_generic_write_pwm(255);
>>> + gpd_generic_write_pwm(data, 255);
>>> break;
>>> case MANUAL:
>>> - gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
>>> + gpd_generic_write_pwm(data, data->pwm_value);
>>> break;
>>> case AUTOMATIC:
>>> - gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
>>> + gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
>>> break;
>>> }
>>> }
>>> -static void gpd_duo_set_pwm_enable(enum FAN_PWM_ENABLE pwm_enable)
>>> +static void gpd_duo_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE pwm_enable)
>>> {
>>> switch (pwm_enable) {
>>> case DISABLE:
>>> - gpd_duo_write_pwm(255);
>>> + gpd_duo_write_pwm(data, 255);
>>> break;
>>> case MANUAL:
>>> - gpd_duo_write_pwm(gpd_driver_priv.pwm_value);
>>> + gpd_duo_write_pwm(data, data->pwm_value);
>>> break;
>>> case AUTOMATIC:
>>> - gpd_ecram_write(gpd_driver_priv.drvdata->pwm_write, 0);
>>> + gpd_ecram_write(data->drvdata, data->drvdata->pwm_write, 0);
>>> break;
>>> }
>>> }
>>> -static void gpd_wm2_set_pwm_enable(enum FAN_PWM_ENABLE enable)
>>> +static void gpd_wm2_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
>>> {
>>> - const struct gpd_fan_drvdata *const drvdata = gpd_driver_priv.drvdata;
>>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>> switch (enable) {
>>> case DISABLE:
>>> - gpd_generic_write_pwm(255);
>>> - gpd_ecram_write(drvdata->manual_control_enable, 1);
>>> + gpd_generic_write_pwm(data, 255);
>>> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
>>> break;
>>> case MANUAL:
>>> - gpd_generic_write_pwm(gpd_driver_priv.pwm_value);
>>> - gpd_ecram_write(drvdata->manual_control_enable, 1);
>>> + gpd_generic_write_pwm(data, data->pwm_value);
>>> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 1);
>>> break;
>>> case AUTOMATIC:
>>> - gpd_ecram_write(drvdata->manual_control_enable, 0);
>>> + gpd_ecram_write(drvdata, drvdata->manual_control_enable, 0);
>>> break;
>>> }
>>> }
>>> // Write value for pwm1_enable
>>> -static void gpd_set_pwm_enable(enum FAN_PWM_ENABLE enable)
>>> +static void gpd_set_pwm_enable(struct gpd_fan_data *data, enum FAN_PWM_ENABLE enable)
>>> {
>>> if (enable == MANUAL)
>>> // Set pwm_value to max firstly when switching to manual mode, in
>>> // consideration of device safety.
>>> - gpd_driver_priv.pwm_value = 255;
>>> + data->pwm_value = 255;
>>> - switch (gpd_driver_priv.drvdata->board) {
>>> + switch (data->drvdata->board) {
>>> case win_mini:
>>> case win4_6800u:
>>> case mpc2:
>>> - gpd_win_mini_set_pwm_enable(enable);
>>> + gpd_win_mini_set_pwm_enable(data, enable);
>>> break;
>>> case duo:
>>> - gpd_duo_set_pwm_enable(enable);
>>> + gpd_duo_set_pwm_enable(data, enable);
>>> break;
>>> case win_max_2:
>>> - gpd_wm2_set_pwm_enable(enable);
>>> + gpd_wm2_set_pwm_enable(data, enable);
>>> break;
>>> }
>>> }
>>> @@ -505,15 +504,16 @@ static umode_t gpd_fan_hwmon_is_visible(__always_unused const void *drvdata,
>>> return 0;
>>> }
>>> -static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>>> +static int gpd_fan_hwmon_read(struct device *dev,
>>> enum hwmon_sensor_types type, u32 attr,
>>> __always_unused int channel, long *val)
>>> {
>>> + struct gpd_fan_data *data = dev_get_drvdata(dev);
>>> int ret;
>>> if (type == hwmon_fan) {
>>> if (attr == hwmon_fan_input) {
>>> - ret = gpd_read_rpm();
>>> + ret = gpd_read_rpm(data);
>>> if (ret < 0)
>>> return ret;
>>> @@ -524,10 +524,10 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>>> } else if (type == hwmon_pwm) {
>>> switch (attr) {
>>> case hwmon_pwm_enable:
>>> - *val = gpd_driver_priv.pwm_enable;
>>> + *val = data->pwm_enable;
>>> return 0;
>>> case hwmon_pwm_input:
>>> - ret = gpd_read_pwm();
>>> + ret = gpd_read_pwm(data);
>>> if (ret < 0)
>>> return ret;
>>> @@ -540,27 +540,29 @@ static int gpd_fan_hwmon_read(__always_unused struct device *dev,
>>> return -EOPNOTSUPP;
>>> }
>>> -static int gpd_fan_hwmon_write(__always_unused struct device *dev,
>>> +static int gpd_fan_hwmon_write(struct device *dev,
>>> enum hwmon_sensor_types type, u32 attr,
>>> __always_unused int channel, long val)
>>> {
>>> + struct gpd_fan_data *data = dev_get_drvdata(dev);
>>> +
>>> if (type == hwmon_pwm) {
>>> switch (attr) {
>>> case hwmon_pwm_enable:
>>> if (!in_range(val, 0, 3))
>>> return -EINVAL;
>>> - gpd_driver_priv.pwm_enable = val;
>>> + data->pwm_enable = val;
>>> - gpd_set_pwm_enable(gpd_driver_priv.pwm_enable);
>>> + gpd_set_pwm_enable(data, data->pwm_enable);
>>> return 0;
>>> case hwmon_pwm_input:
>>> if (!in_range(val, 0, 256))
>>> return -EINVAL;
>>> - gpd_driver_priv.pwm_value = val;
>>> + data->pwm_value = val;
>>> - return gpd_write_pwm(val);
>>> + return gpd_write_pwm(data, val);
>>> }
>>> }
>>> @@ -584,26 +586,27 @@ static struct hwmon_chip_info gpd_fan_chip_info = {
>>> .info = gpd_fan_hwmon_channel_info
>>> };
>>> -static void gpd_win4_init_ec(void)
>>> +static void gpd_win4_init_ec(struct gpd_fan_data *data)
>>> {
>>> + const struct gpd_fan_drvdata *drvdata = data->drvdata;
>>> u8 chip_id, chip_ver;
>>> - gpd_ecram_read(0x2000, &chip_id);
>>> + gpd_ecram_read(drvdata, 0x2000, &chip_id);
>>> if (chip_id == 0x55) {
>>> - gpd_ecram_read(0x1060, &chip_ver);
>>> - gpd_ecram_write(0x1060, chip_ver | 0x80);
>>> + gpd_ecram_read(drvdata, 0x1060, &chip_ver);
>>> + gpd_ecram_write(drvdata, 0x1060, chip_ver | 0x80);
>>> }
>>> }
>>> -static void gpd_init_ec(void)
>>> +static void gpd_init_ec(struct gpd_fan_data *data)
>>> {
>>> // The buggy firmware won't initialize EC properly on boot.
>>> // Before its initialization, reading RPM will always return 0,
>>> // and writing PWM will have no effect.
>>> // Initialize it manually on driver load.
>>> - if (gpd_driver_priv.drvdata->board == win4_6800u)
>>> - gpd_win4_init_ec();
>>> + if (data->drvdata->board == win4_6800u)
>>> + gpd_win4_init_ec(data);
>>> }
>>> static int gpd_fan_probe(struct platform_device *pdev)
>>> @@ -611,7 +614,9 @@ static int gpd_fan_probe(struct platform_device *pdev)
>>> struct device *dev = &pdev->dev;
>>> const struct resource *region;
>>> const struct resource *res;
>>> - const struct device *hwdev;
>>> + struct device *hwdev;
>>> + struct gpd_fan_data *data;
>>> + const struct gpd_fan_drvdata **match_ptr;
>>> res = platform_get_resource(pdev, IORESOURCE_IO, 0);
>>> if (!res)
>>> @@ -624,24 +629,42 @@ static int gpd_fan_probe(struct platform_device *pdev)
>>> return dev_err_probe(dev, -EBUSY,
>>> "Failed to request region\n");
>>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + match_ptr = dev_get_platdata(dev);
>>> + if (!match_ptr)
>>> + return -EINVAL;
>>> + data->drvdata = *match_ptr;
>>> +
>>> + data->pwm_enable = AUTOMATIC;
>>> + data->pwm_value = 255;
>>> +
>>> + dev_set_drvdata(dev, data);
>>> +
>>> + gpd_init_ec(data);
>>> +
>>> hwdev = devm_hwmon_device_register_with_info(dev,
>>> DRIVER_NAME,
>>> - NULL,
>>> + data,
>>> &gpd_fan_chip_info,
>>> NULL);
>>> if (IS_ERR(hwdev))
>>> return dev_err_probe(dev, PTR_ERR(hwdev),
>>> "Failed to register hwmon device\n");
>>> - gpd_init_ec();
>>> -
>
> This suspiciously looks like a bug fix which is not mentioned in the
> commit description. Either explain why it is now needed before hwmon
> registration and why that was not the case before, or submit as separate
> bug fix.
ok, I will send a separate fix patch. Thanks!
>>> return 0;
>>> }
>>> -static void gpd_fan_remove(__always_unused struct platform_device *pdev)
>>> +static void gpd_fan_remove(struct platform_device *pdev)
>>> {
>>> - gpd_driver_priv.pwm_enable = AUTOMATIC;
>>> - gpd_set_pwm_enable(AUTOMATIC);
>>> + struct gpd_fan_data *data = dev_get_drvdata(&pdev->dev);
>>> +
>>> + if (data) {
>>> + data->pwm_enable = AUTOMATIC;
>>> + gpd_set_pwm_enable(data, AUTOMATIC);
>>> + }
>>> }
>>> static struct platform_driver gpd_fan_driver = {
>>> @@ -668,6 +691,7 @@ static int __init gpd_fan_init(void)
>>> if (!match) {
>>> const struct dmi_system_id *dmi_match =
>>> dmi_first_match(dmi_table);
>>> +
>>> if (dmi_match)
>>> match = dmi_match->driver_data;
>>> }
>>> @@ -675,10 +699,6 @@ static int __init gpd_fan_init(void)
>>> 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,
>>> @@ -690,7 +710,8 @@ static int __init gpd_fan_init(void)
>>> gpd_fan_platform_device = platform_create_bundle(&gpd_fan_driver,
>>> gpd_fan_probe,
>>> gpd_fan_resources,
>>> - 1, NULL, 0);
>>> + 1,
>>> + &match, sizeof(match));
>
> Why &match instead of just match ? match is a pointer to driver_data which itself
> is a pointer. Passing a pointer to a pointer just to dereference it in the probe
> function doesn't make sense to me.
>
thanks! I will modify to : match, sizeof(*match));
Pei.
Thanks for your reiview and your time!
>>> if (IS_ERR(gpd_fan_platform_device)) {
>>> pr_warn("Failed to create platform device\n");
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-05-27 8:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-04-05 8:40 [PATCH 0/3] cleanups in hwmon gpd-fan Pei Xiao
2026-04-05 8:40 ` [PATCH 1/3] hwmon: (gpd-fan): remove global variable gpd_driver_priv Pei Xiao
2026-05-22 5:50 ` Pei Xiao
2026-05-22 13:52 ` Guenter Roeck
2026-05-27 8:06 ` Pei Xiao
2026-04-05 8:40 ` [PATCH 2/3] hwmon: (gpd-fan): upgrade log level from warn to err for platform device creation failure Pei Xiao
2026-04-05 8:40 ` [PATCH 3/3] hwmon: (gpd-fan): rename enum constants to uppercase as per kernel coding style Pei Xiao
2026-04-05 13:53 ` Guenter Roeck
2026-04-06 0:58 ` Pei Xiao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome