Add watchdog timer support for the Congatec Board Controller.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/watchdog/Kconfig    |  10 ++
 drivers/watchdog/Makefile   |   1 +
 drivers/watchdog/cgbc_wdt.c | 217 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bae1d97cce89..07b711fc8bb2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1142,6 +1142,16 @@ config ALIM7101_WDT
 
 	  Most people will say N.
 
+config CGBC_WDT
+	tristate "Congatec Board Controller Watchdog Timer"
+	depends on MFD_CGBC
+	select WATCHDOG_CORE
+	help
+	  Enables watchdog timer support for the Congatec Board Controller.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called cgbc_wdt.
+
 config EBC_C384_WDT
 	tristate "WinSystems EBC-C384 Watchdog Timer"
 	depends on (X86 || COMPILE_TEST) && HAS_IOPORT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b51030f035a6..5aa66ba91346 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
 obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
 obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
 obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_CGBC_WDT) += cgbc_wdt.o
 obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
 obj-$(CONFIG_EXAR_WDT) += exar_wdt.o
 obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
diff --git a/drivers/watchdog/cgbc_wdt.c b/drivers/watchdog/cgbc_wdt.c
new file mode 100644
index 000000000000..9327e87b52e8
--- /dev/null
+++ b/drivers/watchdog/cgbc_wdt.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Congatec Board Controller watchdog driver
+ *
+ * Copyright (C) 2024 Bootlin
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#include <linux/mfd/cgbc.h>
+
+#define CGBC_WDT_CMD_TRIGGER	0x27
+#define CGBC_WDT_CMD_INIT	0x28
+#define CGBC_WDT_DISABLE	0x00
+
+#define CGBC_WDT_MODE_SINGLE_EVENT 0x02
+
+#define DEFAULT_TIMEOUT_SEC	30
+#define DEFAULT_PRETIMEOUT_SEC	0
+
+enum action {
+	ACTION_INT = 0,
+	ACTION_SMI,
+	ACTION_RESET,
+	ACTION_BUTTON,
+};
+
+static unsigned int timeout = DEFAULT_TIMEOUT_SEC;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout,
+		 "Watchdog timeout in seconds. (>=0, default="
+		 __MODULE_STRING(DEFAULT_TIMEOUT_SEC) ")");
+
+static unsigned int pretimeout = DEFAULT_PRETIMEOUT_SEC;
+module_param(pretimeout, uint, 0);
+MODULE_PARM_DESC(pretimeout,
+		 "Watchdog pretimeout in seconds. (>=0, default="
+		 __MODULE_STRING(DEFAULT_PRETIMEOUT_SEC) ")");
+
+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) ")");
+
+struct cgbc_wdt_data {
+	struct cgbc_device_data	*cgbc;
+	struct watchdog_device	wdd;
+	enum action timeout_action;
+	enum action pretimeout_action;
+};
+
+struct cgbc_wdt_cmd_cfg {
+	u8 cmd;
+	u8 mode;
+	u8 action;
+	u8 timeout1[3];
+	u8 timeout2[3];
+	u8 reserved[3];
+	u8 delay[3];
+} __packed;
+
+static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);
+
+static int cgbc_wdt_start(struct watchdog_device *wdd)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+	struct cgbc_device_data *cgbc = wdt_data->cgbc;
+	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
+	unsigned int timeout2 = wdd->pretimeout * 1000;
+	u8 action;
+
+	struct cgbc_wdt_cmd_cfg cmd_start = {
+		.cmd = CGBC_WDT_CMD_INIT,
+		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
+		.timeout1[0] = (u8)timeout1,
+		.timeout1[1] = (u8)(timeout1 >> 8),
+		.timeout1[2] = (u8)(timeout1 >> 16),
+		.timeout2[0] = (u8)timeout2,
+		.timeout2[1] = (u8)(timeout2 >> 8),
+		.timeout2[2] = (u8)(timeout2 >> 16),
+	};
+
+	if (wdd->pretimeout) {
+		action = 2;
+		action |= wdt_data->pretimeout_action << 2;
+		action |= wdt_data->timeout_action << 4;
+	} else {
+		action = 1;
+		action |= wdt_data->timeout_action << 2;
+	}
+
+	cmd_start.action = action;
+
+	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
+}
+
+static int cgbc_wdt_stop(struct watchdog_device *wdd)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+	struct cgbc_device_data *cgbc = wdt_data->cgbc;
+	struct cgbc_wdt_cmd_cfg cmd_stop = {
+		.cmd = CGBC_WDT_CMD_INIT,
+		.mode = CGBC_WDT_DISABLE,
+	};
+
+	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
+}
+
+static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+	struct cgbc_device_data *cgbc = wdt_data->cgbc;
+	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
+
+	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
+}
+
+static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
+				   unsigned int pretimeout)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+
+	wdd->pretimeout = pretimeout;
+	wdt_data->pretimeout_action = ACTION_SMI;
+
+	if (watchdog_active(wdd))
+		return cgbc_wdt_start(wdd);
+
+	return 0;
+}
+
+static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
+				unsigned int timeout)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+
+	if (timeout < wdd->pretimeout) {
+		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");
+		wdd->pretimeout = 0;
+	}
+
+	wdd->timeout = timeout;
+	wdt_data->timeout_action = ACTION_RESET;
+
+	if (watchdog_active(wdd))
+		return cgbc_wdt_start(wdd);
+
+	return 0;
+}
+
+static const struct watchdog_info cgbc_wdt_info = {
+	.identity	= "CGBC Watchdog",
+	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
+};
+
+static const struct watchdog_ops cgbc_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= cgbc_wdt_start,
+	.stop		= cgbc_wdt_stop,
+	.ping		= cgbc_wdt_keepalive,
+	.set_timeout	= cgbc_wdt_set_timeout,
+	.set_pretimeout = cgbc_wdt_set_pretimeout,
+};
+
+static int cgbc_wdt_probe(struct platform_device *pdev)
+{
+	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct cgbc_wdt_data *wdt_data;
+	struct watchdog_device *wdd;
+	int ret;
+
+	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);
+	if (!wdt_data)
+		return -ENOMEM;
+
+	wdt_data->cgbc = cgbc;
+	wdd = &wdt_data->wdd;
+	wdd->parent = dev;
+
+	wdd->info = &cgbc_wdt_info;
+	wdd->ops = &cgbc_wdt_ops;
+
+	watchdog_set_drvdata(wdd, wdt_data);
+	watchdog_set_nowayout(wdd, nowayout);
+
+	cgbc_wdt_set_timeout(wdd, timeout);
+	cgbc_wdt_set_pretimeout(wdd, pretimeout);
+
+	platform_set_drvdata(pdev, wdt_data);
+	watchdog_stop_on_reboot(wdd);
+	watchdog_stop_on_unregister(wdd);
+
+	ret = devm_watchdog_register_device(dev, wdd);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver cgbc_wdt_driver = {
+	.driver		= {
+		.name	= "cgbc-wdt",
+	},
+	.probe		= cgbc_wdt_probe,
+};
+
+module_platform_driver(cgbc_wdt_driver);
+
+MODULE_DESCRIPTION("Congatec Board Controller Watchdog Driver");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.2

On Fri, Aug 09, 2024 at 04:52:08PM +0200, Thomas Richard wrote:
> Add watchdog timer support for the Congatec Board Controller.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/watchdog/Kconfig    |  10 ++
>  drivers/watchdog/Makefile   |   1 +
>  drivers/watchdog/cgbc_wdt.c | 217 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bae1d97cce89..07b711fc8bb2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1142,6 +1142,16 @@ config ALIM7101_WDT
>  
>  	  Most people will say N.
>  
> +config CGBC_WDT
> +	tristate "Congatec Board Controller Watchdog Timer"
> +	depends on MFD_CGBC
> +	select WATCHDOG_CORE
> +	help
> +	  Enables watchdog timer support for the Congatec Board Controller.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called cgbc_wdt.
> +
>  config EBC_C384_WDT
>  	tristate "WinSystems EBC-C384 Watchdog Timer"
>  	depends on (X86 || COMPILE_TEST) && HAS_IOPORT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b51030f035a6..5aa66ba91346 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -106,6 +106,7 @@ obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
>  obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
>  obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
>  obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
> +obj-$(CONFIG_CGBC_WDT) += cgbc_wdt.o
>  obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
>  obj-$(CONFIG_EXAR_WDT) += exar_wdt.o
>  obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
> diff --git a/drivers/watchdog/cgbc_wdt.c b/drivers/watchdog/cgbc_wdt.c
> new file mode 100644
> index 000000000000..9327e87b52e8
> --- /dev/null
> +++ b/drivers/watchdog/cgbc_wdt.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Congatec Board Controller watchdog driver
> + *
> + * Copyright (C) 2024 Bootlin
> + * Author: Thomas Richard <thomas.richard@bootlin.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#include <linux/mfd/cgbc.h>
> +
> +#define CGBC_WDT_CMD_TRIGGER	0x27
> +#define CGBC_WDT_CMD_INIT	0x28
> +#define CGBC_WDT_DISABLE	0x00
> +
> +#define CGBC_WDT_MODE_SINGLE_EVENT 0x02
> +
> +#define DEFAULT_TIMEOUT_SEC	30
> +#define DEFAULT_PRETIMEOUT_SEC	0
> +
> +enum action {
> +	ACTION_INT = 0,
> +	ACTION_SMI,
> +	ACTION_RESET,
> +	ACTION_BUTTON,
> +};
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT_SEC;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +		 "Watchdog timeout in seconds. (>=0, default="
> +		 __MODULE_STRING(DEFAULT_TIMEOUT_SEC) ")");
> +
> +static unsigned int pretimeout = DEFAULT_PRETIMEOUT_SEC;
> +module_param(pretimeout, uint, 0);
> +MODULE_PARM_DESC(pretimeout,
> +		 "Watchdog pretimeout in seconds. (>=0, default="
> +		 __MODULE_STRING(DEFAULT_PRETIMEOUT_SEC) ")");
> +
> +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) ")");
> +
> +struct cgbc_wdt_data {
> +	struct cgbc_device_data	*cgbc;
> +	struct watchdog_device	wdd;
> +	enum action timeout_action;
> +	enum action pretimeout_action;
> +};
> +
> +struct cgbc_wdt_cmd_cfg {
> +	u8 cmd;
> +	u8 mode;
> +	u8 action;
> +	u8 timeout1[3];
> +	u8 timeout2[3];
> +	u8 reserved[3];
> +	u8 delay[3];
> +} __packed;
> +
> +static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);

static_assert() is declared in linux/build_bug.h. Please include all
necessary include files explicitly and do not depend on indirect includes.

> +
> +static int cgbc_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);

Unusual way to get wdt_data instead of using container_of().
Any special reason ?

> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
> +	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
> +	unsigned int timeout2 = wdd->pretimeout * 1000;
> +	u8 action;
> +
> +	struct cgbc_wdt_cmd_cfg cmd_start = {
> +		.cmd = CGBC_WDT_CMD_INIT,
> +		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
> +		.timeout1[0] = (u8)timeout1,
> +		.timeout1[1] = (u8)(timeout1 >> 8),
> +		.timeout1[2] = (u8)(timeout1 >> 16),
> +		.timeout2[0] = (u8)timeout2,
> +		.timeout2[1] = (u8)(timeout2 >> 8),
> +		.timeout2[2] = (u8)(timeout2 >> 16),
> +	};
> +
> +	if (wdd->pretimeout) {
> +		action = 2;
> +		action |= wdt_data->pretimeout_action << 2;
> +		action |= wdt_data->timeout_action << 4;
> +	} else {
> +		action = 1;
> +		action |= wdt_data->timeout_action << 2;
> +	}
> +
> +	cmd_start.action = action;
> +
> +	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
> +	struct cgbc_wdt_cmd_cfg cmd_stop = {
> +		.cmd = CGBC_WDT_CMD_INIT,
> +		.mode = CGBC_WDT_DISABLE,
> +	};
> +
> +	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
> +	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
> +
> +	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
> +				   unsigned int pretimeout)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +
> +	wdd->pretimeout = pretimeout;
> +	wdt_data->pretimeout_action = ACTION_SMI;
> +
> +	if (watchdog_active(wdd))
> +		return cgbc_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
> +				unsigned int timeout)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +
> +	if (timeout < wdd->pretimeout) {
> +		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");

That is a normal condition which does not warrant a log message.
Also see drivers/watchdog/watchdog_dev.c around line 385.

> +		wdd->pretimeout = 0;
> +	}
> +
> +	wdd->timeout = timeout;
> +	wdt_data->timeout_action = ACTION_RESET;

Both timeout_action and pretimeout_action are set statically.
What is the point of doing that instead of just using
ACTION_RESET and ACTION_SMI as needed irectly ?

> +
> +	if (watchdog_active(wdd))
> +		return cgbc_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info cgbc_wdt_info = {
> +	.identity	= "CGBC Watchdog",
> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> +		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
> +};
> +
> +static const struct watchdog_ops cgbc_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= cgbc_wdt_start,
> +	.stop		= cgbc_wdt_stop,
> +	.ping		= cgbc_wdt_keepalive,
> +	.set_timeout	= cgbc_wdt_set_timeout,
> +	.set_pretimeout = cgbc_wdt_set_pretimeout,
> +};
> +
> +static int cgbc_wdt_probe(struct platform_device *pdev)
> +{
> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct cgbc_wdt_data *wdt_data;
> +	struct watchdog_device *wdd;
> +	int ret;
> +
> +	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);

devm_kzalloc() is declared in linux/device.h. Again, please include all
necessary include files explicitly.

> +	if (!wdt_data)
> +		return -ENOMEM;
> +
> +	wdt_data->cgbc = cgbc;
> +	wdd = &wdt_data->wdd;
> +	wdd->parent = dev;
> +

No limits ? That is unusual. Are you sure the driver accepts all
timeouts from 0 to UINT_MAX ?

> +	wdd->info = &cgbc_wdt_info;
> +	wdd->ops = &cgbc_wdt_ops;
> +
> +	watchdog_set_drvdata(wdd, wdt_data);
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	cgbc_wdt_set_timeout(wdd, timeout);
> +	cgbc_wdt_set_pretimeout(wdd, pretimeout);

The more common approach would be to set default limits in wdd->{timout,pretimeout}
and only override the values if needed, ie if provided using module parameters.
That implies initializing the module parameters with 0. YOur call, though.

> +
> +	platform_set_drvdata(pdev, wdt_data);
> +	watchdog_stop_on_reboot(wdd);
> +	watchdog_stop_on_unregister(wdd);
> +
> +	ret = devm_watchdog_register_device(dev, wdd);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Why not just
	return devm_watchdog_register_device(dev, wdd);
?

> +}
> +
> +static struct platform_driver cgbc_wdt_driver = {
> +	.driver		= {
> +		.name	= "cgbc-wdt",
> +	},
> +	.probe		= cgbc_wdt_probe,
> +};
> +
> +module_platform_driver(cgbc_wdt_driver);
> +
> +MODULE_DESCRIPTION("Congatec Board Controller Watchdog Driver");
> +MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.39.2
> 
>

Hi Guenter,

>> +
>> +struct cgbc_wdt_cmd_cfg {
>> +	u8 cmd;
>> +	u8 mode;
>> +	u8 action;
>> +	u8 timeout1[3];
>> +	u8 timeout2[3];
>> +	u8 reserved[3];
>> +	u8 delay[3];
>> +} __packed;
>> +
>> +static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);
> 
> static_assert() is declared in linux/build_bug.h. Please include all
> necessary include files explicitly and do not depend on indirect includes.

Fixed in next iteration.

> 
>> +
>> +static int cgbc_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> 
> Unusual way to get wdt_data instead of using container_of().
> Any special reason ?

No special reason, I saw that watchdog_get_drvdata() was often used in
watchdog drivers (more than container_of()) to get wdt_data.
But I can use container_of() if you think I should.

> 
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
>> +	unsigned int timeout2 = wdd->pretimeout * 1000;
>> +	u8 action;
>> +
>> +	struct cgbc_wdt_cmd_cfg cmd_start = {
>> +		.cmd = CGBC_WDT_CMD_INIT,
>> +		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
>> +		.timeout1[0] = (u8)timeout1,
>> +		.timeout1[1] = (u8)(timeout1 >> 8),
>> +		.timeout1[2] = (u8)(timeout1 >> 16),
>> +		.timeout2[0] = (u8)timeout2,
>> +		.timeout2[1] = (u8)(timeout2 >> 8),
>> +		.timeout2[2] = (u8)(timeout2 >> 16),
>> +	};
>> +
>> +	if (wdd->pretimeout) {
>> +		action = 2;
>> +		action |= wdt_data->pretimeout_action << 2;
>> +		action |= wdt_data->timeout_action << 4;
>> +	} else {
>> +		action = 1;
>> +		action |= wdt_data->timeout_action << 2;
>> +	}
>> +
>> +	cmd_start.action = action;
>> +
>> +	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	struct cgbc_wdt_cmd_cfg cmd_stop = {
>> +		.cmd = CGBC_WDT_CMD_INIT,
>> +		.mode = CGBC_WDT_DISABLE,
>> +	};
>> +
>> +	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
>> +
>> +	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
>> +				   unsigned int pretimeout)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +
>> +	wdd->pretimeout = pretimeout;
>> +	wdt_data->pretimeout_action = ACTION_SMI;
>> +
>> +	if (watchdog_active(wdd))
>> +		return cgbc_wdt_start(wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
>> +				unsigned int timeout)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +
>> +	if (timeout < wdd->pretimeout) {
>> +		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");
> 
> That is a normal condition which does not warrant a log message.
> Also see drivers/watchdog/watchdog_dev.c around line 385.

Fixed in next iteration.

> 
>> +		wdd->pretimeout = 0;
>> +	}
>> +
>> +	wdd->timeout = timeout;
>> +	wdt_data->timeout_action = ACTION_RESET;
> 
> Both timeout_action and pretimeout_action are set statically.
> What is the point of doing that instead of just using
> ACTION_RESET and ACTION_SMI as needed irectly ?

Yes indeed, using ACTION_RESET and ACTION_SMI directly in
cgbc_wdt_start() makes the code smaller.

> 
>> +
>> +	if (watchdog_active(wdd))
>> +		return cgbc_wdt_start(wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct watchdog_info cgbc_wdt_info = {
>> +	.identity	= "CGBC Watchdog",
>> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>> +		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
>> +};
>> +
>> +static const struct watchdog_ops cgbc_wdt_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.start		= cgbc_wdt_start,
>> +	.stop		= cgbc_wdt_stop,
>> +	.ping		= cgbc_wdt_keepalive,
>> +	.set_timeout	= cgbc_wdt_set_timeout,
>> +	.set_pretimeout = cgbc_wdt_set_pretimeout,
>> +};
>> +
>> +static int cgbc_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
>> +	struct device *dev = &pdev->dev;
>> +	struct cgbc_wdt_data *wdt_data;
>> +	struct watchdog_device *wdd;
>> +	int ret;
>> +
>> +	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);
> 
> devm_kzalloc() is declared in linux/device.h. Again, please include all
> necessary include files explicitly.

Fixed in next iteration.

> 
>> +	if (!wdt_data)
>> +		return -ENOMEM;
>> +
>> +	wdt_data->cgbc = cgbc;
>> +	wdd = &wdt_data->wdd;
>> +	wdd->parent = dev;
>> +
> 
> No limits ? That is unusual. Are you sure the driver accepts all
> timeouts from 0 to UINT_MAX ?

Yes limits are needed.
For next iteration I added 1s as min_timeout (which seems to be the
usual value, and it is accepted by the hardware), and a max_timeout.

> 
>> +	wdd->info = &cgbc_wdt_info;
>> +	wdd->ops = &cgbc_wdt_ops;
>> +
>> +	watchdog_set_drvdata(wdd, wdt_data);
>> +	watchdog_set_nowayout(wdd, nowayout);
>> +
>> +	cgbc_wdt_set_timeout(wdd, timeout);
>> +	cgbc_wdt_set_pretimeout(wdd, pretimeout);
> 
> The more common approach would be to set default limits in wdd->{timout,pretimeout}
> and only override the values if needed, ie if provided using module parameters.
> That implies initializing the module parameters with 0. YOur call, though.

Ok.
For next iteration I added limits (min_timeout, max_timeout), the
timeout module parameter is set to 0 by default, and
watchdog_init_timeout() is called in the probe.

> 
>> +
>> +	platform_set_drvdata(pdev, wdt_data);
>> +	watchdog_stop_on_reboot(wdd);
>> +	watchdog_stop_on_unregister(wdd);
>> +
>> +	ret = devm_watchdog_register_device(dev, wdd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> Why not just
> 	return devm_watchdog_register_device(dev, wdd);
> ?

I don't know :)
Fixed in the next iteration.

Thanks for the review !!

Thomas.