From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52671)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1XXPPD-0005dC-Jo
	for qemu-devel@nongnu.org; Fri, 26 Sep 2014 02:53:33 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1XXPP7-0001MJ-Eb
	for qemu-devel@nongnu.org; Fri, 26 Sep 2014 02:53:27 -0400
Received: from mx1.redhat.com ([209.132.183.28]:43111)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1XXPP7-0001M2-6d
	for qemu-devel@nongnu.org; Fri, 26 Sep 2014 02:53:21 -0400
Message-ID: <1411714384.865.2.camel@nilsson.home.kraxel.org>
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 26 Sep 2014 08:53:04 +0200
In-Reply-To: <33183CC9F5247A488A2544077AF1902086DDBAE0@SZXEMA503-MBS.china.huawei.com>
References: <1411681094-8474-1-git-send-email-jano.vesely@gmail.com>
	<33183CC9F5247A488A2544077AF1902086DDBAE0@SZXEMA503-MBS.china.huawei.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 1/3] usb-hid: Move descriptor decision
 to	usb-hid initfn
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Gonglei (Arei)" <arei.gonglei@huawei.com>
Cc: Jan Vesely <jano.vesely@gmail.com>, QEMU <qemu-devel@nongnu.org>

  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