All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn
@ 2014-09-25 21:38 Jan Vesely
  2014-09-25 21:38 ` [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration Jan Vesely
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jan Vesely @ 2014-09-25 21:38 UTC (permalink / raw)
  To: QEMU; +Cc: Gerd Hoffmann

v2: rebase

Signed-off-by: Jan Vesely <jano.vesely@gmail.com>
---
 hw/usb/dev-hid.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 467ec86..90746ed 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -566,9 +566,23 @@ static void usb_hid_handle_destroy(USBDevice *dev)
     hid_free(&us->hid);
 }
 
-static void usb_hid_initfn(USBDevice *dev, int kind)
+static void usb_hid_initfn(USBDevice *dev, int kind,
+                           const USBDesc *usb1, const USBDesc *usb2,
+                           Error **errp)
 {
     USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
+    switch (us->usb_version) {
+    case 1:
+        dev->usb_desc = usb1;
+        break;
+    case 2:
+        dev->usb_desc = usb2;
+        break;
+    default:
+        error_setg(errp, "Invalid usb version %d for usb hid device"
+                   "(must be 1 or 2)", us->usb_version);
+        return;
+    }
 
     if (dev->serial) {
         usb_desc_set_string(dev, STR_SERIALNUMBER, dev->serial);
@@ -583,32 +597,18 @@ static void usb_hid_initfn(USBDevice *dev, int kind)
 
 static void usb_tablet_realize(USBDevice *dev, Error **errp)
 {
-    USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
-
-    switch (us->usb_version) {
-    case 1:
-        dev->usb_desc = &desc_tablet;
-        break;
-    case 2:
-        dev->usb_desc = &desc_tablet2;
-        break;
-    default:
-        error_setg(errp, "Invalid usb version %d for usb-tablet "
-                   "(must be 1 or 2)", us->usb_version);
-        return;
-    }
 
-    usb_hid_initfn(dev, HID_TABLET);
+    usb_hid_initfn(dev, HID_TABLET, &desc_tablet, &desc_tablet2, errp);
 }
 
 static void usb_mouse_realize(USBDevice *dev, Error **errp)
 {
-    usb_hid_initfn(dev, HID_MOUSE);
+    usb_hid_initfn(dev, HID_MOUSE, &desc_mouse, &desc_mouse, errp);
 }
 
 static void usb_keyboard_realize(USBDevice *dev, Error **errp)
 {
-    usb_hid_initfn(dev, HID_KEYBOARD);
+    usb_hid_initfn(dev, HID_KEYBOARD, &desc_keyboard, &desc_keyboard, errp);
 }
 
 static int usb_ptr_post_load(void *opaque, int version_id)
@@ -690,7 +690,6 @@ static void usb_mouse_class_initfn(ObjectClass *klass, void *data)
     usb_hid_class_initfn(klass, data);
     uc->realize        = usb_mouse_realize;
     uc->product_desc   = "QEMU USB Mouse";
-    uc->usb_desc       = &desc_mouse;
     dc->vmsd = &vmstate_usb_ptr;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
@@ -715,7 +714,6 @@ static void usb_keyboard_class_initfn(ObjectClass *klass, void *data)
     usb_hid_class_initfn(klass, data);
     uc->realize        = usb_keyboard_realize;
     uc->product_desc   = "QEMU USB Keyboard";
-    uc->usb_desc       = &desc_keyboard;
     dc->vmsd = &vmstate_usb_kbd;
     dc->props = usb_keyboard_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration
  2014-09-25 21:38 [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Jan Vesely
@ 2014-09-25 21:38 ` Jan Vesely
  2014-09-26  7:06   ` Gerd Hoffmann
  2014-09-25 21:38 ` [Qemu-devel] [PATCH v2 3/3] usb-hid: Add high speed keyboard configuration Jan Vesely
  2014-09-26  6:21 ` [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Gonglei (Arei)
  2 siblings, 1 reply; 9+ messages in thread
From: Jan Vesely @ 2014-09-25 21:38 UTC (permalink / raw)
  To: QEMU; +Cc: Gerd Hoffmann

v2: add usb_mouse_properties
    use macros for bmAttributes
v3: rebase
v4: rebase

Signed-off-by: Jan Vesely <jano.vesely@gmail.com>
---
 hw/usb/dev-hid.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 90746ed..a946528 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -104,6 +104,37 @@ static const USBDescIface desc_iface_mouse = {
     },
 };
 
+static const USBDescIface desc_iface_mouse2 = {
+    .bInterfaceNumber              = 0,
+    .bNumEndpoints                 = 1,
+    .bInterfaceClass               = USB_CLASS_HID,
+    .bInterfaceSubClass            = 0x01, /* boot */
+    .bInterfaceProtocol            = 0x02,
+    .ndesc                         = 1,
+    .descs = (USBDescOther[]) {
+        {
+            /* HID descriptor */
+            .data = (uint8_t[]) {
+                0x09,          /*  u8  bLength */
+                USB_DT_HID,    /*  u8  bDescriptorType */
+                0x01, 0x00,    /*  u16 HID_class */
+                0x00,          /*  u8  country_code */
+                0x01,          /*  u8  num_descriptors */
+                USB_DT_REPORT, /*  u8  type: Report */
+                52, 0,         /*  u16 len */
+            },
+        },
+    },
+    .eps = (USBDescEndpoint[]) {
+        {
+            .bEndpointAddress      = USB_DIR_IN | 0x01,
+            .bmAttributes          = USB_ENDPOINT_XFER_INT,
+            .wMaxPacketSize        = 4,
+            .bInterval             = 7, /* 2 ^ (8-1) * 125 usecs = 8 ms */
+        },
+    },
+};
+
 static const USBDescIface desc_iface_tablet = {
     .bInterfaceNumber              = 0,
     .bNumEndpoints                 = 1,
@@ -212,6 +243,23 @@ static const USBDescDevice desc_device_mouse = {
     },
 };
 
+static const USBDescDevice desc_device_mouse2 = {
+    .bcdUSB                        = 0x0200,
+    .bMaxPacketSize0               = 64,
+    .bNumConfigurations            = 1,
+    .confs = (USBDescConfig[]) {
+        {
+            .bNumInterfaces        = 1,
+            .bConfigurationValue   = 1,
+            .iConfiguration        = STR_CONFIG_MOUSE,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
+            .bMaxPower             = 50,
+            .nif = 1,
+            .ifs = &desc_iface_mouse2,
+        },
+    },
+};
+
 static const USBDescDevice desc_device_tablet = {
     .bcdUSB                        = 0x0100,
     .bMaxPacketSize0               = 8,
@@ -281,6 +329,21 @@ static const USBDesc desc_mouse = {
     .msos = &desc_msos_suspend,
 };
 
+static const USBDesc desc_mouse2 = {
+    .id = {
+        .idVendor          = 0x0627,
+        .idProduct         = 0x0001,
+        .bcdDevice         = 0,
+        .iManufacturer     = STR_MANUFACTURER,
+        .iProduct          = STR_PRODUCT_MOUSE,
+        .iSerialNumber     = STR_SERIALNUMBER,
+    },
+    .full = &desc_device_mouse,
+    .high = &desc_device_mouse2,
+    .str  = desc_strings,
+    .msos = &desc_msos_suspend,
+};
+
 static const USBDesc desc_tablet = {
     .id = {
         .idVendor          = 0x0627,
@@ -603,7 +666,7 @@ static void usb_tablet_realize(USBDevice *dev, Error **errp)
 
 static void usb_mouse_realize(USBDevice *dev, Error **errp)
 {
-    usb_hid_initfn(dev, HID_MOUSE, &desc_mouse, &desc_mouse, errp);
+    usb_hid_initfn(dev, HID_MOUSE, &desc_mouse, &desc_mouse2, errp);
 }
 
 static void usb_keyboard_realize(USBDevice *dev, Error **errp)
@@ -682,6 +745,11 @@ static const TypeInfo usb_tablet_info = {
     .class_init    = usb_tablet_class_initfn,
 };
 
+static Property usb_mouse_properties[] = {
+        DEFINE_PROP_UINT32("usb_version", USBHIDState, usb_version, 2),
+        DEFINE_PROP_END_OF_LIST(),
+};
+
 static void usb_mouse_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -691,6 +759,7 @@ static void usb_mouse_class_initfn(ObjectClass *klass, void *data)
     uc->realize        = usb_mouse_realize;
     uc->product_desc   = "QEMU USB Mouse";
     dc->vmsd = &vmstate_usb_ptr;
+    dc->props = usb_mouse_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] usb-hid: Add high speed keyboard configuration
  2014-09-25 21:38 [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Jan Vesely
  2014-09-25 21:38 ` [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration Jan Vesely
@ 2014-09-25 21:38 ` Jan Vesely
  2014-09-26  6:21 ` [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Gonglei (Arei)
  2 siblings, 0 replies; 9+ messages in thread
From: Jan Vesely @ 2014-09-25 21:38 UTC (permalink / raw)
  To: QEMU; +Cc: Gerd Hoffmann

v2: rebase

Signed-off-by: Jan Vesely <jano.vesely@gmail.com>
---
 hw/usb/dev-hid.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index a946528..7b67489 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -226,6 +226,37 @@ static const USBDescIface desc_iface_keyboard = {
     },
 };
 
+static const USBDescIface desc_iface_keyboard2 = {
+    .bInterfaceNumber              = 0,
+    .bNumEndpoints                 = 1,
+    .bInterfaceClass               = USB_CLASS_HID,
+    .bInterfaceSubClass            = 0x01, /* boot */
+    .bInterfaceProtocol            = 0x01, /* keyboard */
+    .ndesc                         = 1,
+    .descs = (USBDescOther[]) {
+        {
+            /* HID descriptor */
+            .data = (uint8_t[]) {
+                0x09,          /*  u8  bLength */
+                USB_DT_HID,    /*  u8  bDescriptorType */
+                0x11, 0x01,    /*  u16 HID_class */
+                0x00,          /*  u8  country_code */
+                0x01,          /*  u8  num_descriptors */
+                USB_DT_REPORT, /*  u8  type: Report */
+                0x3f, 0,       /*  u16 len */
+            },
+        },
+    },
+    .eps = (USBDescEndpoint[]) {
+        {
+            .bEndpointAddress      = USB_DIR_IN | 0x01,
+            .bmAttributes          = USB_ENDPOINT_XFER_INT,
+            .wMaxPacketSize        = 8,
+            .bInterval             = 7, /* 2 ^ (8-1) * 125 usecs = 8 ms */
+        },
+    },
+};
+
 static const USBDescDevice desc_device_mouse = {
     .bcdUSB                        = 0x0100,
     .bMaxPacketSize0               = 8,
@@ -311,6 +342,23 @@ static const USBDescDevice desc_device_keyboard = {
     },
 };
 
+static const USBDescDevice desc_device_keyboard2 = {
+    .bcdUSB                        = 0x0200,
+    .bMaxPacketSize0               = 64,
+    .bNumConfigurations            = 1,
+    .confs = (USBDescConfig[]) {
+        {
+            .bNumInterfaces        = 1,
+            .bConfigurationValue   = 1,
+            .iConfiguration        = STR_CONFIG_KEYBOARD,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
+            .bMaxPower             = 50,
+            .nif = 1,
+            .ifs = &desc_iface_keyboard2,
+        },
+    },
+};
+
 static const USBDescMSOS desc_msos_suspend = {
     .SelectiveSuspendEnabled = true,
 };
@@ -387,6 +435,21 @@ static const USBDesc desc_keyboard = {
     .msos = &desc_msos_suspend,
 };
 
+static const USBDesc desc_keyboard2 = {
+    .id = {
+        .idVendor          = 0x0627,
+        .idProduct         = 0x0001,
+        .bcdDevice         = 0,
+        .iManufacturer     = STR_MANUFACTURER,
+        .iProduct          = STR_PRODUCT_KEYBOARD,
+        .iSerialNumber     = STR_SERIALNUMBER,
+    },
+    .full = &desc_device_keyboard,
+    .high = &desc_device_keyboard2,
+    .str  = desc_strings,
+    .msos = &desc_msos_suspend,
+};
+
 static const uint8_t qemu_mouse_hid_report_descriptor[] = {
     0x05, 0x01,		/* Usage Page (Generic Desktop) */
     0x09, 0x02,		/* Usage (Mouse) */
@@ -671,7 +734,7 @@ static void usb_mouse_realize(USBDevice *dev, Error **errp)
 
 static void usb_keyboard_realize(USBDevice *dev, Error **errp)
 {
-    usb_hid_initfn(dev, HID_KEYBOARD, &desc_keyboard, &desc_keyboard, errp);
+    usb_hid_initfn(dev, HID_KEYBOARD, &desc_keyboard, &desc_keyboard2, errp);
 }
 
 static int usb_ptr_post_load(void *opaque, int version_id)
@@ -771,6 +834,7 @@ static const TypeInfo usb_mouse_info = {
 };
 
 static Property usb_keyboard_properties[] = {
+        DEFINE_PROP_UINT32("usb_version", USBHIDState, usb_version, 2),
         DEFINE_PROP_STRING("display", USBHIDState, display),
         DEFINE_PROP_END_OF_LIST(),
 };
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn
  2014-09-25 21:38 [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Jan Vesely
  2014-09-25 21:38 ` [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration Jan Vesely
  2014-09-25 21:38 ` [Qemu-devel] [PATCH v2 3/3] usb-hid: Add high speed keyboard configuration Jan Vesely
@ 2014-09-26  6:21 ` Gonglei (Arei)
  2014-09-26  6:53   ` Gerd Hoffmann
  2 siblings, 1 reply; 9+ messages in thread
From: Gonglei (Arei) @ 2014-09-26  6:21 UTC (permalink / raw)
  To: Jan Vesely, QEMU; +Cc: Gerd Hoffmann

> Subject: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to
> usb-hid initfn
> 
> v2: rebase
> 
> Signed-off-by: Jan Vesely <jano.vesely@gmail.com>
> ---
>  hw/usb/dev-hid.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 467ec86..90746ed 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -566,9 +566,23 @@ static void usb_hid_handle_destroy(USBDevice *dev)
>      hid_free(&us->hid);
>  }
> 
> -static void usb_hid_initfn(USBDevice *dev, int kind)
> +static void usb_hid_initfn(USBDevice *dev, int kind,
> +                           const USBDesc *usb1, const USBDesc
> *usb2,
> +                           Error **errp)

I don't think it is a good idea that adding usb version as parameters.
If we want to support usb3.0 tablet, we have to change usb_hid_initfn() too
in the future and the usb-mouse, usb-kbd also need to changed despite
usb-mouse and usb-kbd don't support it yet.

Best regards,
-Gonglei

>  {
>      USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
> +    switch (us->usb_version) {
> +    case 1:
> +        dev->usb_desc = usb1;
> +        break;
> +    case 2:
> +        dev->usb_desc = usb2;
> +        break;
> +    default:
> +        error_setg(errp, "Invalid usb version %d for usb hid device"
> +                   "(must be 1 or 2)", us->usb_version);
> +        return;
> +    }
> 
>      if (dev->serial) {
>          usb_desc_set_string(dev, STR_SERIALNUMBER, dev->serial);
> @@ -583,32 +597,18 @@ static void usb_hid_initfn(USBDevice *dev, int kind)
> 
>  static void usb_tablet_realize(USBDevice *dev, Error **errp)
>  {
> -    USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
> -
> -    switch (us->usb_version) {
> -    case 1:
> -        dev->usb_desc = &desc_tablet;
> -        break;
> -    case 2:
> -        dev->usb_desc = &desc_tablet2;
> -        break;
> -    default:
> -        error_setg(errp, "Invalid usb version %d for usb-tablet "
> -                   "(must be 1 or 2)", us->usb_version);
> -        return;
> -    }
> 
> -    usb_hid_initfn(dev, HID_TABLET);
> +    usb_hid_initfn(dev, HID_TABLET, &desc_tablet, &desc_tablet2, errp);
>  }
> 
>  static void usb_mouse_realize(USBDevice *dev, Error **errp)
>  {
> -    usb_hid_initfn(dev, HID_MOUSE);
> +    usb_hid_initfn(dev, HID_MOUSE, &desc_mouse, &desc_mouse, errp);
>  }
> 
>  static void usb_keyboard_realize(USBDevice *dev, Error **errp)
>  {
> -    usb_hid_initfn(dev, HID_KEYBOARD);
> +    usb_hid_initfn(dev, HID_KEYBOARD, &desc_keyboard, &desc_keyboard,
> errp);
>  }
> 
>  static int usb_ptr_post_load(void *opaque, int version_id)
> @@ -690,7 +690,6 @@ static void usb_mouse_class_initfn(ObjectClass *klass,
> void *data)
>      usb_hid_class_initfn(klass, data);
>      uc->realize        = usb_mouse_realize;
>      uc->product_desc   = "QEMU USB Mouse";
> -    uc->usb_desc       = &desc_mouse;
>      dc->vmsd = &vmstate_usb_ptr;
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
> @@ -715,7 +714,6 @@ static void usb_keyboard_class_initfn(ObjectClass
> *klass, void *data)
>      usb_hid_class_initfn(klass, data);
>      uc->realize        = usb_keyboard_realize;
>      uc->product_desc   = "QEMU USB Keyboard";
> -    uc->usb_desc       = &desc_keyboard;
>      dc->vmsd = &vmstate_usb_kbd;
>      dc->props = usb_keyboard_properties;
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> --
> 1.9.3
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn
  2014-09-26  6:21 ` [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Gonglei (Arei)
@ 2014-09-26  6:53   ` Gerd Hoffmann
  2014-09-26  7:11     ` Gonglei (Arei)
  0 siblings, 1 reply; 9+ messages in thread
From: Gerd Hoffmann @ 2014-09-26  6:53 UTC (permalink / raw)
  To: Gonglei (Arei); +Cc: Jan Vesely, QEMU

  Hi,

> > -static void usb_hid_initfn(USBDevice *dev, int kind)
> > +static void usb_hid_initfn(USBDevice *dev, int kind,
> > +                           const USBDesc *usb1, const USBDesc
> > *usb2,
> > +                           Error **errp)
> 
> I don't think it is a good idea that adding usb version as parameters.
> If we want to support usb3.0 tablet, we have to change usb_hid_initfn() too
> in the future and the usb-mouse, usb-kbd also need to changed despite
> usb-mouse and usb-kbd don't support it yet.

Adding a usb3 parameter should the need arise in the future (unlikely
IMO as xhci can handle all three usb speeds) isn't much of a problem I
think.

Error handling should be improved though:

> >  {
> >      USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
> > +    switch (us->usb_version) {
> > +    case 1:
> > +        dev->usb_desc = usb1;
> > +        break;
> > +    case 2:
> > +        dev->usb_desc = usb2;
> > +        break;
> > +    default:

dev->usb_desc = NULL;
}
if (dev->usb_desc) {

> > +        error_setg(errp, "Invalid usb version %d for usb hid device"
> > +                   "(must be 1 or 2)", us->usb_version);
> > +        return;
> > +    }

This way it'll be possible to pass NULL for usb2 and have usb_hid_initfn
handle it correctly.

> >  static void usb_mouse_realize(USBDevice *dev, Error **errp)
> >  {
> > -    usb_hid_initfn(dev, HID_MOUSE);
> > +    usb_hid_initfn(dev, HID_MOUSE, &desc_mouse, &desc_mouse, errp);

usb_hid_initfn(ev, HID_MOUSE, &desc_mouse, NULL, errp);

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration
  2014-09-25 21:38 ` [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration Jan Vesely
@ 2014-09-26  7:06   ` Gerd Hoffmann
  2014-09-26 14:43     ` Jan Vesely
  0 siblings, 1 reply; 9+ messages in thread
From: Gerd Hoffmann @ 2014-09-26  7:06 UTC (permalink / raw)
  To: Jan Vesely; +Cc: QEMU

On Do, 2014-09-25 at 17:38 -0400, Jan Vesely wrote:
> v2: add usb_mouse_properties
>     use macros for bmAttributes
> v3: rebase
> v4: rebase

patch looks good, the only thing missing is the compat property, to make
sure qemu machine types for 2.1 & older continue to have usb1 mouse+kbd
by default.

The property for the tablet is in hw/i386/pc_piix.c, have a look there
how to do it.  The compat properties for more recent machine types live
in a header file (include/hw/i386/pc.h) so they can be shared between pc
and q35.

Oh, and the patch history should not be in the commit message.  You can
add the history as notes (see 'git notes').  'git format-patch' will use
three dashes to separate the notes and 'git am' will strip them off so
they appear on the list message but not in the commit log.  Adding the
tree dashes separator to the commit message will work too, at least as
long as you don't send pull requests (in which case 'git am' would have
no chance to strip off the notes).

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn
  2014-09-26  6:53   ` Gerd Hoffmann
@ 2014-09-26  7:11     ` Gonglei (Arei)
  0 siblings, 0 replies; 9+ messages in thread
From: Gonglei (Arei) @ 2014-09-26  7:11 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Jan Vesely, QEMU

> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, September 26, 2014 2:53 PM
> To: Gonglei (Arei)
> Cc: Jan Vesely; QEMU
> Subject: Re: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to
> usb-hid initfn
> 
>   Hi,
> 
> > > -static void usb_hid_initfn(USBDevice *dev, int kind)
> > > +static void usb_hid_initfn(USBDevice *dev, int kind,
> > > +                           const USBDesc *usb1, const USBDesc
> > > *usb2,
> > > +                           Error **errp)
> >
> > I don't think it is a good idea that adding usb version as parameters.
> > If we want to support usb3.0 tablet, we have to change usb_hid_initfn() too
> > in the future and the usb-mouse, usb-kbd also need to changed despite
> > usb-mouse and usb-kbd don't support it yet.
> 
> Adding a usb3 parameter should the need arise in the future (unlikely
> IMO as xhci can handle all three usb speeds) isn't much of a problem I
> think.
> 
> Error handling should be improved though:
> 
> > >  {
> > >      USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
> > > +    switch (us->usb_version) {
> > > +    case 1:
> > > +        dev->usb_desc = usb1;
> > > +        break;
> > > +    case 2:
> > > +        dev->usb_desc = usb2;
> > > +        break;
> > > +    default:
> 
> dev->usb_desc = NULL;
> }
> if (dev->usb_desc) {
> 
> > > +        error_setg(errp, "Invalid usb version %d for usb hid device"
> > > +                   "(must be 1 or 2)", us->usb_version);
> > > +        return;
> > > +    }
> 
> This way it'll be possible to pass NULL for usb2 and have usb_hid_initfn
> handle it correctly.
> 
> > >  static void usb_mouse_realize(USBDevice *dev, Error **errp)
> > >  {
> > > -    usb_hid_initfn(dev, HID_MOUSE);
> > > +    usb_hid_initfn(dev, HID_MOUSE, &desc_mouse, &desc_mouse,
> errp);
> 
> usb_hid_initfn(ev, HID_MOUSE, &desc_mouse, NULL, errp);
> 

Sounds good. :)

Best regards,
-Gonglei

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration
  2014-09-26  7:06   ` Gerd Hoffmann
@ 2014-09-26 14:43     ` Jan Vesely
  2014-09-29  7:32       ` Gerd Hoffmann
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Vesely @ 2014-09-26 14:43 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: QEMU

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Fri, 2014-09-26 at 09:06 +0200, Gerd Hoffmann wrote:
> On Do, 2014-09-25 at 17:38 -0400, Jan Vesely wrote:
> > v2: add usb_mouse_properties
> >     use macros for bmAttributes
> > v3: rebase
> > v4: rebase
> 
> patch looks good, the only thing missing is the compat property, to make
> sure qemu machine types for 2.1 & older continue to have usb1 mouse+kbd
> by default.
> 
> The property for the tablet is in hw/i386/pc_piix.c, have a look there
> how to do it.  The compat properties for more recent machine types live
> in a header file (include/hw/i386/pc.h) so they can be shared between pc
> and q35.


I gave mouse and kbd the same treatment as tablet has in
hw/i386/pc_piix.c.
Speaking about properties do I need to add something for display and
head properties?
tablet adds both of them and they seem to be used together, however
keyboard only had display, and I added neither for mouse.


> 
> Oh, and the patch history should not be in the commit message.  You can
> add the history as notes (see 'git notes').  'git format-patch' will use
> three dashes to separate the notes and 'git am' will strip them off so
> they appear on the list message but not in the commit log.  Adding the
> tree dashes separator to the commit message will work too, at least as
> long as you don't send pull requests (in which case 'git am' would have
> no chance to strip off the notes).

OK I'll move these out of commit messages.

thanks,
jan

> 
> cheers,
>   Gerd
> 
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration
  2014-09-26 14:43     ` Jan Vesely
@ 2014-09-29  7:32       ` Gerd Hoffmann
  0 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2014-09-29  7:32 UTC (permalink / raw)
  To: Jan Vesely; +Cc: QEMU

  Hi,

> Speaking about properties do I need to add something for display and
> head properties?

No.

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-09-29  7:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-25 21:38 [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Jan Vesely
2014-09-25 21:38 ` [Qemu-devel] [PATCH v4 2/3] usb-hid: Add high speed mouse configuration Jan Vesely
2014-09-26  7:06   ` Gerd Hoffmann
2014-09-26 14:43     ` Jan Vesely
2014-09-29  7:32       ` Gerd Hoffmann
2014-09-25 21:38 ` [Qemu-devel] [PATCH v2 3/3] usb-hid: Add high speed keyboard configuration Jan Vesely
2014-09-26  6:21 ` [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn Gonglei (Arei)
2014-09-26  6:53   ` Gerd Hoffmann
2014-09-26  7:11     ` Gonglei (Arei)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.