Hi Guenter, Thank you for your prompt answer. > -----Original Message----- > From: Guenter Roeck [mailto:linux@xxxxxxxxxxxx] > Sent: 2015年7月29日 9:23 > To: Yang, Wenyou; wim@xxxxxxxxx; robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx; > mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx > Cc: sylvain.rochet@xxxxxxxxxxxx; Ferre, Nicolas; boris.brezillon@free- > electrons.com; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > watchdog@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxx] > >> Sent: 2015年7月28日 15:14 > >> To: Yang, Wenyou; wim@xxxxxxxxx; robh+dt@xxxxxxxxxx; > >> pawel.moll@xxxxxxx; mark.rutland@xxxxxxx; > >> ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx > >> Cc: sylvain.rochet@xxxxxxxxxxxx; Ferre, Nicolas; > >> boris.brezillon@free- electrons.com; devicetree@xxxxxxxxxxxxxxx; > >> linux-kernel@xxxxxxxxxxxxxxx; linux- watchdog@xxxxxxxxxxxxxxx; > >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >> 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????z谵{???塄}?财??j:+v?? ? ???2??璀??摺?囤??z夸z罐?+?????w棹f
