[PATCH v2 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Sun, Jun 28, 2015 at 6:17 PM, Shawn Guo <shawnguo at kernel.org> wrote:
> On Thu, Jun 04, 2015 at 01:15:59PM -0700, Tim Harvey wrote:
>> @@ -90,6 +92,11 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>>       struct imx2_wdt_device *wdev = container_of(this,
>>                                                   struct imx2_wdt_device,
>>                                                   restart_handler);
>> +
>> +     /* Use external reset */
>> +     if (wdev->ext_reset)
>> +             wcr_enable |= IMX2_WDT_WCR_SRS;
>> +
>
> The existing code simply writes the register with only WDE (Watchdog
> Enable) bit set.  That said, bit WDA and SRS are cleared at the same
> time.  What's the result of that?  Both internal reset and external
> reset will be asserted?  If this is the case, it's unsafe per your reply
> to Frank's comment, right?  So we should only clear one bit between
> these two based on ext_reset flag to avoid asserting both internal and
> external reset?
>
> Shawn

Shawn,

Correct.

Are you saying that you think we should change the above to something
like the following to ensure in the internal reset case external reset
is not also being asserted?:

+     /* Use external reset */
+     if (wdev->ext_reset)
+             wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert
internal system reset */
+     else
+             wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert external reset */

I was not worried about the else case above because if you hit that
path your device-tree is saying you are not using external reset
anyway.

Tim

>
>>       /* Assert SRS signal */
>>       regmap_write(wdev->regmap, 0, wcr_enable);
>>       /*