On Tuesday 14 February 2023 05:47:08 Christophe Leroy wrote: > Le 13/02/2023 à 21:11, Pali Rohár a écrit : > > 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. > > 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? > > > > 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. > > > > > So I wrote *_pic_init() function which is p2020 specific, like already > > existing ds and rdb specific functions in their own source files.