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

[Minkyu Kang]

On 16/01/14 01:09, Gerhard Sittig wrote:
> 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.

Indeed.. there are many magic numbers in this file.
At past, we  were agreed to use "magic numbers with comment" instead of defines, because of much changes.
For now, of course it can be modified to using defines.
But I think it is not a scope of this patch. :)

> 
> 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.)

Thanks,
Minkyu Kang.