Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver

[Nicolas THERY <nicolas.thery@st.com>]

On 2012-12-16 22:51, Albert Wang wrote:
[...]
>>>
>>> +static void mcam_clk_set(struct mcam_camera *mcam, int on)
>>> +{
>>> +	unsigned int i;
>>> +
>>> +	if (on) {
>>> +		for (i = 0; i < mcam->clk_num; i++) {
>>> +			if (mcam->clk[i])
>>> +				clk_enable(mcam->clk[i]);
>>> +		}
>>> +	} else {
>>> +		for (i = mcam->clk_num; i > 0; i--) {
>>> +			if (mcam->clk[i - 1])
>>> +				clk_disable(mcam->clk[i - 1]);
>>> +		}
>>> +	}
>>> +}
>>
>> A couple of minor comments:
>>
>> - This function is always called with a constant value for "on".  It would
>>   be easier to read (and less prone to unfortunate brace errors) if it
>>   were just two functions: mcam_clk_enable() and mcam_clk_disable().
>>
> [Albert Wang] OK, that's fine to split it to 2 functions. :)
> 
>> - I'd write the second for loop as:
>>
>> 	for (i = mcal->clk_num - 1; i >= 0; i==) {
>>
>>   just to match the values used in the other direction and avoid the
>>   subscript arithmetic.
>>
> [Albert Wang] Yes, we can improve it. :)

Bewar: i is unsigned so testing i >= 0 will loop forever.

[...]