From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37814) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9gmQ-0001Of-0n for qemu-devel@nongnu.org; Thu, 17 Dec 2015 17:12:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9gmO-0005wa-Uf for qemu-devel@nongnu.org; Thu, 17 Dec 2015 17:12:09 -0500 Received: from mail-vk0-x236.google.com ([2607:f8b0:400c:c05::236]:32809) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9gmO-0005wP-Oj for qemu-devel@nongnu.org; Thu, 17 Dec 2015 17:12:08 -0500 Received: by mail-vk0-x236.google.com with SMTP id a188so55710766vkc.0 for ; Thu, 17 Dec 2015 14:12:08 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1449851831-4966-1-git-send-email-peter.maydell@linaro.org> <1449851831-4966-3-git-send-email-peter.maydell@linaro.org> From: Peter Maydell Date: Thu, 17 Dec 2015 22:11:48 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis Cc: Patch Tracking , Peter Crosthwaite , Markus Armbruster , "qemu-devel@nongnu.org Developers" , Kevin O'Connor , qemu-arm , "Edgar E. Iglesias" , Paolo Bonzini On 17 December 2015 at 21:45, Alistair Francis wrote: > On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell wrote: >> Turn the SD card into a QOM device. >> This conversion only changes the device itself; the various >> functions which are effectively methods on the device are not >> touched at this point. >> >> Signed-off-by: Peter Maydell >> @@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = { >> } >> }; >> >> -/* We do not model the chip select pin, so allow the board to select >> - whether card should be in SSI or MMC/SD mode. It is also up to the >> - board to ensure that ssi transfers only occur when the chip select >> - is asserted. */ >> +/* Legacy initialization function for use by non-qdevified callers */ >> SDState *sd_init(BlockBackend *blk, bool is_spi) >> { >> - SDState *sd; >> + DeviceState *dev; >> + Error *err = NULL; >> >> - if (blk && blk_is_read_only(blk)) { >> - fprintf(stderr, "sd_init: Cannot use read-only drive\n"); >> + dev = qdev_create(NULL, TYPE_SD); >> + qdev_prop_set_drive(dev, "drive", blk, &err); >> + if (err) { >> + error_report("sd_init failed: %s", error_get_pretty(err)); >> return NULL; >> } >> - >> - sd = (SDState *) g_malloc0(sizeof(SDState)); >> - sd->buf = blk_blockalign(blk, 512); >> - sd->spi = is_spi; >> - sd->enable = true; >> - sd->blk = blk; >> - sd_reset(sd); >> - if (sd->blk) { >> - /* Attach dev if not already attached. (This call ignores an >> - * error return code if sd->blk is already attached.) */ >> - /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */ >> - blk_attach_dev(sd->blk, sd); > > This functionality is removed with this patch. If the block is not > already attached won't this cause errors? The block backend is attached when we set the "drive" property (which we do in this function in the new code just above). [the actual call to blk_attach_dev() is in parse_drive() in hw/core/qdev-properties-system.c.] The blk_set_dev_ops() moves in this patch to sd_realize(). There is incidentally no longer any case where the block backend could be already attached when we call sd_init(), because the only case there was when it had been attached to the sdhci controller device because of the x-drive property on that device, and we removed the property in the previous patch. It's exactly because setting a drive property does an implicit blk_attach_dev that this code previously had to have special casing for "attach of an already attached blk". >> - blk_set_dev_ops(sd->blk, &sd_block_ops, sd); >> + qdev_prop_set_bit(dev, "spi", is_spi); >> + object_property_set_bool(OBJECT(dev), true, "realized", &err); >> + if (err) { >> + error_report("sd_init failed: %s", error_get_pretty(err)); >> + return NULL; >> } >> - vmstate_register(NULL, -1, &sd_vmstate, sd); >> - return sd; >> + >> + return SD(dev); >> } >> >> void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert) >> @@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable) >> { >> sd->enable = enable; >> } >> + >> +static void sd_instance_init(Object *obj) >> +{ >> + SDState *sd = SD(obj); >> + >> + sd->enable = true; >> +} >> + >> +static void sd_realize(DeviceState *dev, Error ** errp) > > You have one too many spaces here. Yep, will fix. thanks -- PMM