On 3/29/20 10:42 PM, yuechao.zhao(赵越超) wrote: > Hi Guenter > > Thank you very much. > I misunderstood your suggestion. (Please make it 60 * 255 (or use a respective define) to clarify where the number comes from) > So, I will modify it. > Also, I will define a module parameter which specifies the timeout in minutes instead of seconds. > The module parameter already specifies the timeout in minutes. Guenter > Thanks > Yuechao > > -----邮件原件----- > 发件人: Guenter Roeck [mailto:groeck7@xxxxxxxxx] 代表 Guenter Roeck > 发送时间: 2020年3月28日 4:54 > 收件人: yuechao.zhao(赵越超) > 抄送: 345351830@xxxxxx; Jean Delvare; linux-hwmon@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Amy.Shih; Oakley.Ding; Jia.Sui(贾睢); shengkui.leng(冷胜魁); Rainbow.Zhang(張玉) > 主题: Re: 答复: [v2,1/1] hwmon: (nct7904) Add watchdog function > > 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 >
