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 <w...@mail.berlios.de>
> >> ---
> >> 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
> >>
