[PATCH v2 0/2] watchdog: f71808e_wdt: migrate to new kernel API

[PATCH v2 0/2] watchdog: f71808e_wdt: migrate to new kernel API

[Ahmad Fatoum]

This series migrates the driver to use first the driver model, then
the new kernel watchdog API.

I tested it on a f81866.

v1 had a wrong title (f71808e_wdt: migrate to kernel). It's available here:
https://lore.kernel.org/linux-watchdog/20200611191750.28096-1-a.fatoum@pengutronix.de/

v1 -> v2:
    - reworked to platform device/driver pair (Guenther)
    - squashed identifier renaming into the patches that touch
      the respective lines anyway
    - fixed checkpatch.pl nitpicks (Guenther)
    - fixed locally used variable declared without static (0-day)
    - fixed unneded line break due to old line limit (Guenther)
    - renamed struct fintek_wdog_data to struct fintek_wdt

Ahmad Fatoum (2):
  watchdog: f71808e_wdt: refactor to platform device/driver pair
  watchdog: f71808e_wdt: migrate to new kernel watchdog API

 drivers/watchdog/Kconfig       |   1 +
 drivers/watchdog/f71808e_wdt.c | 815 ++++++++++++---------------------
 2 files changed, 292 insertions(+), 524 deletions(-)

-- 
2.28.0

[PATCH v2 1/2] watchdog: f71808e_wdt: refactor to platform device/driver pair

[Ahmad Fatoum]

Driver so far wasn't ported to the driver model and set up its
miscdevice out of the init after probing the I/O ports for a watchdog
with correct vendor and device revision.

Keep the device detection part at init time, but move watchdog setup
to a platform driver probe function.

While at it, refactor some of the driver code we have to now touch
anyway:

 - platform_device_id is used instead of the two big switches mapping
   hardware ID to an enum and then mapping it to a pinconf function
 - we rename f71808e_ and watchdog_data to fintek_wdt, to avoid mix up
   of the generic parts up with the device specific parts

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/watchdog/f71808e_wdt.c | 377 +++++++++++++++++++--------------
 1 file changed, 215 insertions(+), 162 deletions(-)

diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
index f60beec1bbae..4ff7a2509125 100644
--- a/drivers/watchdog/f71808e_wdt.c
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -9,12 +9,15 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/err.h>
+#include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
@@ -110,22 +113,6 @@ module_param(start_withtimeout, uint, 0);
 MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
 	" given initial timeout. Zero (default) disables this feature.");
 
-enum chips { f71808fg, f71858fg, f71862fg, f71868, f71869, f71882fg, f71889fg,
-	     f81803, f81865, f81866};
-
-static const char *f71808e_names[] = {
-	"f71808fg",
-	"f71858fg",
-	"f71862fg",
-	"f71868",
-	"f71869",
-	"f71882fg",
-	"f71889fg",
-	"f81803",
-	"f81865",
-	"f81866",
-};
-
 /* Super-I/O Function prototypes */
 static inline int superio_inb(int base, int reg);
 static inline int superio_inw(int base, int reg);
@@ -136,9 +123,17 @@ static inline int superio_enter(int base);
 static inline void superio_select(int base, int ld);
 static inline void superio_exit(int base);
 
-struct watchdog_data {
+struct fintek_wdt;
+
+struct fintek_wdt_variant {
+	u16 id;
+	void (*pinconf)(struct fintek_wdt *wd);
+	const char *identity_override;
+};
+
+struct fintek_wdt {
 	unsigned short	sioaddr;
-	enum chips	type;
+	const struct fintek_wdt_variant *variant;
 	unsigned long	opened;
 	struct mutex	lock;
 	char		expect_close;
@@ -152,10 +147,15 @@ struct watchdog_data {
 	char		caused_reboot;	/* last reboot was by the watchdog */
 };
 
-static struct watchdog_data watchdog = {
+static struct fintek_wdt watchdog = {
 	.lock = __MUTEX_INITIALIZER(watchdog.lock),
 };
 
+static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
+{
+	return wd->variant->id == SIO_F81865_ID || wd->variant->id == SIO_F81866_ID;
+}
+
 /* Super I/O functions */
 static inline int superio_inb(int base, int reg)
 {
@@ -247,7 +247,7 @@ static int watchdog_set_pulse_width(unsigned int pw)
 	int err = 0;
 	unsigned int t1 = 25, t2 = 125, t3 = 5000;
 
-	if (watchdog.type == f71868) {
+	if (watchdog.variant->id == SIO_F71868_ID) {
 		t1 = 30;
 		t2 = 150;
 		t3 = 6000;
@@ -309,7 +309,6 @@ static int watchdog_keepalive(void)
 static int watchdog_start(void)
 {
 	int err;
-	u8 tmp;
 
 	/* Make sure we don't die as soon as the watchdog is enabled below */
 	err = watchdog_keepalive();
@@ -323,81 +322,12 @@ static int watchdog_start(void)
 	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
 
 	/* Watchdog pin configuration */
-	switch (watchdog.type) {
-	case f71808fg:
-		/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
-		break;
-
-	case f71862fg:
-		if (f71862fg_pin == 63) {
-			/* SPI must be disabled first to use this pin! */
-			superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
-			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
-		} else if (f71862fg_pin == 56) {
-			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
-		}
-		break;
-
-	case f71868:
-	case f71869:
-		/* GPIO14 --> WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 4);
-		break;
-
-	case f71882fg:
-		/* Set pin 56 to WDTRST# */
-		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
-		break;
-
-	case f71889fg:
-		/* set pin 40 to WDTRST# */
-		superio_outb(watchdog.sioaddr, SIO_REG_MFUNCT3,
-			superio_inb(watchdog.sioaddr, SIO_REG_MFUNCT3) & 0xcf);
-		break;
-
-	case f81803:
-		/* Enable TSI Level register bank */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_CLOCK_SEL, 3);
-		/* Set pin 27 to WDTRST# */
-		superio_outb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
-			superio_inb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL));
-		break;
-
-	case f81865:
-		/* Set pin 70 to WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 5);
-		break;
-
-	case f81866:
-		/*
-		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
-		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
-		 *     BIT5: 0 -> WDTRST#
-		 *           1 -> GPIO15
-		 */
-		tmp = superio_inb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL);
-		tmp &= ~(BIT(3) | BIT(0));
-		tmp |= BIT(2);
-		superio_outb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
-
-		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
-		break;
-
-	default:
-		/*
-		 * 'default' label to shut up the compiler and catch
-		 * programmer errors
-		 */
-		err = -ENODEV;
-		goto exit_superio;
-	}
+	watchdog.variant->pinconf(&watchdog);
 
 	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
 	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
 
-	if (watchdog.type == f81865 || watchdog.type == f81866)
+	if (has_f81865_wdo_conf(&watchdog))
 		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
 				F81865_FLAG_WDOUT_EN);
 	else
@@ -425,7 +355,6 @@ static int watchdog_start(void)
 				F71808FG_FLAG_WD_PULSE);
 	}
 
-exit_superio:
 	superio_exit(watchdog.sioaddr);
 exit_unlock:
 	mutex_unlock(&watchdog.lock);
@@ -665,21 +594,39 @@ static struct notifier_block watchdog_notifier = {
 	.notifier_call = watchdog_notify_sys,
 };
 
-static int __init watchdog_init(int sioaddr)
+static int fintek_wdt_probe(struct platform_device *pdev)
 {
+	const struct platform_device_id *id;
+	unsigned short sioaddr;
+	struct resource *res;
 	int wdt_conf, err = 0;
+	const char *identity;
+
+	if (watchdog_miscdev.parent)
+		return -EBUSY;
+
+	watchdog_miscdev.parent = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res)
+		return -ENXIO;
+
+	id = platform_get_device_id(pdev);
+	watchdog.variant = (void *)id->driver_data;
+	if (!watchdog.variant)
+		return -ENODEV;
 
 	/* No need to lock watchdog.lock here because no entry points
 	 * into the module have been registered yet.
 	 */
-	watchdog.sioaddr = sioaddr;
+	watchdog.sioaddr = res->start;
 	watchdog.ident.options = WDIOF_MAGICCLOSE
 				| WDIOF_KEEPALIVEPING
 				| WDIOF_CARDRESET;
 
-	snprintf(watchdog.ident.identity,
-		sizeof(watchdog.ident.identity), "%s watchdog",
-		f71808e_names[watchdog.type]);
+	identity = watchdog.variant->identity_override ?: id->name;
+
+	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
 
 	err = superio_enter(sioaddr);
 	if (err)
@@ -772,73 +719,176 @@ static int __init watchdog_init(int sioaddr)
 	return err;
 }
 
-static int __init f71808e_find(int sioaddr)
+static void f71808fg_pinconf(struct fintek_wdt *wd)
 {
+	/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT2, 3);
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 3);
+}
+
+static void f71862fg_pinconf(struct fintek_wdt *wd)
+{
+	if (f71862fg_pin == 63) {
+		/* SPI must be disabled first to use this pin! */
+		superio_clear_bit(wd->sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
+		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT3, 4);
+	} else if (f71862fg_pin == 56) {
+		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
+	}
+}
+
+static void f71868_pinconf(struct fintek_wdt *wd)
+{
+	/* GPIO14 --> WDTRST# */
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT1, 4);
+}
+
+static void f71882fg_pinconf(struct fintek_wdt *wd)
+{
+	/* Set pin 56 to WDTRST# */
+	superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
+}
+
+static void f71889fg_pinconf(struct fintek_wdt *wd)
+{
+	/* set pin 40 to WDTRST# */
+	superio_outb(wd->sioaddr, SIO_REG_MFUNCT3,
+		     superio_inb(wd->sioaddr, SIO_REG_MFUNCT3) & 0xcf);
+}
+
+static void f81803_pinconf(struct fintek_wdt *wd)
+{
+	/* Enable TSI Level register bank */
+	superio_clear_bit(wd->sioaddr, SIO_REG_CLOCK_SEL, 3);
+	/* Set pin 27 to WDTRST# */
+	superio_outb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
+		     superio_inb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL));
+}
+
+static void f81865_pinconf(struct fintek_wdt *wd)
+{
+	/* Set pin 70 to WDTRST# */
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 5);
+}
+
+static void f81866_pinconf(struct fintek_wdt *wd)
+{
+	u8 tmp;
+	/*
+	 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
+	 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
+	 *     BIT5: 0 -> WDTRST#
+	 *           1 -> GPIO15
+	 */
+	tmp = superio_inb(wd->sioaddr, SIO_F81866_REG_PORT_SEL);
+	tmp &= ~(BIT(3) | BIT(0));
+	tmp |= BIT(2);
+	superio_outb(wd->sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
+
+	superio_clear_bit(wd->sioaddr, SIO_F81866_REG_GPIO1, 5);
+}
+
+#define FINTEK_DRV_DATA(...) \
+	(kernel_ulong_t)(&(struct fintek_wdt_variant) { __VA_ARGS__ })
+
+/*
+ * Array is used by both platform bus to map name to driver data and by
+ * init below to map id in driver_data to name for device creation.
+ */
+static const struct platform_device_id fintek_wdt_id_table[] = {
+	{ "f71808fg watchdog", FINTEK_DRV_DATA(SIO_F71808_ID,  f71808fg_pinconf) },
+	{ "f71862fg watchdog", FINTEK_DRV_DATA(SIO_F71862_ID,  f71862fg_pinconf) },
+	{ "f71868 watchdog",   FINTEK_DRV_DATA(SIO_F71868_ID,  f71868_pinconf) },
+	{ "f71869 watchdog",   FINTEK_DRV_DATA(SIO_F71869_ID,  f71868_pinconf) },
+	/* For backwards-compatibility, we define a different watchdog identity name */
+	{ "f71869a watchdog",  FINTEK_DRV_DATA(SIO_F71869A_ID, f71868_pinconf,
+					       .identity_override = "f71869 watchdog") },
+	{ "f71882fg watchdog", FINTEK_DRV_DATA(SIO_F71882_ID,  f71882fg_pinconf) },
+	{ "f71889fg watchdog", FINTEK_DRV_DATA(SIO_F71889_ID,  f71889fg_pinconf) },
+	{ "f81803 watchdog",   FINTEK_DRV_DATA(SIO_F81803_ID,  f81803_pinconf) },
+	{ "f81865 watchdog",   FINTEK_DRV_DATA(SIO_F81865_ID,  f81865_pinconf) },
+	{ "f81866 watchdog",   FINTEK_DRV_DATA(SIO_F81866_ID,  f81866_pinconf) },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
+
+/* Confirmed (by datasheet) not to have a watchdog.
+ * We list this here anyway to maintain backwards-compatibility in not warning about them
+ */
+static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
+
+static int fintek_wdt_remove(struct platform_device *pdev)
+{
+	if (watchdog_is_running()) {
+		pr_warn("Watchdog timer still running, stopping it\n");
+		watchdog_stop();
+	}
+	misc_deregister(&watchdog_miscdev);
+	unregister_reboot_notifier(&watchdog_notifier);
+	return 0;
+}
+
+static struct platform_driver fintek_wdt_driver = {
+	.probe		= fintek_wdt_probe,
+	.remove		= fintek_wdt_remove,
+	.id_table	= fintek_wdt_id_table,
+	.driver		= {
+		.name	= DRVNAME,
+	},
+};
+
+static struct platform_device *fintek_wdt_pdev;
+
+static __init const char *fintek_wdt_find(int sioaddr)
+{
+	const char *name = ERR_PTR(-ENODEV);
+	const struct platform_device_id *id;
+	struct fintek_wdt_variant *variant;
 	u16 devid;
-	int err = superio_enter(sioaddr);
+	const u16 *pdevid;
+	int err;
+
+	err = superio_enter(sioaddr);
 	if (err)
-		return err;
+		return ERR_PTR(err);
 
 	devid = superio_inw(sioaddr, SIO_REG_MANID);
 	if (devid != SIO_FINTEK_ID) {
 		pr_debug("Not a Fintek device\n");
-		err = -ENODEV;
-		goto exit;
+		goto out;
 	}
 
 	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
-	switch (devid) {
-	case SIO_F71808_ID:
-		watchdog.type = f71808fg;
-		break;
-	case SIO_F71862_ID:
-		watchdog.type = f71862fg;
-		break;
-	case SIO_F71868_ID:
-		watchdog.type = f71868;
-		break;
-	case SIO_F71869_ID:
-	case SIO_F71869A_ID:
-		watchdog.type = f71869;
-		break;
-	case SIO_F71882_ID:
-		watchdog.type = f71882fg;
-		break;
-	case SIO_F71889_ID:
-		watchdog.type = f71889fg;
-		break;
-	case SIO_F71858_ID:
-		/* Confirmed (by datasheet) not to have a watchdog. */
-		err = -ENODEV;
-		goto exit;
-	case SIO_F81803_ID:
-		watchdog.type = f81803;
-		break;
-	case SIO_F81865_ID:
-		watchdog.type = f81865;
-		break;
-	case SIO_F81866_ID:
-		watchdog.type = f81866;
-		break;
-	default:
-		pr_info("Unrecognized Fintek device: %04x\n",
-			(unsigned int)devid);
-		err = -ENODEV;
-		goto exit;
+	for (id = fintek_wdt_id_table; id->driver_data; id++) {
+		variant = (void *)id->driver_data;
+		if (variant->id == devid) {
+			name = id->name;
+			pr_info("Found %s chip, revision %d\n",
+				name, superio_inb(sioaddr, SIO_REG_DEVREV));
+			goto out;
+		}
+	}
+
+	for (pdevid = fintek_no_wdt_table; *pdevid; pdevid++) {
+		if (*pdevid == devid)
+			goto out;
 	}
 
-	pr_info("Found %s watchdog chip, revision %d\n",
-		f71808e_names[watchdog.type],
-		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
-exit:
+	pr_info("Unrecognized Fintek device: %04x\n", devid);
+out:
 	superio_exit(sioaddr);
-	return err;
+	return name;
 }
 
-static int __init f71808e_init(void)
+static struct resource wdt_res = {
+	.name = "superio port",
+	.flags = IORESOURCE_IO,
+};
+
+static int __init fintek_wdt_init(void)
 {
-	static const unsigned short addrs[] = { 0x2e, 0x4e };
-	int err = -ENODEV;
+	unsigned short addrs[] = { 0x2e, 0x4e };
+	const char *wdt_name;
 	int i;
 
 	if (f71862fg_pin != 63 && f71862fg_pin != 56) {
@@ -846,30 +896,33 @@ static int __init f71808e_init(void)
 		return -EINVAL;
 	}
 
+	platform_driver_register(&fintek_wdt_driver);
+
 	for (i = 0; i < ARRAY_SIZE(addrs); i++) {
-		err = f71808e_find(addrs[i]);
-		if (err == 0)
+		wdt_name = fintek_wdt_find(addrs[i]);
+		if (!IS_ERR(wdt_name))
 			break;
 	}
+
 	if (i == ARRAY_SIZE(addrs))
-		return err;
+		return PTR_ERR(wdt_name);
+
+	wdt_res.start = addrs[i];
+	wdt_res.end = wdt_res.start + 1;
 
-	return watchdog_init(addrs[i]);
+	fintek_wdt_pdev = platform_device_register_simple(wdt_name, -1, &wdt_res, 1);
+	return PTR_ERR_OR_ZERO(fintek_wdt_pdev);
 }
 
-static void __exit f71808e_exit(void)
+static void __exit fintek_wdt_exit(void)
 {
-	if (watchdog_is_running()) {
-		pr_warn("Watchdog timer still running, stopping it\n");
-		watchdog_stop();
-	}
-	misc_deregister(&watchdog_miscdev);
-	unregister_reboot_notifier(&watchdog_notifier);
+	platform_device_unregister(fintek_wdt_pdev);
+	platform_driver_unregister(&fintek_wdt_driver);
 }
 
 MODULE_DESCRIPTION("F71808E Watchdog Driver");
 MODULE_AUTHOR("Giel van Schijndel <me@mortis.eu>");
 MODULE_LICENSE("GPL");
 
-module_init(f71808e_init);
-module_exit(f71808e_exit);
+module_init(fintek_wdt_init);
+module_exit(fintek_wdt_exit);
-- 
2.28.0

[PATCH v2 2/2] watchdog: f71808e_wdt: migrate to new kernel watchdog API

[Ahmad Fatoum]

Migrating the driver lets us drop the watchdog misc device boilerplate
and reduces size by 280~ lines. It also brings us support for new
functionality like CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED.

While at it, also rename all local identifiers starting with watchdog_
to start with easier to tell apart fintek_wdt_ instead.

This incurs a slight backwards-compatibility break, because the new
kernel watchdog API doesn't support unloading modules for drivers
whose watchdog hardware is reported to be running.

This means following scenario will be no longer supported:
 - BIOS has enabled watchdog
 - Module is loaded and unloaded without opening watchdog
 - module_exit is expected to succeed and disable watchdog HW

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/watchdog/Kconfig       |   1 +
 drivers/watchdog/f71808e_wdt.c | 514 ++++++++-------------------------
 2 files changed, 115 insertions(+), 400 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ab7aad5a1e69..81f9817b291c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1066,6 +1066,7 @@ config EBC_C384_WDT
 config F71808E_WDT
 	tristate "Fintek F718xx, F818xx Super I/O Watchdog"
 	depends on X86
+	select WATCHDOG_CORE
 	help
 	  This is the driver for the hardware watchdog on the Fintek F71808E,
 	  F71862FG, F71868, F71869, F71882FG, F71889FG, F81803, F81865, and
diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
index 4ff7a2509125..32e759356354 100644
--- a/drivers/watchdog/f71808e_wdt.c
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -3,6 +3,7 @@
  *   Copyright (C) 2006 by Hans Edgington <hans@edgington.nl>              *
  *   Copyright (C) 2007-2009 Hans de Goede <hdegoede@redhat.com>           *
  *   Copyright (C) 2010 Giel van Schijndel <me@mortis.eu>                  *
+ *   Copyright (C) 2020 Ahmad Fatoum <a.fatoum@pengutronix.de>             *
  *                                                                         *
  ***************************************************************************/
 
@@ -10,18 +11,12 @@
 
 #include <linux/err.h>
 #include <linux/slab.h>
-#include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/mod_devicetable.h>
-#include <linux/mutex.h>
-#include <linux/notifier.h>
-#include <linux/reboot.h>
-#include <linux/uaccess.h>
 #include <linux/watchdog.h>
 
 #define DRVNAME "f71808e_wdt"
@@ -132,23 +127,15 @@ struct fintek_wdt_variant {
 };
 
 struct fintek_wdt {
+	struct watchdog_device wdd;
 	unsigned short	sioaddr;
 	const struct fintek_wdt_variant *variant;
-	unsigned long	opened;
-	struct mutex	lock;
-	char		expect_close;
 	struct watchdog_info ident;
 
-	unsigned short	timeout;
 	u8		timer_val;	/* content for the wd_time register */
 	char		minutes_mode;
 	u8		pulse_val;	/* pulse width flag */
 	char		pulse_mode;	/* enable pulse output mode? */
-	char		caused_reboot;	/* last reboot was by the watchdog */
-};
-
-static struct fintek_wdt watchdog = {
-	.lock = __MUTEX_INITIALIZER(watchdog.lock),
 };
 
 static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
@@ -218,505 +205,244 @@ static inline void superio_exit(int base)
 	release_region(base, 2);
 }
 
-static int watchdog_set_timeout(int timeout)
+static int fintek_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
 {
-	if (timeout <= 0
-	 || timeout >  max_timeout) {
-		pr_err("watchdog timeout out of range\n");
-		return -EINVAL;
-	}
+	struct fintek_wdt *wd = watchdog_get_drvdata(wdd);
 
-	mutex_lock(&watchdog.lock);
-
-	watchdog.timeout = timeout;
+	wdd->timeout = timeout;
 	if (timeout > 0xff) {
-		watchdog.timer_val = DIV_ROUND_UP(timeout, 60);
-		watchdog.minutes_mode = true;
+		wd->timer_val = DIV_ROUND_UP(timeout, 60);
+		wd->minutes_mode = true;
 	} else {
-		watchdog.timer_val = timeout;
-		watchdog.minutes_mode = false;
+		wd->timer_val = timeout;
+		wd->minutes_mode = false;
 	}
 
-	mutex_unlock(&watchdog.lock);
-
 	return 0;
 }
 
-static int watchdog_set_pulse_width(unsigned int pw)
+static int fintek_wdt_set_pulse_width(struct fintek_wdt *wd, unsigned int pw)
 {
-	int err = 0;
 	unsigned int t1 = 25, t2 = 125, t3 = 5000;
 
-	if (watchdog.variant->id == SIO_F71868_ID) {
+	if (wd->variant->id == SIO_F71868_ID) {
 		t1 = 30;
 		t2 = 150;
 		t3 = 6000;
 	}
 
-	mutex_lock(&watchdog.lock);
-
 	if        (pw <=  1) {
-		watchdog.pulse_val = 0;
+		wd->pulse_val = 0;
 	} else if (pw <= t1) {
-		watchdog.pulse_val = 1;
+		wd->pulse_val = 1;
 	} else if (pw <= t2) {
-		watchdog.pulse_val = 2;
+		wd->pulse_val = 2;
 	} else if (pw <= t3) {
-		watchdog.pulse_val = 3;
+		wd->pulse_val = 3;
 	} else {
 		pr_err("pulse width out of range\n");
-		err = -EINVAL;
-		goto exit_unlock;
+		return -EINVAL;
 	}
 
-	watchdog.pulse_mode = pw;
+	wd->pulse_mode = pw;
 
-exit_unlock:
-	mutex_unlock(&watchdog.lock);
-	return err;
+	return 0;
 }
 
-static int watchdog_keepalive(void)
+static int fintek_wdt_keepalive(struct watchdog_device *wdd)
 {
-	int err = 0;
+	struct fintek_wdt *wd = watchdog_get_drvdata(wdd);
+	int err;
 
-	mutex_lock(&watchdog.lock);
-	err = superio_enter(watchdog.sioaddr);
+	err = superio_enter(wd->sioaddr);
 	if (err)
-		goto exit_unlock;
-	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+		return err;
+	superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT);
 
-	if (watchdog.minutes_mode)
+	if (wd->minutes_mode)
 		/* select minutes for timer units */
-		superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+		superio_set_bit(wd->sioaddr, F71808FG_REG_WDT_CONF,
 				F71808FG_FLAG_WD_UNIT);
 	else
 		/* select seconds for timer units */
-		superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
-				F71808FG_FLAG_WD_UNIT);
+		superio_clear_bit(wd->sioaddr, F71808FG_REG_WDT_CONF,
+				  F71808FG_FLAG_WD_UNIT);
 
 	/* Set timer value */
-	superio_outb(watchdog.sioaddr, F71808FG_REG_WD_TIME,
-			   watchdog.timer_val);
+	superio_outb(wd->sioaddr, F71808FG_REG_WD_TIME,
+		     wd->timer_val);
 
-	superio_exit(watchdog.sioaddr);
+	superio_exit(wd->sioaddr);
 
-exit_unlock:
-	mutex_unlock(&watchdog.lock);
-	return err;
+	return 0;
 }
 
-static int watchdog_start(void)
+static int fintek_wdt_start(struct watchdog_device *wdd)
 {
+	struct fintek_wdt *wd = watchdog_get_drvdata(wdd);
 	int err;
 
 	/* Make sure we don't die as soon as the watchdog is enabled below */
-	err = watchdog_keepalive();
+	err = fintek_wdt_keepalive(wdd);
 	if (err)
 		return err;
 
-	mutex_lock(&watchdog.lock);
-	err = superio_enter(watchdog.sioaddr);
+	err = superio_enter(wd->sioaddr);
 	if (err)
-		goto exit_unlock;
-	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+		return err;
+	superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT);
 
 	/* Watchdog pin configuration */
-	watchdog.variant->pinconf(&watchdog);
+	wd->variant->pinconf(wd);
 
-	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
-	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
+	superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT);
+	superio_set_bit(wd->sioaddr, SIO_REG_ENABLE, 0);
 
-	if (has_f81865_wdo_conf(&watchdog))
-		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
+	if (has_f81865_wdo_conf(wd))
+		superio_set_bit(wd->sioaddr, F81865_REG_WDO_CONF,
 				F81865_FLAG_WDOUT_EN);
 	else
-		superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDO_CONF,
+		superio_set_bit(wd->sioaddr, F71808FG_REG_WDO_CONF,
 				F71808FG_FLAG_WDOUT_EN);
 
-	superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+	superio_set_bit(wd->sioaddr, F71808FG_REG_WDT_CONF,
 			F71808FG_FLAG_WD_EN);
 
-	if (watchdog.pulse_mode) {
+	set_bit(WDOG_HW_RUNNING, &wdd->status);
+
+	if (wd->pulse_mode) {
 		/* Select "pulse" output mode with given duration */
-		u8 wdt_conf = superio_inb(watchdog.sioaddr,
-				F71808FG_REG_WDT_CONF);
+		u8 wdt_conf = superio_inb(wd->sioaddr, F71808FG_REG_WDT_CONF);
 
 		/* Set WD_PSWIDTH bits (1:0) */
-		wdt_conf = (wdt_conf & 0xfc) | (watchdog.pulse_val & 0x03);
+		wdt_conf = (wdt_conf & 0xfc) | (wd->pulse_val & 0x03);
 		/* Set WD_PULSE to "pulse" mode */
 		wdt_conf |= BIT(F71808FG_FLAG_WD_PULSE);
 
-		superio_outb(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
-				wdt_conf);
+		superio_outb(wd->sioaddr, F71808FG_REG_WDT_CONF, wdt_conf);
 	} else {
 		/* Select "level" output mode */
-		superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
-				F71808FG_FLAG_WD_PULSE);
+		superio_clear_bit(wd->sioaddr, F71808FG_REG_WDT_CONF,
+				  F71808FG_FLAG_WD_PULSE);
 	}
 
-	superio_exit(watchdog.sioaddr);
-exit_unlock:
-	mutex_unlock(&watchdog.lock);
+	superio_exit(wd->sioaddr);
 
 	return err;
 }
 
-static int watchdog_stop(void)
-{
-	int err = 0;
-
-	mutex_lock(&watchdog.lock);
-	err = superio_enter(watchdog.sioaddr);
-	if (err)
-		goto exit_unlock;
-	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
-
-	superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
-			F71808FG_FLAG_WD_EN);
-
-	superio_exit(watchdog.sioaddr);
-
-exit_unlock:
-	mutex_unlock(&watchdog.lock);
-
-	return err;
-}
-
-static int watchdog_get_status(void)
-{
-	int status = 0;
-
-	mutex_lock(&watchdog.lock);
-	status = (watchdog.caused_reboot) ? WDIOF_CARDRESET : 0;
-	mutex_unlock(&watchdog.lock);
-
-	return status;
-}
-
-static bool watchdog_is_running(void)
-{
-	/*
-	 * if we fail to determine the watchdog's status assume it to be
-	 * running to be on the safe side
-	 */
-	bool is_running = true;
-
-	mutex_lock(&watchdog.lock);
-	if (superio_enter(watchdog.sioaddr))
-		goto exit_unlock;
-	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
-
-	is_running = (superio_inb(watchdog.sioaddr, SIO_REG_ENABLE) & BIT(0))
-		&& (superio_inb(watchdog.sioaddr, F71808FG_REG_WDT_CONF)
-			& BIT(F71808FG_FLAG_WD_EN));
-
-	superio_exit(watchdog.sioaddr);
-
-exit_unlock:
-	mutex_unlock(&watchdog.lock);
-	return is_running;
-}
-
-/* /dev/watchdog api */
-
-static int watchdog_open(struct inode *inode, struct file *file)
+static int fintek_wdt_stop(struct watchdog_device *wdd)
 {
+	struct fintek_wdt *wd = watchdog_get_drvdata(wdd);
 	int err;
 
-	/* If the watchdog is alive we don't need to start it again */
-	if (test_and_set_bit(0, &watchdog.opened))
-		return -EBUSY;
-
-	err = watchdog_start();
-	if (err) {
-		clear_bit(0, &watchdog.opened);
+	err = superio_enter(wd->sioaddr);
+	if (err)
 		return err;
-	}
+	superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT);
 
-	if (nowayout)
-		__module_get(THIS_MODULE);
-
-	watchdog.expect_close = 0;
-	return stream_open(inode, file);
-}
+	superio_clear_bit(wd->sioaddr, F71808FG_REG_WDT_CONF,
+			  F71808FG_FLAG_WD_EN);
 
-static int watchdog_release(struct inode *inode, struct file *file)
-{
-	clear_bit(0, &watchdog.opened);
+	superio_exit(wd->sioaddr);
 
-	if (!watchdog.expect_close) {
-		watchdog_keepalive();
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-	} else if (!nowayout) {
-		watchdog_stop();
-	}
 	return 0;
 }
 
-/*
- *      watchdog_write:
- *      @file: file handle to the watchdog
- *      @buf: buffer to write
- *      @count: count of bytes
- *      @ppos: pointer to the position to write. No seeks allowed
- *
- *      A write to a watchdog device is defined as a keepalive signal. Any
- *      write of data will do, as we we don't define content meaning.
- */
-
-static ssize_t watchdog_write(struct file *file, const char __user *buf,
-			    size_t count, loff_t *ppos)
-{
-	if (count) {
-		if (!nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			bool expect_close = false;
-
-			for (i = 0; i != count; i++) {
-				char c;
-				if (get_user(c, buf + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = true;
-			}
-
-			/* Properly order writes across fork()ed processes */
-			mutex_lock(&watchdog.lock);
-			watchdog.expect_close = expect_close;
-			mutex_unlock(&watchdog.lock);
-		}
-
-		/* someone wrote to us, we should restart timer */
-		watchdog_keepalive();
-	}
-	return count;
-}
-
-/*
- *      watchdog_ioctl:
- *      @inode: inode of the device
- *      @file: file handle to the device
- *      @cmd: watchdog command
- *      @arg: argument pointer
- *
- *      The watchdog API defines a common set of functions for all watchdogs
- *      according to their available features.
- */
-static long watchdog_ioctl(struct file *file, unsigned int cmd,
-	unsigned long arg)
-{
-	int status;
-	int new_options;
-	int new_timeout;
-	union {
-		struct watchdog_info __user *ident;
-		int __user *i;
-	} uarg;
-
-	uarg.i = (int __user *)arg;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(uarg.ident, &watchdog.ident,
-			sizeof(watchdog.ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-		status = watchdog_get_status();
-		if (status < 0)
-			return status;
-		return put_user(status, uarg.i);
-
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, uarg.i);
-
-	case WDIOC_SETOPTIONS:
-		if (get_user(new_options, uarg.i))
-			return -EFAULT;
-
-		if (new_options & WDIOS_DISABLECARD)
-			watchdog_stop();
-
-		if (new_options & WDIOS_ENABLECARD)
-			return watchdog_start();
-		fallthrough;
-
-	case WDIOC_KEEPALIVE:
-		watchdog_keepalive();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_timeout, uarg.i))
-			return -EFAULT;
-
-		if (watchdog_set_timeout(new_timeout))
-			return -EINVAL;
-
-		watchdog_keepalive();
-		fallthrough;
-
-	case WDIOC_GETTIMEOUT:
-		return put_user(watchdog.timeout, uarg.i);
-
-	default:
-		return -ENOTTY;
-
-	}
-}
+static const struct watchdog_ops f71808e_wdog_ops = {
+	.owner = THIS_MODULE,
+	.start = fintek_wdt_start,
+	.stop = fintek_wdt_stop,
+	.ping = fintek_wdt_keepalive,
+	.set_timeout = fintek_wdt_set_timeout,
+};
 
-static int watchdog_notify_sys(struct notifier_block *this, unsigned long code,
-	void *unused)
+static bool fintek_wdt_is_running(struct fintek_wdt *wd, u8 wdt_conf)
 {
-	if (code == SYS_DOWN || code == SYS_HALT)
-		watchdog_stop();
-	return NOTIFY_DONE;
+	return (superio_inb(wd->sioaddr, SIO_REG_ENABLE) & BIT(0)) &&
+		(wdt_conf & BIT(F71808FG_FLAG_WD_EN));
 }
 
-static const struct file_operations watchdog_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.open		= watchdog_open,
-	.release	= watchdog_release,
-	.write		= watchdog_write,
-	.unlocked_ioctl	= watchdog_ioctl,
-	.compat_ioctl	= compat_ptr_ioctl,
-};
-
-static struct miscdevice watchdog_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &watchdog_fops,
-};
-
-static struct notifier_block watchdog_notifier = {
-	.notifier_call = watchdog_notify_sys,
-};
-
 static int fintek_wdt_probe(struct platform_device *pdev)
 {
+	struct fintek_wdt *wd;
+	struct watchdog_device *wdd;
 	const struct platform_device_id *id;
-	unsigned short sioaddr;
 	struct resource *res;
 	int wdt_conf, err = 0;
 	const char *identity;
 
-	if (watchdog_miscdev.parent)
-		return -EBUSY;
-
-	watchdog_miscdev.parent = &pdev->dev;
-
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
 	if (!res)
 		return -ENXIO;
 
 	id = platform_get_device_id(pdev);
-	watchdog.variant = (void *)id->driver_data;
-	if (!watchdog.variant)
+	if (!id->driver_data)
 		return -ENODEV;
 
-	/* No need to lock watchdog.lock here because no entry points
-	 * into the module have been registered yet.
-	 */
-	watchdog.sioaddr = res->start;
-	watchdog.ident.options = WDIOF_MAGICCLOSE
+	wd = devm_kzalloc(&pdev->dev, sizeof(*wd), GFP_KERNEL);
+	if (!wd)
+		return -ENOMEM;
+
+	wd->sioaddr = res->start;
+	wd->variant = (void *)id->driver_data;
+	wd->ident.options = WDIOF_SETTIMEOUT
+				| WDIOF_MAGICCLOSE
 				| WDIOF_KEEPALIVEPING
 				| WDIOF_CARDRESET;
 
-	identity = watchdog.variant->identity_override ?: id->name;
+	identity = wd->variant->identity_override ?: id->name;
 
-	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
+	memcpy(wd->ident.identity, identity, sizeof(wd->ident.identity));
 
-	err = superio_enter(sioaddr);
+	err = superio_enter(wd->sioaddr);
 	if (err)
 		return err;
-	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+	superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT);
 
-	wdt_conf = superio_inb(sioaddr, F71808FG_REG_WDT_CONF);
-	watchdog.caused_reboot = wdt_conf & BIT(F71808FG_FLAG_WDTMOUT_STS);
+	wdt_conf = superio_inb(wd->sioaddr, F71808FG_REG_WDT_CONF);
 
 	/*
 	 * We don't want WDTMOUT_STS to stick around till regular reboot.
 	 * Write 1 to the bit to clear it to zero.
 	 */
-	superio_outb(sioaddr, F71808FG_REG_WDT_CONF,
+	superio_outb(wd->sioaddr, F71808FG_REG_WDT_CONF,
 		     wdt_conf | BIT(F71808FG_FLAG_WDTMOUT_STS));
 
-	superio_exit(sioaddr);
+	wdd = &wd->wdd;
 
-	err = watchdog_set_timeout(timeout);
-	if (err)
-		return err;
-	err = watchdog_set_pulse_width(pulse_width);
-	if (err)
-		return err;
+	if (fintek_wdt_is_running(wd, wdt_conf))
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+
+	superio_exit(wd->sioaddr);
 
-	err = register_reboot_notifier(&watchdog_notifier);
+	err = fintek_wdt_set_pulse_width(wd, pulse_width);
 	if (err)
 		return err;
 
-	err = misc_register(&watchdog_miscdev);
-	if (err) {
-		pr_err("cannot register miscdev on minor=%d\n",
-		       watchdog_miscdev.minor);
-		goto exit_reboot;
-	}
-
-	if (start_withtimeout) {
-		if (start_withtimeout <= 0
-		 || start_withtimeout >  max_timeout) {
-			pr_err("starting timeout out of range\n");
-			err = -EINVAL;
-			goto exit_miscdev;
-		}
-
-		err = watchdog_start();
-		if (err) {
-			pr_err("cannot start watchdog timer\n");
-			goto exit_miscdev;
-		}
-
-		mutex_lock(&watchdog.lock);
-		err = superio_enter(sioaddr);
-		if (err)
-			goto exit_unlock;
-		superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
-
-		if (start_withtimeout > 0xff) {
-			/* select minutes for timer units */
-			superio_set_bit(sioaddr, F71808FG_REG_WDT_CONF,
-				F71808FG_FLAG_WD_UNIT);
-			superio_outb(sioaddr, F71808FG_REG_WD_TIME,
-				DIV_ROUND_UP(start_withtimeout, 60));
-		} else {
-			/* select seconds for timer units */
-			superio_clear_bit(sioaddr, F71808FG_REG_WDT_CONF,
-				F71808FG_FLAG_WD_UNIT);
-			superio_outb(sioaddr, F71808FG_REG_WD_TIME,
-				start_withtimeout);
-		}
-
-		superio_exit(sioaddr);
-		mutex_unlock(&watchdog.lock);
+	wdd->info		= &wd->ident;
+	wdd->ops		= &f71808e_wdog_ops;
+	wdd->min_timeout	= 1;
+	wdd->max_timeout	= max_timeout;
+	wdd->timeout		= timeout;
 
-		if (nowayout)
-			__module_get(THIS_MODULE);
+	watchdog_set_nowayout(wdd, nowayout);
+	watchdog_stop_on_unregister(wdd);
+	watchdog_stop_on_reboot(wdd);
+	watchdog_init_timeout(wdd, start_withtimeout, NULL);
+	watchdog_set_drvdata(wdd, wd);
+	platform_set_drvdata(pdev, wd);
 
-		pr_info("watchdog started with initial timeout of %u sec\n",
-			start_withtimeout);
-	}
-
-	return 0;
+	if (wdt_conf & BIT(F71808FG_FLAG_WDTMOUT_STS))
+		wdd->bootstatus	= WDIOF_CARDRESET;
 
-exit_unlock:
-	mutex_unlock(&watchdog.lock);
-exit_miscdev:
-	misc_deregister(&watchdog_miscdev);
-exit_reboot:
-	unregister_reboot_notifier(&watchdog_notifier);
+	/*
+	 * WATCHDOG_HANDLE_BOOT_ENABLED can result in keepalive being directly
+	 * called without a set_timeout before, so it needs to be done here once
+	 */
+	fintek_wdt_set_timeout(wdd, wdd->timeout);
 
-	return err;
+	return devm_watchdog_register_device(&pdev->dev, wdd);
 }
 
 static void f71808fg_pinconf(struct fintek_wdt *wd)
@@ -817,20 +543,8 @@ MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
  */
 static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
 
-static int fintek_wdt_remove(struct platform_device *pdev)
-{
-	if (watchdog_is_running()) {
-		pr_warn("Watchdog timer still running, stopping it\n");
-		watchdog_stop();
-	}
-	misc_deregister(&watchdog_miscdev);
-	unregister_reboot_notifier(&watchdog_notifier);
-	return 0;
-}
-
 static struct platform_driver fintek_wdt_driver = {
 	.probe		= fintek_wdt_probe,
-	.remove		= fintek_wdt_remove,
 	.id_table	= fintek_wdt_id_table,
 	.driver		= {
 		.name	= DRVNAME,
-- 
2.28.0

Re: [PATCH v2 1/2] watchdog: f71808e_wdt: refactor to platform device/driver pair

[Guenter Roeck]

On Tue, Oct 20, 2020 at 08:21:11AM +0200, Ahmad Fatoum wrote:
> Driver so far wasn't ported to the driver model and set up its
> miscdevice out of the init after probing the I/O ports for a watchdog
> with correct vendor and device revision.
> 
> Keep the device detection part at init time, but move watchdog setup
> to a platform driver probe function.
> 
> While at it, refactor some of the driver code we have to now touch
> anyway:
> 
>  - platform_device_id is used instead of the two big switches mapping
>    hardware ID to an enum and then mapping it to a pinconf function
>  - we rename f71808e_ and watchdog_data to fintek_wdt, to avoid mix up
>    of the generic parts up with the device specific parts
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Sorry, I just found this. Sorry, it got lost.
The changes you are making here go way beyond a conversion to a platform
driver. I am not necessarily opposed to the changes, but they should be
split into multiple patches to simlify (and to a large degree enable)
a meaningful review. More comments inline.

> ---
>  drivers/watchdog/f71808e_wdt.c | 377 +++++++++++++++++++--------------
>  1 file changed, 215 insertions(+), 162 deletions(-)
> 
> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
> index f60beec1bbae..4ff7a2509125 100644
> --- a/drivers/watchdog/f71808e_wdt.c
> +++ b/drivers/watchdog/f71808e_wdt.c
> @@ -9,12 +9,15 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/err.h>
> +#include <linux/slab.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/mutex.h>
>  #include <linux/notifier.h>
>  #include <linux/reboot.h>
> @@ -110,22 +113,6 @@ module_param(start_withtimeout, uint, 0);
>  MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
>  	" given initial timeout. Zero (default) disables this feature.");
>  
> -enum chips { f71808fg, f71858fg, f71862fg, f71868, f71869, f71882fg, f71889fg,
> -	     f81803, f81865, f81866};
> -
> -static const char *f71808e_names[] = {
> -	"f71808fg",
> -	"f71858fg",
> -	"f71862fg",
> -	"f71868",
> -	"f71869",
> -	"f71882fg",
> -	"f71889fg",
> -	"f81803",
> -	"f81865",
> -	"f81866",
> -};
> -
>  /* Super-I/O Function prototypes */
>  static inline int superio_inb(int base, int reg);
>  static inline int superio_inw(int base, int reg);
> @@ -136,9 +123,17 @@ static inline int superio_enter(int base);
>  static inline void superio_select(int base, int ld);
>  static inline void superio_exit(int base);
>  
> -struct watchdog_data {
> +struct fintek_wdt;
> +
> +struct fintek_wdt_variant {
> +	u16 id;
> +	void (*pinconf)(struct fintek_wdt *wd);
> +	const char *identity_override;
> +};
> +
> +struct fintek_wdt {
>  	unsigned short	sioaddr;
> -	enum chips	type;
> +	const struct fintek_wdt_variant *variant;

I'd rather have this all in here. Just add 
	void (*pinconf)(struct fintek_wdt *wd);
and assign it in the probe function, or call it
from an array indexed with type.

This change is also independent of the conversion to a platform
driver and should be done in a separate patch.

>  	unsigned long	opened;
>  	struct mutex	lock;
>  	char		expect_close;
> @@ -152,10 +147,15 @@ struct watchdog_data {
>  	char		caused_reboot;	/* last reboot was by the watchdog */
>  };
>  
> -static struct watchdog_data watchdog = {
> +static struct fintek_wdt watchdog = {
>  	.lock = __MUTEX_INITIALIZER(watchdog.lock),
>  };
>  
> +static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
> +{
> +	return wd->variant->id == SIO_F81865_ID || wd->variant->id == SIO_F81866_ID;
> +}
> +
This change is independent of the conversion to a platform driver
and should be a separate patch.

>  /* Super I/O functions */
>  static inline int superio_inb(int base, int reg)
>  {
> @@ -247,7 +247,7 @@ static int watchdog_set_pulse_width(unsigned int pw)
>  	int err = 0;
>  	unsigned int t1 = 25, t2 = 125, t3 = 5000;
>  
> -	if (watchdog.type == f71868) {
> +	if (watchdog.variant->id == SIO_F71868_ID) {

I am not sure if dropping watchdog.type and replacing it with SIO_F71868_ID
is a good idea. In the current code, there is no 1:1 match of those,
specifically for SIO_F71869_ID/SIO_F71869A_ID. Deviating from that only
complicates the code.

>  		t1 = 30;
>  		t2 = 150;
>  		t3 = 6000;
> @@ -309,7 +309,6 @@ static int watchdog_keepalive(void)
>  static int watchdog_start(void)
>  {
>  	int err;
> -	u8 tmp;
>  
>  	/* Make sure we don't die as soon as the watchdog is enabled below */
>  	err = watchdog_keepalive();
> @@ -323,81 +322,12 @@ static int watchdog_start(void)
>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>  
>  	/* Watchdog pin configuration */
> -	switch (watchdog.type) {
> -	case f71808fg:
> -		/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
> -		break;
> -
> -	case f71862fg:
> -		if (f71862fg_pin == 63) {
> -			/* SPI must be disabled first to use this pin! */
> -			superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
> -		} else if (f71862fg_pin == 56) {
> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
> -		}
> -		break;
> -
> -	case f71868:
> -	case f71869:
> -		/* GPIO14 --> WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 4);
> -		break;
> -
> -	case f71882fg:
> -		/* Set pin 56 to WDTRST# */
> -		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
> -		break;
> -
> -	case f71889fg:
> -		/* set pin 40 to WDTRST# */
> -		superio_outb(watchdog.sioaddr, SIO_REG_MFUNCT3,
> -			superio_inb(watchdog.sioaddr, SIO_REG_MFUNCT3) & 0xcf);
> -		break;
> -
> -	case f81803:
> -		/* Enable TSI Level register bank */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_CLOCK_SEL, 3);
> -		/* Set pin 27 to WDTRST# */
> -		superio_outb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
> -			superio_inb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL));
> -		break;
> -
> -	case f81865:
> -		/* Set pin 70 to WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 5);
> -		break;
> -
> -	case f81866:
> -		/*
> -		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
> -		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
> -		 *     BIT5: 0 -> WDTRST#
> -		 *           1 -> GPIO15
> -		 */
> -		tmp = superio_inb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL);
> -		tmp &= ~(BIT(3) | BIT(0));
> -		tmp |= BIT(2);
> -		superio_outb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
> -
> -		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
> -		break;
> -
> -	default:
> -		/*
> -		 * 'default' label to shut up the compiler and catch
> -		 * programmer errors
> -		 */
> -		err = -ENODEV;
> -		goto exit_superio;
> -	}
> +	watchdog.variant->pinconf(&watchdog);
>  
>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>  	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
>  
> -	if (watchdog.type == f81865 || watchdog.type == f81866)
> +	if (has_f81865_wdo_conf(&watchdog))
>  		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
>  				F81865_FLAG_WDOUT_EN);
>  	else
> @@ -425,7 +355,6 @@ static int watchdog_start(void)
>  				F71808FG_FLAG_WD_PULSE);
>  	}
>  
> -exit_superio:
>  	superio_exit(watchdog.sioaddr);
>  exit_unlock:
>  	mutex_unlock(&watchdog.lock);
> @@ -665,21 +594,39 @@ static struct notifier_block watchdog_notifier = {
>  	.notifier_call = watchdog_notify_sys,
>  };
>  
> -static int __init watchdog_init(int sioaddr)
> +static int fintek_wdt_probe(struct platform_device *pdev)
>  {
> +	const struct platform_device_id *id;
> +	unsigned short sioaddr;
> +	struct resource *res;
>  	int wdt_conf, err = 0;
> +	const char *identity;
> +
> +	if (watchdog_miscdev.parent)
> +		return -EBUSY;
> +
> +	watchdog_miscdev.parent = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (!res)
> +		return -ENXIO;
> +
> +	id = platform_get_device_id(pdev);
> +	watchdog.variant = (void *)id->driver_data;
> +	if (!watchdog.variant)
> +		return -ENODEV;
>  
>  	/* No need to lock watchdog.lock here because no entry points
>  	 * into the module have been registered yet.
>  	 */
> -	watchdog.sioaddr = sioaddr;
> +	watchdog.sioaddr = res->start;
>  	watchdog.ident.options = WDIOF_MAGICCLOSE
>  				| WDIOF_KEEPALIVEPING
>  				| WDIOF_CARDRESET;
>  
> -	snprintf(watchdog.ident.identity,
> -		sizeof(watchdog.ident.identity), "%s watchdog",
> -		f71808e_names[watchdog.type]);
> +	identity = watchdog.variant->identity_override ?: id->name;
> +
> +	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
>  
>  	err = superio_enter(sioaddr);
>  	if (err)
> @@ -772,73 +719,176 @@ static int __init watchdog_init(int sioaddr)
>  	return err;
>  }
>  
> -static int __init f71808e_find(int sioaddr)
> +static void f71808fg_pinconf(struct fintek_wdt *wd)
>  {
> +	/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT2, 3);
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 3);
> +}
> +
> +static void f71862fg_pinconf(struct fintek_wdt *wd)
> +{
> +	if (f71862fg_pin == 63) {
> +		/* SPI must be disabled first to use this pin! */
> +		superio_clear_bit(wd->sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT3, 4);
> +	} else if (f71862fg_pin == 56) {
> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
> +	}
> +}
> +
> +static void f71868_pinconf(struct fintek_wdt *wd)
> +{
> +	/* GPIO14 --> WDTRST# */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT1, 4);
> +}
> +
> +static void f71882fg_pinconf(struct fintek_wdt *wd)
> +{
> +	/* Set pin 56 to WDTRST# */
> +	superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
> +}
> +
> +static void f71889fg_pinconf(struct fintek_wdt *wd)
> +{
> +	/* set pin 40 to WDTRST# */
> +	superio_outb(wd->sioaddr, SIO_REG_MFUNCT3,
> +		     superio_inb(wd->sioaddr, SIO_REG_MFUNCT3) & 0xcf);
> +}
> +
> +static void f81803_pinconf(struct fintek_wdt *wd)
> +{
> +	/* Enable TSI Level register bank */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_CLOCK_SEL, 3);
> +	/* Set pin 27 to WDTRST# */
> +	superio_outb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
> +		     superio_inb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL));
> +}
> +
> +static void f81865_pinconf(struct fintek_wdt *wd)
> +{
> +	/* Set pin 70 to WDTRST# */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 5);
> +}
> +
> +static void f81866_pinconf(struct fintek_wdt *wd)
> +{
> +	u8 tmp;
> +	/*
> +	 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
> +	 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
> +	 *     BIT5: 0 -> WDTRST#
> +	 *           1 -> GPIO15
> +	 */
> +	tmp = superio_inb(wd->sioaddr, SIO_F81866_REG_PORT_SEL);
> +	tmp &= ~(BIT(3) | BIT(0));
> +	tmp |= BIT(2);
> +	superio_outb(wd->sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
> +
> +	superio_clear_bit(wd->sioaddr, SIO_F81866_REG_GPIO1, 5);
> +}
> +
> +#define FINTEK_DRV_DATA(...) \
> +	(kernel_ulong_t)(&(struct fintek_wdt_variant) { __VA_ARGS__ })
> +
> +/*
> + * Array is used by both platform bus to map name to driver data and by
> + * init below to map id in driver_data to name for device creation.
> + */
> +static const struct platform_device_id fintek_wdt_id_table[] = {
> +	{ "f71808fg watchdog", FINTEK_DRV_DATA(SIO_F71808_ID,  f71808fg_pinconf) },
> +	{ "f71862fg watchdog", FINTEK_DRV_DATA(SIO_F71862_ID,  f71862fg_pinconf) },
> +	{ "f71868 watchdog",   FINTEK_DRV_DATA(SIO_F71868_ID,  f71868_pinconf) },
> +	{ "f71869 watchdog",   FINTEK_DRV_DATA(SIO_F71869_ID,  f71868_pinconf) },
> +	/* For backwards-compatibility, we define a different watchdog identity name */
> +	{ "f71869a watchdog",  FINTEK_DRV_DATA(SIO_F71869A_ID, f71868_pinconf,
> +					       .identity_override = "f71869 watchdog") },

I don't think .identity_override adds real value.

> +	{ "f71882fg watchdog", FINTEK_DRV_DATA(SIO_F71882_ID,  f71882fg_pinconf) },
> +	{ "f71889fg watchdog", FINTEK_DRV_DATA(SIO_F71889_ID,  f71889fg_pinconf) },
> +	{ "f81803 watchdog",   FINTEK_DRV_DATA(SIO_F81803_ID,  f81803_pinconf) },
> +	{ "f81865 watchdog",   FINTEK_DRV_DATA(SIO_F81865_ID,  f81865_pinconf) },
> +	{ "f81866 watchdog",   FINTEK_DRV_DATA(SIO_F81866_ID,  f81866_pinconf) },

I am not entirely sure I understand exactly what the macro does, and checkpatch
doesn't like it either.

Anyway, I don't think this complexity is necessary. Other similar drivers add
device information as platform driver data (using platform_device_add_data)
and get it using dev_get_platdata(). That would be much simpler and more
straightforward.

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
> +
> +/* Confirmed (by datasheet) not to have a watchdog.
> + * We list this here anyway to maintain backwards-compatibility in not warning about them
> + */
> +static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
> +
> +static int fintek_wdt_remove(struct platform_device *pdev)
> +{
> +	if (watchdog_is_running()) {
> +		pr_warn("Watchdog timer still running, stopping it\n");
> +		watchdog_stop();
> +	}
> +	misc_deregister(&watchdog_miscdev);
> +	unregister_reboot_notifier(&watchdog_notifier);
> +	return 0;
> +}
> +
> +static struct platform_driver fintek_wdt_driver = {
> +	.probe		= fintek_wdt_probe,
> +	.remove		= fintek_wdt_remove,
> +	.id_table	= fintek_wdt_id_table,
> +	.driver		= {
> +		.name	= DRVNAME,
> +	},
> +};
> +
> +static struct platform_device *fintek_wdt_pdev;
> +
> +static __init const char *fintek_wdt_find(int sioaddr)
> +{
> +	const char *name = ERR_PTR(-ENODEV);
> +	const struct platform_device_id *id;
> +	struct fintek_wdt_variant *variant;
>  	u16 devid;
> -	int err = superio_enter(sioaddr);
> +	const u16 *pdevid;
> +	int err;
> +
> +	err = superio_enter(sioaddr);
>  	if (err)
> -		return err;
> +		return ERR_PTR(err);
>  
>  	devid = superio_inw(sioaddr, SIO_REG_MANID);
>  	if (devid != SIO_FINTEK_ID) {
>  		pr_debug("Not a Fintek device\n");
> -		err = -ENODEV;
> -		goto exit;
> +		goto out;
>  	}
>  
>  	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
> -	switch (devid) {
> -	case SIO_F71808_ID:
> -		watchdog.type = f71808fg;
> -		break;
> -	case SIO_F71862_ID:
> -		watchdog.type = f71862fg;
> -		break;
> -	case SIO_F71868_ID:
> -		watchdog.type = f71868;
> -		break;
> -	case SIO_F71869_ID:
> -	case SIO_F71869A_ID:
> -		watchdog.type = f71869;
> -		break;
> -	case SIO_F71882_ID:
> -		watchdog.type = f71882fg;
> -		break;
> -	case SIO_F71889_ID:
> -		watchdog.type = f71889fg;
> -		break;
> -	case SIO_F71858_ID:
> -		/* Confirmed (by datasheet) not to have a watchdog. */
> -		err = -ENODEV;
> -		goto exit;
> -	case SIO_F81803_ID:
> -		watchdog.type = f81803;
> -		break;
> -	case SIO_F81865_ID:
> -		watchdog.type = f81865;
> -		break;
> -	case SIO_F81866_ID:
> -		watchdog.type = f81866;
> -		break;
> -	default:
> -		pr_info("Unrecognized Fintek device: %04x\n",
> -			(unsigned int)devid);
> -		err = -ENODEV;
> -		goto exit;
> +	for (id = fintek_wdt_id_table; id->driver_data; id++) {
> +		variant = (void *)id->driver_data;
> +		if (variant->id == devid) {
> +			name = id->name;
> +			pr_info("Found %s chip, revision %d\n",
> +				name, superio_inb(sioaddr, SIO_REG_DEVREV));
> +			goto out;
> +		}
> +	}
> +
> +	for (pdevid = fintek_no_wdt_table; *pdevid; pdevid++) {
> +		if (*pdevid == devid)
> +			goto out;

I'd rather keep the old code here. I don't see a substantial benefit in
its replacement.

>  	}
>  
> -	pr_info("Found %s watchdog chip, revision %d\n",
> -		f71808e_names[watchdog.type],
> -		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
> -exit:
> +	pr_info("Unrecognized Fintek device: %04x\n", devid);
> +out:
>  	superio_exit(sioaddr);
> -	return err;
> +	return name;
>  }
>  
> -static int __init f71808e_init(void)
> +static struct resource wdt_res = {
> +	.name = "superio port",
> +	.flags = IORESOURCE_IO,
> +};
> +
> +static int __init fintek_wdt_init(void)
>  {
> -	static const unsigned short addrs[] = { 0x2e, 0x4e };
> -	int err = -ENODEV;
> +	unsigned short addrs[] = { 0x2e, 0x4e };

Why drop the static here ? It won't change.

> +	const char *wdt_name;
>  	int i;
>  
>  	if (f71862fg_pin != 63 && f71862fg_pin != 56) {
> @@ -846,30 +896,33 @@ static int __init f71808e_init(void)
>  		return -EINVAL;
>  	}
>  
> +	platform_driver_register(&fintek_wdt_driver);
> +

The driver should only be registered if a watchdog device is found.

>  	for (i = 0; i < ARRAY_SIZE(addrs); i++) {
> -		err = f71808e_find(addrs[i]);
> -		if (err == 0)
> +		wdt_name = fintek_wdt_find(addrs[i]);
> +		if (!IS_ERR(wdt_name))
>  			break;
>  	}
> +
>  	if (i == ARRAY_SIZE(addrs))
> -		return err;
> +		return PTR_ERR(wdt_name);
> +
> +	wdt_res.start = addrs[i];
> +	wdt_res.end = wdt_res.start + 1;
>  
> -	return watchdog_init(addrs[i]);
> +	fintek_wdt_pdev = platform_device_register_simple(wdt_name, -1, &wdt_res, 1);
> +	return PTR_ERR_OR_ZERO(fintek_wdt_pdev);
>  }
>  
> -static void __exit f71808e_exit(void)
> +static void __exit fintek_wdt_exit(void)
>  {
> -	if (watchdog_is_running()) {
> -		pr_warn("Watchdog timer still running, stopping it\n");
> -		watchdog_stop();
> -	}
> -	misc_deregister(&watchdog_miscdev);
> -	unregister_reboot_notifier(&watchdog_notifier);
> +	platform_device_unregister(fintek_wdt_pdev);
> +	platform_driver_unregister(&fintek_wdt_driver);
>  }
>  
>  MODULE_DESCRIPTION("F71808E Watchdog Driver");
>  MODULE_AUTHOR("Giel van Schijndel <me@mortis.eu>");
>  MODULE_LICENSE("GPL");
>  
> -module_init(f71808e_init);
> -module_exit(f71808e_exit);
> +module_init(fintek_wdt_init);
> +module_exit(fintek_wdt_exit);

Re: [PATCH v2 1/2] watchdog: f71808e_wdt: refactor to platform device/driver pair

[Ahmad Fatoum]

Hello Guenter,

Thanks for the review. Comments inline.

On 11/8/20 6:26 PM, Guenter Roeck wrote:
> On Tue, Oct 20, 2020 at 08:21:11AM +0200, Ahmad Fatoum wrote:
>> Driver so far wasn't ported to the driver model and set up its
>> miscdevice out of the init after probing the I/O ports for a watchdog
>> with correct vendor and device revision.
>>
>> Keep the device detection part at init time, but move watchdog setup
>> to a platform driver probe function.
>>
>> While at it, refactor some of the driver code we have to now touch
>> anyway:
>>
>>  - platform_device_id is used instead of the two big switches mapping
>>    hardware ID to an enum and then mapping it to a pinconf function
>>  - we rename f71808e_ and watchdog_data to fintek_wdt, to avoid mix up
>>    of the generic parts up with the device specific parts
>>
>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Sorry, I just found this. Sorry, it got lost.
> The changes you are making here go way beyond a conversion to a platform
> driver. I am not necessarily opposed to the changes, but they should be
> split into multiple patches to simlify (and to a large degree enable)
> a meaningful review. More comments inline.

Most code shuffling around was due to the use of platform_device_id.
I can look into splitting this up though.

> 
>> ---
>>  drivers/watchdog/f71808e_wdt.c | 377 +++++++++++++++++++--------------
>>  1 file changed, 215 insertions(+), 162 deletions(-)
>>
>> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
>> index f60beec1bbae..4ff7a2509125 100644
>> --- a/drivers/watchdog/f71808e_wdt.c
>> +++ b/drivers/watchdog/f71808e_wdt.c
>> @@ -9,12 +9,15 @@
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>>  #include <linux/err.h>
>> +#include <linux/slab.h>
>>  #include <linux/fs.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>>  #include <linux/ioport.h>
>>  #include <linux/miscdevice.h>
>>  #include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mod_devicetable.h>
>>  #include <linux/mutex.h>
>>  #include <linux/notifier.h>
>>  #include <linux/reboot.h>
>> @@ -110,22 +113,6 @@ module_param(start_withtimeout, uint, 0);
>>  MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
>>  	" given initial timeout. Zero (default) disables this feature.");
>>  
>> -enum chips { f71808fg, f71858fg, f71862fg, f71868, f71869, f71882fg, f71889fg,
>> -	     f81803, f81865, f81866};
>> -
>> -static const char *f71808e_names[] = {
>> -	"f71808fg",
>> -	"f71858fg",
>> -	"f71862fg",
>> -	"f71868",
>> -	"f71869",
>> -	"f71882fg",
>> -	"f71889fg",
>> -	"f81803",
>> -	"f81865",
>> -	"f81866",
>> -};
>> -
>>  /* Super-I/O Function prototypes */
>>  static inline int superio_inb(int base, int reg);
>>  static inline int superio_inw(int base, int reg);
>> @@ -136,9 +123,17 @@ static inline int superio_enter(int base);
>>  static inline void superio_select(int base, int ld);
>>  static inline void superio_exit(int base);
>>  
>> -struct watchdog_data {
>> +struct fintek_wdt;
>> +
>> +struct fintek_wdt_variant {
>> +	u16 id;
>> +	void (*pinconf)(struct fintek_wdt *wd);
>> +	const char *identity_override;
>> +};
>> +
>> +struct fintek_wdt {
>>  	unsigned short	sioaddr;
>> -	enum chips	type;
>> +	const struct fintek_wdt_variant *variant;
> 
> I'd rather have this all in here. Just add 
> 	void (*pinconf)(struct fintek_wdt *wd);
> and assign it in the probe function, or call it
> from an array indexed with type.
> 
> This change is also independent of the conversion to a platform
> driver and should be done in a separate patch.

Ok.

>>  	unsigned long	opened;
>>  	struct mutex	lock;
>>  	char		expect_close;
>> @@ -152,10 +147,15 @@ struct watchdog_data {
>>  	char		caused_reboot;	/* last reboot was by the watchdog */
>>  };
>>  
>> -static struct watchdog_data watchdog = {
>> +static struct fintek_wdt watchdog = {
>>  	.lock = __MUTEX_INITIALIZER(watchdog.lock),
>>  };
>>  
>> +static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
>> +{
>> +	return wd->variant->id == SIO_F81865_ID || wd->variant->id == SIO_F81866_ID;
>> +}
>> +
> This change is independent of the conversion to a platform driver
> and should be a separate patch.

Ok.

>>  /* Super I/O functions */
>>  static inline int superio_inb(int base, int reg)
>>  {
>> @@ -247,7 +247,7 @@ static int watchdog_set_pulse_width(unsigned int pw)
>>  	int err = 0;
>>  	unsigned int t1 = 25, t2 = 125, t3 = 5000;
>>  
>> -	if (watchdog.type == f71868) {
>> +	if (watchdog.variant->id == SIO_F71868_ID) {
> 
> I am not sure if dropping watchdog.type and replacing it with SIO_F71868_ID
> is a good idea. In the current code, there is no 1:1 match of those,
> specifically for SIO_F71869_ID/SIO_F71869A_ID. Deviating from that only
> complicates the code.

We can have a 1:1 match if we drop some quirks. See below.

> 
>>  		t1 = 30;
>>  		t2 = 150;
>>  		t3 = 6000;
>> @@ -309,7 +309,6 @@ static int watchdog_keepalive(void)
>>  static int watchdog_start(void)
>>  {
>>  	int err;
>> -	u8 tmp;
>>  
>>  	/* Make sure we don't die as soon as the watchdog is enabled below */
>>  	err = watchdog_keepalive();
>> @@ -323,81 +322,12 @@ static int watchdog_start(void)
>>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>>  
>>  	/* Watchdog pin configuration */
>> -	switch (watchdog.type) {
>> -	case f71808fg:
>> -		/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
>> -		break;
>> -
>> -	case f71862fg:
>> -		if (f71862fg_pin == 63) {
>> -			/* SPI must be disabled first to use this pin! */
>> -			superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
>> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
>> -		} else if (f71862fg_pin == 56) {
>> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
>> -		}
>> -		break;
>> -
>> -	case f71868:
>> -	case f71869:
>> -		/* GPIO14 --> WDTRST# */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 4);
>> -		break;
>> -
>> -	case f71882fg:
>> -		/* Set pin 56 to WDTRST# */
>> -		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
>> -		break;
>> -
>> -	case f71889fg:
>> -		/* set pin 40 to WDTRST# */
>> -		superio_outb(watchdog.sioaddr, SIO_REG_MFUNCT3,
>> -			superio_inb(watchdog.sioaddr, SIO_REG_MFUNCT3) & 0xcf);
>> -		break;
>> -
>> -	case f81803:
>> -		/* Enable TSI Level register bank */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_CLOCK_SEL, 3);
>> -		/* Set pin 27 to WDTRST# */
>> -		superio_outb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
>> -			superio_inb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL));
>> -		break;
>> -
>> -	case f81865:
>> -		/* Set pin 70 to WDTRST# */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 5);
>> -		break;
>> -
>> -	case f81866:
>> -		/*
>> -		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
>> -		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
>> -		 *     BIT5: 0 -> WDTRST#
>> -		 *           1 -> GPIO15
>> -		 */
>> -		tmp = superio_inb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL);
>> -		tmp &= ~(BIT(3) | BIT(0));
>> -		tmp |= BIT(2);
>> -		superio_outb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
>> -
>> -		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
>> -		break;
>> -
>> -	default:
>> -		/*
>> -		 * 'default' label to shut up the compiler and catch
>> -		 * programmer errors
>> -		 */
>> -		err = -ENODEV;
>> -		goto exit_superio;
>> -	}
>> +	watchdog.variant->pinconf(&watchdog);
>>  
>>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>>  	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
>>  
>> -	if (watchdog.type == f81865 || watchdog.type == f81866)
>> +	if (has_f81865_wdo_conf(&watchdog))
>>  		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
>>  				F81865_FLAG_WDOUT_EN);
>>  	else
>> @@ -425,7 +355,6 @@ static int watchdog_start(void)
>>  				F71808FG_FLAG_WD_PULSE);
>>  	}
>>  
>> -exit_superio:
>>  	superio_exit(watchdog.sioaddr);
>>  exit_unlock:
>>  	mutex_unlock(&watchdog.lock);
>> @@ -665,21 +594,39 @@ static struct notifier_block watchdog_notifier = {
>>  	.notifier_call = watchdog_notify_sys,
>>  };
>>  
>> -static int __init watchdog_init(int sioaddr)
>> +static int fintek_wdt_probe(struct platform_device *pdev)
>>  {
>> +	const struct platform_device_id *id;
>> +	unsigned short sioaddr;
>> +	struct resource *res;
>>  	int wdt_conf, err = 0;
>> +	const char *identity;
>> +
>> +	if (watchdog_miscdev.parent)
>> +		return -EBUSY;
>> +
>> +	watchdog_miscdev.parent = &pdev->dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> +	if (!res)
>> +		return -ENXIO;
>> +
>> +	id = platform_get_device_id(pdev);
>> +	watchdog.variant = (void *)id->driver_data;
>> +	if (!watchdog.variant)
>> +		return -ENODEV;
>>  
>>  	/* No need to lock watchdog.lock here because no entry points
>>  	 * into the module have been registered yet.
>>  	 */
>> -	watchdog.sioaddr = sioaddr;
>> +	watchdog.sioaddr = res->start;
>>  	watchdog.ident.options = WDIOF_MAGICCLOSE
>>  				| WDIOF_KEEPALIVEPING
>>  				| WDIOF_CARDRESET;
>>  
>> -	snprintf(watchdog.ident.identity,
>> -		sizeof(watchdog.ident.identity), "%s watchdog",
>> -		f71808e_names[watchdog.type]);
>> +	identity = watchdog.variant->identity_override ?: id->name;
>> +
>> +	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
>>  
>>  	err = superio_enter(sioaddr);
>>  	if (err)
>> @@ -772,73 +719,176 @@ static int __init watchdog_init(int sioaddr)
>>  	return err;
>>  }
>>  
>> -static int __init f71808e_find(int sioaddr)
>> +static void f71808fg_pinconf(struct fintek_wdt *wd)
>>  {
>> +	/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT2, 3);
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 3);
>> +}
>> +
>> +static void f71862fg_pinconf(struct fintek_wdt *wd)
>> +{
>> +	if (f71862fg_pin == 63) {
>> +		/* SPI must be disabled first to use this pin! */
>> +		superio_clear_bit(wd->sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
>> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT3, 4);
>> +	} else if (f71862fg_pin == 56) {
>> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
>> +	}
>> +}
>> +
>> +static void f71868_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* GPIO14 --> WDTRST# */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT1, 4);
>> +}
>> +
>> +static void f71882fg_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* Set pin 56 to WDTRST# */
>> +	superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
>> +}
>> +
>> +static void f71889fg_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* set pin 40 to WDTRST# */
>> +	superio_outb(wd->sioaddr, SIO_REG_MFUNCT3,
>> +		     superio_inb(wd->sioaddr, SIO_REG_MFUNCT3) & 0xcf);
>> +}
>> +
>> +static void f81803_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* Enable TSI Level register bank */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_CLOCK_SEL, 3);
>> +	/* Set pin 27 to WDTRST# */
>> +	superio_outb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
>> +		     superio_inb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL));
>> +}
>> +
>> +static void f81865_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* Set pin 70 to WDTRST# */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 5);
>> +}
>> +
>> +static void f81866_pinconf(struct fintek_wdt *wd)
>> +{
>> +	u8 tmp;
>> +	/*
>> +	 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
>> +	 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
>> +	 *     BIT5: 0 -> WDTRST#
>> +	 *           1 -> GPIO15
>> +	 */
>> +	tmp = superio_inb(wd->sioaddr, SIO_F81866_REG_PORT_SEL);
>> +	tmp &= ~(BIT(3) | BIT(0));
>> +	tmp |= BIT(2);
>> +	superio_outb(wd->sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
>> +
>> +	superio_clear_bit(wd->sioaddr, SIO_F81866_REG_GPIO1, 5);
>> +}
>> +
>> +#define FINTEK_DRV_DATA(...) \
>> +	(kernel_ulong_t)(&(struct fintek_wdt_variant) { __VA_ARGS__ })
>> +
>> +/*
>> + * Array is used by both platform bus to map name to driver data and by
>> + * init below to map id in driver_data to name for device creation.
>> + */
>> +static const struct platform_device_id fintek_wdt_id_table[] = {
>> +	{ "f71808fg watchdog", FINTEK_DRV_DATA(SIO_F71808_ID,  f71808fg_pinconf) },
>> +	{ "f71862fg watchdog", FINTEK_DRV_DATA(SIO_F71862_ID,  f71862fg_pinconf) },
>> +	{ "f71868 watchdog",   FINTEK_DRV_DATA(SIO_F71868_ID,  f71868_pinconf) },
>> +	{ "f71869 watchdog",   FINTEK_DRV_DATA(SIO_F71869_ID,  f71868_pinconf) },
>> +	/* For backwards-compatibility, we define a different watchdog identity name */
>> +	{ "f71869a watchdog",  FINTEK_DRV_DATA(SIO_F71869A_ID, f71868_pinconf,
>> +					       .identity_override = "f71869 watchdog") },
> 
> I don't think .identity_override adds real value.

Userspace that relied on the identity name for whatever would be broken
otherwise. Are you ok with this? (And if yes, could I just rename
all of them to drop the space?)

> 
>> +	{ "f71882fg watchdog", FINTEK_DRV_DATA(SIO_F71882_ID,  f71882fg_pinconf) },
>> +	{ "f71889fg watchdog", FINTEK_DRV_DATA(SIO_F71889_ID,  f71889fg_pinconf) },
>> +	{ "f81803 watchdog",   FINTEK_DRV_DATA(SIO_F81803_ID,  f81803_pinconf) },
>> +	{ "f81865 watchdog",   FINTEK_DRV_DATA(SIO_F81865_ID,  f81865_pinconf) },
>> +	{ "f81866 watchdog",   FINTEK_DRV_DATA(SIO_F81866_ID,  f81866_pinconf) },
> 
> I am not entirely sure I understand exactly what the macro does, and checkpatch
> doesn't like it either.

I had to rebase onto v5.10-rc3 to get checkpatch to complain about
"ERROR: Macros with complex values should be enclosed in parentheses"

> Anyway, I don't think this complexity is necessary. Other similar drivers add
> device information as platform driver data (using platform_device_add_data)
> and get it using dev_get_platdata(). That would be much simpler and more
> straightforward.

Passing function pointers via platdata would be unusual, no?

>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
>> +
>> +/* Confirmed (by datasheet) not to have a watchdog.
>> + * We list this here anyway to maintain backwards-compatibility in not warning about them
>> + */
>> +static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
>> +
>> +static int fintek_wdt_remove(struct platform_device *pdev)
>> +{
>> +	if (watchdog_is_running()) {
>> +		pr_warn("Watchdog timer still running, stopping it\n");
>> +		watchdog_stop();
>> +	}
>> +	misc_deregister(&watchdog_miscdev);
>> +	unregister_reboot_notifier(&watchdog_notifier);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver fintek_wdt_driver = {
>> +	.probe		= fintek_wdt_probe,
>> +	.remove		= fintek_wdt_remove,
>> +	.id_table	= fintek_wdt_id_table,
>> +	.driver		= {
>> +		.name	= DRVNAME,
>> +	},
>> +};
>> +
>> +static struct platform_device *fintek_wdt_pdev;
>> +
>> +static __init const char *fintek_wdt_find(int sioaddr)
>> +{
>> +	const char *name = ERR_PTR(-ENODEV);
>> +	const struct platform_device_id *id;
>> +	struct fintek_wdt_variant *variant;
>>  	u16 devid;
>> -	int err = superio_enter(sioaddr);
>> +	const u16 *pdevid;
>> +	int err;
>> +
>> +	err = superio_enter(sioaddr);
>>  	if (err)
>> -		return err;
>> +		return ERR_PTR(err);
>>  
>>  	devid = superio_inw(sioaddr, SIO_REG_MANID);
>>  	if (devid != SIO_FINTEK_ID) {
>>  		pr_debug("Not a Fintek device\n");
>> -		err = -ENODEV;
>> -		goto exit;
>> +		goto out;
>>  	}
>>  
>>  	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
>> -	switch (devid) {
>> -	case SIO_F71808_ID:
>> -		watchdog.type = f71808fg;
>> -		break;
>> -	case SIO_F71862_ID:
>> -		watchdog.type = f71862fg;
>> -		break;
>> -	case SIO_F71868_ID:
>> -		watchdog.type = f71868;
>> -		break;
>> -	case SIO_F71869_ID:
>> -	case SIO_F71869A_ID:
>> -		watchdog.type = f71869;
>> -		break;
>> -	case SIO_F71882_ID:
>> -		watchdog.type = f71882fg;
>> -		break;
>> -	case SIO_F71889_ID:
>> -		watchdog.type = f71889fg;
>> -		break;
>> -	case SIO_F71858_ID:
>> -		/* Confirmed (by datasheet) not to have a watchdog. */
>> -		err = -ENODEV;
>> -		goto exit;
>> -	case SIO_F81803_ID:
>> -		watchdog.type = f81803;
>> -		break;
>> -	case SIO_F81865_ID:
>> -		watchdog.type = f81865;
>> -		break;
>> -	case SIO_F81866_ID:
>> -		watchdog.type = f81866;
>> -		break;
>> -	default:
>> -		pr_info("Unrecognized Fintek device: %04x\n",
>> -			(unsigned int)devid);
>> -		err = -ENODEV;
>> -		goto exit;
>> +	for (id = fintek_wdt_id_table; id->driver_data; id++) {
>> +		variant = (void *)id->driver_data;
>> +		if (variant->id == devid) {
>> +			name = id->name;
>> +			pr_info("Found %s chip, revision %d\n",
>> +				name, superio_inb(sioaddr, SIO_REG_DEVREV));
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	for (pdevid = fintek_no_wdt_table; *pdevid; pdevid++) {
>> +		if (*pdevid == devid)
>> +			goto out;
> 
> I'd rather keep the old code here. I don't see a substantial benefit in
> its replacement.

I didn't see any benefit in the existance of such a big switch, when
the id_table has the same function.

>>  	}
>>  
>> -	pr_info("Found %s watchdog chip, revision %d\n",
>> -		f71808e_names[watchdog.type],
>> -		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
>> -exit:
>> +	pr_info("Unrecognized Fintek device: %04x\n", devid);
>> +out:
>>  	superio_exit(sioaddr);
>> -	return err;
>> +	return name;
>>  }
>>  
>> -static int __init f71808e_init(void)
>> +static struct resource wdt_res = {
>> +	.name = "superio port",
>> +	.flags = IORESOURCE_IO,
>> +};
>> +
>> +static int __init fintek_wdt_init(void)
>>  {
>> -	static const unsigned short addrs[] = { 0x2e, 0x4e };
>> -	int err = -ENODEV;
>> +	unsigned short addrs[] = { 0x2e, 0x4e };
> 
> Why drop the static here ? It won't change.

I'd have expected the compiler to generate exactly the same
code in this case, so the static const would be unnecessary.
I'll reinstate the static const in v3.

>> +	const char *wdt_name;
>>  	int i;
>>  
>>  	if (f71862fg_pin != 63 && f71862fg_pin != 56) {
>> @@ -846,30 +896,33 @@ static int __init f71808e_init(void)
>>  		return -EINVAL;
>>  	}
>>  
>> +	platform_driver_register(&fintek_wdt_driver);
>> +
> 
> The driver should only be registered if a watchdog device is found.

Right, I am missing a platform_driver_unregister. Will fix.
 
>>  	for (i = 0; i < ARRAY_SIZE(addrs); i++) {
>> -		err = f71808e_find(addrs[i]);
>> -		if (err == 0)
>> +		wdt_name = fintek_wdt_find(addrs[i]);
>> +		if (!IS_ERR(wdt_name))
>>  			break;
>>  	}
>> +
>>  	if (i == ARRAY_SIZE(addrs))
>> -		return err;
>> +		return PTR_ERR(wdt_name);
>> +
>> +	wdt_res.start = addrs[i];
>> +	wdt_res.end = wdt_res.start + 1;
>>  
>> -	return watchdog_init(addrs[i]);
>> +	fintek_wdt_pdev = platform_device_register_simple(wdt_name, -1, &wdt_res, 1);
>> +	return PTR_ERR_OR_ZERO(fintek_wdt_pdev);
>>  }
>>  
>> -static void __exit f71808e_exit(void)
>> +static void __exit fintek_wdt_exit(void)
>>  {
>> -	if (watchdog_is_running()) {
>> -		pr_warn("Watchdog timer still running, stopping it\n");
>> -		watchdog_stop();
>> -	}
>> -	misc_deregister(&watchdog_miscdev);
>> -	unregister_reboot_notifier(&watchdog_notifier);
>> +	platform_device_unregister(fintek_wdt_pdev);
>> +	platform_driver_unregister(&fintek_wdt_driver);
>>  }
>>  
>>  MODULE_DESCRIPTION("F71808E Watchdog Driver");
>>  MODULE_AUTHOR("Giel van Schijndel <me@mortis.eu>");
>>  MODULE_LICENSE("GPL");
>>  
>> -module_init(f71808e_init);
>> -module_exit(f71808e_exit);
>> +module_init(fintek_wdt_init);
>> +module_exit(fintek_wdt_exit);
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |