From: Basak, Partha <[email protected]>
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 <[email protected]>
Signed-off-by: Basak, Partha <[email protected]>
Signed-off-by: Benoit Cousson <[email protected]>
Signed-off-by: Rajendra Nayak <[email protected]>
Cc: Paul Walmsley <[email protected]>
Cc: Kevin Hilman <[email protected]>
---
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);
}
-
/* Public functions for use by core code */
/**
@@ -271,6 +271,47 @@ int omap_device_count_resources(struct o
}
/**
+ * 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++)
+ /* 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;
+
+ ret = clk_add_alias(oc->role,
+ dev_name(&od->pdev.dev),
+ (char *)oc->clk, NULL);
+ if (ret)
+ dev_err(&od->pdev.dev, "omap_device:\
+ 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;
+ /*
+ * Add Clock alias for all optional
+ * clocks present in the hwmod
+ */
+ omap_device_add_opt_clk_alias(od);
+
if (ret)
goto odbs_exit4;
Hi Partha,
On 8/23/2010 5:44 PM, Basak, Partha wrote:
> From: Basak, Partha<[email protected]>
>
> 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<[email protected]>
> Signed-off-by: Basak, Partha<[email protected]>
> Signed-off-by: Benoit Cousson<[email protected]>
> Signed-off-by: Rajendra Nayak<[email protected]>
>
> Cc: Paul Walmsley<[email protected]>
> Cc: Kevin Hilman<[email protected]>
> ---
> 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
> -----Original Message-----
> From: Cousson, Benoit
> Sent: Tuesday, August 24, 2010 1:45 AM
> To: Basak, Partha
> Cc: [email protected]; Varadarajan, Charulatha; Nayak,
> Rajendra; Paul Walmsley; Kevin Hilman
> Subject: Re: [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<[email protected]>
> >
> > 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<[email protected]>
> > Signed-off-by: Basak, Partha<[email protected]>
> > Signed-off-by: Benoit Cousson<[email protected]>
> > Signed-off-by: Rajendra Nayak<[email protected]>
> >
> > Cc: Paul Walmsley<[email protected]>
> > Cc: Kevin Hilman<[email protected]>
> > ---
> > 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 */
>
OK
> > /**
> > + * 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;
>
Not clear on your comment, wanted to take care of the scenario where a device is having multiple hwmods
> 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.
>
Agreed
> 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.
I see your point, but most of the functions in this layer are having omap_device * as the parameter, so I maintained consistency
>
> >
> > + /*
> > + * 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.
>
Agreed, will remove
> > + omap_device_add_opt_clk_alias(od);
> > +
> > if (ret)
> > goto odbs_exit4;
> >
>
> Regards,
> Benoit
On 8/25/2010 12:45 PM, Basak, Partha wrote:
>
>> From: Cousson, Benoit
>> Sent: Tuesday, August 24, 2010 1:45 AM
>>
>> Hi Partha,
>>
>> On 8/23/2010 5:44 PM, Basak, Partha wrote:
>>> From: Basak, Partha<[email protected]>
>>>
>>> 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<[email protected]>
>>> Signed-off-by: Basak, Partha<[email protected]>
>>> Signed-off-by: Benoit Cousson<[email protected]>
>>> Signed-off-by: Rajendra Nayak<[email protected]>
>>>
>>> Cc: Paul Walmsley<[email protected]>
>>> Cc: Kevin Hilman<[email protected]>
>>> ---
>>> 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 */
>>
> OK
>>> /**
>>> + * 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;
>>
> Not clear on your comment, wanted to take care of the scenario where a device is having multiple hwmods
Which one? the one below or before? These comments are just about the
readability of this patch.
>> 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.
>>
> Agreed
>
>> 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.
>
> I see your point, but most of the functions in this layer are having omap_device * as the parameter, so I maintained consistency
Since your function is anyway a private function, I don't see any reason
to do that.
FYI, I joined an updated version.
Regards,
Benoit
---
From d8a374c679b7f26229cf054520deb0ac416b7f4c Mon Sep 17 00:00:00 2001
From: Basak, Partha <[email protected]>
Date: Mon, 23 Aug 2010 21:14:29 +0530
Subject: [PATCH] OMAP: hwmod: handle opt clocks node using clk_add_alias
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 <[email protected]>
Signed-off-by: Basak, Partha <[email protected]>
Signed-off-by: Benoit Cousson <[email protected]>
Signed-off-by: Rajendra Nayak <[email protected]>
Cc: Paul Walmsley <[email protected]>
Cc: Kevin Hilman <[email protected]>
---
arch/arm/plat-omap/omap_device.c | 39
+++++++++++++++++++++++++++++++++++++-
1 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/arch/arm/plat-omap/omap_device.c
b/arch/arm/plat-omap/omap_device.c
index d2b1609..d876cec 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -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,6 +244,40 @@ static inline struct omap_device
*_find_by_pdev(struct platform_device *pdev)
return container_of(pdev, struct omap_device, pdev);
}
+/**
+ * _add_optional_clock_alias - Add clock alias for hwmod optional clocks
+ * @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 _add_optional_clock_alias(struct omap_device *od,
+ struct omap_hwmod *oh)
+{
+ int i;
+ struct omap_hwmod_opt_clk *oc;
+
+ for (i = oh->opt_clks_cnt, oc = oh->opt_clks; i > 0; i--, oc++) {
+ int ret;
+
+ if (!oc->_clk || !IS_ERR(clk_get(&od->pdev.dev, oc->role)))
+ return;
+
+ ret = clk_add_alias(oc->role, dev_name(&od->pdev.dev),
+ (char *)oc->clk, NULL);
+ if (ret)
+ pr_err("omap_device: clk_add_alias for %s failed\n",
+ oc->role);
+ }
+}
+
/* Public functions for use by core code */
@@ -421,8 +456,10 @@ struct omap_device *omap_device_build_ss(const char
*pdev_name, int pdev_id,
else
ret = omap_device_register(od);
- for (i = 0; i < oh_cnt; i++)
+ for (i = 0; i < oh_cnt; i++) {
hwmods[i]->od = od;
+ _add_optional_clock_alias(od, hwmods[i]);
+ }
if (ret)
goto odbs_exit4;
--
1.6.0.4