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.
