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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.

> 
> thanks
> -- PMM