Re: [patch] pvfb: Split mouse and keyboard into separate devices.

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@suse.de> writes:

>   Hi,
>
> This patch creates two separate input devices for keyboard and mouse
> events.  The reason for this is to separate them in the linux input
> layer and allow them being routed different ways.
>
> Use case:  Configure the X-Server like this to get the mouse
> events directly from the linux input layer, which has the major
> advantage that absolute coordinates work correctly:
>
> Section "InputDevice"
>   Driver       "evdev"
>   Identifier   "Mouse[1]"
>   Option       "Device" "/dev/input/event<nr>"
> EndSection
>
> This makes the keyboard stop working though in case mouse and
> keyboard events are coming through the same input device.

Review below.

I'll leave judging whether this is a good idea to those who know more
about X than I do.

> please apply,
>   Gerd
>
> -- 
> Gerd Hoffmann <kraxel@suse.de>
[...]
> ---
>  linux-2.6-xen-sparse/drivers/xen/fbfront/xenkbd.c |   60 +++++++++++++---------
>  1 file changed, 38 insertions(+), 22 deletions(-)
>
> Index: build-64-unstable-13762/linux-2.6-xen-sparse/drivers/xen/fbfront/xenkbd.c
> ===================================================================
> --- build-64-unstable-13762.orig/linux-2.6-xen-sparse/drivers/xen/fbfront/xenkbd.c
> +++ build-64-unstable-13762/linux-2.6-xen-sparse/drivers/xen/fbfront/xenkbd.c
> @@ -29,7 +29,8 @@
>  
>  struct xenkbd_info
>  {
> -	struct input_dev *dev;
> +	struct input_dev *kbd;
> +	struct input_dev *ptr;
>  	struct xenkbd_page *page;
>  	int irq;
>  	struct xenbus_device *xbdev;
> @@ -60,19 +61,21 @@ static irqreturn_t input_handler(int rq,
>  
>  		switch (event->type) {
>  		case XENKBD_TYPE_MOTION:
> -			input_report_rel(info->dev, REL_X, event->motion.rel_x);
> -			input_report_rel(info->dev, REL_Y, event->motion.rel_y);
> +			input_report_rel(info->ptr, REL_X, event->motion.rel_x);
> +			input_report_rel(info->ptr, REL_Y, event->motion.rel_y);
> +			input_sync(info->ptr);
>  			break;
>  		case XENKBD_TYPE_KEY:
> -			input_report_key(info->dev, event->key.keycode, event->key.pressed);
> +			input_report_key(info->kbd, event->key.keycode, event->key.pressed);
> +			input_sync(info->kbd);
>  			break;
>  		case XENKBD_TYPE_POS:
> -			input_report_abs(info->dev, ABS_X, event->pos.abs_x);
> -			input_report_abs(info->dev, ABS_Y, event->pos.abs_y);
> +			input_report_abs(info->ptr, ABS_X, event->pos.abs_x);
> +			input_report_abs(info->ptr, ABS_Y, event->pos.abs_y);
> +			input_sync(info->ptr);
>  			break;
>  		}
>  	}
> -	input_sync(info->dev);
>  	mb();			/* ensure we got ring contents */
>  	page->in_cons = cons;
>  	notify_remote_via_irq(info->irq);
> @@ -85,7 +88,7 @@ int __devinit xenkbd_probe(struct xenbus
>  {
>  	int ret, i;
>  	struct xenkbd_info *info;
> -	struct input_dev *input_dev;
> +	struct input_dev *kbd, *ptr;
>  
>  	info = kzalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info) {
> @@ -101,32 +104,44 @@ int __devinit xenkbd_probe(struct xenbus
>  	info->page->in_cons = info->page->in_prod = 0;
>  	info->page->out_cons = info->page->out_prod = 0;
>  
> -	input_dev = input_allocate_device();
> -	if (!input_dev)
> +	kbd = input_allocate_device();
> +	ptr = input_allocate_device();
> +	if (!kbd || !ptr)
>  		goto error_nomem;

If just one of two fails, the other is leaked, I fear.

>  
> -	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
> -	input_dev->keybit[LONG(BTN_MOUSE)]
> +	ptr->evbit[0] = BIT(EV_REL) | BIT(EV_ABS);
> +	ptr->keybit[LONG(BTN_MOUSE)]
>  		= BIT(BTN_LEFT) | BIT(BTN_MIDDLE) | BIT(BTN_RIGHT);
>  	/* TODO additional buttons */
> -	input_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y);
> +	ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y);
>  
> +	kbd->evbit[0] = BIT(EV_KEY);
>  	/* FIXME not sure this is quite right */
>  	for (i = 0; i < 256; i++)
> -		set_bit(i, input_dev->keybit);
> +		set_bit(i, kbd->keybit);
>  
> -	input_dev->name = "Xen Virtual Keyboard/Mouse";
> +	kbd->name = "Xen Virtual Keyboard";
> +	ptr->name = "Xen Virtual Touchscreen";

Touchscreen is going to confuse people.  I'd rather call it a mouse.
Or pointing device, if need be.

>  
> -	input_set_abs_params(input_dev, ABS_X, 0, XENFB_WIDTH, 0, 0);
> -	input_set_abs_params(input_dev, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
> +	input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
> +	input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
>  
> -	ret = input_register_device(input_dev);
> +	ret = input_register_device(kbd);
>  	if (ret) {
> -		input_free_device(input_dev);
> -		xenbus_dev_fatal(dev, ret, "input_register_device");
> +		input_free_device(kbd);
> +		input_free_device(ptr);
> +		xenbus_dev_fatal(dev, ret, "input_register_device(kbd)");
>  		goto error;
>  	}
> -	info->dev = input_dev;
> +	info->kbd = kbd;
> +
> +	ret = input_register_device(ptr);
> +	if (ret) {
> +		input_free_device(ptr);
> +		xenbus_dev_fatal(dev, ret, "input_register_device(ptr)");
> +		goto error;
> +	}
> +	info->ptr = ptr;
>  
>  	ret = xenkbd_connect_backend(dev, info);
>  	if (ret < 0)


It *might* be simpler to have something like

 error:
        if (ptr)
        	input_free_device(ptr);
        if (kbd)
        	input_free_device(kbd);

> @@ -155,7 +170,8 @@ static int xenkbd_remove(struct xenbus_d
>  	struct xenkbd_info *info = dev->dev.driver_data;
>  
>  	xenkbd_disconnect_backend(info);
> -	input_unregister_device(info->dev);
> +	input_unregister_device(info->kbd);
> +	input_unregister_device(info->ptr);
>  	free_page((unsigned long)info->page);
>  	kfree(info);
>  	return 0;