[U-Boot] [PATCH 12/35] arm: socfpga: clock: Implant order into bit definitions

[Wolfgang Denk]

Dear Marek Vasut,

In message <1410779188-6880-13-git-send-email-marex at denx.de> you wrote:
> The bit definitions for clock manager are complete chaos. Implement
> some basic logical order into them.
...
> +#define CLKMGR_BYPASS_MAINPLL_SET(x)		(((x) << 0) & 0x00000001)
> +#define CLKMGR_BYPASS_PERPLLSRC_SET(x)	(((x) << 4) & 0x00000010)
> +#define CLKMGR_BYPASS_PERPLL_SET(x)		(((x) << 3) & 0x00000008)
> +#define CLKMGR_BYPASS_SDRPLLSRC_SET(x)	(((x) << 2) & 0x00000004)
> +#define CLKMGR_BYPASS_SDRPLL_SET(x)		(((x) << 1) & 0x00000002)

What is the purpose of all these funny shift operation shere?  Is it
just to obfuscate the meaning of the code, or is the code eventually
wrong?

As is above could be rewritten much simpler without the shifts:

#define CLKMGR_BYPASS_MAINPLL_SET(x)	((x) & 1)
#define CLKMGR_BYPASS_PERPLLSRC_SET(x)	((x) & 1)
#define CLKMGR_BYPASS_PERPLL_SET(x)	((x) & 1)
#define CLKMGR_BYPASS_SDRPLLSRC_SET(x)	((x) & 1)
#define CLKMGR_BYPASS_SDRPLL_SET(x)	((x) & 1)


Note also that the macros are misnamed - "<something>_SET" means an
action, i. e. the macro is supposed to set some bits in the argument,
but here you actually perform a test if something "is set".

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: wd at denx.de
When it is incorrect, it is, at least *authoritatively* incorrect.
                                    - Hitchiker's Guide To The Galaxy