switch_gpio for headset detection

[Misael Lopez]

Hi,

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

[Greg KH]

On Wed, Mar 18, 2009 at 1:54 PM, Misael Lopez <mes...@gmail.com> wrote:
>
> Hi,
>
> 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.

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

[Misael Lopez]

Hi Greg,

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";

[Greg KH]

On Wed, Mar 18, 2009 at 7:58 PM, Misael Lopez <mes...@gmail.com> wrote:
> Hi Greg,
>
> 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.

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

[Misael Lopez]

>> 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.
>
> Do you have a pointer to those drivers?

Those drivers are provided along with the android kernel. You can find them at:

http://android.git.kernel.org/?p=kernel/common.git;a=tree;f=drivers/switch;h=c02cb39fc78a60aa4b046481596d7b184b39fa8a;hb=453dc2b690b52248ac6dcb068965f1a37d675cef

> 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

[Greg KH]

On Wed, Mar 18, 2009 at 10:52 PM, Misael Lopez <mes...@gmail.com> wrote:
>>> 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.
>>
>> Do you have a pointer to those drivers?
>
> Those drivers are provided along with the android kernel. You can find them at:
>
> http://android.git.kernel.org/?p=kernel/common.git;a=tree;f=drivers/switch;h=c02cb39fc78a60aa4b046481596d7b184b39fa8a;hb=453dc2b690b52248ac6dcb068965f1a37d675cef

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

[Misael Lopez]

2009/3/19 Greg KH <gre...@gmail.com>:

> On Wed, Mar 18, 2009 at 10:52 PM, Misael Lopez <mes...@gmail.com> wrote:
>>>> 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.
>>>
>>> Do you have a pointer to those drivers?
>>
>> Those drivers are provided along with the android kernel. You can find them at:
>>
>> http://android.git.kernel.org/?p=kernel/common.git;a=tree;f=drivers/switch;h=c02cb39fc78a60aa4b046481596d7b184b39fa8a;hb=453dc2b690b52248ac6dcb068965f1a37d675cef
>
> Ah, thanks, missed that.

> 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

[Mark Brown]

On Thu, Mar 19, 2009 at 05:50:35PM -0600, Misael Lopez wrote:
> 2009/3/19 Greg KH <gre...@gmail.com>:

> > 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.

[Greg KH]

On Thu, Mar 19, 2009 at 4:50 PM, Misael Lopez <mes...@gmail.com> wrote:
> 2009/3/19 Greg KH <gre...@gmail.com>:
>> On Wed, Mar 18, 2009 at 10:52 PM, Misael Lopez <mes...@gmail.com> wrote:
>>>>> 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.
>>>>
>>>> Do you have a pointer to those drivers?
>>>
>>> Those drivers are provided along with the android kernel. You can find them at:
>>>
>>> http://android.git.kernel.org/?p=kernel/common.git;a=tree;f=drivers/switch;h=c02cb39fc78a60aa4b046481596d7b184b39fa8a;hb=453dc2b690b52248ac6dcb068965f1a37d675cef
>>
>> Ah, thanks, missed that.
>
>> 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

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

[Misael Lopez]

>>> 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
>
> Yes, I do know of that, so why ignore it?

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

[Michael Trimarchi]

Hi

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]

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.
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)?

Is the above functionality not working with android kernel?Or is it a
bug for which you have this patch??


On Aug 26, 1:53 am, Michael Trimarchi <trimar...@gandalf.sssup.it>
wrote:

>  switch_gpio.patch
> 2KViewDownload

[Michael Trimarchi]

Hi

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