Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1194762

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

Show all headers | View raw

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.

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/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/2] watchdog: at91sam9_wdt: add new feature support Wenyou Yang <wenyou.yang@atmel.com> - 2015-07-28 09:10 +0200
  [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support Wenyou Yang <wenyou.yang@atmel.com> - 2015-07-28 09:10 +0200
    Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature  support Guenter Roeck <linux@roeck-us.net> - 2015-07-28 09:20 +0200
      RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature  support "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2015-07-29 02:40 +0200
        Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature  support Guenter Roeck <linux@roeck-us.net> - 2015-07-29 03:30 +0200
          RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature  support "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2015-07-29 04:00 +0200
  [PATCH 2/2] Documentation: dt: binding: atmel-wdt: add a new compitable Wenyou Yang <wenyou.yang@atmel.com> - 2015-07-28 09:10 +0200

csiph-web