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.
[...]--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
