From: Wenkai <[email protected]>
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
From: Wenkai <[email protected]>
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 <[email protected]>
---
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
From: Wenkai <[email protected]>
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 <[email protected]>
---
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
From: Wenkai <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
*/
+#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
On Thu, Oct 05, 2023 at 04:51:18PM +0800, [email protected] wrote:
> From: Wenkai <[email protected]>
>
> 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
Guenter Roeck 於 10/6/2023 11:02 AM 寫道:
> On Thu, Oct 05, 2023 at 04:51:18PM +0800, [email protected] wrote:
>> From: Wenkai <[email protected]>
>>
>> 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
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, [email protected] wrote:
> > > From: Wenkai <[email protected]>
> > >
> > > 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
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, [email protected] wrote:
>>>> From: Wenkai <[email protected]>
>>>>
>>>> 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
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
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