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

[Peter Maydell]

On 27 August 2011 18:38, Edgar E. Iglesias <address@hidden> 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