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