Message-ID: <7e4e45e1-9a77-4780-a5bd-ac44cd7c6cdd@kernel.org>
Date: Tue, 16 Jul 2024 21:28:15 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Cryolitia@...il.com, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-doc@...r.kernel.org, Celeste Liu <CoelacanthusHex@...il.com>,
 Marcin Strągowski <marcin@...agowski.com>
Subject: Re: [PATCH v2 1/2] hwmon: add GPD devices sensor driver

On 16/07/2024 18:58, Cryolitia PukNgae via B4 Relay wrote:
> From: Cryolitia PukNgae <Cryolitia@...il.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@...agowski.com>
> Signed-off-by: Cryolitia PukNgae <Cryolitia@...il.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@...il.com>
> +L:	linux-hwmon@...r.kernel.org
> +S:	Maintained
> +F:	drivers/hwmon/gpd-fan.c
> +
>  GPD POCKET FAN DRIVER
>  M:	Hans de Goede <hdegoede@...hat.com>
>  L:	platform-driver-x86@...r.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@...il.com>");

That's some odd indentation above.

Best regards,
Krzysztof

Powered by blists - more mailing lists