by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2 09/10] powerpc: Use ppc_md_progress()

On Sat, 2023-02-18 at 10:15 +0100, Christophe Leroy wrote:
> Many places have:
>
> if (ppc.md_progress)
> ppc.md_progress();
>
> Use ppc_md_progress() instead.
>
> Note that checkpatch complains about using function names,
> but this is not a function taking format strings, so we
> leave the function names for now.

If you are changing almost all of these uses, why not
drop the unused 2nd argument 'hex' at the same time?

void ppc_printk_progress(char *s, unsigned short hex)
{
pr_info("%s\n", s);
}

> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
[]
> @@ -127,8 +127,7 @@ __setup("l3cr=", ppc_setup_l3cr);
> static int __init ppc_init(void)
> {
> /* clear the progress line */
> - if (ppc_md.progress)
> - ppc_md.progress(" ", 0xffff);
> + ppc_md_progress(" ", 0xffff);

ppc_md_progress(" ");

Although this example seems especially useless.

> diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
[]
> @@ -347,7 +347,7 @@ void __init MMU_init_hw(void)
> if (!mmu_has_feature(MMU_FTR_HPTE_TABLE))
> return;
>
> - if ( ppc_md.progress ) ppc_md.progress("hash:enter", 0x105);
> + ppc_md_progress("hash:enter", 0x105);

ppc_md_progress("hash:enter");

[]

> diff --git a/arch/powerpc/platforms/52xx/efika.c b/arch/powerpc/platforms/52xx/efika.c
[]
> @@ -189,8 +189,7 @@ static void __init efika_setup_arch(void)
> mpc52xx_pm_init();
> #endif
>
> - if (ppc_md.progress)
> - ppc_md.progress("Linux/PPC " UTS_RELEASE " running on Efika ;-)\n", 0x0);
> + ppc_md_progress("Linux/PPC " UTS_RELEASE " running on Efika ;-)\n", 0x0);

And perhaps remove the extra newlines here and other places as
it doesn't seem useful to have unnecessary blank lines in the log.

2023-02-20 00:42:48

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH v2 09/10] powerpc: Use ppc_md_progress()

Joe Perches <[email protected]> writes:
> On Sat, 2023-02-18 at 10:15 +0100, Christophe Leroy wrote:
>> Many places have:
>>
>> if (ppc.md_progress)
>> ppc.md_progress();
>>
>> Use ppc_md_progress() instead.
>>
>> Note that checkpatch complains about using function names,
>> but this is not a function taking format strings, so we
>> leave the function names for now.
>
> If you are changing almost all of these uses, why not
> drop the unused 2nd argument 'hex' at the same time?

The hex argument is printed by some "progress" implementations.

rtas_progress() will print it to the machine's 7-segment display on some
very old machines. Grepping around I think it might be only old CHRP
machines that use the hex code.

And we only have a handful of sites that actually pass a non-zero hex
code anyway.

So I think we could drop the hex argument.

But the vast bulk of the uses are just printing "foo_setup_arch()" which
is of dubious utility and could probably just be dropped.

If the machine has a working udbg backend then before we get to setup
arch we will have initialised the udbg console, and so all printk
messages should be going to udbg anyway.

cheers