From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1Ku7wX-0000B8-0h
	for qemu-devel@nongnu.org; Sun, 26 Oct 2008 11:54:17 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1Ku7wS-0008Pp-BW
	for qemu-devel@nongnu.org; Sun, 26 Oct 2008 11:54:16 -0400
Received: from [199.232.76.173] (port=56143 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Ku7wS-0008P8-25
	for qemu-devel@nongnu.org; Sun, 26 Oct 2008 11:54:12 -0400
Received: from wf-out-1314.google.com ([209.85.200.169]:31541)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <blauwirbel@gmail.com>) id 1Ku7wR-0000Dl-K3
	for qemu-devel@nongnu.org; Sun, 26 Oct 2008 11:54:11 -0400
Received: by wf-out-1314.google.com with SMTP id 27so1654703wfd.4
	for <qemu-devel@nongnu.org>; Sun, 26 Oct 2008 08:54:08 -0700 (PDT)
Message-ID: <f43fc5580810260854q41264525xdaf9199686d9dcb8@mail.gmail.com>
Date: Sun, 26 Oct 2008 17:54:08 +0200
From: "Blue Swirl" <blauwirbel@gmail.com>
Subject: Re: [Qemu-devel] [PATCH 1/3] sh4: mmio based CF support on r2d board.
In-Reply-To: <200810261517.m9QFHUR4026096@smtp12.dti.ne.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <20080923013136.490117fd.yoshii.takashi@gmail.com>
	<200810261517.m9QFHUR4026096@smtp12.dti.ne.jp>
Reply-To: qemu-devel@nongnu.org
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: qemu-devel@nongnu.org

On 10/26/08, takasi-y@ops.dti.ne.jp <takasi-y@ops.dti.ne.jp> wrote:
> This patch adds emulation for a CompactFlash on sh4/r2d board.
>  The device is CF, but wired to be worked as True-IDE mode, and connected
>   directly to SH bus. So, this code is to supports generally mmio based IDEs
>   which are supported by "pata_platform" driver in linux kernel.

+    if(addr & 7)

I'd add a space between if and (.

+static uint32_t mmio_ide_status_read (void *opaque,target_phys_addr_t addr)

Please add a space between the comma after opaque and target_phys_addr_t.

>  +void mmio_ide_init (int *mmio, BlockDriverState *hd0, BlockDriverState *hd1,
>  +       qemu_irq irq, int shift)
>  +{
>  +    MMIOState *s = qemu_mallocz(sizeof(MMIOState));
>  +    IDEState *ide = qemu_mallocz(sizeof(IDEState) * 2);
>  +    int *io;
>  +
>  +    ide_init2(ide, hd0, hd1, irq);
>  +
>  +    s->dev = ide;
>  +    s->shift = shift;
>  +
>  +    mmio[0] = cpu_register_io_memory(0, mmio_ide_reads, mmio_ide_writes, s);
>  +    mmio[1] = cpu_register_io_memory(0, mmio_ide_status, mmio_ide_cmd, s);
>  +}

It would be better to make the init function take instead of mmio
pointer, two target_phys_addr_t parameters and do the physical memory
registration there.

>  +    extern void mmio_ide_init (int*, BlockDriverState*, BlockDriverState*,
>  +        qemu_irq, int);

This should be in a .h file (maybe pc.h), without "extern" and the
parameter names should be included.

Otherwise, the whole patch looks OK.