On 05/21/2015 03:05 AM, Fu Wei wrote:
Hi Guenter,
Thanks for review. :-)
feedback inline below
On 21 May 2015 at 17:04, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 05/21/2015 01:32 AM, fu.wei@xxxxxxxxxx wrote:
From: Fu Wei <fu.wei@xxxxxxxxxx>
Also update Documentation/watchdog/watchdog-kernel-api.txt to
introduce:
(1)the new elements in the watchdog_device and watchdog_ops struct;
(2)the new API "watchdog_init_timeouts".
Reasons:
(1)kernel already has two watchdog drivers are using "pretimeout":
drivers/char/ipmi/ipmi_watchdog.c
drivers/watchdog/kempld_wdt.c(but the definition is different)
(2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
---
[ ... ]
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
+ unsigned int pretimeout_parm,
+ unsigned int timeout_parm,
+ void (*update_limits)(struct
watchdog_device *),
+ struct device *dev);
-The watchdog_init_timeout function allows you to initialize the timeout
field
-using the module timeout parameter or by retrieving the timeout-sec
property from
-the device tree (if the module timeout parameter is invalid). Best
practice is
-to set the default timeout value as timeout value in the watchdog_device
and
-then use this function to set the user "preferred" timeout value.
+The watchdog_init_timeouts function allows you to initialize the
pretimeout and
+timeout fields using the module pretimeout and timeout parameter or by
+retrieving the elements in the timeout-sec property(the first element is
for
+timeout, the second one is for pretimeout) from the device tree(if the
module
+pretimeout and timeout parameter are invalid).
+Normally, the pretimeout value will affect the limitation of timeout, and
it
+is also hardware related. So you can write a function in your driver to
update
+the limitation of timeout, according to the pretimeout value. Then pass
the
+function pointer by the update_limits parameter. If you driver doesn't
+need this adjustment, just pass NULL to the update_limits parameter.
You've lost me a bit with the update_limits function.
watchdog_init_timeouts()
is called from the driver.
yes, that is the help function which will be called from watchdog
driver, like SBSA watchdog driver
Why should the function have to call back into
the
driver to update the parameters which are passed from the driver ?
Let me explain this, please correct me if I misunderstand something.
According to the concept of "pretimeout" in kernel, the timeout
contains the pretimeout, like
* Kernel/API: P---------| pretimeout
* |-------------------------------T timeout
If you set up the value of pretimeout, that means pretimeout
<min_timeout < timeout < max_timeout < (pretimeout +
max_timeout_for_1th_stage)
For min_timeout > pretimeout. if some one setup a timeout like :
pretimeout > timeout > min_timeout, I think that could be a problem
For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some
one setup a timeout like (pretimeout + max_timeout_for_1th_stage) <
timeout > max_timeout .
I have explained a little in doc, but the adjustment may have
something to do with hardware, like max_timeout_for_1th_stage(in SBSA
watchdog , limited by WCV)
maybe this problem wouldn't happen ,if you set up max_timeout to a
small number. so you can pass NULL to the pointer.
but I think maybe for other device , that may happen.
Seems to me the driver can do that calculation first, then call
watchdog_init_timeouts() with the result. Am I missing something ?
maybe I am overthinking it :-)
please correct me
I just sent a more complete review. In general I think this problem
(where the driver needs to update timeout limits based on the value of
pretimeout) is very driver specific, and should be kept in the driver.
I would prefer to keep it out of the API if possible.
Unless I am missing something, it should be possible to call the
update_limits function in the driver after calling init_timeouts.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
