| Message ID | 20110728055346.GA11921@foobar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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