Re: [Qemu-devel] [PATCH 02/17] hw/onenand: Qdevify

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Sun, Aug 28, 2011 at 02:21:29PM +0100, Peter Maydell wrote:
> On 27 August 2011 18:38, Edgar E. Iglesias <edgar.iglesias@gmail.com> 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