Dear Christophe Leroy,
In message 8947f895b86efe0396b1825998a64a3340fff597.1498751837.git.christophe.leroy@c-s.fr you wrote:
Signed-off-by: Christophe Leroy christophe.leroy@c-s.fr
arch/powerpc/cpu/mpc8xx/cpu_init.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c index 0f935aff9e..e8045cc5c6 100644 --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c @@ -55,6 +55,16 @@ void cpu_init_f (volatile immap_t * immr) reg |= CONFIG_SYS_SCCR; immr->im_clkrst.car_sccr = reg;
- /* BUG MPC866 GLL2 consideration */
- reg = immr->im_clkrst.car_sccr;
- /* probably we use the mode 1:2:1 */
- if ((reg & 0x00060000) == 0x00020000) {
reg &= ~0x00060000;immr->im_clkrst.car_sccr = reg;reg |= 0x00020000;immr->im_clkrst.car_sccr = reg;- }
This is an excellent example for future situations, so thanks a lot for bringing it up early.
The code you are adding here violates basic (modern) programming standards. To access device registers, proper I/O accessors must be used. In this case the code should use the clrsetbits_be32() macro.
Strictly speaking, you should receive a full NAK on this code.
You may now argument that the surrounding code is full of similar examples of obsolete, broken code, and you are just following this (bad) example.
The standard reply to this would be: well, while you are modifying this file, please clean up the other ocurrences of such bad code as well.
Are you aware of this process? How do you think to handle such situations in the future?
What I really, really am scared of is that you might follow your employer's requirements ("new version only introduces a reduced amount of modifications in source code") instead of following your duties as a custodian (cleaning up the code to meet current programming standards).
Can you please make an explixit statement here how you are going to handle this in the future? [And again, schedule is important, too.]
Best regards,
Wolfgang Denk