Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description

["Pali Rohár" <pali@kernel.org>]

On Monday 13 February 2023 19:58:15 Christophe Leroy wrote:
> Le 09/02/2023 à 01:15, Pali Rohár a écrit :
> >>
> >> This patch moves all p2020 boards from mpc85xx_rdb.c and mpc85xx_ds.c
> >> files into new p2020.c file, and plus it copies all helper functions
> >> which p2020 boards requires. This patch does not introduce any new code
> >> or functional change. It should be really plain copy/move.
> 
> Yes after looking into it in more details, it is exactly that. You 
> copied all helper functions but this is not said in the commit message.
> I think it should be said, and more important it should be explained why.
> Because this is exactly what I was not understanding, why I couldn't see 
> all moved functions: just because many of them were not moved but copied.
> 
> In the two first pages you made some function static, and then you 
> duplicated it. Why ? Why not keep it global and just use it from one 
> place to the other ?
> 
> Because after patch 3 we have:
> 
> arch/powerpc/platforms/85xx/mpc85xx_rdb.c:static void __init 
> mpc85xx_rdb_pic_init(void)
> arch/powerpc/platforms/85xx/p2020.c:static void __init 
> mpc85xx_rdb_pic_init(void)
> 
> arch/powerpc/platforms/85xx/mpc85xx_ds.c:static void __init 
> mpc85xx_ds_pic_init(void)
> arch/powerpc/platforms/85xx/p2020.c:static void __init 
> mpc85xx_ds_pic_init(void)
> 
> Why not just drop patches 1 and 2 and keep those two functions and all 
> the other common functions like mpc85xx_8259_cascade() 
> mpc85xx_ds_uli_init() and a lot more  in a separate common file ?
> 
> Christophe

After applying all patches there is no mpc85xx_rdb_pic_init() /
mpc85xx_ds_pic_init() function in p2020.c file. There is
p2020_pic_init() in p2020.c but it is slightly different than previous
two functions.

Maybe it could be possible to create one function mpc85xx_pic_init() as
unification of previous 3 functions, but such change would be needed to
test on lot of mpc85xx boards, which would be affected by such change.
And I do not have them for testing. I have only P2020.

So I wrote *_pic_init() function which is p2020 specific, like already
existing ds and rdb specific functions in their own source files.