Dear Guenter,

Thank you for reviewing,

Guenter Roeck [off-list ref] 於 2025年5月23日 週五 下午6:02寫道：

quoted

+static unsigned int timeout[NCT6694_WDT_MAX_DEVS] = {
+     [0 ... (NCT6694_WDT_MAX_DEVS - 1)] = NCT6694_DEFAULT_TIMEOUT
+};
+module_param_array(timeout, int, NULL, 0644);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
+
+static unsigned int pretimeout[NCT6694_WDT_MAX_DEVS] = {
+     [0 ... (NCT6694_WDT_MAX_DEVS - 1)] = NCT6694_DEFAULT_PRETIMEOUT
+};
+module_param_array(pretimeout, int, NULL, 0644);
+MODULE_PARM_DESC(pretimeout, "Watchdog pre-timeout in seconds");
+

How is this supposed to work if there are multiple NCT6694 in the system ?

As far as I can tell, they only share the same default timeout and
pretimeout values, which can be overriden using ioctl for individual
devices.

quoted

+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) ")");

...

quoted

+static int nct6694_wdt_setting(struct watchdog_device *wdev,
+                            u32 timeout_val, u8 timeout_act,
+                            u32 pretimeout_val, u8 pretimeout_act)
+{
+     struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev);
+     struct nct6694_wdt_setup *setup = &data->msg->setup;
+     const struct nct6694_cmd_header cmd_hd = {
+             .mod = NCT6694_WDT_MOD,
+             .cmd = NCT6694_WDT_SETUP,
+             .sel = NCT6694_WDT_SETUP_SEL(data->wdev_idx),
+             .len = cpu_to_le16(sizeof(*setup))
+     };
+     unsigned int timeout_fmt, pretimeout_fmt;
+
+     guard(mutex)(&data->lock);
+

Watchdog drivers are already mutex protected in the watchdog core.

Okay, I will remove the mutex in v12.

quoted

+     if (pretimeout_val == 0)
+             pretimeout_act = NCT6694_ACTION_NONE;
+
+     timeout_fmt = (timeout_val * 1000) | (timeout_act << 24);
+     pretimeout_fmt = (pretimeout_val * 1000) | (pretimeout_act << 24);
+
+     memset(setup, 0, sizeof(*setup));
+     setup->timeout = cpu_to_le32(timeout_fmt);
+     setup->pretimeout = cpu_to_le32(pretimeout_fmt);
+
+     return nct6694_write_msg(data->nct6694, &cmd_hd, setup);
+}

...

quoted

+static int nct6694_wdt_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
+     struct nct6694_wdt_data *data;
+     struct watchdog_device *wdev;
+     int ret, wdev_idx;
+
+     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+     if (!data)
+             return -ENOMEM;
+
+     data->msg = devm_kzalloc(dev, sizeof(union nct6694_wdt_msg),
+                              GFP_KERNEL);
+     if (!data->msg)
+             return -ENOMEM;
+
+     wdev_idx = ida_alloc(&nct6694_wdt_ida, GFP_KERNEL);
+     if (wdev_idx < 0)
+             return wdev_idx;
+

Sorry, I fail to understand why this is needed or even makes sense.
That ID is global, so if there is more than one chip they all get more or
less random IDs assigned. Why would that be valuable or necessary ?
If it is just necessary to give the watchdog different IDs, and the
values don't matter, why not just use pdev->id ?
I guess that won't work either because it is used as array index below.
You'll have to find a different means to identify which of the two watchdogs
is handled by this instance of the driver.

This warrants a detailed explanation why there need to be globally unique IDs
across multiple chips.

I will update the code to use pdev->id as assigned by the MFD in the next patch.

quoted

+     ret = devm_add_action_or_reset(dev, nct6694_wdt_ida_remove, data);
+     if (ret)
+             return ret;
+
+     data->dev = dev;
+     data->nct6694 = nct6694;
+     data->wdev_idx = wdev_idx;
+
+     wdev = &data->wdev;
+     wdev->info = &nct6694_wdt_info;
+     wdev->ops = &nct6694_wdt_ops;
+     wdev->timeout = timeout[wdev_idx];
+     wdev->pretimeout = pretimeout[wdev_idx];
+     if (timeout[wdev_idx] < pretimeout[wdev_idx]) {

Maybe I am missing it, but I don't see where wdev_idx is bound to [0,1].
It is global. There could be a dozen of those chips connected through
USB.

quoted

+             dev_warn(data->dev, "pretimeout < timeout. Setting to zero\n");
+             wdev->pretimeout = 0;
+     }
+

Best regards,
Ming