From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40639 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PSqO4-0000Lp-4V for qemu-devel@nongnu.org; Wed, 15 Dec 2010 07:23:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PSqO3-0004WC-2z for qemu-devel@nongnu.org; Wed, 15 Dec 2010 07:23:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PSqO2-0004Vy-Rh for qemu-devel@nongnu.org; Wed, 15 Dec 2010 07:23:15 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oBFCNDk8031259 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 15 Dec 2010 07:23:13 -0500 Message-ID: <4D08B32F.1000007@redhat.com> Date: Wed, 15 Dec 2010 13:23:11 +0100 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/5] usb-ccid: add CCID bus References: <1292179059-21617-1-git-send-email-alevy@redhat.com> <1292179059-21617-2-git-send-email-alevy@redhat.com> In-Reply-To: <1292179059-21617-2-git-send-email-alevy@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: qemu-devel@nongnu.org On 12/12/10 19:37, Alon Levy wrote: > A CCID device is a smart card reader. It is a USB device, defined at [1]. > This patch introduces the usb-ccid device that is a ccid bus. Next patches will > introduce two card types to use it, a passthru card and an emulated card. Looks good overall, just some minor nits / questions: > +struct CCIDCardState { > + DeviceState qdev; Add "uint32_t slot" here? Also adding a bus property for it would be good, even though it can't be anything but '0' right now, to prepare the interfaces for the day when we'll support more than a single card slot and thus can attach multiple cards to the ccid bus. > +static VMStateDescription ccid_vmstate = { > + .name = CCID_DEV_NAME, > + .version_id = 1, > + .minimum_version_id = 1, > + .post_load = ccid_post_load, > + .pre_save = ccid_pre_save, > + .fields = (VMStateField []) { > + VMSTATE_STRUCT(dev, USBCCIDState, 1, usb_device_vmstate, USBDevice), Can we please disable this for now? USB migration support doesn't exist, and when we add it chances are that it isn't compatible with what you have here. > +static struct USBDeviceInfo ccid_info = { > + .product_desc = "QEMU USB CCID", > + .qdev.name = CCID_DEV_NAME, > + .qdev.size = sizeof(USBCCIDState), > + .qdev.vmsd =&ccid_vmstate, Best leave the vmstate structs in the code (with a comment) and just zap this line which winds it up. thanks, Gerd