Date: Tue, 29 Mar 2016 21:34:07 +0200
From: Karsten Merker <merker@debian.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Florian Euchner <florian.euchner@gmail.com>
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Input: CM109: Fix handling of volume and mute buttons
Message-ID: <20160329193407.GA2732@excalibur.cnev.de>
References: <1459023797-6418-1-git-send-email-florian.euchner@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1459023797-6418-1-git-send-email-florian.euchner@gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 8556
Lines: 220

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

-- 
Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
Werbung sowie der Markt- oder Meinungsforschung.