I'm using switch_class and switch_gpio drivers for headset detection,
however I had to make some modifications to the current code to make
it work. I'd like to share that and also get feedback from you.
1. When requesting the gpio-based interrupt line I had problems with
the current flag (IRQF_TRIGGER_LOW) because it's not supported by the
Interrupt Handler mechanism. The types of interrupts supported in my
case are rising/falling triggers. For headset detection, it seems
logical to me to use rising/falling triggers for insertion/removal
detection, so I think we can use them instead directly in the
switch_gpio driver. Although probably I'm missing a situation when
level triggers need to be used. If that is the case, then letting
switch_gpio client drivers to specify the irqflags could be a good
idea.
2. The DEVPATH value generated by the uevent doesn't match with the
one specified in HeadsetObserver class, which is currently
"DEVPATH=/devices/virtual/switch/h2w". switch_class driver generates a
value "DEVPATH=/class/switch/h2w". I guess that is only a matter of
updating HeadsetObserver code.
Please let me know your comments.
-Misa
It would probably be easier for you to post your patch, as that way it
would be more obvious as to what you are doing.
Can you please post them?
thanks,
greg k-h
Below are the my changes in kernel side. As mentioned in previous
mail, I'm using Switch GPIO driver (which uses Switch Class driver) to
generate the headset uevent. In the patch, it's also included the
board file (for OMAP 3430SDP board) although it's not part of standard
android kernel it may be useful for reference.
diff --git a/arch/arm/mach-omap2/board-3430sdp.c
b/arch/arm/mach-omap2/board-3430sdp.c
index 0000d3b..701c861 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -24,6 +24,7 @@
#include <linux/spi/ads7846.h>
#include <linux/i2c/twl4030.h>
#include <linux/mm.h>
+#include <linux/switch.h>
#include <mach/hardware.h>
#include <asm/mach-types.h>
@@ -1140,10 +1141,24 @@ static struct platform_device sdp3430_lcd_device = {
.id = -1,
};
+
+static struct gpio_switch_platform_data headset_switch_data = {
+ .name = "h2w",
+ .gpio = OMAP_MAX_GPIO_LINES + 2, /*TWL4030 GPIO_2 */
+};
+
+static struct platform_device headset_switch_device = {
+ .name = "switch-gpio",
+ .dev = {
+ .platform_data = &headset_switch_data,
+ }
+};
+
static struct platform_device *sdp3430_devices[] __initdata = {
&sdp3430_smc91x_device,
&irda_device,
&sdp3430_lcd_device,
+ &headset_switch_device,
};
static inline void __init sdp3430_init_smc91x(void)
@@ -1224,6 +1239,7 @@ static struct twl4030_gpio_platform_data
sdp3430_gpio_data = {
.irq_end = TWL4030_GPIO_IRQ_END,
};
+
static struct twl4030_usb_data sdp3430_usb_data = {
.usb_mode = T2_USB_MODE_ULPI,
};
diff --git a/drivers/switch/switch_gpio.c b/drivers/switch/switch_gpio.c
index cfd1339..82e29a2 100644
--- a/drivers/switch/switch_gpio.c
+++ b/drivers/switch/switch_gpio.c
@@ -22,6 +22,7 @@
#include <linux/switch.h>
#include <linux/workqueue.h>
#include <linux/gpio.h>
+#include <linux/interrupt.h>
struct gpio_switch_data {
struct switch_dev sdev;
@@ -110,7 +111,8 @@ static int gpio_switch_probe(struct platform_device *pdev)
}
ret = request_irq(switch_data->irq, gpio_irq_handler,
- IRQF_TRIGGER_LOW, pdev->name, switch_data);
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ pdev->name, switch_data);
if (ret < 0)
goto err_request_irq;
In Android side, the DEVPATH value generated by the uevent doesn't
match with the one specified in HeadsetObserver.
diff --git a/services/java/com/android/server/HeadsetObserver.java
b/services/java/com/android/server/HeadsetObserver.java
index 2bea731..e5c7706 100644
--- a/services/java/com/android/server/HeadsetObserver.java
+++ b/services/java/com/android/server/HeadsetObserver.java
@@ -32,7 +32,7 @@ import java.io.FileNotFoundException;
class HeadsetObserver extends UEventObserver {
private static final String TAG = HeadsetObserver.class.getSimpleName();
- private static final String HEADSET_UEVENT_MATCH =
"DEVPATH=/devices/virtual/switch/h2w";
+ private static final String HEADSET_UEVENT_MATCH =
"DEVPATH=/class/switch/h2w";
private static final String HEADSET_STATE_PATH =
"/sys/class/switch/h2w/state";
private static final String HEADSET_NAME_PATH =
"/sys/class/switch/h2w/name";
Do you have a pointer to those drivers?
I ask as I'm trying to get all of these various pieces merged upstream
properly, and if there are drivers that are out-of-tree, I'd really like
to know about them.
Looks sane to me.
> In Android side, the DEVPATH value generated by the uevent doesn't
> match with the one specified in HeadsetObserver.
>
> diff --git a/services/java/com/android/server/HeadsetObserver.java
> b/services/java/com/android/server/HeadsetObserver.java
> index 2bea731..e5c7706 100644
> --- a/services/java/com/android/server/HeadsetObserver.java
> +++ b/services/java/com/android/server/HeadsetObserver.java
> @@ -32,7 +32,7 @@ import java.io.FileNotFoundException;
> class HeadsetObserver extends UEventObserver {
> private static final String TAG = HeadsetObserver.class.getSimpleName();
>
> - private static final String HEADSET_UEVENT_MATCH =
> "DEVPATH=/devices/virtual/switch/h2w";
> + private static final String HEADSET_UEVENT_MATCH =
> "DEVPATH=/class/switch/h2w";
That worries me, are you perhaps not building your kernel with
CONFIG_SYSFS_DEPRECATED disabled? Do you not have any
"/sys/devices/virtual/" files? You shouldn't have to make this change.
thanks,
greg k-h
Those drivers are provided along with the android kernel. You can find them at:
> I ask as I'm trying to get all of these various pieces merged upstream
> properly, and if there are drivers that are out-of-tree, I'd really like
> to know about them.
>> In Android side, the DEVPATH value generated by the uevent doesn't
>> match with the one specified in HeadsetObserver.
>>
>> diff --git a/services/java/com/android/server/HeadsetObserver.java
>> b/services/java/com/android/server/HeadsetObserver.java
>> index 2bea731..e5c7706 100644
>> --- a/services/java/com/android/server/HeadsetObserver.java
>> +++ b/services/java/com/android/server/HeadsetObserver.java
>> @@ -32,7 +32,7 @@ import java.io.FileNotFoundException;
>> class HeadsetObserver extends UEventObserver {
>> private static final String TAG = HeadsetObserver.class.getSimpleName();
>>
>> - private static final String HEADSET_UEVENT_MATCH =
>> "DEVPATH=/devices/virtual/switch/h2w";
>> + private static final String HEADSET_UEVENT_MATCH =
>> "DEVPATH=/class/switch/h2w";
>
> That worries me, are you perhaps not building your kernel with
> CONFIG_SYSFS_DEPRECATED disabled? Do you not have any
> "/sys/devices/virtual/" files? You shouldn't have to make this change.
Yes, I had that flag enabled. After disabling it, now I can see the
entry under /sys/devices/virtual/.
Thanks!
-Misa
Ah, thanks, missed that.
Perhaps such a generic "switch" name would be good to change, that's
very vague.
Also, why a special class? Why not use the input layer for stuff like
this?
And, why have a name attribute that should be obvious from the name of
the struct device? Why the duplication?
Also, putting 280 bytes on the stack isn't that nice in the
switch_set_state() function, is it really needed? Why can't that be
dynamic?
And I don't think the "name" has to be duplicated here as it is already
transmitted in the uevent.
Hey code review on the android-kernel list, it's about time :)
thanks,
greg k-h
> Also, why a special class? Why not use the input layer for stuff like
> this?
In the particular case of headset detection, I originally considered
using ALSA Jack mechanism (and Soc Jack wrapper), which uses input
layer. You probably know about that already.
- Generic ALSA Jack mechanism
http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=blob;f=sound/core/jack.c;h=dd4a12dc09aa44f4c50c5b60f2cf5584c5635589;hb=632087748c3795a54d5631e640df65592774e045
- Wrapper for ALSA SoC drivers
http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=blob;f=sound/soc/soc-jack.c;h=28346fb2e70c08033158920f4ae7a35032239762;hb=632087748c3795a54d5631e640df65592774e045
> Also, putting 280 bytes on the stack isn't that nice in the
> switch_set_state() function, is it really needed? Why can't that be
> dynamic?
Please take a look of below change:
diff --git a/drivers/switch/switch_class.c b/drivers/switch/switch_class.c
index 7702882..eee5188 100644
--- a/drivers/switch/switch_class.c
+++ b/drivers/switch/switch_class.c
@@ -22,10 +22,14 @@
#include <linux/fs.h>
#include <linux/err.h>
#include <linux/switch.h>
+#include <linux/string.h>
struct class *switch_class;
static atomic_t device_count;
+static const char *switch_name_text = "SWITCH_NAME=";
+static const char *switch_state_text = "SWITCH_STATE=";
+
static ssize_t state_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -59,8 +63,8 @@ static DEVICE_ATTR(name, S_IRUGO | S_IWUSR, name_show, NULL);
void switch_set_state(struct switch_dev *sdev, int state)
{
- char name_buf[120];
- char state_buf[120];
+ char *name_buf;
+ char *state_buf;
char *prop_buf;
char *envp[3];
int env_offset = 0;
@@ -75,21 +79,27 @@ void switch_set_state(struct switch_dev *sdev, int state)
if (length > 0) {
if (prop_buf[length - 1] == '\n')
prop_buf[length - 1] = 0;
- snprintf(name_buf, sizeof(name_buf),
- "SWITCH_NAME=%s", prop_buf);
+ length += strlen(switch_name_text);
+ name_buf = kzalloc(length, GFP_KERNEL);
+ snprintf(name_buf, length,
+ "%s%s", switch_name_text, prop_buf);
envp[env_offset++] = name_buf;
}
length = state_show(sdev->dev, NULL, prop_buf);
if (length > 0) {
if (prop_buf[length - 1] == '\n')
prop_buf[length - 1] = 0;
- snprintf(state_buf, sizeof(state_buf),
- "SWITCH_STATE=%s", prop_buf);
+ length += strlen(switch_state_text);
+ state_buf = kzalloc(length, GFP_KERNEL);
+ snprintf(state_buf, length,
+ "%s%s", switch_state_text, prop_buf);
envp[env_offset++] = state_buf;
}
envp[env_offset] = NULL;
kobject_uevent_env(&sdev->dev->kobj, KOBJ_CHANGE, envp);
free_page((unsigned long)prop_buf);
+ kfree(name_buf);
+ kfree(state_buf);
} else {
printk(KERN_ERR "out of memory in switch_set_state\n");
kobject_uevent(&sdev->dev->kobj, KOBJ_CHANGE);
> And I don't think the "name" has to be duplicated here as it is already
> transmitted in the uevent.
The uevent generated by switch_class contains:
change@/devices/virtual/switch/h2w
ACTION=change
DEVPATH=/devices/virtual/switch/h2w
SUBSYSTEM=switch
SWITCH_NAME=h2w
SWITCH_STATE=0
SEQNUM=846
Where to get the "name" if SWITCH_NAME were not included? To extract
from the path?
How can switch's value be checked? Is it right using SWITCH_STATE?
Does the uevent sends that information in some way already?
I guess ACTION could be a choice, although switch_class driver sets it
always to KOBJ_CHANGE.
When I _cat_ the uevent entry (of Jack in ALSA (input subsystem based)
I see PHYSDEVPATH, PRODUCT, NAME, etc; but when I _cat_ the uevent
entry generated by Switch Class it's empty. Any reason behind that?
Thanks,
-Misa
> > Also, why a special class? ?Why not use the input layer for stuff like
> > this?
> In the particular case of headset detection, I originally considered
> using ALSA Jack mechanism (and Soc Jack wrapper), which uses input
> layer. You probably know about that already.
What made you decide not to use either the generic ALSA stuff or ASoC?
If there's something preventing you using them then it'd be good to fix
it.
Yes, I do know of that, so why ignore it?
Looks better, thanks.
>> And I don't think the "name" has to be duplicated here as it is already
>> transmitted in the uevent.
>
> The uevent generated by switch_class contains:
>
> change@/devices/virtual/switch/h2w
> ACTION=change
> DEVPATH=/devices/virtual/switch/h2w
> SUBSYSTEM=switch
> SWITCH_NAME=h2w
> SWITCH_STATE=0
> SEQNUM=846
>
> Where to get the "name" if SWITCH_NAME were not included? To extract
> from the path?
yes.
> How can switch's value be checked? Is it right using SWITCH_STATE?
I don't want to reply as I don't think you should be doing this this way
at all :)
Use the kernel interfaces we already have for this kind of thing, don't
create new ones, that just ensures that we are not going to be able to
get this code merged upstream and you will be maintaining it on your own
for the next 12 years, forward porting it for every api change that
happens.
> Does the uevent sends that information in some way already?
> I guess ACTION could be a choice, although switch_class driver sets it
> always to KOBJ_CHANGE.
Change is correct.
> When I _cat_ the uevent entry (of Jack in ALSA (input subsystem based)
> I see PHYSDEVPATH, PRODUCT, NAME, etc; but when I _cat_ the uevent
> entry generated by Switch Class it's empty. Any reason behind that?
You must have CONFIG_SYSFS_DEPRECATED enabled to see those values,
(that's where PHYSDEVPATH comes from). PRODUCT and NAME, I'm not sure
where it comes from, probably the ALSA core.
Please rethink this and use the apis we already have.
thanks,
greg k-h
The only reason I chose the Android's Switch Class/Gpio driver is
because it fully matches what HeadsetObserver expects in the uevent.
I'll use ASoC mechanism instead, and modify HeadsetObserver accordingly.
Thanks,
-Misa
I'm using this patch in android. It is not for submission of course
Yes this is the right thing todo, I don't see all the code but what
happen if the headset is removed
during suspend? Where the worker is forced to be rescheduled in soc-jack?
Michael
> --~--~---------~--~----~------------~-------~--~----~
> unsubscribe: android-kerne...@googlegroups.com
> website: http://groups.google.com/group/android-kernel
> -~----------~----~----~----~------~----~------~--~---
>
>
>
ani wrote:
> Hello Michael,
>
> What is this patch suppose to achieve which is not already done in
> current android kernel?
>
> AFAIK this patch is a workaround when headset is not detected if you
> insert during suspend as the system is in sleep
> state and also if the headset event is not registered as wake up
> source.
>
just an example here:
pxa320 in deepsleepmode the gpio is not a wakeup source so you need
to recheck the headset state.
> So basically when we wake up the pm will call resume and there you are
> scheduling your work to find out if the
> headset state is changed or not(this is what is achieved by this
> patch)?
>
Yes
> Is the above functionality not working with android kernel?Or is it a
> bug for which you have this patch??
>
>
>
If you want to use the android-kernel, you need to do some work around
for your specific
platform.
Michael