Skip to contentSkip to conversation

hw/ds1338.c: Fix handling of DATE (wday) register

PATCH In review Antoine Mathys 2 participants 2 messages Dec 3, 2012 – Dec 4, 2012

lore
Share & tools
lore

Antoine Mathys

author
#0 lore raw

PATCH 1/1 hw/ds1338.c: Fix handling of DATE (wday) register

Per the datasheet, the DATE (wday) register is user defined. Implement this.

Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
hw/ds1338.c +12 -2
index 8f85635..c502934 100644
@@ -20,6 +20,7 @@
 typedef struct {
     I2CSlave i2c;
     int64_t offset;
+    uint8_t wday_offset;
     uint8_t nvram[NVRAM_SIZE];
     int32_t ptr;
     bool addr_byte;

@@ -33,6 +34,7 @@ static const VMStateDescription vmstate_ds1338 = {
     .fields = (VMStateField[]) {
         VMSTATE_I2C_SLAVE(i2c, DS1338State),
         VMSTATE_INT64(offset, DS1338State),
+        VMSTATE_UINT8(wday_offset, DS1338State),
         VMSTATE_UINT8_ARRAY(nvram, DS1338State, NVRAM_SIZE),
         VMSTATE_INT32(ptr, DS1338State),
         VMSTATE_BOOL(addr_byte, DS1338State),

@@ -62,7 +64,7 @@ static void write_time(DS1338State *s, const struct tm *tm)
     } else {
         s->nvram[2] = to_bcd(tm->tm_hour);
     }
-    s->nvram[3] = to_bcd(tm->tm_wday + 1);
+    s->nvram[3] = to_bcd((tm->tm_wday + s->wday_offset) % 7 + 1);
     s->nvram[4] = to_bcd(tm->tm_mday);
     s->nvram[5] = to_bcd(tm->tm_mon + 1);
     s->nvram[6] = to_bcd(tm->tm_year - 100);

@@ -164,7 +166,12 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             }
             break;
         case 3:
-            now.tm_wday = from_bcd(data & 0x07) - 1;
+            {
+                int user_wday = from_bcd(data & 0x07) - 1;
+                if ((user_wday >= 0) && (user_wday <= 6)) {
+                    s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
+                }
+            }
             break;
         case 4:
             now.tm_mday = from_bcd(data & 0x3f);

@@ -194,6 +201,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 
 static int ds1338_init(I2CSlave *i2c)
 {
+    DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
+    s->wday_offset = 0;
+
     return 0;
 }
Signature · 1 line
1.7.10.4
#1 lore raw
Quoted 20 lines · Antoine Mathys

Per the datasheet, the DATE (wday) register is user defined. Implement this.

Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
hw/ds1338.c +1 -0
index 8f85635..c502934 100644
@@ -20,6 +20,7 @@
 typedef struct {
     I2CSlave i2c;
     int64_t offset;
+    uint8_t wday_offset;
     uint8_t nvram[NVRAM_SIZE];
     int32_t ptr;
     bool addr_byte;

hw/ds1338.c static const VMStateDescription vmstate_ds1338 = { · line 40

Quoted 7 lines
hw/ds1338.c +1 -0
@@ -33,6 +34,7 @@ static const VMStateDescription vmstate_ds1338 = {
     .fields = (VMStateField[]) {
         VMSTATE_I2C_SLAVE(i2c, DS1338State),
         VMSTATE_INT64(offset, DS1338State),
+        VMSTATE_UINT8(wday_offset, DS1338State),
         VMSTATE_UINT8_ARRAY(nvram, DS1338State, NVRAM_SIZE),
         VMSTATE_INT32(ptr, DS1338State),
         VMSTATE_BOOL(addr_byte, DS1338State),

This breaks migration -- you need to bump the version_id to 2 and mark the new field as only present in the new version, like this:

     VMSTATE_UINT8_V(wday_offset, DS1338State, 2),

hw/ds1338.c static void write_time(DS1338State *s, const struct tm · line 68

Quoted 7 lines
hw/ds1338.c +1 -1
@@ -62,7 +64,7 @@ static void write_time(DS1338State *s, const struct tm
 *tm)
     } else {
         s->nvram[2] = to_bcd(tm->tm_hour);
     }
-    s->nvram[3] = to_bcd(tm->tm_wday + 1);
+    s->nvram[3] = to_bcd((tm->tm_wday + s->wday_offset) % 7 + 1);

There's not much point doing a to_bcd() on a value guaranteed to be in [0..9].

hw/ds1338.c static int ds1338_send(I2CSlave *i2c, uint8_t data) · line 169

Quoted 9 lines
hw/ds1338.c +0 -0
     s->nvram[4] = to_bcd(tm->tm_mday);
     s->nvram[5] = to_bcd(tm->tm_mon + 1);

     s->nvram[6] = to_bcd(tm->tm_year - 100);
hw/ds1338.c +1 -1
@@ -164,7 +166,12 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             }
             break;
         case 3:
-            now.tm_wday = from_bcd(data & 0x07) - 1;
+            {

This brace should line up with the 'c' in 'case' (ditto the closing brace).

hw/ds1338.c static int ds1338_send(I2CSlave *i2c, uint8_t data) · line 170

Quoted 1 line
hw/ds1338.c +1 -0
+                int user_wday = from_bcd(data & 0x07) - 1;

...again, from_bcd() is pointless here.

hw/ds1338.c static int ds1338_send(I2CSlave *i2c, uint8_t data) · line 171

Quoted 1 line
hw/ds1338.c +1 -0
+                if ((user_wday >= 0) && (user_wday <= 6)) {

This condition is an obscure way to guard against the undefined case of the guest writing zero to a register that's supposed to contain values between 1 and 7. It would also be good to have a comment saying explicitly that the datasheet says you get undefined operation here.

(for curiosity, do you happen to have real hardware to see what it does in this case?)

hw/ds1338.c static int ds1338_send(I2CSlave *i2c, uint8_t data) · line 206

Quoted 13 lines
hw/ds1338.c +6 -0
+                    s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
+                }
+            }
             break;
         case 4:
             now.tm_mday = from_bcd(data & 0x3f);

@@ -194,6 +201,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 
 static int ds1338_init(I2CSlave *i2c)
 {
+    DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
+    s->wday_offset = 0;
+

This doesn't belong in the device init function, you need to create a reset function and reset it there. (ie set dc->reset in the class init function, see eg hw/pl190.c for an example).

Quoted 6 lines
hw/ds1338.c +0 -1
     return 0;
 }
 
--

1.7.10.4

thanks
-- PMM