| Message ID | 20240718-gpd_fan-v4-1-116e5431a9fe@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: add GPD devices sensor driver | expand |
Hi Cryolitia, kernel test robot noticed the following build warnings: [auto build test WARNING on d67978318827d06f1c0fa4c31343a279e9df6fde] url: https://github.com/intel-lab-lkp/linux/commits/Cryolitia-PukNgae-via-B4-Relay/hwmon-add-GPD-devices-sensor-driver/20240718-104420 base: d67978318827d06f1c0fa4c31343a279e9df6fde patch link: https://lore.kernel.org/r/20240718-gpd_fan-v4-1-116e5431a9fe%40gmail.com patch subject: [PATCH v4 1/2] hwmon: add GPD devices sensor driver config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240719/202407190105.hnZaypzI-lkp@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240719/202407190105.hnZaypzI-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202407190105.hnZaypzI-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/gpd-fan.c:153:35: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 153 | .driver_data = &gpd_win_mini_quirk, | ^ drivers/hwmon/gpd-fan.c:162:35: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 162 | .driver_data = &gpd_win4_quirk, | ^ drivers/hwmon/gpd-fan.c:171:35: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 171 | .driver_data = &gpd_wm2_quirk, | ^ drivers/hwmon/gpd-fan.c:181:35: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 181 | .driver_data = &gpd_wm2_quirk, | ^ vim +/const +153 drivers/hwmon/gpd-fan.c 144 145 static const struct dmi_system_id gpd_devices[] = { 146 { 147 // GPD Win Mini 148 // GPD Win Mini with AMD Ryzen 8840U 149 .matches = { 150 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 151 DMI_MATCH(DMI_PRODUCT_NAME, "G1617-01") 152 }, > 153 .driver_data = &gpd_win_mini_quirk, 154 }, 155 { 156 // GPD Win 4 with AMD Ryzen 6800U 157 .matches = { 158 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 159 DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"), 160 DMI_MATCH(DMI_BOARD_VERSION, "Default string"), 161 }, 162 .driver_data = &gpd_win4_quirk, 163 }, 164 { 165 // GPD Win 4 with Ryzen 7840U 166 .matches = { 167 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 168 DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"), 169 DMI_MATCH(DMI_BOARD_VERSION, "Ver. 1.0"), 170 }, 171 .driver_data = &gpd_wm2_quirk, 172 }, 173 { 174 // GPD Win Max 2 with Ryzen 6800U 175 // GPD Win Max 2 2023 with Ryzen 7840U 176 // GPD Win Max 2 2024 with Ryzen 8840U 177 .matches = { 178 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 179 DMI_MATCH(DMI_PRODUCT_NAME, "G1619-04"), 180 }, 181 .driver_data = &gpd_wm2_quirk, 182 }, 183 {} 184 }; 185
Hi Cryolitia, kernel test robot noticed the following build errors: [auto build test ERROR on d67978318827d06f1c0fa4c31343a279e9df6fde] url: https://github.com/intel-lab-lkp/linux/commits/Cryolitia-PukNgae-via-B4-Relay/hwmon-add-GPD-devices-sensor-driver/20240718-104420 base: d67978318827d06f1c0fa4c31343a279e9df6fde patch link: https://lore.kernel.org/r/20240718-gpd_fan-v4-1-116e5431a9fe%40gmail.com patch subject: [PATCH v4 1/2] hwmon: add GPD devices sensor driver config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240719/202407190547.V7oIPiBV-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240719/202407190547.V7oIPiBV-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202407190547.V7oIPiBV-lkp@intel.com/ All error/warnings (new ones prefixed by >>): >> drivers/hwmon/gpd-fan.c:153:18: error: initializing 'void *' with an expression of type 'const struct gpd_model_quirk *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 153 | .driver_data = &gpd_win_mini_quirk, | ^~~~~~~~~~~~~~~~~~~ drivers/hwmon/gpd-fan.c:162:18: error: initializing 'void *' with an expression of type 'const struct gpd_model_quirk *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 162 | .driver_data = &gpd_win4_quirk, | ^~~~~~~~~~~~~~~ drivers/hwmon/gpd-fan.c:171:18: error: initializing 'void *' with an expression of type 'const struct gpd_model_quirk *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 171 | .driver_data = &gpd_wm2_quirk, | ^~~~~~~~~~~~~~ drivers/hwmon/gpd-fan.c:181:18: error: initializing 'void *' with an expression of type 'const struct gpd_model_quirk *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 181 | .driver_data = &gpd_wm2_quirk, | ^~~~~~~~~~~~~~ >> drivers/hwmon/gpd-fan.c:496:4: warning: label followed by a declaration is a C23 extension [-Wc23-extensions] 496 | s32 ret = data->quirk->read_rpm(data); | ^ drivers/hwmon/gpd-fan.c:513:4: warning: label followed by a declaration is a C23 extension [-Wc23-extensions] 513 | s16 ret = data->quirk->read_pwm(data); | ^ drivers/hwmon/gpd-fan.c:541:4: warning: label followed by a declaration is a C23 extension [-Wc23-extensions] 541 | u8 var = clamp_val(val, 0, 255); | ^ 3 warnings and 4 errors generated. vim +153 drivers/hwmon/gpd-fan.c 144 145 static const struct dmi_system_id gpd_devices[] = { 146 { 147 // GPD Win Mini 148 // GPD Win Mini with AMD Ryzen 8840U 149 .matches = { 150 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 151 DMI_MATCH(DMI_PRODUCT_NAME, "G1617-01") 152 }, > 153 .driver_data = &gpd_win_mini_quirk, 154 }, 155 { 156 // GPD Win 4 with AMD Ryzen 6800U 157 .matches = { 158 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 159 DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"), 160 DMI_MATCH(DMI_BOARD_VERSION, "Default string"), 161 }, > 162 .driver_data = &gpd_win4_quirk, 163 }, 164 { 165 // GPD Win 4 with Ryzen 7840U 166 .matches = { 167 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 168 DMI_MATCH(DMI_PRODUCT_NAME, "G1618-04"), 169 DMI_MATCH(DMI_BOARD_VERSION, "Ver. 1.0"), 170 }, 171 .driver_data = &gpd_wm2_quirk, 172 }, 173 { 174 // GPD Win Max 2 with Ryzen 6800U 175 // GPD Win Max 2 2023 with Ryzen 7840U 176 // GPD Win Max 2 2024 with Ryzen 8840U 177 .matches = { 178 DMI_MATCH(DMI_SYS_VENDOR, "GPD"), 179 DMI_MATCH(DMI_PRODUCT_NAME, "G1619-04"), 180 }, 181 .driver_data = &gpd_wm2_quirk, 182 }, 183 {} 184 }; 185
On Thu, Jul 18, 2024 at 10:22:23AM +0800, Cryolitia PukNgae via B4 Relay wrote: > From: Cryolitia PukNgae <Cryolitia@gmail.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. > I have only tested it on my device Win Max 2 2023. > > Tested-by: Marcin Strągowski <marcin@stragowski.com> > Signed-off-by: Cryolitia PukNgae <Cryolitia@gmail.com> I am havng a hard time reviewing this driver. I am going to pint out a few issues, but this is far from a complete review. Guenter > --- > MAINTAINERS | 6 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/gpd-fan.c | 683 ++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 700 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index af4b4c271342..9ced72cec42b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9372,6 +9372,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@gmail.com> > +L: linux-hwmon@vger.kernel.org > +S: Maintained > +F: drivers/hwmon/gpd-fan.c > + > GPD POCKET FAN DRIVER > M: Hans de Goede <hdegoede@redhat.com> > L: platform-driver-x86@vger.kernel.org > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index b60fe2e58ad6..368165a25979 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -727,6 +727,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 EC fan control" > + 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 b1c7056c37db..91c288451290 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -87,6 +87,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 000000000000..7cf435ef1f1d > --- /dev/null > +++ b/drivers/hwmon/gpd-fan.c > @@ -0,0 +1,683 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// Copyright (c) 2024 Cryolitia PukNgae > + > +/* > + * Based on this repo: > + * https://github.com/Cryolitia/gpd-fan-driver > + */ > + > +#include <linux/acpi.h> > +#include <linux/dmi.h> > +#include <linux/hwmon.h> > +#include <linux/ioport.h> > +#include <linux/jiffies.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > + > +#define DRIVER_NAME "gpdfan" > + > +// model param, see document > +static char *gpd_fan_model = ""; > +module_param(gpd_fan_model, charp, 0444); > + > +// EC read/write locker > +// Should never access EC at the same time, otherwise system down. > +static DEFINE_MUTEX(gpd_fan_lock); > + > +// general struct declare start > + > +enum FAN_PWM_ENABLE { > + DISABLE = 0, > + MANUAL = 1, > + AUTOMATIC = 2, > +}; > + > +struct gpd_driver_priv { > + enum FAN_PWM_ENABLE pwm_enable; > + u8 pwm_value; > + > + u16 fan_speed_cached; > + u8 read_pwm_cached; > + > + unsigned long fan_speed_last_update; > + unsigned long read_pwm_last_update; > + > + const struct gpd_model_quirk *const quirk; What is the benefit of declaring the _pointer_ a constant ? > +}; > + > +struct gpd_model_ec_address { > + 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; > +}; > + > +struct gpd_model_quirk { > + const char *model_name; > + const struct gpd_model_ec_address address; > + > + s32 (*read_rpm)(struct gpd_driver_priv *data); > + int (*set_pwm_enable)(struct gpd_driver_priv *data, > + enum FAN_PWM_ENABLE); > + s16 (*read_pwm)(struct gpd_driver_priv *data); > + int (*write_pwm)(const struct gpd_driver_priv *data, u8 val); > +}; > + > +// general struct declare end > + > +// model quirk function declare start > + > +static s32 gpd_read_rpm(struct gpd_driver_priv *data); > +static s16 gpd_read_pwm(struct gpd_driver_priv *data); > +static int gpd_write_pwm(const struct gpd_driver_priv *data, u8 val); > + > +static int gpd_win_mini_set_pwm_enable(struct gpd_driver_priv *data, > + enum FAN_PWM_ENABLE pwm_enable); > +static int gpd_win_mini_write_pwm(const struct gpd_driver_priv *data, u8 val); > + > +static s32 gpd_win4_read_rpm(struct gpd_driver_priv *data); > + > +static s32 gpd_wm2_read_rpm(struct gpd_driver_priv *data); > +static s16 gpd_wm2_read_pwm(struct gpd_driver_priv *data); > +static int gpd_wm2_set_pwm_enable(struct gpd_driver_priv *data, > + enum FAN_PWM_ENABLE enable); > +static int gpd_wm2_write_pwm(const struct gpd_driver_priv *data, u8 val); > + Forward declarations point to a bad driver design. Please reorder the code to avoid it. > +// model quirk function declare end > + > +// model quirk struct declare start > + Isn't that obvious ? > +static const struct gpd_model_quirk gpd_win_mini_quirk = { > + .model_name = "win_mini", > + .address = { > + .addr_port = 0x4E, > + .data_port = 0x4F, > + .manual_control_enable = 0x047A, > + .rpm_read = 0x0478, > + .pwm_write = 0x047A, > + .pwm_max = 244, > + }, > + .read_rpm = gpd_read_rpm, > + .set_pwm_enable = gpd_win_mini_set_pwm_enable, > + .read_pwm = gpd_read_pwm, > + .write_pwm = gpd_win_mini_write_pwm, I am not in favor of those embedded function pointers, just like I am not in favor of function pointers passed as arguments. Both make the code difficult to read and understand. I would suggest to consider changing the code to avoid it. , > +}; > + > +static const struct gpd_model_quirk gpd_win4_quirk = { > + .model_name = "win4", > + .address = { > + .addr_port = 0x2E, > + .data_port = 0x2F, > + .manual_control_enable = 0xC311, > + .rpm_read = 0xC880, > + .pwm_write = 0xC311, > + .pwm_max = 127, > + }, > + .read_rpm = gpd_win4_read_rpm, > + // same as GPD Win Mini > + .set_pwm_enable = gpd_win_mini_set_pwm_enable, > + .read_pwm = gpd_read_pwm, > + // same as GPD Win Mini I do not see te value in those comments. > + .write_pwm = gpd_win_mini_write_pwm, > +}; > + > +static const struct gpd_model_quirk gpd_wm2_quirk = { > + .model_name = "wm2", > + .address = { > + .addr_port = 0x4E, > + .data_port = 0x4F, > + .manual_control_enable = 0x0275, > + .rpm_read = 0x0218, > + .pwm_write = 0x1809, > + .pwm_max = 184, > + }, > + .read_rpm = gpd_wm2_read_rpm, > + .set_pwm_enable = gpd_wm2_set_pwm_enable, > + .read_pwm = gpd_wm2_read_pwm, > + .write_pwm = gpd_wm2_write_pwm, > +}; > + > +// model quirk struct declare end > + Isn't that obvious ? > +static const struct dmi_system_id gpd_devices[] = { > + { > + // 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_quirk, As 0-day points out, .driver data is not a pointer to a constant. > + }, > + { > + // 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_quirk, > + }, > + { > + // 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"), > + }, > + .driver_data = &gpd_wm2_quirk, > + }, > + { > + // 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_quirk, > + }, > + {} > +}; > + > +static const struct gpd_model_quirk *gpd_module_quirks[] = { > + &gpd_win_mini_quirk, &gpd_win4_quirk, &gpd_wm2_quirk, NULL > +}; > + > +// device EC truly access function start > + What is a "truly access function" ? > +static int gpd_ecram_read(const struct gpd_model_ec_address *address, > + u16 offset, u8 *val) > +{ > + int ret; > + u16 addr_port = address->addr_port; > + u16 data_port = address->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_model_ec_address *address, > + u16 offset, u8 value) > +{ > + int ret; > + u16 addr_port = address->addr_port; > + u16 data_port = address->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; > +} > + > +// device EC truly access function end > + > +// device quirk function implement start > + > +static s32 > +gpd_read_cached_fan_speed(struct gpd_driver_priv *data, > + s32 (*read_uncached)(const struct gpd_driver_priv *)) > +{ > + // Update per 1000 milliseconds > + if (time_after(jiffies, data->fan_speed_last_update + HZ)) { > + s32 ret = read_uncached(data); > + > + if (ret < 0) > + return ret; > + > + data->fan_speed_cached = ret; > + data->fan_speed_last_update = jiffies; > + } > + return data->fan_speed_cached; > +} > + > +static s32 > +gpd_read_cached_pwm(struct gpd_driver_priv *data, > + s16 (*read_uncached)(const struct gpd_driver_priv *)) > +{ > + // Update per 1000 milliseconds > + if (time_after(jiffies, data->read_pwm_last_update + HZ)) { > + s16 ret = read_uncached(data); > + > + if (ret < 0) > + return ret; > + > + data->read_pwm_cached = ret; > + data->read_pwm_last_update = jiffies; > + } > + return data->read_pwm_cached; > +} > + > +static s32 gpd_read_rpm_uncached(const struct gpd_driver_priv *data) > +{ > + u8 high, low; > + int ret; > + const struct gpd_model_ec_address *address = &data->quirk->address; > + > + ret = gpd_ecram_read(address, address->rpm_read, &high); > + if (ret) > + return ret; > + ret = gpd_ecram_read(address, address->rpm_read + 1, &low); > + if (ret) > + return ret; > + > + return (u16)high << 8 | low; > +} > + > +static s32 gpd_read_rpm(struct gpd_driver_priv *data) > +{ > + return gpd_read_cached_fan_speed(data, gpd_read_rpm_uncached); > +} > + > +static s16 gpd_read_pwm(struct gpd_driver_priv *data) > +{ > + return data->pwm_value; > +} > + > +static int gpd_write_pwm(const struct gpd_driver_priv *data, u8 val) > +{ > + const struct gpd_model_ec_address *address = &data->quirk->address; > + u8 actual; > + > + // PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it. > + actual = val * (address->pwm_max - 1) / 255 + 1; > + return gpd_ecram_write(address, address->pwm_write, actual); > +} > + > +static int gpd_win_mini_set_pwm_enable(struct gpd_driver_priv *data, > + enum FAN_PWM_ENABLE pwm_enable) > +{ > + switch (pwm_enable) { > + case DISABLE: > + return gpd_write_pwm(data, 255); > + case MANUAL: > + return gpd_write_pwm(data, data->pwm_value); > + case AUTOMATIC: > + return gpd_write_pwm(data, 0); > + } > + return 0; > +} > + > +static int gpd_win_mini_write_pwm(const struct gpd_driver_priv *data, u8 val) > +{ > + if (data->pwm_enable == MANUAL) > + return gpd_write_pwm(data, val); > + return 0; > +} > + > +static s32 gpd_win4_read_rpm_uncached(const struct gpd_driver_priv *data) > +{ > + const struct gpd_model_ec_address *address = &data->quirk->address; > + u8 PWMCTR; > + s32 ret; > + > + gpd_ecram_read(address, 0x1841, &PWMCTR); > + if (PWMCTR != 0x7F) > + gpd_ecram_write(address, 0x1841, 0x7F); > + > + ret = gpd_read_rpm_uncached(data); > + > + if (ret < 0) > + return ret; > + > + if (ret == 0) { > + //re-init EC > + u8 chip_id; > + > + gpd_ecram_read(address, 0x2000, &chip_id); > + if (chip_id == 0x55) { > + u8 chip_ver; > + > + if (gpd_ecram_read(address, 0x1060, &chip_ver)) { > + gpd_ecram_write(address, 0x1060, > + chip_ver | 0x80); > + } > + } > + } > + return ret; > +} > + > +static s32 gpd_win4_read_rpm(struct gpd_driver_priv *data) > +{ > + return gpd_read_cached_fan_speed(data, gpd_win4_read_rpm_uncached); > +} > + > +static s32 gpd_wm2_read_rpm_uncached(const struct gpd_driver_priv *data) > +{ > + const struct gpd_model_ec_address *address = &data->quirk->address; > + > + for (u16 pwm_ctr_offset = 0x1841; pwm_ctr_offset <= 0x1843; > + pwm_ctr_offset++) { > + u8 PWMCTR; > + > + gpd_ecram_read(address, pwm_ctr_offset, &PWMCTR); > + if (PWMCTR != 0xB8) > + gpd_ecram_write(address, pwm_ctr_offset, 0xB8); > + } > + return gpd_read_rpm_uncached(data); > +} > + > +static s32 gpd_wm2_read_rpm(struct gpd_driver_priv *data) > +{ > + return gpd_read_cached_fan_speed(data, gpd_wm2_read_rpm_uncached); > +} > + > +static s16 gpd_wm2_read_pwm_uncached(const struct gpd_driver_priv *data) > +{ > + const struct gpd_model_ec_address *address = &data->quirk->address; > + u8 var; > + int ret = gpd_ecram_read(address, address->pwm_write, &var); > + > + if (ret < 0) > + return ret; > + > + return var * 255 / address->pwm_max; > +} > + > +static s16 gpd_wm2_read_pwm(struct gpd_driver_priv *data) > +{ > + return gpd_read_cached_pwm(data, gpd_wm2_read_pwm_uncached); > +} > + > +static int gpd_wm2_set_pwm_enable(struct gpd_driver_priv *data, > + enum FAN_PWM_ENABLE enable) > +{ > + const struct gpd_model_ec_address *address = &data->quirk->address; > + int ret; > + > + switch (enable) { > + case DISABLE: > + ret = gpd_write_pwm(data, 255); > + > + if (ret) > + return ret; > + > + return gpd_ecram_write(address, address->manual_control_enable, > + 1); > + case MANUAL: > + ret = gpd_write_pwm(data, data->pwm_value); > + > + if (ret) > + return ret; > + > + return gpd_ecram_write(address, address->manual_control_enable, > + 1); > + case AUTOMATIC: > + ret = gpd_ecram_write(address, address->manual_control_enable, > + 0); > + > + // Immediately refresh the PWM value > + gpd_read_cached_pwm(data, gpd_wm2_read_pwm_uncached); > + > + return ret; > + } > + return 0; > +} > + > +static int gpd_wm2_write_pwm(const struct gpd_driver_priv *data, u8 val) > +{ > + if (data->pwm_enable != DISABLE) > + return gpd_write_pwm(data, val); > + > + return 0; > +} > + > +// device quirk function implement end > + > +// hwmon subsystem start > + > +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(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, __always_unused int channel, long *val) > +{ > + struct gpd_driver_priv *data = dev_get_drvdata(dev); > + > + if (type == hwmon_fan) { > + switch (attr) { > + case hwmon_fan_input: > + s32 ret = data->quirk->read_rpm(data); > + > + if (ret < 0) > + return ret; > + > + *val = ret; > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + } > + if (type == hwmon_pwm) { > + switch (attr) { > + case hwmon_pwm_enable: > + *val = data->pwm_enable; > + return 0; > + case hwmon_pwm_input: > + s16 ret = data->quirk->read_pwm(data); > + > + if (ret < 0) > + return ret; > + > + *val = ret; > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + } > + return -EOPNOTSUPP; > +} > + > +static int gpd_fan_hwmon_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, __always_unused int channel, long val) > +{ > + struct gpd_driver_priv *data = dev_get_drvdata(dev); > + > + if (type == hwmon_pwm) { > + switch (attr) { > + case hwmon_pwm_enable: > + if (!in_range(val, 0, 3)) > + return -EINVAL; > + data->pwm_enable = val; > + return data->quirk->set_pwm_enable(data, > + data->pwm_enable); > + case hwmon_pwm_input: > + u8 var = clamp_val(val, 0, 255); > + > + data->pwm_value = var; > + return data->quirk->write_pwm(data, 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 > +}; > + > +// hwmon subsystem end > + > +static int gpd_fan_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct gpd_driver_priv *data; > + const struct resource *plat_res; > + const struct device *dev_reg; > + const struct resource *region_res; > + > + data = dev_get_platdata(&pdev->dev); > + if (IS_ERR(data)) > + return -ENODEV; > + With all the "const" spread through the driver, this one is really odd. I have never seen a driver there the _platform data_ is used to store instance-specific information. Normally _that_ information is considered constant and not modified by a driver. I really have to say that it is extremely odd to have the init function declare values such as pwm enable and pwm value and use it in the driver. Please provide a rationale for this unusual approach. Also, dev_get_platdata() returns whever it finds in dev->platform_data. I find it difficult to understand why that would ever be an ERR_PTR(). Please explain. > + plat_res = platform_get_resource(pdev, IORESOURCE_IO, 0); > + if (IS_ERR(plat_res)) > + return dev_err_probe(dev, PTR_ERR(plat_res), > + "Failed to get platform resource\n"); > + > + region_res = devm_request_region(dev, plat_res->start, > + resource_size(plat_res), DRIVER_NAME); > + if (IS_ERR(region_res)) > + return dev_err_probe(dev, PTR_ERR(region_res), > + "Failed to request region\n"); > + > + dev_reg = devm_hwmon_device_register_with_info( > + dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL); CHECK: Lines should not end with a '(' #756: FILE: drivers/hwmon/gpd-fan.c:593: + dev_reg = devm_hwmon_device_register_with_info( Plus on top of that multi-line code should be aligned with '('. > + if (IS_ERR(dev_reg)) > + return dev_err_probe(dev, PTR_ERR(region_res), > + "Failed to register hwmon device\n"); > + > + return 0; > +} > + > +static int gpd_fan_remove(struct platform_device *pdev) > +{ > + struct gpd_driver_priv *data = dev_get_platdata(&pdev->dev); > + > + data->pwm_enable = AUTOMATIC; > + data->quirk->set_pwm_enable(data, AUTOMATIC); > + This is even more unusual. Can you point me to other drivers in the kernel using that same approach for handling device specific private data ? > + return 0; > +} > + > +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_model_quirk *match = NULL; > + > + for (const struct gpd_model_quirk **p = gpd_module_quirks; *p; p++) { > + if (strcmp(gpd_fan_model, (*p)->model_name) == 0) { !strcmp > + match = *p; > + break; > + } > + } > + > + if (!match) > + match = dmi_first_match(gpd_devices)->driver_data; This assumes that dmi_first_match(gpd_devices) always returns a valid pointer. How is that guaranteed ? > + > + if (!match) > + return -ENODEV; And why check for driver_data after dmi_first_match() returned a valid pointer ? What would be the point of creating a match entry in gpd_devices without valid driver_data pointer ? > + > + struct gpd_driver_priv data = { > + .pwm_enable = AUTOMATIC, > + .pwm_value = 255, This is unusual, to say it mildly. Since the pwm value is never read from the controller/chip, this is just a random value. > + .fan_speed_cached = 0, > + .read_pwm_cached = 0, > + .fan_speed_last_update = jiffies, > + .read_pwm_last_update = jiffies, Is it on purpose that the values won't be updated for a second after registering the driver ? If so, please provide a rationale. > + .quirk = match, > + }; > + > + struct resource gpd_fan_resources[] = { > + { > + .start = data.quirk->address.addr_port, > + .end = data.quirk->address.data_port, > + .flags = IORESOURCE_IO, > + }, > + }; > + > + gpd_fan_platform_device = platform_create_bundle( CHECK: Lines should not end with a '(' #820: FILE: drivers/hwmon/gpd-fan.c:657: + gpd_fan_platform_device = platform_create_bundle( > + &gpd_fan_driver, gpd_fan_probe, gpd_fan_resources, 1, &data, > + sizeof(struct gpd_driver_priv)); > + > + 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_init(gpd_fan_init) > +module_exit(gpd_fan_exit) > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Cryolitia <Cryolitia@gmail.com>"); > +MODULE_DESCRIPTION("GPD Devices fan control driver"); > +MODULE_ALIAS("dmi:*:svnGPD:pnG1617-01:*"); > +MODULE_ALIAS("dmi:*:svnGPD:pnG1618-04:*"); > +MODULE_ALIAS("dmi:*:svnGPD:pnG1619-04:*");
On 2024/7/19 09:41, Guenter Roeck wrote: > I am havng a hard time reviewing this driver. I am going to pint out a few > issues, but this is far from a complete review. I'm new to kernel development, so please forgive my mistakes and thank you for your patience. I am modifying the source code of this driver according to your suggestions, and I would like to discuss some of your comments first. >> +static const struct gpd_model_quirk gpd_win4_quirk = { >> + .model_name = "win4", >> + .address = { >> + .addr_port = 0x2E, >> + .data_port = 0x2F, >> + .manual_control_enable = 0xC311, >> + .rpm_read = 0xC880, >> + .pwm_write = 0xC311, >> + .pwm_max = 127, >> + }, >> + .read_rpm = gpd_win4_read_rpm, >> + // same as GPD Win Mini >> + .set_pwm_enable = gpd_win_mini_set_pwm_enable, >> + .read_pwm = gpd_read_pwm, >> + // same as GPD Win Mini > I do not see te value in those comments. It's the struct of win4, but it's part of functions are the same as win_mini's. The comment is to remind that, it's by design to use win_mini's function, not by mistake. >> + >> +static int gpd_fan_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct gpd_driver_priv *data; >> + const struct resource *plat_res; >> + const struct device *dev_reg; >> + const struct resource *region_res; >> + >> + data = dev_get_platdata(&pdev->dev); >> + if (IS_ERR(data)) >> + return -ENODEV; >> + > With all the "const" spread through the driver, this one is really odd. > I have never seen a driver there the _platform data_ is used to store > instance-specific information. Normally _that_ information is considered > constant and not modified by a driver. I really have to say that it is > extremely odd to have the init function > declare values such as pwm enable and pwm value and use it in the driver. > > Please provide a rationale for this unusual approach. I don't know how to pass which model the init function found. Is it a good idea the use a global pointer to point to the instance-specific information? >> + plat_res = platform_get_resource(pdev, IORESOURCE_IO, 0); >> + if (IS_ERR(plat_res)) >> + return dev_err_probe(dev, PTR_ERR(plat_res), >> + "Failed to get platform resource\n"); >> + >> + region_res = devm_request_region(dev, plat_res->start, >> + resource_size(plat_res), DRIVER_NAME); >> + if (IS_ERR(region_res)) >> + return dev_err_probe(dev, PTR_ERR(region_res), >> + "Failed to request region\n"); >> + >> + dev_reg = devm_hwmon_device_register_with_info( >> + dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL); > CHECK: Lines should not end with a '(' > #756: FILE: drivers/hwmon/gpd-fan.c:593: > + dev_reg = devm_hwmon_device_register_with_info( > > Plus on top of that multi-line code should be aligned with '('. The source code has been formatted by clang-format with kernel's `.clang-format` file. But I would be glad to manually adjust it's style if needed. >> +static int gpd_fan_remove(struct platform_device *pdev) >> +{ >> + struct gpd_driver_priv *data = dev_get_platdata(&pdev->dev); >> + >> + data->pwm_enable = AUTOMATIC; >> + data->quirk->set_pwm_enable(data, AUTOMATIC); >> + > This is even more unusual. Can you point me to other drivers in the kernel > using that same approach for handling device specific private data ? It's to set EC back to default status if user rmmod the driver, to prevent a hardware damage. For example, they may use a userspace program to adjusting the fan curve, setting the EC to manually control mode. It happened that the device was in low power consumption and fan speed during rmmod, and the user remove the module and then performed some tasks that generated a lot of heat. Since the module was uninstalled and the EC was still in manual mode, there was nothing to protect the device. I don't know how to implement this part elegantly >> + >> + struct gpd_driver_priv data = { >> + .pwm_enable = AUTOMATIC, >> + .pwm_value = 255, > This is unusual, to say it mildly. Since the pwm value is never read > from the controller/chip, this is just a random value. We cannot read pwm out on win_mini, only wm2 support it. It's also to prevent the device from damaging. Assuming the user switches to manual control mode immediately after loading the module, the fan will always run at full speed until the user specifies the fan speed.
diff --git a/MAINTAINERS b/MAINTAINERS index af4b4c271342..9ced72cec42b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9372,6 +9372,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@gmail.com> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: drivers/hwmon/gpd-fan.c + GPD POCKET FAN DRIVER M: Hans de Goede <hdegoede@redhat.com> L: platform-driver-x86@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index b60fe2e58ad6..368165a25979 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -727,6 +727,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 EC fan control" + 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 b1c7056c37db..91c288451290 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -87,6 +87,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 000000000000..7cf435ef1f1d --- /dev/null +++ b/drivers/hwmon/gpd-fan.c @@ -0,0 +1,683 @@ +// SPDX-License-Identifier: GPL-2.0+ +// Copyright (c) 2024 Cryolitia PukNgae + +/* + * Based on this repo: + * https://github.com/Cryolitia/gpd-fan-driver + */ + +#include <linux/acpi.h> +#include <linux/dmi.h> +#include <linux/hwmon.h> +#include <linux/ioport.h> +#include <linux/jiffies.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +#define DRIVER_NAME "gpdfan" + +// model param, see document +static char *gpd_fan_model = ""; +module_param(gpd_fan_model, charp, 0444); + +// EC read/write locker +// Should never access EC at the same time, otherwise system down. +static DEFINE_MUTEX(gpd_fan_lock); + +// general struct declare start + +enum FAN_PWM_ENABLE { + DISABLE = 0, + MANUAL = 1, + AUTOMATIC = 2, +}; + +struct gpd_driver_priv { + enum FAN_PWM_ENABLE pwm_enable; + u8 pwm_value; + + u16 fan_speed_cached; + u8 read_pwm_cached; + + unsigned long fan_speed_last_update; + unsigned long read_pwm_last_update; + + const struct gpd_model_quirk *const quirk; +}; + +struct gpd_model_ec_address { + 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; +}; + +struct gpd_model_quirk { + const char *model_name; + const struct gpd_model_ec_address address; + + s32 (*read_rpm)(struct gpd_driver_priv *data); + int (*set_pwm_enable)(struct gpd_driver_priv *data, + enum FAN_PWM_ENABLE); + s16 (*read_pwm)(struct gpd_driver_priv *data); + int (*write_pwm)(const struct gpd_driver_priv *data, u8 val); +}; + +// general struct declare end + +// model quirk function declare start + +static s32 gpd_read_rpm(struct gpd_driver_priv *data); +static s16 gpd_read_pwm(struct gpd_driver_priv *data); +static int gpd_write_pwm(const struct gpd_driver_priv *data, u8 val); + +static int gpd_win_mini_set_pwm_enable(struct gpd_driver_priv *data, + enum FAN_PWM_ENABLE pwm_enable); +static int gpd_win_mini_write_pwm(const struct gpd_driver_priv *data, u8 val); + +static s32 gpd_win4_read_rpm(struct gpd_driver_priv *data); + +static s32 gpd_wm2_read_rpm(struct gpd_driver_priv *data); +static s16 gpd_wm2_read_pwm(struct gpd_driver_priv *data); +static int gpd_wm2_set_pwm_enable(struct gpd_driver_priv *data, + enum FAN_PWM_ENABLE enable); +static int gpd_wm2_write_pwm(const struct gpd_driver_priv *data, u8 val); + +// model quirk function declare end + +// model quirk struct declare start + +static const struct gpd_model_quirk gpd_win_mini_quirk = { + .model_name = "win_mini", + .address = { + .addr_port = 0x4E, + .data_port = 0x4F, + .manual_control_enable = 0x047A, + .rpm_read = 0x0478, + .pwm_write = 0x047A, + .pwm_max = 244, + }, + .read_rpm = gpd_read_rpm, + .set_pwm_enable = gpd_win_mini_set_pwm_enable, + .read_pwm = gpd_read_pwm, + .write_pwm = gpd_win_mini_write_pwm, +}; + +static const struct gpd_model_quirk gpd_win4_quirk = { + .model_name = "win4", + .address = { + .addr_port = 0x2E, + .data_port = 0x2F, + .manual_control_enable = 0xC311, + .rpm_read = 0xC880, + .pwm_write = 0xC311, + .pwm_max = 127, + }, + .read_rpm = gpd_win4_read_rpm, + // same as GPD Win Mini + .set_pwm_enable = gpd_win_mini_set_pwm_enable, + .read_pwm = gpd_read_pwm, + // same as GPD Win Mini + .write_pwm = gpd_win_mini_write_pwm, +}; + +static const struct gpd_model_quirk gpd_wm2_quirk = { + .model_name = "wm2", + .address = { + .addr_port = 0x4E, + .data_port = 0x4F, + .manual_control_enable = 0x0275, + .rpm_read = 0x0218, + .pwm_write = 0x1809, + .pwm_max = 184, + }, + .read_rpm = gpd_wm2_read_rpm, + .set_pwm_enable = gpd_wm2_set_pwm_enable, + .read_pwm = gpd_wm2_read_pwm, + .write_pwm = gpd_wm2_write_pwm, +}; + +// model quirk struct declare end + +static const struct dmi_system_id gpd_devices[] = { + { + // 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_quirk, + }, + { + // 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_quirk, + }, + { + // 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"), + }, + .driver_data = &gpd_wm2_quirk, + }, + { + // 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_quirk, + }, + {} +}; + +static const struct gpd_model_quirk *gpd_module_quirks[] = { + &gpd_win_mini_quirk, &gpd_win4_quirk, &gpd_wm2_quirk, NULL +}; + +// device EC truly access function start + +static int gpd_ecram_read(const struct gpd_model_ec_address *address, + u16 offset, u8 *val) +{ + int ret; + u16 addr_port = address->addr_port; + u16 data_port = address->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_model_ec_address *address, + u16 offset, u8 value) +{ + int ret; + u16 addr_port = address->addr_port; + u16 data_port = address->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; +} + +// device EC truly access function end + +// device quirk function implement start + +static s32 +gpd_read_cached_fan_speed(struct gpd_driver_priv *data, + s32 (*read_uncached)(const struct gpd_driver_priv *)) +{ + // Update per 1000 milliseconds + if (time_after(jiffies, data->fan_speed_last_update + HZ)) { + s32 ret = read_uncached(data); + + if (ret < 0) + return ret; + + data->fan_speed_cached = ret; + data->fan_speed_last_update = jiffies; + } + return data->fan_speed_cached; +} + +static s32 +gpd_read_cached_pwm(struct gpd_driver_priv *data, + s16 (*read_uncached)(const struct gpd_driver_priv *)) +{ + // Update per 1000 milliseconds + if (time_after(jiffies, data->read_pwm_last_update + HZ)) { + s16 ret = read_uncached(data); + + if (ret < 0) + return ret; + + data->read_pwm_cached = ret; + data->read_pwm_last_update = jiffies; + } + return data->read_pwm_cached; +} + +static s32 gpd_read_rpm_uncached(const struct gpd_driver_priv *data) +{ + u8 high, low; + int ret; + const struct gpd_model_ec_address *address = &data->quirk->address; + + ret = gpd_ecram_read(address, address->rpm_read, &high); + if (ret) + return ret; + ret = gpd_ecram_read(address, address->rpm_read + 1, &low); + if (ret) + return ret; + + return (u16)high << 8 | low; +} + +static s32 gpd_read_rpm(struct gpd_driver_priv *data) +{ + return gpd_read_cached_fan_speed(data, gpd_read_rpm_uncached); +} + +static s16 gpd_read_pwm(struct gpd_driver_priv *data) +{ + return data->pwm_value; +} + +static int gpd_write_pwm(const struct gpd_driver_priv *data, u8 val) +{ + const struct gpd_model_ec_address *address = &data->quirk->address; + u8 actual; + + // PWM value's range in EC is 1 - pwm_max, cast 0 - 255 to it. + actual = val * (address->pwm_max - 1) / 255 + 1; + return gpd_ecram_write(address, address->pwm_write, actual); +} + +static int gpd_win_mini_set_pwm_enable(struct gpd_driver_priv *data, + enum FAN_PWM_ENABLE pwm_enable) +{ + switch (pwm_enable) { + case DISABLE: + return gpd_write_pwm(data, 255); + case MANUAL: + return gpd_write_pwm(data, data->pwm_value); + case AUTOMATIC: + return gpd_write_pwm(data, 0); + } + return 0; +} + +static int gpd_win_mini_write_pwm(const struct gpd_driver_priv *data, u8 val) +{ + if (data->pwm_enable == MANUAL) + return gpd_write_pwm(data, val); + return 0; +} + +static s32 gpd_win4_read_rpm_uncached(const struct gpd_driver_priv *data) +{ + const struct gpd_model_ec_address *address = &data->quirk->address; + u8 PWMCTR; + s32 ret; + + gpd_ecram_read(address, 0x1841, &PWMCTR); + if (PWMCTR != 0x7F) + gpd_ecram_write(address, 0x1841, 0x7F); + + ret = gpd_read_rpm_uncached(data); + + if (ret < 0) + return ret; + + if (ret == 0) { + //re-init EC + u8 chip_id; + + gpd_ecram_read(address, 0x2000, &chip_id); + if (chip_id == 0x55) { + u8 chip_ver; + + if (gpd_ecram_read(address, 0x1060, &chip_ver)) { + gpd_ecram_write(address, 0x1060, + chip_ver | 0x80); + } + } + } + return ret; +} + +static s32 gpd_win4_read_rpm(struct gpd_driver_priv *data) +{ + return gpd_read_cached_fan_speed(data, gpd_win4_read_rpm_uncached); +} + +static s32 gpd_wm2_read_rpm_uncached(const struct gpd_driver_priv *data) +{ + const struct gpd_model_ec_address *address = &data->quirk->address; + + for (u16 pwm_ctr_offset = 0x1841; pwm_ctr_offset <= 0x1843; + pwm_ctr_offset++) { + u8 PWMCTR; + + gpd_ecram_read(address, pwm_ctr_offset, &PWMCTR); + if (PWMCTR != 0xB8) + gpd_ecram_write(address, pwm_ctr_offset, 0xB8); + } + return gpd_read_rpm_uncached(data); +} + +static s32 gpd_wm2_read_rpm(struct gpd_driver_priv *data) +{ + return gpd_read_cached_fan_speed(data, gpd_wm2_read_rpm_uncached); +} + +static s16 gpd_wm2_read_pwm_uncached(const struct gpd_driver_priv *data) +{ + const struct gpd_model_ec_address *address = &data->quirk->address; + u8 var; + int ret = gpd_ecram_read(address, address->pwm_write, &var); + + if (ret < 0) + return ret; + + return var * 255 / address->pwm_max; +} + +static s16 gpd_wm2_read_pwm(struct gpd_driver_priv *data) +{ + return gpd_read_cached_pwm(data, gpd_wm2_read_pwm_uncached); +} + +static int gpd_wm2_set_pwm_enable(struct gpd_driver_priv *data, + enum FAN_PWM_ENABLE enable) +{ + const struct gpd_model_ec_address *address = &data->quirk->address; + int ret; + + switch (enable) { + case DISABLE: + ret = gpd_write_pwm(data, 255); + + if (ret) + return ret; + + return gpd_ecram_write(address, address->manual_control_enable, + 1); + case MANUAL: + ret = gpd_write_pwm(data, data->pwm_value); + + if (ret) + return ret; + + return gpd_ecram_write(address, address->manual_control_enable, + 1); + case AUTOMATIC: + ret = gpd_ecram_write(address, address->manual_control_enable, + 0); + + // Immediately refresh the PWM value + gpd_read_cached_pwm(data, gpd_wm2_read_pwm_uncached); + + return ret; + } + return 0; +} + +static int gpd_wm2_write_pwm(const struct gpd_driver_priv *data, u8 val) +{ + if (data->pwm_enable != DISABLE) + return gpd_write_pwm(data, val); + + return 0; +} + +// device quirk function implement end + +// hwmon subsystem start + +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(struct device *dev, enum hwmon_sensor_types type, + u32 attr, __always_unused int channel, long *val) +{ + struct gpd_driver_priv *data = dev_get_drvdata(dev); + + if (type == hwmon_fan) { + switch (attr) { + case hwmon_fan_input: + s32 ret = data->quirk->read_rpm(data); + + if (ret < 0) + return ret; + + *val = ret; + return 0; + default: + return -EOPNOTSUPP; + } + } + if (type == hwmon_pwm) { + switch (attr) { + case hwmon_pwm_enable: + *val = data->pwm_enable; + return 0; + case hwmon_pwm_input: + s16 ret = data->quirk->read_pwm(data); + + if (ret < 0) + return ret; + + *val = ret; + return 0; + default: + return -EOPNOTSUPP; + } + } + return -EOPNOTSUPP; +} + +static int gpd_fan_hwmon_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, __always_unused int channel, long val) +{ + struct gpd_driver_priv *data = dev_get_drvdata(dev); + + if (type == hwmon_pwm) { + switch (attr) { + case hwmon_pwm_enable: + if (!in_range(val, 0, 3)) + return -EINVAL; + data->pwm_enable = val; + return data->quirk->set_pwm_enable(data, + data->pwm_enable); + case hwmon_pwm_input: + u8 var = clamp_val(val, 0, 255); + + data->pwm_value = var; + return data->quirk->write_pwm(data, 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 +}; + +// hwmon subsystem end + +static int gpd_fan_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct gpd_driver_priv *data; + const struct resource *plat_res; + const struct device *dev_reg; + const struct resource *region_res; + + data = dev_get_platdata(&pdev->dev); + if (IS_ERR(data)) + return -ENODEV; + + plat_res = platform_get_resource(pdev, IORESOURCE_IO, 0); + if (IS_ERR(plat_res)) + return dev_err_probe(dev, PTR_ERR(plat_res), + "Failed to get platform resource\n"); + + region_res = devm_request_region(dev, plat_res->start, + resource_size(plat_res), DRIVER_NAME); + if (IS_ERR(region_res)) + return dev_err_probe(dev, PTR_ERR(region_res), + "Failed to request region\n"); + + dev_reg = devm_hwmon_device_register_with_info( + dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL); + if (IS_ERR(dev_reg)) + return dev_err_probe(dev, PTR_ERR(region_res), + "Failed to register hwmon device\n"); + + return 0; +} + +static int gpd_fan_remove(struct platform_device *pdev) +{ + struct gpd_driver_priv *data = dev_get_platdata(&pdev->dev); + + data->pwm_enable = AUTOMATIC; + data->quirk->set_pwm_enable(data, AUTOMATIC); + + return 0; +} + +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_model_quirk *match = NULL; + + for (const struct gpd_model_quirk **p = gpd_module_quirks; *p; p++) { + if (strcmp(gpd_fan_model, (*p)->model_name) == 0) { + match = *p; + break; + } + } + + if (!match) + match = dmi_first_match(gpd_devices)->driver_data; + + if (!match) + return -ENODEV; + + struct gpd_driver_priv data = { + .pwm_enable = AUTOMATIC, + .pwm_value = 255, + .fan_speed_cached = 0, + .read_pwm_cached = 0, + .fan_speed_last_update = jiffies, + .read_pwm_last_update = jiffies, + .quirk = match, + }; + + struct resource gpd_fan_resources[] = { + { + .start = data.quirk->address.addr_port, + .end = data.quirk->address.data_port, + .flags = IORESOURCE_IO, + }, + }; + + gpd_fan_platform_device = platform_create_bundle( + &gpd_fan_driver, gpd_fan_probe, gpd_fan_resources, 1, &data, + sizeof(struct gpd_driver_priv)); + + 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_init(gpd_fan_init) +module_exit(gpd_fan_exit) + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Cryolitia <Cryolitia@gmail.com>"); +MODULE_DESCRIPTION("GPD Devices fan control driver"); +MODULE_ALIAS("dmi:*:svnGPD:pnG1617-01:*"); +MODULE_ALIAS("dmi:*:svnGPD:pnG1618-04:*"); +MODULE_ALIAS("dmi:*:svnGPD:pnG1619-04:*");