Hi Tom, On Wed, 21 Sept 2022 at 15:49, Tom Rini <tr...@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 <ma...@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 <ma...@denx.de> > > > > > > --- > > > > > > Cc: Simon Glass <s...@chromium.org> > > > > > > Cc: Tom Rini <tr...@konsulko.com> > > > > > > --- > > > > > > > > > > Reviewed-by: Simon Glass <s...@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
