RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

[Yang, Wenyou]

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