Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934934AbbEOUUR (ORCPT ); Fri, 15 May 2015 16:20:17 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:37633 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934056AbbEOUUO (ORCPT ); Fri, 15 May 2015 16:20:14 -0400 Date: Fri, 15 May 2015 13:20:05 -0700 From: Guenter Roeck To: "Opensource [Steve Twiss]" Cc: LINUXKERNEL , LINUXWATCHDOG , Wim Van Sebroeck , Alessandro Zummo , DEVICETREE , David Dajun Chen , Dmitry Torokhov , Ian Campbell , Kumar Gala , LINUXINPUT , Lee Jones , Liam Girdwood , Mark Brown , Mark Rutland , Pawel Moll , RTCLINUX , Rob Herring , Samuel Ortiz , Support Opensource Subject: Re: [PATCH V2 3/4] watchdog: da9062: DA9062 watchdog driver Message-ID: <20150515202005.GA23133@roeck-us.net> References: <6a51f163b99edfad9165ad29609abb072dbaa2b7.1431621833.git.stwiss.opensource@diasemi.com> <55555639.8090505@roeck-us.net> <6ED8E3B22081A4459DAC7699F3695FB7014B22B63A@SW-EX-MBX02.diasemi.com> <5555ED4F.5070003@roeck-us.net> <6ED8E3B22081A4459DAC7699F3695FB7014B22B6E2@SW-EX-MBX02.diasemi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6ED8E3B22081A4459DAC7699F3695FB7014B22B6E2@SW-EX-MBX02.diasemi.com> 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: 2706 Lines: 74 On Fri, May 15, 2015 at 03:35:50PM +0000, Opensource [Steve Twiss] wrote: > > > > + > > > +/* E_WDG_WARN interrupt handler */ > > > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data) > > > +{ > > > + struct da9062_watchdog *wdt = data; > > > + > > > + dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n"); > > > + return IRQ_HANDLED; > > > +} > > > + > > On 15 May 2015 13:58 Guenter Roeck wrote: > > [...] > > > >>> + > > >>> + irq = platform_get_irq_byname(pdev, "WDG_WARN"); > > >>> + if (irq < 0) { > > >>> + dev_err(wdt->hw->dev, "Failed to get IRQ.\n"); > > >>> + ret = irq; > > >>> + goto error; > > [...] > > > > > > >> Also, is the interrupt mandatory ? All it does is to display a message. > > >> Looks very optional to me. > > > > > > It is a place holder for something more application specific. > > > I could remove it, but I figured it would just get re-added when somebody takes the > > > driver and modifies it for their needs. > > > > > > If this is a problem however, it can go. > > > Please advise .. > > > > > > > Then this someone should add the code. For the time being, it just increases > > kernel size and may cause the driver to fail for no good reason. Plus, given > > the driver apparently works without interrupt, even then it should be > > optional, and the driver does not have to fail loading if it is not supported on a > > given platform. > > > > Hi Guenter, > > I'm not sure if I got my previous point across there ... > > Leaving this in wouldn't really do any real harm I think. If this feature is not supported > in somebody's platform then there wouldn't be a problem, the IRQ would fire (as a > warning that the watchdog was about to time-out), the handler function would be > executed, it would handle the IRQ -- and that would be it. Nothing would happen apart > from a debug print. I didn't get my point across either. Problem is that your driver fails to load if the interrupt is not there. With the interrupt really being optional, I don't see the point in making it mandatory just to display a message if it fires. > > There are already examples of this in the kernel, I've not looked very hard ... > http://lxr.free-electrons.com/source/drivers/mfd/qcom_rpm.c#L412 > http://lxr.free-electrons.com/source/drivers/mfd/arizona-irq.c#L72 > Never a good argument to make with me. 100 people doing something wrong doesn't mean you should continue to do so. 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/