[PATCH v2 0/3] wdt: lantiq: Convert to watchdog_device

[PATCH v2 0/3] wdt: lantiq: Convert to watchdog_device

[Hauke Mehrtens]

This converts the watchdog driver for the lantiq SoCs to use the
watchdog_device subsystem instead of implementing the ioctls on its own.

Changes since:
v1:
 * Removed includes for file system operations
 * reordered the adding of new defines
 * Added struct ltq_wdt_hw to store the chip specific function pointer
   instead of using a typedef
 * priv->clk_rate now contains the number of clocks the wdt decreases per
   second
 * call watchdog_init_timeout()
 * keep the watchdog running if it was already running before the driver
   was loaded

Hauke Mehrtens (3):
  wdt: lantiq: update register names to better match spec
  wdt: lantiq: Convert to watchdog_device
  wdt: lantiq: add get_timeleft callback

 drivers/watchdog/Kconfig      |   1 +
 drivers/watchdog/lantiq_wdt.c | 309 ++++++++++++++++++++----------------------
 2 files changed, 148 insertions(+), 162 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] wdt: lantiq: update register names to better match spec

[Hauke Mehrtens]

Some of the names of the bits were confusing to me.
Now the bits share the same prefix as the register they are set on.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/watchdog/lantiq_wdt.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
index 7f43cefa0eae..e5937be09bd4 100644
--- a/drivers/watchdog/lantiq_wdt.c
+++ b/drivers/watchdog/lantiq_wdt.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
+#include <linux/bitops.h>
 #include <linux/watchdog.h>
 #include <linux/of_platform.h>
 #include <linux/uaccess.h>
@@ -40,18 +41,17 @@
  * essentially the following two magic passwords need to be written to allow
  * IO access to the WDT core
  */
-#define LTQ_WDT_PW1		0x00BE0000
-#define LTQ_WDT_PW2		0x00DC0000
+#define LTQ_WDT_CR_PW1		0x00BE0000
+#define LTQ_WDT_CR_PW2		0x00DC0000
 
-#define LTQ_WDT_CR		0x0	/* watchdog control register */
-#define LTQ_WDT_SR		0x8	/* watchdog status register */
+#define LTQ_WDT_CR		0x0		/* watchdog control register */
+#define  LTQ_WDT_CR_GEN		BIT(31)		/* enable bit */
+#define  LTQ_WDT_CR_PWL		(0x3 << 26)	/* Pre-warning limit set to 1/16 of max WDT period */
+#define  LTQ_WDT_CR_CLKDIV	(0x3 << 24)	/* set clock divider to 0x40000 */
+#define  LTQ_WDT_CR_PW_MASK	GENMASK(23, 16)	/* Password field */
+#define  LTQ_WDT_CR_RELOAD_MASK	GENMASK(15, 0)	/* Reload value */
 
-#define LTQ_WDT_SR_EN		(0x1 << 31)	/* enable bit */
-#define LTQ_WDT_SR_PWD		(0x3 << 26)	/* turn on power */
-#define LTQ_WDT_SR_CLKDIV	(0x3 << 24)	/* turn on clock and set */
-						/* divider to 0x40000 */
 #define LTQ_WDT_DIVIDER		0x40000
-#define LTQ_MAX_TIMEOUT		((1 << 16) - 1)	/* the reload field is 16 bit */
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
@@ -68,26 +68,26 @@ ltq_wdt_enable(void)
 {
 	unsigned long int timeout = ltq_wdt_timeout *
 			(ltq_io_region_clk_rate / LTQ_WDT_DIVIDER) + 0x1000;
-	if (timeout > LTQ_MAX_TIMEOUT)
-		timeout = LTQ_MAX_TIMEOUT;
+	if (timeout > LTQ_WDT_CR_RELOAD_MASK)
+		timeout = LTQ_WDT_CR_RELOAD_MASK;
 
 	/* write the first password magic */
-	ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR);
+	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
 	/* write the second magic plus the configuration and new timeout */
-	ltq_w32(LTQ_WDT_SR_EN | LTQ_WDT_SR_PWD | LTQ_WDT_SR_CLKDIV |
-		LTQ_WDT_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR);
+	ltq_w32(LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
+		LTQ_WDT_CR_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR);
 }
 
 static void
 ltq_wdt_disable(void)
 {
 	/* write the first password magic */
-	ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR);
+	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
 	/*
 	 * write the second password magic with no config
 	 * this turns the watchdog off
 	 */
-	ltq_w32(LTQ_WDT_PW2, ltq_wdt_membase + LTQ_WDT_CR);
+	ltq_w32(LTQ_WDT_CR_PW2, ltq_wdt_membase + LTQ_WDT_CR);
 }
 
 static ssize_t
-- 
2.11.0

[PATCH v2 2/3] wdt: lantiq: Convert to watchdog_device

[Hauke Mehrtens]

Instead of doing the ioctl handling manually just use register a
watchdog_device and let the watchdog framework do the ioctl handling.

This also removes the ltq_wdt_bootstatus_set typedef and replaces it
with a structure providing the chip specific functions pointer.
The watchdog_init_timeout() function is now used and the initial timeout
can be provided in device tree.
If the watchdog was already activated it will not be stopped any more,
but the settings from the driver will be used and the watchdog subsystem
will take care.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/watchdog/Kconfig      |   1 +
 drivers/watchdog/lantiq_wdt.c | 277 +++++++++++++++++++-----------------------
 2 files changed, 126 insertions(+), 152 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 5ea8909a41f9..5637d8be31e0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1621,6 +1621,7 @@ config IMGPDC_WDT
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
+	select WATCHDOG_CORE
 	help
 	  Hardware driver for the Lantiq SoC Watchdog Timer.
 
diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
index e5937be09bd4..666db382b824 100644
--- a/drivers/watchdog/lantiq_wdt.c
+++ b/drivers/watchdog/lantiq_wdt.c
@@ -8,11 +8,7 @@
  *  Based on EP93xx wdt driver
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/module.h>
-#include <linux/fs.h>
-#include <linux/miscdevice.h>
 #include <linux/bitops.h>
 #include <linux/watchdog.h>
 #include <linux/of_platform.h>
@@ -55,154 +51,99 @@
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
-static void __iomem *ltq_wdt_membase;
-static unsigned long ltq_io_region_clk_rate;
+struct ltq_wdt_hw {
+	int (*bootstatus_get)(struct device *dev);
+};
 
-static unsigned long ltq_wdt_bootstatus;
-static unsigned long ltq_wdt_in_use;
-static int ltq_wdt_timeout = 30;
-static int ltq_wdt_ok_to_close;
+struct ltq_wdt_priv {
+	struct watchdog_device wdt;
+	void __iomem *membase;
+	unsigned long clk_rate;
+};
 
-static void
-ltq_wdt_enable(void)
+static u32 ltq_wdt_r32(struct ltq_wdt_priv *priv, u32 offset)
 {
-	unsigned long int timeout = ltq_wdt_timeout *
-			(ltq_io_region_clk_rate / LTQ_WDT_DIVIDER) + 0x1000;
-	if (timeout > LTQ_WDT_CR_RELOAD_MASK)
-		timeout = LTQ_WDT_CR_RELOAD_MASK;
-
-	/* write the first password magic */
-	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
-	/* write the second magic plus the configuration and new timeout */
-	ltq_w32(LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
-		LTQ_WDT_CR_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR);
+	return __raw_readl(priv->membase + offset);
 }
 
-static void
-ltq_wdt_disable(void)
+static void ltq_wdt_w32(struct ltq_wdt_priv *priv, u32 val, u32 offset)
 {
-	/* write the first password magic */
-	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
-	/*
-	 * write the second password magic with no config
-	 * this turns the watchdog off
-	 */
-	ltq_w32(LTQ_WDT_CR_PW2, ltq_wdt_membase + LTQ_WDT_CR);
+	return __raw_writel(val, priv->membase + offset);
 }
 
-static ssize_t
-ltq_wdt_write(struct file *file, const char __user *data,
-		size_t len, loff_t *ppos)
+static void ltq_wdt_mask(struct ltq_wdt_priv *priv, u32 clear, u32 set,
+			 u32 offset)
 {
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			ltq_wdt_ok_to_close = 0;
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					ltq_wdt_ok_to_close = 1;
-				else
-					ltq_wdt_ok_to_close = 0;
-			}
-		}
-		ltq_wdt_enable();
-	}
+	u32 val = ltq_wdt_r32(priv, offset);
 
-	return len;
+	val &= ~(clear);
+	val |= set;
+	ltq_wdt_w32(priv, val, offset);
 }
 
-static struct watchdog_info ident = {
+static struct ltq_wdt_priv *ltq_wdt_get_priv(struct watchdog_device *wdt)
+{
+	return container_of(wdt, struct ltq_wdt_priv, wdt);
+}
+
+static struct watchdog_info ltq_wdt_info = {
 	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
-			WDIOF_CARDRESET,
+		   WDIOF_CARDRESET,
 	.identity = "ltq_wdt",
 };
 
-static long
-ltq_wdt_ioctl(struct file *file,
-		unsigned int cmd, unsigned long arg)
+static int ltq_wdt_start(struct watchdog_device *wdt)
 {
-	int ret = -ENOTTY;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
-				sizeof(ident)) ? -EFAULT : 0;
-		break;
-
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(ltq_wdt_bootstatus, (int __user *)arg);
-		break;
-
-	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int __user *)arg);
-		break;
-
-	case WDIOC_SETTIMEOUT:
-		ret = get_user(ltq_wdt_timeout, (int __user *)arg);
-		if (!ret)
-			ltq_wdt_enable();
-		/* intentional drop through */
-	case WDIOC_GETTIMEOUT:
-		ret = put_user(ltq_wdt_timeout, (int __user *)arg);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		ltq_wdt_enable();
-		ret = 0;
-		break;
-	}
-	return ret;
+	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
+	u32 timeout;
+
+	timeout = wdt->timeout * priv->clk_rate;
+
+	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
+	/* write the second magic plus the configuration and new timeout */
+	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK | LTQ_WDT_CR_RELOAD_MASK,
+		     LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
+		     LTQ_WDT_CR_PW2 | timeout,
+		     LTQ_WDT_CR);
+
+	return 0;
 }
 
-static int
-ltq_wdt_open(struct inode *inode, struct file *file)
+static int ltq_wdt_stop(struct watchdog_device *wdt)
 {
-	if (test_and_set_bit(0, &ltq_wdt_in_use))
-		return -EBUSY;
-	ltq_wdt_in_use = 1;
-	ltq_wdt_enable();
+	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
 
-	return nonseekable_open(inode, file);
+	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
+	ltq_wdt_mask(priv, LTQ_WDT_CR_GEN | LTQ_WDT_CR_PW_MASK,
+		     LTQ_WDT_CR_PW2, LTQ_WDT_CR);
+
+	return 0;
 }
 
-static int
-ltq_wdt_release(struct inode *inode, struct file *file)
+static int ltq_wdt_ping(struct watchdog_device *wdt)
 {
-	if (ltq_wdt_ok_to_close)
-		ltq_wdt_disable();
-	else
-		pr_err("watchdog closed without warning\n");
-	ltq_wdt_ok_to_close = 0;
-	clear_bit(0, &ltq_wdt_in_use);
+	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
+	u32 timeout;
+
+	timeout = wdt->timeout * priv->clk_rate;
+
+	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
+	/* write the second magic plus the configuration and new timeout */
+	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK | LTQ_WDT_CR_RELOAD_MASK,
+		     LTQ_WDT_CR_PW2 | timeout, LTQ_WDT_CR);
 
 	return 0;
 }
 
-static const struct file_operations ltq_wdt_fops = {
+static const struct watchdog_ops ltq_wdt_ops = {
 	.owner		= THIS_MODULE,
-	.write		= ltq_wdt_write,
-	.unlocked_ioctl	= ltq_wdt_ioctl,
-	.open		= ltq_wdt_open,
-	.release	= ltq_wdt_release,
-	.llseek		= no_llseek,
+	.start		= ltq_wdt_start,
+	.stop		= ltq_wdt_stop,
+	.ping		= ltq_wdt_ping,
 };
 
-static struct miscdevice ltq_wdt_miscdev = {
-	.minor	= WATCHDOG_MINOR,
-	.name	= "watchdog",
-	.fops	= &ltq_wdt_fops,
-};
-
-typedef int (*ltq_wdt_bootstatus_set)(struct platform_device *pdev);
-
-static int ltq_wdt_bootstatus_xrx(struct platform_device *pdev)
+static int ltq_wdt_bootstatus_xrx(struct device *dev)
 {
-	struct device *dev = &pdev->dev;
 	struct regmap *rcu_regmap;
 	u32 val;
 	int err;
@@ -216,14 +157,13 @@ static int ltq_wdt_bootstatus_xrx(struct platform_device *pdev)
 		return err;
 
 	if (val & LTQ_XRX_RCU_RST_STAT_WDT)
-		ltq_wdt_bootstatus = WDIOF_CARDRESET;
+		return WDIOF_CARDRESET;
 
 	return 0;
 }
 
-static int ltq_wdt_bootstatus_falcon(struct platform_device *pdev)
+static int ltq_wdt_bootstatus_falcon(struct device *dev)
 {
-	struct device *dev = &pdev->dev;
 	struct regmap *rcu_regmap;
 	u32 val;
 	int err;
@@ -238,62 +178,95 @@ static int ltq_wdt_bootstatus_falcon(struct platform_device *pdev)
 		return err;
 
 	if ((val & LTQ_FALCON_SYS1_CPU0RS_MASK) == LTQ_FALCON_SYS1_CPU0RS_WDT)
-		ltq_wdt_bootstatus = WDIOF_CARDRESET;
+		return WDIOF_CARDRESET;
 
 	return 0;
 }
 
-static int
-ltq_wdt_probe(struct platform_device *pdev)
+static int ltq_wdt_probe(struct platform_device *pdev)
 {
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct device *dev = &pdev->dev;
+	struct ltq_wdt_priv *priv;
+	struct watchdog_device *wdt;
+	struct resource *res;
 	struct clk *clk;
-	ltq_wdt_bootstatus_set ltq_wdt_bootstatus_set;
+	const struct ltq_wdt_hw *ltq_wdt_hw;
 	int ret;
+	u32 status;
 
-	ltq_wdt_membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(ltq_wdt_membase))
-		return PTR_ERR(ltq_wdt_membase);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
 
-	ltq_wdt_bootstatus_set = of_device_get_match_data(&pdev->dev);
-	if (ltq_wdt_bootstatus_set) {
-		ret = ltq_wdt_bootstatus_set(pdev);
-		if (ret)
-			return ret;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->membase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->membase))
+		return PTR_ERR(priv->membase);
 
 	/* we do not need to enable the clock as it is always running */
 	clk = clk_get_io();
 	if (IS_ERR(clk)) {
-		dev_err(&pdev->dev, "Failed to get clock\n");
-		return -ENOENT;
+		dev_err(dev, "Failed to get clock");
+		return PTR_ERR(clk);
 	}
-	ltq_io_region_clk_rate = clk_get_rate(clk);
+	priv->clk_rate = clk_get_rate(clk) / LTQ_WDT_DIVIDER;
 	clk_put(clk);
+	if (!priv->clk_rate) {
+		dev_err(dev, "clock has wrong value of 0");
+		return -EIO;
+	}
 
-	dev_info(&pdev->dev, "Init done\n");
-	return misc_register(&ltq_wdt_miscdev);
-}
+	wdt = &priv->wdt;
+	wdt->info		= &ltq_wdt_info;
+	wdt->ops		= &ltq_wdt_ops;
+	wdt->min_timeout	= 1;
+	wdt->max_timeout	= LTQ_WDT_CR_RELOAD_MASK / priv->clk_rate;
+	wdt->timeout		= wdt->max_timeout;
+	wdt->parent		= dev;
+
+	ltq_wdt_hw = of_device_get_match_data(dev);
+	if (ltq_wdt_hw && ltq_wdt_hw->bootstatus_get) {
+		ret = ltq_wdt_hw->bootstatus_get(dev);
+		if (ret < 0)
+			return ret;
+		wdt->bootstatus = ret;
+	}
 
-static int
-ltq_wdt_remove(struct platform_device *pdev)
-{
-	misc_deregister(&ltq_wdt_miscdev);
+	watchdog_set_nowayout(wdt, nowayout);
+	watchdog_init_timeout(wdt, 0, dev);
+
+	status = ltq_wdt_r32(priv, LTQ_WDT_CR);
+	if (status & LTQ_WDT_CR_GEN) {
+		/*
+		 * If the watchdog is already running overwrite it with our
+		 * new settings. Stop is not needed as the start call will
+		 * replace all settings anyway.
+		 */
+		ltq_wdt_start(wdt);
+		set_bit(WDOG_HW_RUNNING, &wdt->status);
+	}
 
-	return 0;
+	return devm_watchdog_register_device(dev, wdt);
 }
 
+static const struct ltq_wdt_hw ltq_wdt_xrx100 = {
+	.bootstatus_get = ltq_wdt_bootstatus_xrx,
+};
+
+static const struct ltq_wdt_hw ltq_wdt_falcon = {
+	.bootstatus_get = ltq_wdt_bootstatus_falcon,
+};
+
 static const struct of_device_id ltq_wdt_match[] = {
-	{ .compatible = "lantiq,wdt", .data = NULL},
-	{ .compatible = "lantiq,xrx100-wdt", .data = ltq_wdt_bootstatus_xrx },
-	{ .compatible = "lantiq,falcon-wdt", .data = ltq_wdt_bootstatus_falcon },
+	{ .compatible = "lantiq,wdt", .data = NULL },
+	{ .compatible = "lantiq,xrx100-wdt", .data = &ltq_wdt_xrx100 },
+	{ .compatible = "lantiq,falcon-wdt", .data = &ltq_wdt_falcon },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ltq_wdt_match);
 
 static struct platform_driver ltq_wdt_driver = {
 	.probe = ltq_wdt_probe,
-	.remove = ltq_wdt_remove,
 	.driver = {
 		.name = "wdt",
 		.of_match_table = ltq_wdt_match,
-- 
2.11.0

[PATCH v2 3/3] wdt: lantiq: add get_timeleft callback

[Hauke Mehrtens]

This callback will provide the current time left.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/watchdog/lantiq_wdt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
index 666db382b824..3aac61ef855a 100644
--- a/drivers/watchdog/lantiq_wdt.c
+++ b/drivers/watchdog/lantiq_wdt.c
@@ -46,6 +46,8 @@
 #define  LTQ_WDT_CR_CLKDIV	(0x3 << 24)	/* set clock divider to 0x40000 */
 #define  LTQ_WDT_CR_PW_MASK	GENMASK(23, 16)	/* Password field */
 #define  LTQ_WDT_CR_RELOAD_MASK	GENMASK(15, 0)	/* Reload value */
+#define LTQ_WDT_SR		0x8		/* watchdog status register */
+#define  LTQ_WDT_SR_VALUE_MASK	GENMASK(15, 0)	/* Timer value */
 
 #define LTQ_WDT_DIVIDER		0x40000
 
@@ -135,11 +137,21 @@ static int ltq_wdt_ping(struct watchdog_device *wdt)
 	return 0;
 }
 
+static unsigned int ltq_wdt_get_timeleft(struct watchdog_device *wdt)
+{
+	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
+	u64 timeout;
+
+	timeout = ltq_wdt_r32(priv, LTQ_WDT_SR) & LTQ_WDT_SR_VALUE_MASK;
+	return do_div(timeout, priv->clk_rate);
+}
+
 static const struct watchdog_ops ltq_wdt_ops = {
 	.owner		= THIS_MODULE,
 	.start		= ltq_wdt_start,
 	.stop		= ltq_wdt_stop,
 	.ping		= ltq_wdt_ping,
+	.get_timeleft	= ltq_wdt_get_timeleft,
 };
 
 static int ltq_wdt_bootstatus_xrx(struct device *dev)
-- 
2.11.0

Re: [v2,1/3] wdt: lantiq: update register names to better match spec

[Guenter Roeck]

On Sun, Sep 02, 2018 at 01:32:31PM +0200, Hauke Mehrtens wrote:
> Some of the names of the bits were confusing to me.
> Now the bits share the same prefix as the register they are set on.
> 

Plus you changed the description. Overall I find the new defines
more confusing than the old ones, especially the changed meaning.
Can you point me to a reference manual ?

> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/watchdog/lantiq_wdt.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
> index 7f43cefa0eae..e5937be09bd4 100644
> --- a/drivers/watchdog/lantiq_wdt.c
> +++ b/drivers/watchdog/lantiq_wdt.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/fs.h>
>  #include <linux/miscdevice.h>
> +#include <linux/bitops.h>
>  #include <linux/watchdog.h>
>  #include <linux/of_platform.h>
>  #include <linux/uaccess.h>
> @@ -40,18 +41,17 @@
>   * essentially the following two magic passwords need to be written to allow
>   * IO access to the WDT core
>   */
> -#define LTQ_WDT_PW1		0x00BE0000
> -#define LTQ_WDT_PW2		0x00DC0000
> +#define LTQ_WDT_CR_PW1		0x00BE0000
> +#define LTQ_WDT_CR_PW2		0x00DC0000
>  
> -#define LTQ_WDT_CR		0x0	/* watchdog control register */
> -#define LTQ_WDT_SR		0x8	/* watchdog status register */
> +#define LTQ_WDT_CR		0x0		/* watchdog control register */
> +#define  LTQ_WDT_CR_GEN		BIT(31)		/* enable bit */
> +#define  LTQ_WDT_CR_PWL		(0x3 << 26)	/* Pre-warning limit set to 1/16 of max WDT period */

Old was "turn on power"

> +#define  LTQ_WDT_CR_CLKDIV	(0x3 << 24)	/* set clock divider to 0x40000 */

Old was: "turn on clock and set divider to 0x40000". Note that the comment
itself does not really add much value, except that it suggests that one
of the bits may _not_ really set a clock mask.

So this doesn't just rename the defines, it also changes the meaning.

Also, while some may feel differently, I do think we should stick with the
80-column limit as long as it exists and generates checkpatch warnings.

> +#define  LTQ_WDT_CR_PW_MASK	GENMASK(23, 16)	/* Password field */
> +#define  LTQ_WDT_CR_RELOAD_MASK	GENMASK(15, 0)	/* Reload value */
>  

The mixed use of GENMASK() for some defines and shift for others is
confusing. 

> -#define LTQ_WDT_SR_EN		(0x1 << 31)	/* enable bit */
> -#define LTQ_WDT_SR_PWD		(0x3 << 26)	/* turn on power */
> -#define LTQ_WDT_SR_CLKDIV	(0x3 << 24)	/* turn on clock and set */
> -						/* divider to 0x40000 */
>  #define LTQ_WDT_DIVIDER		0x40000
> -#define LTQ_MAX_TIMEOUT		((1 << 16) - 1)	/* the reload field is 16 bit */
>  
I question if the change from LTQ_MAX_TIMEOUT to LTQ_WDT_CR_RELOAD_MASK
really adds value. Sure, the maximum value must fit into a mask, but it
is still a maximum value.

>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  
> @@ -68,26 +68,26 @@ ltq_wdt_enable(void)
>  {
>  	unsigned long int timeout = ltq_wdt_timeout *
>  			(ltq_io_region_clk_rate / LTQ_WDT_DIVIDER) + 0x1000;
> -	if (timeout > LTQ_MAX_TIMEOUT)
> -		timeout = LTQ_MAX_TIMEOUT;
> +	if (timeout > LTQ_WDT_CR_RELOAD_MASK)
> +		timeout = LTQ_WDT_CR_RELOAD_MASK;

And this is less confusing than before ? Not to me. I find the new code
more confusing.

>  
>  	/* write the first password magic */
> -	ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR);
> +	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
>  	/* write the second magic plus the configuration and new timeout */
> -	ltq_w32(LTQ_WDT_SR_EN | LTQ_WDT_SR_PWD | LTQ_WDT_SR_CLKDIV | -
> LTQ_WDT_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR); +
> ltq_w32(LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV | +
> LTQ_WDT_CR_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR); }
>  
>  static void ltq_wdt_disable(void) { /* write the first password magic */ -
>  ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR); +
>  ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR); /* * write the second
>  password magic with no config * this turns the watchdog off */ -
>  ltq_w32(LTQ_WDT_PW2, ltq_wdt_membase + LTQ_WDT_CR); +
>  ltq_w32(LTQ_WDT_CR_PW2, ltq_wdt_membase + LTQ_WDT_CR); }
>  
>  static ssize_t

Re: [PATCH v2 2/3] wdt: lantiq: Convert to watchdog_device

[Guenter Roeck]

On 09/02/2018 04:32 AM, Hauke Mehrtens wrote:
> Instead of doing the ioctl handling manually just use register a
> watchdog_device and let the watchdog framework do the ioctl handling.
> 
> This also removes the ltq_wdt_bootstatus_set typedef and replaces it
> with a structure providing the chip specific functions pointer.
> The watchdog_init_timeout() function is now used and the initial timeout
> can be provided in device tree.
> If the watchdog was already activated it will not be stopped any more,
> but the settings from the driver will be used and the watchdog subsystem
> will take care.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>   drivers/watchdog/Kconfig      |   1 +
>   drivers/watchdog/lantiq_wdt.c | 277 +++++++++++++++++++-----------------------
>   2 files changed, 126 insertions(+), 152 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 5ea8909a41f9..5637d8be31e0 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1621,6 +1621,7 @@ config IMGPDC_WDT
>   config LANTIQ_WDT
>   	tristate "Lantiq SoC watchdog"
>   	depends on LANTIQ
> +	select WATCHDOG_CORE
>   	help
>   	  Hardware driver for the Lantiq SoC Watchdog Timer.
>   
> diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
> index e5937be09bd4..666db382b824 100644
> --- a/drivers/watchdog/lantiq_wdt.c
> +++ b/drivers/watchdog/lantiq_wdt.c
> @@ -8,11 +8,7 @@
>    *  Based on EP93xx wdt driver
>    */
>   
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
>   #include <linux/module.h>
> -#include <linux/fs.h>
> -#include <linux/miscdevice.h>
>   #include <linux/bitops.h>
>   #include <linux/watchdog.h>
>   #include <linux/of_platform.h>
> @@ -55,154 +51,99 @@
>   
>   static bool nowayout = WATCHDOG_NOWAYOUT;
>   
> -static void __iomem *ltq_wdt_membase;
> -static unsigned long ltq_io_region_clk_rate;
> +struct ltq_wdt_hw {
> +	int (*bootstatus_get)(struct device *dev);
> +};
>   
> -static unsigned long ltq_wdt_bootstatus;
> -static unsigned long ltq_wdt_in_use;
> -static int ltq_wdt_timeout = 30;
> -static int ltq_wdt_ok_to_close;
> +struct ltq_wdt_priv {
> +	struct watchdog_device wdt;
> +	void __iomem *membase;
> +	unsigned long clk_rate;
> +};
>   
> -static void
> -ltq_wdt_enable(void)
> +static u32 ltq_wdt_r32(struct ltq_wdt_priv *priv, u32 offset)
>   {
> -	unsigned long int timeout = ltq_wdt_timeout *
> -			(ltq_io_region_clk_rate / LTQ_WDT_DIVIDER) + 0x1000;
> -	if (timeout > LTQ_WDT_CR_RELOAD_MASK)
> -		timeout = LTQ_WDT_CR_RELOAD_MASK;
> -
> -	/* write the first password magic */
> -	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
> -	/* write the second magic plus the configuration and new timeout */
> -	ltq_w32(LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
> -		LTQ_WDT_CR_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR);
> +	return __raw_readl(priv->membase + offset);
>   }
>   
> -static void
> -ltq_wdt_disable(void)
> +static void ltq_wdt_w32(struct ltq_wdt_priv *priv, u32 val, u32 offset)
>   {
> -	/* write the first password magic */
> -	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
> -	/*
> -	 * write the second password magic with no config
> -	 * this turns the watchdog off
> -	 */
> -	ltq_w32(LTQ_WDT_CR_PW2, ltq_wdt_membase + LTQ_WDT_CR);
> +	return __raw_writel(val, priv->membase + offset);
>   }
>   
> -static ssize_t
> -ltq_wdt_write(struct file *file, const char __user *data,
> -		size_t len, loff_t *ppos)
> +static void ltq_wdt_mask(struct ltq_wdt_priv *priv, u32 clear, u32 set,
> +			 u32 offset)
>   {
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			ltq_wdt_ok_to_close = 0;
> -			for (i = 0; i != len; i++) {
> -				char c;
> -
> -				if (get_user(c, data + i))
> -					return -EFAULT;
> -				if (c == 'V')
> -					ltq_wdt_ok_to_close = 1;
> -				else
> -					ltq_wdt_ok_to_close = 0;
> -			}
> -		}
> -		ltq_wdt_enable();
> -	}
> +	u32 val = ltq_wdt_r32(priv, offset);
>   
> -	return len;
> +	val &= ~(clear);
> +	val |= set;
> +	ltq_wdt_w32(priv, val, offset);
>   }
>   
> -static struct watchdog_info ident = {
> +static struct ltq_wdt_priv *ltq_wdt_get_priv(struct watchdog_device *wdt)
> +{
> +	return container_of(wdt, struct ltq_wdt_priv, wdt);
> +}
> +
> +static struct watchdog_info ltq_wdt_info = {
>   	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> -			WDIOF_CARDRESET,
> +		   WDIOF_CARDRESET,
>   	.identity = "ltq_wdt",
>   };
>   
> -static long
> -ltq_wdt_ioctl(struct file *file,
> -		unsigned int cmd, unsigned long arg)
> +static int ltq_wdt_start(struct watchdog_device *wdt)
>   {
> -	int ret = -ENOTTY;
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
> -				sizeof(ident)) ? -EFAULT : 0;
> -		break;
> -
> -	case WDIOC_GETBOOTSTATUS:
> -		ret = put_user(ltq_wdt_bootstatus, (int __user *)arg);
> -		break;
> -
> -	case WDIOC_GETSTATUS:
> -		ret = put_user(0, (int __user *)arg);
> -		break;
> -
> -	case WDIOC_SETTIMEOUT:
> -		ret = get_user(ltq_wdt_timeout, (int __user *)arg);
> -		if (!ret)
> -			ltq_wdt_enable();
> -		/* intentional drop through */
> -	case WDIOC_GETTIMEOUT:
> -		ret = put_user(ltq_wdt_timeout, (int __user *)arg);
> -		break;
> -
> -	case WDIOC_KEEPALIVE:
> -		ltq_wdt_enable();
> -		ret = 0;
> -		break;
> -	}
> -	return ret;
> +	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
> +	u32 timeout;
> +
> +	timeout = wdt->timeout * priv->clk_rate;
> +
> +	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
> +	/* write the second magic plus the configuration and new timeout */
> +	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK | LTQ_WDT_CR_RELOAD_MASK,
> +		     LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
> +		     LTQ_WDT_CR_PW2 | timeout,
> +		     LTQ_WDT_CR);
> +
> +	return 0;
>   }
>   
> -static int
> -ltq_wdt_open(struct inode *inode, struct file *file)
> +static int ltq_wdt_stop(struct watchdog_device *wdt)
>   {
> -	if (test_and_set_bit(0, &ltq_wdt_in_use))
> -		return -EBUSY;
> -	ltq_wdt_in_use = 1;
> -	ltq_wdt_enable();
> +	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
>   
> -	return nonseekable_open(inode, file);
> +	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
> +	ltq_wdt_mask(priv, LTQ_WDT_CR_GEN | LTQ_WDT_CR_PW_MASK,
> +		     LTQ_WDT_CR_PW2, LTQ_WDT_CR);
> +
> +	return 0;
>   }
>   
> -static int
> -ltq_wdt_release(struct inode *inode, struct file *file)
> +static int ltq_wdt_ping(struct watchdog_device *wdt)
>   {
> -	if (ltq_wdt_ok_to_close)
> -		ltq_wdt_disable();
> -	else
> -		pr_err("watchdog closed without warning\n");
> -	ltq_wdt_ok_to_close = 0;
> -	clear_bit(0, &ltq_wdt_in_use);
> +	struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
> +	u32 timeout;
> +
> +	timeout = wdt->timeout * priv->clk_rate;
> +
> +	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
> +	/* write the second magic plus the configuration and new timeout */
> +	ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK | LTQ_WDT_CR_RELOAD_MASK,
> +		     LTQ_WDT_CR_PW2 | timeout, LTQ_WDT_CR);
>   
>   	return 0;
>   }
>   
> -static const struct file_operations ltq_wdt_fops = {
> +static const struct watchdog_ops ltq_wdt_ops = {
>   	.owner		= THIS_MODULE,
> -	.write		= ltq_wdt_write,
> -	.unlocked_ioctl	= ltq_wdt_ioctl,
> -	.open		= ltq_wdt_open,
> -	.release	= ltq_wdt_release,
> -	.llseek		= no_llseek,
> +	.start		= ltq_wdt_start,
> +	.stop		= ltq_wdt_stop,
> +	.ping		= ltq_wdt_ping,
>   };
>   
> -static struct miscdevice ltq_wdt_miscdev = {
> -	.minor	= WATCHDOG_MINOR,
> -	.name	= "watchdog",
> -	.fops	= &ltq_wdt_fops,
> -};
> -
> -typedef int (*ltq_wdt_bootstatus_set)(struct platform_device *pdev);
> -
> -static int ltq_wdt_bootstatus_xrx(struct platform_device *pdev)
> +static int ltq_wdt_bootstatus_xrx(struct device *dev)
>   {
> -	struct device *dev = &pdev->dev;
>   	struct regmap *rcu_regmap;
>   	u32 val;
>   	int err;
> @@ -216,14 +157,13 @@ static int ltq_wdt_bootstatus_xrx(struct platform_device *pdev)
>   		return err;
>   
>   	if (val & LTQ_XRX_RCU_RST_STAT_WDT)
> -		ltq_wdt_bootstatus = WDIOF_CARDRESET;
> +		return WDIOF_CARDRESET;
>   
>   	return 0;
>   }
>   
> -static int ltq_wdt_bootstatus_falcon(struct platform_device *pdev)
> +static int ltq_wdt_bootstatus_falcon(struct device *dev)
>   {
> -	struct device *dev = &pdev->dev;
>   	struct regmap *rcu_regmap;
>   	u32 val;
>   	int err;
> @@ -238,62 +178,95 @@ static int ltq_wdt_bootstatus_falcon(struct platform_device *pdev)
>   		return err;
>   
>   	if ((val & LTQ_FALCON_SYS1_CPU0RS_MASK) == LTQ_FALCON_SYS1_CPU0RS_WDT)
> -		ltq_wdt_bootstatus = WDIOF_CARDRESET;
> +		return WDIOF_CARDRESET;
>   
>   	return 0;
>   }
>   
> -static int
> -ltq_wdt_probe(struct platform_device *pdev)
> +static int ltq_wdt_probe(struct platform_device *pdev)
>   {
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	struct device *dev = &pdev->dev;
> +	struct ltq_wdt_priv *priv;
> +	struct watchdog_device *wdt;
> +	struct resource *res;
>   	struct clk *clk;
> -	ltq_wdt_bootstatus_set ltq_wdt_bootstatus_set;
> +	const struct ltq_wdt_hw *ltq_wdt_hw;
>   	int ret;
> +	u32 status;
>   
> -	ltq_wdt_membase = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(ltq_wdt_membase))
> -		return PTR_ERR(ltq_wdt_membase);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
>   
> -	ltq_wdt_bootstatus_set = of_device_get_match_data(&pdev->dev);
> -	if (ltq_wdt_bootstatus_set) {
> -		ret = ltq_wdt_bootstatus_set(pdev);
> -		if (ret)
> -			return ret;
> -	}
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->membase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->membase))
> +		return PTR_ERR(priv->membase);
>   
>   	/* we do not need to enable the clock as it is always running */
>   	clk = clk_get_io();
>   	if (IS_ERR(clk)) {

Not new, nor that it really matters, but this error check is quite pointless:
The function never returns an error.

> -		dev_err(&pdev->dev, "Failed to get clock\n");
> -		return -ENOENT;
> +		dev_err(dev, "Failed to get clock");
> +		return PTR_ERR(clk);
>   	}
> -	ltq_io_region_clk_rate = clk_get_rate(clk);
> +	priv->clk_rate = clk_get_rate(clk) / LTQ_WDT_DIVIDER;
>   	clk_put(clk);

Calling clk_put() here actually odd and should be dropped. Unlike clk_get(),
clk_get_io() does not get a reference to the clock instance. It just returns
a pointer to a static clock.

> +	if (!priv->clk_rate) {
> +		dev_err(dev, "clock has wrong value of 0");

This is misleading: It is not known if the clock rate is indeed 0.
It is only known that the clock rate is below LTQ_WDT_DIVIDER.

> +		return -EIO;

I am not happy with EIO in this case. There is no indication
of an I/O error.

> +	}
>   
> -	dev_info(&pdev->dev, "Init done\n");
> -	return misc_register(&ltq_wdt_miscdev);
> -}
> +	wdt = &priv->wdt;
> +	wdt->info		= &ltq_wdt_info;
> +	wdt->ops		= &ltq_wdt_ops;
> +	wdt->min_timeout	= 1;
> +	wdt->max_timeout	= LTQ_WDT_CR_RELOAD_MASK / priv->clk_rate;
> +	wdt->timeout		= wdt->max_timeout;
> +	wdt->parent		= dev;
> +
> +	ltq_wdt_hw = of_device_get_match_data(dev);
> +	if (ltq_wdt_hw && ltq_wdt_hw->bootstatus_get) {
> +		ret = ltq_wdt_hw->bootstatus_get(dev);
> +		if (ret < 0)
> +			return ret;

This is also quite pointless: The functions never return an error.
Might as well just assign the returned value to bootstatus.

While it makes some sense to have the error checks above (after all,
the API could change), this one really does not make sense since it is
in full driver control.

> +		wdt->bootstatus = ret;
> +	}
>   
> -static int
> -ltq_wdt_remove(struct platform_device *pdev)
> -{
> -	misc_deregister(&ltq_wdt_miscdev);
> +	watchdog_set_nowayout(wdt, nowayout);
> +	watchdog_init_timeout(wdt, 0, dev);
> +
> +	status = ltq_wdt_r32(priv, LTQ_WDT_CR);
> +	if (status & LTQ_WDT_CR_GEN) {
> +		/*
> +		 * If the watchdog is already running overwrite it with our
> +		 * new settings. Stop is not needed as the start call will
> +		 * replace all settings anyway.
> +		 */
> +		ltq_wdt_start(wdt);
> +		set_bit(WDOG_HW_RUNNING, &wdt->status);
> +	}
>   
> -	return 0;
> +	return devm_watchdog_register_device(dev, wdt);
>   }
>   
> +static const struct ltq_wdt_hw ltq_wdt_xrx100 = {
> +	.bootstatus_get = ltq_wdt_bootstatus_xrx,
> +};
> +
> +static const struct ltq_wdt_hw ltq_wdt_falcon = {
> +	.bootstatus_get = ltq_wdt_bootstatus_falcon,
> +};
> +
>   static const struct of_device_id ltq_wdt_match[] = {
> -	{ .compatible = "lantiq,wdt", .data = NULL},
> -	{ .compatible = "lantiq,xrx100-wdt", .data = ltq_wdt_bootstatus_xrx },
> -	{ .compatible = "lantiq,falcon-wdt", .data = ltq_wdt_bootstatus_falcon },
> +	{ .compatible = "lantiq,wdt", .data = NULL },
> +	{ .compatible = "lantiq,xrx100-wdt", .data = &ltq_wdt_xrx100 },
> +	{ .compatible = "lantiq,falcon-wdt", .data = &ltq_wdt_falcon },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, ltq_wdt_match);
>   
>   static struct platform_driver ltq_wdt_driver = {
>   	.probe = ltq_wdt_probe,
> -	.remove = ltq_wdt_remove,
>   	.driver = {
>   		.name = "wdt",
>   		.of_match_table = ltq_wdt_match,
> 

Re: [v2,1/3] wdt: lantiq: update register names to better match spec

[Hauke Mehrtens]

[-- Attachment #1.1: Type: text/plain, Size: 5470 bytes --]

On 09/04/2018 01:09 AM, Guenter Roeck wrote:
> On Sun, Sep 02, 2018 at 01:32:31PM +0200, Hauke Mehrtens wrote:
>> Some of the names of the bits were confusing to me.
>> Now the bits share the same prefix as the register they are set on.
>>
> 
> Plus you changed the description. Overall I find the new defines
> more confusing than the old ones, especially the changed meaning.
> Can you point me to a reference manual ?

Ok I will update the commit message.

Some of the defines had wrong comments in this driver.

>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/watchdog/lantiq_wdt.c | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
>> index 7f43cefa0eae..e5937be09bd4 100644
>> --- a/drivers/watchdog/lantiq_wdt.c
>> +++ b/drivers/watchdog/lantiq_wdt.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/module.h>
>>  #include <linux/fs.h>
>>  #include <linux/miscdevice.h>
>> +#include <linux/bitops.h>
>>  #include <linux/watchdog.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/uaccess.h>
>> @@ -40,18 +41,17 @@
>>   * essentially the following two magic passwords need to be written to allow
>>   * IO access to the WDT core
>>   */
>> -#define LTQ_WDT_PW1		0x00BE0000
>> -#define LTQ_WDT_PW2		0x00DC0000
>> +#define LTQ_WDT_CR_PW1		0x00BE0000
>> +#define LTQ_WDT_CR_PW2		0x00DC0000
>>  
>> -#define LTQ_WDT_CR		0x0	/* watchdog control register */
>> -#define LTQ_WDT_SR		0x8	/* watchdog status register */
>> +#define LTQ_WDT_CR		0x0		/* watchdog control register */
>> +#define  LTQ_WDT_CR_GEN		BIT(31)		/* enable bit */
>> +#define  LTQ_WDT_CR_PWL		(0x3 << 26)	/* Pre-warning limit set to 1/16 of max WDT period */
> 
> Old was "turn on power"

I do not know why someone commented this as "turn on power", this is the
Pre warnings limit register (26:25) for all SoCs I am aware of.

>> +#define  LTQ_WDT_CR_CLKDIV	(0x3 << 24)	/* set clock divider to 0x40000 */
> 
> Old was: "turn on clock and set divider to 0x40000". Note that the comment
> itself does not really add much value, except that it suggests that one> of the bits may _not_ really set a clock mask.

This sets only the clock divider, all possible 4 values for these two
bits are valid dividers. For Linux with multi second watchdog timeouts
only the biggest diver makes sense.

> So this doesn't just rename the defines, it also changes the meaning.
> 
> Also, while some may feel differently, I do think we should stick with the
> 80-column limit as long as it exists and generates checkpatch warnings.

I will add the comment above the line if if gets too long.

>> +#define  LTQ_WDT_CR_PW_MASK	GENMASK(23, 16)	/* Password field */
>> +#define  LTQ_WDT_CR_RELOAD_MASK	GENMASK(15, 0)	/* Reload value */
>>  
> 
> The mixed use of GENMASK() for some defines and shift for others is
> confusing.

I used the shifts when it is a value in a register and I used the
GENMASK when it is a MASK of a register.

It is not supported in this driver to change the pre timeout warning and
the clock divider and from the use cases I am aware of it also does not
make sense so I only the useful value is added here.

>> -#define LTQ_WDT_SR_EN		(0x1 << 31)	/* enable bit */
>> -#define LTQ_WDT_SR_PWD		(0x3 << 26)	/* turn on power */
>> -#define LTQ_WDT_SR_CLKDIV	(0x3 << 24)	/* turn on clock and set */
>> -						/* divider to 0x40000 */
>>  #define LTQ_WDT_DIVIDER		0x40000
>> -#define LTQ_MAX_TIMEOUT		((1 << 16) - 1)	/* the reload field is 16 bit */
>>  
> I question if the change from LTQ_MAX_TIMEOUT to LTQ_WDT_CR_RELOAD_MASK
> really adds value. Sure, the maximum value must fit into a mask, but it
> is still a maximum value.

I need the same value as a max timeout value and I need it as a mask to
make sure all bits of the timeout are set to 0.

I can change this back to LTQ_MAX_TIMEOUT and use it also as a mask.

>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>>  
>> @@ -68,26 +68,26 @@ ltq_wdt_enable(void)
>>  {
>>  	unsigned long int timeout = ltq_wdt_timeout *
>>  			(ltq_io_region_clk_rate / LTQ_WDT_DIVIDER) + 0x1000;
>> -	if (timeout > LTQ_MAX_TIMEOUT)
>> -		timeout = LTQ_MAX_TIMEOUT;
>> +	if (timeout > LTQ_WDT_CR_RELOAD_MASK)
>> +		timeout = LTQ_WDT_CR_RELOAD_MASK;
> 
> And this is less confusing than before ? Not to me. I find the new code
> more confusing.

I agree.

> 
>>  
>>  	/* write the first password magic */
>> -	ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR);
>> +	ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
>>  	/* write the second magic plus the configuration and new timeout */
>> -	ltq_w32(LTQ_WDT_SR_EN | LTQ_WDT_SR_PWD | LTQ_WDT_SR_CLKDIV | -
>> LTQ_WDT_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR); +
>> ltq_w32(LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV | +
>> LTQ_WDT_CR_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR); }
>>  
>>  static void ltq_wdt_disable(void) { /* write the first password magic */ -
>>  ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR); +
>>  ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR); /* * write the second
>>  password magic with no config * this turns the watchdog off */ -
>>  ltq_w32(LTQ_WDT_PW2, ltq_wdt_membase + LTQ_WDT_CR); +
>>  ltq_w32(LTQ_WDT_CR_PW2, ltq_wdt_membase + LTQ_WDT_CR); }
>>  
>>  static ssize_t



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] wdt: lantiq: Convert to watchdog_device

[Hauke Mehrtens]

[-- Attachment #1.1: Type: text/plain, Size: 16647 bytes --]

On 09/04/2018 01:43 AM, Guenter Roeck wrote:
> On 09/02/2018 04:32 AM, Hauke Mehrtens wrote:
>> Instead of doing the ioctl handling manually just use register a
>> watchdog_device and let the watchdog framework do the ioctl handling.
>>
>> This also removes the ltq_wdt_bootstatus_set typedef and replaces it
>> with a structure providing the chip specific functions pointer.
>> The watchdog_init_timeout() function is now used and the initial timeout
>> can be provided in device tree.
>> If the watchdog was already activated it will not be stopped any more,
>> but the settings from the driver will be used and the watchdog subsystem
>> will take care.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>   drivers/watchdog/Kconfig      |   1 +
>>   drivers/watchdog/lantiq_wdt.c | 277
>> +++++++++++++++++++-----------------------
>>   2 files changed, 126 insertions(+), 152 deletions(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 5ea8909a41f9..5637d8be31e0 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1621,6 +1621,7 @@ config IMGPDC_WDT
>>   config LANTIQ_WDT
>>       tristate "Lantiq SoC watchdog"
>>       depends on LANTIQ
>> +    select WATCHDOG_CORE
>>       help
>>         Hardware driver for the Lantiq SoC Watchdog Timer.
>>   diff --git a/drivers/watchdog/lantiq_wdt.c
>> b/drivers/watchdog/lantiq_wdt.c
>> index e5937be09bd4..666db382b824 100644
>> --- a/drivers/watchdog/lantiq_wdt.c
>> +++ b/drivers/watchdog/lantiq_wdt.c
>> @@ -8,11 +8,7 @@
>>    *  Based on EP93xx wdt driver
>>    */
>>   -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> -
>>   #include <linux/module.h>
>> -#include <linux/fs.h>
>> -#include <linux/miscdevice.h>
>>   #include <linux/bitops.h>
>>   #include <linux/watchdog.h>
>>   #include <linux/of_platform.h>
>> @@ -55,154 +51,99 @@
>>     static bool nowayout = WATCHDOG_NOWAYOUT;
>>   -static void __iomem *ltq_wdt_membase;
>> -static unsigned long ltq_io_region_clk_rate;
>> +struct ltq_wdt_hw {
>> +    int (*bootstatus_get)(struct device *dev);
>> +};
>>   -static unsigned long ltq_wdt_bootstatus;
>> -static unsigned long ltq_wdt_in_use;
>> -static int ltq_wdt_timeout = 30;
>> -static int ltq_wdt_ok_to_close;
>> +struct ltq_wdt_priv {
>> +    struct watchdog_device wdt;
>> +    void __iomem *membase;
>> +    unsigned long clk_rate;
>> +};
>>   -static void
>> -ltq_wdt_enable(void)
>> +static u32 ltq_wdt_r32(struct ltq_wdt_priv *priv, u32 offset)
>>   {
>> -    unsigned long int timeout = ltq_wdt_timeout *
>> -            (ltq_io_region_clk_rate / LTQ_WDT_DIVIDER) + 0x1000;
>> -    if (timeout > LTQ_WDT_CR_RELOAD_MASK)
>> -        timeout = LTQ_WDT_CR_RELOAD_MASK;
>> -
>> -    /* write the first password magic */
>> -    ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
>> -    /* write the second magic plus the configuration and new timeout */
>> -    ltq_w32(LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
>> -        LTQ_WDT_CR_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR);
>> +    return __raw_readl(priv->membase + offset);
>>   }
>>   -static void
>> -ltq_wdt_disable(void)
>> +static void ltq_wdt_w32(struct ltq_wdt_priv *priv, u32 val, u32 offset)
>>   {
>> -    /* write the first password magic */
>> -    ltq_w32(LTQ_WDT_CR_PW1, ltq_wdt_membase + LTQ_WDT_CR);
>> -    /*
>> -     * write the second password magic with no config
>> -     * this turns the watchdog off
>> -     */
>> -    ltq_w32(LTQ_WDT_CR_PW2, ltq_wdt_membase + LTQ_WDT_CR);
>> +    return __raw_writel(val, priv->membase + offset);
>>   }
>>   -static ssize_t
>> -ltq_wdt_write(struct file *file, const char __user *data,
>> -        size_t len, loff_t *ppos)
>> +static void ltq_wdt_mask(struct ltq_wdt_priv *priv, u32 clear, u32 set,
>> +             u32 offset)
>>   {
>> -    if (len) {
>> -        if (!nowayout) {
>> -            size_t i;
>> -
>> -            ltq_wdt_ok_to_close = 0;
>> -            for (i = 0; i != len; i++) {
>> -                char c;
>> -
>> -                if (get_user(c, data + i))
>> -                    return -EFAULT;
>> -                if (c == 'V')
>> -                    ltq_wdt_ok_to_close = 1;
>> -                else
>> -                    ltq_wdt_ok_to_close = 0;
>> -            }
>> -        }
>> -        ltq_wdt_enable();
>> -    }
>> +    u32 val = ltq_wdt_r32(priv, offset);
>>   -    return len;
>> +    val &= ~(clear);
>> +    val |= set;
>> +    ltq_wdt_w32(priv, val, offset);
>>   }
>>   -static struct watchdog_info ident = {
>> +static struct ltq_wdt_priv *ltq_wdt_get_priv(struct watchdog_device
>> *wdt)
>> +{
>> +    return container_of(wdt, struct ltq_wdt_priv, wdt);
>> +}
>> +
>> +static struct watchdog_info ltq_wdt_info = {
>>       .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT |
>> WDIOF_KEEPALIVEPING |
>> -            WDIOF_CARDRESET,
>> +           WDIOF_CARDRESET,
>>       .identity = "ltq_wdt",
>>   };
>>   -static long
>> -ltq_wdt_ioctl(struct file *file,
>> -        unsigned int cmd, unsigned long arg)
>> +static int ltq_wdt_start(struct watchdog_device *wdt)
>>   {
>> -    int ret = -ENOTTY;
>> -
>> -    switch (cmd) {
>> -    case WDIOC_GETSUPPORT:
>> -        ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
>> -                sizeof(ident)) ? -EFAULT : 0;
>> -        break;
>> -
>> -    case WDIOC_GETBOOTSTATUS:
>> -        ret = put_user(ltq_wdt_bootstatus, (int __user *)arg);
>> -        break;
>> -
>> -    case WDIOC_GETSTATUS:
>> -        ret = put_user(0, (int __user *)arg);
>> -        break;
>> -
>> -    case WDIOC_SETTIMEOUT:
>> -        ret = get_user(ltq_wdt_timeout, (int __user *)arg);
>> -        if (!ret)
>> -            ltq_wdt_enable();
>> -        /* intentional drop through */
>> -    case WDIOC_GETTIMEOUT:
>> -        ret = put_user(ltq_wdt_timeout, (int __user *)arg);
>> -        break;
>> -
>> -    case WDIOC_KEEPALIVE:
>> -        ltq_wdt_enable();
>> -        ret = 0;
>> -        break;
>> -    }
>> -    return ret;
>> +    struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
>> +    u32 timeout;
>> +
>> +    timeout = wdt->timeout * priv->clk_rate;
>> +
>> +    ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
>> +    /* write the second magic plus the configuration and new timeout */
>> +    ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK | LTQ_WDT_CR_RELOAD_MASK,
>> +             LTQ_WDT_CR_GEN | LTQ_WDT_CR_PWL | LTQ_WDT_CR_CLKDIV |
>> +             LTQ_WDT_CR_PW2 | timeout,
>> +             LTQ_WDT_CR);
>> +
>> +    return 0;
>>   }
>>   -static int
>> -ltq_wdt_open(struct inode *inode, struct file *file)
>> +static int ltq_wdt_stop(struct watchdog_device *wdt)
>>   {
>> -    if (test_and_set_bit(0, &ltq_wdt_in_use))
>> -        return -EBUSY;
>> -    ltq_wdt_in_use = 1;
>> -    ltq_wdt_enable();
>> +    struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
>>   -    return nonseekable_open(inode, file);
>> +    ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
>> +    ltq_wdt_mask(priv, LTQ_WDT_CR_GEN | LTQ_WDT_CR_PW_MASK,
>> +             LTQ_WDT_CR_PW2, LTQ_WDT_CR);
>> +
>> +    return 0;
>>   }
>>   -static int
>> -ltq_wdt_release(struct inode *inode, struct file *file)
>> +static int ltq_wdt_ping(struct watchdog_device *wdt)
>>   {
>> -    if (ltq_wdt_ok_to_close)
>> -        ltq_wdt_disable();
>> -    else
>> -        pr_err("watchdog closed without warning\n");
>> -    ltq_wdt_ok_to_close = 0;
>> -    clear_bit(0, &ltq_wdt_in_use);
>> +    struct ltq_wdt_priv *priv = ltq_wdt_get_priv(wdt);
>> +    u32 timeout;
>> +
>> +    timeout = wdt->timeout * priv->clk_rate;
>> +
>> +    ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK, LTQ_WDT_CR_PW1, LTQ_WDT_CR);
>> +    /* write the second magic plus the configuration and new timeout */
>> +    ltq_wdt_mask(priv, LTQ_WDT_CR_PW_MASK | LTQ_WDT_CR_RELOAD_MASK,
>> +             LTQ_WDT_CR_PW2 | timeout, LTQ_WDT_CR);
>>         return 0;
>>   }
>>   -static const struct file_operations ltq_wdt_fops = {
>> +static const struct watchdog_ops ltq_wdt_ops = {
>>       .owner        = THIS_MODULE,
>> -    .write        = ltq_wdt_write,
>> -    .unlocked_ioctl    = ltq_wdt_ioctl,
>> -    .open        = ltq_wdt_open,
>> -    .release    = ltq_wdt_release,
>> -    .llseek        = no_llseek,
>> +    .start        = ltq_wdt_start,
>> +    .stop        = ltq_wdt_stop,
>> +    .ping        = ltq_wdt_ping,
>>   };
>>   -static struct miscdevice ltq_wdt_miscdev = {
>> -    .minor    = WATCHDOG_MINOR,
>> -    .name    = "watchdog",
>> -    .fops    = &ltq_wdt_fops,
>> -};
>> -
>> -typedef int (*ltq_wdt_bootstatus_set)(struct platform_device *pdev);
>> -
>> -static int ltq_wdt_bootstatus_xrx(struct platform_device *pdev)
>> +static int ltq_wdt_bootstatus_xrx(struct device *dev)
>>   {
>> -    struct device *dev = &pdev->dev;
>>       struct regmap *rcu_regmap;
>>       u32 val;
>>       int err;
>> @@ -216,14 +157,13 @@ static int ltq_wdt_bootstatus_xrx(struct
>> platform_device *pdev)
>>           return err;
>>         if (val & LTQ_XRX_RCU_RST_STAT_WDT)
>> -        ltq_wdt_bootstatus = WDIOF_CARDRESET;
>> +        return WDIOF_CARDRESET;
>>         return 0;
>>   }
>>   -static int ltq_wdt_bootstatus_falcon(struct platform_device *pdev)
>> +static int ltq_wdt_bootstatus_falcon(struct device *dev)
>>   {
>> -    struct device *dev = &pdev->dev;
>>       struct regmap *rcu_regmap;
>>       u32 val;
>>       int err;
>> @@ -238,62 +178,95 @@ static int ltq_wdt_bootstatus_falcon(struct
>> platform_device *pdev)
>>           return err;
>>         if ((val & LTQ_FALCON_SYS1_CPU0RS_MASK) ==
>> LTQ_FALCON_SYS1_CPU0RS_WDT)
>> -        ltq_wdt_bootstatus = WDIOF_CARDRESET;
>> +        return WDIOF_CARDRESET;
>>         return 0;
>>   }
>>   -static int
>> -ltq_wdt_probe(struct platform_device *pdev)
>> +static int ltq_wdt_probe(struct platform_device *pdev)
>>   {
>> -    struct resource *res = platform_get_resource(pdev,
>> IORESOURCE_MEM, 0);
>> +    struct device *dev = &pdev->dev;
>> +    struct ltq_wdt_priv *priv;
>> +    struct watchdog_device *wdt;
>> +    struct resource *res;
>>       struct clk *clk;
>> -    ltq_wdt_bootstatus_set ltq_wdt_bootstatus_set;
>> +    const struct ltq_wdt_hw *ltq_wdt_hw;
>>       int ret;
>> +    u32 status;
>>   -    ltq_wdt_membase = devm_ioremap_resource(&pdev->dev, res);
>> -    if (IS_ERR(ltq_wdt_membase))
>> -        return PTR_ERR(ltq_wdt_membase);
>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>>   -    ltq_wdt_bootstatus_set = of_device_get_match_data(&pdev->dev);
>> -    if (ltq_wdt_bootstatus_set) {
>> -        ret = ltq_wdt_bootstatus_set(pdev);
>> -        if (ret)
>> -            return ret;
>> -    }
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    priv->membase = devm_ioremap_resource(dev, res);
>> +    if (IS_ERR(priv->membase))
>> +        return PTR_ERR(priv->membase);
>>         /* we do not need to enable the clock as it is always running */
>>       clk = clk_get_io();
>>       if (IS_ERR(clk)) {
> 
> Not new, nor that it really matters, but this error check is quite
> pointless:
> The function never returns an error.

Ok, I will remove this.

> 
>> -        dev_err(&pdev->dev, "Failed to get clock\n");
>> -        return -ENOENT;
>> +        dev_err(dev, "Failed to get clock");
>> +        return PTR_ERR(clk);
>>       }
>> -    ltq_io_region_clk_rate = clk_get_rate(clk);
>> +    priv->clk_rate = clk_get_rate(clk) / LTQ_WDT_DIVIDER;
>>       clk_put(clk);
> 
> Calling clk_put() here actually odd and should be dropped. Unlike
> clk_get(),
> clk_get_io() does not get a reference to the clock instance. It just
> returns
> a pointer to a static clock.

I will remove it.

>> +    if (!priv->clk_rate) {
>> +        dev_err(dev, "clock has wrong value of 0");
> 
> This is misleading: It is not known if the clock rate is indeed 0.
> It is only known that the clock rate is below LTQ_WDT_DIVIDER.

Ok I will update this.

>> +        return -EIO;
> 
> I am not happy with EIO in this case. There is no indication
> of an I/O error.

Is -EINVAL better?

>> +    }
>>   -    dev_info(&pdev->dev, "Init done\n");
>> -    return misc_register(&ltq_wdt_miscdev);
>> -}
>> +    wdt = &priv->wdt;
>> +    wdt->info        = &ltq_wdt_info;
>> +    wdt->ops        = &ltq_wdt_ops;
>> +    wdt->min_timeout    = 1;
>> +    wdt->max_timeout    = LTQ_WDT_CR_RELOAD_MASK / priv->clk_rate;
>> +    wdt->timeout        = wdt->max_timeout;
>> +    wdt->parent        = dev;
>> +
>> +    ltq_wdt_hw = of_device_get_match_data(dev);
>> +    if (ltq_wdt_hw && ltq_wdt_hw->bootstatus_get) {
>> +        ret = ltq_wdt_hw->bootstatus_get(dev);
>> +        if (ret < 0)
>> +            return ret;
> 
> This is also quite pointless: The functions never return an error.
> Might as well just assign the returned value to bootstatus.
> 
> While it makes some sense to have the error checks above (after all,
> the API could change), this one really does not make sense since it is
> in full driver control.

Ok, I will remove this.

> 
>> +        wdt->bootstatus = ret;
>> +    }
>>   -static int
>> -ltq_wdt_remove(struct platform_device *pdev)
>> -{
>> -    misc_deregister(&ltq_wdt_miscdev);
>> +    watchdog_set_nowayout(wdt, nowayout);
>> +    watchdog_init_timeout(wdt, 0, dev);
>> +
>> +    status = ltq_wdt_r32(priv, LTQ_WDT_CR);
>> +    if (status & LTQ_WDT_CR_GEN) {
>> +        /*
>> +         * If the watchdog is already running overwrite it with our
>> +         * new settings. Stop is not needed as the start call will
>> +         * replace all settings anyway.
>> +         */
>> +        ltq_wdt_start(wdt);
>> +        set_bit(WDOG_HW_RUNNING, &wdt->status);
>> +    }
>>   -    return 0;
>> +    return devm_watchdog_register_device(dev, wdt);
>>   }
>>   +static const struct ltq_wdt_hw ltq_wdt_xrx100 = {
>> +    .bootstatus_get = ltq_wdt_bootstatus_xrx,
>> +};
>> +
>> +static const struct ltq_wdt_hw ltq_wdt_falcon = {
>> +    .bootstatus_get = ltq_wdt_bootstatus_falcon,
>> +};
>> +
>>   static const struct of_device_id ltq_wdt_match[] = {
>> -    { .compatible = "lantiq,wdt", .data = NULL},
>> -    { .compatible = "lantiq,xrx100-wdt", .data =
>> ltq_wdt_bootstatus_xrx },
>> -    { .compatible = "lantiq,falcon-wdt", .data =
>> ltq_wdt_bootstatus_falcon },
>> +    { .compatible = "lantiq,wdt", .data = NULL },
>> +    { .compatible = "lantiq,xrx100-wdt", .data = &ltq_wdt_xrx100 },
>> +    { .compatible = "lantiq,falcon-wdt", .data = &ltq_wdt_falcon },
>>       {},
>>   };
>>   MODULE_DEVICE_TABLE(of, ltq_wdt_match);
>>     static struct platform_driver ltq_wdt_driver = {
>>       .probe = ltq_wdt_probe,
>> -    .remove = ltq_wdt_remove,
>>       .driver = {
>>           .name = "wdt",
>>           .of_match_table = ltq_wdt_match,
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] wdt: lantiq: Convert to watchdog_device

[Guenter Roeck]

On 09/09/2018 02:07 PM, Hauke Mehrtens wrote:
[ ... ]>
>>> +    if (!priv->clk_rate) {
>>> +        dev_err(dev, "clock has wrong value of 0");
>>
>> This is misleading: It is not known if the clock rate is indeed 0.
>> It is only known that the clock rate is below LTQ_WDT_DIVIDER.
> 
> Ok I will update this.
> 
>>> +        return -EIO;
>>
>> I am not happy with EIO in this case. There is no indication
>> of an I/O error.
> 
> Is -EINVAL better?
> 

Yes.

Thanks,
Guenter