From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42780) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qxg9p-00023n-A2 for qemu-devel@nongnu.org; Sun, 28 Aug 2011 10:16:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qxg9o-0002Ez-8p for qemu-devel@nongnu.org; Sun, 28 Aug 2011 10:16:17 -0400 Received: from mail-fx0-f45.google.com ([209.85.161.45]:49287) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qxg9o-0002Eu-3J for qemu-devel@nongnu.org; Sun, 28 Aug 2011 10:16:16 -0400 Received: by fxbb27 with SMTP id b27so4173328fxb.4 for ; Sun, 28 Aug 2011 07:16:15 -0700 (PDT) Date: Sun, 28 Aug 2011 16:16:11 +0200 From: "Edgar E. Iglesias" Message-ID: <20110828141611.GD11446@zapo> References: <1314302711-20498-1-git-send-email-peter.maydell@linaro.org> <1314302711-20498-3-git-send-email-peter.maydell@linaro.org> <20110827173820.GB11446@zapo> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 02/17] hw/onenand: Qdevify List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, patches@linaro.org On Sun, Aug 28, 2011 at 02:21:29PM +0100, Peter Maydell wrote: > On 27 August 2011 18:38, Edgar E. Iglesias wrote: > > On Thu, Aug 25, 2011 at 09:04:56PM +0100, Peter Maydell wrote: > >> @@ -659,7 +713,7 @@ static void onenand_write(void *opaque, target_phys_addr_t addr, > >>          if (s->intstatus & (1 << 15)) > >>              break; > >>          s->command = value; > >> -        onenand_command(s, s->command); > >> +        onenand_command(s); > >>          break; > > > > > > This s->command change doesnt seem related, is there a reason for it that > > I'm missing? > > Are you objecting to the change itself or the fact it's in this > patch rather than its own patch? (I'm happy to split it out into > a separate patch if you prefer.) > > I think the change itself is right -- in a qdev device these > functions are basically methods on the qdev object, and it > doesn't make sense to pass a method one of the object's own > fields as a method argument. So either we should have > onenand_command(s, value); > and make onenand_command do the setting of s->command; > or we do what this patch does and let onenand_command() > read the member field. OK thanks, I see them more like stylistic changes and would have probably left them out for the sake of reviewability. But it doesn't matter, my main concern was that I was missing something more important here. No need to respin just for this.. > >> +DeviceState *onenand_init(BlockDriverState *bdrv, > >> +                          uint16_t man_id, uint16_t dev_id, uint16_t ver_id, > >> +                          int regshift, qemu_irq irq) > >>  { > >> -    OneNANDState *s = (OneNANDState *) opaque; > >> +    DeviceState *dev = qdev_create(NULL, "onenand"); > >> +    qdev_prop_set_uint16(dev, "manufacturer_id", man_id); > >> +    qdev_prop_set_uint16(dev, "device_id", dev_id); > >> +    qdev_prop_set_uint16(dev, "version_id", ver_id); > >> +    qdev_prop_set_int32(dev, "shift", regshift); > >> +    if (bdrv) { > >> +        qdev_prop_set_drive_nofail(dev, "drive", bdrv); > >> +    } > >> +    qdev_init_nofail(dev); > >> +    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); > >> +    return dev; > >> +} > > > > Personally Im not a fan of having code that conceptually runs above Qdev, > > embedded in qdev models. But there seems to be a lack of agreement on this > > and its commonly done elsewere. I'm not NAKing but if you agree, and would > > like to move it out, I'd appreciate it. > > I think they're really just utility functions which are working around > the problem qdev has that instantiating, configuring and wiring up > a qdev model is so verbose. But I'm happy to lose this one, especially > since it's only used once. (well, twice once I get the n900 model in > a submittable state...) Thanks. Please note that I'm not opposed to the helpers per se, but more with the placement of them. It was shortly discussed here (a long while ago): http://lists.gnu.org/archive/html/qemu-devel/2009-09/msg00843.html Cheers