[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks |
Date: |
Thu, 11 Feb 2021 16:29:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
Hi Peter,
On 2/9/21 2:20 PM, Peter Maydell wrote:
> The Clock framework allows users to specify a callback which is
> called after the clock's period has been updated. Some users need to
> also have a callback which is called before the clock period is
> updated.
>
> As the first step in adding support for notifying Clock users on
> pre-update events, add an argument to the ClockCallback to specify
> what event is being notified, and add an argument to the various
> functions for registering a callback to specify which events are
> of interest to that callback.
>
> Note that the documentation update renders correct the previously
> incorrect claim in 'Adding a new clock' that callbacks "will be
> explained in a following section".
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: (suggested by Luc) instead of making callback functions check
> whether 'event' is one they are interested in, specify mask of
> interesting events at callback registration time.
> ---
> docs/devel/clocks.rst | 52 +++++++++++++++++++++++++++-----
> include/hw/clock.h | 21 +++++++++++--
> include/hw/qdev-clock.h | 17 ++++++++---
> hw/adc/npcm7xx_adc.c | 2 +-
> hw/arm/armsse.c | 9 +++---
> hw/char/cadence_uart.c | 4 +--
> hw/char/ibex_uart.c | 4 +--
> hw/char/pl011.c | 5 +--
> hw/core/clock.c | 20 +++++++++---
> hw/core/qdev-clock.c | 8 +++--
> hw/mips/cps.c | 2 +-
> hw/misc/bcm2835_cprman.c | 23 ++++++++------
> hw/misc/npcm7xx_clk.c | 26 +++++++++++++---
> hw/misc/npcm7xx_pwm.c | 2 +-
> hw/misc/zynq_slcr.c | 5 +--
> hw/timer/cmsdk-apb-dualtimer.c | 5 +--
> hw/timer/cmsdk-apb-timer.c | 4 +--
> hw/timer/npcm7xx_timer.c | 2 +-
> hw/watchdog/cmsdk-apb-watchdog.c | 5 +--
> target/mips/cpu.c | 2 +-
> 20 files changed, 160 insertions(+), 58 deletions(-)
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index e5f45e2626d..5c73b4e7ae9 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -22,7 +22,17 @@
> #define TYPE_CLOCK "clock"
> OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
>
> -typedef void ClockCallback(void *opaque);
> +/*
> + * Argument to ClockCallback functions indicating why the callback
> + * has been called. A mask of these values logically ORed together
> + * is used to specify which events are interesting when the callback
> + * is registered, so these values must all be different bit values.
> + */
> +typedef enum ClockEvent {
> + ClockUpdate = 1, /* Clock period has just updated */
Just wondering loudly (QEMU doesn't have enum naming conventions),
maybe rename ClockUpdate -> ClockUpdateEvent to clarify.
> +} ClockEvent;
> +
> +typedef void ClockCallback(void *opaque, ClockEvent event);
>
> /*
> * clock store a value representing the clock's period in 2^-32ns unit.
> @@ -50,6 +60,7 @@ typedef void ClockCallback(void *opaque);
> * @canonical_path: clock path string cache (used for trace purpose)
> * @callback: called when clock changes
> * @callback_opaque: argument for @callback
> + * @callback_events: mask of events when callback should be called
> * @source: source (or parent in clock tree) of the clock
> * @children: list of clocks connected to this one (it is their source)
> * @sibling: structure used to form a clock list
> @@ -67,6 +78,7 @@ struct Clock {
> char *canonical_path;
> ClockCallback *callback;
> void *callback_opaque;
> + int callback_events;
Isn't "unsigned" recommended for bit mask?
Otherwise (minor Hao Wu's NULL remark):
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>