| Message ID | 20240717-gpd_fan-v2-1-f7b7e6b9f21b@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: add GPD devices sensor driver | expand |
On 7/16/24 09:58, 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 tested it on my device. > > Tested-by: Marcin Strągowski <marcin@stragowski.com> > Signed-off-by: Cryolitia PukNgae <Cryolitia@gmail.com> > --- > MAINTAINERS | 6 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/gpd-fan.c | 759 ++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 776 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..b7e7e73528af > --- /dev/null > +++ b/drivers/hwmon/gpd-fan.c > @@ -0,0 +1,759 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include <linux/acpi.h> > +#include <linux/debugfs.h> > +#include <linux/dmi.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> I do not see why this include would be necessary. > +#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" > + > +static char *gpd_fan_model = ""; > +module_param(gpd_fan_model, charp, 0444); > + > +static DEFINE_MUTEX(gpd_fan_locker); > + > +enum FUN_PWM_ENABLE { > + DISABLE = 0, > + MANUAL = 1, > + AUTOMATIC = 2, > +}; > + > +struct driver_private_data { > + enum FUN_PWM_ENABLE pwm_enable; > + u8 pwm_value; > + > + u16 fan_speed_cached; > + u8 read_pwm_cached; > + > + // minimum 1000 mill seconds > + u32 update_interval_per_second; The update_interval sysfs attribte is expected to be passed to and handled by the chip. It is not intended to provide a cache timeout to the driver. As used, this attribute is unacceptable. I would suggest to either drop caching in the driver, or, if the chip needs a timeout between accesses, to use a constant. > + > + unsigned long fan_speed_last_update; > + unsigned long read_pwm_last_update; > + > + const struct model_quirk *const quirk; > +}; > + > +struct 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 model_quirk { > + const char *model_name; > + > + bool tested; > + > + const struct model_ec_address address; > + > + int (*const read_rpm)(struct driver_private_data *, u16 *); > + > + int (*const set_pwm_enable)(struct driver_private_data *, > + enum FUN_PWM_ENABLE); > + > + int (*const read_pwm)(struct driver_private_data *, u8 *); > + > + int (*const write_pwm)(const struct driver_private_data *, u8); > +}; > + > +static int gpd_ecram_read(const struct model_ec_address *const address, > + const u16 offset, u8 *const val) > +{ > + int ret = mutex_lock_interruptible(&gpd_fan_locker); > + > + if (ret) > + return ret; > + > + u16 addr_port = address->addr_port; > + u16 data_port = address->data_port; > + > + 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_locker); > + return 0; > +} > + > +static int gpd_ecram_write(const struct model_ec_address *const address, > + const u16 offset, const u8 value) > +{ > + int ret = mutex_lock_interruptible(&gpd_fan_locker); > + > + if (ret) > + return ret; > + > + u16 addr_port = address->addr_port; > + u16 data_port = address->data_port; > + > + 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_locker); > + return 0; > +} > + > +#define DEFINE_GPD_READ_CACHED(name, type) \ > + static int gpd_read_cached_##name( \ > + struct driver_private_data *const data, \ > + int (*read_uncached)(const struct driver_private_data *, \ > + type *)) \ > + { \ > + if (time_after( \ > + jiffies, \ > + data->name##_last_update + \ > + HZ * data->update_interval_per_second)) { \ > + type var; \ > + int ret = read_uncached(data, &var); \ > + if (ret) \ > + return ret; \ > + data->name##_cached = var; \ > + data->name##_last_update = jiffies; \ > + } \ > + return 0; \ > + } > + The use of a function macro, combined with passing yet another function to it. makes it all but impossible to review this driver. I would strongly suggest to rewrite it to avoid this macro. > +DEFINE_GPD_READ_CACHED(fan_speed, u16); > +DEFINE_GPD_READ_CACHED(read_pwm, u8); > + > +static int gpd_read_rpm_uncached(const struct driver_private_data *const data, > + u16 *const val) > +{ > + u8 high, low; > + int ret; > + const struct model_ec_address *const 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; > + > + *val = high << 8 | low; > + return 0; > +} > + > +static int gpd_read_rpm(struct driver_private_data *const data, u16 *const val) > +{ > + int ret = gpd_read_cached_fan_speed(data, gpd_read_rpm_uncached); > + *val = data->fan_speed_cached; > + return ret; > +} > + > +static int gpd_read_pwm(struct driver_private_data *const data, u8 *const val) > +{ > + *val = data->pwm_value; > + return 0; > +} > + > +static int gpd_write_pwm(const struct driver_private_data *const data, > + const u8 val) > +{ > + const struct model_ec_address *const address = &data->quirk->address; > + > + u8 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 driver_private_data *const data, > + const enum FUN_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 driver_private_data *const data, > + const u8 val) > +{ > + if (data->pwm_enable == MANUAL) > + return gpd_write_pwm(data, val); > + return 0; > +} > + > +static const struct model_quirk gpd_win_mini_quirk = { > + .model_name = "win_mini", > + .tested = false, > + .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 int > +gpd_win4_read_rpm_uncached(const struct driver_private_data *const data, > + u16 *const val) > +{ > + const struct model_ec_address *const address = &data->quirk->address; > + u8 PWMCTR; > + > + gpd_ecram_read(address, 0x1841, &PWMCTR); > + if (PWMCTR != 0x7F) > + gpd_ecram_write(address, 0x1841, 0x7F); > + > + int ret = gpd_read_rpm_uncached(data, val); > + > + if (ret) > + return ret; > + > + if (*val == 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 0; > +} > + > +static int gpd_win4_read_rpm(struct driver_private_data *const data, > + u16 *const val) > +{ > + int ret = gpd_read_cached_fan_speed(data, gpd_win4_read_rpm_uncached); > + > + *val = data->fan_speed_cached; > + return ret; > +} > + > +static const struct model_quirk gpd_win4_quirk = { > + .model_name = "win4", > + .tested = false, > + .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 int > +gpd_wm2_read_rpm_uncached(const struct driver_private_data *const data, > + u16 *const val) > +{ > + const struct model_ec_address *const 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, val); > +} > + > +static int gpd_wm2_read_rpm(struct driver_private_data *const data, > + u16 *const val) > +{ > + int ret = gpd_read_cached_fan_speed(data, gpd_wm2_read_rpm_uncached); > + *val = data->fan_speed_cached; > + return ret; > +} > + > +static int > +gpd_wm2_read_pwm_uncached(const struct driver_private_data *const data, > + u8 *const val) > +{ > + const struct model_ec_address *const address = &data->quirk->address; > + u8 var; > + > + int ret = gpd_ecram_read(address, address->pwm_write, &var); > + > + *val = var * 255 / address->pwm_max; > + > + return ret; > +} > + > +static int gpd_wm2_read_pwm(struct driver_private_data *const data, > + u8 *const val) > +{ > + int ret = gpd_read_cached_read_pwm(data, gpd_wm2_read_pwm_uncached); > + *val = data->read_pwm_cached; > + return ret; > +} > + > +static int gpd_wm2_set_pwm_enable(struct driver_private_data *const data, > + const enum FUN_PWM_ENABLE enable) > +{ > + const struct model_ec_address *const address = &data->quirk->address; > + > + switch (enable) { > + case DISABLE: { > + int ret = gpd_write_pwm(data, 255); > + > + if (ret) > + return ret; > + > + return gpd_ecram_write(address, address->manual_control_enable, > + 1); > + } > + case MANUAL: { > + int ret = gpd_write_pwm(data, data->pwm_value); > + > + if (ret) > + return ret; > + > + return gpd_ecram_write(address, address->manual_control_enable, > + 1); > + } > + case AUTOMATIC: { > + int ret = gpd_ecram_write(address, > + address->manual_control_enable, 0); > + > + // Immediately refresh the PWM value > + gpd_read_cached_read_pwm(data, gpd_wm2_read_pwm_uncached); > + > + return ret; > + } > + } > + return 0; > +} > + > +static int gpd_wm2_write_pwm(const struct driver_private_data *const data, > + const u8 val) > +{ > + if (data->pwm_enable != DISABLE) > + return gpd_write_pwm(data, val); > + > + return 0; > +} > + > +static const struct model_quirk gpd_wm2_quirk = { > + .model_name = "wm2", > + .tested = true, > + .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, > +}; > + > +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 = (void *) &gpd_win_mini_quirk, A typecast of a pointer to a void * is unnecessary. > + }, > + { > + // 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 = (void *) &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 = (void *) &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 = (void *) &gpd_wm2_quirk, > + }, > + {} > +}; > + > +static const struct model_quirk *gpd_module_quirks[] = { &gpd_win_mini_quirk, > + &gpd_win4_quirk, > + &gpd_wm2_quirk, NULL }; > + > +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_mode: > + return 0444; > + case hwmon_pwm_enable: > + case hwmon_pwm_input: > + return 0644; Just like elsewhere, default: is missing. > + } > + } else if (type == hwmon_chip && attr == hwmon_chip_update_interval) { > + return 0644; > + } > + 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 driver_private_data *data = dev_get_drvdata(dev); > + > + if (type == hwmon_fan) { > + switch (attr) { > + case hwmon_fan_input: { > + u16 var; > + int ret = data->quirk->read_rpm(data, &var); > + > + *val = var; > + return ret; > + } > + } > + } else if (type == hwmon_pwm) { > + switch (attr) { > + case hwmon_pwm_mode: > + *val = 1; > + return 0; Unnecessary attribute. If the mode is always 1 there is no need for it. Please do not provide any attributes with fixed values (and, no, other drivers doing that is not an argument). > + case hwmon_pwm_enable: > + *val = data->pwm_enable; > + return 0; > + case hwmon_pwm_input: { > + u8 var; > + int ret = data->quirk->read_pwm(data, &var); I don't know why this was dropped in checkpatch, but please keep an empty line between variable declarations and their use. > + *val = var; Do not assign (random) return values if the function returned an error. This needs to be something like if (ret) return ret; *val = var; return 0; On a side note, separating the return value from the actual data value is unnecessary. Something like ret = data->quirk->read_pwm(daat); if (ret < 0) return ret; *val = ret; would generate more efficient code and would be easier to read. > + return ret; > + } > + } Those double { } just to keep variable declarations local make the code more difficult to read and review and serve no other real purpose. > + } else if (type == hwmon_chip) { > + switch (attr) { > + case hwmon_chip_update_interval: > + *val = 1000 * data->update_interval_per_second; > + return 0; default: missing, which will be a field case for static analyzers. > + } > + } > + return -EINVAL; EINVAL = Invalid Argument. This is appropriate if an invalid argument was provided, not if for whatever reason the driver has an internal bug and requests a write which is not supported. This should be -EOPNOTSUPP. > +} > + > +static int gpd_fan_hwmon_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, __always_unused int channel, > + long val) > +{ > + struct driver_private_data *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); > + } > + } > + } else if (type == hwmon_chip) { > + if (attr == hwmon_chip_update_interval) { > + int interval = val / 1000; > + > + if (interval < 1) > + interval = 1; > + data->update_interval_per_second = interval; > + return 0; > + } > + } > + return -EINVAL; This is a read function. They can not have an invalid argument. Use -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(chip, HWMON_C_UPDATE_INTERVAL), > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT), > + HWMON_CHANNEL_INFO(pwm, > + HWMON_PWM_INPUT | HWMON_PWM_ENABLE | HWMON_PWM_MODE), > + NULL > +}; > + > +static struct hwmon_chip_info gpd_fan_chip_info = { > + .ops = &gpd_fan_ops, > + .info = gpd_fan_hwmon_channel_info > +}; > + > +struct dentry *DEBUG_FS_ENTRY; > + static, and please do not use CAPITAL letters for variable names. > +static int debugfs_manual_control_get(void *data, u64 *val) > +{ > + struct model_ec_address *address = data; > + u8 u8_val; > + > + int ret = gpd_ecram_read(address, address->manual_control_enable, > + &u8_val); > + *val = (u64)u8_val; > + return ret; > +} > + > +static int debugfs_manual_control_set(void *data, u64 val) > +{ > + struct model_ec_address *address = data; > + > + return gpd_ecram_write(address, address->manual_control_enable, > + clamp_val(val, 0, 255)); > +} > + > +static int debugfs_pwm_get(void *data, u64 *val) > +{ > + struct model_ec_address *address = data; > + u8 u8_val; > + int ret = gpd_ecram_read(address, address->pwm_write, &u8_val); > + > + *val = (u64)u8_val; > + return ret; > +} > + > +static int debugfs_pwm_set(void *data, u64 val) > +{ > + struct model_ec_address *address = data; > + > + return gpd_ecram_write(address, address->pwm_write, > + clamp_val(val, 0, 255)); > +} > + Those debugfs attributes are undocumented and unacceptable. Besides, there is already a standard attribute for the pwm value, and duplicating it with debugfs does not even make sense. > +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_manual_control_fops, > + debugfs_manual_control_get, debugfs_manual_control_set, > + "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_pwm_fops, debugfs_pwm_get, debugfs_pwm_set, > + "%llu\n"); > + > +static int gpd_fan_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct driver_private_data *data; > + > + data = dev_get_platdata(&pdev->dev); > + if (IS_ERR_OR_NULL(data)) > + return -ENODEV; > + > + const struct resource *plat_res = > + platform_get_resource(pdev, IORESOURCE_IO, 0); > + if (IS_ERR_OR_NULL(plat_res)) { > + pr_warn("Failed to get platform resource\n"); > + return -ENODEV; > + } > + > + const struct resource *region_res = devm_request_region( > + dev, plat_res->start, resource_size(plat_res), DRIVER_NAME); > + if (IS_ERR_OR_NULL(region_res)) { > + pr_warn("Failed to request region\n"); > + return -EBUSY; > + } > + > + const struct device *dev_reg = devm_hwmon_device_register_with_info( > + dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL); > + if (IS_ERR_OR_NULL(dev_reg)) { > + pr_warn("Failed to register hwmon device\n"); > + return -EBUSY; > + } > + > + struct dentry *debug_fs_entry = debugfs_create_dir(DRIVER_NAME, NULL); > + > + if (!IS_ERR(debug_fs_entry)) { This error check is unnecessary, as is the local variable. > + DEBUG_FS_ENTRY = debug_fs_entry; > + debugfs_create_file_size("manual_control_reg", > + 0600, DEBUG_FS_ENTRY, > + (void *)&data->quirk->address, Unnecessary typecast. Why use debugfs_create_file_size() instead of debugfs_create_file() ? > + &debugfs_manual_control_fops, > + sizeof(u8)); > + debugfs_create_file_size("pwm_reg", 0600, > + DEBUG_FS_ENTRY, > + (void *)&data->quirk->address, unnecessary typecast > + &debugfs_pwm_fops, sizeof(u8)); > + } > + > + pr_debug("GPD Devices fan driver probed\n"); > + return 0; > +} > + > +static int gpd_fan_remove(__always_unused struct platform_device *pdev) > +{ > + struct driver_private_data *data = dev_get_platdata(&pdev->dev); > + > + data->pwm_enable = AUTOMATIC; > + data->quirk->set_pwm_enable(data, AUTOMATIC); > + > + if (!IS_ERR_OR_NULL(DEBUG_FS_ENTRY)) { > + debugfs_remove_recursive(DEBUG_FS_ENTRY); > + DEBUG_FS_ENTRY = NULL; > + } > + > + pr_debug("GPD Devices fan driver removed\n"); > + 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 model_quirk *match = NULL; > + > + for (const struct model_quirk **p = gpd_module_quirks; *p != NULL; > + p++) { > + if (strcmp(gpd_fan_model, (*p)->model_name) == 0) { > + match = *p; > + break; > + } > + } > + > + if (match == NULL) { !match Pleae run checkpatch --strict on this patch and fix what it reports. > + match = dmi_first_match(gpd_devices)->driver_data; > + if (!IS_ERR_OR_NULL(match) && !match->tested) { dmi_first_match() never returns an ERR_PTR. > + pr_warn("GPD Fan Driver do have the quirk for your device, but it's not tested. Please tested carefully by model parameter gpd_fan_model=%s and report.\n", > + match->model_name); > + match = NULL; > + } > + } > + > + if (IS_ERR_OR_NULL(match)) { match is never an ERR_PTR. > + pr_debug("GPD Devices not supported\n"); > + return -ENODEV; > + } > + > + pr_info("Loading GPD fan model quirk: %s\n", match->model_name); > + > + struct driver_private_data data = { > + .pwm_enable = AUTOMATIC, > + .pwm_value = 255, > + .fan_speed_cached = 0, > + .read_pwm_cached = 0, > + .update_interval_per_second = 1, > + .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 driver_private_data)); > + > + if (IS_ERR(gpd_fan_platform_device)) { > + pr_warn("Failed to create platform device\n"); > + return PTR_ERR(gpd_fan_platform_device); > + } > + > + pr_debug("GPD Devices fan driver loaded\n"); > + return 0; > +} > + > +static void __exit gpd_fan_exit(void) > +{ > + platform_device_unregister(gpd_fan_platform_device); > + platform_driver_unregister(&gpd_fan_driver); > + pr_debug("GPD Devices fan driver unloaded\n"); > +} > + > +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 16/07/2024 18:58, 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 tested it on my device. > > Tested-by: Marcin Strągowski <marcin@stragowski.com> > Signed-off-by: Cryolitia PukNgae <Cryolitia@gmail.com> > --- > MAINTAINERS | 6 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/gpd-fan.c | 759 ++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 776 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..b7e7e73528af > --- /dev/null > +++ b/drivers/hwmon/gpd-fan.c > @@ -0,0 +1,759 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include <linux/acpi.h> > +#include <linux/debugfs.h> > +#include <linux/dmi.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.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" > + > +static char *gpd_fan_model = ""; > +module_param(gpd_fan_model, charp, 0444); > + > +static DEFINE_MUTEX(gpd_fan_locker); > + > +enum FUN_PWM_ENABLE { FUN? > + DISABLE = 0, > + MANUAL = 1, > + AUTOMATIC = 2, > +}; > + > +struct driver_private_data { > + enum FUN_PWM_ENABLE pwm_enable; > + u8 pwm_value; > + > + u16 fan_speed_cached; > + u8 read_pwm_cached; > + > + // minimum 1000 mill seconds > + u32 update_interval_per_second; > + > + unsigned long fan_speed_last_update; > + unsigned long read_pwm_last_update; > + > + const struct model_quirk *const quirk; > +}; > + > +struct 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 model_quirk { > + const char *model_name; > + > + bool tested; Why would you add here untested models? > + > + const struct model_ec_address address; > + > + int (*const read_rpm)(struct driver_private_data *, u16 *); > + > + int (*const set_pwm_enable)(struct driver_private_data *, > + enum FUN_PWM_ENABLE); > + Drop all or most of these blank lines. > + int (*const read_pwm)(struct driver_private_data *, u8 *); > + > + int (*const write_pwm)(const struct driver_private_data *, u8); > +}; > + > +static int gpd_ecram_read(const struct model_ec_address *const address, > + const u16 offset, u8 *const val) > +{ > + int ret = mutex_lock_interruptible(&gpd_fan_locker); Don't mix mutexes with variable declaration. See existing examples how this looks like. > + > +static const struct model_quirk gpd_win4_quirk = { You mix data definition with functions. That's confusing code. Look at other recent drivers how this is organized, usually definitions are together. > + .model_name = "win4", > + .tested = false, > + .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 int > +gpd_wm2_read_rpm_uncached(const struct driver_private_data *const data, > + u16 *const val) > +{ > + const struct model_ec_address *const 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, val); > +} > + > +static int gpd_wm2_read_rpm(struct driver_private_data *const data, > + u16 *const val) All your functions are really weird. The const here is just meaningless. So meaningless that just wrong and confusing, even though technically correct. This applies everywhere - drop this odd style and keep only meaningful const. ... > + > +static int gpd_fan_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct driver_private_data *data; > + > + data = dev_get_platdata(&pdev->dev); > + if (IS_ERR_OR_NULL(data)) > + return -ENODEV; > + > + const struct resource *plat_res = > + platform_get_resource(pdev, IORESOURCE_IO, 0); > + if (IS_ERR_OR_NULL(plat_res)) { > + pr_warn("Failed to get platform resource\n"); > + return -ENODEV; > + } > + > + const struct resource *region_res = devm_request_region( > + dev, plat_res->start, resource_size(plat_res), DRIVER_NAME); > + if (IS_ERR_OR_NULL(region_res)) { > + pr_warn("Failed to request region\n"); > + return -EBUSY; > + } > + > + const struct device *dev_reg = devm_hwmon_device_register_with_info( Declarations go to the top of the function. > + dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL); > + if (IS_ERR_OR_NULL(dev_reg)) { > + pr_warn("Failed to register hwmon device\n"); > + return -EBUSY; > + } > + > + struct dentry *debug_fs_entry = debugfs_create_dir(DRIVER_NAME, NULL); > + > + if (!IS_ERR(debug_fs_entry)) { > + DEBUG_FS_ENTRY = debug_fs_entry; > + debugfs_create_file_size("manual_control_reg", > + 0600, DEBUG_FS_ENTRY, > + (void *)&data->quirk->address, > + &debugfs_manual_control_fops, > + sizeof(u8)); > + debugfs_create_file_size("pwm_reg", 0600, > + DEBUG_FS_ENTRY, > + (void *)&data->quirk->address, > + &debugfs_pwm_fops, sizeof(u8)); > + } > + > + pr_debug("GPD Devices fan driver probed\n"); Drop > + return 0; > +} > + > +static int gpd_fan_remove(__always_unused struct platform_device *pdev) > +{ > + struct driver_private_data *data = dev_get_platdata(&pdev->dev); > + > + data->pwm_enable = AUTOMATIC; > + data->quirk->set_pwm_enable(data, AUTOMATIC); > + > + if (!IS_ERR_OR_NULL(DEBUG_FS_ENTRY)) { > + debugfs_remove_recursive(DEBUG_FS_ENTRY); > + DEBUG_FS_ENTRY = NULL; > + } > + > + pr_debug("GPD Devices fan driver removed\n"); Drop > + 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, so how do you support more than one device? > + > +static int __init gpd_fan_init(void) > +{ > + const struct model_quirk *match = NULL; > + > + for (const struct model_quirk **p = gpd_module_quirks; *p != NULL; > + p++) { > + if (strcmp(gpd_fan_model, (*p)->model_name) == 0) { > + match = *p; > + break; > + } > + } > + > + if (match == NULL) { > + match = dmi_first_match(gpd_devices)->driver_data; > + if (!IS_ERR_OR_NULL(match) && !match->tested) { > + pr_warn("GPD Fan Driver do have the quirk for your device, but it's not tested. Please tested carefully by model parameter gpd_fan_model=%s and report.\n", > + match->model_name); > + match = NULL; > + } > + } > + > + if (IS_ERR_OR_NULL(match)) { > + pr_debug("GPD Devices not supported\n"); Drop > + return -ENODEV; > + } > + > + pr_info("Loading GPD fan model quirk: %s\n", match->model_name); Drop > + > + struct driver_private_data data = { > + .pwm_enable = AUTOMATIC, > + .pwm_value = 255, > + .fan_speed_cached = 0, > + .read_pwm_cached = 0, > + .update_interval_per_second = 1, > + .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 driver_private_data)); > + > + if (IS_ERR(gpd_fan_platform_device)) { > + pr_warn("Failed to create platform device\n"); Drop > + return PTR_ERR(gpd_fan_platform_device); > + } > + > + pr_debug("GPD Devices fan driver loaded\n"); Drop such debug statements, kernel cannot print anything on module loading/unloading. > + return 0; > +} > + > +static void __exit gpd_fan_exit(void) > +{ > + platform_device_unregister(gpd_fan_platform_device); > + platform_driver_unregister(&gpd_fan_driver); > + pr_debug("GPD Devices fan driver unloaded\n"); Drop such debug statements, kernel cannot print anything on module loading/unloading. > +} > + > +module_init(gpd_fan_init) > + > + module_exit(gpd_fan_exit) > + > + MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Cryolitia <Cryolitia@gmail.com>"); That's some odd indentation above. Best regards, Krzysztof
On 2024/7/17 03:28, Krzysztof Kozlowski wrote: > Why would you add here untested models? The manufacturer GPD sent me these quirks and reg addresses, and they used them to write their Windows user space driver. They are unlikely to have problems. It was fully tested by several people on WIn Max 2 2023 and Win 4, and there's only little address difference with other model. > static, so how do you support more than one device? The device will have and only have one EC controller, I use platform_device just for easier alloc and release resources. Sincerely thank you for your guidance, v3 has been sent: https://lore.kernel.org/r/20240717-gpd_fan-v3-0-8d7efb1263b7@gmail.com
On 2024/7/17 02:04, Guenter Roeck wrote: >> + pr_warn("GPD Fan Driver do have the quirk for your >> device, but it's not tested. Please tested carefully by model >> parameter gpd_fan_model=%s and report.\n", >> + match->model_name); >> + match = NULL; >> + } >> + } >> + >> + if (IS_ERR_OR_NULL(match)) { > > match is never an ERR_PTR. Sincerely thank you for your guidance, v3 has been sent: https://lore.kernel.org/r/20240717-gpd_fan-v3-0-8d7efb1263b7@gmail.com
On 17/07/2024 09:01, Cryolitia PukNgae wrote: > > On 2024/7/17 03:28, Krzysztof Kozlowski wrote: >> Why would you add here untested models? > The manufacturer GPD sent me these quirks and reg addresses, and they > used them to write their Windows user space driver. They are unlikely to > have problems. It was fully tested by several people on WIn Max 2 2023 > and Win 4, and there's only little address difference with other model. Then just skip this testing checks. Best regards, Krzysztof
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..b7e7e73528af --- /dev/null +++ b/drivers/hwmon/gpd-fan.c @@ -0,0 +1,759 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <linux/acpi.h> +#include <linux/debugfs.h> +#include <linux/dmi.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.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" + +static char *gpd_fan_model = ""; +module_param(gpd_fan_model, charp, 0444); + +static DEFINE_MUTEX(gpd_fan_locker); + +enum FUN_PWM_ENABLE { + DISABLE = 0, + MANUAL = 1, + AUTOMATIC = 2, +}; + +struct driver_private_data { + enum FUN_PWM_ENABLE pwm_enable; + u8 pwm_value; + + u16 fan_speed_cached; + u8 read_pwm_cached; + + // minimum 1000 mill seconds + u32 update_interval_per_second; + + unsigned long fan_speed_last_update; + unsigned long read_pwm_last_update; + + const struct model_quirk *const quirk; +}; + +struct 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 model_quirk { + const char *model_name; + + bool tested; + + const struct model_ec_address address; + + int (*const read_rpm)(struct driver_private_data *, u16 *); + + int (*const set_pwm_enable)(struct driver_private_data *, + enum FUN_PWM_ENABLE); + + int (*const read_pwm)(struct driver_private_data *, u8 *); + + int (*const write_pwm)(const struct driver_private_data *, u8); +}; + +static int gpd_ecram_read(const struct model_ec_address *const address, + const u16 offset, u8 *const val) +{ + int ret = mutex_lock_interruptible(&gpd_fan_locker); + + if (ret) + return ret; + + u16 addr_port = address->addr_port; + u16 data_port = address->data_port; + + 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_locker); + return 0; +} + +static int gpd_ecram_write(const struct model_ec_address *const address, + const u16 offset, const u8 value) +{ + int ret = mutex_lock_interruptible(&gpd_fan_locker); + + if (ret) + return ret; + + u16 addr_port = address->addr_port; + u16 data_port = address->data_port; + + 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_locker); + return 0; +} + +#define DEFINE_GPD_READ_CACHED(name, type) \ + static int gpd_read_cached_##name( \ + struct driver_private_data *const data, \ + int (*read_uncached)(const struct driver_private_data *, \ + type *)) \ + { \ + if (time_after( \ + jiffies, \ + data->name##_last_update + \ + HZ * data->update_interval_per_second)) { \ + type var; \ + int ret = read_uncached(data, &var); \ + if (ret) \ + return ret; \ + data->name##_cached = var; \ + data->name##_last_update = jiffies; \ + } \ + return 0; \ + } + +DEFINE_GPD_READ_CACHED(fan_speed, u16); +DEFINE_GPD_READ_CACHED(read_pwm, u8); + +static int gpd_read_rpm_uncached(const struct driver_private_data *const data, + u16 *const val) +{ + u8 high, low; + int ret; + const struct model_ec_address *const 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; + + *val = high << 8 | low; + return 0; +} + +static int gpd_read_rpm(struct driver_private_data *const data, u16 *const val) +{ + int ret = gpd_read_cached_fan_speed(data, gpd_read_rpm_uncached); + *val = data->fan_speed_cached; + return ret; +} + +static int gpd_read_pwm(struct driver_private_data *const data, u8 *const val) +{ + *val = data->pwm_value; + return 0; +} + +static int gpd_write_pwm(const struct driver_private_data *const data, + const u8 val) +{ + const struct model_ec_address *const address = &data->quirk->address; + + u8 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 driver_private_data *const data, + const enum FUN_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 driver_private_data *const data, + const u8 val) +{ + if (data->pwm_enable == MANUAL) + return gpd_write_pwm(data, val); + return 0; +} + +static const struct model_quirk gpd_win_mini_quirk = { + .model_name = "win_mini", + .tested = false, + .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 int +gpd_win4_read_rpm_uncached(const struct driver_private_data *const data, + u16 *const val) +{ + const struct model_ec_address *const address = &data->quirk->address; + u8 PWMCTR; + + gpd_ecram_read(address, 0x1841, &PWMCTR); + if (PWMCTR != 0x7F) + gpd_ecram_write(address, 0x1841, 0x7F); + + int ret = gpd_read_rpm_uncached(data, val); + + if (ret) + return ret; + + if (*val == 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 0; +} + +static int gpd_win4_read_rpm(struct driver_private_data *const data, + u16 *const val) +{ + int ret = gpd_read_cached_fan_speed(data, gpd_win4_read_rpm_uncached); + + *val = data->fan_speed_cached; + return ret; +} + +static const struct model_quirk gpd_win4_quirk = { + .model_name = "win4", + .tested = false, + .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 int +gpd_wm2_read_rpm_uncached(const struct driver_private_data *const data, + u16 *const val) +{ + const struct model_ec_address *const 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, val); +} + +static int gpd_wm2_read_rpm(struct driver_private_data *const data, + u16 *const val) +{ + int ret = gpd_read_cached_fan_speed(data, gpd_wm2_read_rpm_uncached); + *val = data->fan_speed_cached; + return ret; +} + +static int +gpd_wm2_read_pwm_uncached(const struct driver_private_data *const data, + u8 *const val) +{ + const struct model_ec_address *const address = &data->quirk->address; + u8 var; + + int ret = gpd_ecram_read(address, address->pwm_write, &var); + + *val = var * 255 / address->pwm_max; + + return ret; +} + +static int gpd_wm2_read_pwm(struct driver_private_data *const data, + u8 *const val) +{ + int ret = gpd_read_cached_read_pwm(data, gpd_wm2_read_pwm_uncached); + *val = data->read_pwm_cached; + return ret; +} + +static int gpd_wm2_set_pwm_enable(struct driver_private_data *const data, + const enum FUN_PWM_ENABLE enable) +{ + const struct model_ec_address *const address = &data->quirk->address; + + switch (enable) { + case DISABLE: { + int ret = gpd_write_pwm(data, 255); + + if (ret) + return ret; + + return gpd_ecram_write(address, address->manual_control_enable, + 1); + } + case MANUAL: { + int ret = gpd_write_pwm(data, data->pwm_value); + + if (ret) + return ret; + + return gpd_ecram_write(address, address->manual_control_enable, + 1); + } + case AUTOMATIC: { + int ret = gpd_ecram_write(address, + address->manual_control_enable, 0); + + // Immediately refresh the PWM value + gpd_read_cached_read_pwm(data, gpd_wm2_read_pwm_uncached); + + return ret; + } + } + return 0; +} + +static int gpd_wm2_write_pwm(const struct driver_private_data *const data, + const u8 val) +{ + if (data->pwm_enable != DISABLE) + return gpd_write_pwm(data, val); + + return 0; +} + +static const struct model_quirk gpd_wm2_quirk = { + .model_name = "wm2", + .tested = true, + .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, +}; + +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 = (void *) &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 = (void *) &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 = (void *) &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 = (void *) &gpd_wm2_quirk, + }, + {} +}; + +static const struct model_quirk *gpd_module_quirks[] = { &gpd_win_mini_quirk, + &gpd_win4_quirk, + &gpd_wm2_quirk, NULL }; + +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_mode: + return 0444; + case hwmon_pwm_enable: + case hwmon_pwm_input: + return 0644; + } + } else if (type == hwmon_chip && attr == hwmon_chip_update_interval) { + return 0644; + } + 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 driver_private_data *data = dev_get_drvdata(dev); + + if (type == hwmon_fan) { + switch (attr) { + case hwmon_fan_input: { + u16 var; + int ret = data->quirk->read_rpm(data, &var); + + *val = var; + return ret; + } + } + } else if (type == hwmon_pwm) { + switch (attr) { + case hwmon_pwm_mode: + *val = 1; + return 0; + case hwmon_pwm_enable: + *val = data->pwm_enable; + return 0; + case hwmon_pwm_input: { + u8 var; + int ret = data->quirk->read_pwm(data, &var); + *val = var; + return ret; + } + } + } else if (type == hwmon_chip) { + switch (attr) { + case hwmon_chip_update_interval: + *val = 1000 * data->update_interval_per_second; + return 0; + } + } + return -EINVAL; +} + +static int gpd_fan_hwmon_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, __always_unused int channel, + long val) +{ + struct driver_private_data *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); + } + } + } else if (type == hwmon_chip) { + if (attr == hwmon_chip_update_interval) { + int interval = val / 1000; + + if (interval < 1) + interval = 1; + data->update_interval_per_second = interval; + return 0; + } + } + return -EINVAL; +} + +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(chip, HWMON_C_UPDATE_INTERVAL), + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT), + HWMON_CHANNEL_INFO(pwm, + HWMON_PWM_INPUT | HWMON_PWM_ENABLE | HWMON_PWM_MODE), + NULL +}; + +static struct hwmon_chip_info gpd_fan_chip_info = { + .ops = &gpd_fan_ops, + .info = gpd_fan_hwmon_channel_info +}; + +struct dentry *DEBUG_FS_ENTRY; + +static int debugfs_manual_control_get(void *data, u64 *val) +{ + struct model_ec_address *address = data; + u8 u8_val; + + int ret = gpd_ecram_read(address, address->manual_control_enable, + &u8_val); + *val = (u64)u8_val; + return ret; +} + +static int debugfs_manual_control_set(void *data, u64 val) +{ + struct model_ec_address *address = data; + + return gpd_ecram_write(address, address->manual_control_enable, + clamp_val(val, 0, 255)); +} + +static int debugfs_pwm_get(void *data, u64 *val) +{ + struct model_ec_address *address = data; + u8 u8_val; + int ret = gpd_ecram_read(address, address->pwm_write, &u8_val); + + *val = (u64)u8_val; + return ret; +} + +static int debugfs_pwm_set(void *data, u64 val) +{ + struct model_ec_address *address = data; + + return gpd_ecram_write(address, address->pwm_write, + clamp_val(val, 0, 255)); +} + +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_manual_control_fops, + debugfs_manual_control_get, debugfs_manual_control_set, + "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_pwm_fops, debugfs_pwm_get, debugfs_pwm_set, + "%llu\n"); + +static int gpd_fan_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct driver_private_data *data; + + data = dev_get_platdata(&pdev->dev); + if (IS_ERR_OR_NULL(data)) + return -ENODEV; + + const struct resource *plat_res = + platform_get_resource(pdev, IORESOURCE_IO, 0); + if (IS_ERR_OR_NULL(plat_res)) { + pr_warn("Failed to get platform resource\n"); + return -ENODEV; + } + + const struct resource *region_res = devm_request_region( + dev, plat_res->start, resource_size(plat_res), DRIVER_NAME); + if (IS_ERR_OR_NULL(region_res)) { + pr_warn("Failed to request region\n"); + return -EBUSY; + } + + const struct device *dev_reg = devm_hwmon_device_register_with_info( + dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL); + if (IS_ERR_OR_NULL(dev_reg)) { + pr_warn("Failed to register hwmon device\n"); + return -EBUSY; + } + + struct dentry *debug_fs_entry = debugfs_create_dir(DRIVER_NAME, NULL); + + if (!IS_ERR(debug_fs_entry)) { + DEBUG_FS_ENTRY = debug_fs_entry; + debugfs_create_file_size("manual_control_reg", + 0600, DEBUG_FS_ENTRY, + (void *)&data->quirk->address, + &debugfs_manual_control_fops, + sizeof(u8)); + debugfs_create_file_size("pwm_reg", 0600, + DEBUG_FS_ENTRY, + (void *)&data->quirk->address, + &debugfs_pwm_fops, sizeof(u8)); + } + + pr_debug("GPD Devices fan driver probed\n"); + return 0; +} + +static int gpd_fan_remove(__always_unused struct platform_device *pdev) +{ + struct driver_private_data *data = dev_get_platdata(&pdev->dev); + + data->pwm_enable = AUTOMATIC; + data->quirk->set_pwm_enable(data, AUTOMATIC); + + if (!IS_ERR_OR_NULL(DEBUG_FS_ENTRY)) { + debugfs_remove_recursive(DEBUG_FS_ENTRY); + DEBUG_FS_ENTRY = NULL; + } + + pr_debug("GPD Devices fan driver removed\n"); + 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 model_quirk *match = NULL; + + for (const struct model_quirk **p = gpd_module_quirks; *p != NULL; + p++) { + if (strcmp(gpd_fan_model, (*p)->model_name) == 0) { + match = *p; + break; + } + } + + if (match == NULL) { + match = dmi_first_match(gpd_devices)->driver_data; + if (!IS_ERR_OR_NULL(match) && !match->tested) { + pr_warn("GPD Fan Driver do have the quirk for your device, but it's not tested. Please tested carefully by model parameter gpd_fan_model=%s and report.\n", + match->model_name); + match = NULL; + } + } + + if (IS_ERR_OR_NULL(match)) { + pr_debug("GPD Devices not supported\n"); + return -ENODEV; + } + + pr_info("Loading GPD fan model quirk: %s\n", match->model_name); + + struct driver_private_data data = { + .pwm_enable = AUTOMATIC, + .pwm_value = 255, + .fan_speed_cached = 0, + .read_pwm_cached = 0, + .update_interval_per_second = 1, + .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 driver_private_data)); + + if (IS_ERR(gpd_fan_platform_device)) { + pr_warn("Failed to create platform device\n"); + return PTR_ERR(gpd_fan_platform_device); + } + + pr_debug("GPD Devices fan driver loaded\n"); + return 0; +} + +static void __exit gpd_fan_exit(void) +{ + platform_device_unregister(gpd_fan_platform_device); + platform_driver_unregister(&gpd_fan_driver); + pr_debug("GPD Devices fan driver unloaded\n"); +} + +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:*");