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
