From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754228AbbERRXs (ORCPT ); Mon, 18 May 2015 13:23:48 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:32793 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752601AbbERRXo (ORCPT ); Mon, 18 May 2015 13:23:44 -0400 Date: Mon, 18 May 2015 10:23:30 -0700 From: Guenter Roeck To: Fu Wei Cc: Arnd Bergmann , Suravee Suthikulpanit , Linaro ACPI Mailman List , linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Wei Fu , G Gregory , Al Stone , Hanjun Guo , Timur Tabi , Ashwin Chaugule , vgandhi@codeaurora.org, wim@iguana.be, Jon Masters , Leo Duran , Jon Corbet Subject: Re: [PATCH 4/6] Watchdog: introdouce "pretimeout" into framework Message-ID: <20150518172329.GA17202@roeck-us.net> References: <=fu.wei@linaro.org> <1431689090-3125-1-git-send-email-fu.wei@linaro.org> <2464980.1PKdnqcc3c@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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]; > >> + else > >> + ret = -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