[PATCH 1/3] watchdog: i6300esb: use the watchdog subsystem

[PATCH 1/3] watchdog: i6300esb: use the watchdog subsystem

[Radu Rendec]

Change the i6300esb driver to use the watchdog subsystem instead of the
legacy watchdog API. This is mainly just getting rid of the "write" and
"ioctl" methods and part of the watchdog control logic (which are all
implemented by the watchdog subsystem).

Even though the watchdog subsystem supports registering and handling
multiple watchdog devices at the same time, the i6300esb driver still
has a limitation of only one i6300esb device due to some global variable
usage that comes from the original design. However, the driver can now
coexist with other watchdog devices (supported by different drivers).

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 191 +++++++++-----------------------------------
 1 file changed, 39 insertions(+), 152 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index d7befd58b391..1df41a09553c 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -21,6 +21,8 @@
  *	Version 0.02
  *  20050210 David Härdeman <david@2gen.com>
  *	Ported driver to kernel 2.6
+ *  20171016 Radu Rendec <rrendec@arista.com>
+ *	Change driver to use the watchdog subsystem
  */
 
 /*
@@ -77,10 +79,7 @@
 /* internal variables */
 static void __iomem *BASEADDR;
 static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
-static unsigned long timer_alive;
 static struct pci_dev *esb_pci;
-static unsigned short triggered; /* The status of the watchdog upon boot */
-static char esb_expect_close;
 
 /* We can only use 1 card due to the /dev/watchdog restriction */
 static int cards_found;
@@ -116,21 +115,22 @@ static inline void esb_unlock_registers(void)
 	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
 }
 
-static int esb_timer_start(void)
+static int esb_timer_start(struct watchdog_device *wdd)
 {
+	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
 	u8 val;
 
 	spin_lock(&esb_lock);
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* Enable or Enable + Lock? */
-	val = ESB_WDT_ENABLE | (nowayout ? ESB_WDT_LOCK : 0x00);
+	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
 	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
 	spin_unlock(&esb_lock);
 	return 0;
 }
 
-static int esb_timer_stop(void)
+static int esb_timer_stop(struct watchdog_device *wdd)
 {
 	u8 val;
 
@@ -147,22 +147,21 @@ static int esb_timer_stop(void)
 	return val & ESB_WDT_ENABLE;
 }
 
-static void esb_timer_keepalive(void)
+static int esb_timer_keepalive(struct watchdog_device *wdd)
 {
 	spin_lock(&esb_lock);
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* FIXME: Do we need to flush anything here? */
 	spin_unlock(&esb_lock);
+	return 0;
 }
 
-static int esb_timer_set_heartbeat(int time)
+static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
+		unsigned int time)
 {
 	u32 val;
 
-	if (time < 0x1 || time > (2 * 0x03ff))
-		return -EINVAL;
-
 	spin_lock(&esb_lock);
 
 	/* We shift by 9, so if we are passed a value of 1 sec,
@@ -186,148 +185,33 @@ static int esb_timer_set_heartbeat(int time)
 	/* FIXME: Do we need to flush everything out? */
 
 	/* Done */
-	heartbeat = time;
 	spin_unlock(&esb_lock);
 	return 0;
 }
 
 /*
- *	/dev/watchdog handling
+ * Watchdog Subsystem Interfaces
  */
 
-static int esb_open(struct inode *inode, struct file *file)
-{
-	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &timer_alive))
-		return -EBUSY;
-
-	/* Reload and activate timer */
-	esb_timer_start();
-
-	return nonseekable_open(inode, file);
-}
-
-static int esb_release(struct inode *inode, struct file *file)
-{
-	/* Shut off the timer. */
-	if (esb_expect_close == 42)
-		esb_timer_stop();
-	else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		esb_timer_keepalive();
-	}
-	clear_bit(0, &timer_alive);
-	esb_expect_close = 0;
-	return 0;
-}
-
-static ssize_t esb_write(struct file *file, const char __user *data,
-			  size_t len, loff_t *ppos)
-{
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* note: just in case someone wrote the magic character
-			 * five months ago... */
-			esb_expect_close = 0;
-
-			/* scan to see whether or not we got the
-			 * magic character */
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					esb_expect_close = 42;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		esb_timer_keepalive();
-	}
-	return len;
-}
-
-static long esb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int new_options, retval = -EINVAL;
-	int new_heartbeat;
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	static const struct watchdog_info ident = {
-		.options =		WDIOF_SETTIMEOUT |
-					WDIOF_KEEPALIVEPING |
-					WDIOF_MAGICCLOSE,
-		.firmware_version =	0,
-		.identity =		ESB_MODULE_NAME,
-	};
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &ident,
-					sizeof(ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-		return put_user(0, p);
-
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(triggered, p);
-
-	case WDIOC_SETOPTIONS:
-	{
-		if (get_user(new_options, p))
-			return -EFAULT;
-
-		if (new_options & WDIOS_DISABLECARD) {
-			esb_timer_stop();
-			retval = 0;
-		}
-
-		if (new_options & WDIOS_ENABLECARD) {
-			esb_timer_start();
-			retval = 0;
-		}
-		return retval;
-	}
-	case WDIOC_KEEPALIVE:
-		esb_timer_keepalive();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-	{
-		if (get_user(new_heartbeat, p))
-			return -EFAULT;
-		if (esb_timer_set_heartbeat(new_heartbeat))
-			return -EINVAL;
-		esb_timer_keepalive();
-		/* Fall */
-	}
-	case WDIOC_GETTIMEOUT:
-		return put_user(heartbeat, p);
-	default:
-		return -ENOTTY;
-	}
-}
-
-/*
- *      Kernel Interfaces
- */
+static struct watchdog_info esb_info = {
+	.identity = ESB_MODULE_NAME,
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
 
-static const struct file_operations esb_fops = {
+static const struct watchdog_ops esb_ops = {
 	.owner = THIS_MODULE,
-	.llseek = no_llseek,
-	.write = esb_write,
-	.unlocked_ioctl = esb_ioctl,
-	.open = esb_open,
-	.release = esb_release,
+	.start = esb_timer_start,
+	.stop = esb_timer_stop,
+	.set_timeout = esb_timer_set_heartbeat,
+	.ping = esb_timer_keepalive,
 };
 
-static struct miscdevice esb_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &esb_fops,
+static struct watchdog_device esb_dev = {
+	.info = &esb_info,
+	.ops = &esb_ops,
+	.min_timeout = 1,
+	.max_timeout = 2 * 0x03ff,
+	.timeout = WATCHDOG_HEARTBEAT,
 };
 
 /*
@@ -405,14 +289,14 @@ static void esb_initdevice(void)
 	esb_unlock_registers();
 	val2 = readw(ESB_RELOAD_REG);
 	if (val2 & ESB_WDT_TIMEOUT)
-		triggered = WDIOF_CARDRESET;
+		esb_dev.bootstatus = WDIOF_CARDRESET;
 
 	/* Reset WDT_TIMEOUT flag and timers */
 	esb_unlock_registers();
 	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
 
 	/* And set the correct timeout value */
-	esb_timer_set_heartbeat(heartbeat);
+	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
 }
 
 static int esb_probe(struct pci_dev *pdev,
@@ -443,17 +327,18 @@ static int esb_probe(struct pci_dev *pdev,
 	}
 
 	/* Initialize the watchdog and make sure it does not run */
+	watchdog_init_timeout(&esb_dev, heartbeat, NULL);
+	watchdog_set_nowayout(&esb_dev, nowayout);
 	esb_initdevice();
 
 	/* Register the watchdog so that userspace has access to it */
-	ret = misc_register(&esb_miscdev);
+	ret = watchdog_register_device(&esb_dev);
 	if (ret != 0) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       WATCHDOG_MINOR, ret);
+		pr_err("cannot register watchdog device (err=%d)\n", ret);
 		goto err_unmap;
 	}
 	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
-		BASEADDR, heartbeat, nowayout);
+		BASEADDR, esb_dev.timeout, nowayout);
 	return 0;
 
 err_unmap:
@@ -466,12 +351,14 @@ static int esb_probe(struct pci_dev *pdev,
 
 static void esb_remove(struct pci_dev *pdev)
 {
+	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &esb_dev.status);
+
 	/* Stop the timer before we leave */
-	if (!nowayout)
-		esb_timer_stop();
+	if (!_wdd_nowayout)
+		esb_timer_stop(&esb_dev);
 
 	/* Deregister */
-	misc_deregister(&esb_miscdev);
+	watchdog_unregister_device(&esb_dev);
 	iounmap(BASEADDR);
 	pci_release_region(esb_pci, 0);
 	pci_disable_device(esb_pci);
@@ -480,7 +367,7 @@ static void esb_remove(struct pci_dev *pdev)
 
 static void esb_shutdown(struct pci_dev *pdev)
 {
-	esb_timer_stop();
+	esb_timer_stop(&esb_dev);
 }
 
 static struct pci_driver esb_driver = {
-- 
2.13.6

[PATCH 2/3] watchdog: i6300esb: support multiple devices

[Radu Rendec]

Support multiple i6300esb devices simultaneously, by removing the single
device restriction in the original design and leveraging the multiple
device support of the watchdog subsystem.

This patch replaces the global definitions of watchdog device data with
a dynamically allocated structure. This structure is allocated during
device probe, so multiple independent structures can be allocated if
multiple devices are probed.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 178 +++++++++++++++++++++++---------------------
 1 file changed, 94 insertions(+), 84 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 1df41a09553c..855b4f80ad09 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -23,6 +23,7 @@
  *	Ported driver to kernel 2.6
  *  20171016 Radu Rendec <rrendec@arista.com>
  *	Change driver to use the watchdog subsystem
+ *	Add support for multiple 6300ESB devices
  */
 
 /*
@@ -53,10 +54,10 @@
 #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
 
 /* Memory mapped registers */
-#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
-#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
-#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
-#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
+#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
+#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
+#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
+#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
 
 /* Lock register bits */
 #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
@@ -76,12 +77,7 @@
 #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
 #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
 
-/* internal variables */
-static void __iomem *BASEADDR;
-static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
-static struct pci_dev *esb_pci;
-
-/* We can only use 1 card due to the /dev/watchdog restriction */
+/* Probed devices counter; used only for printing the initial info message */
 static int cards_found;
 
 /* module parameters */
@@ -99,6 +95,16 @@ MODULE_PARM_DESC(nowayout,
 		"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+/* internal variables */
+struct esb_dev {
+	struct watchdog_device wdd;
+	void __iomem *base;
+	spinlock_t lock;
+	struct pci_dev *pdev;
+};
+
+#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
+
 /*
  * Some i6300ESB specific functions
  */
@@ -109,39 +115,41 @@ MODULE_PARM_DESC(nowayout,
  * reload register. After this the appropriate registers can be written
  * to once before they need to be unlocked again.
  */
-static inline void esb_unlock_registers(void)
+static inline void esb_unlock_registers(struct esb_dev *edev)
 {
-	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
-	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
+	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
+	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
 }
 
 static int esb_timer_start(struct watchdog_device *wdd)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
 	u8 val;
 
-	spin_lock(&esb_lock);
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	spin_lock(&edev->lock);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* Enable or Enable + Lock? */
 	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
-	spin_unlock(&esb_lock);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
+	spin_unlock(&edev->lock);
 	return 0;
 }
 
 static int esb_timer_stop(struct watchdog_device *wdd)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	u8 val;
 
-	spin_lock(&esb_lock);
+	spin_lock(&edev->lock);
 	/* First, reset timers as suggested by the docs */
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* Then disable the WDT */
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
-	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
-	spin_unlock(&esb_lock);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
+	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
+	spin_unlock(&edev->lock);
 
 	/* Returns 0 if the timer was disabled, non-zero otherwise */
 	return val & ESB_WDT_ENABLE;
@@ -149,20 +157,23 @@ static int esb_timer_stop(struct watchdog_device *wdd)
 
 static int esb_timer_keepalive(struct watchdog_device *wdd)
 {
-	spin_lock(&esb_lock);
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	struct esb_dev *edev = to_esb_dev(wdd);
+
+	spin_lock(&edev->lock);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* FIXME: Do we need to flush anything here? */
-	spin_unlock(&esb_lock);
+	spin_unlock(&edev->lock);
 	return 0;
 }
 
 static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 		unsigned int time)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	u32 val;
 
-	spin_lock(&esb_lock);
+	spin_lock(&edev->lock);
 
 	/* We shift by 9, so if we are passed a value of 1 sec,
 	 * val will be 1 << 9 = 512, then write that to two
@@ -171,21 +182,21 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 	val = time << 9;
 
 	/* Write timer 1 */
-	esb_unlock_registers();
-	writel(val, ESB_TIMER1_REG);
+	esb_unlock_registers(edev);
+	writel(val, ESB_TIMER1_REG(edev));
 
 	/* Write timer 2 */
-	esb_unlock_registers();
-	writel(val, ESB_TIMER2_REG);
+	esb_unlock_registers(edev);
+	writel(val, ESB_TIMER2_REG(edev));
 
 	/* Reload */
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 
 	/* FIXME: Do we need to flush everything out? */
 
 	/* Done */
-	spin_unlock(&esb_lock);
+	spin_unlock(&edev->lock);
 	return 0;
 }
 
@@ -206,14 +217,6 @@ static const struct watchdog_ops esb_ops = {
 	.ping = esb_timer_keepalive,
 };
 
-static struct watchdog_device esb_dev = {
-	.info = &esb_info,
-	.ops = &esb_ops,
-	.min_timeout = 1,
-	.max_timeout = 2 * 0x03ff,
-	.timeout = WATCHDOG_HEARTBEAT,
-};
-
 /*
  * Data for PCI driver interface
  */
@@ -227,38 +230,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
  *      Init & exit routines
  */
 
-static unsigned char esb_getdevice(struct pci_dev *pdev)
+static unsigned char esb_getdevice(struct esb_dev *edev)
 {
-	if (pci_enable_device(pdev)) {
+	if (pci_enable_device(edev->pdev)) {
 		pr_err("failed to enable device\n");
 		goto err_devput;
 	}
 
-	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
+	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
 		pr_err("failed to request region\n");
 		goto err_disable;
 	}
 
-	BASEADDR = pci_ioremap_bar(pdev, 0);
-	if (BASEADDR == NULL) {
+	edev->base = pci_ioremap_bar(edev->pdev, 0);
+	if (edev->base == NULL) {
 		/* Something's wrong here, BASEADDR has to be set */
 		pr_err("failed to get BASEADDR\n");
 		goto err_release;
 	}
 
 	/* Done */
-	esb_pci = pdev;
+	dev_set_drvdata(&edev->pdev->dev, edev);
 	return 1;
 
 err_release:
-	pci_release_region(pdev, 0);
+	pci_release_region(edev->pdev, 0);
 err_disable:
-	pci_disable_device(pdev);
+	pci_disable_device(edev->pdev);
 err_devput:
 	return 0;
 }
 
-static void esb_initdevice(void)
+static void esb_initdevice(struct esb_dev *edev)
 {
 	u8 val1;
 	u16 val2;
@@ -275,33 +278,34 @@ static void esb_initdevice(void)
 	 * any interrupts as there is not much we can do with it
 	 * right now.
 	 */
-	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
+	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
 
 	/* Check that the WDT isn't already locked */
-	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
+	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
 	if (val1 & ESB_WDT_LOCK)
 		pr_warn("nowayout already set\n");
 
 	/* Set the timer to watchdog mode and disable it for now */
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
 
 	/* Check if the watchdog was previously triggered */
-	esb_unlock_registers();
-	val2 = readw(ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	val2 = readw(ESB_RELOAD_REG(edev));
 	if (val2 & ESB_WDT_TIMEOUT)
-		esb_dev.bootstatus = WDIOF_CARDRESET;
+		edev->wdd.bootstatus = WDIOF_CARDRESET;
 
 	/* Reset WDT_TIMEOUT flag and timers */
-	esb_unlock_registers();
-	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
 
 	/* And set the correct timeout value */
-	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
+	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
 }
 
 static int esb_probe(struct pci_dev *pdev,
 		const struct pci_device_id *ent)
 {
+	struct esb_dev *edev;
 	int ret;
 
 	cards_found++;
@@ -309,13 +313,14 @@ static int esb_probe(struct pci_dev *pdev,
 		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
 			ESB_VERSION);
 
-	if (cards_found > 1) {
-		pr_err("This driver only supports 1 device\n");
-		return -ENODEV;
-	}
+	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
+	if (!edev)
+		return -ENOMEM;
 
 	/* Check whether or not the hardware watchdog is there */
-	if (!esb_getdevice(pdev) || esb_pci == NULL)
+	spin_lock_init(&edev->lock);
+	edev->pdev = pdev;
+	if (!esb_getdevice(edev))
 		return -ENODEV;
 
 	/* Check that the heartbeat value is within it's range;
@@ -327,47 +332,52 @@ static int esb_probe(struct pci_dev *pdev,
 	}
 
 	/* Initialize the watchdog and make sure it does not run */
-	watchdog_init_timeout(&esb_dev, heartbeat, NULL);
-	watchdog_set_nowayout(&esb_dev, nowayout);
-	esb_initdevice();
+	edev->wdd.info = &esb_info;
+	edev->wdd.ops = &esb_ops;
+	edev->wdd.min_timeout = 1;
+	edev->wdd.max_timeout = 2 * 0x03ff;
+	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
+	watchdog_set_nowayout(&edev->wdd, nowayout);
+	esb_initdevice(edev);
 
 	/* Register the watchdog so that userspace has access to it */
-	ret = watchdog_register_device(&esb_dev);
+	ret = watchdog_register_device(&edev->wdd);
 	if (ret != 0) {
 		pr_err("cannot register watchdog device (err=%d)\n", ret);
 		goto err_unmap;
 	}
 	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
-		BASEADDR, esb_dev.timeout, nowayout);
+		edev->base, edev->wdd.timeout, nowayout);
 	return 0;
 
 err_unmap:
-	iounmap(BASEADDR);
-	pci_release_region(esb_pci, 0);
-	pci_disable_device(esb_pci);
-	esb_pci = NULL;
+	iounmap(edev->base);
+	pci_release_region(edev->pdev, 0);
+	pci_disable_device(edev->pdev);
 	return ret;
 }
 
 static void esb_remove(struct pci_dev *pdev)
 {
-	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &esb_dev.status);
+	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
+	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &edev->wdd.status);
 
 	/* Stop the timer before we leave */
 	if (!_wdd_nowayout)
-		esb_timer_stop(&esb_dev);
+		esb_timer_stop(&edev->wdd);
 
 	/* Deregister */
-	watchdog_unregister_device(&esb_dev);
-	iounmap(BASEADDR);
-	pci_release_region(esb_pci, 0);
-	pci_disable_device(esb_pci);
-	esb_pci = NULL;
+	watchdog_unregister_device(&edev->wdd);
+	iounmap(edev->base);
+	pci_release_region(edev->pdev, 0);
+	pci_disable_device(edev->pdev);
 }
 
 static void esb_shutdown(struct pci_dev *pdev)
 {
-	esb_timer_stop(&esb_dev);
+	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
+
+	esb_timer_stop(&edev->wdd);
 }
 
 static struct pci_driver esb_driver = {
-- 
2.13.6

[PATCH 3/3] watchdog: i6300esb: do not hardcode heartbeat limits

[Radu Rendec]

The minimum, maximum and default values for the watchdog heartbeat
(timeout) were hardcoded in several places (including module parameter
description and warning message for invalid module parameter value).

This patch adds macros for the aforementioned values and replaces all
occurences of hardcoded values by these macros.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 855b4f80ad09..68d76512b08f 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -82,12 +82,16 @@ static int cards_found;
 
 /* module parameters */
 /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
-#define WATCHDOG_HEARTBEAT 30
-static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
+#define ESB_HEARTBEAT_MIN 1
+#define ESB_HEARTBEAT_MAX 2046
+#define ESB_HEARTBEAT_DEFAULT 30
+static int heartbeat = ESB_HEARTBEAT_DEFAULT;  /* in seconds */
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat,
-		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
-				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
+	"Watchdog heartbeat in seconds. ("
+	__MODULE_STRING(ESB_HEARTBEAT_MIN) "<heartbeat<"
+	__MODULE_STRING(ESB_HEARTBEAT_MAX) ", default="
+	__MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
@@ -325,17 +329,19 @@ static int esb_probe(struct pci_dev *pdev,
 
 	/* Check that the heartbeat value is within it's range;
 	   if not reset to the default */
-	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
-		heartbeat = WATCHDOG_HEARTBEAT;
-		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
+	if (heartbeat < ESB_HEARTBEAT_MIN || heartbeat > ESB_HEARTBEAT_MAX) {
+		heartbeat = ESB_HEARTBEAT_DEFAULT;
+		pr_info("heartbeat value must be "
+			__MODULE_STRING(ESB_HEARTBEAT_MIN) "<heartbeat<"
+			__MODULE_STRING(ESB_HEARTBEAT_MAX) ", using %d\n",
 			heartbeat);
 	}
 
 	/* Initialize the watchdog and make sure it does not run */
 	edev->wdd.info = &esb_info;
 	edev->wdd.ops = &esb_ops;
-	edev->wdd.min_timeout = 1;
-	edev->wdd.max_timeout = 2 * 0x03ff;
+	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
+	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
 	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
 	watchdog_set_nowayout(&edev->wdd, nowayout);
 	esb_initdevice(edev);
-- 
2.13.6

Re: [1/3] watchdog: i6300esb: use the watchdog subsystem

[Guenter Roeck]

On Mon, Oct 16, 2017 at 07:25:26PM +0100, Radu Rendec wrote:
> Change the i6300esb driver to use the watchdog subsystem instead of the
> legacy watchdog API. This is mainly just getting rid of the "write" and
> "ioctl" methods and part of the watchdog control logic (which are all
> implemented by the watchdog subsystem).
> 
> Even though the watchdog subsystem supports registering and handling
> multiple watchdog devices at the same time, the i6300esb driver still
> has a limitation of only one i6300esb device due to some global variable
> usage that comes from the original design. However, the driver can now
> coexist with other watchdog devices (supported by different drivers).
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 191 +++++++++-----------------------------------
>  1 file changed, 39 insertions(+), 152 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index d7befd58b391..1df41a09553c 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -21,6 +21,8 @@
>   *	Version 0.02
>   *  20050210 David Härdeman <david@2gen.com>
>   *	Ported driver to kernel 2.6
> + *  20171016 Radu Rendec <rrendec@arista.com>
> + *	Change driver to use the watchdog subsystem
>   */
>  
>  /*
> @@ -77,10 +79,7 @@
>  /* internal variables */
>  static void __iomem *BASEADDR;
>  static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
> -static unsigned long timer_alive;
>  static struct pci_dev *esb_pci;
> -static unsigned short triggered; /* The status of the watchdog upon boot */
> -static char esb_expect_close;
>  
>  /* We can only use 1 card due to the /dev/watchdog restriction */
>  static int cards_found;
> @@ -116,21 +115,22 @@ static inline void esb_unlock_registers(void)
>  	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
>  }
>  
> -static int esb_timer_start(void)
> +static int esb_timer_start(struct watchdog_device *wdd)
>  {
> +	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>  	u8 val;
>  
>  	spin_lock(&esb_lock);
>  	esb_unlock_registers();
>  	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>  	/* Enable or Enable + Lock? */
> -	val = ESB_WDT_ENABLE | (nowayout ? ESB_WDT_LOCK : 0x00);
> +	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
>  	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
>  	spin_unlock(&esb_lock);
>  	return 0;
>  }
>  
> -static int esb_timer_stop(void)
> +static int esb_timer_stop(struct watchdog_device *wdd)
>  {
>  	u8 val;
>  
> @@ -147,22 +147,21 @@ static int esb_timer_stop(void)
>  	return val & ESB_WDT_ENABLE;
>  }
>  
> -static void esb_timer_keepalive(void)
> +static int esb_timer_keepalive(struct watchdog_device *wdd)
>  {
>  	spin_lock(&esb_lock);
>  	esb_unlock_registers();
>  	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>  	/* FIXME: Do we need to flush anything here? */
>  	spin_unlock(&esb_lock);
> +	return 0;
>  }
>  
> -static int esb_timer_set_heartbeat(int time)
> +static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
> +		unsigned int time)
>  {
>  	u32 val;
>  
> -	if (time < 0x1 || time > (2 * 0x03ff))
> -		return -EINVAL;
> -
>  	spin_lock(&esb_lock);
>  
>  	/* We shift by 9, so if we are passed a value of 1 sec,
> @@ -186,148 +185,33 @@ static int esb_timer_set_heartbeat(int time)
>  	/* FIXME: Do we need to flush everything out? */
>  
>  	/* Done */
> -	heartbeat = time;
>  	spin_unlock(&esb_lock);

Needs to set wdt->timeout.

>  	return 0;
>  }
>  
>  /*
> - *	/dev/watchdog handling
> + * Watchdog Subsystem Interfaces
>   */
>  
> -static int esb_open(struct inode *inode, struct file *file)
> -{
> -	/* /dev/watchdog can only be opened once */
> -	if (test_and_set_bit(0, &timer_alive))
> -		return -EBUSY;
> -
> -	/* Reload and activate timer */
> -	esb_timer_start();
> -
> -	return nonseekable_open(inode, file);
> -}
> -
> -static int esb_release(struct inode *inode, struct file *file)
> -{
> -	/* Shut off the timer. */
> -	if (esb_expect_close == 42)
> -		esb_timer_stop();
> -	else {
> -		pr_crit("Unexpected close, not stopping watchdog!\n");
> -		esb_timer_keepalive();
> -	}
> -	clear_bit(0, &timer_alive);
> -	esb_expect_close = 0;
> -	return 0;
> -}
> -
> -static ssize_t esb_write(struct file *file, const char __user *data,
> -			  size_t len, loff_t *ppos)
> -{
> -	/* See if we got the magic character 'V' and reload the timer */
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			/* note: just in case someone wrote the magic character
> -			 * five months ago... */
> -			esb_expect_close = 0;
> -
> -			/* scan to see whether or not we got the
> -			 * magic character */
> -			for (i = 0; i != len; i++) {
> -				char c;
> -				if (get_user(c, data + i))
> -					return -EFAULT;
> -				if (c == 'V')
> -					esb_expect_close = 42;
> -			}
> -		}
> -
> -		/* someone wrote to us, we should reload the timer */
> -		esb_timer_keepalive();
> -	}
> -	return len;
> -}
> -
> -static long esb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> -{
> -	int new_options, retval = -EINVAL;
> -	int new_heartbeat;
> -	void __user *argp = (void __user *)arg;
> -	int __user *p = argp;
> -	static const struct watchdog_info ident = {
> -		.options =		WDIOF_SETTIMEOUT |
> -					WDIOF_KEEPALIVEPING |
> -					WDIOF_MAGICCLOSE,
> -		.firmware_version =	0,
> -		.identity =		ESB_MODULE_NAME,
> -	};
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		return copy_to_user(argp, &ident,
> -					sizeof(ident)) ? -EFAULT : 0;
> -
> -	case WDIOC_GETSTATUS:
> -		return put_user(0, p);
> -
> -	case WDIOC_GETBOOTSTATUS:
> -		return put_user(triggered, p);
> -
> -	case WDIOC_SETOPTIONS:
> -	{
> -		if (get_user(new_options, p))
> -			return -EFAULT;
> -
> -		if (new_options & WDIOS_DISABLECARD) {
> -			esb_timer_stop();
> -			retval = 0;
> -		}
> -
> -		if (new_options & WDIOS_ENABLECARD) {
> -			esb_timer_start();
> -			retval = 0;
> -		}
> -		return retval;
> -	}
> -	case WDIOC_KEEPALIVE:
> -		esb_timer_keepalive();
> -		return 0;
> -
> -	case WDIOC_SETTIMEOUT:
> -	{
> -		if (get_user(new_heartbeat, p))
> -			return -EFAULT;
> -		if (esb_timer_set_heartbeat(new_heartbeat))
> -			return -EINVAL;
> -		esb_timer_keepalive();
> -		/* Fall */
> -	}
> -	case WDIOC_GETTIMEOUT:
> -		return put_user(heartbeat, p);
> -	default:
> -		return -ENOTTY;
> -	}
> -}
> -
> -/*
> - *      Kernel Interfaces
> - */
> +static struct watchdog_info esb_info = {
> +	.identity = ESB_MODULE_NAME,
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +};
>  
> -static const struct file_operations esb_fops = {
> +static const struct watchdog_ops esb_ops = {
>  	.owner = THIS_MODULE,
> -	.llseek = no_llseek,
> -	.write = esb_write,
> -	.unlocked_ioctl = esb_ioctl,
> -	.open = esb_open,
> -	.release = esb_release,
> +	.start = esb_timer_start,
> +	.stop = esb_timer_stop,
> +	.set_timeout = esb_timer_set_heartbeat,
> +	.ping = esb_timer_keepalive,
>  };
>  
> -static struct miscdevice esb_miscdev = {
> -	.minor = WATCHDOG_MINOR,
> -	.name = "watchdog",
> -	.fops = &esb_fops,
> +static struct watchdog_device esb_dev = {
> +	.info = &esb_info,
> +	.ops = &esb_ops,
> +	.min_timeout = 1,
> +	.max_timeout = 2 * 0x03ff,
> +	.timeout = WATCHDOG_HEARTBEAT,
>  };
>  
>  /*
> @@ -405,14 +289,14 @@ static void esb_initdevice(void)
>  	esb_unlock_registers();
>  	val2 = readw(ESB_RELOAD_REG);
>  	if (val2 & ESB_WDT_TIMEOUT)
> -		triggered = WDIOF_CARDRESET;
> +		esb_dev.bootstatus = WDIOF_CARDRESET;
>  
>  	/* Reset WDT_TIMEOUT flag and timers */
>  	esb_unlock_registers();
>  	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
>  
>  	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(heartbeat);
> +	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
>  }
>  
>  static int esb_probe(struct pci_dev *pdev,
> @@ -443,17 +327,18 @@ static int esb_probe(struct pci_dev *pdev,
>  	}
>  
>  	/* Initialize the watchdog and make sure it does not run */
> +	watchdog_init_timeout(&esb_dev, heartbeat, NULL);
> +	watchdog_set_nowayout(&esb_dev, nowayout);
>  	esb_initdevice();
>  
>  	/* Register the watchdog so that userspace has access to it */
> -	ret = misc_register(&esb_miscdev);
> +	ret = watchdog_register_device(&esb_dev);
>  	if (ret != 0) {
> -		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> -		       WATCHDOG_MINOR, ret);
> +		pr_err("cannot register watchdog device (err=%d)\n", ret);

dev_err()

>  		goto err_unmap;
>  	}
>  	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",

dev_info()

> -		BASEADDR, heartbeat, nowayout);
> +		BASEADDR, esb_dev.timeout, nowayout);
>  	return 0;
>  
>  err_unmap:
> @@ -466,12 +351,14 @@ static int esb_probe(struct pci_dev *pdev,
>  
>  static void esb_remove(struct pci_dev *pdev)
>  {
> +	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &esb_dev.status);
> +
>  	/* Stop the timer before we leave */
> -	if (!nowayout)
> -		esb_timer_stop();
> +	if (!_wdd_nowayout)
> +		esb_timer_stop(&esb_dev);

Can you call watchdog_stop_on_unregister() in probe instead ?

>  
>  	/* Deregister */
> -	misc_deregister(&esb_miscdev);
> +	watchdog_unregister_device(&esb_dev);
>  	iounmap(BASEADDR);
>  	pci_release_region(esb_pci, 0);
>  	pci_disable_device(esb_pci);
> @@ -480,7 +367,7 @@ static void esb_remove(struct pci_dev *pdev)
>  
>  static void esb_shutdown(struct pci_dev *pdev)
>  {
> -	esb_timer_stop();
> +	esb_timer_stop(&esb_dev);

Call watchdog_stop_on_reboot() in probe ?

>  }
>  
>  static struct pci_driver esb_driver = {

Re: [2/3] watchdog: i6300esb: support multiple devices

[Guenter Roeck]

On Mon, Oct 16, 2017 at 07:25:27PM +0100, Radu Rendec wrote:
> Support multiple i6300esb devices simultaneously, by removing the single
> device restriction in the original design and leveraging the multiple
> device support of the watchdog subsystem.
> 
> This patch replaces the global definitions of watchdog device data with
> a dynamically allocated structure. This structure is allocated during
> device probe, so multiple independent structures can be allocated if
> multiple devices are probed.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

Looks good, except it will need a rebase to match changes I asked for
in patch 1.

Guenter

> ---
>  drivers/watchdog/i6300esb.c | 178 +++++++++++++++++++++++---------------------
>  1 file changed, 94 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 1df41a09553c..855b4f80ad09 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -23,6 +23,7 @@
>   *	Ported driver to kernel 2.6
>   *  20171016 Radu Rendec <rrendec@arista.com>
>   *	Change driver to use the watchdog subsystem
> + *	Add support for multiple 6300ESB devices
>   */
>  
>  /*
> @@ -53,10 +54,10 @@
>  #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
>  
>  /* Memory mapped registers */
> -#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
> -#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
> -#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
> -#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
> +#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
> +#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
> +#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
> +#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
>  
>  /* Lock register bits */
>  #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
> @@ -76,12 +77,7 @@
>  #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
>  #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
>  
> -/* internal variables */
> -static void __iomem *BASEADDR;
> -static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
> -static struct pci_dev *esb_pci;
> -
> -/* We can only use 1 card due to the /dev/watchdog restriction */
> +/* Probed devices counter; used only for printing the initial info message */
>  static int cards_found;
>  
>  /* module parameters */
> @@ -99,6 +95,16 @@ MODULE_PARM_DESC(nowayout,
>  		"Watchdog cannot be stopped once started (default="
>  				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +/* internal variables */
> +struct esb_dev {
> +	struct watchdog_device wdd;
> +	void __iomem *base;
> +	spinlock_t lock;
> +	struct pci_dev *pdev;
> +};
> +
> +#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
> +
>  /*
>   * Some i6300ESB specific functions
>   */
> @@ -109,39 +115,41 @@ MODULE_PARM_DESC(nowayout,
>   * reload register. After this the appropriate registers can be written
>   * to once before they need to be unlocked again.
>   */
> -static inline void esb_unlock_registers(void)
> +static inline void esb_unlock_registers(struct esb_dev *edev)
>  {
> -	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
> -	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
> +	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
> +	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
>  }
>  
>  static int esb_timer_start(struct watchdog_device *wdd)
>  {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>  	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>  	u8 val;
>  
> -	spin_lock(&esb_lock);
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	spin_lock(&edev->lock);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  	/* Enable or Enable + Lock? */
>  	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
> -	spin_unlock(&esb_lock);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
> +	spin_unlock(&edev->lock);
>  	return 0;
>  }
>  
>  static int esb_timer_stop(struct watchdog_device *wdd)
>  {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>  	u8 val;
>  
> -	spin_lock(&esb_lock);
> +	spin_lock(&edev->lock);
>  	/* First, reset timers as suggested by the docs */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  	/* Then disable the WDT */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
> -	spin_unlock(&esb_lock);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
> +	spin_unlock(&edev->lock);
>  
>  	/* Returns 0 if the timer was disabled, non-zero otherwise */
>  	return val & ESB_WDT_ENABLE;
> @@ -149,20 +157,23 @@ static int esb_timer_stop(struct watchdog_device *wdd)
>  
>  static int esb_timer_keepalive(struct watchdog_device *wdd)
>  {
> -	spin_lock(&esb_lock);
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	struct esb_dev *edev = to_esb_dev(wdd);
> +
> +	spin_lock(&edev->lock);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  	/* FIXME: Do we need to flush anything here? */
> -	spin_unlock(&esb_lock);
> +	spin_unlock(&edev->lock);
>  	return 0;
>  }
>  
>  static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>  		unsigned int time)
>  {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>  	u32 val;
>  
> -	spin_lock(&esb_lock);
> +	spin_lock(&edev->lock);
>  
>  	/* We shift by 9, so if we are passed a value of 1 sec,
>  	 * val will be 1 << 9 = 512, then write that to two
> @@ -171,21 +182,21 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>  	val = time << 9;
>  
>  	/* Write timer 1 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER1_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER1_REG(edev));
>  
>  	/* Write timer 2 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER2_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER2_REG(edev));
>  
>  	/* Reload */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  
>  	/* FIXME: Do we need to flush everything out? */
>  
>  	/* Done */
> -	spin_unlock(&esb_lock);
> +	spin_unlock(&edev->lock);
>  	return 0;
>  }
>  
> @@ -206,14 +217,6 @@ static const struct watchdog_ops esb_ops = {
>  	.ping = esb_timer_keepalive,
>  };
>  
> -static struct watchdog_device esb_dev = {
> -	.info = &esb_info,
> -	.ops = &esb_ops,
> -	.min_timeout = 1,
> -	.max_timeout = 2 * 0x03ff,
> -	.timeout = WATCHDOG_HEARTBEAT,
> -};
> -
>  /*
>   * Data for PCI driver interface
>   */
> @@ -227,38 +230,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
>   *      Init & exit routines
>   */
>  
> -static unsigned char esb_getdevice(struct pci_dev *pdev)
> +static unsigned char esb_getdevice(struct esb_dev *edev)
>  {
> -	if (pci_enable_device(pdev)) {
> +	if (pci_enable_device(edev->pdev)) {
>  		pr_err("failed to enable device\n");
>  		goto err_devput;
>  	}
>  
> -	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> +	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
>  		pr_err("failed to request region\n");
>  		goto err_disable;
>  	}
>  
> -	BASEADDR = pci_ioremap_bar(pdev, 0);
> -	if (BASEADDR == NULL) {
> +	edev->base = pci_ioremap_bar(edev->pdev, 0);
> +	if (edev->base == NULL) {
>  		/* Something's wrong here, BASEADDR has to be set */
>  		pr_err("failed to get BASEADDR\n");
>  		goto err_release;
>  	}
>  
>  	/* Done */
> -	esb_pci = pdev;
> +	dev_set_drvdata(&edev->pdev->dev, edev);
>  	return 1;
>  
>  err_release:
> -	pci_release_region(pdev, 0);
> +	pci_release_region(edev->pdev, 0);
>  err_disable:
> -	pci_disable_device(pdev);
> +	pci_disable_device(edev->pdev);
>  err_devput:
>  	return 0;
>  }
>  
> -static void esb_initdevice(void)
> +static void esb_initdevice(struct esb_dev *edev)
>  {
>  	u8 val1;
>  	u16 val2;
> @@ -275,33 +278,34 @@ static void esb_initdevice(void)
>  	 * any interrupts as there is not much we can do with it
>  	 * right now.
>  	 */
> -	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
> +	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
>  
>  	/* Check that the WDT isn't already locked */
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
>  	if (val1 & ESB_WDT_LOCK)
>  		pr_warn("nowayout already set\n");
>  
>  	/* Set the timer to watchdog mode and disable it for now */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
>  
>  	/* Check if the watchdog was previously triggered */
> -	esb_unlock_registers();
> -	val2 = readw(ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	val2 = readw(ESB_RELOAD_REG(edev));
>  	if (val2 & ESB_WDT_TIMEOUT)
> -		esb_dev.bootstatus = WDIOF_CARDRESET;
> +		edev->wdd.bootstatus = WDIOF_CARDRESET;
>  
>  	/* Reset WDT_TIMEOUT flag and timers */
> -	esb_unlock_registers();
> -	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
>  
>  	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
> +	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
>  }
>  
>  static int esb_probe(struct pci_dev *pdev,
>  		const struct pci_device_id *ent)
>  {
> +	struct esb_dev *edev;
>  	int ret;
>  
>  	cards_found++;
> @@ -309,13 +313,14 @@ static int esb_probe(struct pci_dev *pdev,
>  		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
>  			ESB_VERSION);
>  
> -	if (cards_found > 1) {
> -		pr_err("This driver only supports 1 device\n");
> -		return -ENODEV;
> -	}
> +	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
> +	if (!edev)
> +		return -ENOMEM;
>  
>  	/* Check whether or not the hardware watchdog is there */
> -	if (!esb_getdevice(pdev) || esb_pci == NULL)
> +	spin_lock_init(&edev->lock);
> +	edev->pdev = pdev;
> +	if (!esb_getdevice(edev))
>  		return -ENODEV;
>  
>  	/* Check that the heartbeat value is within it's range;
> @@ -327,47 +332,52 @@ static int esb_probe(struct pci_dev *pdev,
>  	}
>  
>  	/* Initialize the watchdog and make sure it does not run */
> -	watchdog_init_timeout(&esb_dev, heartbeat, NULL);
> -	watchdog_set_nowayout(&esb_dev, nowayout);
> -	esb_initdevice();
> +	edev->wdd.info = &esb_info;
> +	edev->wdd.ops = &esb_ops;
> +	edev->wdd.min_timeout = 1;
> +	edev->wdd.max_timeout = 2 * 0x03ff;
> +	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
> +	watchdog_set_nowayout(&edev->wdd, nowayout);
> +	esb_initdevice(edev);
>  
>  	/* Register the watchdog so that userspace has access to it */
> -	ret = watchdog_register_device(&esb_dev);
> +	ret = watchdog_register_device(&edev->wdd);
>  	if (ret != 0) {
>  		pr_err("cannot register watchdog device (err=%d)\n", ret);
>  		goto err_unmap;
>  	}
>  	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> -		BASEADDR, esb_dev.timeout, nowayout);
> +		edev->base, edev->wdd.timeout, nowayout);
>  	return 0;
>  
>  err_unmap:
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>  	return ret;
>  }
>  
>  static void esb_remove(struct pci_dev *pdev)
>  {
> -	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &esb_dev.status);
> +	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> +	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &edev->wdd.status);
>  
>  	/* Stop the timer before we leave */
>  	if (!_wdd_nowayout)
> -		esb_timer_stop(&esb_dev);
> +		esb_timer_stop(&edev->wdd);
>  
>  	/* Deregister */
> -	watchdog_unregister_device(&esb_dev);
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	watchdog_unregister_device(&edev->wdd);
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>  }
>  
>  static void esb_shutdown(struct pci_dev *pdev)
>  {
> -	esb_timer_stop(&esb_dev);
> +	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> +
> +	esb_timer_stop(&edev->wdd);
>  }
>  
>  static struct pci_driver esb_driver = {

Re: [3/3] watchdog: i6300esb: do not hardcode heartbeat limits

[Guenter Roeck]

On Mon, Oct 16, 2017 at 07:25:28PM +0100, Radu Rendec wrote:
> The minimum, maximum and default values for the watchdog heartbeat
> (timeout) were hardcoded in several places (including module parameter
> description and warning message for invalid module parameter value).
> 
> This patch adds macros for the aforementioned values and replaces all
> occurences of hardcoded values by these macros.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 855b4f80ad09..68d76512b08f 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -82,12 +82,16 @@ static int cards_found;
>  
>  /* module parameters */
>  /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
> -#define WATCHDOG_HEARTBEAT 30
> -static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
> +#define ESB_HEARTBEAT_MIN 1
> +#define ESB_HEARTBEAT_MAX 2046
> +#define ESB_HEARTBEAT_DEFAULT 30
> +static int heartbeat = ESB_HEARTBEAT_DEFAULT;  /* in seconds */

Ah, I just noticed. This defeats watchdog_init_timeout().

In patch 1, the timeout in struct watchdog_device to the default,
and set this variable to 0. watchdog_init_timeout() will then overwrite
the default (which is set in struct watchdog_device) with the module
parameter if the module parameter is set to a value != 0.

Thanks,
Guenter

>  module_param(heartbeat, int, 0);
>  MODULE_PARM_DESC(heartbeat,
> -		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
> -				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
> +	"Watchdog heartbeat in seconds. ("
> +	__MODULE_STRING(ESB_HEARTBEAT_MIN) "<heartbeat<"
> +	__MODULE_STRING(ESB_HEARTBEAT_MAX) ", default="
> +	__MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0);
> @@ -325,17 +329,19 @@ static int esb_probe(struct pci_dev *pdev,
>  
>  	/* Check that the heartbeat value is within it's range;
>  	   if not reset to the default */
> -	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
> -		heartbeat = WATCHDOG_HEARTBEAT;
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
> +	if (heartbeat < ESB_HEARTBEAT_MIN || heartbeat > ESB_HEARTBEAT_MAX) {
> +		heartbeat = ESB_HEARTBEAT_DEFAULT;
> +		pr_info("heartbeat value must be "
> +			__MODULE_STRING(ESB_HEARTBEAT_MIN) "<heartbeat<"
> +			__MODULE_STRING(ESB_HEARTBEAT_MAX) ", using %d\n",
>  			heartbeat);
>  	}
>  
>  	/* Initialize the watchdog and make sure it does not run */
>  	edev->wdd.info = &esb_info;
>  	edev->wdd.ops = &esb_ops;
> -	edev->wdd.min_timeout = 1;
> -	edev->wdd.max_timeout = 2 * 0x03ff;
> +	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
> +	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
>  	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
>  	watchdog_set_nowayout(&edev->wdd, nowayout);
>  	esb_initdevice(edev);

[PATCH v2 1/4] watchdog: i6300esb: use the watchdog subsystem

[Radu Rendec]

Change the i6300esb driver to use the watchdog subsystem instead of the
legacy watchdog API. This is mainly just getting rid of the "write" and
"ioctl" methods and part of the watchdog control logic (which are all
implemented by the watchdog subsystem).

Even though the watchdog subsystem supports registering and handling
multiple watchdog devices at the same time, the i6300esb driver still
has a limitation of only one i6300esb device due to some global variable
usage that comes from the original design. However, the driver can now
coexist with other watchdog devices (supported by different drivers).

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 222 ++++++++++----------------------------------
 1 file changed, 47 insertions(+), 175 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index d7befd58b391..6e8578dd5dab 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -21,6 +21,8 @@
  *	Version 0.02
  *  20050210 David Härdeman <david@2gen.com>
  *	Ported driver to kernel 2.6
+ *  20171016 Radu Rendec <rrendec@arista.com>
+ *	Change driver to use the watchdog subsystem
  */
 
 /*
@@ -44,7 +46,6 @@
 /* Module and version information */
 #define ESB_VERSION "0.05"
 #define ESB_MODULE_NAME "i6300ESB timer"
-#define ESB_DRIVER_NAME ESB_MODULE_NAME ", v" ESB_VERSION
 
 /* PCI configuration registers */
 #define ESB_CONFIG_REG  0x60            /* Config register                   */
@@ -77,10 +78,7 @@
 /* internal variables */
 static void __iomem *BASEADDR;
 static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
-static unsigned long timer_alive;
 static struct pci_dev *esb_pci;
-static unsigned short triggered; /* The status of the watchdog upon boot */
-static char esb_expect_close;
 
 /* We can only use 1 card due to the /dev/watchdog restriction */
 static int cards_found;
@@ -88,7 +86,7 @@ static int cards_found;
 /* module parameters */
 /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
 #define WATCHDOG_HEARTBEAT 30
-static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
+static int heartbeat; /* in seconds */
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat,
 		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
@@ -116,21 +114,22 @@ static inline void esb_unlock_registers(void)
 	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
 }
 
-static int esb_timer_start(void)
+static int esb_timer_start(struct watchdog_device *wdd)
 {
+	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
 	u8 val;
 
 	spin_lock(&esb_lock);
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* Enable or Enable + Lock? */
-	val = ESB_WDT_ENABLE | (nowayout ? ESB_WDT_LOCK : 0x00);
+	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
 	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
 	spin_unlock(&esb_lock);
 	return 0;
 }
 
-static int esb_timer_stop(void)
+static int esb_timer_stop(struct watchdog_device *wdd)
 {
 	u8 val;
 
@@ -147,22 +146,21 @@ static int esb_timer_stop(void)
 	return val & ESB_WDT_ENABLE;
 }
 
-static void esb_timer_keepalive(void)
+static int esb_timer_keepalive(struct watchdog_device *wdd)
 {
 	spin_lock(&esb_lock);
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* FIXME: Do we need to flush anything here? */
 	spin_unlock(&esb_lock);
+	return 0;
 }
 
-static int esb_timer_set_heartbeat(int time)
+static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
+		unsigned int time)
 {
 	u32 val;
 
-	if (time < 0x1 || time > (2 * 0x03ff))
-		return -EINVAL;
-
 	spin_lock(&esb_lock);
 
 	/* We shift by 9, so if we are passed a value of 1 sec,
@@ -186,148 +184,34 @@ static int esb_timer_set_heartbeat(int time)
 	/* FIXME: Do we need to flush everything out? */
 
 	/* Done */
-	heartbeat = time;
+	wdd->timeout = time;
 	spin_unlock(&esb_lock);
 	return 0;
 }
 
 /*
- *	/dev/watchdog handling
+ * Watchdog Subsystem Interfaces
  */
 
-static int esb_open(struct inode *inode, struct file *file)
-{
-	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &timer_alive))
-		return -EBUSY;
-
-	/* Reload and activate timer */
-	esb_timer_start();
-
-	return nonseekable_open(inode, file);
-}
-
-static int esb_release(struct inode *inode, struct file *file)
-{
-	/* Shut off the timer. */
-	if (esb_expect_close == 42)
-		esb_timer_stop();
-	else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		esb_timer_keepalive();
-	}
-	clear_bit(0, &timer_alive);
-	esb_expect_close = 0;
-	return 0;
-}
-
-static ssize_t esb_write(struct file *file, const char __user *data,
-			  size_t len, loff_t *ppos)
-{
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* note: just in case someone wrote the magic character
-			 * five months ago... */
-			esb_expect_close = 0;
-
-			/* scan to see whether or not we got the
-			 * magic character */
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					esb_expect_close = 42;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		esb_timer_keepalive();
-	}
-	return len;
-}
-
-static long esb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int new_options, retval = -EINVAL;
-	int new_heartbeat;
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	static const struct watchdog_info ident = {
-		.options =		WDIOF_SETTIMEOUT |
-					WDIOF_KEEPALIVEPING |
-					WDIOF_MAGICCLOSE,
-		.firmware_version =	0,
-		.identity =		ESB_MODULE_NAME,
-	};
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &ident,
-					sizeof(ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-		return put_user(0, p);
-
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(triggered, p);
-
-	case WDIOC_SETOPTIONS:
-	{
-		if (get_user(new_options, p))
-			return -EFAULT;
-
-		if (new_options & WDIOS_DISABLECARD) {
-			esb_timer_stop();
-			retval = 0;
-		}
-
-		if (new_options & WDIOS_ENABLECARD) {
-			esb_timer_start();
-			retval = 0;
-		}
-		return retval;
-	}
-	case WDIOC_KEEPALIVE:
-		esb_timer_keepalive();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-	{
-		if (get_user(new_heartbeat, p))
-			return -EFAULT;
-		if (esb_timer_set_heartbeat(new_heartbeat))
-			return -EINVAL;
-		esb_timer_keepalive();
-		/* Fall */
-	}
-	case WDIOC_GETTIMEOUT:
-		return put_user(heartbeat, p);
-	default:
-		return -ENOTTY;
-	}
-}
-
-/*
- *      Kernel Interfaces
- */
+static struct watchdog_info esb_info = {
+	.identity = ESB_MODULE_NAME,
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
 
-static const struct file_operations esb_fops = {
+static const struct watchdog_ops esb_ops = {
 	.owner = THIS_MODULE,
-	.llseek = no_llseek,
-	.write = esb_write,
-	.unlocked_ioctl = esb_ioctl,
-	.open = esb_open,
-	.release = esb_release,
+	.start = esb_timer_start,
+	.stop = esb_timer_stop,
+	.set_timeout = esb_timer_set_heartbeat,
+	.ping = esb_timer_keepalive,
 };
 
-static struct miscdevice esb_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &esb_fops,
+static struct watchdog_device esb_dev = {
+	.info = &esb_info,
+	.ops = &esb_ops,
+	.min_timeout = 1,
+	.max_timeout = 2 * 0x03ff,
+	.timeout = WATCHDOG_HEARTBEAT,
 };
 
 /*
@@ -346,19 +230,19 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
 static unsigned char esb_getdevice(struct pci_dev *pdev)
 {
 	if (pci_enable_device(pdev)) {
-		pr_err("failed to enable device\n");
+		dev_err(&pdev->dev, "failed to enable device\n");
 		goto err_devput;
 	}
 
 	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
-		pr_err("failed to request region\n");
+		dev_err(&pdev->dev, "failed to request region\n");
 		goto err_disable;
 	}
 
 	BASEADDR = pci_ioremap_bar(pdev, 0);
 	if (BASEADDR == NULL) {
 		/* Something's wrong here, BASEADDR has to be set */
-		pr_err("failed to get BASEADDR\n");
+		dev_err(&pdev->dev, "failed to get BASEADDR\n");
 		goto err_release;
 	}
 
@@ -396,7 +280,7 @@ static void esb_initdevice(void)
 	/* Check that the WDT isn't already locked */
 	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
 	if (val1 & ESB_WDT_LOCK)
-		pr_warn("nowayout already set\n");
+		dev_warn(&esb_pci->dev, "nowayout already set\n");
 
 	/* Set the timer to watchdog mode and disable it for now */
 	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
@@ -405,14 +289,14 @@ static void esb_initdevice(void)
 	esb_unlock_registers();
 	val2 = readw(ESB_RELOAD_REG);
 	if (val2 & ESB_WDT_TIMEOUT)
-		triggered = WDIOF_CARDRESET;
+		esb_dev.bootstatus = WDIOF_CARDRESET;
 
 	/* Reset WDT_TIMEOUT flag and timers */
 	esb_unlock_registers();
 	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
 
 	/* And set the correct timeout value */
-	esb_timer_set_heartbeat(heartbeat);
+	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
 }
 
 static int esb_probe(struct pci_dev *pdev,
@@ -434,26 +318,25 @@ static int esb_probe(struct pci_dev *pdev,
 	if (!esb_getdevice(pdev) || esb_pci == NULL)
 		return -ENODEV;
 
-	/* Check that the heartbeat value is within it's range;
-	   if not reset to the default */
-	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
-		heartbeat = WATCHDOG_HEARTBEAT;
-		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
-			heartbeat);
-	}
-
 	/* Initialize the watchdog and make sure it does not run */
+	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
+		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
+			esb_dev.timeout);
+	watchdog_set_nowayout(&esb_dev, nowayout);
+	watchdog_stop_on_reboot(&esb_dev);
+	watchdog_stop_on_unregister(&esb_dev);
 	esb_initdevice();
 
 	/* Register the watchdog so that userspace has access to it */
-	ret = misc_register(&esb_miscdev);
+	ret = watchdog_register_device(&esb_dev);
 	if (ret != 0) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       WATCHDOG_MINOR, ret);
+		dev_err(&pdev->dev,
+			"cannot register watchdog device (err=%d)\n", ret);
 		goto err_unmap;
 	}
-	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
-		BASEADDR, heartbeat, nowayout);
+	dev_info(&pdev->dev,
+		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
+		BASEADDR, esb_dev.timeout, nowayout);
 	return 0;
 
 err_unmap:
@@ -466,29 +349,18 @@ static int esb_probe(struct pci_dev *pdev,
 
 static void esb_remove(struct pci_dev *pdev)
 {
-	/* Stop the timer before we leave */
-	if (!nowayout)
-		esb_timer_stop();
-
-	/* Deregister */
-	misc_deregister(&esb_miscdev);
+	watchdog_unregister_device(&esb_dev);
 	iounmap(BASEADDR);
 	pci_release_region(esb_pci, 0);
 	pci_disable_device(esb_pci);
 	esb_pci = NULL;
 }
 
-static void esb_shutdown(struct pci_dev *pdev)
-{
-	esb_timer_stop();
-}
-
 static struct pci_driver esb_driver = {
 	.name		= ESB_MODULE_NAME,
 	.id_table	= esb_pci_tbl,
 	.probe          = esb_probe,
 	.remove         = esb_remove,
-	.shutdown       = esb_shutdown,
 };
 
 module_pci_driver(esb_driver);
-- 
2.13.6

[PATCH v2 2/4] watchdog: i6300esb: support multiple devices

[Radu Rendec]

Support multiple i6300esb devices simultaneously, by removing the single
device restriction in the original design and leveraging the multiple
device support of the watchdog subsystem.

This patch replaces the global definitions of watchdog device data with
a dynamically allocated structure. This structure is allocated during
device probe, so multiple independent structures can be allocated if
multiple devices are probed.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 189 +++++++++++++++++++++++---------------------
 1 file changed, 100 insertions(+), 89 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 6e8578dd5dab..0b4489661a82 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -23,6 +23,7 @@
  *	Ported driver to kernel 2.6
  *  20171016 Radu Rendec <rrendec@arista.com>
  *	Change driver to use the watchdog subsystem
+ *	Add support for multiple 6300ESB devices
  */
 
 /*
@@ -52,10 +53,10 @@
 #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
 
 /* Memory mapped registers */
-#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
-#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
-#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
-#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
+#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
+#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
+#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
+#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
 
 /* Lock register bits */
 #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
@@ -75,12 +76,7 @@
 #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
 #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
 
-/* internal variables */
-static void __iomem *BASEADDR;
-static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
-static struct pci_dev *esb_pci;
-
-/* We can only use 1 card due to the /dev/watchdog restriction */
+/* Probed devices counter; used only for printing the initial info message */
 static int cards_found;
 
 /* module parameters */
@@ -98,6 +94,16 @@ MODULE_PARM_DESC(nowayout,
 		"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+/* internal variables */
+struct esb_dev {
+	struct watchdog_device wdd;
+	void __iomem *base;
+	spinlock_t lock;
+	struct pci_dev *pdev;
+};
+
+#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
+
 /*
  * Some i6300ESB specific functions
  */
@@ -108,39 +114,41 @@ MODULE_PARM_DESC(nowayout,
  * reload register. After this the appropriate registers can be written
  * to once before they need to be unlocked again.
  */
-static inline void esb_unlock_registers(void)
+static inline void esb_unlock_registers(struct esb_dev *edev)
 {
-	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
-	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
+	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
+	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
 }
 
 static int esb_timer_start(struct watchdog_device *wdd)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
 	u8 val;
 
-	spin_lock(&esb_lock);
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	spin_lock(&edev->lock);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* Enable or Enable + Lock? */
 	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
-	spin_unlock(&esb_lock);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
+	spin_unlock(&edev->lock);
 	return 0;
 }
 
 static int esb_timer_stop(struct watchdog_device *wdd)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	u8 val;
 
-	spin_lock(&esb_lock);
+	spin_lock(&edev->lock);
 	/* First, reset timers as suggested by the docs */
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* Then disable the WDT */
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
-	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
-	spin_unlock(&esb_lock);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
+	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
+	spin_unlock(&edev->lock);
 
 	/* Returns 0 if the timer was disabled, non-zero otherwise */
 	return val & ESB_WDT_ENABLE;
@@ -148,20 +156,23 @@ static int esb_timer_stop(struct watchdog_device *wdd)
 
 static int esb_timer_keepalive(struct watchdog_device *wdd)
 {
-	spin_lock(&esb_lock);
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	struct esb_dev *edev = to_esb_dev(wdd);
+
+	spin_lock(&edev->lock);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* FIXME: Do we need to flush anything here? */
-	spin_unlock(&esb_lock);
+	spin_unlock(&edev->lock);
 	return 0;
 }
 
 static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 		unsigned int time)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	u32 val;
 
-	spin_lock(&esb_lock);
+	spin_lock(&edev->lock);
 
 	/* We shift by 9, so if we are passed a value of 1 sec,
 	 * val will be 1 << 9 = 512, then write that to two
@@ -170,22 +181,22 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 	val = time << 9;
 
 	/* Write timer 1 */
-	esb_unlock_registers();
-	writel(val, ESB_TIMER1_REG);
+	esb_unlock_registers(edev);
+	writel(val, ESB_TIMER1_REG(edev));
 
 	/* Write timer 2 */
-	esb_unlock_registers();
-	writel(val, ESB_TIMER2_REG);
+	esb_unlock_registers(edev);
+	writel(val, ESB_TIMER2_REG(edev));
 
 	/* Reload */
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 
 	/* FIXME: Do we need to flush everything out? */
 
 	/* Done */
 	wdd->timeout = time;
-	spin_unlock(&esb_lock);
+	spin_unlock(&edev->lock);
 	return 0;
 }
 
@@ -206,14 +217,6 @@ static const struct watchdog_ops esb_ops = {
 	.ping = esb_timer_keepalive,
 };
 
-static struct watchdog_device esb_dev = {
-	.info = &esb_info,
-	.ops = &esb_ops,
-	.min_timeout = 1,
-	.max_timeout = 2 * 0x03ff,
-	.timeout = WATCHDOG_HEARTBEAT,
-};
-
 /*
  * Data for PCI driver interface
  */
@@ -227,38 +230,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
  *      Init & exit routines
  */
 
-static unsigned char esb_getdevice(struct pci_dev *pdev)
+static unsigned char esb_getdevice(struct esb_dev *edev)
 {
-	if (pci_enable_device(pdev)) {
-		dev_err(&pdev->dev, "failed to enable device\n");
+	if (pci_enable_device(edev->pdev)) {
+		dev_err(&edev->pdev->dev, "failed to enable device\n");
 		goto err_devput;
 	}
 
-	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
-		dev_err(&pdev->dev, "failed to request region\n");
+	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
+		dev_err(&edev->pdev->dev, "failed to request region\n");
 		goto err_disable;
 	}
 
-	BASEADDR = pci_ioremap_bar(pdev, 0);
-	if (BASEADDR == NULL) {
+	edev->base = pci_ioremap_bar(edev->pdev, 0);
+	if (edev->base == NULL) {
 		/* Something's wrong here, BASEADDR has to be set */
-		dev_err(&pdev->dev, "failed to get BASEADDR\n");
+		dev_err(&edev->pdev->dev, "failed to get BASEADDR\n");
 		goto err_release;
 	}
 
 	/* Done */
-	esb_pci = pdev;
+	dev_set_drvdata(&edev->pdev->dev, edev);
 	return 1;
 
 err_release:
-	pci_release_region(pdev, 0);
+	pci_release_region(edev->pdev, 0);
 err_disable:
-	pci_disable_device(pdev);
+	pci_disable_device(edev->pdev);
 err_devput:
 	return 0;
 }
 
-static void esb_initdevice(void)
+static void esb_initdevice(struct esb_dev *edev)
 {
 	u8 val1;
 	u16 val2;
@@ -275,33 +278,34 @@ static void esb_initdevice(void)
 	 * any interrupts as there is not much we can do with it
 	 * right now.
 	 */
-	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
+	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
 
 	/* Check that the WDT isn't already locked */
-	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
+	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
 	if (val1 & ESB_WDT_LOCK)
-		dev_warn(&esb_pci->dev, "nowayout already set\n");
+		dev_warn(&edev->pdev->dev, "nowayout already set\n");
 
 	/* Set the timer to watchdog mode and disable it for now */
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
 
 	/* Check if the watchdog was previously triggered */
-	esb_unlock_registers();
-	val2 = readw(ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	val2 = readw(ESB_RELOAD_REG(edev));
 	if (val2 & ESB_WDT_TIMEOUT)
-		esb_dev.bootstatus = WDIOF_CARDRESET;
+		edev->wdd.bootstatus = WDIOF_CARDRESET;
 
 	/* Reset WDT_TIMEOUT flag and timers */
-	esb_unlock_registers();
-	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
 
 	/* And set the correct timeout value */
-	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
+	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
 }
 
 static int esb_probe(struct pci_dev *pdev,
 		const struct pci_device_id *ent)
 {
+	struct esb_dev *edev;
 	int ret;
 
 	cards_found++;
@@ -309,26 +313,33 @@ static int esb_probe(struct pci_dev *pdev,
 		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
 			ESB_VERSION);
 
-	if (cards_found > 1) {
-		pr_err("This driver only supports 1 device\n");
-		return -ENODEV;
-	}
+	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
+	if (!edev)
+		return -ENOMEM;
 
 	/* Check whether or not the hardware watchdog is there */
-	if (!esb_getdevice(pdev) || esb_pci == NULL)
+	spin_lock_init(&edev->lock);
+	edev->pdev = pdev;
+	if (!esb_getdevice(edev))
 		return -ENODEV;
 
 	/* Initialize the watchdog and make sure it does not run */
-	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
-		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
-			esb_dev.timeout);
-	watchdog_set_nowayout(&esb_dev, nowayout);
-	watchdog_stop_on_reboot(&esb_dev);
-	watchdog_stop_on_unregister(&esb_dev);
-	esb_initdevice();
+	edev->wdd.info = &esb_info;
+	edev->wdd.ops = &esb_ops;
+	edev->wdd.min_timeout = 1;
+	edev->wdd.max_timeout = 2 * 0x03ff;
+	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
+	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
+		dev_info(&pdev->dev,
+			"heartbeat value must be 1<heartbeat<2046, using %u\n",
+			edev->wdd.timeout);
+	watchdog_set_nowayout(&edev->wdd, nowayout);
+	watchdog_stop_on_reboot(&edev->wdd);
+	watchdog_stop_on_unregister(&edev->wdd);
+	esb_initdevice(edev);
 
 	/* Register the watchdog so that userspace has access to it */
-	ret = watchdog_register_device(&esb_dev);
+	ret = watchdog_register_device(&edev->wdd);
 	if (ret != 0) {
 		dev_err(&pdev->dev,
 			"cannot register watchdog device (err=%d)\n", ret);
@@ -336,24 +347,24 @@ static int esb_probe(struct pci_dev *pdev,
 	}
 	dev_info(&pdev->dev,
 		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
-		BASEADDR, esb_dev.timeout, nowayout);
+		edev->base, edev->wdd.timeout, nowayout);
 	return 0;
 
 err_unmap:
-	iounmap(BASEADDR);
-	pci_release_region(esb_pci, 0);
-	pci_disable_device(esb_pci);
-	esb_pci = NULL;
+	iounmap(edev->base);
+	pci_release_region(edev->pdev, 0);
+	pci_disable_device(edev->pdev);
 	return ret;
 }
 
 static void esb_remove(struct pci_dev *pdev)
 {
-	watchdog_unregister_device(&esb_dev);
-	iounmap(BASEADDR);
-	pci_release_region(esb_pci, 0);
-	pci_disable_device(esb_pci);
-	esb_pci = NULL;
+	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
+
+	watchdog_unregister_device(&edev->wdd);
+	iounmap(edev->base);
+	pci_release_region(edev->pdev, 0);
+	pci_disable_device(edev->pdev);
 }
 
 static struct pci_driver esb_driver = {
-- 
2.13.6

[PATCH v2 3/4] watchdog: i6300esb: do not hardcode heartbeat limits

[Radu Rendec]

The minimum, maximum and default values for the watchdog heartbeat
(timeout) were hardcoded in several places (including module parameter
description and warning message for invalid module parameter value).

This patch adds macros for the aforementioned values and replaces all
occurences of hardcoded values by these macros.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 0b4489661a82..3f7f66513dfa 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -81,12 +81,16 @@ static int cards_found;
 
 /* module parameters */
 /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
-#define WATCHDOG_HEARTBEAT 30
+#define ESB_HEARTBEAT_MIN 1
+#define ESB_HEARTBEAT_MAX 2046
+#define ESB_HEARTBEAT_DEFAULT 30
+#define ESB_HEARTBEAT_RANGE __MODULE_STRING(ESB_HEARTBEAT_MIN) \
+	"<heartbeat<" __MODULE_STRING(ESB_HEARTBEAT_MAX)
 static int heartbeat; /* in seconds */
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat,
-		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
-				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
+	"Watchdog heartbeat in seconds. (" ESB_HEARTBEAT_RANGE
+	", default=" __MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
@@ -326,13 +330,13 @@ static int esb_probe(struct pci_dev *pdev,
 	/* Initialize the watchdog and make sure it does not run */
 	edev->wdd.info = &esb_info;
 	edev->wdd.ops = &esb_ops;
-	edev->wdd.min_timeout = 1;
-	edev->wdd.max_timeout = 2 * 0x03ff;
-	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
+	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
+	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
+	edev->wdd.timeout = ESB_HEARTBEAT_DEFAULT;
 	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
 		dev_info(&pdev->dev,
-			"heartbeat value must be 1<heartbeat<2046, using %u\n",
-			edev->wdd.timeout);
+			"heartbeat value must be " ESB_HEARTBEAT_RANGE
+			", using %u\n", edev->wdd.timeout);
 	watchdog_set_nowayout(&edev->wdd, nowayout);
 	watchdog_stop_on_reboot(&edev->wdd);
 	watchdog_stop_on_unregister(&edev->wdd);
-- 
2.13.6

[PATCH v2 4/4] watchdog: i6300esb: bump version to 0.6

[Radu Rendec]

The new version incorporates two major changes:
* Watchdog subsystem support - allows the driver to coexist with other
  watchdog drivers; simplifies the code base
* Multiple device support - supports multiple 6300ESB timer devices and
  and exposes them as separate watchdogs

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 3f7f66513dfa..fba81fa2f1c1 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -45,7 +45,7 @@
 #include <linux/io.h>
 
 /* Module and version information */
-#define ESB_VERSION "0.05"
+#define ESB_VERSION "0.06"
 #define ESB_MODULE_NAME "i6300ESB timer"
 
 /* PCI configuration registers */
-- 
2.13.6

Re: [PATCH v2 1/4] watchdog: i6300esb: use the watchdog subsystem

[Guenter Roeck]

On Wed, Oct 25, 2017 at 04:39:11PM +0100, Radu Rendec wrote:
> Change the i6300esb driver to use the watchdog subsystem instead of the
> legacy watchdog API. This is mainly just getting rid of the "write" and
> "ioctl" methods and part of the watchdog control logic (which are all
> implemented by the watchdog subsystem).
> 
> Even though the watchdog subsystem supports registering and handling
> multiple watchdog devices at the same time, the i6300esb driver still
> has a limitation of only one i6300esb device due to some global variable
> usage that comes from the original design. However, the driver can now
> coexist with other watchdog devices (supported by different drivers).
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 222 ++++++++++----------------------------------
>  1 file changed, 47 insertions(+), 175 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index d7befd58b391..6e8578dd5dab 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -21,6 +21,8 @@
>   *	Version 0.02
>   *  20050210 David Härdeman <david@2gen.com>
>   *	Ported driver to kernel 2.6
> + *  20171016 Radu Rendec <rrendec@arista.com>
> + *	Change driver to use the watchdog subsystem
>   */
>  
>  /*
> @@ -44,7 +46,6 @@
>  /* Module and version information */
>  #define ESB_VERSION "0.05"
>  #define ESB_MODULE_NAME "i6300ESB timer"
> -#define ESB_DRIVER_NAME ESB_MODULE_NAME ", v" ESB_VERSION
>  
>  /* PCI configuration registers */
>  #define ESB_CONFIG_REG  0x60            /* Config register                   */
> @@ -77,10 +78,7 @@
>  /* internal variables */
>  static void __iomem *BASEADDR;
>  static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
> -static unsigned long timer_alive;
>  static struct pci_dev *esb_pci;
> -static unsigned short triggered; /* The status of the watchdog upon boot */
> -static char esb_expect_close;
>  
>  /* We can only use 1 card due to the /dev/watchdog restriction */
>  static int cards_found;
> @@ -88,7 +86,7 @@ static int cards_found;
>  /* module parameters */
>  /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
>  #define WATCHDOG_HEARTBEAT 30
> -static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
> +static int heartbeat; /* in seconds */
>  module_param(heartbeat, int, 0);
>  MODULE_PARM_DESC(heartbeat,
>  		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
> @@ -116,21 +114,22 @@ static inline void esb_unlock_registers(void)
>  	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
>  }
>  
> -static int esb_timer_start(void)
> +static int esb_timer_start(struct watchdog_device *wdd)
>  {
> +	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>  	u8 val;
>  
>  	spin_lock(&esb_lock);

Actually, this lock should no longer be needed. The watchdog core handles
locking. Sorry, should have noticed before.

>  	esb_unlock_registers();
>  	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>  	/* Enable or Enable + Lock? */
> -	val = ESB_WDT_ENABLE | (nowayout ? ESB_WDT_LOCK : 0x00);
> +	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
>  	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
>  	spin_unlock(&esb_lock);
>  	return 0;
>  }
>  
> -static int esb_timer_stop(void)
> +static int esb_timer_stop(struct watchdog_device *wdd)
>  {
>  	u8 val;
>  
> @@ -147,22 +146,21 @@ static int esb_timer_stop(void)
>  	return val & ESB_WDT_ENABLE;
>  }
>  
> -static void esb_timer_keepalive(void)
> +static int esb_timer_keepalive(struct watchdog_device *wdd)
>  {
>  	spin_lock(&esb_lock);
>  	esb_unlock_registers();
>  	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>  	/* FIXME: Do we need to flush anything here? */
>  	spin_unlock(&esb_lock);
> +	return 0;
>  }
>  
> -static int esb_timer_set_heartbeat(int time)
> +static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
> +		unsigned int time)
>  {
>  	u32 val;
>  
> -	if (time < 0x1 || time > (2 * 0x03ff))
> -		return -EINVAL;
> -
>  	spin_lock(&esb_lock);
>  
>  	/* We shift by 9, so if we are passed a value of 1 sec,
> @@ -186,148 +184,34 @@ static int esb_timer_set_heartbeat(int time)
>  	/* FIXME: Do we need to flush everything out? */
>  
>  	/* Done */
> -	heartbeat = time;
> +	wdd->timeout = time;
>  	spin_unlock(&esb_lock);
>  	return 0;
>  }
>  
>  /*
> - *	/dev/watchdog handling
> + * Watchdog Subsystem Interfaces
>   */
>  
> -static int esb_open(struct inode *inode, struct file *file)
> -{
> -	/* /dev/watchdog can only be opened once */
> -	if (test_and_set_bit(0, &timer_alive))
> -		return -EBUSY;
> -
> -	/* Reload and activate timer */
> -	esb_timer_start();
> -
> -	return nonseekable_open(inode, file);
> -}
> -
> -static int esb_release(struct inode *inode, struct file *file)
> -{
> -	/* Shut off the timer. */
> -	if (esb_expect_close == 42)
> -		esb_timer_stop();
> -	else {
> -		pr_crit("Unexpected close, not stopping watchdog!\n");
> -		esb_timer_keepalive();
> -	}
> -	clear_bit(0, &timer_alive);
> -	esb_expect_close = 0;
> -	return 0;
> -}
> -
> -static ssize_t esb_write(struct file *file, const char __user *data,
> -			  size_t len, loff_t *ppos)
> -{
> -	/* See if we got the magic character 'V' and reload the timer */
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			/* note: just in case someone wrote the magic character
> -			 * five months ago... */
> -			esb_expect_close = 0;
> -
> -			/* scan to see whether or not we got the
> -			 * magic character */
> -			for (i = 0; i != len; i++) {
> -				char c;
> -				if (get_user(c, data + i))
> -					return -EFAULT;
> -				if (c == 'V')
> -					esb_expect_close = 42;
> -			}
> -		}
> -
> -		/* someone wrote to us, we should reload the timer */
> -		esb_timer_keepalive();
> -	}
> -	return len;
> -}
> -
> -static long esb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> -{
> -	int new_options, retval = -EINVAL;
> -	int new_heartbeat;
> -	void __user *argp = (void __user *)arg;
> -	int __user *p = argp;
> -	static const struct watchdog_info ident = {
> -		.options =		WDIOF_SETTIMEOUT |
> -					WDIOF_KEEPALIVEPING |
> -					WDIOF_MAGICCLOSE,
> -		.firmware_version =	0,
> -		.identity =		ESB_MODULE_NAME,
> -	};
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		return copy_to_user(argp, &ident,
> -					sizeof(ident)) ? -EFAULT : 0;
> -
> -	case WDIOC_GETSTATUS:
> -		return put_user(0, p);
> -
> -	case WDIOC_GETBOOTSTATUS:
> -		return put_user(triggered, p);
> -
> -	case WDIOC_SETOPTIONS:
> -	{
> -		if (get_user(new_options, p))
> -			return -EFAULT;
> -
> -		if (new_options & WDIOS_DISABLECARD) {
> -			esb_timer_stop();
> -			retval = 0;
> -		}
> -
> -		if (new_options & WDIOS_ENABLECARD) {
> -			esb_timer_start();
> -			retval = 0;
> -		}
> -		return retval;
> -	}
> -	case WDIOC_KEEPALIVE:
> -		esb_timer_keepalive();
> -		return 0;
> -
> -	case WDIOC_SETTIMEOUT:
> -	{
> -		if (get_user(new_heartbeat, p))
> -			return -EFAULT;
> -		if (esb_timer_set_heartbeat(new_heartbeat))
> -			return -EINVAL;
> -		esb_timer_keepalive();
> -		/* Fall */
> -	}
> -	case WDIOC_GETTIMEOUT:
> -		return put_user(heartbeat, p);
> -	default:
> -		return -ENOTTY;
> -	}
> -}
> -
> -/*
> - *      Kernel Interfaces
> - */
> +static struct watchdog_info esb_info = {
> +	.identity = ESB_MODULE_NAME,
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +};
>  
> -static const struct file_operations esb_fops = {
> +static const struct watchdog_ops esb_ops = {
>  	.owner = THIS_MODULE,
> -	.llseek = no_llseek,
> -	.write = esb_write,
> -	.unlocked_ioctl = esb_ioctl,
> -	.open = esb_open,
> -	.release = esb_release,
> +	.start = esb_timer_start,
> +	.stop = esb_timer_stop,
> +	.set_timeout = esb_timer_set_heartbeat,
> +	.ping = esb_timer_keepalive,
>  };
>  
> -static struct miscdevice esb_miscdev = {
> -	.minor = WATCHDOG_MINOR,
> -	.name = "watchdog",
> -	.fops = &esb_fops,
> +static struct watchdog_device esb_dev = {
> +	.info = &esb_info,
> +	.ops = &esb_ops,
> +	.min_timeout = 1,
> +	.max_timeout = 2 * 0x03ff,
> +	.timeout = WATCHDOG_HEARTBEAT,
>  };
>  
>  /*
> @@ -346,19 +230,19 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
>  static unsigned char esb_getdevice(struct pci_dev *pdev)
>  {
>  	if (pci_enable_device(pdev)) {
> -		pr_err("failed to enable device\n");
> +		dev_err(&pdev->dev, "failed to enable device\n");
>  		goto err_devput;
>  	}
>  
>  	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> -		pr_err("failed to request region\n");
> +		dev_err(&pdev->dev, "failed to request region\n");
>  		goto err_disable;
>  	}
>  
>  	BASEADDR = pci_ioremap_bar(pdev, 0);
>  	if (BASEADDR == NULL) {
>  		/* Something's wrong here, BASEADDR has to be set */
> -		pr_err("failed to get BASEADDR\n");
> +		dev_err(&pdev->dev, "failed to get BASEADDR\n");
>  		goto err_release;
>  	}
>  
> @@ -396,7 +280,7 @@ static void esb_initdevice(void)
>  	/* Check that the WDT isn't already locked */
>  	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
>  	if (val1 & ESB_WDT_LOCK)
> -		pr_warn("nowayout already set\n");
> +		dev_warn(&esb_pci->dev, "nowayout already set\n");
>  
>  	/* Set the timer to watchdog mode and disable it for now */
>  	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> @@ -405,14 +289,14 @@ static void esb_initdevice(void)
>  	esb_unlock_registers();
>  	val2 = readw(ESB_RELOAD_REG);
>  	if (val2 & ESB_WDT_TIMEOUT)
> -		triggered = WDIOF_CARDRESET;
> +		esb_dev.bootstatus = WDIOF_CARDRESET;
>  
>  	/* Reset WDT_TIMEOUT flag and timers */
>  	esb_unlock_registers();
>  	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
>  
>  	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(heartbeat);
> +	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
>  }
>  
>  static int esb_probe(struct pci_dev *pdev,
> @@ -434,26 +318,25 @@ static int esb_probe(struct pci_dev *pdev,
>  	if (!esb_getdevice(pdev) || esb_pci == NULL)
>  		return -ENODEV;
>  
> -	/* Check that the heartbeat value is within it's range;
> -	   if not reset to the default */
> -	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
> -		heartbeat = WATCHDOG_HEARTBEAT;
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
> -			heartbeat);
> -	}
> -
>  	/* Initialize the watchdog and make sure it does not run */
> +	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
> +		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
> +			esb_dev.timeout);
> +	watchdog_set_nowayout(&esb_dev, nowayout);
> +	watchdog_stop_on_reboot(&esb_dev);
> +	watchdog_stop_on_unregister(&esb_dev);
>  	esb_initdevice();
>  
>  	/* Register the watchdog so that userspace has access to it */
> -	ret = misc_register(&esb_miscdev);
> +	ret = watchdog_register_device(&esb_dev);
>  	if (ret != 0) {
> -		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> -		       WATCHDOG_MINOR, ret);
> +		dev_err(&pdev->dev,
> +			"cannot register watchdog device (err=%d)\n", ret);
>  		goto err_unmap;
>  	}
> -	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> -		BASEADDR, heartbeat, nowayout);
> +	dev_info(&pdev->dev,
> +		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> +		BASEADDR, esb_dev.timeout, nowayout);
>  	return 0;
>  
>  err_unmap:
> @@ -466,29 +349,18 @@ static int esb_probe(struct pci_dev *pdev,
>  
>  static void esb_remove(struct pci_dev *pdev)
>  {
> -	/* Stop the timer before we leave */
> -	if (!nowayout)
> -		esb_timer_stop();
> -
> -	/* Deregister */
> -	misc_deregister(&esb_miscdev);
> +	watchdog_unregister_device(&esb_dev);
>  	iounmap(BASEADDR);
>  	pci_release_region(esb_pci, 0);
>  	pci_disable_device(esb_pci);
>  	esb_pci = NULL;
>  }
>  
> -static void esb_shutdown(struct pci_dev *pdev)
> -{
> -	esb_timer_stop();
> -}
> -
>  static struct pci_driver esb_driver = {
>  	.name		= ESB_MODULE_NAME,
>  	.id_table	= esb_pci_tbl,
>  	.probe          = esb_probe,
>  	.remove         = esb_remove,
> -	.shutdown       = esb_shutdown,
>  };
>  
>  module_pci_driver(esb_driver);
> -- 
> 2.13.6
> 

Re: [PATCH v2 2/4] watchdog: i6300esb: support multiple devices

[Guenter Roeck]

On Wed, Oct 25, 2017 at 04:39:12PM +0100, Radu Rendec wrote:
> Support multiple i6300esb devices simultaneously, by removing the single
> device restriction in the original design and leveraging the multiple
> device support of the watchdog subsystem.
> 
> This patch replaces the global definitions of watchdog device data with
> a dynamically allocated structure. This structure is allocated during
> device probe, so multiple independent structures can be allocated if
> multiple devices are probed.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 189 +++++++++++++++++++++++---------------------
>  1 file changed, 100 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 6e8578dd5dab..0b4489661a82 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -23,6 +23,7 @@
>   *	Ported driver to kernel 2.6
>   *  20171016 Radu Rendec <rrendec@arista.com>
>   *	Change driver to use the watchdog subsystem
> + *	Add support for multiple 6300ESB devices
>   */
>  
>  /*
> @@ -52,10 +53,10 @@
>  #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
>  
>  /* Memory mapped registers */
> -#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
> -#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
> -#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
> -#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
> +#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
> +#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
> +#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
> +#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
>  
>  /* Lock register bits */
>  #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
> @@ -75,12 +76,7 @@
>  #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
>  #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
>  
> -/* internal variables */
> -static void __iomem *BASEADDR;
> -static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
> -static struct pci_dev *esb_pci;
> -
> -/* We can only use 1 card due to the /dev/watchdog restriction */
> +/* Probed devices counter; used only for printing the initial info message */
>  static int cards_found;

Now unnecessary.

>  
>  /* module parameters */
> @@ -98,6 +94,16 @@ MODULE_PARM_DESC(nowayout,
>  		"Watchdog cannot be stopped once started (default="
>  				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +/* internal variables */
> +struct esb_dev {
> +	struct watchdog_device wdd;
> +	void __iomem *base;
> +	spinlock_t lock;

Unnecessary - see patch 1.

> +	struct pci_dev *pdev;
> +};
> +
> +#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
> +
>  /*
>   * Some i6300ESB specific functions
>   */
> @@ -108,39 +114,41 @@ MODULE_PARM_DESC(nowayout,
>   * reload register. After this the appropriate registers can be written
>   * to once before they need to be unlocked again.
>   */
> -static inline void esb_unlock_registers(void)
> +static inline void esb_unlock_registers(struct esb_dev *edev)
>  {
> -	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
> -	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
> +	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
> +	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
>  }
>  
>  static int esb_timer_start(struct watchdog_device *wdd)
>  {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>  	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>  	u8 val;
>  
> -	spin_lock(&esb_lock);
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	spin_lock(&edev->lock);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  	/* Enable or Enable + Lock? */
>  	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
> -	spin_unlock(&esb_lock);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
> +	spin_unlock(&edev->lock);
>  	return 0;
>  }
>  
>  static int esb_timer_stop(struct watchdog_device *wdd)
>  {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>  	u8 val;
>  
> -	spin_lock(&esb_lock);
> +	spin_lock(&edev->lock);
>  	/* First, reset timers as suggested by the docs */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  	/* Then disable the WDT */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
> -	spin_unlock(&esb_lock);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
> +	spin_unlock(&edev->lock);
>  
>  	/* Returns 0 if the timer was disabled, non-zero otherwise */
>  	return val & ESB_WDT_ENABLE;
> @@ -148,20 +156,23 @@ static int esb_timer_stop(struct watchdog_device *wdd)
>  
>  static int esb_timer_keepalive(struct watchdog_device *wdd)
>  {
> -	spin_lock(&esb_lock);
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	struct esb_dev *edev = to_esb_dev(wdd);
> +
> +	spin_lock(&edev->lock);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  	/* FIXME: Do we need to flush anything here? */
> -	spin_unlock(&esb_lock);
> +	spin_unlock(&edev->lock);
>  	return 0;
>  }
>  
>  static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>  		unsigned int time)
>  {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>  	u32 val;
>  
> -	spin_lock(&esb_lock);
> +	spin_lock(&edev->lock);
>  
>  	/* We shift by 9, so if we are passed a value of 1 sec,
>  	 * val will be 1 << 9 = 512, then write that to two
> @@ -170,22 +181,22 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>  	val = time << 9;
>  
>  	/* Write timer 1 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER1_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER1_REG(edev));
>  
>  	/* Write timer 2 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER2_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER2_REG(edev));
>  
>  	/* Reload */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>  
>  	/* FIXME: Do we need to flush everything out? */
>  
>  	/* Done */
>  	wdd->timeout = time;
> -	spin_unlock(&esb_lock);
> +	spin_unlock(&edev->lock);
>  	return 0;
>  }
>  
> @@ -206,14 +217,6 @@ static const struct watchdog_ops esb_ops = {
>  	.ping = esb_timer_keepalive,
>  };
>  
> -static struct watchdog_device esb_dev = {
> -	.info = &esb_info,
> -	.ops = &esb_ops,
> -	.min_timeout = 1,
> -	.max_timeout = 2 * 0x03ff,
> -	.timeout = WATCHDOG_HEARTBEAT,
> -};
> -
>  /*
>   * Data for PCI driver interface
>   */
> @@ -227,38 +230,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
>   *      Init & exit routines
>   */
>  
> -static unsigned char esb_getdevice(struct pci_dev *pdev)
> +static unsigned char esb_getdevice(struct esb_dev *edev)
>  {
> -	if (pci_enable_device(pdev)) {
> -		dev_err(&pdev->dev, "failed to enable device\n");
> +	if (pci_enable_device(edev->pdev)) {
> +		dev_err(&edev->pdev->dev, "failed to enable device\n");
>  		goto err_devput;
>  	}
>  
> -	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> -		dev_err(&pdev->dev, "failed to request region\n");
> +	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
> +		dev_err(&edev->pdev->dev, "failed to request region\n");
>  		goto err_disable;
>  	}
>  
> -	BASEADDR = pci_ioremap_bar(pdev, 0);
> -	if (BASEADDR == NULL) {
> +	edev->base = pci_ioremap_bar(edev->pdev, 0);
> +	if (edev->base == NULL) {
>  		/* Something's wrong here, BASEADDR has to be set */
> -		dev_err(&pdev->dev, "failed to get BASEADDR\n");
> +		dev_err(&edev->pdev->dev, "failed to get BASEADDR\n");
>  		goto err_release;
>  	}
>  
>  	/* Done */
> -	esb_pci = pdev;
> +	dev_set_drvdata(&edev->pdev->dev, edev);
>  	return 1;
>  
>  err_release:
> -	pci_release_region(pdev, 0);
> +	pci_release_region(edev->pdev, 0);
>  err_disable:
> -	pci_disable_device(pdev);
> +	pci_disable_device(edev->pdev);
>  err_devput:
>  	return 0;
>  }
>  
> -static void esb_initdevice(void)
> +static void esb_initdevice(struct esb_dev *edev)
>  {
>  	u8 val1;
>  	u16 val2;
> @@ -275,33 +278,34 @@ static void esb_initdevice(void)
>  	 * any interrupts as there is not much we can do with it
>  	 * right now.
>  	 */
> -	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
> +	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
>  
>  	/* Check that the WDT isn't already locked */
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
>  	if (val1 & ESB_WDT_LOCK)
> -		dev_warn(&esb_pci->dev, "nowayout already set\n");
> +		dev_warn(&edev->pdev->dev, "nowayout already set\n");
>  
>  	/* Set the timer to watchdog mode and disable it for now */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
>  
>  	/* Check if the watchdog was previously triggered */
> -	esb_unlock_registers();
> -	val2 = readw(ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	val2 = readw(ESB_RELOAD_REG(edev));
>  	if (val2 & ESB_WDT_TIMEOUT)
> -		esb_dev.bootstatus = WDIOF_CARDRESET;
> +		edev->wdd.bootstatus = WDIOF_CARDRESET;
>  
>  	/* Reset WDT_TIMEOUT flag and timers */
> -	esb_unlock_registers();
> -	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
>  
>  	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
> +	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
>  }
>  
>  static int esb_probe(struct pci_dev *pdev,
>  		const struct pci_device_id *ent)
>  {
> +	struct esb_dev *edev;
>  	int ret;
>  
>  	cards_found++;

Now unnecessary.

> @@ -309,26 +313,33 @@ static int esb_probe(struct pci_dev *pdev,
>  		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
>  			ESB_VERSION);
>  
> -	if (cards_found > 1) {
> -		pr_err("This driver only supports 1 device\n");
> -		return -ENODEV;
> -	}
> +	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
> +	if (!edev)
> +		return -ENOMEM;
>  
>  	/* Check whether or not the hardware watchdog is there */
> -	if (!esb_getdevice(pdev) || esb_pci == NULL)
> +	spin_lock_init(&edev->lock);
> +	edev->pdev = pdev;
> +	if (!esb_getdevice(edev))
>  		return -ENODEV;
>  
>  	/* Initialize the watchdog and make sure it does not run */
> -	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
> -			esb_dev.timeout);
> -	watchdog_set_nowayout(&esb_dev, nowayout);
> -	watchdog_stop_on_reboot(&esb_dev);
> -	watchdog_stop_on_unregister(&esb_dev);
> -	esb_initdevice();
> +	edev->wdd.info = &esb_info;
> +	edev->wdd.ops = &esb_ops;
> +	edev->wdd.min_timeout = 1;
> +	edev->wdd.max_timeout = 2 * 0x03ff;
> +	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
> +	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
> +		dev_info(&pdev->dev,
> +			"heartbeat value must be 1<heartbeat<2046, using %u\n",
> +			edev->wdd.timeout);
> +	watchdog_set_nowayout(&edev->wdd, nowayout);
> +	watchdog_stop_on_reboot(&edev->wdd);
> +	watchdog_stop_on_unregister(&edev->wdd);
> +	esb_initdevice(edev);
>  
>  	/* Register the watchdog so that userspace has access to it */
> -	ret = watchdog_register_device(&esb_dev);
> +	ret = watchdog_register_device(&edev->wdd);
>  	if (ret != 0) {
>  		dev_err(&pdev->dev,
>  			"cannot register watchdog device (err=%d)\n", ret);
> @@ -336,24 +347,24 @@ static int esb_probe(struct pci_dev *pdev,
>  	}
>  	dev_info(&pdev->dev,
>  		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> -		BASEADDR, esb_dev.timeout, nowayout);
> +		edev->base, edev->wdd.timeout, nowayout);
>  	return 0;
>  
>  err_unmap:
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>  	return ret;
>  }
>  
>  static void esb_remove(struct pci_dev *pdev)
>  {
> -	watchdog_unregister_device(&esb_dev);
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> +
> +	watchdog_unregister_device(&edev->wdd);
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>  }
>  
>  static struct pci_driver esb_driver = {
> -- 
> 2.13.6
> 

Re: [PATCH v2 3/4] watchdog: i6300esb: do not hardcode heartbeat limits

[Guenter Roeck]

On Wed, Oct 25, 2017 at 04:39:13PM +0100, Radu Rendec wrote:
> The minimum, maximum and default values for the watchdog heartbeat
> (timeout) were hardcoded in several places (including module parameter
> description and warning message for invalid module parameter value).
> 
> This patch adds macros for the aforementioned values and replaces all
> occurences of hardcoded values by these macros.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 0b4489661a82..3f7f66513dfa 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -81,12 +81,16 @@ static int cards_found;
>  
>  /* module parameters */
>  /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
> -#define WATCHDOG_HEARTBEAT 30
> +#define ESB_HEARTBEAT_MIN 1
> +#define ESB_HEARTBEAT_MAX 2046
> +#define ESB_HEARTBEAT_DEFAULT 30

Please use tabs above.

#define ESB_HEARTBEAT_MIN	1
#define ESB_HEARTBEAT_MAX	2046
#define ESB_HEARTBEAT_DEFAULT	30

> +#define ESB_HEARTBEAT_RANGE __MODULE_STRING(ESB_HEARTBEAT_MIN) \
> +	"<heartbeat<" __MODULE_STRING(ESB_HEARTBEAT_MAX)
>  static int heartbeat; /* in seconds */
>  module_param(heartbeat, int, 0);
>  MODULE_PARM_DESC(heartbeat,
> -		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
> -				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
> +	"Watchdog heartbeat in seconds. (" ESB_HEARTBEAT_RANGE
> +	", default=" __MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0);
> @@ -326,13 +330,13 @@ static int esb_probe(struct pci_dev *pdev,
>  	/* Initialize the watchdog and make sure it does not run */
>  	edev->wdd.info = &esb_info;
>  	edev->wdd.ops = &esb_ops;
> -	edev->wdd.min_timeout = 1;
> -	edev->wdd.max_timeout = 2 * 0x03ff;
> -	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
> +	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
> +	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
> +	edev->wdd.timeout = ESB_HEARTBEAT_DEFAULT;
>  	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
>  		dev_info(&pdev->dev,
> -			"heartbeat value must be 1<heartbeat<2046, using %u\n",
> -			edev->wdd.timeout);
> +			"heartbeat value must be " ESB_HEARTBEAT_RANGE
> +			", using %u\n", edev->wdd.timeout);
>  	watchdog_set_nowayout(&edev->wdd, nowayout);
>  	watchdog_stop_on_reboot(&edev->wdd);
>  	watchdog_stop_on_unregister(&edev->wdd);
> -- 
> 2.13.6
> 

Re: [PATCH v2 4/4] watchdog: i6300esb: bump version to 0.6

[Guenter Roeck]

On Wed, Oct 25, 2017 at 04:39:14PM +0100, Radu Rendec wrote:
> The new version incorporates two major changes:
> * Watchdog subsystem support - allows the driver to coexist with other
>   watchdog drivers; simplifies the code base
> * Multiple device support - supports multiple 6300ESB timer devices and
>   and exposes them as separate watchdogs
> 

Can you drop the version number instead ? It doesn't really serve a
useful purpose and ran out of favor upstream.

Thanks,
Guenter

> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 3f7f66513dfa..fba81fa2f1c1 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -45,7 +45,7 @@
>  #include <linux/io.h>
>  
>  /* Module and version information */
> -#define ESB_VERSION "0.05"
> +#define ESB_VERSION "0.06"
>  #define ESB_MODULE_NAME "i6300ESB timer"
>  
>  /* PCI configuration registers */
> -- 
> 2.13.6
> 

Re: [PATCH v2 2/4] watchdog: i6300esb: support multiple devices

[Radu Rendec]

On Wed, 2017-10-25 at 13:55 -0700, Guenter Roeck wrote:
> On Wed, Oct 25, 2017 at 04:39:12PM +0100, Radu Rendec wrote:
> > -/* We can only use 1 card due to the /dev/watchdog restriction */
> > +/* Probed devices counter; used only for printing the initial info message */
> >  static int cards_found;
> 
> Now unnecessary.

[snip]

> >  static int esb_probe(struct pci_dev *pdev,
> >  		const struct pci_device_id *ent)
> >  {
> > +	struct esb_dev *edev;
> >  	int ret;
> >  
> >  	cards_found++;
> 
> Now unnecessary.

This variable is still used for printing the module "greeting" message
only when the first device is probed. The code snippet is below, but
unfortunately the diff context doesn't include the condition that tests
cards_found (the line was originally there, I didn't touch it).

> > @@ -309,26 +313,33 @@ static int esb_probe(struct pci_dev *pdev,
> >  		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
> >  			ESB_VERSION);
> >  

So we should either keep the variable or drop the "greeting" message
altogether. Perhaps it makes more sense if we use it as a bool (rather
than a counter)? Something like:

if (!cards_found) {
	pr_info(...);
	cards_found = 1;
}

I fixed the other issues and I will submit a new version of the patch
series as soon as I get your input on this one. 

Thanks,
Radu

Re: [PATCH v2 2/4] watchdog: i6300esb: support multiple devices

[Guenter Roeck]

On 10/26/2017 04:35 AM, Radu Rendec wrote:
> On Wed, 2017-10-25 at 13:55 -0700, Guenter Roeck wrote:
>> On Wed, Oct 25, 2017 at 04:39:12PM +0100, Radu Rendec wrote:
>>> -/* We can only use 1 card due to the /dev/watchdog restriction */
>>> +/* Probed devices counter; used only for printing the initial info message */
>>>   static int cards_found;
>>
>> Now unnecessary.
> 
> [snip]
> 
>>>   static int esb_probe(struct pci_dev *pdev,
>>>   		const struct pci_device_id *ent)
>>>   {
>>> +	struct esb_dev *edev;
>>>   	int ret;
>>>   
>>>   	cards_found++;
>>
>> Now unnecessary.
> 
> This variable is still used for printing the module "greeting" message
> only when the first device is probed. The code snippet is below, but
> unfortunately the diff context doesn't include the condition that tests
> cards_found (the line was originally there, I didn't touch it).
> 
>>> @@ -309,26 +313,33 @@ static int esb_probe(struct pci_dev *pdev,
>>>   		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
>>>   			ESB_VERSION);
>>>   
> 
> So we should either keep the variable or drop the "greeting" message
> altogether. Perhaps it makes more sense if we use it as a bool (rather
> than a counter)? Something like:
> 
> if (!cards_found) {
> 	pr_info(...);
> 	cards_found = 1;
> }
> 

Just drop that pr_info. There is an info message at the end of the probe
function which is sufficient. Make that dev_info if you didn't already,
and it will report additional instance data automatically.

> I fixed the other issues and I will submit a new version of the patch
> series as soon as I get your input on this one.
> 
> Thanks,
> Radu
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v3 1/4] watchdog: i6300esb: use the watchdog subsystem

[Radu Rendec]

Change the i6300esb driver to use the watchdog subsystem instead of the
legacy watchdog API. This is mainly just getting rid of the "write" and
"ioctl" methods and part of the watchdog control logic (which are all
implemented by the watchdog subsystem).

Even though the watchdog subsystem supports registering and handling
multiple watchdog devices at the same time, the i6300esb driver still
has a limitation of only one i6300esb device due to some global variable
usage that comes from the original design. However, the driver can now
coexist with other watchdog devices (supported by different drivers).

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 232 +++++++++-----------------------------------
 1 file changed, 47 insertions(+), 185 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index d7befd58b391..4f4287d83c21 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -21,6 +21,8 @@
  *	Version 0.02
  *  20050210 David Härdeman <david@2gen.com>
  *	Ported driver to kernel 2.6
+ *  20171016 Radu Rendec <rrendec@arista.com>
+ *	Change driver to use the watchdog subsystem
  */
 
 /*
@@ -44,7 +46,6 @@
 /* Module and version information */
 #define ESB_VERSION "0.05"
 #define ESB_MODULE_NAME "i6300ESB timer"
-#define ESB_DRIVER_NAME ESB_MODULE_NAME ", v" ESB_VERSION
 
 /* PCI configuration registers */
 #define ESB_CONFIG_REG  0x60            /* Config register                   */
@@ -76,11 +77,7 @@
 
 /* internal variables */
 static void __iomem *BASEADDR;
-static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
-static unsigned long timer_alive;
 static struct pci_dev *esb_pci;
-static unsigned short triggered; /* The status of the watchdog upon boot */
-static char esb_expect_close;
 
 /* We can only use 1 card due to the /dev/watchdog restriction */
 static int cards_found;
@@ -88,7 +85,7 @@ static int cards_found;
 /* module parameters */
 /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
 #define WATCHDOG_HEARTBEAT 30
-static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
+static int heartbeat; /* in seconds */
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat,
 		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
@@ -116,55 +113,47 @@ static inline void esb_unlock_registers(void)
 	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
 }
 
-static int esb_timer_start(void)
+static int esb_timer_start(struct watchdog_device *wdd)
 {
+	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
 	u8 val;
 
-	spin_lock(&esb_lock);
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* Enable or Enable + Lock? */
-	val = ESB_WDT_ENABLE | (nowayout ? ESB_WDT_LOCK : 0x00);
+	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
 	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
-	spin_unlock(&esb_lock);
 	return 0;
 }
 
-static int esb_timer_stop(void)
+static int esb_timer_stop(struct watchdog_device *wdd)
 {
 	u8 val;
 
-	spin_lock(&esb_lock);
 	/* First, reset timers as suggested by the docs */
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* Then disable the WDT */
 	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
 	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
-	spin_unlock(&esb_lock);
 
 	/* Returns 0 if the timer was disabled, non-zero otherwise */
 	return val & ESB_WDT_ENABLE;
 }
 
-static void esb_timer_keepalive(void)
+static int esb_timer_keepalive(struct watchdog_device *wdd)
 {
-	spin_lock(&esb_lock);
 	esb_unlock_registers();
 	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
 	/* FIXME: Do we need to flush anything here? */
-	spin_unlock(&esb_lock);
+	return 0;
 }
 
-static int esb_timer_set_heartbeat(int time)
+static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
+		unsigned int time)
 {
 	u32 val;
 
-	if (time < 0x1 || time > (2 * 0x03ff))
-		return -EINVAL;
-
-	spin_lock(&esb_lock);
-
 	/* We shift by 9, so if we are passed a value of 1 sec,
 	 * val will be 1 << 9 = 512, then write that to two
 	 * timers => 2 * 512 = 1024 (which is decremented at 1KHz)
@@ -186,148 +175,33 @@ static int esb_timer_set_heartbeat(int time)
 	/* FIXME: Do we need to flush everything out? */
 
 	/* Done */
-	heartbeat = time;
-	spin_unlock(&esb_lock);
+	wdd->timeout = time;
 	return 0;
 }
 
 /*
- *	/dev/watchdog handling
+ * Watchdog Subsystem Interfaces
  */
 
-static int esb_open(struct inode *inode, struct file *file)
-{
-	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &timer_alive))
-		return -EBUSY;
-
-	/* Reload and activate timer */
-	esb_timer_start();
-
-	return nonseekable_open(inode, file);
-}
-
-static int esb_release(struct inode *inode, struct file *file)
-{
-	/* Shut off the timer. */
-	if (esb_expect_close == 42)
-		esb_timer_stop();
-	else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		esb_timer_keepalive();
-	}
-	clear_bit(0, &timer_alive);
-	esb_expect_close = 0;
-	return 0;
-}
-
-static ssize_t esb_write(struct file *file, const char __user *data,
-			  size_t len, loff_t *ppos)
-{
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* note: just in case someone wrote the magic character
-			 * five months ago... */
-			esb_expect_close = 0;
-
-			/* scan to see whether or not we got the
-			 * magic character */
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					esb_expect_close = 42;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		esb_timer_keepalive();
-	}
-	return len;
-}
-
-static long esb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int new_options, retval = -EINVAL;
-	int new_heartbeat;
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	static const struct watchdog_info ident = {
-		.options =		WDIOF_SETTIMEOUT |
-					WDIOF_KEEPALIVEPING |
-					WDIOF_MAGICCLOSE,
-		.firmware_version =	0,
-		.identity =		ESB_MODULE_NAME,
-	};
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &ident,
-					sizeof(ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-		return put_user(0, p);
-
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(triggered, p);
-
-	case WDIOC_SETOPTIONS:
-	{
-		if (get_user(new_options, p))
-			return -EFAULT;
-
-		if (new_options & WDIOS_DISABLECARD) {
-			esb_timer_stop();
-			retval = 0;
-		}
-
-		if (new_options & WDIOS_ENABLECARD) {
-			esb_timer_start();
-			retval = 0;
-		}
-		return retval;
-	}
-	case WDIOC_KEEPALIVE:
-		esb_timer_keepalive();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-	{
-		if (get_user(new_heartbeat, p))
-			return -EFAULT;
-		if (esb_timer_set_heartbeat(new_heartbeat))
-			return -EINVAL;
-		esb_timer_keepalive();
-		/* Fall */
-	}
-	case WDIOC_GETTIMEOUT:
-		return put_user(heartbeat, p);
-	default:
-		return -ENOTTY;
-	}
-}
-
-/*
- *      Kernel Interfaces
- */
+static struct watchdog_info esb_info = {
+	.identity = ESB_MODULE_NAME,
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
 
-static const struct file_operations esb_fops = {
+static const struct watchdog_ops esb_ops = {
 	.owner = THIS_MODULE,
-	.llseek = no_llseek,
-	.write = esb_write,
-	.unlocked_ioctl = esb_ioctl,
-	.open = esb_open,
-	.release = esb_release,
+	.start = esb_timer_start,
+	.stop = esb_timer_stop,
+	.set_timeout = esb_timer_set_heartbeat,
+	.ping = esb_timer_keepalive,
 };
 
-static struct miscdevice esb_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &esb_fops,
+static struct watchdog_device esb_dev = {
+	.info = &esb_info,
+	.ops = &esb_ops,
+	.min_timeout = 1,
+	.max_timeout = 2 * 0x03ff,
+	.timeout = WATCHDOG_HEARTBEAT,
 };
 
 /*
@@ -346,19 +220,19 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
 static unsigned char esb_getdevice(struct pci_dev *pdev)
 {
 	if (pci_enable_device(pdev)) {
-		pr_err("failed to enable device\n");
+		dev_err(&pdev->dev, "failed to enable device\n");
 		goto err_devput;
 	}
 
 	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
-		pr_err("failed to request region\n");
+		dev_err(&pdev->dev, "failed to request region\n");
 		goto err_disable;
 	}
 
 	BASEADDR = pci_ioremap_bar(pdev, 0);
 	if (BASEADDR == NULL) {
 		/* Something's wrong here, BASEADDR has to be set */
-		pr_err("failed to get BASEADDR\n");
+		dev_err(&pdev->dev, "failed to get BASEADDR\n");
 		goto err_release;
 	}
 
@@ -396,7 +270,7 @@ static void esb_initdevice(void)
 	/* Check that the WDT isn't already locked */
 	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
 	if (val1 & ESB_WDT_LOCK)
-		pr_warn("nowayout already set\n");
+		dev_warn(&esb_pci->dev, "nowayout already set\n");
 
 	/* Set the timer to watchdog mode and disable it for now */
 	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
@@ -405,14 +279,14 @@ static void esb_initdevice(void)
 	esb_unlock_registers();
 	val2 = readw(ESB_RELOAD_REG);
 	if (val2 & ESB_WDT_TIMEOUT)
-		triggered = WDIOF_CARDRESET;
+		esb_dev.bootstatus = WDIOF_CARDRESET;
 
 	/* Reset WDT_TIMEOUT flag and timers */
 	esb_unlock_registers();
 	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
 
 	/* And set the correct timeout value */
-	esb_timer_set_heartbeat(heartbeat);
+	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
 }
 
 static int esb_probe(struct pci_dev *pdev,
@@ -434,26 +308,25 @@ static int esb_probe(struct pci_dev *pdev,
 	if (!esb_getdevice(pdev) || esb_pci == NULL)
 		return -ENODEV;
 
-	/* Check that the heartbeat value is within it's range;
-	   if not reset to the default */
-	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
-		heartbeat = WATCHDOG_HEARTBEAT;
-		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
-			heartbeat);
-	}
-
 	/* Initialize the watchdog and make sure it does not run */
+	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
+		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
+			esb_dev.timeout);
+	watchdog_set_nowayout(&esb_dev, nowayout);
+	watchdog_stop_on_reboot(&esb_dev);
+	watchdog_stop_on_unregister(&esb_dev);
 	esb_initdevice();
 
 	/* Register the watchdog so that userspace has access to it */
-	ret = misc_register(&esb_miscdev);
+	ret = watchdog_register_device(&esb_dev);
 	if (ret != 0) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       WATCHDOG_MINOR, ret);
+		dev_err(&pdev->dev,
+			"cannot register watchdog device (err=%d)\n", ret);
 		goto err_unmap;
 	}
-	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
-		BASEADDR, heartbeat, nowayout);
+	dev_info(&pdev->dev,
+		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
+		BASEADDR, esb_dev.timeout, nowayout);
 	return 0;
 
 err_unmap:
@@ -466,29 +339,18 @@ static int esb_probe(struct pci_dev *pdev,
 
 static void esb_remove(struct pci_dev *pdev)
 {
-	/* Stop the timer before we leave */
-	if (!nowayout)
-		esb_timer_stop();
-
-	/* Deregister */
-	misc_deregister(&esb_miscdev);
+	watchdog_unregister_device(&esb_dev);
 	iounmap(BASEADDR);
 	pci_release_region(esb_pci, 0);
 	pci_disable_device(esb_pci);
 	esb_pci = NULL;
 }
 
-static void esb_shutdown(struct pci_dev *pdev)
-{
-	esb_timer_stop();
-}
-
 static struct pci_driver esb_driver = {
 	.name		= ESB_MODULE_NAME,
 	.id_table	= esb_pci_tbl,
 	.probe          = esb_probe,
 	.remove         = esb_remove,
-	.shutdown       = esb_shutdown,
 };
 
 module_pci_driver(esb_driver);
-- 
2.13.6

[PATCH v3 2/4] watchdog: i6300esb: support multiple devices

[Radu Rendec]

Support multiple i6300esb devices simultaneously, by removing the single
device restriction in the original design and leveraging the multiple
device support of the watchdog subsystem.

This patch replaces the global definitions of watchdog device data with
a dynamically allocated structure. This structure is allocated during
device probe, so multiple independent structures can be allocated if
multiple devices are probed.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 170 +++++++++++++++++++++++---------------------
 1 file changed, 90 insertions(+), 80 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 4f4287d83c21..147462f431de 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -23,6 +23,7 @@
  *	Ported driver to kernel 2.6
  *  20171016 Radu Rendec <rrendec@arista.com>
  *	Change driver to use the watchdog subsystem
+ *	Add support for multiple 6300ESB devices
  */
 
 /*
@@ -52,10 +53,10 @@
 #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
 
 /* Memory mapped registers */
-#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
-#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
-#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
-#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
+#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
+#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
+#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
+#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
 
 /* Lock register bits */
 #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
@@ -75,11 +76,7 @@
 #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
 #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
 
-/* internal variables */
-static void __iomem *BASEADDR;
-static struct pci_dev *esb_pci;
-
-/* We can only use 1 card due to the /dev/watchdog restriction */
+/* Probed devices counter; used only for printing the initial info message */
 static int cards_found;
 
 /* module parameters */
@@ -97,6 +94,15 @@ MODULE_PARM_DESC(nowayout,
 		"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+/* internal variables */
+struct esb_dev {
+	struct watchdog_device wdd;
+	void __iomem *base;
+	struct pci_dev *pdev;
+};
+
+#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
+
 /*
  * Some i6300ESB specific functions
  */
@@ -107,35 +113,37 @@ MODULE_PARM_DESC(nowayout,
  * reload register. After this the appropriate registers can be written
  * to once before they need to be unlocked again.
  */
-static inline void esb_unlock_registers(void)
+static inline void esb_unlock_registers(struct esb_dev *edev)
 {
-	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
-	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
+	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
+	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
 }
 
 static int esb_timer_start(struct watchdog_device *wdd)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
 	u8 val;
 
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* Enable or Enable + Lock? */
 	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
 	return 0;
 }
 
 static int esb_timer_stop(struct watchdog_device *wdd)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	u8 val;
 
 	/* First, reset timers as suggested by the docs */
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* Then disable the WDT */
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
-	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
+	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
 
 	/* Returns 0 if the timer was disabled, non-zero otherwise */
 	return val & ESB_WDT_ENABLE;
@@ -143,8 +151,10 @@ static int esb_timer_stop(struct watchdog_device *wdd)
 
 static int esb_timer_keepalive(struct watchdog_device *wdd)
 {
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	struct esb_dev *edev = to_esb_dev(wdd);
+
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 	/* FIXME: Do we need to flush anything here? */
 	return 0;
 }
@@ -152,6 +162,7 @@ static int esb_timer_keepalive(struct watchdog_device *wdd)
 static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 		unsigned int time)
 {
+	struct esb_dev *edev = to_esb_dev(wdd);
 	u32 val;
 
 	/* We shift by 9, so if we are passed a value of 1 sec,
@@ -161,16 +172,16 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
 	val = time << 9;
 
 	/* Write timer 1 */
-	esb_unlock_registers();
-	writel(val, ESB_TIMER1_REG);
+	esb_unlock_registers(edev);
+	writel(val, ESB_TIMER1_REG(edev));
 
 	/* Write timer 2 */
-	esb_unlock_registers();
-	writel(val, ESB_TIMER2_REG);
+	esb_unlock_registers(edev);
+	writel(val, ESB_TIMER2_REG(edev));
 
 	/* Reload */
-	esb_unlock_registers();
-	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
 
 	/* FIXME: Do we need to flush everything out? */
 
@@ -196,14 +207,6 @@ static const struct watchdog_ops esb_ops = {
 	.ping = esb_timer_keepalive,
 };
 
-static struct watchdog_device esb_dev = {
-	.info = &esb_info,
-	.ops = &esb_ops,
-	.min_timeout = 1,
-	.max_timeout = 2 * 0x03ff,
-	.timeout = WATCHDOG_HEARTBEAT,
-};
-
 /*
  * Data for PCI driver interface
  */
@@ -217,38 +220,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
  *      Init & exit routines
  */
 
-static unsigned char esb_getdevice(struct pci_dev *pdev)
+static unsigned char esb_getdevice(struct esb_dev *edev)
 {
-	if (pci_enable_device(pdev)) {
-		dev_err(&pdev->dev, "failed to enable device\n");
+	if (pci_enable_device(edev->pdev)) {
+		dev_err(&edev->pdev->dev, "failed to enable device\n");
 		goto err_devput;
 	}
 
-	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
-		dev_err(&pdev->dev, "failed to request region\n");
+	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
+		dev_err(&edev->pdev->dev, "failed to request region\n");
 		goto err_disable;
 	}
 
-	BASEADDR = pci_ioremap_bar(pdev, 0);
-	if (BASEADDR == NULL) {
+	edev->base = pci_ioremap_bar(edev->pdev, 0);
+	if (edev->base == NULL) {
 		/* Something's wrong here, BASEADDR has to be set */
-		dev_err(&pdev->dev, "failed to get BASEADDR\n");
+		dev_err(&edev->pdev->dev, "failed to get BASEADDR\n");
 		goto err_release;
 	}
 
 	/* Done */
-	esb_pci = pdev;
+	dev_set_drvdata(&edev->pdev->dev, edev);
 	return 1;
 
 err_release:
-	pci_release_region(pdev, 0);
+	pci_release_region(edev->pdev, 0);
 err_disable:
-	pci_disable_device(pdev);
+	pci_disable_device(edev->pdev);
 err_devput:
 	return 0;
 }
 
-static void esb_initdevice(void)
+static void esb_initdevice(struct esb_dev *edev)
 {
 	u8 val1;
 	u16 val2;
@@ -265,33 +268,34 @@ static void esb_initdevice(void)
 	 * any interrupts as there is not much we can do with it
 	 * right now.
 	 */
-	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
+	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
 
 	/* Check that the WDT isn't already locked */
-	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
+	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
 	if (val1 & ESB_WDT_LOCK)
-		dev_warn(&esb_pci->dev, "nowayout already set\n");
+		dev_warn(&edev->pdev->dev, "nowayout already set\n");
 
 	/* Set the timer to watchdog mode and disable it for now */
-	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
+	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
 
 	/* Check if the watchdog was previously triggered */
-	esb_unlock_registers();
-	val2 = readw(ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	val2 = readw(ESB_RELOAD_REG(edev));
 	if (val2 & ESB_WDT_TIMEOUT)
-		esb_dev.bootstatus = WDIOF_CARDRESET;
+		edev->wdd.bootstatus = WDIOF_CARDRESET;
 
 	/* Reset WDT_TIMEOUT flag and timers */
-	esb_unlock_registers();
-	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
+	esb_unlock_registers(edev);
+	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
 
 	/* And set the correct timeout value */
-	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
+	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
 }
 
 static int esb_probe(struct pci_dev *pdev,
 		const struct pci_device_id *ent)
 {
+	struct esb_dev *edev;
 	int ret;
 
 	cards_found++;
@@ -299,26 +303,32 @@ static int esb_probe(struct pci_dev *pdev,
 		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
 			ESB_VERSION);
 
-	if (cards_found > 1) {
-		pr_err("This driver only supports 1 device\n");
-		return -ENODEV;
-	}
+	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
+	if (!edev)
+		return -ENOMEM;
 
 	/* Check whether or not the hardware watchdog is there */
-	if (!esb_getdevice(pdev) || esb_pci == NULL)
+	edev->pdev = pdev;
+	if (!esb_getdevice(edev))
 		return -ENODEV;
 
 	/* Initialize the watchdog and make sure it does not run */
-	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
-		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
-			esb_dev.timeout);
-	watchdog_set_nowayout(&esb_dev, nowayout);
-	watchdog_stop_on_reboot(&esb_dev);
-	watchdog_stop_on_unregister(&esb_dev);
-	esb_initdevice();
+	edev->wdd.info = &esb_info;
+	edev->wdd.ops = &esb_ops;
+	edev->wdd.min_timeout = 1;
+	edev->wdd.max_timeout = 2 * 0x03ff;
+	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
+	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
+		dev_info(&pdev->dev,
+			"heartbeat value must be 1<heartbeat<2046, using %u\n",
+			edev->wdd.timeout);
+	watchdog_set_nowayout(&edev->wdd, nowayout);
+	watchdog_stop_on_reboot(&edev->wdd);
+	watchdog_stop_on_unregister(&edev->wdd);
+	esb_initdevice(edev);
 
 	/* Register the watchdog so that userspace has access to it */
-	ret = watchdog_register_device(&esb_dev);
+	ret = watchdog_register_device(&edev->wdd);
 	if (ret != 0) {
 		dev_err(&pdev->dev,
 			"cannot register watchdog device (err=%d)\n", ret);
@@ -326,24 +336,24 @@ static int esb_probe(struct pci_dev *pdev,
 	}
 	dev_info(&pdev->dev,
 		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
-		BASEADDR, esb_dev.timeout, nowayout);
+		edev->base, edev->wdd.timeout, nowayout);
 	return 0;
 
 err_unmap:
-	iounmap(BASEADDR);
-	pci_release_region(esb_pci, 0);
-	pci_disable_device(esb_pci);
-	esb_pci = NULL;
+	iounmap(edev->base);
+	pci_release_region(edev->pdev, 0);
+	pci_disable_device(edev->pdev);
 	return ret;
 }
 
 static void esb_remove(struct pci_dev *pdev)
 {
-	watchdog_unregister_device(&esb_dev);
-	iounmap(BASEADDR);
-	pci_release_region(esb_pci, 0);
-	pci_disable_device(esb_pci);
-	esb_pci = NULL;
+	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
+
+	watchdog_unregister_device(&edev->wdd);
+	iounmap(edev->base);
+	pci_release_region(edev->pdev, 0);
+	pci_disable_device(edev->pdev);
 }
 
 static struct pci_driver esb_driver = {
-- 
2.13.6

[PATCH v3 3/4] watchdog: i6300esb: do not hardcode heartbeat limits

[Radu Rendec]

The minimum, maximum and default values for the watchdog heartbeat
(timeout) were hardcoded in several places (including module parameter
description and warning message for invalid module parameter value).

This patch adds macros for the aforementioned values and replaces all
occurences of hardcoded values by these macros.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 147462f431de..4d92925cdc1e 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -81,12 +81,16 @@ static int cards_found;
 
 /* module parameters */
 /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
-#define WATCHDOG_HEARTBEAT 30
+#define ESB_HEARTBEAT_MIN	1
+#define ESB_HEARTBEAT_MAX	2046
+#define ESB_HEARTBEAT_DEFAULT	30
+#define ESB_HEARTBEAT_RANGE __MODULE_STRING(ESB_HEARTBEAT_MIN) \
+	"<heartbeat<" __MODULE_STRING(ESB_HEARTBEAT_MAX)
 static int heartbeat; /* in seconds */
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat,
-		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
-				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
+	"Watchdog heartbeat in seconds. (" ESB_HEARTBEAT_RANGE
+	", default=" __MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
@@ -315,13 +319,13 @@ static int esb_probe(struct pci_dev *pdev,
 	/* Initialize the watchdog and make sure it does not run */
 	edev->wdd.info = &esb_info;
 	edev->wdd.ops = &esb_ops;
-	edev->wdd.min_timeout = 1;
-	edev->wdd.max_timeout = 2 * 0x03ff;
-	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
+	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
+	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
+	edev->wdd.timeout = ESB_HEARTBEAT_DEFAULT;
 	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
 		dev_info(&pdev->dev,
-			"heartbeat value must be 1<heartbeat<2046, using %u\n",
-			edev->wdd.timeout);
+			"heartbeat value must be " ESB_HEARTBEAT_RANGE
+			", using %u\n", edev->wdd.timeout);
 	watchdog_set_nowayout(&edev->wdd, nowayout);
 	watchdog_stop_on_reboot(&edev->wdd);
 	watchdog_stop_on_unregister(&edev->wdd);
-- 
2.13.6

[PATCH v3 4/4] watchdog: i6300esb: remove info message and version number

[Radu Rendec]

The initial info message (early in the esb_probe() function) is not very
useful and we already have a message on successful probe, which includes
device identification. If the probe fails (e.g. PCI related errors),
additional messages are printed anyway.

The version number was only used in the initial info message. Other than
that, it serves no useful purpose and it ran out of favor upstream
anyway.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/i6300esb.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 4d92925cdc1e..950c71a8bb22 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -30,8 +30,6 @@
  *      Includes, defines, variables, module parameters, ...
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -45,7 +43,6 @@
 #include <linux/io.h>
 
 /* Module and version information */
-#define ESB_VERSION "0.05"
 #define ESB_MODULE_NAME "i6300ESB timer"
 
 /* PCI configuration registers */
@@ -76,9 +73,6 @@
 #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
 #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
 
-/* Probed devices counter; used only for printing the initial info message */
-static int cards_found;
-
 /* module parameters */
 /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
 #define ESB_HEARTBEAT_MIN	1
@@ -302,11 +296,6 @@ static int esb_probe(struct pci_dev *pdev,
 	struct esb_dev *edev;
 	int ret;
 
-	cards_found++;
-	if (cards_found == 1)
-		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
-			ESB_VERSION);
-
 	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
 	if (!edev)
 		return -ENOMEM;
-- 
2.13.6

Re: [PATCH v3 2/4] watchdog: i6300esb: support multiple devices

[Guenter Roeck]

On 10/26/2017 09:10 AM, Radu Rendec wrote:
> Support multiple i6300esb devices simultaneously, by removing the single
> device restriction in the original design and leveraging the multiple
> device support of the watchdog subsystem.
> 
> This patch replaces the global definitions of watchdog device data with
> a dynamically allocated structure. This structure is allocated during
> device probe, so multiple independent structures can be allocated if
> multiple devices are probed.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>   drivers/watchdog/i6300esb.c | 170 +++++++++++++++++++++++---------------------
>   1 file changed, 90 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 4f4287d83c21..147462f431de 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -23,6 +23,7 @@
>    *	Ported driver to kernel 2.6
>    *  20171016 Radu Rendec <rrendec@arista.com>
>    *	Change driver to use the watchdog subsystem
> + *	Add support for multiple 6300ESB devices
>    */
>   
>   /*
> @@ -52,10 +53,10 @@
>   #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
>   
>   /* Memory mapped registers */
> -#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
> -#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
> -#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
> -#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
> +#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
> +#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
> +#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
> +#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
>   
>   /* Lock register bits */
>   #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
> @@ -75,11 +76,7 @@
>   #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
>   #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
>   
> -/* internal variables */
> -static void __iomem *BASEADDR;
> -static struct pci_dev *esb_pci;
> -
> -/* We can only use 1 card due to the /dev/watchdog restriction */
> +/* Probed devices counter; used only for printing the initial info message */
>   static int cards_found;
>   
Leftover ?

>   /* module parameters */
> @@ -97,6 +94,15 @@ MODULE_PARM_DESC(nowayout,
>   		"Watchdog cannot be stopped once started (default="
>   				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
> +/* internal variables */
> +struct esb_dev {
> +	struct watchdog_device wdd;
> +	void __iomem *base;
> +	struct pci_dev *pdev;
> +};
> +
> +#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
> +
>   /*
>    * Some i6300ESB specific functions
>    */
> @@ -107,35 +113,37 @@ MODULE_PARM_DESC(nowayout,
>    * reload register. After this the appropriate registers can be written
>    * to once before they need to be unlocked again.
>    */
> -static inline void esb_unlock_registers(void)
> +static inline void esb_unlock_registers(struct esb_dev *edev)
>   {
> -	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
> -	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
> +	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
> +	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
>   }
>   
>   static int esb_timer_start(struct watchdog_device *wdd)
>   {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>   	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>   	u8 val;
>   
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   	/* Enable or Enable + Lock? */
>   	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
>   	return 0;
>   }
>   
>   static int esb_timer_stop(struct watchdog_device *wdd)
>   {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>   	u8 val;
>   
>   	/* First, reset timers as suggested by the docs */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   	/* Then disable the WDT */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
>   
>   	/* Returns 0 if the timer was disabled, non-zero otherwise */
>   	return val & ESB_WDT_ENABLE;
> @@ -143,8 +151,10 @@ static int esb_timer_stop(struct watchdog_device *wdd)
>   
>   static int esb_timer_keepalive(struct watchdog_device *wdd)
>   {
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	struct esb_dev *edev = to_esb_dev(wdd);
> +
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   	/* FIXME: Do we need to flush anything here? */
>   	return 0;
>   }
> @@ -152,6 +162,7 @@ static int esb_timer_keepalive(struct watchdog_device *wdd)
>   static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>   		unsigned int time)
>   {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>   	u32 val;
>   
>   	/* We shift by 9, so if we are passed a value of 1 sec,
> @@ -161,16 +172,16 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>   	val = time << 9;
>   
>   	/* Write timer 1 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER1_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER1_REG(edev));
>   
>   	/* Write timer 2 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER2_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER2_REG(edev));
>   
>   	/* Reload */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   
>   	/* FIXME: Do we need to flush everything out? */
>   
> @@ -196,14 +207,6 @@ static const struct watchdog_ops esb_ops = {
>   	.ping = esb_timer_keepalive,
>   };
>   
> -static struct watchdog_device esb_dev = {
> -	.info = &esb_info,
> -	.ops = &esb_ops,
> -	.min_timeout = 1,
> -	.max_timeout = 2 * 0x03ff,
> -	.timeout = WATCHDOG_HEARTBEAT,
> -};
> -
>   /*
>    * Data for PCI driver interface
>    */
> @@ -217,38 +220,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
>    *      Init & exit routines
>    */
>   
> -static unsigned char esb_getdevice(struct pci_dev *pdev)
> +static unsigned char esb_getdevice(struct esb_dev *edev)
>   {
> -	if (pci_enable_device(pdev)) {
> -		dev_err(&pdev->dev, "failed to enable device\n");
> +	if (pci_enable_device(edev->pdev)) {
> +		dev_err(&edev->pdev->dev, "failed to enable device\n");
>   		goto err_devput;
>   	}
>   
> -	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> -		dev_err(&pdev->dev, "failed to request region\n");
> +	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
> +		dev_err(&edev->pdev->dev, "failed to request region\n");
>   		goto err_disable;
>   	}
>   
> -	BASEADDR = pci_ioremap_bar(pdev, 0);
> -	if (BASEADDR == NULL) {
> +	edev->base = pci_ioremap_bar(edev->pdev, 0);
> +	if (edev->base == NULL) {
>   		/* Something's wrong here, BASEADDR has to be set */
> -		dev_err(&pdev->dev, "failed to get BASEADDR\n");
> +		dev_err(&edev->pdev->dev, "failed to get BASEADDR\n");
>   		goto err_release;
>   	}
>   
>   	/* Done */
> -	esb_pci = pdev;
> +	dev_set_drvdata(&edev->pdev->dev, edev);
>   	return 1;
>   
>   err_release:
> -	pci_release_region(pdev, 0);
> +	pci_release_region(edev->pdev, 0);
>   err_disable:
> -	pci_disable_device(pdev);
> +	pci_disable_device(edev->pdev);
>   err_devput:
>   	return 0;
>   }
>   
> -static void esb_initdevice(void)
> +static void esb_initdevice(struct esb_dev *edev)
>   {
>   	u8 val1;
>   	u16 val2;
> @@ -265,33 +268,34 @@ static void esb_initdevice(void)
>   	 * any interrupts as there is not much we can do with it
>   	 * right now.
>   	 */
> -	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
> +	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
>   
>   	/* Check that the WDT isn't already locked */
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
>   	if (val1 & ESB_WDT_LOCK)
> -		dev_warn(&esb_pci->dev, "nowayout already set\n");
> +		dev_warn(&edev->pdev->dev, "nowayout already set\n");
>   
>   	/* Set the timer to watchdog mode and disable it for now */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
>   
>   	/* Check if the watchdog was previously triggered */
> -	esb_unlock_registers();
> -	val2 = readw(ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	val2 = readw(ESB_RELOAD_REG(edev));
>   	if (val2 & ESB_WDT_TIMEOUT)
> -		esb_dev.bootstatus = WDIOF_CARDRESET;
> +		edev->wdd.bootstatus = WDIOF_CARDRESET;
>   
>   	/* Reset WDT_TIMEOUT flag and timers */
> -	esb_unlock_registers();
> -	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
>   
>   	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
> +	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
>   }
>   
>   static int esb_probe(struct pci_dev *pdev,
>   		const struct pci_device_id *ent)
>   {
> +	struct esb_dev *edev;
>   	int ret;
>   
>   	cards_found++;

Leftover ?

> @@ -299,26 +303,32 @@ static int esb_probe(struct pci_dev *pdev,
>   		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
>   			ESB_VERSION);
>   
> -	if (cards_found > 1) {
> -		pr_err("This driver only supports 1 device\n");
> -		return -ENODEV;
> -	}
> +	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
> +	if (!edev)
> +		return -ENOMEM;
>   
>   	/* Check whether or not the hardware watchdog is there */
> -	if (!esb_getdevice(pdev) || esb_pci == NULL)
> +	edev->pdev = pdev;
> +	if (!esb_getdevice(edev))
>   		return -ENODEV;
>   
>   	/* Initialize the watchdog and make sure it does not run */
> -	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
> -			esb_dev.timeout);
> -	watchdog_set_nowayout(&esb_dev, nowayout);
> -	watchdog_stop_on_reboot(&esb_dev);
> -	watchdog_stop_on_unregister(&esb_dev);
> -	esb_initdevice();
> +	edev->wdd.info = &esb_info;
> +	edev->wdd.ops = &esb_ops;
> +	edev->wdd.min_timeout = 1;
> +	edev->wdd.max_timeout = 2 * 0x03ff;
> +	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
> +	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
> +		dev_info(&pdev->dev,
> +			"heartbeat value must be 1<heartbeat<2046, using %u\n",
> +			edev->wdd.timeout);
> +	watchdog_set_nowayout(&edev->wdd, nowayout);
> +	watchdog_stop_on_reboot(&edev->wdd);
> +	watchdog_stop_on_unregister(&edev->wdd);
> +	esb_initdevice(edev);
>   
>   	/* Register the watchdog so that userspace has access to it */
> -	ret = watchdog_register_device(&esb_dev);
> +	ret = watchdog_register_device(&edev->wdd);
>   	if (ret != 0) {
>   		dev_err(&pdev->dev,
>   			"cannot register watchdog device (err=%d)\n", ret);
> @@ -326,24 +336,24 @@ static int esb_probe(struct pci_dev *pdev,
>   	}
>   	dev_info(&pdev->dev,
>   		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> -		BASEADDR, esb_dev.timeout, nowayout);
> +		edev->base, edev->wdd.timeout, nowayout);
>   	return 0;
>   
>   err_unmap:
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>   	return ret;
>   }
>   
>   static void esb_remove(struct pci_dev *pdev)
>   {
> -	watchdog_unregister_device(&esb_dev);
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> +
> +	watchdog_unregister_device(&edev->wdd);
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>   }
>   
>   static struct pci_driver esb_driver = {
> 

Re: [PATCH v3 1/4] watchdog: i6300esb: use the watchdog subsystem

[Guenter Roeck]

On 10/26/2017 09:10 AM, Radu Rendec wrote:
> Change the i6300esb driver to use the watchdog subsystem instead of the
> legacy watchdog API. This is mainly just getting rid of the "write" and
> "ioctl" methods and part of the watchdog control logic (which are all
> implemented by the watchdog subsystem).
> 
> Even though the watchdog subsystem supports registering and handling
> multiple watchdog devices at the same time, the i6300esb driver still
> has a limitation of only one i6300esb device due to some global variable
> usage that comes from the original design. However, the driver can now
> coexist with other watchdog devices (supported by different drivers).
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/i6300esb.c | 232 +++++++++-----------------------------------
>   1 file changed, 47 insertions(+), 185 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index d7befd58b391..4f4287d83c21 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -21,6 +21,8 @@
>    *	Version 0.02
>    *  20050210 David Härdeman <david@2gen.com>
>    *	Ported driver to kernel 2.6
> + *  20171016 Radu Rendec <rrendec@arista.com>
> + *	Change driver to use the watchdog subsystem
>    */
>   
>   /*
> @@ -44,7 +46,6 @@
>   /* Module and version information */
>   #define ESB_VERSION "0.05"
>   #define ESB_MODULE_NAME "i6300ESB timer"
> -#define ESB_DRIVER_NAME ESB_MODULE_NAME ", v" ESB_VERSION
>   
>   /* PCI configuration registers */
>   #define ESB_CONFIG_REG  0x60            /* Config register                   */
> @@ -76,11 +77,7 @@
>   
>   /* internal variables */
>   static void __iomem *BASEADDR;
> -static DEFINE_SPINLOCK(esb_lock); /* Guards the hardware */
> -static unsigned long timer_alive;
>   static struct pci_dev *esb_pci;
> -static unsigned short triggered; /* The status of the watchdog upon boot */
> -static char esb_expect_close;
>   
>   /* We can only use 1 card due to the /dev/watchdog restriction */
>   static int cards_found;
> @@ -88,7 +85,7 @@ static int cards_found;
>   /* module parameters */
>   /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
>   #define WATCHDOG_HEARTBEAT 30
> -static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
> +static int heartbeat; /* in seconds */
>   module_param(heartbeat, int, 0);
>   MODULE_PARM_DESC(heartbeat,
>   		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
> @@ -116,55 +113,47 @@ static inline void esb_unlock_registers(void)
>   	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
>   }
>   
> -static int esb_timer_start(void)
> +static int esb_timer_start(struct watchdog_device *wdd)
>   {
> +	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>   	u8 val;
>   
> -	spin_lock(&esb_lock);
>   	esb_unlock_registers();
>   	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>   	/* Enable or Enable + Lock? */
> -	val = ESB_WDT_ENABLE | (nowayout ? ESB_WDT_LOCK : 0x00);
> +	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
>   	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
> -	spin_unlock(&esb_lock);
>   	return 0;
>   }
>   
> -static int esb_timer_stop(void)
> +static int esb_timer_stop(struct watchdog_device *wdd)
>   {
>   	u8 val;
>   
> -	spin_lock(&esb_lock);
>   	/* First, reset timers as suggested by the docs */
>   	esb_unlock_registers();
>   	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>   	/* Then disable the WDT */
>   	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
>   	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
> -	spin_unlock(&esb_lock);
>   
>   	/* Returns 0 if the timer was disabled, non-zero otherwise */
>   	return val & ESB_WDT_ENABLE;
>   }
>   
> -static void esb_timer_keepalive(void)
> +static int esb_timer_keepalive(struct watchdog_device *wdd)
>   {
> -	spin_lock(&esb_lock);
>   	esb_unlock_registers();
>   	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
>   	/* FIXME: Do we need to flush anything here? */
> -	spin_unlock(&esb_lock);
> +	return 0;
>   }
>   
> -static int esb_timer_set_heartbeat(int time)
> +static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
> +		unsigned int time)
>   {
>   	u32 val;
>   
> -	if (time < 0x1 || time > (2 * 0x03ff))
> -		return -EINVAL;
> -
> -	spin_lock(&esb_lock);
> -
>   	/* We shift by 9, so if we are passed a value of 1 sec,
>   	 * val will be 1 << 9 = 512, then write that to two
>   	 * timers => 2 * 512 = 1024 (which is decremented at 1KHz)
> @@ -186,148 +175,33 @@ static int esb_timer_set_heartbeat(int time)
>   	/* FIXME: Do we need to flush everything out? */
>   
>   	/* Done */
> -	heartbeat = time;
> -	spin_unlock(&esb_lock);
> +	wdd->timeout = time;
>   	return 0;
>   }
>   
>   /*
> - *	/dev/watchdog handling
> + * Watchdog Subsystem Interfaces
>    */
>   
> -static int esb_open(struct inode *inode, struct file *file)
> -{
> -	/* /dev/watchdog can only be opened once */
> -	if (test_and_set_bit(0, &timer_alive))
> -		return -EBUSY;
> -
> -	/* Reload and activate timer */
> -	esb_timer_start();
> -
> -	return nonseekable_open(inode, file);
> -}
> -
> -static int esb_release(struct inode *inode, struct file *file)
> -{
> -	/* Shut off the timer. */
> -	if (esb_expect_close == 42)
> -		esb_timer_stop();
> -	else {
> -		pr_crit("Unexpected close, not stopping watchdog!\n");
> -		esb_timer_keepalive();
> -	}
> -	clear_bit(0, &timer_alive);
> -	esb_expect_close = 0;
> -	return 0;
> -}
> -
> -static ssize_t esb_write(struct file *file, const char __user *data,
> -			  size_t len, loff_t *ppos)
> -{
> -	/* See if we got the magic character 'V' and reload the timer */
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			/* note: just in case someone wrote the magic character
> -			 * five months ago... */
> -			esb_expect_close = 0;
> -
> -			/* scan to see whether or not we got the
> -			 * magic character */
> -			for (i = 0; i != len; i++) {
> -				char c;
> -				if (get_user(c, data + i))
> -					return -EFAULT;
> -				if (c == 'V')
> -					esb_expect_close = 42;
> -			}
> -		}
> -
> -		/* someone wrote to us, we should reload the timer */
> -		esb_timer_keepalive();
> -	}
> -	return len;
> -}
> -
> -static long esb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> -{
> -	int new_options, retval = -EINVAL;
> -	int new_heartbeat;
> -	void __user *argp = (void __user *)arg;
> -	int __user *p = argp;
> -	static const struct watchdog_info ident = {
> -		.options =		WDIOF_SETTIMEOUT |
> -					WDIOF_KEEPALIVEPING |
> -					WDIOF_MAGICCLOSE,
> -		.firmware_version =	0,
> -		.identity =		ESB_MODULE_NAME,
> -	};
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		return copy_to_user(argp, &ident,
> -					sizeof(ident)) ? -EFAULT : 0;
> -
> -	case WDIOC_GETSTATUS:
> -		return put_user(0, p);
> -
> -	case WDIOC_GETBOOTSTATUS:
> -		return put_user(triggered, p);
> -
> -	case WDIOC_SETOPTIONS:
> -	{
> -		if (get_user(new_options, p))
> -			return -EFAULT;
> -
> -		if (new_options & WDIOS_DISABLECARD) {
> -			esb_timer_stop();
> -			retval = 0;
> -		}
> -
> -		if (new_options & WDIOS_ENABLECARD) {
> -			esb_timer_start();
> -			retval = 0;
> -		}
> -		return retval;
> -	}
> -	case WDIOC_KEEPALIVE:
> -		esb_timer_keepalive();
> -		return 0;
> -
> -	case WDIOC_SETTIMEOUT:
> -	{
> -		if (get_user(new_heartbeat, p))
> -			return -EFAULT;
> -		if (esb_timer_set_heartbeat(new_heartbeat))
> -			return -EINVAL;
> -		esb_timer_keepalive();
> -		/* Fall */
> -	}
> -	case WDIOC_GETTIMEOUT:
> -		return put_user(heartbeat, p);
> -	default:
> -		return -ENOTTY;
> -	}
> -}
> -
> -/*
> - *      Kernel Interfaces
> - */
> +static struct watchdog_info esb_info = {
> +	.identity = ESB_MODULE_NAME,
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +};
>   
> -static const struct file_operations esb_fops = {
> +static const struct watchdog_ops esb_ops = {
>   	.owner = THIS_MODULE,
> -	.llseek = no_llseek,
> -	.write = esb_write,
> -	.unlocked_ioctl = esb_ioctl,
> -	.open = esb_open,
> -	.release = esb_release,
> +	.start = esb_timer_start,
> +	.stop = esb_timer_stop,
> +	.set_timeout = esb_timer_set_heartbeat,
> +	.ping = esb_timer_keepalive,
>   };
>   
> -static struct miscdevice esb_miscdev = {
> -	.minor = WATCHDOG_MINOR,
> -	.name = "watchdog",
> -	.fops = &esb_fops,
> +static struct watchdog_device esb_dev = {
> +	.info = &esb_info,
> +	.ops = &esb_ops,
> +	.min_timeout = 1,
> +	.max_timeout = 2 * 0x03ff,
> +	.timeout = WATCHDOG_HEARTBEAT,
>   };
>   
>   /*
> @@ -346,19 +220,19 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
>   static unsigned char esb_getdevice(struct pci_dev *pdev)
>   {
>   	if (pci_enable_device(pdev)) {
> -		pr_err("failed to enable device\n");
> +		dev_err(&pdev->dev, "failed to enable device\n");
>   		goto err_devput;
>   	}
>   
>   	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> -		pr_err("failed to request region\n");
> +		dev_err(&pdev->dev, "failed to request region\n");
>   		goto err_disable;
>   	}
>   
>   	BASEADDR = pci_ioremap_bar(pdev, 0);
>   	if (BASEADDR == NULL) {
>   		/* Something's wrong here, BASEADDR has to be set */
> -		pr_err("failed to get BASEADDR\n");
> +		dev_err(&pdev->dev, "failed to get BASEADDR\n");
>   		goto err_release;
>   	}
>   
> @@ -396,7 +270,7 @@ static void esb_initdevice(void)
>   	/* Check that the WDT isn't already locked */
>   	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
>   	if (val1 & ESB_WDT_LOCK)
> -		pr_warn("nowayout already set\n");
> +		dev_warn(&esb_pci->dev, "nowayout already set\n");
>   
>   	/* Set the timer to watchdog mode and disable it for now */
>   	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> @@ -405,14 +279,14 @@ static void esb_initdevice(void)
>   	esb_unlock_registers();
>   	val2 = readw(ESB_RELOAD_REG);
>   	if (val2 & ESB_WDT_TIMEOUT)
> -		triggered = WDIOF_CARDRESET;
> +		esb_dev.bootstatus = WDIOF_CARDRESET;
>   
>   	/* Reset WDT_TIMEOUT flag and timers */
>   	esb_unlock_registers();
>   	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
>   
>   	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(heartbeat);
> +	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
>   }
>   
>   static int esb_probe(struct pci_dev *pdev,
> @@ -434,26 +308,25 @@ static int esb_probe(struct pci_dev *pdev,
>   	if (!esb_getdevice(pdev) || esb_pci == NULL)
>   		return -ENODEV;
>   
> -	/* Check that the heartbeat value is within it's range;
> -	   if not reset to the default */
> -	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
> -		heartbeat = WATCHDOG_HEARTBEAT;
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
> -			heartbeat);
> -	}
> -
>   	/* Initialize the watchdog and make sure it does not run */
> +	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
> +		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
> +			esb_dev.timeout);
> +	watchdog_set_nowayout(&esb_dev, nowayout);
> +	watchdog_stop_on_reboot(&esb_dev);
> +	watchdog_stop_on_unregister(&esb_dev);
>   	esb_initdevice();
>   
>   	/* Register the watchdog so that userspace has access to it */
> -	ret = misc_register(&esb_miscdev);
> +	ret = watchdog_register_device(&esb_dev);
>   	if (ret != 0) {
> -		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> -		       WATCHDOG_MINOR, ret);
> +		dev_err(&pdev->dev,
> +			"cannot register watchdog device (err=%d)\n", ret);
>   		goto err_unmap;
>   	}
> -	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> -		BASEADDR, heartbeat, nowayout);
> +	dev_info(&pdev->dev,
> +		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> +		BASEADDR, esb_dev.timeout, nowayout);
>   	return 0;
>   
>   err_unmap:
> @@ -466,29 +339,18 @@ static int esb_probe(struct pci_dev *pdev,
>   
>   static void esb_remove(struct pci_dev *pdev)
>   {
> -	/* Stop the timer before we leave */
> -	if (!nowayout)
> -		esb_timer_stop();
> -
> -	/* Deregister */
> -	misc_deregister(&esb_miscdev);
> +	watchdog_unregister_device(&esb_dev);
>   	iounmap(BASEADDR);
>   	pci_release_region(esb_pci, 0);
>   	pci_disable_device(esb_pci);
>   	esb_pci = NULL;
>   }
>   
> -static void esb_shutdown(struct pci_dev *pdev)
> -{
> -	esb_timer_stop();
> -}
> -
>   static struct pci_driver esb_driver = {
>   	.name		= ESB_MODULE_NAME,
>   	.id_table	= esb_pci_tbl,
>   	.probe          = esb_probe,
>   	.remove         = esb_remove,
> -	.shutdown       = esb_shutdown,
>   };
>   
>   module_pci_driver(esb_driver);
> 

Re: [PATCH v3 2/4] watchdog: i6300esb: support multiple devices

[Guenter Roeck]

On 10/26/2017 09:10 AM, Radu Rendec wrote:
> Support multiple i6300esb devices simultaneously, by removing the single
> device restriction in the original design and leveraging the multiple
> device support of the watchdog subsystem.
> 
> This patch replaces the global definitions of watchdog device data with
> a dynamically allocated structure. This structure is allocated during
> device probe, so multiple independent structures can be allocated if
> multiple devices are probed.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

Never mind my other comment; cards_found is removed in a later patch.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/i6300esb.c | 170 +++++++++++++++++++++++---------------------
>   1 file changed, 90 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 4f4287d83c21..147462f431de 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -23,6 +23,7 @@
>    *	Ported driver to kernel 2.6
>    *  20171016 Radu Rendec <rrendec@arista.com>
>    *	Change driver to use the watchdog subsystem
> + *	Add support for multiple 6300ESB devices
>    */
>   
>   /*
> @@ -52,10 +53,10 @@
>   #define ESB_LOCK_REG    0x68            /* WDT lock register                 */
>   
>   /* Memory mapped registers */
> -#define ESB_TIMER1_REG (BASEADDR + 0x00)/* Timer1 value after each reset     */
> -#define ESB_TIMER2_REG (BASEADDR + 0x04)/* Timer2 value after each reset     */
> -#define ESB_GINTSR_REG (BASEADDR + 0x08)/* General Interrupt Status Register */
> -#define ESB_RELOAD_REG (BASEADDR + 0x0c)/* Reload register                   */
> +#define ESB_TIMER1_REG(w) ((w)->base + 0x00)/* Timer1 value after each reset */
> +#define ESB_TIMER2_REG(w) ((w)->base + 0x04)/* Timer2 value after each reset */
> +#define ESB_GINTSR_REG(w) ((w)->base + 0x08)/* General Interrupt Status Reg  */
> +#define ESB_RELOAD_REG(w) ((w)->base + 0x0c)/* Reload register               */
>   
>   /* Lock register bits */
>   #define ESB_WDT_FUNC    (0x01 << 2)   /* Watchdog functionality            */
> @@ -75,11 +76,7 @@
>   #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
>   #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
>   
> -/* internal variables */
> -static void __iomem *BASEADDR;
> -static struct pci_dev *esb_pci;
> -
> -/* We can only use 1 card due to the /dev/watchdog restriction */
> +/* Probed devices counter; used only for printing the initial info message */
>   static int cards_found;
>   
>   /* module parameters */
> @@ -97,6 +94,15 @@ MODULE_PARM_DESC(nowayout,
>   		"Watchdog cannot be stopped once started (default="
>   				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
> +/* internal variables */
> +struct esb_dev {
> +	struct watchdog_device wdd;
> +	void __iomem *base;
> +	struct pci_dev *pdev;
> +};
> +
> +#define to_esb_dev(wptr) container_of(wptr, struct esb_dev, wdd)
> +
>   /*
>    * Some i6300ESB specific functions
>    */
> @@ -107,35 +113,37 @@ MODULE_PARM_DESC(nowayout,
>    * reload register. After this the appropriate registers can be written
>    * to once before they need to be unlocked again.
>    */
> -static inline void esb_unlock_registers(void)
> +static inline void esb_unlock_registers(struct esb_dev *edev)
>   {
> -	writew(ESB_UNLOCK1, ESB_RELOAD_REG);
> -	writew(ESB_UNLOCK2, ESB_RELOAD_REG);
> +	writew(ESB_UNLOCK1, ESB_RELOAD_REG(edev));
> +	writew(ESB_UNLOCK2, ESB_RELOAD_REG(edev));
>   }
>   
>   static int esb_timer_start(struct watchdog_device *wdd)
>   {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>   	int _wdd_nowayout = test_bit(WDOG_NO_WAY_OUT, &wdd->status);
>   	u8 val;
>   
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   	/* Enable or Enable + Lock? */
>   	val = ESB_WDT_ENABLE | (_wdd_nowayout ? ESB_WDT_LOCK : 0x00);
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, val);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, val);
>   	return 0;
>   }
>   
>   static int esb_timer_stop(struct watchdog_device *wdd)
>   {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>   	u8 val;
>   
>   	/* First, reset timers as suggested by the docs */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   	/* Then disable the WDT */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x0);
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x0);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val);
>   
>   	/* Returns 0 if the timer was disabled, non-zero otherwise */
>   	return val & ESB_WDT_ENABLE;
> @@ -143,8 +151,10 @@ static int esb_timer_stop(struct watchdog_device *wdd)
>   
>   static int esb_timer_keepalive(struct watchdog_device *wdd)
>   {
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	struct esb_dev *edev = to_esb_dev(wdd);
> +
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   	/* FIXME: Do we need to flush anything here? */
>   	return 0;
>   }
> @@ -152,6 +162,7 @@ static int esb_timer_keepalive(struct watchdog_device *wdd)
>   static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>   		unsigned int time)
>   {
> +	struct esb_dev *edev = to_esb_dev(wdd);
>   	u32 val;
>   
>   	/* We shift by 9, so if we are passed a value of 1 sec,
> @@ -161,16 +172,16 @@ static int esb_timer_set_heartbeat(struct watchdog_device *wdd,
>   	val = time << 9;
>   
>   	/* Write timer 1 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER1_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER1_REG(edev));
>   
>   	/* Write timer 2 */
> -	esb_unlock_registers();
> -	writel(val, ESB_TIMER2_REG);
> +	esb_unlock_registers(edev);
> +	writel(val, ESB_TIMER2_REG(edev));
>   
>   	/* Reload */
> -	esb_unlock_registers();
> -	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew(ESB_WDT_RELOAD, ESB_RELOAD_REG(edev));
>   
>   	/* FIXME: Do we need to flush everything out? */
>   
> @@ -196,14 +207,6 @@ static const struct watchdog_ops esb_ops = {
>   	.ping = esb_timer_keepalive,
>   };
>   
> -static struct watchdog_device esb_dev = {
> -	.info = &esb_info,
> -	.ops = &esb_ops,
> -	.min_timeout = 1,
> -	.max_timeout = 2 * 0x03ff,
> -	.timeout = WATCHDOG_HEARTBEAT,
> -};
> -
>   /*
>    * Data for PCI driver interface
>    */
> @@ -217,38 +220,38 @@ MODULE_DEVICE_TABLE(pci, esb_pci_tbl);
>    *      Init & exit routines
>    */
>   
> -static unsigned char esb_getdevice(struct pci_dev *pdev)
> +static unsigned char esb_getdevice(struct esb_dev *edev)
>   {
> -	if (pci_enable_device(pdev)) {
> -		dev_err(&pdev->dev, "failed to enable device\n");
> +	if (pci_enable_device(edev->pdev)) {
> +		dev_err(&edev->pdev->dev, "failed to enable device\n");
>   		goto err_devput;
>   	}
>   
> -	if (pci_request_region(pdev, 0, ESB_MODULE_NAME)) {
> -		dev_err(&pdev->dev, "failed to request region\n");
> +	if (pci_request_region(edev->pdev, 0, ESB_MODULE_NAME)) {
> +		dev_err(&edev->pdev->dev, "failed to request region\n");
>   		goto err_disable;
>   	}
>   
> -	BASEADDR = pci_ioremap_bar(pdev, 0);
> -	if (BASEADDR == NULL) {
> +	edev->base = pci_ioremap_bar(edev->pdev, 0);
> +	if (edev->base == NULL) {
>   		/* Something's wrong here, BASEADDR has to be set */
> -		dev_err(&pdev->dev, "failed to get BASEADDR\n");
> +		dev_err(&edev->pdev->dev, "failed to get BASEADDR\n");
>   		goto err_release;
>   	}
>   
>   	/* Done */
> -	esb_pci = pdev;
> +	dev_set_drvdata(&edev->pdev->dev, edev);
>   	return 1;
>   
>   err_release:
> -	pci_release_region(pdev, 0);
> +	pci_release_region(edev->pdev, 0);
>   err_disable:
> -	pci_disable_device(pdev);
> +	pci_disable_device(edev->pdev);
>   err_devput:
>   	return 0;
>   }
>   
> -static void esb_initdevice(void)
> +static void esb_initdevice(struct esb_dev *edev)
>   {
>   	u8 val1;
>   	u16 val2;
> @@ -265,33 +268,34 @@ static void esb_initdevice(void)
>   	 * any interrupts as there is not much we can do with it
>   	 * right now.
>   	 */
> -	pci_write_config_word(esb_pci, ESB_CONFIG_REG, 0x0003);
> +	pci_write_config_word(edev->pdev, ESB_CONFIG_REG, 0x0003);
>   
>   	/* Check that the WDT isn't already locked */
> -	pci_read_config_byte(esb_pci, ESB_LOCK_REG, &val1);
> +	pci_read_config_byte(edev->pdev, ESB_LOCK_REG, &val1);
>   	if (val1 & ESB_WDT_LOCK)
> -		dev_warn(&esb_pci->dev, "nowayout already set\n");
> +		dev_warn(&edev->pdev->dev, "nowayout already set\n");
>   
>   	/* Set the timer to watchdog mode and disable it for now */
> -	pci_write_config_byte(esb_pci, ESB_LOCK_REG, 0x00);
> +	pci_write_config_byte(edev->pdev, ESB_LOCK_REG, 0x00);
>   
>   	/* Check if the watchdog was previously triggered */
> -	esb_unlock_registers();
> -	val2 = readw(ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	val2 = readw(ESB_RELOAD_REG(edev));
>   	if (val2 & ESB_WDT_TIMEOUT)
> -		esb_dev.bootstatus = WDIOF_CARDRESET;
> +		edev->wdd.bootstatus = WDIOF_CARDRESET;
>   
>   	/* Reset WDT_TIMEOUT flag and timers */
> -	esb_unlock_registers();
> -	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG);
> +	esb_unlock_registers(edev);
> +	writew((ESB_WDT_TIMEOUT | ESB_WDT_RELOAD), ESB_RELOAD_REG(edev));
>   
>   	/* And set the correct timeout value */
> -	esb_timer_set_heartbeat(&esb_dev, esb_dev.timeout);
> +	esb_timer_set_heartbeat(&edev->wdd, edev->wdd.timeout);
>   }
>   
>   static int esb_probe(struct pci_dev *pdev,
>   		const struct pci_device_id *ent)
>   {
> +	struct esb_dev *edev;
>   	int ret;
>   
>   	cards_found++;
> @@ -299,26 +303,32 @@ static int esb_probe(struct pci_dev *pdev,
>   		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
>   			ESB_VERSION);
>   
> -	if (cards_found > 1) {
> -		pr_err("This driver only supports 1 device\n");
> -		return -ENODEV;
> -	}
> +	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
> +	if (!edev)
> +		return -ENOMEM;
>   
>   	/* Check whether or not the hardware watchdog is there */
> -	if (!esb_getdevice(pdev) || esb_pci == NULL)
> +	edev->pdev = pdev;
> +	if (!esb_getdevice(edev))
>   		return -ENODEV;
>   
>   	/* Initialize the watchdog and make sure it does not run */
> -	if (watchdog_init_timeout(&esb_dev, heartbeat, NULL))
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %u\n",
> -			esb_dev.timeout);
> -	watchdog_set_nowayout(&esb_dev, nowayout);
> -	watchdog_stop_on_reboot(&esb_dev);
> -	watchdog_stop_on_unregister(&esb_dev);
> -	esb_initdevice();
> +	edev->wdd.info = &esb_info;
> +	edev->wdd.ops = &esb_ops;
> +	edev->wdd.min_timeout = 1;
> +	edev->wdd.max_timeout = 2 * 0x03ff;
> +	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
> +	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
> +		dev_info(&pdev->dev,
> +			"heartbeat value must be 1<heartbeat<2046, using %u\n",
> +			edev->wdd.timeout);
> +	watchdog_set_nowayout(&edev->wdd, nowayout);
> +	watchdog_stop_on_reboot(&edev->wdd);
> +	watchdog_stop_on_unregister(&edev->wdd);
> +	esb_initdevice(edev);
>   
>   	/* Register the watchdog so that userspace has access to it */
> -	ret = watchdog_register_device(&esb_dev);
> +	ret = watchdog_register_device(&edev->wdd);
>   	if (ret != 0) {
>   		dev_err(&pdev->dev,
>   			"cannot register watchdog device (err=%d)\n", ret);
> @@ -326,24 +336,24 @@ static int esb_probe(struct pci_dev *pdev,
>   	}
>   	dev_info(&pdev->dev,
>   		"initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
> -		BASEADDR, esb_dev.timeout, nowayout);
> +		edev->base, edev->wdd.timeout, nowayout);
>   	return 0;
>   
>   err_unmap:
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>   	return ret;
>   }
>   
>   static void esb_remove(struct pci_dev *pdev)
>   {
> -	watchdog_unregister_device(&esb_dev);
> -	iounmap(BASEADDR);
> -	pci_release_region(esb_pci, 0);
> -	pci_disable_device(esb_pci);
> -	esb_pci = NULL;
> +	struct esb_dev *edev = dev_get_drvdata(&pdev->dev);
> +
> +	watchdog_unregister_device(&edev->wdd);
> +	iounmap(edev->base);
> +	pci_release_region(edev->pdev, 0);
> +	pci_disable_device(edev->pdev);
>   }
>   
>   static struct pci_driver esb_driver = {
> 

Re: [PATCH v3 3/4] watchdog: i6300esb: do not hardcode heartbeat limits

[Guenter Roeck]

On 10/26/2017 09:10 AM, Radu Rendec wrote:
> The minimum, maximum and default values for the watchdog heartbeat
> (timeout) were hardcoded in several places (including module parameter
> description and warning message for invalid module parameter value).
> 
> This patch adds macros for the aforementioned values and replaces all
> occurences of hardcoded values by these macros.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/i6300esb.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 147462f431de..4d92925cdc1e 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -81,12 +81,16 @@ static int cards_found;
>   
>   /* module parameters */
>   /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
> -#define WATCHDOG_HEARTBEAT 30
> +#define ESB_HEARTBEAT_MIN	1
> +#define ESB_HEARTBEAT_MAX	2046
> +#define ESB_HEARTBEAT_DEFAULT	30
> +#define ESB_HEARTBEAT_RANGE __MODULE_STRING(ESB_HEARTBEAT_MIN) \
> +	"<heartbeat<" __MODULE_STRING(ESB_HEARTBEAT_MAX)
>   static int heartbeat; /* in seconds */
>   module_param(heartbeat, int, 0);
>   MODULE_PARM_DESC(heartbeat,
> -		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
> -				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
> +	"Watchdog heartbeat in seconds. (" ESB_HEARTBEAT_RANGE
> +	", default=" __MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
>   
>   static bool nowayout = WATCHDOG_NOWAYOUT;
>   module_param(nowayout, bool, 0);
> @@ -315,13 +319,13 @@ static int esb_probe(struct pci_dev *pdev,
>   	/* Initialize the watchdog and make sure it does not run */
>   	edev->wdd.info = &esb_info;
>   	edev->wdd.ops = &esb_ops;
> -	edev->wdd.min_timeout = 1;
> -	edev->wdd.max_timeout = 2 * 0x03ff;
> -	edev->wdd.timeout = WATCHDOG_HEARTBEAT;
> +	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
> +	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
> +	edev->wdd.timeout = ESB_HEARTBEAT_DEFAULT;
>   	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
>   		dev_info(&pdev->dev,
> -			"heartbeat value must be 1<heartbeat<2046, using %u\n",
> -			edev->wdd.timeout);
> +			"heartbeat value must be " ESB_HEARTBEAT_RANGE
> +			", using %u\n", edev->wdd.timeout);
>   	watchdog_set_nowayout(&edev->wdd, nowayout);
>   	watchdog_stop_on_reboot(&edev->wdd);
>   	watchdog_stop_on_unregister(&edev->wdd);
> 

Re: [PATCH v3 4/4] watchdog: i6300esb: remove info message and version number

[Guenter Roeck]

On 10/26/2017 09:10 AM, Radu Rendec wrote:
> The initial info message (early in the esb_probe() function) is not very
> useful and we already have a message on successful probe, which includes
> device identification. If the probe fails (e.g. PCI related errors),
> additional messages are printed anyway.
> 
> The version number was only used in the initial info message. Other than
> that, it serves no useful purpose and it ran out of favor upstream
> anyway.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

... and thanks a lot for doing this!

Guenter

> ---
>   drivers/watchdog/i6300esb.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 4d92925cdc1e..950c71a8bb22 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -30,8 +30,6 @@
>    *      Includes, defines, variables, module parameters, ...
>    */
>   
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
>   #include <linux/module.h>
>   #include <linux/types.h>
>   #include <linux/kernel.h>
> @@ -45,7 +43,6 @@
>   #include <linux/io.h>
>   
>   /* Module and version information */
> -#define ESB_VERSION "0.05"
>   #define ESB_MODULE_NAME "i6300ESB timer"
>   
>   /* PCI configuration registers */
> @@ -76,9 +73,6 @@
>   #define ESB_UNLOCK1     0x80            /* Step 1 to unlock reset registers  */
>   #define ESB_UNLOCK2     0x86            /* Step 2 to unlock reset registers  */
>   
> -/* Probed devices counter; used only for printing the initial info message */
> -static int cards_found;
> -
>   /* module parameters */
>   /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
>   #define ESB_HEARTBEAT_MIN	1
> @@ -302,11 +296,6 @@ static int esb_probe(struct pci_dev *pdev,
>   	struct esb_dev *edev;
>   	int ret;
>   
> -	cards_found++;
> -	if (cards_found == 1)
> -		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
> -			ESB_VERSION);
> -
>   	edev = devm_kzalloc(&pdev->dev, sizeof(*edev), GFP_KERNEL);
>   	if (!edev)
>   		return -ENOMEM;
> 

Re: [PATCH v3 4/4] watchdog: i6300esb: remove info message and version number

[Radu Rendec]

On Thu, 2017-10-26 at 18:05 -0700, Guenter Roeck wrote:
> On 10/26/2017 09:10 AM, Radu Rendec wrote:
> > The initial info message (early in the esb_probe() function) is not very
> > useful and we already have a message on successful probe, which includes
> > device identification. If the probe fails (e.g. PCI related errors),
> > additional messages are printed anyway.
> > 
> > The version number was only used in the initial info message. Other than
> > that, it serves no useful purpose and it ran out of favor upstream
> > anyway.
> > 
> > Signed-off-by: Radu Rendec <rrendec@arista.com>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> ... and thanks a lot for doing this!

You're welcome. Thanks for reviewing this and for all the input!

We also benefit from this work - on the long run, it's always easier to
work with upstream and get changes accepted than maintain custom
patches.

Best,
Radu