Hi Rasmus, On Mon, 28 Jun 2021 at 01:44, Rasmus Villemoes <rasmus.villem...@prevas.dk> wrote: > > On 26/06/2021 20.32, Simon Glass wrote: > > Hi Rasmus, > > > > On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes > > <rasmus.villem...@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.
>From a driver model perspective, it is good practice to have a 'start' method separate from probing. I understand that you are saying that the watchdog should always be enabled, but we can handle that with a separate method to start it. I don't think it needs to be started in the post-probe method. We can just start it later. Imagine you probe the device but then go into SDRAM init that hangs the CPU for a few seconds. We need a way to signal that we want the watchdog to start, so the board owner has a choice as to when this happens. I also understand this is not quite how things work at present and I'm fine with copying the existing behaviour. It's just that you are introducing a driver model interface and I have pretty strong views about the separation between probe at start, with that. Regards, Simon