On Thu, Mar 19, 2009 at 4:50 PM, Misael Lopez <[email protected]> wrote:
> 2009/3/19 Greg KH <[email protected]>:
>> On Wed, Mar 18, 2009 at 10:52 PM, Misael Lopez <[email protected]> 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?
>> 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);
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
--~--~---------~--~----~------------~-------~--~----~
unsubscribe: [email protected]
website: http://groups.google.com/group/android-kernel
-~----------~----~----~----~------~----~------~--~---
