lkml.org 
[lkml]   [2010]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] OMAP: HWMOD: Handle opt clocks using clk_add_alias
Hi Partha,

On 8/23/2010 5:44 PM, Basak, Partha wrote:
> From: Basak, Partha<p-basak2@ti.com>
>
> For every optional clock present per hwmod per omap-device, this function
> adds an entry in the clocks list of the form<dev-id=dev_name, con-id=role>,
> if an entry is already present in the list of the form<dev-id=NULL, con-id=role>.
>
> The function is called from within the framework inside omap_device_build_ss(),
> after omap_device_register.
>
> This allows drivers to get a pointer to its optional clocks based on its role
> by calling clk_get(<dev*>,<role>).
>
> Link to discussions related to this patch:
> http://www.spinics.net/lists/linux-omap/msg34809.html
>
> Signed-off-by: Charulatha V<charu@ti.com>
> Signed-off-by: Basak, Partha<p-basak2@ti.com>
> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>
> Cc: Paul Walmsley<paul@pwsan.com>
> Cc: Kevin Hilman<khilman@deeprootsystems.com>
> ---
> arch/arm/plat-omap/omap_device.c | 31 ++++++++++++++++++++++++++++++-
> 1 files changed, 30 insertions(+), 1 deletions(-)
>
> Index: linux-omap-pm/arch/arm/plat-omap/omap_device.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/plat-omap/omap_device.c 2010-08-18 02:48:30.789079550 +0530
> +++ linux-omap-pm/arch/arm/plat-omap/omap_device.c 2010-08-24 07:19:43.637080138 +0530
> @@ -82,6 +82,7 @@
> #include<linux/slab.h>
> #include<linux/err.h>
> #include<linux/io.h>
> +#include<linux/clk.h>
>
> #include<plat/omap_device.h>
> #include<plat/omap_hwmod.h>
> @@ -243,7 +244,6 @@ static inline struct omap_device *_find_
> return container_of(pdev, struct omap_device, pdev);
> }
>
> -

That fix is not related to the patch subject.

> /* Public functions for use by core code */
>
> /**
> @@ -271,6 +271,47 @@ int omap_device_count_resources(struct o
> }

This is not a public function, so you should move it before the /*
Public functions for use by core code */

> /**
> + * omap_device_add_opt_clk_alias - Add alias for optional clocks in the
> + * clocks list.
> + *
> + * @od: struct omap_device *od
> + *
> + * For every optional clock present per hwmod per omap-device, this function
> + * adds an entry in the clocks list of the form<dev-id=dev_name, con-id=role>
> + * if an entry is already present in it with the form<dev-id=NULL,con-id=role>
> + *
> + * The function is called from inside omap_device_build_ss(),
> + * after omap_device_register.
> + *
> + * This allows drivers to get a pointer to its optional clocks based on its role
> + * by calling clk_get(<dev*>,<role>).
> + */
> +
> +static void omap_device_add_opt_clk_alias(struct omap_device *od)
> +{
> + int i, j;
> + struct omap_hwmod_opt_clk *oc;
> + struct omap_hwmod *oh;
> +
> + for (i = 0, oh = *od->hwmods; i< od->hwmods_cnt; i++, oh++)

You can avoid that iteration and the extra level of indentation by
re-using the iteration done before the call to that function.

> + /* Add Clock alias for all optional clocks*/
> + for (j = oh->opt_clks_cnt, oc = oh->opt_clks;
> + j> 0; j--, oc++) {
> + if ((oc->_clk)&&
> + (IS_ERR(clk_get(&od->pdev.dev, oc->role)))) {
> + int ret;

You can avoid the indentation by returning in case of error.
You can avoid as well 2 pairs of parens.

> +
> + ret = clk_add_alias(oc->role,
> + dev_name(&od->pdev.dev),
> + (char *)oc->clk, NULL);

The indentation is not align with the inner parameters... which is not
easy in your case due to the too many indentation level you have in this
function.

> + if (ret)
> + dev_err(&od->pdev.dev, "omap_device:\

This is not correct way of splitting long string, use "omap_device:"
instead.

Even if dev_err is usable because you have the dev pointer, it is in
fact an error from the omap_device code code, so using pr_err is
probably more appropriate (TBC).

> + clk_add_alias for %s failed\n",
> + oc->role);
> + }
> + }
> +}
> +/**
> * omap_device_fill_resources - fill in array of struct resource
> * @od: struct omap_device *
> * @res: pointer to an array of struct resource to be filled in
> @@ -424,6 +465,12 @@ struct omap_device *omap_device_build_ss
> for (i = 0; i< oh_cnt; i++)
> hwmods[i]->od = od;

You can call a per hwmod API here in order to avoid the iteration inside
the function.

>
> + /*
> + * Add Clock alias for all optional
> + * clocks present in the hwmod
> + */

That comment is not bringing any new information, the function is
already documented in the kernel doc comment.

> + omap_device_add_opt_clk_alias(od);
> +
> if (ret)
> goto odbs_exit4;
>

Regards,
Benoit


\
 
 \ /
  Last update: 2010-08-23 22:17    [W:0.055 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site