Hi Tom,
On Wed, 21 Sept 2022 at 15:49, Tom Rini trini@konsulko.com wrote:
On Wed, Sep 21, 2022 at 02:56:29AM +0200, Marek Vasut wrote:
On 9/20/22 21:04, Tom Rini wrote:
On Tue, Sep 20, 2022 at 05:56:50PM +0200, Marek Vasut wrote:
On 9/20/22 17:43, Simon Glass wrote:
On Mon, 19 Sept 2022 at 21:52, Marek Vasut marex@denx.de wrote:
Make spl_board_init() a weak symbol and get rid of Kconfig symbols and ifdeffery guarding this function. Since the spl_board_init() is now a weak symbol, boards can either use the default implementation which is empty and gets inlined with zero text increase, or override the implementation with their own as needed.
Signed-off-by: Marek Vasut marex@denx.de
Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
Reviewed-by: Simon Glass sjg@chromium.org
but please add a nice comment to spl_board_init() indicating what it is for and when it is called.
Actually, I wonder, should we start turning all the other symbols which are now protected by Kconfig symbol (the #ifdef CONFIG_FOO) into weak symbols without any need for Kconfig symbol guard instead as well?
I'm not sure this is a good idea. An #ifdef in and of itself is not a bad thing, and now we add a dummy function on another set of platforms.
The empty dummy function is inlined, so that itself does not increase text size.
No, it's 4/8 bytes, weak functions don't get inlined. It's not a big deal nor a deal breaker, but it's not free.
What is true is that a lot of SPL_foo symbols should be select'd rather than asked because it's not an option. You enable it, it works, you disable it, your platform doesn't work. If it's just the #if, we could rewrite the line as if (CONFIG_IS_ENABLED(BOARD_INIT)) spl_board_init();
as the prototype shouldn't be guarded anyhow.
The third option is what is implemented in this patch -- the board port developer implements the weak function override and it always works, because it is no longer selectable.
It's similar, yes. I think the biggest hang-up for me is that while the Kconfig help text isn't the best documentation for what is needed when adding SPL to a board, it's better than code-only comments. I know Simon asked for a comment on the weak function, but how about starting doc/develop/spl.rst and make at least this new content kernel-doc format so it can be included there?
Yes, well you know my feelings on weak functions and how hard they make it to figure out what code is actually running. I actually like you if(...) idea along with better docs, since the Kconfig option is at least a positive signal that the function is used.
Regards, Simon