[PATCH v2] rockchip: px30-board-tpl: Use CONFIG_TPL_BANNER_PRINT flag

[Łukasz Czechowski]

Hi Quentin,

Thanks for your response.
As for the CONFIG_TPL_SERIAL flag, I agree that it should use 'imply'
to be allowed to disable it, to silence the serial in TPL. However,
currently, disabling this flag makes the code unbuildable (which could
be the reason 'select' is used). I'd keep this change out of scope for
this commit.
I'll prepare v3 then, which will contain only updated Cc and commit title.

Best regards,
Łukasz

śr., 17 kwi 2024 o 11:55 Quentin Schulz
<quentin.schulz at theobroma-systems.com> napisał(a):
>
> Hi Lukasz,
>
> I would have renamed the commit title to be something like
>
> rockchip: px30-board-tpl: sync ifdef guards with full TPL
>
> because CONFIG_TPL_BANNER_PRINT isn't really a flag but a Kconfig symbol
> and the effect of this patch is to not print the banner when the symbol
> is not selected.
>
> Another option could have been (only for the v1, since you do more now):
>
> rockchip: px30-board-tpl: print banner only if CONFIG_TPL_BANNER_PRINT
>
> On 4/17/24 11:35, lukasz.czechowski at thaumatec.com wrote:
> > From: Lukasz Czechowski <lukasz.czechowski at thaumatec.com>
> >
> > Display TPL init information message only when TPL_BANNER_PRINT
> > configuration entry is set. This allows to disable information
> > message in case logs on UART are unwanted.
> > Update parent ifdef condition to check also CONFIG_TPL_SERIAL
> > to match logic of the non-PX30 TPL implementation.
> >
> > Signed-off-by: Lukasz Czechowski <lukasz.czechowski at thaumatec.com>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Philipp Tomsich <philipp.tomsich at vrull.eu>
> > Cc: Kever Yang <kever.yang at rock-chips.com>
> >
>
> Small nitpick, the Cc would make it to the git history, which isn't
> really necessary. I believe you could have those below the --- and
> git-send-email still find them.
>
> Additionally, git send-email allows you to provide --cc and --to manually.
>
> > ---
> > Changes for v2:
> > - Updated parent ifdef condition
> > ---
> >   arch/arm/mach-rockchip/px30-board-tpl.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-rockchip/px30-board-tpl.c b/arch/arm/mach-rockchip/px30-board-tpl.c
> > index 637a5e1b18..db368a7b8c 100644
> > --- a/arch/arm/mach-rockchip/px30-board-tpl.c
> > +++ b/arch/arm/mach-rockchip/px30-board-tpl.c
> > @@ -36,7 +36,7 @@ void board_init_f(ulong dummy)
> >   {
> >       int ret;
> >
> > -#ifdef CONFIG_DEBUG_UART
> > +#if defined(CONFIG_DEBUG_UART) && defined(CONFIG_TPL_SERIAL)
>
> Quite interestingly, CONFIG_TPL_SERIAL cannot be disabled on PX30, maybe
> we should change arch/arm/mach-rockchip/Kconfig to use an `imply`
> instead of `select`.
>
> Anyway, no change required on my side. Only, if there's a v3 (for the Cc
> and commit title, the Kconfig change would be a separate patch anyway).
>
> Reviewed-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>
> Thanks,
> Quentin