Re: [PATCH v2 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 26/06/2021 20.32, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>

>>> I don't think it is good to start it in the post-probe. Can you do it
>>> separately, afterwards?
>>
>> Eh, yes, of course I could do this in the loop in initr_watchdog() where
>> I probe all watchdog devices, but the end result is exactly the same,
>> and it seemed that this was a perfect fit for DM since it provided this
>> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
>> and if it is, that precisely means the developer wanted to start
>> handling the device(s) ASAP.
>>
>>> Probing is supposed to just probe it and it
>>> should be safe to do that without side effects.
>>
>> I agree in general, but watchdog devices are a bit special compared to
>> some random USB controller or LCD display or spi master - those devices
>> generally don't do anything unless asked to by the CPU, while a watchdog
>> device is the other way around, it does its thing _unless_ the CPU asks
>> it not to (often enough). Which in turn, I suppose, is the whole reason
>> wdt-uclass has its own hook into the initr sequence - one needs to probe
>> and possibly start handling the watchdog(s) ASAP.
> 
> It still needs a 'start' method to make it start. Having it start on
> probe means the board will reset at the command line if the device is
> probed. Yuck.

No, because while sitting in the command line waiting for user input,
WATCHDOG_RESET() is called something like a million times per second (or
at least extremely often). For the most common case of there only being
one (or zero) DM watchdogs, I'm not changing anything at all about how
things behave. I'm just expanding the handling done in the wdt-uclass
provided functions initr_watchdog() and watchdog_reset() to all DM
watchdogs, making things more consistent. And there's
CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe
function into a no-op.

As I said, yes, I can move the call of the post_probe function into the
loop in initr_watchdog (and then it wouldn't be called post_probe, but
probably named something including auto_start). In practice, that won't
change anything.

Stefan, what do you think? I think this is the only contentious point at
this time, so I'll do whatever you think is right, then resend the
patches with Simon's other feedback incorporated.

Rasmus