[PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch series aims to add support for the Advantech EIO-IS200
Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200
is a widely used embedded controller, and this series introduces a
native driver for its watchdog timer functionality within the Linux
ecosystem.

Driver Features:
- Complete support for the Advantech EIO-IS200 Embedded Controller's
  hardware watchdog timer.
- Seamless integration with the Linux Watchdog framework, enabling
  standard watchdog functionality.
- Flexible configuration options for watchdog timeout and event types.
- Module parameters for setting default timeout and event type.
- The EIO-IS200 can select special event output pin such as PWRBTN
  (Power button),SCI (ACPI System Control Interrupt), IRQ, and GPIO

Driver Dependencies:
- The driver relies on the Linux Multi-Function Device (MFD) framework
  and related dependencies.
- Assumption of the presence of the Advantech eiois200_core MFD core
  driver.

Wenkai (5):
  watchdog: eiois200_wdt: Constructing Advantech EIO-IS200 watchdog
			  driver
  watchdog: eiois200_wdt: Add PMC support with eiois200_core.
  watchdog: eiois200_wdt: Implement basic watchdog functionalities
  watchdog: eiois200_wdt: Enhanced watchdog functionality and pretimeout
  watchdog: eiois200_wdt: Enhanced IRQ trigger behavior

 drivers/watchdog/Kconfig        |  14 +
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/eiois200_wdt.c | 658 ++++++++++++++++++++++++++++++++
 3 files changed, 673 insertions(+)
 create mode 100644 drivers/watchdog/eiois200_wdt.c


base-commit: 9aab92bc3a8922d4b2e24d10271dfe3034cbf5c2
-- 
2.34.1

[PATCH 1/5] watchdog: eiois200_wdt: Constructing Advantech EIO-IS200 watchdog driver

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This is the initial patch that adds the Advantech EIO-IS200 Watchdog
Driver to the kernel. It includes the necessary Makefile and Kconfig
entries to integrate the driver with the watchdog framework. However,
please note that this patch does not yet implement the actual watchdog
functionality. When configuring the kernel using tools like menuconfig,
as all other eiois200 series drivers, it cannot be selected Y as built
in, you can select M to build it as a module or select N to exclude it.

Please also be aware that this driver is a sub-driver of
`drivers/mfd/eiois200_core`. Therefore, it is required for the proper
compilation of the Advantech EIO-IS200 support. Failing to include this
driver may result in compilation errors.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/watchdog/Kconfig        |  14 ++++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/eiois200_wdt.c | 124 ++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+)
 create mode 100644 drivers/watchdog/eiois200_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ee97d89dfc11..218c90714830 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1079,6 +1079,20 @@ config ADVANTECH_EC_WDT
 		This driver supports Advantech products with ITE based Embedded Controller.
 		It does not support Advantech products with other ECs or without EC.
 
+config EIOIS200_WDT
+	tristate "Advantech EIO IS-200 Watchdog Timer"
+	depends on X86 && m
+	default m
+	select MFD_EIOIS200
+	help
+	  This is the driver for the hardware watchdog on the EIO IS-200 EC
+	  chip as used in Advantech boards (and likely others).
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called eiois200_wdt.
+
+	  Most people will say M.
+
 config ALIM1535_WDT
 	tristate "ALi M1535 PMU Watchdog Timer"
 	depends on X86 && PCI
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 3633f5b98236..713872a4f7de 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -103,6 +103,7 @@ obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
 obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
 obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
 obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
+obj-$(CONFIG_EIOIS200_WDT) += eiois200_wdt.o
 obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
 obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
 obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
diff --git a/drivers/watchdog/eiois200_wdt.c b/drivers/watchdog/eiois200_wdt.c
new file mode 100644
index 000000000000..bf132a75a2ec
--- /dev/null
+++ b/drivers/watchdog/eiois200_wdt.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Advantech EIO-IS200 Watchdog Driver
+ *
+ * Copyright (C) 2023 Advantech Co., Ltd.
+ * Author: wenkai <advantech.susiteam@gmail.com>
+ */
+
+#include <linux/mfd/core.h>
+#include <linux/reboot.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#include "../mfd/eiois200.h"
+
+#define WATCHDOG_TIMEOUT	60
+#define WATCHDOG_PRETIMEOUT	10
+
+static struct _wdt {
+	u32	support;
+	long	last_time;
+	struct	regmap  *iomap;
+	struct	device *dev;
+} wdt;
+
+/* Pointer to the eiois200_core device structure */
+static struct eiois200_dev *eiois200_dev;
+
+static struct watchdog_info wdinfo = {
+	.identity = KBUILD_MODNAME,
+	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+		    WDIOF_MAGICCLOSE,
+};
+
+static struct watchdog_device wddev = {
+	.info	     = &wdinfo,
+	.max_timeout = 0x7FFF,
+	.min_timeout = 1,
+};
+
+static int wdt_set_timeout(struct watchdog_device *dev,
+			   unsigned int _timeout)
+{
+	dev->timeout = _timeout;
+	dev_dbg(wdt.dev, "Set timeout: %d\n", _timeout);
+
+	return 0;
+}
+
+static int wdt_start(struct watchdog_device *dev)
+{
+	return 0;
+}
+
+static int wdt_stop(struct watchdog_device *dev)
+{
+	return 0;
+}
+
+static int wdt_ping(struct watchdog_device *dev)
+{
+	return 0;
+}
+
+static unsigned int wdt_get_timeleft(struct watchdog_device *dev)
+{
+	return 0;
+}
+
+static const struct watchdog_ops wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= wdt_start,
+	.stop		= wdt_stop,
+	.ping		= wdt_ping,
+	.set_timeout	= wdt_set_timeout,
+	.get_timeleft	= wdt_get_timeleft,
+};
+
+static int wdt_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct device *dev = &pdev->dev;
+
+	/* Contact eiois200_core */
+	eiois200_dev = dev_get_drvdata(dev->parent);
+	if (!eiois200_dev)
+		return dev_err_probe(dev, ret,
+				     "Error contact eiois200_core %d\n", ret);
+
+	wdt.dev = dev;
+	wdt.iomap = dev_get_regmap(dev->parent, NULL);
+	if (!wdt.iomap)
+		return dev_err_probe(dev, -ENOMEM, "Query parent regmap fail\n");
+
+	/* Inform watchdog info */
+	wddev.ops = &wdt_ops;
+	ret = watchdog_init_timeout(&wddev, wddev.timeout, dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Init timeout fail\n");
+
+	watchdog_stop_on_reboot(&wddev);
+
+	watchdog_stop_on_unregister(&wddev);
+
+	/* Register watchdog */
+	ret = devm_watchdog_register_device(dev, &wddev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Cannot register watchdog device (err: %d)\n",
+				     ret);
+
+	return 0;
+}
+
+static struct platform_driver eiois200_wdt_driver = {
+	.driver = {
+		.name  = "eiois200_wdt",
+	},
+};
+module_platform_driver_probe(eiois200_wdt_driver, wdt_probe);
+
+MODULE_AUTHOR("wenkai <advantech.susiteam@gmail.com>");
+MODULE_DESCRIPTION("Watchdog interface for Advantech EIO-IS200 embedded controller");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 1/5] watchdog: eiois200_wdt: Constructing Advantech EIO-IS200 watchdog driver

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9aab92bc3a8922d4b2e24d10271dfe3034cbf5c2]

url:    https://github.com/intel-lab-lkp/linux/commits/advantech-susiteam-gmail-com/watchdog-eiois200_wdt-Constructing-Advantech-EIO-IS200-watchdog-driver/20231005-231419
base:   9aab92bc3a8922d4b2e24d10271dfe3034cbf5c2
patch link:    https://lore.kernel.org/r/b627f827d13514c1746beb31bb80db71023e9bb1.1696495372.git.advantech.susiteam%40gmail.com
patch subject: [PATCH 1/5] watchdog: eiois200_wdt: Constructing Advantech EIO-IS200 watchdog driver
config: i386-randconfig-003-20231006 (https://download.01.org/0day-ci/archive/20231006/202310060719.v6gRNpBq-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231006/202310060719.v6gRNpBq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310060719.v6gRNpBq-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/watchdog/eiois200_wdt.c:14:10: fatal error: ../mfd/eiois200.h: No such file or directory
      14 | #include "../mfd/eiois200.h"
         |          ^~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +14 drivers/watchdog/eiois200_wdt.c

    13	
  > 14	#include "../mfd/eiois200.h"
    15	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/5] watchdog: eiois200_wdt: Add PMC support with eiois200_core.

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

All eiois200 series driver functionalities are based on PMC (Peripheral
Management Controller) communication. This patch enables communication
between the driver and the eiois200_core. However, this version does
not yet implement the watchdog timer's functionality.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/watchdog/eiois200_wdt.c | 114 ++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/watchdog/eiois200_wdt.c b/drivers/watchdog/eiois200_wdt.c
index bf132a75a2ec..ce4435ac62f2 100644
--- a/drivers/watchdog/eiois200_wdt.c
+++ b/drivers/watchdog/eiois200_wdt.c
@@ -16,6 +16,40 @@
 #define WATCHDOG_TIMEOUT	60
 #define WATCHDOG_PRETIMEOUT	10
 
+/* Support Flags */
+#define SUPPORT_AVAILABLE	BIT(0)
+#define SUPPORT_RESET		BIT(7)
+
+/* PMC registers */
+#define REG_STATUS		0x00
+#define REG_CONTROL		0x02
+#define REG_EVENT		0x10
+#define REG_RESET_EVENT_TIME	0x14
+#define REG_IRQ_NUMBER		0x17
+
+/* PMC command and control */
+#define CMD_WDT_WRITE		0x2A
+#define CMD_WDT_READ		0x2B
+#define CTRL_STOP		0x00
+#define CTRL_START		0x01
+#define CTRL_TRIGGER		0x02
+
+/* I/O register and its flags */
+#define IOREG_UNLOCK		0x87
+#define IOREG_LOCK		0xAA
+#define IOREG_LDN		0x07
+#define IOREG_LDN_PMCIO		0x0F
+#define IOREG_IRQ		0x70
+#define IOREG_WDT_STATUS	0x30
+
+/* Flags */
+#define FLAG_WDT_ENABLED	0x01
+#define FLAG_TRIGGER_IRQ	BIT(4)
+
+/* PMC read and write a value */
+#define PMC_WRITE(cmd, data)	pmc(CMD_WDT_WRITE, cmd, data)
+#define PMC_READ(cmd, data)	pmc(CMD_WDT_READ, cmd, data)
+
 static struct _wdt {
 	u32	support;
 	long	last_time;
@@ -47,6 +81,48 @@ static int wdt_set_timeout(struct watchdog_device *dev,
 	return 0;
 }
 
+static int pmc(u8 cmd, u8 ctrl, void *payload)
+{
+	struct pmc_op op = {
+		.cmd      = cmd,
+		.control  = ctrl,
+		.size     = ctrl <= REG_EVENT	   ? 1 :
+			    ctrl >= REG_IRQ_NUMBER ? 1 : 4,
+		.payload  = payload,
+	};
+
+	return eiois200_core_pmc_operation(wdt.dev, &op);
+}
+
+static int get_time(u8 ctrl, u32 *val)
+{
+	int ret;
+
+	ret = PMC_READ(ctrl, val);
+
+	/* ms to sec */
+	*val /= 1000;
+
+	return ret;
+}
+
+
+static int wdt_get_config(void)
+{
+	int ret;
+	u32 reset_time;
+
+	/* Get Reset Time */
+	ret = get_time(REG_RESET_EVENT_TIME, &reset_time);
+	if (ret)
+		return ret;
+
+	dev_info(wdt.dev, "Timeout H/W default timeout: %d secs\n", reset_time);
+	wddev.timeout	 = reset_time;
+
+	return 0;
+}
+
 static int wdt_start(struct watchdog_device *dev)
 {
 	return 0;
@@ -67,6 +143,40 @@ static unsigned int wdt_get_timeleft(struct watchdog_device *dev)
 	return 0;
 }
 
+static int wdt_support(void)
+{
+	u8 support;
+
+	if (PMC_READ(REG_STATUS, &support))
+		return -EIO;
+
+	if ((support & SUPPORT_AVAILABLE) == 0)
+		return -EIO;
+
+	/* Must support reset */
+	if ((support & SUPPORT_RESET) != SUPPORT_RESET)
+		return -EIO;
+
+	/* Must has support event **/
+	wdt.support = support;
+
+	return 0;
+}
+static int wdt_init(struct device *dev)
+{
+	int ret = 0;
+
+	ret = wdt_support();
+	if (ret)
+		return ret;
+
+	ret = wdt_get_config();
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
 static const struct watchdog_ops wdt_ops = {
 	.owner		= THIS_MODULE,
 	.start		= wdt_start,
@@ -91,6 +201,10 @@ static int wdt_probe(struct platform_device *pdev)
 	wdt.iomap = dev_get_regmap(dev->parent, NULL);
 	if (!wdt.iomap)
 		return dev_err_probe(dev, -ENOMEM, "Query parent regmap fail\n");
+	
+	/* Initialize EC watchdog */
+	if (wdt_init(dev))
+		return dev_err_probe(dev, -EIO, "wdt_init fail\n");
 
 	/* Inform watchdog info */
 	wddev.ops = &wdt_ops;
-- 
2.34.1

[PATCH 3/5] watchdog: eiois200_wdt: Implement basic watchdog functionalities

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

In this patch, the driver has been extended to include basic watchdog
functionality, allowing users to configure the watchdog timeout duration,
start and stop the watchdog timer, and ping it to prevent system resets.
The driver also reports the remaining time until a watchdog reset is
triggered.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/watchdog/eiois200_wdt.c | 64 ++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/eiois200_wdt.c b/drivers/watchdog/eiois200_wdt.c
index ce4435ac62f2..569e619448e5 100644
--- a/drivers/watchdog/eiois200_wdt.c
+++ b/drivers/watchdog/eiois200_wdt.c
@@ -106,6 +106,29 @@ static int get_time(u8 ctrl, u32 *val)
 	return ret;
 }
 
+static int set_time(u8 ctl, u32 time)
+{
+	/* sec to sec */
+	time *= 1000;
+
+	return PMC_WRITE(ctl, &time);
+}
+
+static int wdt_set_config(void)
+{
+	int ret;
+	u32 reset_time = 0;
+
+	reset_time = wddev.timeout;
+
+	ret = set_time(REG_RESET_EVENT_TIME, reset_time);
+	if (ret)
+		return ret;
+
+	dev_info(wdt.dev, "Config wdt reset time %d\n", reset_time);
+
+	return ret;
+}
 
 static int wdt_get_config(void)
 {
@@ -123,24 +146,57 @@ static int wdt_get_config(void)
 	return 0;
 }
 
+static int set_ctrl(u8 data)
+{
+	return PMC_WRITE(REG_CONTROL, &data);
+}
+
 static int wdt_start(struct watchdog_device *dev)
 {
-	return 0;
+	int ret;
+
+	ret = wdt_set_config();
+	if (ret)
+		return ret;
+
+	ret = set_ctrl(CTRL_START);
+	if (ret == 0) {
+		wdt.last_time = jiffies;
+		dev_dbg(wdt.dev, "Watchdog started\n");
+	}
+
+	return ret;
 }
 
 static int wdt_stop(struct watchdog_device *dev)
 {
-	return 0;
+	dev_dbg(wdt.dev, "Watchdog stopped\n");
+	wdt.last_time = 0;
+
+	return set_ctrl(CTRL_STOP);
 }
 
 static int wdt_ping(struct watchdog_device *dev)
 {
-	return 0;
+	int ret;
+
+	dev_dbg(wdt.dev, "Watchdog pings\n");
+
+	ret = set_ctrl(CTRL_TRIGGER);
+	if (ret == 0)
+		wdt.last_time = jiffies;
+
+	return ret;
 }
 
 static unsigned int wdt_get_timeleft(struct watchdog_device *dev)
 {
-	return 0;
+	unsigned int timeleft = 0;
+
+	if (wdt.last_time != 0)
+		timeleft = wddev.timeout - ((jiffies - wdt.last_time) / HZ);
+
+	return timeleft;
 }
 
 static int wdt_support(void)
-- 
2.34.1

[PATCH 4/5] watchdog: eiois200_wdt: Enhanced watchdog functionality and pretimeout

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch extends the Advantech EIO-IS200 Watchdog Driver to provide
advanced watchdog functionality, including pretimeout support. It allows
the user to specify a timeout or pre-timeout trigger event, let the
event pin output level switches from high to low. The event pin which
can be one of the following:
- PWRBTN (Power button)
- SCI (ACPI System Control Interrupt)
- IRQ
- GPIO

If the pretimeout is specified, when the pretimeout time expires, it
triggers the associated pin. If the timeout expires, it triggers a reset.

If the pretimeout is not specified, the timeout expiration triggers the
associated pin or the reset pin.

This addition to basic watchdog functionality. It ensures proper
integration with the watchdog framework and provides a flexible watchdog
solution for Advantech EIO-IS200-based systems.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/watchdog/eiois200_wdt.c | 159 ++++++++++++++++++++++++++++++--
 1 file changed, 152 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/eiois200_wdt.c b/drivers/watchdog/eiois200_wdt.c
index 569e619448e5..85179806ab7e 100644
--- a/drivers/watchdog/eiois200_wdt.c
+++ b/drivers/watchdog/eiois200_wdt.c
@@ -18,13 +18,21 @@
 
 /* Support Flags */
 #define SUPPORT_AVAILABLE	BIT(0)
+#define SUPPORT_PWRBTN		BIT(3)
+#define SUPPORT_IRQ		BIT(4)
+#define SUPPORT_SCI		BIT(5)
+#define SUPPORT_PIN		BIT(6)
 #define SUPPORT_RESET		BIT(7)
 
 /* PMC registers */
 #define REG_STATUS		0x00
 #define REG_CONTROL		0x02
 #define REG_EVENT		0x10
+#define REG_PWR_EVENT_TIME	0x12
+#define REG_IRQ_EVENT_TIME	0x13
 #define REG_RESET_EVENT_TIME	0x14
+#define REG_PIN_EVENT_TIME	0x15
+#define REG_SCI_EVENT_TIME	0x16
 #define REG_IRQ_NUMBER		0x17
 
 /* PMC command and control */
@@ -50,20 +58,53 @@
 #define PMC_WRITE(cmd, data)	pmc(CMD_WDT_WRITE, cmd, data)
 #define PMC_READ(cmd, data)	pmc(CMD_WDT_READ, cmd, data)
 
+/* Mapping event type to supported bit */
+#define EVENT_BIT(type)   	BIT(type + 2)
+
+enum event_type {
+	EVENT_NONE,
+	EVENT_PWRBTN,
+	EVENT_IRQ,
+	EVENT_SCI,
+	EVENT_PIN
+};
+
 static struct _wdt {
+	u32	event_type;
 	u32	support;
 	long	last_time;
 	struct	regmap  *iomap;
 	struct	device *dev;
 } wdt;
 
+static char * const type_strs[] = {
+	"NONE",
+	"PWRBTN",
+	"IRQ",
+	"SCI",
+	"PIN",
+};
+
+static u32 type_regs[] = {
+	REG_RESET_EVENT_TIME,
+	REG_PWR_EVENT_TIME,
+	REG_IRQ_EVENT_TIME,
+	REG_SCI_EVENT_TIME,
+	REG_PIN_EVENT_TIME,
+};
+
 /* Pointer to the eiois200_core device structure */
 static struct eiois200_dev *eiois200_dev;
 
+/* Specify the pin triggered on pretimeout or timeout */
+static char *event_type = "NONE";
+module_param(event_type, charp, 0);
+MODULE_PARM_DESC(event_type,
+		 "Watchdog timeout event type (RESET, PWRBTN, SCI, IRQ, GPIO)");
 static struct watchdog_info wdinfo = {
 	.identity = KBUILD_MODNAME,
 	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
-		    WDIOF_MAGICCLOSE,
+		    WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
 };
 
 static struct watchdog_device wddev = {
@@ -76,8 +117,42 @@ static int wdt_set_timeout(struct watchdog_device *dev,
 			   unsigned int _timeout)
 {
 	dev->timeout = _timeout;
-	dev_dbg(wdt.dev, "Set timeout: %d\n", _timeout);
+	dev_info(wdt.dev, "Set timeout: %d\n", _timeout);
+
+	return 0;
+}
+
+static int wdt_set_pretimeout(struct watchdog_device *dev,
+			      unsigned int _pretimeout)
+{
+	dev->pretimeout = _pretimeout;
+
+	dev_info(wdt.dev, "Set pretimeout: %d\n", _pretimeout);
+
+	return 0;
+}
+
+static int wdt_get_type(void)
+{
+	int i;
+
+	for (i = 1; i < ARRAY_SIZE(type_strs); i++)
+		if (strcasecmp(event_type, type_strs[i]) == 0) {
+			if ((wdt.support & EVENT_BIT(i)) == 0) {
+				dev_err(wdt.dev,
+					"This board doesn't support %s trigger type\n",
+					event_type);
+				return -EINVAL;
+			}
+
+			dev_info(wdt.dev, "Trigger type is %d:%s\n", 
+					  i, type_strs[i]);
+			wdt.event_type = i;
+
+			return 0;
+		}
 
+	dev_info(wdt.dev, "Event type: %s\n", type_strs[wdt.event_type]);
 	return 0;
 }
 
@@ -116,33 +191,98 @@ static int set_time(u8 ctl, u32 time)
 
 static int wdt_set_config(void)
 {
-	int ret;
+	int ret, type;
+	u32 event_time = 0;
 	u32 reset_time = 0;
 
+	/* event_type should never out of range */
+	if (wdt.event_type > EVENT_PIN)
+		return -EFAULT;
+
+	/* Calculate event time and reset time */
+	if (wddev.pretimeout && wddev.timeout) {
+		if (wddev.timeout < wddev.pretimeout)
+			return -EINVAL;
+
 	reset_time = wddev.timeout;
+		event_time = wddev.timeout - wddev.pretimeout;
 
+	} else if (wddev.timeout) {
+		reset_time = wdt.event_type ? 0	: wddev.timeout;
+		event_time = wdt.event_type ? wddev.timeout : 0;
+	}
+
+	/* Set reset time */
 	ret = set_time(REG_RESET_EVENT_TIME, reset_time);
 	if (ret)
 		return ret;
 
-	dev_info(wdt.dev, "Config wdt reset time %d\n", reset_time);
+	/* Set every other times */
+	for (type = 1; type < ARRAY_SIZE(type_regs); type++) {
+		ret = set_time(type_regs[type],
+			       wdt.event_type == type ? event_time : 0);
+		if (ret)
+			return ret;
+	}
+
+	dev_dbg(wdt.dev, "Config wdt reset time %d\n", reset_time);
+	dev_dbg(wdt.dev, "Config wdt event time %d\n", event_time);
+	dev_dbg(wdt.dev, "Config wdt event type %s\n",
+			  type_strs[wdt.event_type]);
 
 	return ret;
 }
 
 static int wdt_get_config(void)
 {
-	int ret;
-	u32 reset_time;
+	int ret, type;
+	u32 event_time, reset_time;
 
 	/* Get Reset Time */
 	ret = get_time(REG_RESET_EVENT_TIME, &reset_time);
 	if (ret)
 		return ret;
 
-	dev_info(wdt.dev, "Timeout H/W default timeout: %d secs\n", reset_time);
+	dev_dbg(wdt.dev, "Timeout H/W default timeout: %d secs\n", reset_time);
+
+	/* Get every other times **/
+	for (type = 1; type < ARRAY_SIZE(type_regs); type++) {
+		if ((wdt.support & EVENT_BIT(type)) == 0)
+			continue;
+
+		ret = get_time(type_regs[type], &event_time);
+		if (ret)
+			return ret;
+
+		if (event_time == 0)
+			continue;
+
+		if (reset_time) {
+			if (reset_time < event_time)
+				continue;
+
 	wddev.timeout	 = reset_time;
+			wddev.pretimeout = reset_time - event_time;
+
+			dev_dbg(wdt.dev, "Pretimeout H/W enabled with event %s of %d secs\n",
+				 type_strs[type], wddev.pretimeout);
+		} else {
+			wddev.timeout = event_time;
+			wddev.pretimeout = 0;
+		}
+
+		wdt.event_type = type;
+
+		dev_dbg(wdt.dev, "Timeout H/W enabled of %d secs\n",
+				  wddev.timeout);
+		return 0;
+	}
+
+	wdt.event_type	 = EVENT_NONE;
+	wddev.pretimeout = reset_time ? 0	   : WATCHDOG_PRETIMEOUT;
+	wddev.timeout	 = reset_time ? reset_time : WATCHDOG_TIMEOUT;
 
+	dev_dbg(wdt.dev, "Pretimeout H/W disabled");
 	return 0;
 }
 
@@ -218,6 +358,7 @@ static int wdt_support(void)
 
 	return 0;
 }
+
 static int wdt_init(struct device *dev)
 {
 	int ret = 0;
@@ -230,6 +371,9 @@ static int wdt_init(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = wdt_get_type();
+	if (ret)
+		return ret;
 	return ret;
 }
 
@@ -240,6 +384,7 @@ static const struct watchdog_ops wdt_ops = {
 	.ping		= wdt_ping,
 	.set_timeout	= wdt_set_timeout,
 	.get_timeleft	= wdt_get_timeleft,
+	.set_pretimeout = wdt_set_pretimeout,
 };
 
 static int wdt_probe(struct platform_device *pdev)
-- 
2.34.1

[PATCH 5/5] watchdog: eiois200_wdt: Enhanced IRQ trigger behavior

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch improved functionality when IRQ is used as the trigger event.

When the IRQ event selected. If the pretimeout is specified, when the
pretimeout time expires, it will trigger the associated IRQ event and
invoke the pretimeout governor's actions.

And, if the pretimeout is not specified, when the timeout time expires,
the driver initiates an emergency system restart.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/watchdog/eiois200_wdt.c | 237 ++++++++++++++++++++++++++++++--
 1 file changed, 228 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/eiois200_wdt.c b/drivers/watchdog/eiois200_wdt.c
index 85179806ab7e..c9acb63b1152 100644
--- a/drivers/watchdog/eiois200_wdt.c
+++ b/drivers/watchdog/eiois200_wdt.c
@@ -2,10 +2,34 @@
 /*
  * Advantech EIO-IS200 Watchdog Driver
  *
+ * This driver enables watchdog functionality for the Advantech EIO-IS200
+ * embedded controller. Its has a dependency on the eiois200_core module.
+ * It allows the specification of a timeout or pretimeout associated trigger
+ * event, which can be one of the following pins:
+ * - PWRBTN (Power button)
+ * - SCI (ACPI System Control Interrupt)
+ * - IRQ
+ * - GPIO
+ *
+ * If the pretimeout is specified, when the pretimeout time expires, it
+ * triggers the associated pin; if the timeout expires, it always triggers
+ * a reset. If the associated pin is IRQ, the IRQ will trigger the system's
+ * original pretimeout behavior through the pretimeout governor.
+ *
+ * If the pretimeout is not specified, the timeout expiration triggers the
+ * associated pin only. If the associated pin is IRQ, it triggers a system
+ * emergency restart.
+ *
+ * NOTE: Advantech machines are shipped with proper IRQ and related event
+ * configurations. If you are unsure about these settings, just keep the
+ * device's default settings, and load this module without specifying any
+ * parameters.
+ *
  * Copyright (C) 2023 Advantech Co., Ltd.
  * Author: wenkai <advantech.susiteam@gmail.com>
  */
 
+#include <linux/interrupt.h>
 #include <linux/mfd/core.h>
 #include <linux/reboot.h>
 #include <linux/uaccess.h>
@@ -59,7 +83,7 @@
 #define PMC_READ(cmd, data)	pmc(CMD_WDT_READ, cmd, data)
 
 /* Mapping event type to supported bit */
-#define EVENT_BIT(type)   	BIT(type + 2)
+#define EVENT_BIT(type)	BIT(type + 2)
 
 enum event_type {
 	EVENT_NONE,
@@ -72,6 +96,7 @@ enum event_type {
 static struct _wdt {
 	u32	event_type;
 	u32	support;
+	u32	irq;
 	long	last_time;
 	struct	regmap  *iomap;
 	struct	device *dev;
@@ -101,6 +126,12 @@ static char *event_type = "NONE";
 module_param(event_type, charp, 0);
 MODULE_PARM_DESC(event_type,
 		 "Watchdog timeout event type (RESET, PWRBTN, SCI, IRQ, GPIO)");
+
+/* Specify the IRQ number when the IRQ event is triggered */
+static int irq;
+module_param(irq, int, 0);
+MODULE_PARM_DESC(irq, "The IRQ number for IRQ event");
+
 static struct watchdog_info wdinfo = {
 	.identity = KBUILD_MODNAME,
 	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
@@ -145,8 +176,8 @@ static int wdt_get_type(void)
 				return -EINVAL;
 			}
 
-			dev_info(wdt.dev, "Trigger type is %d:%s\n", 
-					  i, type_strs[i]);
+			dev_info(wdt.dev, "Trigger type is %d:%s\n",
+				 i, type_strs[i]);
 			wdt.event_type = i;
 
 			return 0;
@@ -204,7 +235,7 @@ static int wdt_set_config(void)
 		if (wddev.timeout < wddev.pretimeout)
 			return -EINVAL;
 
-	reset_time = wddev.timeout;
+		reset_time = wddev.timeout;
 		event_time = wddev.timeout - wddev.pretimeout;
 
 	} else if (wddev.timeout) {
@@ -228,7 +259,7 @@ static int wdt_set_config(void)
 	dev_dbg(wdt.dev, "Config wdt reset time %d\n", reset_time);
 	dev_dbg(wdt.dev, "Config wdt event time %d\n", event_time);
 	dev_dbg(wdt.dev, "Config wdt event type %s\n",
-			  type_strs[wdt.event_type]);
+		type_strs[wdt.event_type]);
 
 	return ret;
 }
@@ -261,11 +292,11 @@ static int wdt_get_config(void)
 			if (reset_time < event_time)
 				continue;
 
-	wddev.timeout	 = reset_time;
+			wddev.timeout = reset_time;
 			wddev.pretimeout = reset_time - event_time;
 
 			dev_dbg(wdt.dev, "Pretimeout H/W enabled with event %s of %d secs\n",
-				 type_strs[type], wddev.pretimeout);
+				type_strs[type], wddev.pretimeout);
 		} else {
 			wddev.timeout = event_time;
 			wddev.pretimeout = 0;
@@ -274,7 +305,7 @@ static int wdt_get_config(void)
 		wdt.event_type = type;
 
 		dev_dbg(wdt.dev, "Timeout H/W enabled of %d secs\n",
-				  wddev.timeout);
+			wddev.timeout);
 		return 0;
 	}
 
@@ -359,6 +390,180 @@ static int wdt_support(void)
 	return 0;
 }
 
+static int wdt_get_irq_io(void)
+{
+	int ret  = 0;
+	int idx  = EIOIS200_PNP_INDEX;
+	int data = EIOIS200_PNP_DATA;
+	struct regmap *map = wdt.iomap;
+
+	mutex_lock(&eiois200_dev->mutex);
+
+	/* Unlock EC IO port */
+	ret |= regmap_write(map, idx,  IOREG_UNLOCK);
+	ret |= regmap_write(map, idx,  IOREG_UNLOCK);
+
+	/* Select logical device to PMC */
+	ret |= regmap_write(map, idx,  IOREG_LDN);
+	ret |= regmap_write(map, data, IOREG_LDN_PMCIO);
+
+	/* Get IRQ number */
+	ret |= regmap_write(map, idx,  IOREG_IRQ);
+	ret |= regmap_read(map, data, &wdt.irq);
+
+	/* Lock up */
+	ret |= regmap_write(map, idx,  IOREG_LOCK);
+
+	mutex_unlock(&eiois200_dev->mutex);
+
+	return ret ? -EIO : 0;
+}
+
+static int wdt_get_irq_pmc(void)
+{
+	return PMC_READ(REG_IRQ_NUMBER, &wdt.irq);
+}
+
+static int wdt_get_irq(struct device *dev)
+{
+	int ret;
+
+	if ((wdt.support & BIT(EVENT_IRQ)) == 0)
+		return -ENODEV;
+
+	/* Get IRQ number through PMC */
+	ret = wdt_get_irq_pmc();
+	if (ret)
+		return dev_err_probe(dev, ret, "Error get irq by pmc\n");
+
+	if (wdt.irq)
+		return 0;
+
+	/* Get IRQ number from the watchdog device in EC */
+	ret = wdt_get_irq_io();
+	if (ret)
+		return dev_err_probe(dev, ret, "Error get irq by io\n");
+
+	if (wdt.irq == 0)
+		return dev_err_probe(dev, ret, "Error IRQ number = 0\n");
+
+	return ret;
+}
+
+static int wdt_set_irq_io(void)
+{
+	int ret  = 0;
+	int idx  = EIOIS200_PNP_INDEX;
+	int data = EIOIS200_PNP_DATA;
+	struct regmap *map = wdt.iomap;
+
+	mutex_lock(&eiois200_dev->mutex);
+
+	/* Unlock EC IO port */
+	ret |= regmap_write(map, idx,  IOREG_UNLOCK);
+	ret |= regmap_write(map, idx,  IOREG_UNLOCK);
+
+	/* Select logical device to PMC */
+	ret |= regmap_write(map, idx,  IOREG_LDN);
+	ret |= regmap_write(map, data, IOREG_LDN_PMCIO);
+
+	/* Enable WDT */
+	ret |= regmap_write(map, idx,  IOREG_WDT_STATUS);
+	ret |= regmap_write(map, data, FLAG_WDT_ENABLED);
+
+	/* Set IRQ number */
+	ret |= regmap_write(map, idx,  IOREG_IRQ);
+	ret |= regmap_write(map, data, wdt.irq);
+
+	/* Lock up */
+	ret |= regmap_write(map, idx,  IOREG_LOCK);
+
+	mutex_unlock(&eiois200_dev->mutex);
+
+	return ret ? -EIO : 0;
+}
+
+static int wdt_set_irq_pmc(void)
+{
+	return PMC_WRITE(REG_IRQ_NUMBER, &wdt.irq);
+}
+
+static int wdt_set_irq(struct device *dev)
+{
+	int ret;
+
+	if ((wdt.support & BIT(EVENT_IRQ)) == 0)
+		return -ENODEV;
+
+	/* Set IRQ number to the watchdog device in EC */
+	ret = wdt_set_irq_io();
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Error set irq by io\n");
+
+	/* Notice EC that watchdog IRQ changed */
+	ret = wdt_set_irq_pmc();
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Error set irq by pmc\n");
+
+	return ret;
+}
+
+/**
+ * wdt_get_irq_event - Check if IRQ been triggered
+ * Returns:	The current status read from the PMC,
+ *		or 0 if there was an error.
+ */
+static int wdt_get_irq_event(void)
+{
+	u8 status;
+
+	if (PMC_READ(REG_EVENT, &status))
+		return 0;
+
+	return status;
+}
+
+static irqreturn_t wdt_isr(int irq, void *arg)
+{
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t wdt_threaded_isr(int irq, void *arg)
+{
+	u8 status = wdt_get_irq_event() & FLAG_TRIGGER_IRQ;
+
+	if (!status)
+		return IRQ_NONE;
+
+	if (wddev.pretimeout) {
+		watchdog_notify_pretimeout(&wddev);
+	} else {
+		pr_crit("Watchdog Timer expired. Initiating system reboot\n");
+		emergency_restart();
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int query_irq(struct device *dev)
+{
+	int ret;
+
+	if (irq) {
+		wdt.irq = irq;
+	} else {
+		ret = wdt_get_irq(dev);
+		if (ret)
+			return ret;
+	}
+
+	dev_dbg(wdt.dev, "IRQ = %d\n", wdt.irq);
+
+	return wdt_set_irq(dev);
+}
+
 static int wdt_init(struct device *dev)
 {
 	int ret = 0;
@@ -374,6 +579,10 @@ static int wdt_init(struct device *dev)
 	ret = wdt_get_type();
 	if (ret)
 		return ret;
+
+	if (wdt.event_type == EVENT_IRQ)
+		ret = query_irq(dev);
+
 	return ret;
 }
 
@@ -402,11 +611,21 @@ static int wdt_probe(struct platform_device *pdev)
 	wdt.iomap = dev_get_regmap(dev->parent, NULL);
 	if (!wdt.iomap)
 		return dev_err_probe(dev, -ENOMEM, "Query parent regmap fail\n");
-	
+
 	/* Initialize EC watchdog */
 	if (wdt_init(dev))
 		return dev_err_probe(dev, -EIO, "wdt_init fail\n");
 
+	/* Request IRQ */
+	if (wdt.event_type == EVENT_IRQ)
+		ret = devm_request_threaded_irq(dev, wdt.irq, wdt_isr,
+						wdt_threaded_isr,
+						IRQF_SHARED, pdev->name, dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				    "IRQ %d request fail:%d. Disabled.\n",
+				    wdt.irq, ret);
+
 	/* Inform watchdog info */
 	wddev.ops = &wdt_ops;
 	ret = watchdog_init_timeout(&wddev, wddev.timeout, dev);
-- 
2.34.1

Re: [PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[Guenter Roeck]

On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote:
> From: Wenkai <advantech.susiteam@gmail.com>
> 
> This patch series aims to add support for the Advantech EIO-IS200
> Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200
> is a widely used embedded controller, and this series introduces a
> native driver for its watchdog timer functionality within the Linux
> ecosystem.
> 
I am not going to review this patch series. This is just one watchdog driver.
One patch is sufficient.

Guenter

Re: [PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[Wenkai]

Guenter Roeck 於 10/6/2023 11:02 AM 寫道:
> On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote:
>> From: Wenkai <advantech.susiteam@gmail.com>
>>
>> This patch series aims to add support for the Advantech EIO-IS200
>> Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200
>> is a widely used embedded controller, and this series introduces a
>> native driver for its watchdog timer functionality within the Linux
>> ecosystem.
>>
> I am not going to review this patch series. This is just ne watchdog driver.
> One patch is sufficient.
>
> Guenter
Hi Guenter,

Advantech's EIO-IS200 watchdog supports 5 output pins: RESET, Power
Button, SCI, IRQ, and GPIO. The most traditional scenario is that the
Pretimeout triggers IRQ, and the timeout triggers RESET.

However, unfortunately, for industrial usages, there are various use
cases, which require certain mechanisms and logic to manage which signal
is output when Pretimeout and timeout expire. I am concerned that
consolidating all these features into a single patch for upstream may
lead to confusion and make the source code less readable and
understandable.

Therefore, I have divided the implementation into 5 separate patches,
aiming to make the code more comprehensible and acceptable. If it's
acceptable to you, I am more than willing to provide a single patch as
per your preference.

I would also like to note that this watchdog driver is part of the
EIO-IS200 MFD (Multi-Function Device) driver family. It serves as one
of the child-drivers of the drivers/mfd/eiois200_core core driver. It's
important to mention that without the presence of
drivers/mfd/eiois200_core, this child-driver eiois200_wdt cannot be
compiled or used.

Should we wait until the core driver (drivers/mfd/eiois200_core) is
successfully incorporated before upstreaming the watchdog child-driver,
or would you prefer to proceed with these patches independently?

Best regards,
Wenkai

Re: [PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[Guenter Roeck]

On Fri, Oct 06, 2023 at 05:27:48PM +0800, Wenkai wrote:
> 
> 
> Guenter Roeck 於 10/6/2023 11:02 AM 寫道:
> > On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote:
> > > From: Wenkai <advantech.susiteam@gmail.com>
> > > 
> > > This patch series aims to add support for the Advantech EIO-IS200
> > > Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200
> > > is a widely used embedded controller, and this series introduces a
> > > native driver for its watchdog timer functionality within the Linux
> > > ecosystem.
> > > 
> > I am not going to review this patch series. This is just ne watchdog driver.
> > One patch is sufficient.
> > 
> > Guenter
> Hi Guenter,
> 
> Advantech's EIO-IS200 watchdog supports 5 output pins: RESET, Power
> Button, SCI, IRQ, and GPIO. The most traditional scenario is that the
> Pretimeout triggers IRQ, and the timeout triggers RESET.
> 
> However, unfortunately, for industrial usages, there are various use
> cases, which require certain mechanisms and logic to manage which signal
> is output when Pretimeout and timeout expire. I am concerned that
> consolidating all these features into a single patch for upstream may
> lead to confusion and make the source code less readable and
> understandable.
> 

The 1st patch in your series doesn't even compile. I don't call that
understandable.

Oh, it fails to compile because you include a non-existing file from
../mfd directly and because you select a non-existing configuration option
instead of depending on it.

None of those is even remotely acceptable. Are you seriously sending me
a series of patches that don't even build to review ?

> Therefore, I have divided the implementation into 5 separate patches,
> aiming to make the code more comprehensible and acceptable. If it's
> acceptable to you, I am more than willing to provide a single patch as
> per your preference.
> 
Frankly, your series is one more nail in the coffin. I am now seriously
considering to resign as co-maintainer of the watchdog subsystem.

Guenter

Re: [PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[Wenkai]

Guenter Roeck 於 10/6/2023 10:16 PM 寫道:
> On Fri, Oct 06, 2023 at 05:27:48PM +0800, Wenkai wrote:
>>
>> Guenter Roeck 於 10/6/2023 11:02 AM 寫道:
>>> On Thu, Oct 05, 2023 at 04:51:18PM +0800, advantech.susiteam@gmail.com wrote:
>>>> From: Wenkai <advantech.susiteam@gmail.com>
>>>>
>>>> This patch series aims to add support for the Advantech EIO-IS200
>>>> Embedded Controller's watchdog timer to the Linux kernel. The EIO-IS200
>>>> is a widely used embedded controller, and this series introduces a
>>>> native driver for its watchdog timer functionality within the Linux
>>>> ecosystem.
>>>>
>>> I am not going to review this patch series. This is just ne watchdog driver.
>>> One patch is sufficient.
>>>
>>> Guenter
>> Hi Guenter,
>>
>> Advantech's EIO-IS200 watchdog supports 5 output pins: RESET, Power
>> Button, SCI, IRQ, and GPIO. The most traditional scenario is that the
>> Pretimeout triggers IRQ, and the timeout triggers RESET.
>>
>> However, unfortunately, for industrial usages, there are various use
>> cases, which require certain mechanisms and logic to manage which signal
>> is output when Pretimeout and timeout expire. I am concerned that
>> consolidating all these features into a single patch for upstream may
>> lead to confusion and make the source code less readable and
>> understandable.
>>
> The 1st patch in your series doesn't even compile. I don't call that
> understandable.
>
> Oh, it fails to compile because you include a non-existing file from
> ../mfd directly and because you select a non-existing configuration option
> instead of depending on it.
>
> None of those is even remotely acceptable. Are you seriously sending me
> a series of patches that don't even build to review ?

I understand that the patches don't meet the expected quality standards.
The compile issue is due to my MFD core driver, which is currently under
review and has not been merged yet.

I would also like to seek your advice on how to best proceed with the
sub-drivers like the watchdog driver. Should I wait for my core MFD
driver to be successfully merged before submitting the sub-drivers, or
let Jones Lee review my core MFD driver and all its sub-drivers, or is
there another approach that you recommend?
>> Therefore, I have divided the implementation into 5 separate patches,
>> aiming to make the code more comprehensible and acceptable. If it's
>> acceptable to you, I am more than willing to provide a single patch as
>> per your preference.
>>
> Frankly, your series is one more nail in the coffin. I am now seriously
> considering to resign as co-maintainer of the watchdog subsystem.
>
> Guenter

I also appreciate your patience, dedication, and valuable contributions
to the Linux community. Your longstanding efforts and expertise are
commendable and have been instrumental in advancing the Linux ecosystem.
I understand that upstream review can be a meticulous and vital, albeit
thankless, task. I don't want my actions to cause any inconvenience or
distress, especially to someone as esteemed as you are in the Linux
community. Your insights and guidance are incredibly valuable to all of
us.

Once again, thank you for your understanding, and I am committed to
delivering high-quality code for your review.

Best regards,
Wenkai

Re: [PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[Guenter Roeck]

On Wed, Oct 11, 2023 at 12:08:57PM +0800, Wenkai wrote:
> 
> I understand that the patches don't meet the expected quality standards.
> The compile issue is due to my MFD core driver, which is currently under
> review and has not been merged yet.
> 
> I would also like to seek your advice on how to best proceed with the
> sub-drivers like the watchdog driver. Should I wait for my core MFD
> driver to be successfully merged before submitting the sub-drivers, or
> let Jones Lee review my core MFD driver and all its sub-drivers, or is
> there another approach that you recommend?

If the sub-drivers depend on the mfd driver, at least provide a reference
to the patch or patch series introducing that driver. Either case, a direct
include from "../mfd" is simply unacceptable. include/linux/mfd/ does exist
for a reason, after all.

I don't know the best solution for reviewing all the drivers. I didn't
(and do not plan to) look into the driver-driver API. If the interface
is regmap, reviewing sub-drivers on their own should be straightforward.
If the API is with function calls, things get more complicated because
the API needs the sub-drivers to be tested and everything needs to be
reviewed together.

Guenter

Re: [PATCH 0/5] watchdog: eiois200_wdt: Add EIO-IS200 Watchdog Driver

[Wenkai]

Guenter Roeck 於 10/11/2023 11:05 PM 寫道:
> On Wed, Oct 11, 2023 at 12:08:57PM +0800, Wenkai wrote:
>> I understand that the patches don't meet the expected quality standards.
>> The compile issue is due to my MFD core driver, which is currently under
>> review and has not been merged yet.
>>
>> I would also like to seek your advice on how to best proceed with the
>> sub-drivers like the watchdog driver. Should I wait for my core MFD
>> driver to be successfully merged before submitting the sub-drivers, or
>> let Jones Lee review my core MFD driver and all its sub-drivers, or is
>> there another approach that you recommend?
> If the sub-drivers depend on the mfd driver, at least provide a reference
> to the patch or patch series introducing that driver. Either case, a direct
> include from "../mfd" is simply unacceptable. include/linux/mfd/ does exist
> for a reason, after all.

The LKML link is https://lkml.org/lkml/2023/9/6/1245. Is this link to 
the patch
sufficient?

And, I'll move the "eiois200.h" to "include/linux/mfd/".

> I don't know the best solution for reviewing all the drivers. I didn't
> (and do not plan to) look into the driver-driver API. If the interface
> is regmap, reviewing sub-drivers on their own should be straightforward.
> If the API is with function calls, things get more complicated because
> the API needs the sub-drivers to be tested and everything needs to be
> reviewed together.
>
> Guenter

Unfortunately, all sub-drivers mostly communicate with the EC firmware
through the MFD core driver's driver-driver API, only a few uses regmap.
So should the MFD core driver and its sub-drivers be reviewed by the same
maintainers?

Best regards,
Wenkai