Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Philippe Mathieu-Daudé]

On 25/3/24 14:47, Peter Maydell wrote:
> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Currently clock_set() returns whether the clock has
> > been changed or not. In order to combine this information
> > with other clock calls, pass an optional boolean and do
> > not return anything.  The single caller ignores the return
> > value, have it use NULL.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   include/hw/clock.h       | 22 ++++++++++++++++------
> >   hw/core/clock.c          |  8 +++++---
> >   hw/misc/bcm2835_cprman.c |  2 +-
> >   hw/misc/zynq_slcr.c      |  4 ++--
> >   4 files changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/hw/clock.h b/include/hw/clock.h
> > index bb12117f67..474bbc07fe 100644
> > --- a/include/hw/clock.h
> > +++ b/include/hw/clock.h
> > @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
> >    * clock_set:
> >    * @clk: the clock to initialize.
> >    * @value: the clock's value, 0 means unclocked
> > + * @changed: set to true if the clock is changed, ignored if set to NULL.
> >    *
> >    * Set the local cached period value of @clk to @value.
> > - *
> > - * @return: true if the clock is changed.
> >    */
> > -bool clock_set(Clock *clk, uint64_t value);
> > +void clock_set(Clock *clk, uint64_t period, bool *changed);
>
> What's wrong with using the return value? Generally
> returning a value via passing in a pointer is much
> clunkier in C than using the return value, so we only
> do it if we have to (e.g. the return value is already
> being used for something else, or we need to return
> more than one thing at once).

Then I'd rather remove (by inlining) the clock_update*() methods,
to have explicit calls to clock_propagate(), after multiple
clock_set*() calls. clock_update*() are used in 4 files:

$ git grep -l clock_update hw/misc/
hw/misc/bcm2835_cprman.c
hw/misc/npcm7xx_clk.c
hw/misc/npcm7xx_mft.c
hw/misc/stm32l4x5_rcc.c
$ git grep clock_update hw/misc/ | wc -l
      37

Regards,

Phil.