diff mbox

[11/20] eepro100: Use symbolic names for bits in EEPROM id

Message ID 1266164189-21062-11-git-send-email-weil@mail.berlios.de
State New
Headers show

Commit Message

Stefan Weil Feb. 14, 2010, 4:16 p.m. UTC 
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

Comments

Michael S. Tsirkin Feb. 21, 2010, 5:05 p.m. UTC | #1 
On Sun, Feb 14, 2010 at 05:16:20PM +0100, Stefan Weil wrote:
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/eepro100.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index c647322..01bcd6d 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -272,6 +272,21 @@ typedef enum {
>      eeprom_smbus_addr = 0x90,
>  } EEPROMOffset;
>  
> +/* Bit values for EEPROM ID word. */
> +typedef enum {
> +    eeprom_id_mdm = BIT(0),     /* Modem */
> +    eeprom_id_stb = BIT(1),     /* Standby Enable */
> +    eeprom_id_wmr = BIT(2),     /* ??? */
> +    eeprom_id_wol = BIT(5),     /* Wake on LAN */
> +    eeprom_id_dpd = BIT(6),     /* Deep Power Down */
> +    eeprom_id_alt = BIT(7),     /* */
> +    /* BITS(10, 8) device revision */
> +    eeprom_id_bd = BIT(11),     /* boot disable */
> +    eeprom_id_id = BIT(13),     /* id bit */
> +    /* BITS(15, 14) signature */
> +    eeprom_id_valid = BIT(14),  /* signature for valid eeprom */
> +} eeprom_id_bit;

The type name is unused - don't add it?
Can constants be upper case?


> +
>  /* Default values for MDI (PHY) registers */
>  static const uint16_t eepro100_mdi_default[] = {
>      /* MDI Registers 0 - 6, 7 */
> @@ -643,7 +658,7 @@ static void nic_selective_reset(EEPRO100State * s)
>      uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
>      //~ eeprom93xx_reset(s->eeprom);
>      memcpy(eeprom_contents, s->conf.macaddr.a, 6);
> -    eeprom_contents[eeprom_id] = 0x4000;
> +    eeprom_contents[eeprom_id] = eeprom_id_valid;
>      if (s->device == i82557B || s->device == i82557C)
>          eeprom_contents[5] = 0x0100;
>      eeprom_contents[eeprom_phy_id] = 1;
> -- 
> 1.6.6.1
> 
>
Stefan Weil Feb. 21, 2010, 9:38 p.m. UTC | #2 
Michael S. Tsirkin schrieb:
> On Sun, Feb 14, 2010 at 05:16:20PM +0100, Stefan Weil wrote:
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>> hw/eepro100.c | 17 ++++++++++++++++-
>> 1 files changed, 16 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index c647322..01bcd6d 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -272,6 +272,21 @@ typedef enum {
>> eeprom_smbus_addr = 0x90,
>> } EEPROMOffset;
>>
>> +/* Bit values for EEPROM ID word. */
>> +typedef enum {
>> + eeprom_id_mdm = BIT(0), /* Modem */
>> + eeprom_id_stb = BIT(1), /* Standby Enable */
>> + eeprom_id_wmr = BIT(2), /* ??? */
>> + eeprom_id_wol = BIT(5), /* Wake on LAN */
>> + eeprom_id_dpd = BIT(6), /* Deep Power Down */
>> + eeprom_id_alt = BIT(7), /* */
>> + /* BITS(10, 8) device revision */
>> + eeprom_id_bd = BIT(11), /* boot disable */
>> + eeprom_id_id = BIT(13), /* id bit */
>> + /* BITS(15, 14) signature */
>> + eeprom_id_valid = BIT(14), /* signature for valid eeprom */
>> +} eeprom_id_bit;
>
> The type name is unused - don't add it?

Maybe it will be used some day?
Adding a type name now makes the code more uniform
and avoids two deltas when it is used.

There are a lot of symbolic names which are unused
today, for example most of above enum values.
This is common practice.

> Can constants be upper case?

Of course they can. Is this a QEMU coding convention?
I did not see a hint in CODING_STYLE.

If it is the coding style: UPPER_CASE or CamelCase?
Will it be required for all future modifications
of QEMU code?

>
>
>> +
>> /* Default values for MDI (PHY) registers */
>> static const uint16_t eepro100_mdi_default[] = {
>> /* MDI Registers 0 - 6, 7 */
>> @@ -643,7 +658,7 @@ static void nic_selective_reset(EEPRO100State * s)
>> uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
>> //~ eeprom93xx_reset(s->eeprom);
>> memcpy(eeprom_contents, s->conf.macaddr.a, 6);
>> - eeprom_contents[eeprom_id] = 0x4000;
>> + eeprom_contents[eeprom_id] = eeprom_id_valid;
>> if (s->device == i82557B || s->device == i82557C)
>> eeprom_contents[5] = 0x0100;
>> eeprom_contents[eeprom_phy_id] = 1;
>> -- 
>> 1.6.6.1
>>
>>
Michael S. Tsirkin Feb. 22, 2010, 9 a.m. UTC | #3 
On Sun, Feb 21, 2010 at 10:38:02PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Sun, Feb 14, 2010 at 05:16:20PM +0100, Stefan Weil wrote:
> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> >> ---
> >> hw/eepro100.c | 17 ++++++++++++++++-
> >> 1 files changed, 16 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/eepro100.c b/hw/eepro100.c
> >> index c647322..01bcd6d 100644
> >> --- a/hw/eepro100.c
> >> +++ b/hw/eepro100.c
> >> @@ -272,6 +272,21 @@ typedef enum {
> >> eeprom_smbus_addr = 0x90,
> >> } EEPROMOffset;
> >>
> >> +/* Bit values for EEPROM ID word. */
> >> +typedef enum {
> >> + eeprom_id_mdm = BIT(0), /* Modem */
> >> + eeprom_id_stb = BIT(1), /* Standby Enable */
> >> + eeprom_id_wmr = BIT(2), /* ??? */
> >> + eeprom_id_wol = BIT(5), /* Wake on LAN */
> >> + eeprom_id_dpd = BIT(6), /* Deep Power Down */
> >> + eeprom_id_alt = BIT(7), /* */
> >> + /* BITS(10, 8) device revision */
> >> + eeprom_id_bd = BIT(11), /* boot disable */
> >> + eeprom_id_id = BIT(13), /* id bit */
> >> + /* BITS(15, 14) signature */
> >> + eeprom_id_valid = BIT(14), /* signature for valid eeprom */
> >> +} eeprom_id_bit;
> >
> > The type name is unused - don't add it?
> 
> Maybe it will be used some day?
> Adding a type name now makes the code more uniform
> and avoids two deltas when it is used.
> 
> There are a lot of symbolic names which are unused
> today, for example most of above enum values.
> This is common practice.
> 
> > Can constants be upper case?
> 
> Of course they can. Is this a QEMU coding convention?
> I did not see a hint in CODING_STYLE.
> 
> If it is the coding style: UPPER_CASE or CamelCase?

UPPER_CASE

> Will it be required for all future modifications
> of QEMU code?
> 
> >
> >
> >> +
> >> /* Default values for MDI (PHY) registers */
> >> static const uint16_t eepro100_mdi_default[] = {
> >> /* MDI Registers 0 - 6, 7 */
> >> @@ -643,7 +658,7 @@ static void nic_selective_reset(EEPRO100State * s)
> >> uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
> >> //~ eeprom93xx_reset(s->eeprom);
> >> memcpy(eeprom_contents, s->conf.macaddr.a, 6);
> >> - eeprom_contents[eeprom_id] = 0x4000;
> >> + eeprom_contents[eeprom_id] = eeprom_id_valid;
> >> if (s->device == i82557B || s->device == i82557C)
> >> eeprom_contents[5] = 0x0100;
> >> eeprom_contents[eeprom_phy_id] = 1;
> >> -- 
> >> 1.6.6.1
> >>
> >>
diff mbox

Patch

diff --git a/hw/eepro100.c b/hw/eepro100.c
index c647322..01bcd6d 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -272,6 +272,21 @@  typedef enum {
     eeprom_smbus_addr = 0x90,
 } EEPROMOffset;
 
+/* Bit values for EEPROM ID word. */
+typedef enum {
+    eeprom_id_mdm = BIT(0),     /* Modem */
+    eeprom_id_stb = BIT(1),     /* Standby Enable */
+    eeprom_id_wmr = BIT(2),     /* ??? */
+    eeprom_id_wol = BIT(5),     /* Wake on LAN */
+    eeprom_id_dpd = BIT(6),     /* Deep Power Down */
+    eeprom_id_alt = BIT(7),     /* */
+    /* BITS(10, 8) device revision */
+    eeprom_id_bd = BIT(11),     /* boot disable */
+    eeprom_id_id = BIT(13),     /* id bit */
+    /* BITS(15, 14) signature */
+    eeprom_id_valid = BIT(14),  /* signature for valid eeprom */
+} eeprom_id_bit;
+
 /* Default values for MDI (PHY) registers */
 static const uint16_t eepro100_mdi_default[] = {
     /* MDI Registers 0 - 6, 7 */
@@ -643,7 +658,7 @@  static void nic_selective_reset(EEPRO100State * s)
     uint16_t *eeprom_contents = eeprom93xx_data(s->eeprom);
     //~ eeprom93xx_reset(s->eeprom);
     memcpy(eeprom_contents, s->conf.macaddr.a, 6);
-    eeprom_contents[eeprom_id] = 0x4000;
+    eeprom_contents[eeprom_id] = eeprom_id_valid;
     if (s->device == i82557B || s->device == i82557C)
         eeprom_contents[5] = 0x0100;
     eeprom_contents[eeprom_phy_id] = 1;