Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756153AbbEUPS4 (ORCPT ); Thu, 21 May 2015 11:18:56 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:38593 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755806AbbEUPSw (ORCPT ); Thu, 21 May 2015 11:18:52 -0400 Date: Thu, 21 May 2015 08:18:47 -0700 From: Guenter Roeck To: Fu Wei Cc: 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 , Arnd Bergmann , vgandhi@codeaurora.org, wim@iguana.be, Jon Masters , Leo Duran , Jon Corbet , Mark Rutland Subject: Re: [PATCH v2 6/7] Watchdog: introduce ARM SBSA watchdog driver Message-ID: <20150521151847.GA16668@roeck-us.net> References: <=fu.wei@linaro.org> <1432197156-16947-1-git-send-email-fu.wei@linaro.org> <1432197156-16947-7-git-send-email-fu.wei@linaro.org> <555DB4C4.5090606@roeck-us.net> 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 Content-Length: 3318 Lines: 92 On Thu, May 21, 2015 at 07:08:34PM +0800, Fu Wei wrote: > Hi Guenter, > > Thanks for review :-) > > On 21 May 2015 at 18:34, Guenter Roeck wrote: > > On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote: > >> > >> From: Fu Wei > >> > >> This driver bases on linux kernel watchdog framework, and > >> use "pretimeout" in the framework. It supports getting timeout and > >> pretimeout from parameter and FDT at the driver init stage. > >> In first timeout(WS0), the interrupt routine run panic to save > >> system context. > >> > >> Signed-off-by: Fu Wei > >> --- > >> drivers/watchdog/Kconfig | 12 ++ > >> drivers/watchdog/Makefile | 1 + > >> drivers/watchdog/sbsa_gwdt.c | 476 > >> +++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 489 insertions(+) > >> create mode 100644 drivers/watchdog/sbsa_gwdt.c > >> > >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > >> index e5e7c55..25a0df1 100644 > >> --- a/drivers/watchdog/Kconfig > >> +++ b/drivers/watchdog/Kconfig > >> @@ -152,6 +152,18 @@ config ARM_SP805_WATCHDOG > >> ARM Primecell SP805 Watchdog timer. This will reboot your system > >> when > >> the timeout is reached. > >> > >> +config ARM_SBSA_WATCHDOG > >> + tristate "ARM SBSA Generic Watchdog" > >> + depends on ARM || ARM64 || COMPILE_TEST > >> + depends on ARM_ARCH_TIMER > >> + select WATCHDOG_CORE > >> + help > >> + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals > >> + (WS0/WS1), will trigger a warning interrupt(do panic) at the > >> first > >> + timeout(WS0); will reboot your system when the second > >> timeout(WS1) > >> + is reached. > > > > > > I think it would be better to keep the WS0/WS1 out of the help text. > > It is an implementation detail, and no user will be able to make sense of > > it. > > I don't mind to delete it , but those are in SBSA spec, is that an > implementation detail ? > I think so. Ask yourself - assuming you are not involved in the development of this driver, and you read this help text. Is the help text going to help you to know about WS0 and WS1 ? Why not just something like the following, if you want to be verbose ? ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. The first timeout will trigger a panic; the second timeout will trigger a system reset. Anyway, not worth arguing about. > >> + > >> + /* > >> + * Try to determine the frequency from the arch_timer interface > >> + */ > >> + clk = arch_timer_get_rate(); > > > > > > arch_timer_get_rate() does not seem to be exported. Did you try to build > > the driver as module ? > > yes, I have built it as a ko module, that is why I made a patch to > export this interface in the first patch of this patchset > > but I will confirm it again :-) > Guess I'll give it a try myself. I don't really understand how this can work unless arch_timer_get_rate() is exported in your tree. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/