Re: [PATCH v2 5/7] watchdog: Add Nuvoton NCT6694 WDT support

[Ming Yu <a0282524688@xxxxxxxxx>]

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