diff mbox

[2/9] eepro100: Simplify status handling

Message ID 1270554249-24861-3-git-send-email-weil@mail.berlios.de
State New
Headers show

Commit Message

Stefan Weil April 6, 2010, 11:44 a.m. UTC 
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin April 6, 2010, 12:18 p.m. UTC | #1 
On Tue, Apr 06, 2010 at 01:44:02PM +0200, Stefan Weil wrote:
> Signed-off-by: Stefan Weil <weil@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.

> @@ -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
Stefan Weil April 6, 2010, 2:29 p.m. UTC | #2 
Michael S. Tsirkin schrieb:
> On Tue, Apr 06, 2010 at 01:44:02PM +0200, Stefan Weil wrote:
>   
>> Signed-off-by: Stefan Weil <weil@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.

>   
>> @@ -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
>>
Michael S. Tsirkin April 6, 2010, 2:34 p.m. UTC | #3 
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 <weil@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
> >>
diff mbox

Patch

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;
@@ -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);