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

[Guenter Roeck]

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.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/