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.
+static const struct gpd_model_quirk gpd_win4_quirk = {I do not see te value in those comments.
+ .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 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?+With all the "const" spread through the driver, this one is really odd.
+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;
+
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.
+ plat_res = platform_get_resource(pdev, IORESOURCE_IO, 0);CHECK: Lines should not end with a '('
+ 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);
#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 '('.
+static int gpd_fan_remove(struct platform_device *pdev)This is even more unusual. Can you point me to other drivers in the kernel
+{
+ struct gpd_driver_priv *data = dev_get_platdata(&pdev->dev);
+
+ data->pwm_enable = AUTOMATIC;
+ data->quirk->set_pwm_enable(data, AUTOMATIC);
+
using that same approach for handling device specific private data ?
+This is unusual, to say it mildly. Since the pwm value is never read
+ struct gpd_driver_priv data = {
+ .pwm_enable = AUTOMATIC,
+ .pwm_value = 255,
from the controller/chip, this is just a random value.