Adds a driver for the ITE Embedded Controller (EC) on Portwell boards.
It integrates with the Linux GPIO and watchdog subsystems to provide:

- Control/monitoring of up to 8 EC GPIO pins.
- Hardware watchdog timer with 1-255 second timeouts.

The driver communicates with the EC via I/O port 0xe300 and identifies
the hardware by the "PWG" firmware signature. This enables enhanced
system management for Portwell embedded/industrial platforms.

Signed-off-by: Yen-Chi Huang <jesse.huang@portwell.com.tw>
---
 MAINTAINERS                        |   6 +
 drivers/platform/x86/Kconfig       |  14 ++
 drivers/platform/x86/Makefile      |   3 +
 drivers/platform/x86/portwell-ec.c | 224 +++++++++++++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 drivers/platform/x86/portwell-ec.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d5dfb9186962..c52f819786dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19132,6 +19132,12 @@ F:	kernel/time/itimer.c
 F:	kernel/time/posix-*
 F:	kernel/time/namespace.c
 
+PORTWELL EC DRIVER
+M:	Yen-Chi Huang <jesse.huang@portwell.com.tw>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/portwell-ec.c
+
 POWER MANAGEMENT CORE
 M:	"Rafael J. Wysocki" <rafael@kernel.org>
 L:	linux-pm@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 43407e76476b..5b61ab422953 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -779,6 +779,20 @@ config PCENGINES_APU2
 	  To compile this driver as a module, choose M here: the module
 	  will be called pcengines-apuv2.
 
+config PORTWELL_EC
+	tristate "Portwell Embedded Controller driver"
+	depends on X86 && WATCHDOG && GPIOLIB
+	help
+	  This driver provides support for the GPIO pins and watchdog timer
+	  embedded in Portwell's EC.
+
+	  Theoretically, this driver should work on multiple Portwell platforms,
+	  but it has only been tested on the Portwell NANO-6064 board.
+	  If you encounter any issues on other boards, please report them.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called portwell-ec.
+
 config BARCO_P50_GPIO
 	tristate "Barco P50 GPIO driver for identify LED/button"
 	depends on GPIOLIB
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 650dfbebb6c8..83dd82e04457 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -92,6 +92,9 @@ obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
 # PC Engines
 obj-$(CONFIG_PCENGINES_APU2)	+= pcengines-apuv2.o
 
+# Portwell
+obj-$(CONFIG_PORTWELL_EC)	+= portwell-ec.o
+
 # Barco
 obj-$(CONFIG_BARCO_P50_GPIO)	+= barco-p50-gpio.o
 
diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
new file mode 100644
index 000000000000..59c51b80a148
--- /dev/null
+++ b/drivers/platform/x86/portwell-ec.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * portwell-ec.c: Portwell embedded controller driver.
+ *
+ * Tested on:
+ *  - Portwell NANO-6064
+ *
+ * This driver provides support for GPIO and Watchdog Timer
+ * functionalities of the Portwell boards with ITE embedded controller (EC).
+ * The EC is accessed through I/O ports and provides:
+ *  - 8 GPIO pins for control and monitoring
+ *  - Hardware watchdog with 1-255 second timeout range
+ *
+ * It integrates with the Linux GPIO and Watchdog subsystems, allowing
+ * userspace interaction with EC GPIO pins and watchdog control,
+ * ensuring system stability and configurability.
+ *
+ * (C) Copyright 2025 Portwell, Inc.
+ * Author: Yen-Chi Huang (jesse.huang@portwell.com.tw)
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/gpio/driver.h>
+#include <linux/watchdog.h>
+#include <linux/io.h>
+#include <linux/string.h>
+
+#define PORTWELL_EC_IOSPACE 0xe300
+#define PORTWELL_GPIO_PINS 8
+#define PORTWELL_GPIO_DIR_REG 0x2b
+#define PORTWELL_GPIO_VAL_REG 0x2c
+
+#define PORTWELL_WDT_EC_CONFIG_ADDR    0x06
+#define PORTWELL_WDT_EC_COUNT_MIN_ADDR 0x07
+#define PORTWELL_WDT_EC_COUNT_SEC_ADDR 0x08
+#define PORTWELL_WDT_EC_MAX_COUNT_SECOND 255
+#define PORTWELL_EC_FW_VENDOR_ADDRESS  0x4d
+#define PORTWELL_EC_FW_VENDOR_LENGTH   3
+#define PORTWELL_EC_FW_VENDOR_NAME "PWG"
+
+ /* Functions for access EC via IOSPACE*/
+static void pwec_write(u8 index, u8 data)
+{
+	outb(data, PORTWELL_EC_IOSPACE + index);
+}
+
+static u8 pwec_read(u8 address)
+{
+	return inb(PORTWELL_EC_IOSPACE + address);
+}
+
+/* GPIO functions*/
+static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return (pwec_read(PORTWELL_GPIO_VAL_REG) & (1 << offset)) ? 1 : 0;
+}
+
+static void pwec_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
+{
+	u8 tmp = pwec_read(PORTWELL_GPIO_VAL_REG);
+
+	if (val)
+		tmp |= (1 << offset);
+	else
+		tmp &= ~(1 << offset);
+	pwec_write(PORTWELL_GPIO_VAL_REG, tmp);
+}
+
+static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	return (pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset))
+			? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	/* Changing direction causes issues on some boards, so it's disabled for now. */
+	return -EOPNOTSUPP;
+}
+
+static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	/* Changing direction causes issues on some boards, so it's disabled for now. */
+	return -EOPNOTSUPP;
+}
+
+static struct gpio_chip pwec_gpio_chip = {
+	.label = "portwell-ec-gpio",
+	.get_direction = pwec_gpio_get_direction,
+	.direction_input = pwec_gpio_direction_input,
+	.direction_output = pwec_gpio_direction_output,
+	.get = pwec_gpio_get,
+	.set = pwec_gpio_set,
+	.ngpio = PORTWELL_GPIO_PINS,
+};
+
+/* Watchdog functions*/
+static int pwec_wdt_start(struct watchdog_device *wdd)
+{
+	int retry = 10;
+	u8 timeout;
+
+	do {
+		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
+		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
+		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
+		retry--;
+	} while (timeout != wdd->timeout && retry > 0);
+	pr_info("Portwell EC: Watchdog started with timeout: %d seconds\n", wdd->timeout);
+	return (retry > 0) ? 0 : -EIO;
+}
+
+static int pwec_wdt_stop(struct watchdog_device *wdd)
+{
+	pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x00);
+	pr_info("Portwell EC: Watchdog stopped\n");
+	return 0;
+}
+
+static int pwec_wdt_trigger(struct watchdog_device *wdd)
+{
+	int retry = 10;
+	u8 timeout;
+
+	pr_info("Portwell EC: Watchdog triggered with timeout: %d seconds\n", wdd->timeout);
+	do {
+		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
+		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
+		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
+		retry--;
+	} while (timeout != wdd->timeout && retry > 0);
+	return (retry > 0) ? 0 : -EIO;
+}
+
+static int pwec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
+{
+	if (timeout == 0 || timeout > PORTWELL_WDT_EC_MAX_COUNT_SECOND)
+		return -EINVAL;
+	wdd->timeout = timeout;
+	pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
+	pr_info("Portwell EC: Watchdog timeout is set: %d seconds\n", wdd->timeout);
+	return 0;
+}
+
+static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	unsigned int timeout;
+
+	timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
+	pr_info("Portwell EC: Watchdog timeout left: %d seconds\n", timeout);
+	return timeout;
+}
+
+static const struct watchdog_ops pwec_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = pwec_wdt_start,
+	.stop = pwec_wdt_stop,
+	.ping = pwec_wdt_trigger,
+	.set_timeout = pwec_wdt_set_timeout,
+	.get_timeleft = pwec_wdt_get_timeleft,
+};
+
+static struct watchdog_device ec_wdt_dev = {
+	.info = &(struct watchdog_info){
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "Portwell EC Watchdog",
+	},
+	.ops = &pwec_wdt_ops,
+	.timeout = 10,
+	.min_timeout = 1,
+	.max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,
+};
+
+static int pwec_firmware_vendor_check(void)
+{
+	u8 buf[PORTWELL_EC_FW_VENDOR_LENGTH+1];
+	u8 i;
+
+	for (i = 0; i < PORTWELL_EC_FW_VENDOR_LENGTH; i++)
+		buf[i] = pwec_read(PORTWELL_EC_FW_VENDOR_ADDRESS+i);
+	buf[PORTWELL_EC_FW_VENDOR_LENGTH] = '\0';
+	return (strcmp(PORTWELL_EC_FW_VENDOR_NAME, buf) == 0) ? 0 : -ENODEV;
+}
+
+static int __init pwec_init(void)
+{
+	int result;
+
+	result = pwec_firmware_vendor_check();
+	if (result < 0)
+		return result;
+
+	result = gpiochip_add_data(&pwec_gpio_chip, NULL);
+	if (result < 0) {
+		pr_err("Failed to register Portwell EC GPIO\n");
+		return result;
+	}
+
+	result = watchdog_register_device(&ec_wdt_dev);
+	if (result < 0) {
+		gpiochip_remove(&pwec_gpio_chip);
+		pr_err("Failed to register Portwell EC Watchdog\n");
+		return result;
+	}
+
+	pr_info("Portwell EC driver initialized\n");
+	return 0;
+}
+
+static void __exit pwec_exit(void)
+{
+	watchdog_unregister_device(&ec_wdt_dev);
+	gpiochip_remove(&pwec_gpio_chip);
+	pr_info("Portwell EC driver removed\n");
+}
+
+module_init(pwec_init);
+module_exit(pwec_exit);
+
+MODULE_AUTHOR("Yen-Chi Huang");
+MODULE_DESCRIPTION("Portwell EC Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

On Thu, 3 Apr 2025, Yen-Chi Huang wrote:

Please add watchdog drivers people/lists from MAINTAINERS file into the 
next submission.

> Adds a driver for the ITE Embedded Controller (EC) on Portwell boards.
> It integrates with the Linux GPIO and watchdog subsystems to provide:
> 
> - Control/monitoring of up to 8 EC GPIO pins.
> - Hardware watchdog timer with 1-255 second timeouts.
> 
> The driver communicates with the EC via I/O port 0xe300 and identifies
> the hardware by the "PWG" firmware signature. This enables enhanced
> system management for Portwell embedded/industrial platforms.
> 
> Signed-off-by: Yen-Chi Huang <jesse.huang@portwell.com.tw>
> ---
>  MAINTAINERS                        |   6 +
>  drivers/platform/x86/Kconfig       |  14 ++
>  drivers/platform/x86/Makefile      |   3 +
>  drivers/platform/x86/portwell-ec.c | 224 +++++++++++++++++++++++++++++
>  4 files changed, 247 insertions(+)
>  create mode 100644 drivers/platform/x86/portwell-ec.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d5dfb9186962..c52f819786dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19132,6 +19132,12 @@ F:	kernel/time/itimer.c
>  F:	kernel/time/posix-*
>  F:	kernel/time/namespace.c
>  
> +PORTWELL EC DRIVER
> +M:	Yen-Chi Huang <jesse.huang@portwell.com.tw>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/portwell-ec.c
> +
>  POWER MANAGEMENT CORE
>  M:	"Rafael J. Wysocki" <rafael@kernel.org>
>  L:	linux-pm@vger.kernel.org
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 43407e76476b..5b61ab422953 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -779,6 +779,20 @@ config PCENGINES_APU2
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pcengines-apuv2.
>  
> +config PORTWELL_EC
> +	tristate "Portwell Embedded Controller driver"
> +	depends on X86 && WATCHDOG && GPIOLIB
> +	help
> +	  This driver provides support for the GPIO pins and watchdog timer
> +	  embedded in Portwell's EC.
> +
> +	  Theoretically, this driver should work on multiple Portwell platforms,
> +	  but it has only been tested on the Portwell NANO-6064 board.
> +	  If you encounter any issues on other boards, please report them.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called portwell-ec.
> +
>  config BARCO_P50_GPIO
>  	tristate "Barco P50 GPIO driver for identify LED/button"
>  	depends on GPIOLIB
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 650dfbebb6c8..83dd82e04457 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -92,6 +92,9 @@ obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
>  # PC Engines
>  obj-$(CONFIG_PCENGINES_APU2)	+= pcengines-apuv2.o
>  
> +# Portwell
> +obj-$(CONFIG_PORTWELL_EC)	+= portwell-ec.o
> +
>  # Barco
>  obj-$(CONFIG_BARCO_P50_GPIO)	+= barco-p50-gpio.o
>  
> diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
> new file mode 100644
> index 000000000000..59c51b80a148
> --- /dev/null
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * portwell-ec.c: Portwell embedded controller driver.
> + *
> + * Tested on:
> + *  - Portwell NANO-6064
> + *
> + * This driver provides support for GPIO and Watchdog Timer
> + * functionalities of the Portwell boards with ITE embedded controller (EC).
> + * The EC is accessed through I/O ports and provides:
> + *  - 8 GPIO pins for control and monitoring
> + *  - Hardware watchdog with 1-255 second timeout range
> + *
> + * It integrates with the Linux GPIO and Watchdog subsystems, allowing
> + * userspace interaction with EC GPIO pins and watchdog control,
> + * ensuring system stability and configurability.
> + *
> + * (C) Copyright 2025 Portwell, Inc.
> + * Author: Yen-Chi Huang (jesse.huang@portwell.com.tw)
> + *

Remove the extra line.

> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/watchdog.h>
> +#include <linux/io.h>
> +#include <linux/string.h>

Please sort alphabetically.

> +
> +#define PORTWELL_EC_IOSPACE 0xe300
> +#define PORTWELL_GPIO_PINS 8
> +#define PORTWELL_GPIO_DIR_REG 0x2b
> +#define PORTWELL_GPIO_VAL_REG 0x2c
> +
> +#define PORTWELL_WDT_EC_CONFIG_ADDR    0x06
> +#define PORTWELL_WDT_EC_COUNT_MIN_ADDR 0x07
> +#define PORTWELL_WDT_EC_COUNT_SEC_ADDR 0x08
> +#define PORTWELL_WDT_EC_MAX_COUNT_SECOND 255
> +#define PORTWELL_EC_FW_VENDOR_ADDRESS  0x4d
> +#define PORTWELL_EC_FW_VENDOR_LENGTH   3
> +#define PORTWELL_EC_FW_VENDOR_NAME "PWG"
> +
> + /* Functions for access EC via IOSPACE*/

Missing space.

> +static void pwec_write(u8 index, u8 data)
> +{
> +	outb(data, PORTWELL_EC_IOSPACE + index);
> +}
> +
> +static u8 pwec_read(u8 address)
> +{
> +	return inb(PORTWELL_EC_IOSPACE + address);

IIRC, CONFIG_HAS_IOPORT is these days required for iob/outb() so you 
should add depends on HAS_IOPORT into Kconfig.

> +}
> +
> +/* GPIO functions*/

Missing space.

> +static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return (pwec_read(PORTWELL_GPIO_VAL_REG) & (1 << offset)) ? 1 : 0;
> +}
> +
> +static void pwec_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
> +{
> +	u8 tmp = pwec_read(PORTWELL_GPIO_VAL_REG);
> +
> +	if (val)
> +		tmp |= (1 << offset);
> +	else
> +		tmp &= ~(1 << offset);
> +	pwec_write(PORTWELL_GPIO_VAL_REG, tmp);
> +}
> +
> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return (pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset))
> +			? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;

Please move ? to the preceeding line

I'd add a local variable for the read result to make this more readable.

> +}
> +
> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
> +	return -EOPNOTSUPP;
> +}
> +
> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +	/* Changing direction causes issues on some boards, so it's disabled for now. */

Perhaps just one comment above both functions would suffice. The functions 
are right after another so it seems overkill to have the same comment for 
both.

> +	return -EOPNOTSUPP;
> +}
> +
> +static struct gpio_chip pwec_gpio_chip = {
> +	.label = "portwell-ec-gpio",
> +	.get_direction = pwec_gpio_get_direction,
> +	.direction_input = pwec_gpio_direction_input,
> +	.direction_output = pwec_gpio_direction_output,
> +	.get = pwec_gpio_get,
> +	.set = pwec_gpio_set,
> +	.ngpio = PORTWELL_GPIO_PINS,
> +};
> +
> +/* Watchdog functions*/

Missing space

> +static int pwec_wdt_start(struct watchdog_device *wdd)
> +{
> +	int retry = 10;
> +	u8 timeout;
> +
> +	do {
> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01

Please use named defines and FIELD_PREP() instead of comments.

> +		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +		retry--;
> +	} while (timeout != wdd->timeout && retry > 0);
> +	pr_info("Portwell EC: Watchdog started with timeout: %d seconds\n", wdd->timeout);
> +	return (retry > 0) ? 0 : -EIO;
> +}
> +
> +static int pwec_wdt_stop(struct watchdog_device *wdd)
> +{
> +	pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x00);
> +	pr_info("Portwell EC: Watchdog stopped\n");
> +	return 0;
> +}
> +
> +static int pwec_wdt_trigger(struct watchdog_device *wdd)
> +{
> +	int retry = 10;
> +	u8 timeout;
> +
> +	pr_info("Portwell EC: Watchdog triggered with timeout: %d seconds\n", wdd->timeout);

This going to be pretty noisy.

> +	do {
> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
> +		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +		retry--;
> +	} while (timeout != wdd->timeout && retry > 0);
> +	return (retry > 0) ? 0 : -EIO;

Duplicated code.

> +}
> +
> +static int pwec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
> +{
> +	if (timeout == 0 || timeout > PORTWELL_WDT_EC_MAX_COUNT_SECOND)
> +		return -EINVAL;
> +	wdd->timeout = timeout;
> +	pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
> +	pr_info("Portwell EC: Watchdog timeout is set: %d seconds\n", wdd->timeout);
> +	return 0;
> +}
> +
> +static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	unsigned int timeout;
> +
> +	timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +	pr_info("Portwell EC: Watchdog timeout left: %d seconds\n", timeout);
> +	return timeout;
> +}
> +
> +static const struct watchdog_ops pwec_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pwec_wdt_start,
> +	.stop = pwec_wdt_stop,
> +	.ping = pwec_wdt_trigger,
> +	.set_timeout = pwec_wdt_set_timeout,
> +	.get_timeleft = pwec_wdt_get_timeleft,
> +};
> +
> +static struct watchdog_device ec_wdt_dev = {
> +	.info = &(struct watchdog_info){
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "Portwell EC Watchdog",
> +	},
> +	.ops = &pwec_wdt_ops,
> +	.timeout = 10,
> +	.min_timeout = 1,
> +	.max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,
> +};
> +
> +static int pwec_firmware_vendor_check(void)
> +{
> +	u8 buf[PORTWELL_EC_FW_VENDOR_LENGTH+1];

Add spaces around +

> +	u8 i;
> +
> +	for (i = 0; i < PORTWELL_EC_FW_VENDOR_LENGTH; i++)
> +		buf[i] = pwec_read(PORTWELL_EC_FW_VENDOR_ADDRESS+i);

Spaces around +

> +	buf[PORTWELL_EC_FW_VENDOR_LENGTH] = '\0';
> +	return (strcmp(PORTWELL_EC_FW_VENDOR_NAME, buf) == 0) ? 0 : -ENODEV;
> +}
> +
> +static int __init pwec_init(void)
> +{
> +	int result;
> +
> +	result = pwec_firmware_vendor_check();

So this goes immediately to poke some io ports? On any x86 machine?
The cases should be narrowed down first with dmi matching.

> +	if (result < 0)
> +		return result;
> +
> +	result = gpiochip_add_data(&pwec_gpio_chip, NULL);
> +	if (result < 0) {
> +		pr_err("Failed to register Portwell EC GPIO\n");
> +		return result;
> +	}
> +
> +	result = watchdog_register_device(&ec_wdt_dev);
> +	if (result < 0) {
> +		gpiochip_remove(&pwec_gpio_chip);
> +		pr_err("Failed to register Portwell EC Watchdog\n");
> +		return result;
> +	}
> +
> +	pr_info("Portwell EC driver initialized\n");

Ok path should be silent.

> +	return 0;
> +}
> +
> +static void __exit pwec_exit(void)
> +{
> +	watchdog_unregister_device(&ec_wdt_dev);
> +	gpiochip_remove(&pwec_gpio_chip);
> +	pr_info("Portwell EC driver removed\n");

Remove print.

> +}
> +
> +module_init(pwec_init);
> +module_exit(pwec_exit);
> +
> +MODULE_AUTHOR("Yen-Chi Huang");
> +MODULE_DESCRIPTION("Portwell EC Driver");
> +MODULE_LICENSE("GPL");
> 

-- 
 i.

On Thu, 3 Apr 2025, Ilpo Järvinen wrote:
> On Thu, 3 Apr 2025, Yen-Chi Huang wrote:
> 
> Please add watchdog drivers people/lists from MAINTAINERS file into the 
> next submission.

Will add GPIO and watchdog driver maintainers/lists in PATCH v2.

>> +static u8 pwec_read(u8 address)
>> +{
>> +	return inb(PORTWELL_EC_IOSPACE + address);
> 
> IIRC, CONFIG_HAS_IOPORT is these days required for iob/outb() so you 
> should add depends on HAS_IOPORT into Kconfig.

Fixed by adding `depends on HAS_IOPORT` to Kconfig in PATCH v2.

>> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +	return (pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset))
>> +			? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> 
> Please move ? to the preceeding line
> 
> I'd add a local variable for the read result to make this more readable.
Fixed by rewriting this using a local variable and `if...else` in PATCH v2.

>> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
> 
> Perhaps just one comment above both functions would suffice. The functions 
> are right after another so it seems overkill to have the same comment for 
> both.

Fixed by combining the comments into one above both functions in PATCH v2.

>> +static int pwec_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	int retry = 10;
>> +	u8 timeout;
>> +
>> +	do {
>> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
>> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
> 
> Please use named defines and FIELD_PREP() instead of comments.

Fixed by defining `PORTWELL_WDT_CONFIG_ENABLE` and `PORTWELL_WDT_CONFIG_DISABLE`,  
and replacing the hardcoded value with named constants in PATCH v2.  
>> +	do {
>> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
>> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
>> +		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
>> +		retry--;
>> +	} while (timeout != wdd->timeout && retry > 0);
>> +	return (retry > 0) ? 0 : -EIO;
> 
> Duplicated code.

Fixed by reusing `pwec_wdt_trigger()` from `pwec_wdt_start()` to eliminate duplication in PATCH v2.

>> +static int __init pwec_init(void)
>> +{
>> +	int result;
>> +
>> +	result = pwec_firmware_vendor_check();
> 
> So this goes immediately to poke some io ports? On any x86 machine?
> The cases should be narrowed down first with dmi matching.

Will add `dmi_check_system()` to limit EC access to supported platforms in PATCH v2.

The following style and formatting issues were also pointed out and have been fixed:
- Removed extra blank line in the file header comment block
- Sorted header includes
- Fixed missing blank lines before section headers (3 instances)
- Fixed 4 spacing issues around '+' operators
- Removed 7 `pr_info()` calls from success paths

Appreciate your detailed review. it was very helpful.

Best Regards,
Yen-Chi Huang

Hi Ilpo,

Resend this reply due to encoding issue in the previous message.

Thank you for the review.

On Thu, 3 Apr 2025, Ilpo Jarvinen wrote:
> On Thu, 3 Apr 2025, Yen-Chi Huang wrote:
>
> Please add watchdog drivers people/lists from MAINTAINERS file into the 
> next submission.

Will add GPIO and watchdog driver maintainers/lists in PATCH v2.

>> +static u8 pwec_read(u8 address)
>> +{
>> +	return inb(PORTWELL_EC_IOSPACE + address);
>
> IIRC, CONFIG_HAS_IOPORT is these days required for iob/outb() so you 
> should add depends on HAS_IOPORT into Kconfig.

Fixed by adding `depends on HAS_IOPORT` to Kconfig in PATCH v2.

>> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +	return (pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset))
>> +			? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>
> Please move ? to the preceeding line
>
> I'd add a local variable for the read result to make this more readable.
Fixed by rewriting this using a local variable and `if...else` in PATCH v2.

>> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> +	/* Changing direction causes issues on some boards, so it's disabled for now. */
>
> Perhaps just one comment above both functions would suffice. The functions 
> are right after another so it seems overkill to have the same comment for 
> both.

Fixed by combining the comments into one above both functions in PATCH v2.

>> +static int pwec_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	int retry = 10;
>> +	u8 timeout;
>> +
>> +	do {
>> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
>> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
>
> Please use named defines and FIELD_PREP() instead of comments.

Fixed by defining `PORTWELL_WDT_CONFIG_ENABLE` and `PORTWELL_WDT_CONFIG_DISABLE`,  
and replacing the hardcoded value with named constants in PATCH v2.  
>> +	do {
>> +		pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
>> +		pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
>> +		timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
>> +		retry--;
>> +	} while (timeout != wdd->timeout && retry > 0);
>> +	return (retry > 0) ? 0 : -EIO;
>
> Duplicated code.

Fixed by reusing `pwec_wdt_trigger()` from `pwec_wdt_start()` to eliminate duplication in PATCH v2.

>> +static int __init pwec_init(void)
>> +{
>> +	int result;
>> +
>> +	result = pwec_firmware_vendor_check();
>
> So this goes immediately to poke some io ports? On any x86 machine?
> The cases should be narrowed down first with dmi matching.

Will add `dmi_check_system()` to limit EC access to supported platforms in PATCH v2.

The following style and formatting issues were also pointed out and have been fixed:
- Removed extra blank line in the file header comment block
- Sorted header includes
- Fixed missing blank lines before section headers (3 instances)
- Fixed 4 spacing issues around '+' operators
- Removed 7 `pr_info()` calls from success paths

Appreciate your detailed review. it was very helpful.

Best Regards,
Yen-Chi Huang