Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752564AbbG2Buc (ORCPT ); Tue, 28 Jul 2015 21:50:32 -0400 Received: from nasmtp01.atmel.com ([192.199.1.245]:29038 "EHLO DVREDG01.corp.atmel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751135AbbG2Bua (ORCPT ); Tue, 28 Jul 2015 21:50:30 -0400 From: "Yang, Wenyou" To: Guenter Roeck , "wim@iguana.be" , "robh+dt@kernel.org" , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" CC: "sylvain.rochet@finsecur.com" , "Ferre, Nicolas" , "boris.brezillon@free-electrons.com" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-watchdog@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support Thread-Topic: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support Thread-Index: AQHQyQQjyw0A4iIFTkmctiToR/XhHJ3v8dgAgAGl4CD//4pDgIAAip2g Date: Wed, 29 Jul 2015 01:50:25 +0000 Message-ID: References: <1438066825-21923-1-git-send-email-wenyou.yang@atmel.com> <1438066825-21923-2-git-send-email-wenyou.yang@atmel.com> <55B72BC0.8070206@roeck-us.net> <55B82AE1.2050906@roeck-us.net> In-Reply-To: <55B82AE1.2050906@roeck-us.net> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.168.5.13] Content-Type: text/plain; charset="gb2312" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t6T1oai6022079 Content-Length: 3354 Lines: 78 Hi Guenter, Thank you for your prompt answer. > -----Original Message----- > From: Guenter Roeck [mailto:linux@roeck-us.net] > Sent: 2015??7??29?? 9:23 > To: Yang, Wenyou; wim@iguana.be; robh+dt@kernel.org; pawel.moll@arm.com; > mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org > Cc: sylvain.rochet@finsecur.com; Ferre, Nicolas; boris.brezillon@free- > electrons.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature > support > > On 07/28/2015 05:38 PM, Yang, Wenyou wrote: > > Hi Guenter, > > > > Thank you very much for your review. > > > >> -----Original Message----- > >> From: Guenter Roeck [mailto:linux@roeck-us.net] > >> Sent: 2015??7??28?? 15:14 > >> To: Yang, Wenyou; wim@iguana.be; robh+dt@kernel.org; > >> pawel.moll@arm.com; mark.rutland@arm.com; > >> ijc+devicetree@hellion.org.uk; galak@codeaurora.org > >> Cc: sylvain.rochet@finsecur.com; Ferre, Nicolas; > >> boris.brezillon@free- electrons.com; devicetree@vger.kernel.org; > >> linux-kernel@vger.kernel.org; linux- watchdog@vger.kernel.org; > >> linux-arm-kernel@lists.infradead.org > >> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new > >> feature support > >> > >> On 07/28/2015 12:00 AM, Wenyou Yang wrote: > >>> In the datasheet, the new feature is describled as "WDT_MR can be > >>> written until a LOCKMR command is issued in WDT_CR". > >>> That is to say, as long as the bootstrap and u-boot don't issue a > >>> LOCKMR command, WDT_MR can be written in kernel. > >>> > >>> So the driver can enable/disable the watchdog timer hardware, set > >>> WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of > >>> WDT_MR register to set the watchdog timer timeout. > >>> > >>> The timer is not necessary that regularly sends a keepalive ping to > >>> the watchdog timer hardware. > >>> > >>> It is introduced from sama5d4 SoCs. > >>> > >> Since there are so many changes, I wonder is a separate driver would > >> make more sense. > > Yes, a bit many changes. > > I thought reuse the driver code. > > If a separate driver, I am afraid it includes much duplicated code. > > After all, it is for the same device with different feature. > > > > I don't think it is necessary to have multiple drivers for the same peripheral with > different feature. > > > > The concept for the two mechanisms is all different: In one, the watchdog > keepalive is triggered from timer code. In the other, the watchdog timeout is > triggered directly from the heartbeat function. One assumes that the watchdog is > always running, and that it must be pinged even if closed. The other disables the > watchdog on close. > > What I _can_ see is that the driver is becoming an unmaintainable mess, with lots > of if/else in pretty much every function. I consider this much less desirable than a > bit of code duplication - if there is any. Seriously, most of the added code might as > well be for a completely different chip. You are right, I accepted your advice. I will rewrite it. Thanks. > > Guenter Best Regards, Wenyou Yang ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?