From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1Moeup-0000IO-An
	for qemu-devel@nongnu.org; Fri, 18 Sep 2009 10:58:27 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1Moeuj-0000IA-Ts
	for qemu-devel@nongnu.org; Fri, 18 Sep 2009 10:58:26 -0400
Received: from [199.232.76.173] (port=33125 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Moeuj-0000I7-MV
	for qemu-devel@nongnu.org; Fri, 18 Sep 2009 10:58:21 -0400
Received: from mx1.redhat.com ([209.132.183.28]:16285)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <kraxel@redhat.com>) id 1Moeuj-0008Bw-51
	for qemu-devel@nongnu.org; Fri, 18 Sep 2009 10:58:21 -0400
Received: from int-mx05.intmail.prod.int.phx2.redhat.com
	(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8IEwKXf009103
	for <qemu-devel@nongnu.org>; Fri, 18 Sep 2009 10:58:20 -0400
Message-ID: <4AB3A009.1070104@redhat.com>
Date: Fri, 18 Sep 2009 16:58:17 +0200
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 2/5] floppy: move dma setup + drive connect
	to fdctrl_init_common()
References: <1253132744-10492-1-git-send-email-kraxel@redhat.com>	<1253132744-10492-3-git-send-email-kraxel@redhat.com>
	<87fxak48cv.fsf@pike.pond.sub.org>
In-Reply-To: <87fxak48cv.fsf@pike.pond.sub.org>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org

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