Dear Marek, In message <201409152348.15411.ma...@denx.de> you wrote: > > > cm_write_bypass( > > CLKMGR_BYPASS_PERPLLSRC_SET( > > CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1) | > > CLKMGR_BYPASS_SDRPLLSRC_SET( > > CLKMGR_BYPASS_SDRPLLSRC_SELECT_EOSC1) | > > CLKMGR_BYPASS_PERPLL_SET(CLKMGR_BYPASS_ENABLE) > > > > CLKMGR_BYPASS_SDRPLL_SET(CLKMGR_BYPASS_ENABLE) | > > CLKMGR_BYPASS_MAINPLL_SET(CLKMGR_BYPASS_ENABLE)); ... > > But yes, there are more cleanups that could be done there. > > You surely wanted to say "must be done where" ;-) I've yet to decide if I'll > do > it before this patchset or after ; I am more inclined doing it as a > subsequent > patch to allow Altera guys review the changes and match them with their > existing > code. And only then do the cleanup , probably automatically using some script. > > What do you think, Wolfgang, Pavel, Dinh, others ... ?
Well, frankly speaking, the code above (and many similar use cases) are an unreadable and unmaintainable mess. I understand that parts of this are computer-generated definitions, but in my opinion there are several serious disadvantages: - The macro names are so long that the code becomes unreadable; the CodingStyle recommens in "Chapter 4: Naming": C is a Spartan language, and so should your naming be. Unlike Modula-2 and Pascal programmers, C programmers do not use cute names like ThisVariableIsATemporaryCounter. A C programmer would call that variable "tmp", which is much easier to write, and not the least more difficult to understand. ... LOCAL variable names should be short, and to the point. - There is virtually no visual difference between macro names that take arguments (like CLKMGR_BYPASS_PERPLLSRC_SET()) and macro names that are used as arguments (like CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1) which makes parsing the code even more difficult. We should especially keep in mind that things like CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1 are deined in a single C source file only ("arch/arm/cpu/armv7/socfpga/clock_manager.c"), i. e. they have strictly local scope, so the long names make even less sense. - The code not only difficult to read, but even more difficult to debug. Assume you are single stepping through this code, and you reach this statement: > > cm_write_bypass( > > CLKMGR_BYPASS_PERPLLSRC_SET( > > CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1) | > > CLKMGR_BYPASS_SDRPLLSRC_SET( > > CLKMGR_BYPASS_SDRPLLSRC_SELECT_EOSC1) | > > CLKMGR_BYPASS_PERPLL_SET(CLKMGR_BYPASS_ENABLE) > > > > CLKMGR_BYPASS_SDRPLL_SET(CLKMGR_BYPASS_ENABLE) | > > CLKMGR_BYPASS_MAINPLL_SET(CLKMGR_BYPASS_ENABLE)); In the debugger, you see cm_write_bypass(0x0000000D); How long does it take you to figure out if this is what we expect or not? The macros are probably intended to offer unlimited flexibility, but they come at a cost. Do we really need this flexibility? How many of the macros are actually used und how many different ways. Under normal conditions I would simple NAK such a patch and demand that the code is cleaned up before it gets accepted for mainline. Unfortunately, we already have this mess in mainline - somehow "arch/arm/cpu/armv7/socfpga/clock_manager.c" slipped through the reviews without anybody noticing this stuff. So accepting this patch now does not make things dramatically worse, but I understand it would help to prepare the ground for other, urgently needed cleanup. So as far as I am concerned I do not intend to block this with a plain NAK, but I would be really happy if this whole clock manager code would be put on the list of things that need rework, and we don't forget this about more recent tasks coming. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "Everybody is talking about the weather but nobody does anything about it." - Mark Twain _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot