Re: [PATCH v1] rockchip: spl: replace ifdef by IS_ENABLED for timer_init() call condition

[Johan Jonker <jbx6244@gmail.com>]

On 7/24/26 10:45, Quentin Schulz wrote:
> Hi Johan,
> 
> On 7/24/26 12:17 AM, Johan Jonker wrote:
>> Not all Rockchip SoC models use the ARM arch timer.
>> Call the function timer_init() only when
>> CONFIG_SYS_ARCH_TIMER is available.
>> Replace the ifdef call condition by IS_ENABLED
>> to increase build coverage and make the code easier to read.
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Previous version not needed for serie, so resend separate.
>> https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
>>
> 

Hi Quentin,
> You didn't answer Kever's question in the linked patch and I have the same question.

Yes we end up the same code. But...

> 
> This is essentially the same code, so what's the benefit, are you trying to fix a specific issue? How does this improve the situation?
 > How does doing that increase code coverage... etc :)

This patch originates around the time this concept as introduced.
We are changing all code to the new norm and we leave this as it is...
Fix this as well as a favor to Simon as part of the review. As we are there then fix them all as this is the new norm.
  
https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-6-jbx6244@gmail.com/

The concept:

Currently with #ifdef the compiler sees this code:
============= 

	rockchip_stimer_init();

	ret = dram_init();

=============

Now the compiler sees this code:


int timer_init(void)
{
	gd->arch.tbl = 0;
	gd->arch.tbu = 0;

#ifdef CFG_SYS_HZ_CLOCK
	gd->arch.timer_rate_hz = CFG_SYS_HZ_CLOCK;
#else
	gd->arch.timer_rate_hz = read_cntfrq();
#endif
	return 0;
}



	rockchip_stimer_init();

	if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
		timer_init();

	ret = dram_init();

============

By using IS_ENABLED and CONFIG_IS_ENABLED the compiler is able to look further into code and catch possible errors or warnings.
There is even a warning for it in ./scripts/checkpatch.pl

============
__weak void rockchip_stimer_init(void)
{
#if defined(CONFIG_ROCKCHIP_STIMER_BASE)

#endif
}
============
There are exceptions like in rockchip_stimer_init where certain defines are missing, so that's still allowed.
In all other settings we use IS_ENABLED and CONFIG_IS_ENABLED.
Hope that explains your questions.


Johan


> 
> Cheers,
> Quentin