Dear Guenter,
Thank you for your comments,
Guenter Roeck <linux@xxxxxxxxxxxx> 於 2024年11月21日 週四 下午10:15寫道：
> > +config NCT6694_WATCHDOG
> > + tristate "Nuvoton NCT6694 watchdog support"
> > + depends on MFD_NCT6694
> > + select WATCHDOG_CORE
> > + help
> > + If you say yes to this option, support will be included for Nuvoton
> > + NCT6694, a USB device to watchdog timer.
> > +
>
> It is a peripheral expander, not a "USB device to watchdog timer". Watchdog is only
> a small part of its functionality.
>
Understood. I will make the modifications in v3.
> > + This driver can also be built as a module. If so, the module will
> > + be called nct6694_wdt.
...
> > + ret = nct6694_wdt_setting(wdev, wdev->timeout, NCT6694_ACTION_GPO,
> > + wdev->pretimeout, NCT6694_ACTION_GPO);
> > + if (ret)
> > + return ret;
> > +
> > + dev_info(data->dev, "Setting WDT(%d): timeout = %d, pretimeout = %d\n",
> > + data->wdev_idx, wdev->timeout, wdev->pretimeout);
> > +
>
> This is logging noise. Drop or set as debug message.
>
Okay, I'll drop it in v3.
> > + return ret;
> > +}
...
> > +static int nct6694_wdt_set_timeout(struct watchdog_device *wdev,
> > + unsigned int timeout)
> > +{
> > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev);
> > + int ret;
> > +
> > + if (timeout < wdev->pretimeout) {
> > + dev_warn(data->dev, "pretimeout < timeout. Setting to zero\n");
> > + wdev->pretimeout = 0;
> > + }
> > +
> This is only necessary if the pretimeout was not validated during probe
> since otherwise the watchdog core does the check. Please validate it there.
>
Understood. I will make the modifications in v3.
> > + ret = nct6694_wdt_setting(wdev, timeout, NCT6694_ACTION_GPO,
> > + wdev->pretimeout, NCT6694_ACTION_GPO);
> > + if (ret)
> > + return ret;
> > +
> > + wdev->timeout = timeout;
> > +
> > + return ret;
>
> ret == 0 here, so return 0.
>
Okay, fix it in v3.
> > +}
> > +
> > +static int nct6694_wdt_set_pretimeout(struct watchdog_device *wdev,
> > + unsigned int pretimeout)
> > +{
> > + int ret;
> > +
> > + ret = nct6694_wdt_setting(wdev, wdev->timeout, NCT6694_ACTION_GPO,
> > + pretimeout, NCT6694_ACTION_GPO);
> > + if (ret)
> > + return ret;
> > +
> > + wdev->pretimeout = pretimeout;
> > +
> > + return ret;
>
> ret == 0 here, so return 0.
>
Okay, fix it in v3.
> > +}
> > +
> > +static unsigned int nct6694_wdt_get_time(struct watchdog_device *wdev)
> > +{
> > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev);
> > + struct nct6694_wdt_cmd0 *buf = (struct nct6694_wdt_cmd0 *)data->xmit_buf;
> > + struct nct6694 *nct6694 = data->nct6694;
> > + unsigned int timeleft_ms;
> > + int ret;
> > +
> > + guard(mutex)(&data->lock);
> > +
> > + ret = nct6694_read_msg(nct6694, NCT6694_WDT_MOD,
> > + NCT6694_WDT_CMD0_OFFSET(data->wdev_idx),
> > + NCT6694_WDT_CMD0_LEN, buf);
> > + if (ret)
> > + return ret;
>
> The function does not return an error code. Return 0 instead.
Okay, fix it in v3.
> > +
> > + timeleft_ms = le32_to_cpu(buf->countdown);
> > +
> > + return timeleft_ms / 1000;
> > +}
...
> > + wdev = &data->wdev;
> > + wdev->info = &nct6694_wdt_info;
> > + wdev->ops = &nct6694_wdt_ops;
> > + wdev->timeout = timeout;
> > + wdev->pretimeout = pretimeout;
>
> pretimeout should be validated here.
>
Understood. I will make the modifications in v3.
Best regards,
Ming
