On Sat, 15 Dec 2012 17:57:52 +0800
Albert Wang <twan...@marvell.com> wrote:

> From: Libin Yang <lby...@marvell.com>
> 
> 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 <cor...@lwn.net>

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to