[PATCH 4/4] watchdog: Automatically register device with sysreset

[Simon Glass]

Hi Samuel,

On Wed, 3 Nov 2021 at 21:49, Samuel Holland <samuel at sholland.org> wrote:
>
> On 10/31/21 6:46 PM, Simon Glass wrote:
> > Hi Samuel,
> >
> > On Sun, 22 Aug 2021 at 14:41, Samuel Holland <samuel at sholland.org> wrote:
> >>
> >> Add an option to automatically register the first watchdog device with
> >> the wdt_reboot driver for use with sysreset. This allows sysreset to be
> >> a drop-in replacement for platform-specific watchdog reset code, without
> >> needing any device tree changes.
> >>
> >> Signed-off-by: Samuel Holland <samuel at sholland.org>
> >> ---
> >>
> >>  drivers/sysreset/Kconfig             |  7 +++++++
> >>  drivers/sysreset/sysreset_watchdog.c | 24 ++++++++++++++++++++++++
> >>  drivers/watchdog/wdt-uclass.c        |  5 +++++
> >>  include/sysreset.h                   | 14 ++++++++++++++
> >>  4 files changed, 50 insertions(+)
> >
> > This is a bit odd, but I suppose it is OK.
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> >>
> >> diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
> >> index fdc858ccbac..49354b1b27a 100644
> >> --- a/drivers/sysreset/Kconfig
> >> +++ b/drivers/sysreset/Kconfig
> >> @@ -119,6 +119,13 @@ config SYSRESET_WATCHDOG
> >>         help
> >>           Reboot support for generic watchdog reset.
> >>
> >> +config SYSRESET_WATCHDOG_AUTO
> >> +       bool "Automatically register first watchdog with sysreset"
> >> +       depends on SYSRESET_WATCHDOG
> >> +       help
> >> +         If enabled, the first watchdog (as selected by the watchdog uclass)
> >> +         will automatically be registered with the watchdog reboot driver.
> >> +
> >>  config SYSRESET_RESETCTL
> >>         bool "Enable support for reset controller reboot driver"
> >>         select DM_RESET
> >> diff --git a/drivers/sysreset/sysreset_watchdog.c b/drivers/sysreset/sysreset_watchdog.c
> >> index b723f5647cd..35efcac59dd 100644
> >> --- a/drivers/sysreset/sysreset_watchdog.c
> >> +++ b/drivers/sysreset/sysreset_watchdog.c
> >> @@ -5,7 +5,9 @@
> >>
> >>  #include <common.h>
> >>  #include <dm.h>
> >> +#include <dm/device-internal.h>
> >>  #include <errno.h>
> >> +#include <malloc.h>
> >>  #include <sysreset.h>
> >>  #include <wdt.h>
> >>
> >> @@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = {
> >>         .plat_auto      = sizeof(struct wdt_reboot_plat),
> >>         .ops = &wdt_reboot_ops,
> >>  };
> >> +
> >> +#if IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO)
> >> +int sysreset_register_wdt(struct udevice *dev)
> >
> > Could you pass in plat here instead of allocating it? You could add it
> > to wdt-uclass.c as some private data for the uclass...
>
> Yes, that is a good idea, especially now that all watchdogs get
> initialized during boot (so this code also applies to all watchdogs).
>
> It would be simplest to use the plat pointer directly as the watchdog
> reference (otherwise the watchdog needs to store a pointer to itself in
> its uclass priv, which is a bit strange). But that would require using
> dev_set_plat in the wdt_reboot of_to_plat case. I wonder if that is an
> appropriate use of this API? Not much else uses dev_set_plat.
>
> So I'd like to do this as a follow-up.

Yes that's fine, you have the review tag. I was thinking something like:

/* private data for WDT uclass */
struct wdt_priv {
   struct wdt_reboot_plat wdt_reboot;
};

/* pass plat to device_bind()
device_bind(...&priv->wdt_reboot...)

Needs a little thought of course, as you say. I am not keen on using
dev_set_plat() after binding if we can help it.

[..]

Regards,
Simon