On 9/6/24 02:36, Junhao Xie wrote:

I don't see what this is used for.

+	struct pcat_pmu *pmu;
+	struct watchdog_device wdd;
+	u8 timeout;
+	bool started;
+};
+
+static const struct watchdog_info pcat_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "Photonicat PMU Watchdog",
+};
+
+static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout)
+{
+	int ret;
+	u8 time = 0;

Unnecessary initialization.

+	u8 times[3] = { 60, 60, 0 };
+
+	time = MIN(255, MAX(0, timeout));
+
+	ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET,
+				  times, sizeof(times));

Where does this actually send the timeout to the chip ?

+	if (!ret)
+		data->started = timeout != 0;
+
+	return ret;
+}
+
+static int pcat_wdt_start(struct watchdog_device *wdev)
+{
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	return pcat_wdt_setup(data, data->timeout);
+}
+
+static int pcat_wdt_stop(struct watchdog_device *wdev)
+{
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	return pcat_wdt_setup(data, 0);
+}
+
+static int pcat_wdt_ping(struct watchdog_device *wdev)
+{
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	return pcat_pmu_send(data->pmu, PCAT_CMD_HEARTBEAT, NULL, 0);
+}
+
+static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
+{
+	int ret = 0;
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	data->timeout = val;

This needs to store 'timeout' in wdev. Storing it locally is unnecessary.

+	if (data->started)
+		ret = pcat_wdt_setup(data, data->timeout);

This is misleading because it would permit setting the timeout to
0 when the watchdog isn't running, and then when the watchdog is started
it would not really start it. The code should not use a local "started"
variable but call watchdog_active(). It should also not accept "0"
as a valid timeout.

+
+	return ret;
+}
+
+static const struct watchdog_ops pcat_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = pcat_wdt_start,
+	.stop = pcat_wdt_stop,
+	.ping = pcat_wdt_ping,
+	.set_timeout = pcat_wdt_set_timeout,
+};
+
+static int pcat_watchdog_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pcat_watchdog *watchdog;
+
+	watchdog = devm_kzalloc(dev, sizeof(*watchdog), GFP_KERNEL);
+	if (!watchdog)
+		return -ENOMEM;
+
+	watchdog->dev = dev;
+	watchdog->pmu = dev_get_drvdata(dev->parent);
+	watchdog->wdd.info = &pcat_wdt_info;
+	watchdog->wdd.ops = &pcat_wdt_ops;
+	watchdog->wdd.timeout = 60;
+	watchdog->wdd.max_timeout = U8_MAX;
+	watchdog->wdd.min_timeout = 0;

This effectively lets the user ... kind of ... stop the watchdog
by setting the timeout to 0. This is not acceptable.

+	watchdog->wdd.parent = dev;
+
+	watchdog_stop_on_reboot(&watchdog->wdd);
+	watchdog_set_drvdata(&watchdog->wdd, watchdog);
+	platform_set_drvdata(pdev, watchdog);
+

No watchdog_init_timeout() ?

+	return devm_watchdog_register_device(dev, &watchdog->wdd);
+}
+
+static const struct of_device_id pcat_watchdog_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu-watchdog", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pcat_watchdog_dt_ids);
+
+static struct platform_driver pcat_watchdog_driver = {
+	.driver = {
+		.name = "photonicat-watchdog",
+		.of_match_table = pcat_watchdog_dt_ids,
+	},
+	.probe = pcat_watchdog_probe,
+};
+module_platform_driver(pcat_watchdog_driver);
+
+MODULE_AUTHOR("Junhao Xie [off-list ref]");
+MODULE_DESCRIPTION("Photonicat PMU watchdog");
+MODULE_LICENSE("GPL");