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.
>> +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 for the rapid review of this patchset.
-- PMM
