Re: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision to usb-hid initfn

[Gonglei (Arei)]

> From: Gerd Hoffmann [mailto:address@hidden
> 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