On 9/6/24 02:36, Junhao Xie wrote:
This driver provides access to Photonicat PMU watchdog functionality.
Signed-off-by: Junhao Xie <bigfoot@xxxxxxxxxxx>
---
drivers/watchdog/Kconfig | 12 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/photonicat-wdt.c | 124 ++++++++++++++++++++++++++++++
3 files changed, 137 insertions(+)
create mode 100644 drivers/watchdog/photonicat-wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bae1d97cce89..4094216a1c09 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -300,6 +300,18 @@ config MENZ069_WATCHDOG
This driver can also be built as a module. If so the module
will be called menz069_wdt.
+config PHOTONICAT_PMU_WDT
+ tristate "Photonicat PMU Watchdog"
+ depends on MFD_PHOTONICAT_PMU
+ select WATCHDOG_CORE
+ help
+ This driver provides access to Photonicat PMU watchdog functionality.
+
+ Say Y here to include support for the Photonicat PMU Watchdog.
+
+ This driver can also be built as a module. If so the module
+ will be called photonicat-wdt.
+
config WDAT_WDT
tristate "ACPI Watchdog Action Table (WDAT)"
depends on ACPI
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b51030f035a6..14375af84039 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -234,6 +234,7 @@ obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o
obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
+obj-$(CONFIG_PHOTONICAT_PMU_WDT) += photonicat-wdt.o
obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
diff --git a/drivers/watchdog/photonicat-wdt.c b/drivers/watchdog/photonicat-wdt.c
new file mode 100644
index 000000000000..1e758fcfb49f
--- /dev/null
+++ b/drivers/watchdog/photonicat-wdt.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Junhao Xie <bigfoot@xxxxxxxxxxx>
+ */
+
+#include <linux/mfd/photonicat-pmu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+struct pcat_watchdog {
+ struct device *dev;
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 <bigfoot@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("Photonicat PMU watchdog");
+MODULE_LICENSE("GPL");
