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

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

[Arce, Abraham]

Register keyboard device with hwmod framework

Signed-off-by: Abraham Arce <x0066660@ti.com>
---
 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 <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/clk.h>
+#include <linux/slab.h>
 
 #include <mach/hardware.h>
 #include <asm/mach-types.h>
@@ -26,6 +27,9 @@
 #include <plat/mux.h>
 #include <mach/gpio.h>
 #include <plat/mmc.h>
+#include <plat/omap_hwmod.h>
+#include <plat/omap_device.h>
+#include <plat/omap4-keypad.h>
 
 #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

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

[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

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

[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

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

[Kevin Hilman]

Felipe Balbi <felipe.balbi@nokia.com> 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

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

[Tony Lindgren]

* Arce, Abraham <x0066660@ti.com> [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

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

[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

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

[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

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

[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

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

[Tony Lindgren]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [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

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

[Thomas Petazzoni]

On Mon, 31 May 2010 16:44:52 -0500
"Arce, Abraham" <x0066660@ti.com> 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

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

[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

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

[Thomas Petazzoni]

On Wed, 2 Jun 2010 07:45:07 -0500
"Arce, Abraham" <x0066660@ti.com> 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