From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932134AbbERRjo (ORCPT ); Mon, 18 May 2015 13:39:44 -0400 Received: from mail-ob0-f176.google.com ([209.85.214.176]:33734 "EHLO mail-ob0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753990AbbERRjg (ORCPT ); Mon, 18 May 2015 13:39:36 -0400 MIME-Version: 1.0 In-Reply-To: <20150518172329.GA17202@roeck-us.net> References: <=fu.wei@linaro.org> <1431689090-3125-1-git-send-email-fu.wei@linaro.org> <2464980.1PKdnqcc3c@wuerfel> <20150518172329.GA17202@roeck-us.net> Date: Tue, 19 May 2015 01:39:36 +0800 Message-ID: Subject: Re: [PATCH 4/6] Watchdog: introdouce "pretimeout" into framework From: Fu Wei To: Guenter Roeck 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 Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Guenter, np, will do so :-) On 19 May 2015 at 01:23, Guenter Roeck 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 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 -- Best regards, Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021