Hi Jonathan,

Wednesday 03 June 2009 00:04:33 Jonathan McDowell napisał(a):

I'm obviously too late as I've seen the "Applied" mail,

No problem, I'm glad to hear from you.

but some 
comments:

* I don't think SW_HEADPHONE_INSERT is appropriate. input guys, is it 
  not reasonable to have SW_PHONE_HOOK or similar?

I do share you point of view. However, I didn't want to start a discussion if 
there is a need for another symbol or not before the patch got any 
acceptance.

* We assume the bootloader does the appropriate GPIO pin setup for us,
  so I don't think your omap_cfg_reg is required but it doesn't hurt in
  the unlikely event we ever replace the Amstrad PBL.

OK, let it stay there. Do you see a need for replaceing it with a new 
ams_delta_hook_switch_init() function call that just calls omap_cfg_reg()?

* The commented out code to include the GPIO status in sysfs shouldn't
  be included.

Yes, I put it there to get a feedback.

  Does the input layer not provide a way to obtain the 
  state of the switch?

Yes, it does, with EVIOCGSW ioctl()[1]. I personally don't like this way of 
getting the switch status and would rather see it available over sysfs. 
However, input guys may have their own preferences and gpio-keys driver 
belongs to them.

Thanks,
Janusz

[1] http://www.spinics.net/lists/linux-input/msg03482.html

P.S. I include my Signed-off-by, as Tony have requested, to avoid the code 
without one floating around.

Signed-off-by: Janusz Krzysztofik <redacted>

quoted

diff -Npru a/arch/arm/mach-omap1/board-ams-delta.c
b/arch/arm/mach-omap1/board-ams-delta.c ---
a/arch/arm/mach-omap1/board-ams-delta.c	2009-05-17 17:58:18.000000000
+0200 +++ b/arch/arm/mach-omap1/board-ams-delta.c	2009-05-17
16:22:59.000000000 +0200 @@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/input.h>
+#include <linux/gpio_keys.h>
 #include <linux/platform_device.h>

 #include <mach/hardware.h>

@@ -213,10 +214,35 @@ static struct platform_device ams_delta_
 	.id	= -1
 };

+static struct gpio_keys_button ams_delta_gpio_keys_buttons[] = {
+	[0] = {
+		.desc		= "hook_switch",
+		.type		= EV_SW,		/* or EV_KEY ? */
+		.code		= SW_HEADPHONE_INSERT,	/* or a new one ? */
+		.active_low	= 1,
+		.gpio		= AMS_DELTA_GPIO_PIN_HOOK_SWITCH,
+		.debounce_interval = 10,
+	},
+};
+
+static struct gpio_keys_platform_data ams_delta_gpio_keys = {
+	.buttons		= ams_delta_gpio_keys_buttons,
+	.nbuttons		= ARRAY_SIZE(ams_delta_gpio_keys_buttons),
+};
+
+static struct platform_device ams_delta_gpio_keys_device = {
+	.name			= "gpio-keys",
+	.id			= -1,
+	.dev			= {
+		.platform_data	= &ams_delta_gpio_keys,
+	},
+};
+
 static struct platform_device *ams_delta_devices[] __initdata = {
 	&ams_delta_kp_device,
 	&ams_delta_lcd_device,
 	&ams_delta_led_device,
+	&ams_delta_gpio_keys_device,
 };

 static void __init ams_delta_init(void)

@@ -233,6 +259,13 @@ static void __init ams_delta_init_irq(vo

 	omap_usb_init(&ams_delta_usb_config);
 	platform_add_devices(ams_delta_devices, ARRAY_SIZE(ams_delta_devices));
+
+	omap_cfg_reg(P20_1610_GPIO4); /* is this required? */
+
+	/* The hook switch state could be exposed over sysfs with

gpio_export(). +	 * This should be done after the gpio-keys driver calls
gpio_request(), +	 * but I don't know how to do this from outside of the
driver. */ +	/* gpio_export(AMS_DELTA_GPIO_PIN_HOOK_SWITCH, 0); */
 }

 static void __init ams_delta_map_io(void)

J.

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