[RFC/PATCH,2/7] OMAP3: beagle: don't touch omap_device internals
diff mbox

Message ID 20110728055346.GA11921@foobar
State New, archived
Headers show

Commit Message

Nishanth Menon July 28, 2011, 5:53 a.m. UTC
On 11:57-20110722, Felipe Balbi wrote:
[...]
> >  	/* Custom OPP enabled for all xM versions */
> >  	if (cpu_is_omap3630()) {
> > -		struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
> > -		struct omap_hwmod *dh = omap_hwmod_lookup("iva");
> > -		struct device *dev;
> > +		struct device *mpu_dev, *iva_dev;
> >  
> > -		if (!mh || !dh) {
> > +		mpu_dev = omap2_get_mpuss_device();
> > +		iva_dev = omap2_get_iva_device();
> 
> out of curiosity again, nothing to do with this patch.
> 
> Maybe it would be nicer to have an api such as:
> 
> omap2_get_device(name);
> 
> there are already four devices to be gotten, if that number grows any
> bigger, so will the number of helper functions.
I agree, in fact, on a different topic, I hit the same requirement
here is the patch I had done:
From 9f226def811bd50e4bac02f427604034cef77706 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <nm@ti.com>
Date: Wed, 27 Jul 2011 15:02:32 -0500
Subject: [PATCH] OMAP: hwmod: add omap_hwmod_to_device

omap_hwmod_to_device is useful for drivers when they need to
look up the device associated with a hwmod name to map back
into the device structure pointers. These ideally should
be used by drivers in mach directory. This could in effect
replace apis such as omap2_get_mpuss_device,
omap2_get_iva_device, omap2_get_l3_device, omap4_get_dsp_device
etc..

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c             |   33 ++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    2 +
 2 files changed, 35 insertions(+), 0 deletions(-)

Comments

Russell King - ARM Linux July 28, 2011, 10:10 a.m. UTC | #1
On Thu, Jul 28, 2011 at 12:53:47AM -0500, Nishanth Menon wrote:
> +struct device *omap_hwmod_to_device(const char *oh_name)
> +{
> +	struct omap_hwmod *oh;
> +
> +	if (!oh_name) {
> +		WARN(1, "%s: no hwmod name!\n", __func__);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	oh = _lookup(oh_name);
> +	if (IS_ERR_OR_NULL(oh)) {
> +		WARN(1, "%s: no hwmod for %s\n", __func__,
> +			oh_name);
> +		return ERR_PTR(-ENODEV);

It is good practice to always propagate back the error codes from functions
which failed.  So you may need something like:

		return ERR_PTR(oh ? PTR_ERR(oh) : -ENODEV);

here.

> +	}
> +	if (IS_ERR_OR_NULL(oh->od)) {
> +		WARN(1, "%s: no omap_device for %s\n", __func__,
> +			oh_name);
> +		return ERR_PTR(-ENODEV);

Same here.
Benoit Cousson July 28, 2011, 12:57 p.m. UTC | #2
Hi Nishanth,

On 7/28/2011 7:53 AM, Menon, Nishanth wrote:
> On 11:57-20110722, Felipe Balbi wrote:
> [...]
>>>   	/* Custom OPP enabled for all xM versions */
>>>   	if (cpu_is_omap3630()) {
>>> -		struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
>>> -		struct omap_hwmod *dh = omap_hwmod_lookup("iva");
>>> -		struct device *dev;
>>> +		struct device *mpu_dev, *iva_dev;
>>>
>>> -		if (!mh || !dh) {
>>> +		mpu_dev = omap2_get_mpuss_device();
>>> +		iva_dev = omap2_get_iva_device();
>>
>> out of curiosity again, nothing to do with this patch.
>>
>> Maybe it would be nicer to have an api such as:
>>
>> omap2_get_device(name);
>>
>> there are already four devices to be gotten, if that number grows any
>> bigger, so will the number of helper functions.
> I agree, in fact, on a different topic, I hit the same requirement
> here is the patch I had done:
>  From 9f226def811bd50e4bac02f427604034cef77706 Mon Sep 17 00:00:00 2001
> From: Nishanth Menon<nm@ti.com>
> Date: Wed, 27 Jul 2011 15:02:32 -0500
> Subject: [PATCH] OMAP: hwmod: add omap_hwmod_to_device
>
> omap_hwmod_to_device is useful for drivers when they need to
> look up the device associated with a hwmod name to map back
> into the device structure pointers. These ideally should
> be used by drivers in mach directory. This could in effect
> replace apis such as omap2_get_mpuss_device,
> omap2_get_iva_device, omap2_get_l3_device, omap4_get_dsp_device
> etc..
>
> Signed-off-by: Nishanth Menon<nm@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c             |   33 ++++++++++++++++++++++++++
>   arch/arm/plat-omap/include/plat/omap_hwmod.h |    2 +
>   2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 293fa6c..77d01a2 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -142,6 +142,7 @@
>   #include "powerdomain.h"
>   #include<plat/clock.h>
>   #include<plat/omap_hwmod.h>
> +#include<plat/omap_device.h>

I'd rather put that code inside the omap_device.c instead of here.
The omap_device layer is on top of the omap_hwmod.
In order to minimize the dependencies from the low HW description layer 
to the omap_device layer, you should maybe define a 
omap_device_from_hwmod() function or something similar.

That being said, do we really need to get the device from the hwmod 
name? Cannot we use the device name instead?
I do not know all the usecases, that why I'm asking.

Regards,
Benoit
Felipe Balbi July 28, 2011, 12:59 p.m. UTC | #3
Hi,

On Thu, Jul 28, 2011 at 02:57:03PM +0200, Cousson, Benoit wrote:
> Hi Nishanth,
> 
> On 7/28/2011 7:53 AM, Menon, Nishanth wrote:
> >On 11:57-20110722, Felipe Balbi wrote:
> >[...]
> >>>  	/* Custom OPP enabled for all xM versions */
> >>>  	if (cpu_is_omap3630()) {
> >>>-		struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
> >>>-		struct omap_hwmod *dh = omap_hwmod_lookup("iva");
> >>>-		struct device *dev;
> >>>+		struct device *mpu_dev, *iva_dev;
> >>>
> >>>-		if (!mh || !dh) {
> >>>+		mpu_dev = omap2_get_mpuss_device();
> >>>+		iva_dev = omap2_get_iva_device();
> >>
> >>out of curiosity again, nothing to do with this patch.
> >>
> >>Maybe it would be nicer to have an api such as:
> >>
> >>omap2_get_device(name);
> >>
> >>there are already four devices to be gotten, if that number grows any
> >>bigger, so will the number of helper functions.
> >I agree, in fact, on a different topic, I hit the same requirement
> >here is the patch I had done:
> > From 9f226def811bd50e4bac02f427604034cef77706 Mon Sep 17 00:00:00 2001
> >From: Nishanth Menon<nm@ti.com>
> >Date: Wed, 27 Jul 2011 15:02:32 -0500
> >Subject: [PATCH] OMAP: hwmod: add omap_hwmod_to_device
> >
> >omap_hwmod_to_device is useful for drivers when they need to
> >look up the device associated with a hwmod name to map back
> >into the device structure pointers. These ideally should
> >be used by drivers in mach directory. This could in effect
> >replace apis such as omap2_get_mpuss_device,
> >omap2_get_iva_device, omap2_get_l3_device, omap4_get_dsp_device
> >etc..
> >
> >Signed-off-by: Nishanth Menon<nm@ti.com>
> >---
> >  arch/arm/mach-omap2/omap_hwmod.c             |   33 ++++++++++++++++++++++++++
> >  arch/arm/plat-omap/include/plat/omap_hwmod.h |    2 +
> >  2 files changed, 35 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> >index 293fa6c..77d01a2 100644
> >--- a/arch/arm/mach-omap2/omap_hwmod.c
> >+++ b/arch/arm/mach-omap2/omap_hwmod.c
> >@@ -142,6 +142,7 @@
> >  #include "powerdomain.h"
> >  #include<plat/clock.h>
> >  #include<plat/omap_hwmod.h>
> >+#include<plat/omap_device.h>
> 
> I'd rather put that code inside the omap_device.c instead of here.
> The omap_device layer is on top of the omap_hwmod.
> In order to minimize the dependencies from the low HW description
> layer to the omap_device layer, you should maybe define a
> omap_device_from_hwmod() function or something similar.
> 
> That being said, do we really need to get the device from the hwmod
> name? Cannot we use the device name instead?
> I do not know all the usecases, that why I'm asking.

that's a good question, I only suggested the above given the fact that
we already have four functions to grab four different devices. It was
only a way to combine all of those with a simple argument.
Nishanth Menon July 28, 2011, 1:31 p.m. UTC | #4
On Thu, Jul 28, 2011 at 07:57, Cousson, Benoit <b-cousson@ti.com> wrote:
[...]
>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c
>> b/arch/arm/mach-omap2/omap_hwmod.c
>> index 293fa6c..77d01a2 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -142,6 +142,7 @@
>>  #include "powerdomain.h"
>>  #include<plat/clock.h>
>>  #include<plat/omap_hwmod.h>
>> +#include<plat/omap_device.h>
>
> I'd rather put that code inside the omap_device.c instead of here.
> The omap_device layer is on top of the omap_hwmod.
> In order to minimize the dependencies from the low HW description layer to
> the omap_device layer, you should maybe define a omap_device_from_hwmod()
> function or something similar.
Thanks for the review. will check on this.

>
> That being said, do we really need to get the device from the hwmod name?
> Cannot we use the device name instead?
> I do not know all the usecases, that why I'm asking.
mpu.0 , are the device names - which probably lets me walk the kernel
data structrues instead of omap database to get to the right device,
Vs using the common names like "mpu" " make things a little easier to
deal with from driver perspective.

as an example, some of the dev_driver_string(dev):dev_name(dev) (in
bracket hwmod name) I collected from OMAP4 are:
platform:mpu.0 ("mpu")
platform:l3_main_1.0 ('l3_main_1")
pvrsrvkm:pvrsrvkm.0 ("gpu")
rpres:fdif.0 ("fdif")
omap_hsi:omap_hsi.0 ("hsi")
platform:iss.0 ("iss")
etc..

I mean I have'nt been keeping track of the device tree discussions so
dont know if this function could be better done - but I think I agree
with the overall idea that instead of spawning off get_xyz_device() we
need to have some uniform approach to get to the device scaling
silicon - I hoped we could consider the hwmod database/what ever
replacing it to do that.

Regards,
Nishanth Menon

Patch
diff mbox

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 293fa6c..77d01a2 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -142,6 +142,7 @@ 
 #include "powerdomain.h"
 #include <plat/clock.h>
 #include <plat/omap_hwmod.h>
+#include <plat/omap_device.h>
 #include <plat/prcm.h>
 
 #include "cm2xxx_3xxx.h"
@@ -2369,3 +2370,35 @@  int omap_hwmod_no_setup_reset(struct omap_hwmod *oh)
 
 	return 0;
 }
+
+/**
+ * omap_hwmod_to_device() - convert a hwmod name to device pointer
+ * @oh_name: name of the hwmod device
+ *
+ * returns back a struct device * pointer associated with a hwmod
+ * device represented by a hwmod_name
+ */
+struct device *omap_hwmod_to_device(const char *oh_name)
+{
+	struct omap_hwmod *oh;
+
+	if (!oh_name) {
+		WARN(1, "%s: no hwmod name!\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	oh = _lookup(oh_name);
+	if (IS_ERR_OR_NULL(oh)) {
+		WARN(1, "%s: no hwmod for %s\n", __func__,
+			oh_name);
+		return ERR_PTR(-ENODEV);
+	}
+	if (IS_ERR_OR_NULL(oh->od)) {
+		WARN(1, "%s: no omap_device for %s\n", __func__,
+			oh_name);
+		return ERR_PTR(-ENODEV);
+	}
+
+	return &oh->od->pdev.dev;
+}
+EXPORT_SYMBOL(omap_hwmod_to_device);
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 1adea9c..b9eec08 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -611,4 +611,6 @@  extern int omap2430_hwmod_init(void);
 extern int omap3xxx_hwmod_init(void);
 extern int omap44xx_hwmod_init(void);
 
+extern struct device *omap_hwmod_to_device(const char *oh_name);
+
 #endif