Re: [PATCH 3/9] watchdog: Add Photonicat PMU watchdog driver

On 2024/9/6 19:52, Guenter Roeck wrote:

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

quoted

This driver provides access to Photonicat PMU watchdog functionality.

[...]

quoted

+
+struct pcat_watchdog {
+    struct device *dev;

I don't see what this is used for.

I used to use this for logging, but now they are gone, I will delete it.

[...]

quoted

+
+static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout)
+{
+    int ret;
+    u8 time = 0;

Unnecessary initialization.

quoted

+    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 ?

I forgot to fill in timeout into times[2] during refactoring process, I will fix it.

quoted

+    if (!ret)

[...]>> +

quoted

+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.

quoted

+    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.

I will fix the pcat_wdt_set_timeout.

quoted

+

[...]

quoted

+
+    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.

quoted

+    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() ?

Thanks for your correction, I will fix it.

quoted

+    return devm_watchdog_register_device(dev, &watchdog->wdd);

[...]

quoted

+MODULE_LICENSE("GPL");

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao