|
| 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
| [Prev in Thread] | Current Thread | [Next in Thread] |