On 17/07/2024 08:46, Cryolitia PukNgae via B4 Relay wrote:
> From: Cryolitia PukNgae <Cryolitia@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Cryolitia PukNgae <Cryolitia@xxxxxxxxx>
> ---
> MAINTAINERS | 6 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/gpd-fan.c | 674 ++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 691 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
>
...
> +// device EC truly access start
> +
> +static int gpd_ecram_read(const struct gpd_model_ec_address *address,
> + u16 offset, u8 *val)
> +{
> + int ret;
> +
> + ret = mutex_lock_interruptible(&gpd_fan_lock);
> +
> + if (ret)
> + return ret;
> +
> + u16 addr_port = address->addr_port;
> + u16 data_port = address->data_port;
Again, definitions are at the top. Read Linux Coding Style.
<form letter>
This is a friendly reminder during the review process.
It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.
Thank you.
</form letter>
> +
> + 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 = mutex_lock_interruptible(&gpd_fan_lock);
<form letter>
This is a friendly reminder during the review process.
It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.
Thank you.
</form letter>
> +
> + 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_lock);
> + return 0;
> +}
> +
> +// device EC truly access 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 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 = 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;
> +}
> +
...
> +// 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_OR_NULL(data))
> + return -ENODEV;
> +
> + plat_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (IS_ERR_OR_NULL(plat_res))
IS_ERR_OR_NULL is usually poor code.
> + 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_OR_NULL(region_res))
IS_ERR_OR_NULL is usually poor code.
> + 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);
Fix the alignment/wrapping.
> + if (IS_ERR_OR_NULL(dev_reg))
IS_ERR_OR_NULL is usually poor code. Or wrong. Cannot be NULL.
> + return dev_err_probe(dev, PTR_ERR(region_res),
> + "Failed to register hwmon device\n");
> +
> + return 0;
> +}
> +
> +static int gpd_fan_remove(__always_unused struct platform_device *pdev)
> +{
> + struct gpd_driver_priv *data = dev_get_platdata(&pdev->dev);
What is this __always_unused? How pdev can be unused if it is just here?
This entire driver still does not look like using Linux coding style.
> +
> + data->pwm_enable = AUTOMATIC;
> + data->quirk->set_pwm_enable(data, AUTOMATIC);
> +
> + return 0;
> +}
Best regards,
Krzysztof
