On 07/05/2018 08:14 AM, Pascal PAILLET-LME wrote:

First the requested timeout is set, then the caller is notified
that the timeout was set to the maximum possible value ? That doesn't
make sense. If that is really intentional, I would expect a detailed
explanation, and I would expect that the value written into the chip
register matches the value reported back to the user.

+
+	return ret;
+}
+
+static int pmic_wdt_restart_handler(struct notifier_block *this,
+				    unsigned long mode, void *cmd)
+{
+	struct stpmu1_wdt *wdt = container_of(this,
+						   struct stpmu1_wdt,
+						   restart_handler);
+
+	dev_info(wdt->pmic->dev,
+		 "PMIC Watchdog Elapsed (timeout %d), shutdown of PMIC initiated\n",
+		 wdt->wdtdev.timeout);
+

Register a restart handler just to issue a message ? That is quite pointless.
A restart handler is supposed to restart the system. Besides, the message
is highly misleading; there is no reason to believe that it will be called
after the watchdog expired.

This function should restart the system. If it doesn't, drop it.

+	return NOTIFY_DONE;
+}
+
+static const struct watchdog_info pmic_watchdog_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "STPMU1 PMIC Watchdog",
+};
+
+static const struct watchdog_ops pmic_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = pmic_wdt_start,
+	.stop = pmic_wdt_stop,
+	.ping = pmic_wdt_ping,
+	.set_timeout = pmic_wdt_set_timeout,
+};
+
+static int pmic_wdt_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct stpmu1_dev *pmic;
+	struct stpmu1_wdt *wdt;
+
+	if (!pdev->dev.parent)
+		return -EINVAL;
+
+	pmic = dev_get_drvdata(pdev->dev.parent);
+	if (!pmic)
+		return -EINVAL;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(struct stpmu1_wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->pmic = pmic;
+
+	wdt->wdtdev.info = &pmic_watchdog_info;
+	wdt->wdtdev.ops = &pmic_watchdog_ops;
+	wdt->wdtdev.min_timeout = PMIC_WDT_MIN_TIMEOUT;
+	wdt->wdtdev.max_timeout = PMIC_WDT_MAX_TIMEOUT;
+	wdt->wdtdev.timeout = PMIC_WDT_MAX_TIMEOUT;

256 seconds default timeout ? Unusual, just making sure that this is what you want.

+
+	wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	watchdog_set_drvdata(&wdt->wdtdev, wdt);
+	dev_set_drvdata(&pdev->dev, wdt);
+
+	ret = watchdog_register_device(&wdt->wdtdev);
+	if (ret)
+		return ret;
+
+	wdt->restart_handler.notifier_call = pmic_wdt_restart_handler;
+	wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&wdt->restart_handler);

Why is the restart handler provided by the watchdog core not sufficient ?

+	if (ret) {
+		dev_err(wdt->pmic->dev, "failed to register restart handler\n");
+		return ret;
+	}
+
+	dev_dbg(wdt->pmic->dev, "PMIC Watchdog driver probed\n");

The only reasons not to use the devm_ function to register the watchdog
are the restart handler, which 1) doesn't do anything and 2) should use
the watchdog core, and this debug message. I would suggest to use the
devm_ function to register the watchdog instead.

+	return 0;
+}
+
+static int pmic_wdt_remove(struct platform_device *pdev)
+{
+	struct stpmu1_wdt *wdt = dev_get_drvdata(&pdev->dev);
+
+	unregister_restart_handler(&wdt->restart_handler);
+	watchdog_unregister_device(&wdt->wdtdev);
+
+	return 0;
+}
+
+static const struct of_device_id of_pmic_wdt_match[] = {
+	{ .compatible = "st,stpmu1-wdt" },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, of_pmic_wdt_match);
+
+static struct platform_driver stpmu1_wdt_driver = {
+	.probe = pmic_wdt_probe,
+	.remove = pmic_wdt_remove,
+	.driver = {
+		.name = "stpmu1-wdt",
+		.of_match_table = of_pmic_wdt_match,
+	},
+};
+module_platform_driver(stpmu1_wdt_driver);
+
+MODULE_AUTHOR("philippe.peurichard@st.com>");
+MODULE_DESCRIPTION("Watchdog driver for STPMU1 device");
+MODULE_LICENSE("GPL");