From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Nz9ug-0007DJ-GM for qemu-devel@nongnu.org; Tue, 06 Apr 2010 10:37:58 -0400 Received: from [140.186.70.92] (port=49187 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Nz9ue-0007Ce-V9 for qemu-devel@nongnu.org; Tue, 06 Apr 2010 10:37:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Nz9ud-0007D5-AR for qemu-devel@nongnu.org; Tue, 06 Apr 2010 10:37:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9368) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Nz9ud-0007Cw-1A for qemu-devel@nongnu.org; Tue, 06 Apr 2010 10:37:55 -0400 Date: Tue, 6 Apr 2010 17:34:13 +0300 From: "Michael S. Tsirkin" Message-ID: <20100406143413.GC20325@redhat.com> References: <1270554249-24861-1-git-send-email-weil@mail.berlios.de> <1270554249-24861-3-git-send-email-weil@mail.berlios.de> <20100406121830.GE16539@redhat.com> <4BBB4559.6030702@mail.berlios.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4BBB4559.6030702@mail.berlios.de> Subject: [Qemu-devel] Re: [PATCH 2/9] eepro100: Simplify status handling List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: QEMU Developers On Tue, Apr 06, 2010 at 04:29:45PM +0200, Stefan Weil wrote: > Michael S. Tsirkin schrieb: > > On Tue, Apr 06, 2010 at 01:44:02PM +0200, Stefan Weil wrote: > > > >> Signed-off-by: Stefan Weil > >> --- > >> hw/eepro100.c | 9 +++++---- > >> 1 files changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/hw/eepro100.c b/hw/eepro100.c > >> index 0415132..741031c 100644 > >> --- a/hw/eepro100.c > >> +++ b/hw/eepro100.c > >> @@ -175,6 +175,7 @@ typedef enum { > >> } scb_command_bit; > >> > >> typedef enum { > >> + STATUS_NOT_OK = 0, > >> STATUS_C = BIT(15), > >> STATUS_OK = BIT(13), > >> } scb_status_bit; > >> > > > > 0 is not a bit, I would just use 0 as a constant below. > > > > In a philosophical way, not a bit is some kind of a bit. > > I think ok_status = STATUS_NOT_OK is clearer than > ok_status = 0. The reason it's not clear is because STATUS_OK | STATUS_NOT_OK is not a valid combination. IOW, each enum should be: - a set of bits. 0 is not a bit, you write 0 to say "no bits". you can | the values. - a set of values. you can not | values together. So either we have ok_status = 0 -> no bits set, and then ok_status | STATUS_C, or STATUS_NOT_OK = BIT(15) STATUS_OK = BIT(13) | BIT(15), and then just use ok_status. > > > >> @@ -882,7 +883,7 @@ static void action_command(EEPRO100State *s) > >> bool bit_s; > >> bool bit_i; > >> bool bit_nc; > >> - bool success = true; > >> + uint16_t ok_status = STATUS_OK; > >> s->cb_address = s->cu_base + s->cu_offset; > >> read_cb(s); > >> bit_el = ((s->tx.command & COMMAND_EL) != 0); > >> @@ -915,7 +916,7 @@ static void action_command(EEPRO100State *s) > >> case CmdTx: > >> if (bit_nc) { > >> missing("CmdTx: NC = 0"); > >> - success = false; > >> + ok_status = STATUS_NOT_OK; > >> break; > >> } > >> tx_command(s); > >> @@ -932,11 +933,11 @@ static void action_command(EEPRO100State *s) > >> break; > >> default: > >> missing("undefined command"); > >> - success = false; > >> + ok_status = STATUS_NOT_OK; > >> break; > >> } > >> /* Write new status. */ > >> - stw_phys(s->cb_address, s->tx.status | STATUS_C | (success ? STATUS_OK : 0)); > >> + stw_phys(s->cb_address, s->tx.status | ok_status | STATUS_C); > >> if (bit_i) { > >> /* CU completed action. */ > >> eepro100_cx_interrupt(s); > >> -- > >> 1.7.0 > >>