Re: [PATCH] hwmon: gpd-fan: replace custom EC I/O with kernel ec_read/ec_write

[Guenter Roeck]

On 2/26/26 18:49, Morduan Zang wrote:
> Replace the custom gpd_ecram_read() and gpd_ecram_write() functions that
> use direct I/O port access (inb/outb) with the kernel's standard ec_read()
> and ec_write() functions. This provides better abstraction, improves code
> maintainability, and ensures compatibility across different kernel
> versions.

It also adds dependency on ACPI. On top of that, it is quite obviously
buggy (see below).

> Signed-off-by: Morduan Zang <zhangdandan@xxxxxxxxxxxxx>
> ---
>   drivers/hwmon/gpd-fan.c | 37 +++----------------------------------
>   1 file changed, 3 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
> index 1729729b135f..c44957b7fd91 100644
> --- a/drivers/hwmon/gpd-fan.c
> +++ b/drivers/hwmon/gpd-fan.c
> @@ -19,6 +19,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> +#include <linux/acpi.h>
>   
>   #define DRIVER_NAME "gpdfan"
>   #define GPD_PWM_CTR_OFFSET 0x1841
> @@ -243,44 +244,12 @@ static const struct gpd_fan_drvdata *gpd_module_drvdata[] = {
>   // Helper functions to handle EC read/write
>   static void gpd_ecram_read(u16 offset, u8 *val)
>   {
> -	u16 addr_port = gpd_driver_priv.drvdata->addr_port;
> -	u16 data_port = gpd_driver_priv.drvdata->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);
> +	*val = ec_read(offset, val);

ec_read() writes the return value into val, and returns an error code.
So this now returns the lower 8 bit of the error code as "value".

... which of course means that this code was not tested, and who knows
what the ec_read() and ec_write() functions actually do.

This code is so bad that I won't trust any subsequent versions. NACK.

Guenter