Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-11 Thread Tony Lindgren 
* Dmitry Torokhov  [100601 23:07]:
> On Tue, Jun 01, 2010 at 09:53:58PM +0300, Felipe Balbi wrote:
> > On Tue, Jun 01, 2010 at 05:14:09PM +0200, ext Arce, Abraham wrote:
> > >I am using #ifdef CONFIG_ARCH_OMAP4 for this portion of code, what
> > >you are suggesting is to check at runtime?
> > 
> > you need to add both checks. If you build omap3-only or omap2-only
> > you don't want that code to be compiled, but if you build
> > omap1-2-3-4 kernel, you want it to work correctly on all cases.
> > 
> 
> It sould be nice if cpu_is_xxx were stubbed out for "wrong" arches
> withing the same group (like omap, etc) so we could reduce the #ifdef
> clutter.

They are defined as stubs for for non-selected omaps. So in general a
function doing this in the beginning of the function:

if (!cpu_is_omap44xx())
return -ENODEV;

Should already get optimized out if 44xx is not selected in Kconfig.

Regards,

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-02 Thread Thomas Petazzoni 
On Wed, 2 Jun 2010 07:45:07 -0500
"Arce, Abraham"  wrote:

> I'll remove length variable and keep snprintf, below oh_name -> kbd is used 
> again, this will keep name defined in one single place
> 
>   WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
>   name, oh_name);

In this case, why not:

char *oh_name = "kbd";

There's really no point in using snprintf() for statically-defined
strings.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-02 Thread Arce, Abraham 
Thanks Thomas,

> > +   unsigned int length = 0, id = 0;
> > +   int hw_mod_name_len = 16;
> > +   char oh_name[hw_mod_name_len];
> > +   char *name = "omap4-keypad";
> > +
> > +   length = snprintf(oh_name, hw_mod_name_len, "kbd");
> > +
> > +   oh = omap_hwmod_lookup(oh_name);
> > +   if (!oh) {
> > +   pr_err("Could not look up %s\n", oh_name);
> > +   return -EIO;
> > +   }
> 
> Maybe I'm missing something here, but I don't see where "length" is
> being used, and why the snprintf()/oh_name thing is needed. What about:
> 
>   unsigned int id = 0;
>   char *name = "omap4-keypad";
> 
>   oh = omap_hwmod_lookup("kbd");
>   if (!oh) {
>   pr_err("Could not look up kbd\n");
>   return -EIO;
>   }

I'll remove length variable and keep snprintf, below oh_name -> kbd is used 
again, this will keep name defined in one single place

WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
name, oh_name);

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-02 Thread Thomas Petazzoni 
On Mon, 31 May 2010 16:44:52 -0500
"Arce, Abraham"  wrote:

> + unsigned int length = 0, id = 0;
> + int hw_mod_name_len = 16;
> + char oh_name[hw_mod_name_len];
> + char *name = "omap4-keypad";
> +
> + length = snprintf(oh_name, hw_mod_name_len, "kbd");
> +
> + oh = omap_hwmod_lookup(oh_name);
> + if (!oh) {
> + pr_err("Could not look up %s\n", oh_name);
> + return -EIO;
> + }

Maybe I'm missing something here, but I don't see where "length" is
being used, and why the snprintf()/oh_name thing is needed. What about:

unsigned int id = 0;
char *name = "omap4-keypad";

oh = omap_hwmod_lookup("kbd");
if (!oh) {
pr_err("Could not look up kbd\n");
return -EIO;
}

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-01 Thread Dmitry Torokhov 
On Tue, Jun 01, 2010 at 09:53:58PM +0300, Felipe Balbi wrote:
> On Tue, Jun 01, 2010 at 05:14:09PM +0200, ext Arce, Abraham wrote:
> >I am using #ifdef CONFIG_ARCH_OMAP4 for this portion of code, what
> >you are suggesting is to check at runtime?
> 
> you need to add both checks. If you build omap3-only or omap2-only
> you don't want that code to be compiled, but if you build
> omap1-2-3-4 kernel, you want it to work correctly on all cases.
> 

It sould be nice if cpu_is_xxx were stubbed out for "wrong" arches
withing the same group (like omap, etc) so we could reduce the #ifdef
clutter.

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-01 Thread Felipe Balbi 

On Tue, Jun 01, 2010 at 05:14:09PM +0200, ext Arce, Abraham wrote:
I am using #ifdef CONFIG_ARCH_OMAP4 for this portion of code, what you 
are suggesting is to check at runtime?


you need to add both checks. If you build omap3-only or omap2-only you 
don't want that code to be compiled, but if you build omap1-2-3-4 
kernel, you want it to work correctly on all cases.


--
balbi

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-01 Thread Kevin Hilman 
Felipe Balbi  writes:

> On Tue, Jun 01, 2010 at 06:47:34AM +0200, Balbi Felipe (Nokia-D/Helsinki) 
> wrote:
>>>+pdata->device_enable = omap_device_enable;
>>
>>this is undefined at least here or previous patch.
>
> or does it come from omap_device layers ??

It's part of the omap_device layer. 

But, drivers no longer should be calling these hooks using
platform_data.  Instead, drivers should use the runtime PM API.  The
runtime PM core for OMAP will use the omap_device API.

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

RE: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-01 Thread Arce, Abraham 
Hi Tony,

> Please test this with omap3_defconfig too, and make sure it does not break
> things for omap2 and omap3.

Tested, no problems...

> > +int omap4_init_kp(struct omap4_keypad_platform_data *kp)
> > +{
> > +   struct omap_hwmod *oh;
> > +   struct omap_device *od;
> > +   struct omap4_keypad_platform_data *pdata;
> > +
> > +   unsigned int length = 0, id = 0;
> > +   int hw_mod_name_len = 16;
> > +   char oh_name[hw_mod_name_len];
> > +   char *name = "omap4-keypad";
> 
>   if (!cpu_is_omap44xx())
>   return -ENODEV;

I am using #ifdef CONFIG_ARCH_OMAP4 for this portion of code, what you are 
suggesting is to check at runtime?

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-06-01 Thread Tony Lindgren 
* Arce, Abraham  [100601 00:39]:
> Register keyboard device with hwmod framework

Please test this with omap3_defconfig too, and make sure it does not break
things for omap2 and omap3.
 
> +int omap4_init_kp(struct omap4_keypad_platform_data *kp)
> +{
> + struct omap_hwmod *oh;
> + struct omap_device *od;
> + struct omap4_keypad_platform_data *pdata;
> +
> + unsigned int length = 0, id = 0;
> + int hw_mod_name_len = 16;
> + char oh_name[hw_mod_name_len];
> + char *name = "omap4-keypad";

if (!cpu_is_omap44xx())
return -ENODEV;


Might be needed here.

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-05-31 Thread Felipe Balbi 

On Tue, Jun 01, 2010 at 06:47:34AM +0200, Balbi Felipe (Nokia-D/Helsinki) wrote:

+   pdata->device_enable = omap_device_enable;


this is undefined at least here or previous patch.


or does it come from omap_device layers ??

--
balbi

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

Re: [RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-05-31 Thread Felipe Balbi 

On Mon, May 31, 2010 at 11:44:52PM +0200, ext Arce, Abraham wrote:

+   pdata = kzalloc(sizeof(struct omap4_keypad_platform_data), GFP_KERNEL);


you will never free this.

[..]


+   pdata->device_enable = omap_device_enable;


this is undefined at least here or previous patch.


+   pdata->device_idle = omap_device_idle;


same for this


+   pdata->device_shutdown = omap_device_shutdown;


and this.

--
balbi

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

[RFC] [PATCH v3 2/4] OMAP4: Keyboard device registration

2010-05-31 Thread Arce, Abraham 
Register keyboard device with hwmod framework

Signed-off-by: Abraham Arce 
---
 arch/arm/mach-omap2/devices.c |   59 +
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 2271b9b..64d68a9 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -26,6 +27,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 #include "mux.h"
 
@@ -136,6 +140,61 @@ static inline void omap_init_camera(void)
 }
 #endif
 
+#ifdef CONFIG_ARCH_OMAP4
+
+struct omap_device_pm_latency omap_keyboard_latency[] = {
+   {
+   .deactivate_func = omap_device_idle_hwmods,
+   .activate_func   = omap_device_enable_hwmods,
+   .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+   },
+};
+
+int omap4_init_kp(struct omap4_keypad_platform_data *kp)
+{
+   struct omap_hwmod *oh;
+   struct omap_device *od;
+   struct omap4_keypad_platform_data *pdata;
+
+   unsigned int length = 0, id = 0;
+   int hw_mod_name_len = 16;
+   char oh_name[hw_mod_name_len];
+   char *name = "omap4-keypad";
+
+   length = snprintf(oh_name, hw_mod_name_len, "kbd");
+
+   oh = omap_hwmod_lookup(oh_name);
+   if (!oh) {
+   pr_err("Could not look up %s\n", oh_name);
+   return -EIO;
+   }
+
+   pdata = kzalloc(sizeof(struct omap4_keypad_platform_data), GFP_KERNEL);
+   if (!pdata) {
+   WARN(1, "Keyboard pdata memory allocation failed\n");
+   return -ENOMEM;
+   }
+
+   pdata = kp;
+
+   pdata->base = oh->_rt_va;
+   pdata->irq = oh->mpu_irqs[0].irq;
+   pdata->device_enable = omap_device_enable;
+   pdata->device_idle = omap_device_idle;
+   pdata->device_shutdown = omap_device_shutdown;
+
+   od = omap_device_build(name, id, oh, pdata,
+   sizeof(struct matrix_keypad_platform_data),
+   omap_keyboard_latency,
+   ARRAY_SIZE(omap_keyboard_latency), 1);
+   WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
+   name, oh_name);
+
+   return 0;
+}
+
+#endif
+
 #if defined(CONFIG_OMAP_MBOX_FWK) || defined(CONFIG_OMAP_MBOX_FWK_MODULE)
 
 #define MBOX_REG_SIZE   0x120
-- 
1.5.4.3

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