Hi, Nicolas Thank you for your reminder. :) Happy New Year! >-----Original Message----- >From: Nicolas THERY [mailto:nicolas.thery@xxxxxx] >Sent: Friday, 04 January, 2013 01:38 >To: Albert Wang >Cc: Jonathan Corbet; g.liakhovetski@xxxxxx; linux-media@xxxxxxxxxxxxxxx; Libin Yang >Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell- >ccic driver > > > >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. > [Albert Wang] Yes, it looks my original code can work. :) >[...] Thanks Albert Wang 86-21-61092656 -- 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
