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 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.