[U-Boot] [PATCH] arm: exynos: change to use clrbits macro instead of readl/writel function

[Gerhard Sittig]

On Tue, Jan 14, 2014 at 13:01 +0900, Inha Song wrote:
> 
> Use setbits/clrbits macro instead of readl/writel function
> 
> Signed-off-by: Inha Song <ideal.song at samsung.com>

You can probably trim down the subject line's length by omitting
the "change to" words.  It's obvious that a patch changes
something.


It's true that your replacing open-coded read-modify-write
sequences with bit manipulation macros much better reflects what
actually is happening from the application's POV.  But your patch
does more than that (supported by the "now works for me" reply),
and I feel that your commit message should reflect this so others
can be aware (both of the fix, and of the former bug).

"In bypassing" you fix a few bugs (I noticed those around lines
1114 and 1176, and only because I remembered your other
submission which you don't reference) where random data was
written to clock control registers (either uninitialized data, or
potentially stale data that was set from unrelated sources).

So you may want to followup to your former submission, stating
that it has become obsolete or superseeded.  Or send out a v2
series (or any other appropriate version number) with a reference
to the former patch, when you consider this new patch the
improvement to your earlier submission.  This helps reviewers and
maintainers.


> @@ -1114,16 +1099,13 @@ void exynos4_set_lcd_clk(void)
>  	 * MIPI0_PRE_RATIO	[23:20]
>  	 * set fimd ratio
>  	 */
> -	cfg &= ~(0xf);
> -	cfg |= 0x1;
> -	writel(cfg, &clk->div_lcd0);
> +	clrsetbits_le32(&clk->div_lcd0, 0xe, 0x1);
>  }
>  

There still is the question whether you happen to set individual
bits (and where clearing and setting the very same bit is
unexpected), or whether you are writing an integer value into a
bitfield that only spans part of a register.  And whether using
symbolic identifiers helps readers and fellow developers since
magic numbers obfuscate the motivation.

Please check for those style issues.  I've seen e.g. both "and
0xf, or 0x1" as well as "and 0x9, or 0x6" which is inconsistent.
Unless this is the "bits vs numbers" thing, where comments may
help.  (Ignore this latter concern if there already are those
comments, which just are not in the patch's context.  I haven't
checked the source but only looked at your patch.)


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de