From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:58034 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757682AbbDYLu7 (ORCPT ); Sat, 25 Apr 2015 07:50:59 -0400 Received: from mailnull by bh-25.webhostbox.net with sa-checked (Exim 4.85) (envelope-from ) id 1Ylh6N-002S8H-RU for linux-watchdog@vger.kernel.org; Fri, 24 Apr 2015 17:09:19 +0000 Date: Fri, 24 Apr 2015 10:08:48 -0700 From: Guenter Roeck To: Timo Kokkonen Cc: linux-arm-kernel@lists.infradead.org, linux-watchdog@vger.kernel.org, boris.brezillon@free-electrons.com, nicolas.ferre@atmel.com, alexandre.belloni@free-electrons.com, Wenyou.Yang@atmel.com Subject: Re: [PATCHv7 1/8] watchdog: Extend kernel API to know about HW limitations Message-ID: <20150424170848.GA2505@roeck-us.net> References: <1429701102-22320-1-git-send-email-timo.kokkonen@offcode.fi> <1429701102-22320-2-git-send-email-timo.kokkonen@offcode.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429701102-22320-2-git-send-email-timo.kokkonen@offcode.fi> Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org On Wed, Apr 22, 2015 at 02:11:35PM +0300, Timo Kokkonen wrote: > There is a great deal of diversity in the watchdog hardware found on > different devices. Differen hardware have different contstraints on > them, many of the constraints that are excessively difficult for the > user space to satisfy. > > One such constraint is the length of the timeout value, which in many > cases can be just a few seconds. Drivers are creating ad hoc solutions > with timers and workqueues to extend the timeout in order to give user > space more time between updates. Looking at the drivers it is clear > that this has resulted to a lot of duplicate code. > > Add an extension to the watchdog kernel API that allows the driver to > describe tis HW constraints to the watchdog code. A kernel worker in > the core is then used to extend the watchdog timeout on behalf of the > user space. This allows the drivers to be simplified as core takes > over the timer extending. > > Tested-by: Wenyou Yang > Signed-off-by: Timo Kokkonen I started twice to review this series, and each time there is a new version before I can finish the review. Guess I'll wait until it settles down a bit before trying again :-(. Just a quick comment below. > --- > drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++-- > drivers/watchdog/watchdog_dev.c | 75 ++++++++++++++++++++++++++--- > include/linux/watchdog.h | 23 +++++++++ > 3 files changed, 189 insertions(+), 10 deletions(-) > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index cec9b55..fd12489 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd, > EXPORT_SYMBOL_GPL(watchdog_init_timeout); > > /** > + * watchdog_init_parms() - initialize generic watchdog parameters > + * @wdd: Watchdog device to operate > + * @dev: Device that stores the device tree properties > + * > + * Initialize the generic timeout parameters. The driver needs to set > + * hw_features bitmask from @wdd prior calling this function in order > + * to ensure the core knows how to handle the HW. > + * > + * A zero is returned on success and -EINVAL for failure. > + */ > +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev) > +{ > + int ret = 0; > + > + ret = watchdog_init_timeout(wdd, wdd->timeout, dev); You are changing the semantics of watchdog_init_timeout here; for all practical purposes it no longer accepts the timeout passed as parameter, but expects the timeout to be configured in wdd->timeout instead. Please don't do that. Guenter