Re: [Qemu-devel] [PATCH 2/5] floppy: move dma setup + drive connect to fdctrl_init_common()

[Gerd Hoffmann <kraxel@redhat.com>]

>>   fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>> @@ -1870,19 +1861,16 @@ fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>>       fdctrl_sysbus_t *sys;
>>
>>       dev = qdev_create(NULL, "sysbus-fdc");
>> +    sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
>> +    fdctrl =&sys->state;
>> +    fdctrl->dma_chann = dma_chann; /* FIXME */
>
> What needs to be fixed here?  Could that be explained in the comment?

See below.

>> -    fdctrl_connect_drives(fdctrl);
>> +    *fdc_tc = qdev_get_gpio_in(dev, 0); /* FIXME */
>
> Same question.

fdctrl_init_$kind() should just be convinience wrappers.  Creating the 
devices via -device should work too.  Which means the convinience 
wrappers must do nothing but qdev_create + set properties + qdev_init.

> Hmm, as far as I can see, initialization of fdctrl->dma_chann moved to
> the qdev init() method for ISA and Sun4m, but not for sysbus.
> Intentional?  If yes, what about explaining it in the code, or perhaps
> the commit message?

Yes, intentional.  On the ISA bus the dma channel is fixed: #2.  sun4m 
doesn't use dma.  The third variant has one user which uses dma channel 
#0.  So we could either hard-code that one too (like we do for isa).  Or 
we could make it a property so it can be configured as needed.  Dunno 
which of the two variants would be the correct one.

cheers,
   Gerd