WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [patch] pvfb: Split mouse and keyboard into separate dev

To: Gerd Hoffmann <kraxel@xxxxxxx>
Subject: [Xen-devel] Re: [patch] pvfb: Split mouse and keyboard into separate devices.
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Thu, 01 Feb 2007 14:15:09 +0100
Cc: Xen devel list <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 01 Feb 2007 05:14:54 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <45C1C7FC.8080208@xxxxxxx> (Gerd Hoffmann's message of "Thu, 01 Feb 2007 11:59:08 +0100")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <45C1C7FC.8080208@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux)
Gerd Hoffmann <kraxel@xxxxxxx> 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@xxxxxxx>
[...]
> ---
>  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;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel