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

[Gerd Hoffmann]

> >   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