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().
- 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.
> +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.
> +{
> + 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
--
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
