Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
flat view
From: Pali Rohár <pali@kernel.org>
Date: 2023-02-13 20:11:55
Also in:
lkml
On Monday 13 February 2023 19:58:15 Christophe Leroy wrote:
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. 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.