| Message ID | 1459023797-6418-1-git-send-email-florian.euchner@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 26, 2016 at 09:23:17PM +0100, Florian Euchner wrote: > The CM109 driver reported key press events of volume up / down and > record / playback mute buttons, but release events only as soon as > another button was pressed. Track state of volume buttons in order to > report press and release events properly. For the record and playback > mute buttons, only presses are registered by the CM109, therefore > simulate press-n-release. This fixes the volume control buttons of > various USB headsets. > > Signed-off-by: Florian Euchner <florian.euchner@gmail.com> Tested-by: Karsten Merker <merker@debian.org> I have run the tests on top of kernel 4.5.0 with a CM109-based Logilink HS0033 USB headset. Proper testing on top of kernel 4.6rc1 wasn't possible as USB audio is broken in 4.6rc1 and causes the kernel to oops during snd_usb_audio initialization (cf. https://bugzilla.kernel.org/show_bug.cgi?id=115301). evtest log: Event: time 1459279601.422879, type 1 (EV_KEY), code 115 (KEY_VOLUMEUP), value 1 Event: time 1459279601.422879, -------------- EV_SYN ------------ Event: time 1459279601.518856, type 1 (EV_KEY), code 115 (KEY_VOLUMEUP), value 0 Event: time 1459279601.518856, -------------- EV_SYN ------------ Event: time 1459279602.542861, type 1 (EV_KEY), code 114 (KEY_VOLUMEDOWN), value 1 Event: time 1459279602.542861, -------------- EV_SYN ------------ Event: time 1459279602.606847, type 1 (EV_KEY), code 114 (KEY_VOLUMEDOWN), value 0 Event: time 1459279602.606847, -------------- EV_SYN ------------ Event: time 1459279604.494807, type 1 (EV_KEY), code 113 (KEY_MUTE), value 1 Event: time 1459279604.494807, -------------- EV_SYN ------------ Event: time 1459279604.494845, type 1 (EV_KEY), code 113 (KEY_MUTE), value 0 Event: time 1459279604.494845, -------------- EV_SYN ------------ Event: time 1459279605.646785, type 1 (EV_KEY), code 248 (?), value 1 Event: time 1459279605.646785, -------------- EV_SYN ------------ Event: time 1459279605.646804, type 1 (EV_KEY), code 248 (?), value 0 Event: time 1459279605.646804, -------------- EV_SYN ------------ Code 113 is the playback mute button, code 248 is the mic mute button. Regards, Karsten > --- > > The CM109 datasheet states that bit 0 and 1 of HID_IR0 indicate if > volume up / down have been pressed (1) or released (0). Bits 2 and 3 > indicate a press-n-release for playback / record mute, there is no way > to determine when the mute buttons were released. > > I contacted Alfred E. Heggestad, the original author of this driver, > but he understandably couldn't comment on this issue as he didn't work > with the CM109 for quite some time. I cannot test this patch with the > original USB phones the CM109 driver was intended for, I don't own one of > those and the CM109 ones (at least those mentioned in the driver) have > become harder to obtain, but I'd be very surprised if this patch didn't > also work with those. > > drivers/input/misc/cm109.c | 69 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 45 insertions(+), 24 deletions(-) > > diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c > index 9365535..e2c1a80 100644 > --- a/drivers/input/misc/cm109.c > +++ b/drivers/input/misc/cm109.c > @@ -76,8 +76,8 @@ enum { > > BUZZER_ON = 1 << 5, > > - /* up to 256 normal keys, up to 16 special keys */ > - KEYMAP_SIZE = 256 + 16, > + /* up to 256 keys on the normal keymap */ > + KEYMAP_SIZE = 256, > }; > > /* CM109 protocol packet */ > @@ -129,25 +129,14 @@ struct cm109_dev { > int key_code; /* last reported key */ > int keybit; /* 0=new scan 1,2,4,8=scan columns */ > u8 gpi; /* Cached value of GPI (high nibble) */ > + bool volup_cached; /* Cached state of volume up button */ > + bool voldown_cached; /* Cached state of volume down button */ > }; > > /****************************************************************************** > * CM109 key interface > *****************************************************************************/ > > -static unsigned short special_keymap(int code) > -{ > - if (code > 0xff) { > - switch (code - 0xff) { > - case RECORD_MUTE: return KEY_MUTE; > - case PLAYBACK_MUTE: return KEY_MUTE; > - case VOLUME_DOWN: return KEY_VOLUMEDOWN; > - case VOLUME_UP: return KEY_VOLUMEUP; > - } > - } > - return KEY_RESERVED; > -} > - > /* Map device buttons to internal key events. > * > * The "up" and "down" keys, are symbolised by arrows on the button. > @@ -191,7 +180,7 @@ static unsigned short keymap_kip1000(int scancode) > case 0x48: return KEY_ESC; /* hangup */ > case 0x28: return KEY_LEFT; /* IN */ > case 0x18: return KEY_RIGHT; /* OUT */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -224,7 +213,7 @@ static unsigned short keymap_gtalk(int scancode) > case 0x28: return KEY_ESC; /* End (red handset) */ > case 0x48: return KEY_UP; /* Menu up (rocker switch) */ > case 0x88: return KEY_DOWN; /* Menu down (rocker switch) */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -253,7 +242,7 @@ static unsigned short keymap_usbph01(int scancode) > case 0x28: return KEY_ESC; /* hangup */ > case 0x48: return KEY_LEFT; /* IN */ > case 0x88: return KEY_RIGHT; /* OUT */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -284,7 +273,7 @@ static unsigned short keymap_atcom(int scancode) > case 0x28: return KEY_ESC; /* hangup */ > case 0x48: return KEY_LEFT; /* left arrow */ > case 0x88: return KEY_RIGHT; /* right arrow */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -338,9 +327,13 @@ static void cm109_submit_buzz_toggle(struct cm109_dev *dev) > static void cm109_urb_irq_callback(struct urb *urb) > { > struct cm109_dev *dev = urb->context; > + struct input_dev *idev = dev->idev; > const int status = urb->status; > int error; > > + bool volup_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_UP); > + bool voldown_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_DOWN); > + > dev_dbg(&dev->intf->dev, "### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x\n", > dev->irq_data->byte[0], > dev->irq_data->byte[1], > @@ -356,13 +349,35 @@ static void cm109_urb_irq_callback(struct urb *urb) > goto out; > } > > - /* Special keys */ > - if (dev->irq_data->byte[HID_IR0] & 0x0f) { > - const int code = (dev->irq_data->byte[HID_IR0] & 0x0f); > - report_key(dev, dev->keymap[0xff + code]); > + /* Report volume up / down button changes */ > + if (volup_pressed != dev->volup_cached) { > + input_report_key(idev, KEY_VOLUMEUP, volup_pressed); > + input_sync(idev); > + dev->volup_cached = volup_pressed; > } > > - /* Scan key column */ > + if (voldown_pressed != dev->voldown_cached) { > + input_report_key(idev, KEY_VOLUMEDOWN, voldown_pressed); > + input_sync(idev); > + dev->voldown_cached = voldown_pressed; > + } > + > + /* Playback / record mute buttons: simulate press-n-release */ > + if (dev->irq_data->byte[HID_IR0] & PLAYBACK_MUTE) { > + input_report_key(idev, KEY_MUTE, 1); > + input_sync(idev); > + input_report_key(idev, KEY_MUTE, 0); > + input_sync(idev); > + } > + > + if (dev->irq_data->byte[HID_IR0] & RECORD_MUTE) { > + input_report_key(idev, KEY_MICMUTE, 1); > + input_sync(idev); > + input_report_key(idev, KEY_MICMUTE, 0); > + input_sync(idev); > + } > + > + /* Normal keymap: Scan key column */ > if (dev->keybit == 0xf) { > > /* Any changes ? */ > @@ -778,6 +793,12 @@ static int cm109_usb_probe(struct usb_interface *intf, > } > __clear_bit(KEY_RESERVED, input_dev->keybit); > > + /* register available special key events */ > + __set_bit(KEY_VOLUMEUP, input_dev->keybit); > + __set_bit(KEY_VOLUMEDOWN, input_dev->keybit); > + __set_bit(KEY_MUTE, input_dev->keybit); > + __set_bit(KEY_MICMUTE, input_dev->keybit); > + > error = input_register_device(dev->idev); > if (error) > goto err_out; > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Florian, On Sat, Mar 26, 2016 at 09:23:17PM +0100, Florian Euchner wrote: > The CM109 driver reported key press events of volume up / down and > record / playback mute buttons, but release events only as soon as > another button was pressed. Track state of volume buttons in order to > report press and release events properly. For the record and playback > mute buttons, only presses are registered by the CM109, therefore > simulate press-n-release. This fixes the volume control buttons of > various USB headsets. > > Signed-off-by: Florian Euchner <florian.euchner@gmail.com> > --- > > The CM109 datasheet states that bit 0 and 1 of HID_IR0 indicate if > volume up / down have been pressed (1) or released (0). Bits 2 and 3 > indicate a press-n-release for playback / record mute, there is no way > to determine when the mute buttons were released. > > I contacted Alfred E. Heggestad, the original author of this driver, > but he understandably couldn't comment on this issue as he didn't work > with the CM109 for quite some time. I cannot test this patch with the > original USB phones the CM109 driver was intended for, I don't own one of > those and the CM109 ones (at least those mentioned in the driver) have > become harder to obtain, but I'd be very surprised if this patch didn't > also work with those. > > drivers/input/misc/cm109.c | 69 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 45 insertions(+), 24 deletions(-) > > diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c > index 9365535..e2c1a80 100644 > --- a/drivers/input/misc/cm109.c > +++ b/drivers/input/misc/cm109.c > @@ -76,8 +76,8 @@ enum { > > BUZZER_ON = 1 << 5, > > - /* up to 256 normal keys, up to 16 special keys */ > - KEYMAP_SIZE = 256 + 16, > + /* up to 256 keys on the normal keymap */ > + KEYMAP_SIZE = 256, So we are breaking remapping of the "special" keys, why? > }; > > /* CM109 protocol packet */ > @@ -129,25 +129,14 @@ struct cm109_dev { > int key_code; /* last reported key */ > int keybit; /* 0=new scan 1,2,4,8=scan columns */ > u8 gpi; /* Cached value of GPI (high nibble) */ > + bool volup_cached; /* Cached state of volume up button */ > + bool voldown_cached; /* Cached state of volume down button */ > }; > > /****************************************************************************** > * CM109 key interface > *****************************************************************************/ > > -static unsigned short special_keymap(int code) > -{ > - if (code > 0xff) { > - switch (code - 0xff) { > - case RECORD_MUTE: return KEY_MUTE; > - case PLAYBACK_MUTE: return KEY_MUTE; > - case VOLUME_DOWN: return KEY_VOLUMEDOWN; > - case VOLUME_UP: return KEY_VOLUMEUP; > - } > - } > - return KEY_RESERVED; > -} > - > /* Map device buttons to internal key events. > * > * The "up" and "down" keys, are symbolised by arrows on the button. > @@ -191,7 +180,7 @@ static unsigned short keymap_kip1000(int scancode) > case 0x48: return KEY_ESC; /* hangup */ > case 0x28: return KEY_LEFT; /* IN */ > case 0x18: return KEY_RIGHT; /* OUT */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -224,7 +213,7 @@ static unsigned short keymap_gtalk(int scancode) > case 0x28: return KEY_ESC; /* End (red handset) */ > case 0x48: return KEY_UP; /* Menu up (rocker switch) */ > case 0x88: return KEY_DOWN; /* Menu down (rocker switch) */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -253,7 +242,7 @@ static unsigned short keymap_usbph01(int scancode) > case 0x28: return KEY_ESC; /* hangup */ > case 0x48: return KEY_LEFT; /* IN */ > case 0x88: return KEY_RIGHT; /* OUT */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -284,7 +273,7 @@ static unsigned short keymap_atcom(int scancode) > case 0x28: return KEY_ESC; /* hangup */ > case 0x48: return KEY_LEFT; /* left arrow */ > case 0x88: return KEY_RIGHT; /* right arrow */ > - default: return special_keymap(scancode); > + default: return KEY_RESERVED; > } > } > > @@ -338,9 +327,13 @@ static void cm109_submit_buzz_toggle(struct cm109_dev *dev) > static void cm109_urb_irq_callback(struct urb *urb) > { > struct cm109_dev *dev = urb->context; > + struct input_dev *idev = dev->idev; > const int status = urb->status; > int error; > > + bool volup_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_UP); > + bool voldown_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_DOWN); > + > dev_dbg(&dev->intf->dev, "### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x\n", > dev->irq_data->byte[0], > dev->irq_data->byte[1], > @@ -356,13 +349,35 @@ static void cm109_urb_irq_callback(struct urb *urb) > goto out; > } > > - /* Special keys */ > - if (dev->irq_data->byte[HID_IR0] & 0x0f) { > - const int code = (dev->irq_data->byte[HID_IR0] & 0x0f); > - report_key(dev, dev->keymap[0xff + code]); > + /* Report volume up / down button changes */ > + if (volup_pressed != dev->volup_cached) { > + input_report_key(idev, KEY_VOLUMEUP, volup_pressed); > + input_sync(idev); > + dev->volup_cached = volup_pressed; I am not sure why we do this. Why can't we have: static void cm109_report_special(struct cm109_dev *dev) { static const u8 autorelease = RECORD_MUTE | PLAYBACK_MUTE; struct input_dev *idev = dev->idev; u8 data = dev->irq_data->byte[HID_IR0]; unsigned short keycode; int i; for (i = 0; i < 8; i++) { keycode = dev->keymap[0xff + BIT(i)]); if (keycode == KEY_RESERVED) continue; input_report_key(idev, keycode, data & BIT(i)); if (data & autorelease & BIT(i)) { input_sync(idev); input_report_key(idev, keycode, 0); } } input_sync(idev); } Thanks.
diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c index 9365535..e2c1a80 100644 --- a/drivers/input/misc/cm109.c +++ b/drivers/input/misc/cm109.c @@ -76,8 +76,8 @@ enum { BUZZER_ON = 1 << 5, - /* up to 256 normal keys, up to 16 special keys */ - KEYMAP_SIZE = 256 + 16, + /* up to 256 keys on the normal keymap */ + KEYMAP_SIZE = 256, }; /* CM109 protocol packet */ @@ -129,25 +129,14 @@ struct cm109_dev { int key_code; /* last reported key */ int keybit; /* 0=new scan 1,2,4,8=scan columns */ u8 gpi; /* Cached value of GPI (high nibble) */ + bool volup_cached; /* Cached state of volume up button */ + bool voldown_cached; /* Cached state of volume down button */ }; /****************************************************************************** * CM109 key interface *****************************************************************************/ -static unsigned short special_keymap(int code) -{ - if (code > 0xff) { - switch (code - 0xff) { - case RECORD_MUTE: return KEY_MUTE; - case PLAYBACK_MUTE: return KEY_MUTE; - case VOLUME_DOWN: return KEY_VOLUMEDOWN; - case VOLUME_UP: return KEY_VOLUMEUP; - } - } - return KEY_RESERVED; -} - /* Map device buttons to internal key events. * * The "up" and "down" keys, are symbolised by arrows on the button. @@ -191,7 +180,7 @@ static unsigned short keymap_kip1000(int scancode) case 0x48: return KEY_ESC; /* hangup */ case 0x28: return KEY_LEFT; /* IN */ case 0x18: return KEY_RIGHT; /* OUT */ - default: return special_keymap(scancode); + default: return KEY_RESERVED; } } @@ -224,7 +213,7 @@ static unsigned short keymap_gtalk(int scancode) case 0x28: return KEY_ESC; /* End (red handset) */ case 0x48: return KEY_UP; /* Menu up (rocker switch) */ case 0x88: return KEY_DOWN; /* Menu down (rocker switch) */ - default: return special_keymap(scancode); + default: return KEY_RESERVED; } } @@ -253,7 +242,7 @@ static unsigned short keymap_usbph01(int scancode) case 0x28: return KEY_ESC; /* hangup */ case 0x48: return KEY_LEFT; /* IN */ case 0x88: return KEY_RIGHT; /* OUT */ - default: return special_keymap(scancode); + default: return KEY_RESERVED; } } @@ -284,7 +273,7 @@ static unsigned short keymap_atcom(int scancode) case 0x28: return KEY_ESC; /* hangup */ case 0x48: return KEY_LEFT; /* left arrow */ case 0x88: return KEY_RIGHT; /* right arrow */ - default: return special_keymap(scancode); + default: return KEY_RESERVED; } } @@ -338,9 +327,13 @@ static void cm109_submit_buzz_toggle(struct cm109_dev *dev) static void cm109_urb_irq_callback(struct urb *urb) { struct cm109_dev *dev = urb->context; + struct input_dev *idev = dev->idev; const int status = urb->status; int error; + bool volup_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_UP); + bool voldown_pressed = !!(dev->irq_data->byte[HID_IR0] & VOLUME_DOWN); + dev_dbg(&dev->intf->dev, "### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x\n", dev->irq_data->byte[0], dev->irq_data->byte[1], @@ -356,13 +349,35 @@ static void cm109_urb_irq_callback(struct urb *urb) goto out; } - /* Special keys */ - if (dev->irq_data->byte[HID_IR0] & 0x0f) { - const int code = (dev->irq_data->byte[HID_IR0] & 0x0f); - report_key(dev, dev->keymap[0xff + code]); + /* Report volume up / down button changes */ + if (volup_pressed != dev->volup_cached) { + input_report_key(idev, KEY_VOLUMEUP, volup_pressed); + input_sync(idev); + dev->volup_cached = volup_pressed; } - /* Scan key column */ + if (voldown_pressed != dev->voldown_cached) { + input_report_key(idev, KEY_VOLUMEDOWN, voldown_pressed); + input_sync(idev); + dev->voldown_cached = voldown_pressed; + } + + /* Playback / record mute buttons: simulate press-n-release */ + if (dev->irq_data->byte[HID_IR0] & PLAYBACK_MUTE) { + input_report_key(idev, KEY_MUTE, 1); + input_sync(idev); + input_report_key(idev, KEY_MUTE, 0); + input_sync(idev); + } + + if (dev->irq_data->byte[HID_IR0] & RECORD_MUTE) { + input_report_key(idev, KEY_MICMUTE, 1); + input_sync(idev); + input_report_key(idev, KEY_MICMUTE, 0); + input_sync(idev); + } + + /* Normal keymap: Scan key column */ if (dev->keybit == 0xf) { /* Any changes ? */ @@ -778,6 +793,12 @@ static int cm109_usb_probe(struct usb_interface *intf, } __clear_bit(KEY_RESERVED, input_dev->keybit); + /* register available special key events */ + __set_bit(KEY_VOLUMEUP, input_dev->keybit); + __set_bit(KEY_VOLUMEDOWN, input_dev->keybit); + __set_bit(KEY_MUTE, input_dev->keybit); + __set_bit(KEY_MICMUTE, input_dev->keybit); + error = input_register_device(dev->idev); if (error) goto err_out;
The CM109 driver reported key press events of volume up / down and record / playback mute buttons, but release events only as soon as another button was pressed. Track state of volume buttons in order to report press and release events properly. For the record and playback mute buttons, only presses are registered by the CM109, therefore simulate press-n-release. This fixes the volume control buttons of various USB headsets. Signed-off-by: Florian Euchner <florian.euchner@gmail.com> --- The CM109 datasheet states that bit 0 and 1 of HID_IR0 indicate if volume up / down have been pressed (1) or released (0). Bits 2 and 3 indicate a press-n-release for playback / record mute, there is no way to determine when the mute buttons were released. I contacted Alfred E. Heggestad, the original author of this driver, but he understandably couldn't comment on this issue as he didn't work with the CM109 for quite some time. I cannot test this patch with the original USB phones the CM109 driver was intended for, I don't own one of those and the CM109 ones (at least those mentioned in the driver) have become harder to obtain, but I'd be very surprised if this patch didn't also work with those. drivers/input/misc/cm109.c | 69 ++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 24 deletions(-)