Hi Guenter,
np, will do so :-)
On 19 May 2015 at 01:23, Guenter Roeck linux@roeck-us.net wrote:
On Tue, May 19, 2015 at 01:19:22AM +0800, Fu Wei wrote:
Hi Arnd,
Great thanks for your suggestion :-)
feedback inline below
On 15 May 2015 at 22:04, Arnd Bergmann arnd@arndb.de wrote:
On Friday 15 May 2015 19:24:48 fu.wei@linaro.org wrote:
+static void watchdog_check_min_max_pretimeout(struct watchdog_device *wdd) +{
/** Check that we have valid min and max pretimeout values, if* not reset them both to 0 (=not used or unknown)*/if (wdd->min_pretimeout > wdd->max_pretimeout) {pr_info("Invalid min and max pretimeout, resetting to 0!\n");wdd->min_pretimeout = 0;wdd->max_pretimeout = 0;}+}
I would probably just fold this function into the existing watchdog_check_min_max_timeout() and check both normal and pre-timeout there.
yes, I can do that , and that is good idea
+/**
- watchdog_init_pretimeout() - initialize the pretimeout field
- @pretimeout_parm: pretimeout module parameter
- @dev: Device that stores the timeout-sec property
- Initialize the pretimeout field of the watchdog_device struct with either
- the pretimeout module parameter (if it is valid value) or the timeout-sec
- property (only if it is a valid value and the timeout_parm is out of bounds).
- If none of them are valid then we keep the old value (which should normally
- be the default pretimeout value.
- A zero is returned on success and -EINVAL for failure.
- */
+int watchdog_init_pretimeout(struct watchdog_device *wdd,
unsigned int pretimeout_parm, struct device *dev)+{
int ret = 0;u32 timeouts[2];watchdog_check_min_max_pretimeout(wdd);/* try to get the timeout module parameter first */if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm) &&pretimeout_parm) {wdd->pretimeout = pretimeout_parm;return ret;}if (pretimeout_parm)ret = -EINVAL;/* try to get the timeout_sec property */if (!dev || !dev->of_node)return ret;ret = of_property_read_u32_array(dev->of_node,"timeout-sec", timeouts, 2);if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && timeouts[1])wdd->pretimeout = timeouts[1];elseret = -EINVAL;return ret;+} +EXPORT_SYMBOL_GPL(watchdog_init_pretimeout);
Same here: the function is very similar to the watchdog_init_timeout function, and it reads the same property, so just do both here.
The easiest way for that is probably to use of_find_property() and of_prop_next_u32() to read the two numbers.
integrate watchdog_init_pretimeout and watchdog_init_timeout will be a little hard, we may need to change this API to :
watchdog_init_timeouts(struct watchdog_device *wdd, unsigned int timeout_parm, unsigned int pretimeout_parm, struct device *dev)
then we need to update all the watchdog drivers which use this API, maybe we can do this in a individual patchset, after this pretimeout patch is merged.
Is that OK ? :-) any thought?
That is what I would recommend.
Guenter