On Tuesday 14 February 2023 05:47:08 Christophe Leroy wrote:

Le 13/02/2023 à 21:11, Pali Rohár a écrit :

quoted

On Monday 13 February 2023 19:58:15 Christophe Leroy wrote:

quoted

Le 09/02/2023 à 01:15, Pali Rohár a écrit :

quoted

quoted

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.

Ok, fair enough, but then please explain in the commit message that you 
copy the functions and then they will be re-written in following 
patches. That way we know exactly what we are reviewing.

But it is already explained in the commit message. Is not it enough? Or
should I rephrase some parts of the commit message?

quoted

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.

No, if the function are different it's better each platform has its own. 
My comment was for functions that are exactly the same.

quoted

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