Hi, Jonathan >-----Original Message----- >From: Jonathan Corbet [mailto:corbet@xxxxxxx] >Sent: Monday, 17 December, 2012 00:03 >To: Albert Wang >Cc: 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 Sat, 15 Dec 2012 17:57:52 +0800 >Albert Wang <twang13@xxxxxxxxxxx> wrote: > >> From: Libin Yang <lbyang@xxxxxxxxxxx> >> >> This patch adds the clock tree support for marvell-ccic. >> >> Each board may require different clk enabling sequence. >> Developer need add the clk_name in correct sequence in board driver >> to use this feature. >> >> +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. :) >> +static void mcam_init_clk(struct mcam_camera *mcam, >> + struct mmp_camera_platform_data *pdata, int init) > >So why does an "init" function have an "init" parameter? Again, I think >this would be far better split into two functions. Among other things, >that would help to reduce the deep nesting below. > [Albert Wang] Yes, the parameter name is confused. And we will split this function too. :) >> +{ >> + unsigned int i; >> + >> + if (NR_MCAM_CLK < pdata->clk_num) { >> + dev_err(mcam->dev, "Too many mcam clocks defined\n"); >> + mcam->clk_num = 0; >> + return; >> + } >> + >> + if (init) { >> + for (i = 0; i < pdata->clk_num; i++) { >> + if (pdata->clk_name[i] != NULL) { >> + mcam->clk[i] = devm_clk_get(mcam->dev, >> + pdata->clk_name[i]); >> + if (IS_ERR(mcam->clk[i])) { >> + dev_err(mcam->dev, >> + "Could not get clk: %s\n", >> + pdata->clk_name[i]); >> + mcam->clk_num = 0; >> + return; >> + } >> + } >> + } >> + mcam->clk_num = pdata->clk_num; >> + } else >> + mcam->clk_num = 0; >> +} > >Again, minor comments, but I do think the code would be improved by >splitting those functions. Meanwhile: > >Acked-by: Jonathan Corbet <corbet@xxxxxxx> > >jon 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
