Message ID | 200905262057.21890.jkrzyszt@tis.icnet.pl (mailing list archive) |
---|---|
State | Changes Requested, archived |
Commit | 89c6da9692bb04ce6221c371d3161f9722005e99 |
Headers | show |
This patch has been applied to the linux-omap by youw fwiendly patch wobot. Initial commit ID (Likely to change): 89c6da9692bb04ce6221c371d3161f9722005e99 PatchWorks http://patchwork.kernel.org/patch/26069/ Git (Likely to change, and takes a while to get mirrored) http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=89c6da9692bb04ce6221c371d3161f9722005e99 -- 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
* Tony Lindgren <tony@atomide.com> [090602 11:23]: > This patch has been applied to the linux-omap > by youw fwiendly patch wobot. > > Initial commit ID (Likely to change): 89c6da9692bb04ce6221c371d3161f9722005e99 > > PatchWorks > http://patchwork.kernel.org/patch/26069/ > > Git (Likely to change, and takes a while to get mirrored) > http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=89c6da9692bb04ce6221c371d3161f9722005e99 Argh, this is missing Signed-off-by too, please reply with your Signed-off-by. Or repost with proper description. Everybody, please try to send your patches with git send-mail, with proper Signed-off-by and description. It is no fun to manually edit patches. Thanks, Tony -- 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
On Tue, May 26, 2009 at 08:57:20PM +0200, Janusz Krzysztofik wrote: > This trivial patch adds gpio-keys compatible platform device definition to > ams-delta board, that supports hook switch found on this videophone. It is > derived from similiar definitions found in other boards code. The patch is > based on linux-2.6.30-rc5. Any comments are welcome. I'm obviously too late as I've seen the "Applied" mail, 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 can't find anything else in tree I know to be a VOIP phone that has the concept of a hook switch except perhaps the TuxScreen and it doesn't have anything set up that I could find. I think you're correct that EV_SW is appropriate; it may remain in the off-hook state for some time and AFAICT the gpio-key driver would produce a repeating key press if you used EV_KEY. * 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. * The commented out code to include the GPIO status in sysfs shouldn't be included. Does the input layer not provide a way to obtain the state of the switch? > 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.
Tuesday 02 June 2009 20:27:50 Tony Lindgren napisał(a): > * Tony Lindgren <tony@atomide.com> [090602 11:23]: > > This patch has been applied to the linux-omap > > by youw fwiendly patch wobot. > > > > Initial commit ID (Likely to change): > > 89c6da9692bb04ce6221c371d3161f9722005e99 > > > > PatchWorks > > http://patchwork.kernel.org/patch/26069/ > > > > Git (Likely to change, and takes a while to get mirrored) > > http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=com > >mit;h=89c6da9692bb04ce6221c371d3161f9722005e99 > > Argh, this is missing Signed-off-by too, please reply with your > Signed-off-by. Or repost with proper description. Tony, I'm sorry, I'm new here and did not realise that my message would ever be processed by a "fwiendly patch wobot". Was this because of the [PATCH] label I had put into subject line? Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis,icnet.pl> -- 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
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@tis.icnet.pl> > > 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
* Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [090603 01:01]: > Tuesday 02 June 2009 20:27:50 Tony Lindgren napisał(a): > > * Tony Lindgren <tony@atomide.com> [090602 11:23]: > > > This patch has been applied to the linux-omap > > > by youw fwiendly patch wobot. > > > > > > Initial commit ID (Likely to change): > > > 89c6da9692bb04ce6221c371d3161f9722005e99 > > > > > > PatchWorks > > > http://patchwork.kernel.org/patch/26069/ > > > > > > Git (Likely to change, and takes a while to get mirrored) > > > http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=com > > >mit;h=89c6da9692bb04ce6221c371d3161f9722005e99 > > > > Argh, this is missing Signed-off-by too, please reply with your > > Signed-off-by. Or repost with proper description. > > Tony, > > I'm sorry, I'm new here and did not realise that my message would ever be > processed by a "fwiendly patch wobot". Was this because of the [PATCH] label > I had put into subject line? > > > Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis,icnet.pl> > Well sounds like further changes are needed, I did not push it yet so please resubmit. Tony -- 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
Wednesday 03 June 2009 18:36:52 Tony Lindgren napisał(a): > * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [090603 01:01]: > > Tuesday 02 June 2009 20:27:50 Tony Lindgren napisał(a): > > > * Tony Lindgren <tony@atomide.com> [090602 11:23]: > > > > This patch has been applied to the linux-omap > > > > by youw fwiendly patch wobot. > > > > > > > > Initial commit ID (Likely to change): > > > > 89c6da9692bb04ce6221c371d3161f9722005e99 > > > > > > > > PatchWorks > > > > http://patchwork.kernel.org/patch/26069/ > > > > > > > > Git (Likely to change, and takes a while to get mirrored) > > > > http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a > > > >=com mit;h=89c6da9692bb04ce6221c371d3161f9722005e99 > > > > > > Argh, this is missing Signed-off-by too, please reply with your > > > Signed-off-by. Or repost with proper description. > > > > Tony, > > > > I'm sorry, I'm new here and did not realise that my message would ever be > > processed by a "fwiendly patch wobot". Was this because of the [PATCH] > > label I had put into subject line? > > > > > > Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis,icnet.pl> > > Well sounds like further changes are needed, I did not push it yet > so please resubmit. > > Tony OK, I will, thanks, Janusz -- 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
On Wed, Jun 03, 2009 at 11:03:20AM +0200, Janusz Krzysztofik wrote: > 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()? I don't see a need for this; it's always present and not a lot of code to have in the init function as it stands. > > * 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. I think that's a discussion to have with the input guys rather than putting a hack in the platform file then. So really the only issue with the patch that remains is if it justifies adding a new SW_PHONE_HOOK switch type? J.
Hi, Thursday 04 June 2009 20:18:33 Jonathan McDowell napisał(a): > On Wed, Jun 03, 2009 at 11:03:20AM +0200, Janusz Krzysztofik wrote: > > Wednesday 03 June 2009 00:04:33 Jonathan McDowell napisał(a): > > > > > * 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()? > > I don't see a need for this; it's always present and not a lot of code > to have in the init function as it stands. Fine, I'll only add a comment explainig the purpose of the call then. > > > 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. > > I think that's a discussion to have with the input guys rather than > putting a hack in the platform file then. Sure. I have a patch for gpio-keys.c ready to submit, let's see how far we can get with the idea of exporting a gpio-keys driven switch state over gpiolib sysfs. > So really the only issue with the patch that remains is if it justifies > adding a new SW_PHONE_HOOK switch type? I've just submitted a patch that adds new symbol definition to include/linux/input.h. Do I have to wait for acknowledgement before I resubmit my modified patch that depends on that, or can I submit it now? Thanks, Janusz -- 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
On 2009-06-03 18:36, Tony Lindgren wrote: > * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [090603 01:01]: >> Tuesday 02 June 2009 20:27:50 Tony Lindgren napisał(a): >>> * Tony Lindgren <tony@atomide.com> [090602 11:23]: >>>> This patch has been applied to the linux-omap >>>> by youw fwiendly patch wobot. >>>> >>>> Initial commit ID (Likely to change): >>>> 89c6da9692bb04ce6221c371d3161f9722005e99 >>>> >>>> PatchWorks >>>> http://patchwork.kernel.org/patch/26069/ >>>> >>>> Git (Likely to change, and takes a while to get mirrored) >>>> http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=com >>>> mit;h=89c6da9692bb04ce6221c371d3161f9722005e99 >>> Argh, this is missing Signed-off-by too, please reply with your >>> Signed-off-by. Or repost with proper description. >> Tony, >> >> I'm sorry, I'm new here and did not realise that my message would ever be >> processed by a "fwiendly patch wobot". Was this because of the [PATCH] label >> I had put into subject line? >> >> >> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis,icnet.pl> >> > > Well sounds like further changes are needed, I did not push it yet > so please resubmit. Tony, I have noticed that the patch has entered omap1-upstream branch. Is it possible to revert it? It will conflict with a new different, ASoC jack based implementation I am currently working on instead. Thanks, Janusz -- 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
* Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [090624 17:05]: > On 2009-06-03 18:36, Tony Lindgren wrote: >> * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [090603 01:01]: >>> Tuesday 02 June 2009 20:27:50 Tony Lindgren napisał(a): >>>> * Tony Lindgren <tony@atomide.com> [090602 11:23]: >>>>> This patch has been applied to the linux-omap >>>>> by youw fwiendly patch wobot. >>>>> >>>>> Initial commit ID (Likely to change): >>>>> 89c6da9692bb04ce6221c371d3161f9722005e99 >>>>> >>>>> PatchWorks >>>>> http://patchwork.kernel.org/patch/26069/ >>>>> >>>>> Git (Likely to change, and takes a while to get mirrored) >>>>> http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=com >>>>> mit;h=89c6da9692bb04ce6221c371d3161f9722005e99 >>>> Argh, this is missing Signed-off-by too, please reply with your >>>> Signed-off-by. Or repost with proper description. >>> Tony, >>> >>> I'm sorry, I'm new here and did not realise that my message would >>> ever be processed by a "fwiendly patch wobot". Was this because of >>> the [PATCH] label I had put into subject line? >>> >>> >>> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis,icnet.pl> >>> >> >> Well sounds like further changes are needed, I did not push it yet >> so please resubmit. > > Tony, > > I have noticed that the patch has entered omap1-upstream branch. Is it > possible to revert it? It will conflict with a new different, ASoC jack > based implementation I am currently working on instead. Oops, sorry I guess I forgot to revert it there, will remove. Tony -- 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
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)