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?