[PATCH 0/4] watchdog: MAX63xx cleanup and platform settings

[PATCH 0/4] watchdog: MAX63xx cleanup and platform settings

[Vivien Didelot]

The purpose of this patchset is to add a platform data structure for the
max63xx_wdt driver in order to setup the device in a platform code. This is
especially handy if the driver is built-in and/or the device registers aren't
memory mapped.

First, fix the Kconfig entry to allow building the support for architectures
other than ARM. Then clean up the driver to help distinguish different device
connections and to ease the introduction of new features: support for GPIO
wired MAX63xx devices and heartbeat as a platform setting.

Tested with a GPIO wired MAX6373 on an Atom platform, with a built-in driver.

Vivien Didelot (4):
  watchdog: MAX63XX_WATCHDOG does not depend on ARM
  watchdog: max63xx: cleanup
  watchdog: max63xx: add GPIO support
  watchdog: max63xx: add heartbeat to platform data

 drivers/watchdog/Kconfig                  |   9 +-
 drivers/watchdog/max63xx_wdt.c            | 297 +++++++++++++++++++-----------
 include/linux/platform_data/max63xx_wdt.h |  29 +++
 3 files changed, 226 insertions(+), 109 deletions(-)
 create mode 100644 include/linux/platform_data/max63xx_wdt.h

-- 
2.2.2

[PATCH 1/4] watchdog: MAX63XX_WATCHDOG does not depend on ARM

[Vivien Didelot]

Remove the ARM Kconfig dependency since the Maxim MAX63xx devices are
architecture independent.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 08f41ad..40ea835 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -409,7 +409,7 @@ config TS72XX_WATCHDOG
 
 config MAX63XX_WATCHDOG
 	tristate "Max63xx watchdog"
-	depends on ARM && HAS_IOMEM
+	depends on HAS_IOMEM
 	select WATCHDOG_CORE
 	help
 	  Support for memory mapped max63{69,70,71,72,73,74} watchdog timer.
-- 
2.2.2

[PATCH 2/4] watchdog: max63xx: cleanup

[Vivien Didelot]

This patch cleans up the MAX63xx driver with the following:

  * better device description;
  * put module parameter just above their declaration for clarity;
  * remove global variables in favor of a driver data structure;
  * fix "quoted string split across lines" warning from checkpatch.pl;
  * constify timeouts;
  * factorize the SET write operations;
  * few typos and comments...

This will help introduce new features to the driver.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/watchdog/Kconfig       |   7 +-
 drivers/watchdog/max63xx_wdt.c | 207 ++++++++++++++++++++---------------------
 2 files changed, 108 insertions(+), 106 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 40ea835..fc10451 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -408,11 +408,14 @@ config TS72XX_WATCHDOG
 	  module will be called ts72xx_wdt.
 
 config MAX63XX_WATCHDOG
-	tristate "Max63xx watchdog"
+	tristate "Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles"
 	depends on HAS_IOMEM
 	select WATCHDOG_CORE
 	help
-	  Support for memory mapped max63{69,70,71,72,73,74} watchdog timer.
+	  Support for MAX6369, MAX6370, MAX6371, MAX6372, MAX6373, and MAX6374.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called max63xx_wdt.
 
 config IMX2_WDT
 	tristate "IMX2+ Watchdog"
diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index 08da311..1d77669 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -1,7 +1,5 @@
 /*
- * drivers/char/watchdog/max63xx_wdt.c
- *
- * Driver for max63{69,70,71,72,73,74} watchdog timers
+ * Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles
  *
  * Copyright (C) 2009 Marc Zyngier <maz@misterjones.org>
  *
@@ -26,23 +24,33 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 
-#define DEFAULT_HEARTBEAT 60
-#define MAX_HEARTBEAT     60
+#define DEFAULT_HEARTBEAT	60
+#define MAX_HEARTBEAT		60
 
-static unsigned int heartbeat = DEFAULT_HEARTBEAT;
-static bool nowayout  = WATCHDOG_NOWAYOUT;
+#define MAX63XX_DISABLED	3
 
-/*
- * Memory mapping: a single byte, 3 first lower bits to select bit 3
- * to ping the watchdog.
- */
-#define MAX6369_WDSET	(7 << 0)
-#define MAX6369_WDI	(1 << 3)
+static unsigned int heartbeat = DEFAULT_HEARTBEAT;
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat period in seconds from 1 to "
+		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
+		 __MODULE_STRING(DEFAULT_HEARTBEAT));
 
-static DEFINE_SPINLOCK(io_lock);
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 static int nodelay;
-static void __iomem	*wdt_base;
+module_param(nodelay, int, 0);
+MODULE_PARM_DESC(nodelay, "Force selection of a timeout setting without initial delay (MAX6373/74 only, default=0)");
+
+struct max63xx_data {
+	const struct max63xx_timeout *timeout;
+
+	/* For memory mapped pins */
+	void __iomem	*base;
+	spinlock_t	lock;
+};
 
 /*
  * The timeout values used are actually the absolute minimum the chip
@@ -50,34 +58,32 @@ static void __iomem	*wdt_base;
  * (10s setting ends up with a 25s timeout), and can be up to 3 times
  * the nominal setting (according to the datasheet). So please take
  * these values with a grain of salt. Same goes for the initial delay
- * "feature". Only max6373/74 have a few settings without this initial
+ * "feature". Only MAX6373/74 have a few settings without this initial
  * delay (selected with the "nodelay" parameter).
  *
  * I also decided to remove from the tables any timeout smaller than a
- * second, as it looked completly overkill...
+ * second, as it looked completely overkill...
  */
-
-/* Timeouts in second */
 struct max63xx_timeout {
-	u8 wdset;
-	u8 tdelay;
-	u8 twd;
+	const u8 wdset;
+	const u8 tdelay;
+	const u8 twd;
 };
 
-static struct max63xx_timeout max6369_table[] = {
+static const struct max63xx_timeout max6369_table[] = {
 	{ 5,  1,  1 },
 	{ 6, 10, 10 },
 	{ 7, 60, 60 },
 	{ },
 };
 
-static struct max63xx_timeout max6371_table[] = {
+static const struct max63xx_timeout max6371_table[] = {
 	{ 6, 60,  3 },
 	{ 7, 60, 60 },
 	{ },
 };
 
-static struct max63xx_timeout max6373_table[] = {
+static const struct max63xx_timeout max6373_table[] = {
 	{ 2, 60,  1 },
 	{ 5,  0,  1 },
 	{ 1,  3,  3 },
@@ -86,8 +92,6 @@ static struct max63xx_timeout max6373_table[] = {
 	{ },
 };
 
-static struct max63xx_timeout *current_timeout;
-
 static struct max63xx_timeout *
 max63xx_select_timeout(struct max63xx_timeout *table, int value)
 {
@@ -106,111 +110,124 @@ max63xx_select_timeout(struct max63xx_timeout *table, int value)
 	return NULL;
 }
 
-static int max63xx_wdt_ping(struct watchdog_device *wdd)
+static int max63xx_mmap_ping(struct watchdog_device *wdev)
 {
+	struct max63xx_data *data = watchdog_get_drvdata(wdev);
 	u8 val;
 
-	spin_lock(&io_lock);
+	spin_lock(&data->lock);
 
-	val = __raw_readb(wdt_base);
+	val = __raw_readb(data->base);
 
-	__raw_writeb(val | MAX6369_WDI, wdt_base);
-	__raw_writeb(val & ~MAX6369_WDI, wdt_base);
+	/* WDI is the 4th bit of the memory mapped byte */
+	__raw_writeb(val | BIT(3), data->base);
+	__raw_writeb(val & ~BIT(3), data->base);
 
-	spin_unlock(&io_lock);
+	spin_unlock(&data->lock);
 	return 0;
 }
 
-static int max63xx_wdt_start(struct watchdog_device *wdd)
+static inline void max63xx_mmap_set(struct watchdog_device *wdev, u8 set)
 {
-	struct max63xx_timeout *entry = watchdog_get_drvdata(wdd);
+	struct max63xx_data *data = watchdog_get_drvdata(wdev);
 	u8 val;
 
-	spin_lock(&io_lock);
-
-	val = __raw_readb(wdt_base);
-	val &= ~MAX6369_WDSET;
-	val |= entry->wdset;
-	__raw_writeb(val, wdt_base);
+	spin_lock(&data->lock);
 
-	spin_unlock(&io_lock);
+	/* SET0, SET1, SET2 are the 3 lower bits of the memory mapped byte */
+	val = __raw_readb(data->base);
+	val &= ~7;
+	val |= set & 7;
+	__raw_writeb(val, data->base);
 
-	/* check for a edge triggered startup */
-	if (entry->tdelay == 0)
-		max63xx_wdt_ping(wdd);
-	return 0;
+	spin_unlock(&data->lock);
 }
 
-static int max63xx_wdt_stop(struct watchdog_device *wdd)
+static int max63xx_mmap_start(struct watchdog_device *wdev)
 {
-	u8 val;
-
-	spin_lock(&io_lock);
+	struct max63xx_data *data = watchdog_get_drvdata(wdev);
 
-	val = __raw_readb(wdt_base);
-	val &= ~MAX6369_WDSET;
-	val |= 3;
-	__raw_writeb(val, wdt_base);
+	max63xx_mmap_set(wdev, data->timeout->wdset);
 
-	spin_unlock(&io_lock);
+	/* Check for an edge triggered startup */
+	if (data->timeout->tdelay == 0)
+		max63xx_mmap_ping(wdev);
 	return 0;
 }
 
-static const struct watchdog_info max63xx_wdt_info = {
-	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
-	.identity = "max63xx Watchdog",
-};
+static int max63xx_mmap_stop(struct watchdog_device *wdev)
+{
+	max63xx_mmap_set(wdev, MAX63XX_DISABLED);
+	return 0;
+}
 
-static const struct watchdog_ops max63xx_wdt_ops = {
+static const struct watchdog_ops max63xx_mmap_ops = {
 	.owner = THIS_MODULE,
-	.start = max63xx_wdt_start,
-	.stop = max63xx_wdt_stop,
-	.ping = max63xx_wdt_ping,
+	.start = max63xx_mmap_start,
+	.stop = max63xx_mmap_stop,
+	.ping = max63xx_mmap_ping,
 };
 
-static struct watchdog_device max63xx_wdt_dev = {
-	.info = &max63xx_wdt_info,
-	.ops = &max63xx_wdt_ops,
+static const struct watchdog_info max63xx_wdt_info = {
+	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "MAX63xx Watchdog Timer",
 };
 
 static int max63xx_wdt_probe(struct platform_device *pdev)
 {
-	struct resource	*wdt_mem;
+	struct resource *mem;
+	struct watchdog_device *wdev;
+	struct max63xx_data *data;
 	struct max63xx_timeout *table;
 
-	table = (struct max63xx_timeout *)pdev->id_entry->driver_data;
+	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
+	if (!wdev)
+		return -ENOMEM;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	table = (struct max63xx_timeout *) pdev->id_entry->driver_data;
 
 	if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
 		heartbeat = DEFAULT_HEARTBEAT;
 
-	dev_info(&pdev->dev, "requesting %ds heartbeat\n", heartbeat);
-	current_timeout = max63xx_select_timeout(table, heartbeat);
-
-	if (!current_timeout) {
-		dev_err(&pdev->dev, "unable to satisfy heartbeat request\n");
+	data->timeout = max63xx_select_timeout(table, heartbeat);
+	if (!data->timeout) {
+		dev_err(&pdev->dev, "unable to satisfy %ds heartbeat request\n",
+			heartbeat);
 		return -EINVAL;
 	}
 
 	dev_info(&pdev->dev, "using %ds heartbeat with %ds initial delay\n",
-		 current_timeout->twd, current_timeout->tdelay);
+		 data->timeout->twd, data->timeout->tdelay);
+
+	wdev->timeout = data->timeout->twd;
+	wdev->info = &max63xx_wdt_info;
 
-	heartbeat = current_timeout->twd;
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
 
-	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	wdt_base = devm_ioremap_resource(&pdev->dev, wdt_mem);
-	if (IS_ERR(wdt_base))
-		return PTR_ERR(wdt_base);
+	spin_lock_init(&data->lock);
 
-	max63xx_wdt_dev.timeout = heartbeat;
-	watchdog_set_nowayout(&max63xx_wdt_dev, nowayout);
-	watchdog_set_drvdata(&max63xx_wdt_dev, current_timeout);
+	wdev->ops = &max63xx_mmap_ops;
 
-	return watchdog_register_device(&max63xx_wdt_dev);
+	watchdog_set_drvdata(wdev, data);
+	platform_set_drvdata(pdev, wdev);
+
+	watchdog_set_nowayout(wdev, nowayout);
+
+	return watchdog_register_device(wdev);
 }
 
 static int max63xx_wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&max63xx_wdt_dev);
+	struct watchdog_device *wdev = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(wdev);
 	return 0;
 }
 
@@ -229,29 +246,11 @@ static struct platform_driver max63xx_wdt_driver = {
 	.probe		= max63xx_wdt_probe,
 	.remove		= max63xx_wdt_remove,
 	.id_table	= max63xx_id_table,
-	.driver		= {
-		.name	= "max63xx_wdt",
-	},
+	.driver.name	= "max63xx_wdt",
 };
 
 module_platform_driver(max63xx_wdt_driver);
 
 MODULE_AUTHOR("Marc Zyngier <maz@misterjones.org>");
-MODULE_DESCRIPTION("max63xx Watchdog Driver");
-
-module_param(heartbeat, int, 0);
-MODULE_PARM_DESC(heartbeat,
-		 "Watchdog heartbeat period in seconds from 1 to "
-		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
-		 __MODULE_STRING(DEFAULT_HEARTBEAT));
-
-module_param(nowayout, bool, 0);
-MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
-		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-
-module_param(nodelay, int, 0);
-MODULE_PARM_DESC(nodelay,
-		 "Force selection of a timeout setting without initial delay "
-		 "(max6373/74 only, default=0)");
-
+MODULE_DESCRIPTION("Maxim MAX63xx Watchdog Timer driver");
 MODULE_LICENSE("GPL");
-- 
2.2.2

[PATCH 3/4] watchdog: max63xx: add GPIO support

[Vivien Didelot]

This patch adds support for GPIO wired MAX63xx devices. It adds a
platform data structure which can be filled by a platform code with the
GPIO line numbers. The driver takes care of requesting and release them.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/watchdog/Kconfig                  |   2 +-
 drivers/watchdog/max63xx_wdt.c            | 108 ++++++++++++++++++++++++++----
 include/linux/platform_data/max63xx_wdt.h |  27 ++++++++
 3 files changed, 123 insertions(+), 14 deletions(-)
 create mode 100644 include/linux/platform_data/max63xx_wdt.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index fc10451..153910f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -409,7 +409,7 @@ config TS72XX_WATCHDOG
 
 config MAX63XX_WATCHDOG
 	tristate "Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles"
-	depends on HAS_IOMEM
+	depends on HAS_IOMEM || GPIOLIB
 	select WATCHDOG_CORE
 	help
 	  Support for MAX6369, MAX6370, MAX6371, MAX6372, MAX6373, and MAX6374.
diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index 1d77669..bfa7ee3 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -3,13 +3,12 @@
  *
  * Copyright (C) 2009 Marc Zyngier <maz@misterjones.org>
  *
+ * Copyright (C) 2015 Savoir-faire Linux Inc.
+ *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
  * This file is licensed under the terms of the GNU General Public
  * License version 2. This program is licensed "as is" without any
  * warranty of any kind, whether express or implied.
- *
- * This driver assumes the watchdog pins are memory mapped (as it is
- * the case for the Arcom Zeus). Should it be connected over GPIOs or
- * another interface, some abstraction will have to be introduced.
  */
 
 #include <linux/err.h>
@@ -20,6 +19,8 @@
 #include <linux/watchdog.h>
 #include <linux/bitops.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/max63xx_wdt.h>
+#include <linux/gpio.h>
 #include <linux/spinlock.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -47,6 +48,9 @@ MODULE_PARM_DESC(nodelay, "Force selection of a timeout setting without initial
 struct max63xx_data {
 	const struct max63xx_timeout *timeout;
 
+	/* Platform data for optional config, such as GPIOs */
+	struct max63xx_platform_data *pdata;
+
 	/* For memory mapped pins */
 	void __iomem	*base;
 	spinlock_t	lock;
@@ -168,6 +172,50 @@ static const struct watchdog_ops max63xx_mmap_ops = {
 	.ping = max63xx_mmap_ping,
 };
 
+static int max63xx_gpio_ping(struct watchdog_device *wdev)
+{
+	struct max63xx_data *data = watchdog_get_drvdata(wdev);
+
+	gpio_set_value_cansleep(data->pdata->wdi, 1);
+	gpio_set_value_cansleep(data->pdata->wdi, 0);
+
+	return 0;
+}
+
+static inline void max63xx_gpio_set(struct watchdog_device *wdev, u8 set)
+{
+	struct max63xx_data *data = watchdog_get_drvdata(wdev);
+
+	gpio_set_value_cansleep(data->pdata->set0, set & BIT(0));
+	gpio_set_value_cansleep(data->pdata->set1, set & BIT(1));
+	gpio_set_value_cansleep(data->pdata->set2, set & BIT(2));
+}
+
+static int max63xx_gpio_start(struct watchdog_device *wdev)
+{
+	struct max63xx_data *data = watchdog_get_drvdata(wdev);
+
+	max63xx_gpio_set(wdev, data->timeout->wdset);
+
+	/* Check for an edge triggered startup */
+	if (data->timeout->tdelay == 0)
+		max63xx_gpio_ping(wdev);
+	return 0;
+}
+
+static int max63xx_gpio_stop(struct watchdog_device *wdev)
+{
+	max63xx_gpio_set(wdev, MAX63XX_DISABLED);
+	return 0;
+}
+
+static const struct watchdog_ops max63xx_gpio_ops = {
+	.owner = THIS_MODULE,
+	.start = max63xx_gpio_start,
+	.stop = max63xx_gpio_stop,
+	.ping = max63xx_gpio_ping,
+};
+
 static const struct watchdog_info max63xx_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 	.identity = "MAX63xx Watchdog Timer",
@@ -175,7 +223,6 @@ static const struct watchdog_info max63xx_wdt_info = {
 
 static int max63xx_wdt_probe(struct platform_device *pdev)
 {
-	struct resource *mem;
 	struct watchdog_device *wdev;
 	struct max63xx_data *data;
 	struct max63xx_timeout *table;
@@ -190,6 +237,8 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
 
 	table = (struct max63xx_timeout *) pdev->id_entry->driver_data;
 
+	data->pdata = dev_get_platdata(&pdev->dev);
+
 	if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
 		heartbeat = DEFAULT_HEARTBEAT;
 
@@ -206,14 +255,47 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
 	wdev->timeout = data->timeout->twd;
 	wdev->info = &max63xx_wdt_info;
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data->base = devm_ioremap_resource(&pdev->dev, mem);
-	if (IS_ERR(data->base))
-		return PTR_ERR(data->base);
-
-	spin_lock_init(&data->lock);
-
-	wdev->ops = &max63xx_mmap_ops;
+	/* GPIO or memory mapped? */
+	if (data->pdata && data->pdata->wdi) {
+		int err;
+
+		err = devm_gpio_request_one(&pdev->dev, data->pdata->wdi,
+					    GPIOF_OUT_INIT_LOW,
+					    "MAX63XX Watchdog WDI");
+		if (err)
+			return err;
+
+		err = devm_gpio_request_one(&pdev->dev, data->pdata->set0,
+					    GPIOF_OUT_INIT_LOW,
+					    "MAX63XX Watchdog SET0");
+		if (err)
+			return err;
+
+		err = devm_gpio_request_one(&pdev->dev, data->pdata->set1,
+					    GPIOF_OUT_INIT_LOW,
+					    "MAX63XX Watchdog SET1");
+		if (err)
+			return err;
+
+		err = devm_gpio_request_one(&pdev->dev, data->pdata->set2,
+					    GPIOF_OUT_INIT_LOW,
+					    "MAX63XX Watchdog SET2");
+		if (err)
+			return err;
+
+		wdev->ops = &max63xx_gpio_ops;
+	} else {
+		struct resource *mem;
+
+		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		data->base = devm_ioremap_resource(&pdev->dev, mem);
+		if (IS_ERR(data->base))
+			return PTR_ERR(data->base);
+
+		spin_lock_init(&data->lock);
+
+		wdev->ops = &max63xx_mmap_ops;
+	}
 
 	watchdog_set_drvdata(wdev, data);
 	platform_set_drvdata(pdev, wdev);
diff --git a/include/linux/platform_data/max63xx_wdt.h b/include/linux/platform_data/max63xx_wdt.h
new file mode 100644
index 0000000..ae28024
--- /dev/null
+++ b/include/linux/platform_data/max63xx_wdt.h
@@ -0,0 +1,27 @@
+/*
+ * Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles
+ *
+ * Copyright (c) 2015 Savoir-faire Linux Inc.
+ *          Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _PDATA_MAX63XX_H
+#define _PDATA_MAX63XX_H
+
+/**
+ * struct max63xx_platform_data - MAX63xx connectivity info
+ * @wdi:	Watchdog Input GPIO number.
+ * @set0:	Watchdog SET0 GPIO number.
+ * @set1:	Watchdog SET1 GPIO number.
+ * @set2:	Watchdog SET2 GPIO number.
+ */
+struct max63xx_platform_data {
+	unsigned int wdi;
+	unsigned int set0, set1, set2;
+};
+
+#endif /* _PDATA_MAX63XX_H */
-- 
2.2.2

[PATCH 4/4] watchdog: max63xx: add heartbeat to platform data

[Vivien Didelot]

Actually, there is no way but the module parameter to set the desired
heartbeat. This patch allows a platform code to set it in the device
platform data. This is convenient for platforms and built-in drivers.

To do so, initialize heartbeat to zero to allow the module parameter to
takes precedence over the platform setting. If not set, it will still
default to DEFAULT_HEARTBEAT.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/watchdog/max63xx_wdt.c            | 8 ++++++--
 include/linux/platform_data/max63xx_wdt.h | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index bfa7ee3..e6ac163 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -30,10 +30,10 @@
 
 #define MAX63XX_DISABLED	3
 
-static unsigned int heartbeat = DEFAULT_HEARTBEAT;
+static unsigned int heartbeat;
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat period in seconds from 1 to "
-		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
+		 __MODULE_STRING(MAX_HEARTBEAT) ", default will be "
 		 __MODULE_STRING(DEFAULT_HEARTBEAT));
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -239,6 +239,10 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
 
 	data->pdata = dev_get_platdata(&pdev->dev);
 
+	/* The module parameter takes precedence over the platform setting */
+	if (!heartbeat && data->pdata && data->pdata->heartbeat)
+		heartbeat = data->pdata->heartbeat;
+
 	if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
 		heartbeat = DEFAULT_HEARTBEAT;
 
diff --git a/include/linux/platform_data/max63xx_wdt.h b/include/linux/platform_data/max63xx_wdt.h
index ae28024..8127b1a 100644
--- a/include/linux/platform_data/max63xx_wdt.h
+++ b/include/linux/platform_data/max63xx_wdt.h
@@ -14,12 +14,14 @@
 
 /**
  * struct max63xx_platform_data - MAX63xx connectivity info
+ * @heartbeat:	Watchdog heartbeat period in seconds.
  * @wdi:	Watchdog Input GPIO number.
  * @set0:	Watchdog SET0 GPIO number.
  * @set1:	Watchdog SET1 GPIO number.
  * @set2:	Watchdog SET2 GPIO number.
  */
 struct max63xx_platform_data {
+	unsigned int heartbeat;
 	unsigned int wdi;
 	unsigned int set0, set1, set2;
 };
-- 
2.2.2

Re: [PATCH 2/4] watchdog: max63xx: cleanup

[Guenter Roeck]

On 01/29/2015 09:15 AM, Vivien Didelot wrote:
> This patch cleans up the MAX63xx driver with the following:
>
>    * better device description;
>    * put module parameter just above their declaration for clarity;
>    * remove global variables in favor of a driver data structure;
>    * fix "quoted string split across lines" warning from checkpatch.pl;
>    * constify timeouts;
>    * factorize the SET write operations;
>    * few typos and comments...
>
> This will help introduce new features to the driver.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/watchdog/Kconfig       |   7 +-
>   drivers/watchdog/max63xx_wdt.c | 207 ++++++++++++++++++++---------------------
>   2 files changed, 108 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 40ea835..fc10451 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -408,11 +408,14 @@ config TS72XX_WATCHDOG
>   	  module will be called ts72xx_wdt.
>
>   config MAX63XX_WATCHDOG
> -	tristate "Max63xx watchdog"
> +	tristate "Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles"
>   	depends on HAS_IOMEM
>   	select WATCHDOG_CORE
>   	help
> -	  Support for memory mapped max63{69,70,71,72,73,74} watchdog timer.
> +	  Support for MAX6369, MAX6370, MAX6371, MAX6372, MAX6373, and MAX6374.
> +
> +	  To compile this driver as a module, choose M here:
> +	  the module will be called max63xx_wdt.
>
>   config IMX2_WDT
>   	tristate "IMX2+ Watchdog"
> diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
> index 08da311..1d77669 100644
> --- a/drivers/watchdog/max63xx_wdt.c
> +++ b/drivers/watchdog/max63xx_wdt.c
> @@ -1,7 +1,5 @@
>   /*
> - * drivers/char/watchdog/max63xx_wdt.c
> - *
> - * Driver for max63{69,70,71,72,73,74} watchdog timers
> + * Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles
>    *
>    * Copyright (C) 2009 Marc Zyngier <maz@misterjones.org>
>    *
> @@ -26,23 +24,33 @@
>   #include <linux/io.h>
>   #include <linux/slab.h>
>
> -#define DEFAULT_HEARTBEAT 60
> -#define MAX_HEARTBEAT     60
> +#define DEFAULT_HEARTBEAT	60
> +#define MAX_HEARTBEAT		60
>
> -static unsigned int heartbeat = DEFAULT_HEARTBEAT;
> -static bool nowayout  = WATCHDOG_NOWAYOUT;
> +#define MAX63XX_DISABLED	3
>
> -/*
> - * Memory mapping: a single byte, 3 first lower bits to select bit 3
> - * to ping the watchdog.
> - */
> -#define MAX6369_WDSET	(7 << 0)
> -#define MAX6369_WDI	(1 << 3)

Not really sure I understand why you remove those constants.
that is personal preference, not cleanup. Someone else might
submit another cleanup later on and re-introduce them.

Guenter

Re: [PATCH 2/4] watchdog: max63xx: cleanup

[Vivien Didelot]

Hi Guenter,

> > -#define MAX6369_WDSET	(7 << 0)
> > -#define MAX6369_WDI	(1 << 3)
> 
> Not really sure I understand why you remove those constants.
> that is personal preference, not cleanup. Someone else might
> submit another cleanup later on and re-introduce them.

Indeed, I should have explained this. My primary intention with this patchset
is to bring GPIO support to this driver. MAX6369_WDSET, which is a mask, is
specific to the memory mapped support. I first intented to rename it to
MAX6369_MMAP_WDSET to explicitly mention the connection, but I ended up
thinking that a clear comment in the related functions would be much clearer
that a generic named macro.

These masks are explained below in this patch:

In max63xx_mmap_ping():
+       /* WDI is the 4th bit of the memory mapped byte */
+       __raw_writeb(val | BIT(3), data->base);
+       __raw_writeb(val & ~BIT(3), data->base);

and max63xx_mmap_set():
+       /* SET0, SET1, SET2 are the 3 lower bits of the memory mapped byte */
+       val = __raw_readb(data->base);
+       val &= ~7;
+       val |= set & 7;
+       __raw_writeb(val, data->base);

Also, if it is possible for a platform to map these bits in a different way,
some platform settings would have to be added and thus these macros won't be
relevant.

Thanks,
-v

Re: [PATCH 0/4] watchdog: MAX63xx cleanup and platform settings

[Vivien Didelot]

Hi Wim, all,

> The purpose of this patchset is to add a platform data structure for the
> max63xx_wdt driver in order to setup the device in a platform code. This is
> especially handy if the driver is built-in and/or the device registers aren't
> memory mapped.
> 
> First, fix the Kconfig entry to allow building the support for architectures
> other than ARM. Then clean up the driver to help distinguish different device
> connections and to ease the introduction of new features: support for GPIO
> wired MAX63xx devices and heartbeat as a platform setting.
> 
> Tested with a GPIO wired MAX6373 on an Atom platform, with a built-in driver.
> 
> Vivien Didelot (4):
>   watchdog: MAX63XX_WATCHDOG does not depend on ARM
>   watchdog: max63xx: cleanup
>   watchdog: max63xx: add GPIO support
>   watchdog: max63xx: add heartbeat to platform data

Did you get any chance to look into merging this patchset?
(It still applies well on the latest watchdog-next/master tree.)

Best,
-v

Re: [1/4] watchdog: MAX63XX_WATCHDOG does not depend on ARM

[Guenter Roeck]

On Thu, Jan 29, 2015 at 12:15:42PM -0500, Vivien Didelot wrote:
> Remove the ARM Kconfig dependency since the Maxim MAX63xx devices are
> architecture independent.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

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