From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:52020)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1TsdcO-0001Do-Aa
	for qemu-devel@nongnu.org; Tue, 08 Jan 2013 13:09:46 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1TsdcM-00075A-U8
	for qemu-devel@nongnu.org; Tue, 08 Jan 2013 13:09:44 -0500
Received: from cantor2.suse.de ([195.135.220.15]:39366 helo=mx2.suse.de)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1TsdcM-0006ht-LB
	for qemu-devel@nongnu.org; Tue, 08 Jan 2013 13:09:42 -0500
Message-ID: <50EC60B9.60304@suse.de>
Date: Tue, 08 Jan 2013 19:08:57 +0100
From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= <afaerber@suse.de>
MIME-Version: 1.0
References: <cover.1357607262.git.agarcia@igalia.com>
	<4a5728bd52fbd15616ba7a20ffb3e1a90be2cc74.1357607262.git.agarcia@igalia.com>
In-Reply-To: <4a5728bd52fbd15616ba7a20ffb3e1a90be2cc74.1357607262.git.agarcia@igalia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH v4 2/2] Add GE IP-Octal 232 IndustryPack
 emulation
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alberto Garcia <agarcia@igalia.com>
Cc: Blue Swirl <blauwirbel@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Anthony Liguori <aliguori@us.ibm.com>, qemu-devel@nongnu.org

Am 08.01.2013 02:10, schrieb Alberto Garcia:
> The GE IP-Octal 232 is an IndustryPack module that implements eight
> RS-232 serial ports, each one of which can be redirected to a
> character device in the host.
>=20
> Signed-off-by: Alberto Garcia <agarcia@igalia.com>
> ---
>  hw/Makefile.objs |    2 +-
>  hw/ipoctal.c     |  614 ++++++++++++++++++++++++++++++++++++++++++++++=
++++++++

On my suggestion you changed the type name from "ipoctal" to
"ipoctal232" - the file name should probably then be updated, too.
For the function prefixes inside the file it shouldn't matter as long as
they're consistent.

[...]
> +typedef struct IPOctalState IPOctalState;
> +typedef struct SCC2698Channel SCC2698Channel;
> +typedef struct SCC2698Block SCC2698Block;
> +
> +struct SCC2698Channel {
> +    IPOctalState *ipoctal;
> +    CharDriverState *dev;
> +    char *devpath;
> +    bool rx_enabled;
> +    uint8_t mr[2];
> +    uint8_t mr_idx;
> +    uint8_t sr;
> +    uint8_t rhr[RX_FIFO_SIZE];
> +    uint8_t rhr_idx;
> +    uint8_t rx_pending;
> +};
> +
> +struct SCC2698Block {
> +    uint8_t imr;
> +    uint8_t isr;
> +};
> +
> +struct IPOctalState {
> +    IPackDevice dev;

By convention:

IPackDevice parent_obj;

SCC2698...

> +    SCC2698Channel ch[N_CHANNELS];
> +    SCC2698Block blk[N_BLOCKS];
> +    uint8_t irq_vector;
> +};

I understand that this device will be used from the command line only,
so the upcoming TYPE_IPOCTAL232 could remain local to this file for the
cast macros (i.e., no ipoctal232.h needed).
But if, as brought up at LinuxCon, you may want to add qtest cases it
may be helpful to move some of the constants above to their own header
file ipoctal232_regs.h as done for RTC or pending for TMP105. That could
be split out in a later step though, once PCI libqos is available.

Regards,
Andreas

P.S. No need to CC avi@redhat.com any longer, you may have noticed. ;)

--=20
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC=
rnberg