On 3/26/20 7:39 PM, yuechao.zhao(赵越超) wrote: > Hi Guenter > Thank you very much for the suggestion > > But, I wish to consult you on a few questions. > The NCT7904 is very special in watchdog function. Its minimum unit is minutes. > So, what unit do we use for setting the timeout in user space. Minutes or seconds? > Does the kernel document specify it? > Documentation/watchdog/watchdog-api.rst very clearly specifies that the timeout is set in seconds. > Also, about this suggestion: > > + data->wdt.timeout = timeout * 60; /* in seconds */ > > + data->wdt.min_timeout = 1; > > + data->wdt.max_timeout = 15300; > Please make it 60 * 255 (or use a respective define) to clarify where the number comes from. > > The reason for not use 60 * 255(or use a respective define ) is that we will set the default timeout with > "timeout" parameter when "insmod nct7904.ko". This is not an argument for data->wdt.max_timeout = 15300; instead of data->wdt.max_timeout = 60 * 255; or #define MAX_TIMEOUT (60 * 255) ... data->wdt.max_timeout = MAX_TIMEOUT; > The relevant code as follows: > > +static int timeout = WATCHDOG_TIMEOUT; module_param(timeout, int, 0); > > +MODULE_PARM_DESC(timeout, "Watchdog timeout in minutes. 1 <= timeout <= 255, default=" > > + __MODULE_STRING(WATCHODOG_TIMEOUT) "."); > I accepted that you want to have a module parameter which specifies the timeout in minutes instead of seconds (as would be customary). However, I completely fail to understand what that has to do with using an unexplained constant of "15300" when setting the maximum timeout variable instead of "60 * 255" or, as I had suggested, a define. Thanks, Guenter
