| Message ID | 20240509004714.1394547-4-rasmus.villemoes@prevas.dk |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | cyclic/watchdog patches | expand |
Hi Rasmus, On 5/9/24 02:47, Rasmus Villemoes wrote: > There are of course not a whole lot of examples in-tree yet, but > before they appear, let's make this API change: Instead of separately > allocating a 'struct cyclic_info', make the users embed such an > instance in their own structure, and make the convention that the > callback simply receives the 'struct cyclic_info *', from which the > clients can get their own data using the container_of() macro. > > This has a number of advantages. > > First, it means cyclic_register() simply cannot fail, simplifying the > code. The necessary storage will simply be allocated automatically > when the client's own structure is allocated (often via > uclass_priv_auto or similar). > > Second, code for which CONFIG_CYCLIC is just an option can more easily > be written without #ifdefs, if we just provide an empty struct > cyclic_info {}. For example, the nested CONFIG_IS_ENABLED()s in > https://lore.kernel.org/u-boot/20240316201416.211480-1-marek.vasut+renesas@mailbox.org/ > are mostly due to the existence of the 'struct cyclic_info *' member > being guarded by #ifdef CONFIG_CYCLIC. > > And we do probably want to avoid the extra memory overhead of that > member when !CONFIG_CYCLIC. But that is automatic if, instead of a > 'struct cyclic_info *', one simply embeds a 'struct cyclic_info', > which will have size 0 when !CONFIG_CYCLIC. Also, the no-op > cyclic_register() function can just unconditionally be called, and the > compiler will see that (1) the callback is referenced, so not emit a > warning for a maybe-unused function and (2) see that it can actually > never be reached, so not emit any code for it. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > board/Marvell/octeon_nic23/board.c | 9 +++++--- > cmd/cyclic.c | 12 +++++------ > common/cyclic.c | 22 +++++-------------- > doc/develop/cyclic.rst | 26 ++++++++++++++--------- > drivers/watchdog/wdt-uclass.c | 33 +++++++++++++---------------- > include/cyclic.h | 34 +++++++++++++++--------------- > 6 files changed, 64 insertions(+), 72 deletions(-) I like this approach. And would like to pull this in as well, perhaps before leaving on a 2 week vacation mid of next week. Unfortunately this patch 3/3 does not apply on TOT any more. Could you please rebase and re-submit? Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > diff --git a/board/Marvell/octeon_nic23/board.c b/board/Marvell/octeon_nic23/board.c > index bc9332cb74a..74b9c741b7b 100644 > --- a/board/Marvell/octeon_nic23/board.c > +++ b/board/Marvell/octeon_nic23/board.c > @@ -357,10 +357,13 @@ int board_late_init(void) > board_configure_qlms(); > > /* Register cyclic function for PCIe FLR fixup */ > - cyclic = cyclic_register(octeon_board_restore_pf, 100, > - "pcie_flr_fix", NULL); > - if (!cyclic) > + cyclic = calloc(1, sizeof(*cyclic)); > + if (cyclic) { > + cyclic_register(cyclic, octeon_board_restore_pf, 100, > + "pcie_flr_fix"); > + } else { > printf("Registering of cyclic function failed\n"); > + } > > return 0; > } > diff --git a/cmd/cyclic.c b/cmd/cyclic.c > index ad7fc3b975e..315546515f0 100644 > --- a/cmd/cyclic.c > +++ b/cmd/cyclic.c > @@ -14,14 +14,16 @@ > #include <div64.h> > #include <malloc.h> > #include <linux/delay.h> > +#include <linux/kernel.h> > > struct cyclic_demo_info { > + struct cyclic_info cyclic; > uint delay_us; > }; > > -static void cyclic_demo(void *ctx) > +static void cyclic_demo(struct cyclic_info *c) > { > - struct cyclic_demo_info *info = ctx; > + struct cyclic_demo_info *info = container_of(c, struct cyclic_demo_info, cyclic); > > /* Just a small dummy delay here */ > udelay(info->delay_us); > @@ -31,7 +33,6 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > { > struct cyclic_demo_info *info; > - struct cyclic_info *cyclic; > uint time_ms; > > if (argc < 3) > @@ -47,10 +48,7 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc, > info->delay_us = simple_strtoul(argv[2], NULL, 0); > > /* Register demo cyclic function */ > - cyclic = cyclic_register(cyclic_demo, time_ms * 1000, "cyclic_demo", > - info); > - if (!cyclic) > - printf("Registering of cyclic_demo failed\n"); > + cyclic_register(&info->cyclic, cyclic_demo, time_ms * 1000, "cyclic_demo"); > > printf("Registered function \"%s\" to be executed all %dms\n", > "cyclic_demo", time_ms); > diff --git a/common/cyclic.c b/common/cyclic.c > index c62e7fa7d19..ec38fad6775 100644 > --- a/common/cyclic.c > +++ b/common/cyclic.c > @@ -26,34 +26,22 @@ struct hlist_head *cyclic_get_list(void) > return (struct hlist_head *)&gd->cyclic_list; > } > > -struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us, > - const char *name, void *ctx) > +void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, > + uint64_t delay_us, const char *name) > { > - struct cyclic_info *cyclic; > - > - cyclic = calloc(1, sizeof(struct cyclic_info)); > - if (!cyclic) { > - pr_debug("Memory allocation error\n"); > - return NULL; > - } > + memset(cyclic, 0, sizeof(*cyclic)); > > /* Store values in struct */ > cyclic->func = func; > - cyclic->ctx = ctx; > cyclic->name = name; > cyclic->delay_us = delay_us; > cyclic->start_time_us = timer_get_us(); > hlist_add_head(&cyclic->list, cyclic_get_list()); > - > - return cyclic; > } > > -int cyclic_unregister(struct cyclic_info *cyclic) > +void cyclic_unregister(struct cyclic_info *cyclic) > { > hlist_del(&cyclic->list); > - free(cyclic); > - > - return 0; > } > > void cyclic_run(void) > @@ -76,7 +64,7 @@ void cyclic_run(void) > if (time_after_eq64(now, cyclic->next_call)) { > /* Call cyclic function and account it's cpu-time */ > cyclic->next_call = now + cyclic->delay_us; > - cyclic->func(cyclic->ctx); > + cyclic->func(cyclic); > cyclic->run_cnt++; > cpu_time = timer_get_us() - now; > cyclic->cpu_time_us += cpu_time; > diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst > index 67831496a70..bcc1ca6ce6d 100644 > --- a/doc/develop/cyclic.rst > +++ b/doc/develop/cyclic.rst > @@ -19,20 +19,26 @@ Registering a cyclic function > > To register a cyclic function, use something like this:: > > - static void cyclic_demo(void *ctx) > + struct donkey { > + struct cyclic_info cyclic; > + void (*say)(const char *s); > + }; > + > + static void cyclic_demo(struct cyclic_info *c) > { > - /* Just a small dummy delay here */ > - udelay(10); > + struct donkey *donkey = container_of(c, struct donkey, cyclic); > + > + donkey->say("Are we there yet?"); > } > - > - int board_init(void) > + > + int donkey_init(void) > { > - struct cyclic_info *cyclic; > - > + struct donkey *donkey; > + > + /* Initialize donkey ... */ > + > /* Register demo cyclic function */ > - cyclic = cyclic_register(cyclic_demo, 10 * 1000, "cyclic_demo", NULL); > - if (!cyclic) > - printf("Registering of cyclic_demo failed\n"); > + cyclic_register(&donkey->cyclic, cyclic_demo, 10 * 1000, "cyclic_demo"); > > return 0; > } > diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c > index caa1567e0c3..c0332f39dfb 100644 > --- a/drivers/watchdog/wdt-uclass.c > +++ b/drivers/watchdog/wdt-uclass.c > @@ -18,12 +18,15 @@ > #include <asm/global_data.h> > #include <dm/device-internal.h> > #include <dm/lists.h> > +#include <linux/kernel.h> > > DECLARE_GLOBAL_DATA_PTR; > > #define WATCHDOG_TIMEOUT_SECS (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000) > > struct wdt_priv { > + /* The udevice owning this wdt_priv. */ > + struct udevice *dev; > /* Timeout, in seconds, to configure this device to. */ > u32 timeout; > /* > @@ -41,18 +44,17 @@ struct wdt_priv { > /* autostart */ > bool autostart; > > - struct cyclic_info *cyclic; > + struct cyclic_info cyclic; > }; > > -static void wdt_cyclic(void *ctx) > +static void wdt_cyclic(struct cyclic_info *c) > { > - struct udevice *dev = ctx; > - struct wdt_priv *priv; > + struct wdt_priv *priv = container_of(c, struct wdt_priv, cyclic); > + struct udevice *dev = priv->dev; > > if (!device_active(dev)) > return; > > - priv = dev_get_uclass_priv(dev); > if (!priv->running) > return; > > @@ -125,20 +127,14 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) > memset(str, 0, 16); > if (IS_ENABLED(CONFIG_WATCHDOG)) { > if (priv->running) > - cyclic_unregister(priv->cyclic); > + cyclic_unregister(&priv->cyclic); > > /* Register the watchdog driver as a cyclic function */ > - priv->cyclic = cyclic_register(wdt_cyclic, > - priv->reset_period * 1000, > - dev->name, dev); > - if (!priv->cyclic) { > - printf("cyclic_register for %s failed\n", > - dev->name); > - return -ENODEV; > - } else { > - snprintf(str, 16, "every %ldms", > - priv->reset_period); > - } > + cyclic_register(&priv->cyclic, wdt_cyclic, > + priv->reset_period * 1000, > + dev->name); > + > + snprintf(str, 16, "every %ldms", priv->reset_period); > } > > priv->running = true; > @@ -163,7 +159,7 @@ int wdt_stop(struct udevice *dev) > struct wdt_priv *priv = dev_get_uclass_priv(dev); > > if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running) > - cyclic_unregister(priv->cyclic); > + cyclic_unregister(&priv->cyclic); > > priv->running = false; > } > @@ -263,6 +259,7 @@ static int wdt_pre_probe(struct udevice *dev) > autostart = true; > } > priv = dev_get_uclass_priv(dev); > + priv->dev = dev; > priv->timeout = timeout; > priv->reset_period = reset_period; > priv->autostart = autostart; > diff --git a/include/cyclic.h b/include/cyclic.h > index 38946216fb8..dc0749ba03d 100644 > --- a/include/cyclic.h > +++ b/include/cyclic.h > @@ -18,7 +18,6 @@ > * struct cyclic_info - Information about cyclic execution function > * > * @func: Function to call periodically > - * @ctx: Context pointer to get passed to this function > * @name: Name of the cyclic function, e.g. shown in the commands > * @delay_ns: Delay is ns after which this function shall get executed > * @start_time_us: Start time in us, when this function started its execution > @@ -29,8 +28,7 @@ > * @already_warned: Flag that we've warned about exceeding CPU time usage > */ > struct cyclic_info { > - void (*func)(void *ctx); > - void *ctx; > + void (*func)(struct cyclic_info *c); > const char *name; > uint64_t delay_us; > uint64_t start_time_us; > @@ -42,28 +40,30 @@ struct cyclic_info { > }; > > /** Function type for cyclic functions */ > -typedef void (*cyclic_func_t)(void *ctx); > +typedef void (*cyclic_func_t)(struct cyclic_info *c); > > #if defined(CONFIG_CYCLIC) > /** > * cyclic_register - Register a new cyclic function > * > + * @cyclic: Cyclic info structure > * @func: Function to call periodically > * @delay_us: Delay is us after which this function shall get executed > * @name: Cyclic function name/id > - * @ctx: Context to pass to the function > - * @return: pointer to cyclic_struct if OK, NULL on error > + * > + * The function @func will be called with @cyclic as its > + * argument. @cyclic will usually be embedded in some device-specific > + * structure, which the callback can retrieve using container_of(). > */ > -struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us, > - const char *name, void *ctx); > +void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, > + uint64_t delay_us, const char *name); > > /** > * cyclic_unregister - Unregister a cyclic function > * > * @cyclic: Pointer to cyclic_struct of the function that shall be removed > - * @return: 0 if OK, -ve on error > */ > -int cyclic_unregister(struct cyclic_info *cyclic); > +void cyclic_unregister(struct cyclic_info *cyclic); > > /** > * cyclic_unregister_all() - Clean up cyclic functions > @@ -97,17 +97,17 @@ void cyclic_run(void); > */ > void schedule(void); > #else > -static inline struct cyclic_info *cyclic_register(cyclic_func_t func, > - uint64_t delay_us, > - const char *name, > - void *ctx) > + > +struct cyclic_info { > +}; > + > +static inline void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, > + uint64_t delay_us, const char *name) > { > - return NULL; > } > > -static inline int cyclic_unregister(struct cyclic_info *cyclic) > +static inline void cyclic_unregister(struct cyclic_info *cyclic) > { > - return 0; > } > > static inline void cyclic_run(void) Viele Grüße, Stefan Roese
diff --git a/board/Marvell/octeon_nic23/board.c b/board/Marvell/octeon_nic23/board.c index bc9332cb74a..74b9c741b7b 100644 --- a/board/Marvell/octeon_nic23/board.c +++ b/board/Marvell/octeon_nic23/board.c @@ -357,10 +357,13 @@ int board_late_init(void) board_configure_qlms(); /* Register cyclic function for PCIe FLR fixup */ - cyclic = cyclic_register(octeon_board_restore_pf, 100, - "pcie_flr_fix", NULL); - if (!cyclic) + cyclic = calloc(1, sizeof(*cyclic)); + if (cyclic) { + cyclic_register(cyclic, octeon_board_restore_pf, 100, + "pcie_flr_fix"); + } else { printf("Registering of cyclic function failed\n"); + } return 0; } diff --git a/cmd/cyclic.c b/cmd/cyclic.c index ad7fc3b975e..315546515f0 100644 --- a/cmd/cyclic.c +++ b/cmd/cyclic.c @@ -14,14 +14,16 @@ #include <div64.h> #include <malloc.h> #include <linux/delay.h> +#include <linux/kernel.h> struct cyclic_demo_info { + struct cyclic_info cyclic; uint delay_us; }; -static void cyclic_demo(void *ctx) +static void cyclic_demo(struct cyclic_info *c) { - struct cyclic_demo_info *info = ctx; + struct cyclic_demo_info *info = container_of(c, struct cyclic_demo_info, cyclic); /* Just a small dummy delay here */ udelay(info->delay_us); @@ -31,7 +33,6 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct cyclic_demo_info *info; - struct cyclic_info *cyclic; uint time_ms; if (argc < 3) @@ -47,10 +48,7 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc, info->delay_us = simple_strtoul(argv[2], NULL, 0); /* Register demo cyclic function */ - cyclic = cyclic_register(cyclic_demo, time_ms * 1000, "cyclic_demo", - info); - if (!cyclic) - printf("Registering of cyclic_demo failed\n"); + cyclic_register(&info->cyclic, cyclic_demo, time_ms * 1000, "cyclic_demo"); printf("Registered function \"%s\" to be executed all %dms\n", "cyclic_demo", time_ms); diff --git a/common/cyclic.c b/common/cyclic.c index c62e7fa7d19..ec38fad6775 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -26,34 +26,22 @@ struct hlist_head *cyclic_get_list(void) return (struct hlist_head *)&gd->cyclic_list; } -struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us, - const char *name, void *ctx) +void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, + uint64_t delay_us, const char *name) { - struct cyclic_info *cyclic; - - cyclic = calloc(1, sizeof(struct cyclic_info)); - if (!cyclic) { - pr_debug("Memory allocation error\n"); - return NULL; - } + memset(cyclic, 0, sizeof(*cyclic)); /* Store values in struct */ cyclic->func = func; - cyclic->ctx = ctx; cyclic->name = name; cyclic->delay_us = delay_us; cyclic->start_time_us = timer_get_us(); hlist_add_head(&cyclic->list, cyclic_get_list()); - - return cyclic; } -int cyclic_unregister(struct cyclic_info *cyclic) +void cyclic_unregister(struct cyclic_info *cyclic) { hlist_del(&cyclic->list); - free(cyclic); - - return 0; } void cyclic_run(void) @@ -76,7 +64,7 @@ void cyclic_run(void) if (time_after_eq64(now, cyclic->next_call)) { /* Call cyclic function and account it's cpu-time */ cyclic->next_call = now + cyclic->delay_us; - cyclic->func(cyclic->ctx); + cyclic->func(cyclic); cyclic->run_cnt++; cpu_time = timer_get_us() - now; cyclic->cpu_time_us += cpu_time; diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst index 67831496a70..bcc1ca6ce6d 100644 --- a/doc/develop/cyclic.rst +++ b/doc/develop/cyclic.rst @@ -19,20 +19,26 @@ Registering a cyclic function To register a cyclic function, use something like this:: - static void cyclic_demo(void *ctx) + struct donkey { + struct cyclic_info cyclic; + void (*say)(const char *s); + }; + + static void cyclic_demo(struct cyclic_info *c) { - /* Just a small dummy delay here */ - udelay(10); + struct donkey *donkey = container_of(c, struct donkey, cyclic); + + donkey->say("Are we there yet?"); } - - int board_init(void) + + int donkey_init(void) { - struct cyclic_info *cyclic; - + struct donkey *donkey; + + /* Initialize donkey ... */ + /* Register demo cyclic function */ - cyclic = cyclic_register(cyclic_demo, 10 * 1000, "cyclic_demo", NULL); - if (!cyclic) - printf("Registering of cyclic_demo failed\n"); + cyclic_register(&donkey->cyclic, cyclic_demo, 10 * 1000, "cyclic_demo"); return 0; } diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index caa1567e0c3..c0332f39dfb 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -18,12 +18,15 @@ #include <asm/global_data.h> #include <dm/device-internal.h> #include <dm/lists.h> +#include <linux/kernel.h> DECLARE_GLOBAL_DATA_PTR; #define WATCHDOG_TIMEOUT_SECS (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000) struct wdt_priv { + /* The udevice owning this wdt_priv. */ + struct udevice *dev; /* Timeout, in seconds, to configure this device to. */ u32 timeout; /* @@ -41,18 +44,17 @@ struct wdt_priv { /* autostart */ bool autostart; - struct cyclic_info *cyclic; + struct cyclic_info cyclic; }; -static void wdt_cyclic(void *ctx) +static void wdt_cyclic(struct cyclic_info *c) { - struct udevice *dev = ctx; - struct wdt_priv *priv; + struct wdt_priv *priv = container_of(c, struct wdt_priv, cyclic); + struct udevice *dev = priv->dev; if (!device_active(dev)) return; - priv = dev_get_uclass_priv(dev); if (!priv->running) return; @@ -125,20 +127,14 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) memset(str, 0, 16); if (IS_ENABLED(CONFIG_WATCHDOG)) { if (priv->running) - cyclic_unregister(priv->cyclic); + cyclic_unregister(&priv->cyclic); /* Register the watchdog driver as a cyclic function */ - priv->cyclic = cyclic_register(wdt_cyclic, - priv->reset_period * 1000, - dev->name, dev); - if (!priv->cyclic) { - printf("cyclic_register for %s failed\n", - dev->name); - return -ENODEV; - } else { - snprintf(str, 16, "every %ldms", - priv->reset_period); - } + cyclic_register(&priv->cyclic, wdt_cyclic, + priv->reset_period * 1000, + dev->name); + + snprintf(str, 16, "every %ldms", priv->reset_period); } priv->running = true; @@ -163,7 +159,7 @@ int wdt_stop(struct udevice *dev) struct wdt_priv *priv = dev_get_uclass_priv(dev); if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running) - cyclic_unregister(priv->cyclic); + cyclic_unregister(&priv->cyclic); priv->running = false; } @@ -263,6 +259,7 @@ static int wdt_pre_probe(struct udevice *dev) autostart = true; } priv = dev_get_uclass_priv(dev); + priv->dev = dev; priv->timeout = timeout; priv->reset_period = reset_period; priv->autostart = autostart; diff --git a/include/cyclic.h b/include/cyclic.h index 38946216fb8..dc0749ba03d 100644 --- a/include/cyclic.h +++ b/include/cyclic.h @@ -18,7 +18,6 @@ * struct cyclic_info - Information about cyclic execution function * * @func: Function to call periodically - * @ctx: Context pointer to get passed to this function * @name: Name of the cyclic function, e.g. shown in the commands * @delay_ns: Delay is ns after which this function shall get executed * @start_time_us: Start time in us, when this function started its execution @@ -29,8 +28,7 @@ * @already_warned: Flag that we've warned about exceeding CPU time usage */ struct cyclic_info { - void (*func)(void *ctx); - void *ctx; + void (*func)(struct cyclic_info *c); const char *name; uint64_t delay_us; uint64_t start_time_us; @@ -42,28 +40,30 @@ struct cyclic_info { }; /** Function type for cyclic functions */ -typedef void (*cyclic_func_t)(void *ctx); +typedef void (*cyclic_func_t)(struct cyclic_info *c); #if defined(CONFIG_CYCLIC) /** * cyclic_register - Register a new cyclic function * + * @cyclic: Cyclic info structure * @func: Function to call periodically * @delay_us: Delay is us after which this function shall get executed * @name: Cyclic function name/id - * @ctx: Context to pass to the function - * @return: pointer to cyclic_struct if OK, NULL on error + * + * The function @func will be called with @cyclic as its + * argument. @cyclic will usually be embedded in some device-specific + * structure, which the callback can retrieve using container_of(). */ -struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us, - const char *name, void *ctx); +void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, + uint64_t delay_us, const char *name); /** * cyclic_unregister - Unregister a cyclic function * * @cyclic: Pointer to cyclic_struct of the function that shall be removed - * @return: 0 if OK, -ve on error */ -int cyclic_unregister(struct cyclic_info *cyclic); +void cyclic_unregister(struct cyclic_info *cyclic); /** * cyclic_unregister_all() - Clean up cyclic functions @@ -97,17 +97,17 @@ void cyclic_run(void); */ void schedule(void); #else -static inline struct cyclic_info *cyclic_register(cyclic_func_t func, - uint64_t delay_us, - const char *name, - void *ctx) + +struct cyclic_info { +}; + +static inline void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, + uint64_t delay_us, const char *name) { - return NULL; } -static inline int cyclic_unregister(struct cyclic_info *cyclic) +static inline void cyclic_unregister(struct cyclic_info *cyclic) { - return 0; } static inline void cyclic_run(void)
There are of course not a whole lot of examples in-tree yet, but before they appear, let's make this API change: Instead of separately allocating a 'struct cyclic_info', make the users embed such an instance in their own structure, and make the convention that the callback simply receives the 'struct cyclic_info *', from which the clients can get their own data using the container_of() macro. This has a number of advantages. First, it means cyclic_register() simply cannot fail, simplifying the code. The necessary storage will simply be allocated automatically when the client's own structure is allocated (often via uclass_priv_auto or similar). Second, code for which CONFIG_CYCLIC is just an option can more easily be written without #ifdefs, if we just provide an empty struct cyclic_info {}. For example, the nested CONFIG_IS_ENABLED()s in https://lore.kernel.org/u-boot/20240316201416.211480-1-marek.vasut+renesas@mailbox.org/ are mostly due to the existence of the 'struct cyclic_info *' member being guarded by #ifdef CONFIG_CYCLIC. And we do probably want to avoid the extra memory overhead of that member when !CONFIG_CYCLIC. But that is automatic if, instead of a 'struct cyclic_info *', one simply embeds a 'struct cyclic_info', which will have size 0 when !CONFIG_CYCLIC. Also, the no-op cyclic_register() function can just unconditionally be called, and the compiler will see that (1) the callback is referenced, so not emit a warning for a maybe-unused function and (2) see that it can actually never be reached, so not emit any code for it. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- board/Marvell/octeon_nic23/board.c | 9 +++++--- cmd/cyclic.c | 12 +++++------ common/cyclic.c | 22 +++++-------------- doc/develop/cyclic.rst | 26 ++++++++++++++--------- drivers/watchdog/wdt-uclass.c | 33 +++++++++++++---------------- include/cyclic.h | 34 +++++++++++++++--------------- 6 files changed, 64 insertions(+), 72 deletions(-)