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 <jkrzyszt@xxxxxxxxxxxx> > > 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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
