Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks

[Philippe Mathieu-Daudé]

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>