qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()
Date: Mon, 25 Mar 2024 16:11:26 +0100
User-agent: Mozilla Thunderbird

On 25/3/24 16:03, Peter Maydell wrote:
On Mon, 25 Mar 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

On 25/3/24 15:44, Peter Maydell wrote:
On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

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.

You mean, so that we handle "set the clock period" and
"set the mul/div" the same way, by just setting them and making
it always the caller's responsibility to call clock_propagate() ?

Yes, for consistency, to have the clock_set* family behaving
the same way.

Would you keep the bool return for clock_set and clock_set_mul_div
to tell the caller whether a clock_propagate() call is needed ?

Yes (sorry for not being clearer). The API change would be
less invasive, possibly acceptable during the freeze.

Sounds reasonable as an API to me. The other place we currently
do an implicit clock_propagate() is from clock_set_source().
Should we make that require explicit propagate too?

For API consistency, I'd rather do the same. Luc, any objection?

For freeze: is there a way to fix this bug without changing all the
clock APIs first?

Sure, I'll respin that for Arnaud.


thanks
-- PMM




reply via email to

[Prev in Thread] Current Thread [Next in Thread]