Re: [Qemu-devel] [PATCH 04/14] timer: ds1338 clarify HOUR handling
On Thu, Jul 05, 2018 at 11:19:51AM -0700, Michael Davidsaver wrote: > Simplify and comment the translation between > registers and struct tm. > > Signed-off-by: Michael Davidsaver Reviewed-by: David Gibson although.. [snip] > @@ -101,7 +105,9 @@ static void capture_current_time(DS1338State *s) > } else { > s->nvram[R_HOUR] = R_HOUR_SET12_MASK | R_HOUR_AMPM_MASK | > to_bcd(tmp - 12); > } > + > } else { I'm not real fond of blank lines before the ends of blocks. > +/* 24 hour mode. */ > s->nvram[R_HOUR] = to_bcd(now.tm_hour); > } > s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7 + 1; > @@ -178,14 +184,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data) > break; > case R_HOUR: > if (FIELD_EX32(data, HOUR, SET12)) { > -int tmp = from_bcd(FIELD_EX32(data, HOUR, HOUR12)); > +/* 12 hour (1-12) */ > +/* read and wrap 1-12 -> 0-11 */ > +now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u; > if (FIELD_EX32(data, HOUR, AMPM)) { > -tmp += 12; > +now.tm_hour += 12; > } > -if (tmp % 12 == 0) { > -tmp -= 12; > -} > -now.tm_hour = tmp; > + > } else { > now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24)); > } -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[Qemu-devel] [PATCH 04/14] timer: ds1338 clarify HOUR handling
Simplify and comment the translation between registers and struct tm. Signed-off-by: Michael Davidsaver --- hw/timer/ds1338.c | 17 +++-- 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c index b56db5852e..637a0f4246 100644 --- a/hw/timer/ds1338.c +++ b/hw/timer/ds1338.c @@ -89,9 +89,13 @@ static void capture_current_time(DS1338State *s) */ struct tm now; qemu_get_timedate(, s->offset); + s->nvram[R_SEC] = to_bcd(now.tm_sec); s->nvram[R_MIN] = to_bcd(now.tm_min); if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) { +/* 12 hour mode. + * map 0-23 to 1-12 am/pm + */ int tmp = now.tm_hour; if (tmp % 12 == 0) { tmp += 12; @@ -101,7 +105,9 @@ static void capture_current_time(DS1338State *s) } else { s->nvram[R_HOUR] = R_HOUR_SET12_MASK | R_HOUR_AMPM_MASK | to_bcd(tmp - 12); } + } else { +/* 24 hour mode. */ s->nvram[R_HOUR] = to_bcd(now.tm_hour); } s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7 + 1; @@ -178,14 +184,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data) break; case R_HOUR: if (FIELD_EX32(data, HOUR, SET12)) { -int tmp = from_bcd(FIELD_EX32(data, HOUR, HOUR12)); +/* 12 hour (1-12) */ +/* read and wrap 1-12 -> 0-11 */ +now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u; if (FIELD_EX32(data, HOUR, AMPM)) { -tmp += 12; +now.tm_hour += 12; } -if (tmp % 12 == 0) { -tmp -= 12; -} -now.tm_hour = tmp; + } else { now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24)); } -- 2.11.0
Re: [Qemu-devel] [PATCH 04/14] timer: ds1338 clarify HOUR handling
On 24 March 2018 at 19:24, Michael Davidsaverwrote: > Simplify and comment the translation between > registers and struct tm. > > Signed-off-by: Michael Davidsaver > --- > hw/timer/ds1338.c | 35 +-- > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c > index 72a4692d60..9bcda26e51 100644 > --- a/hw/timer/ds1338.c > +++ b/hw/timer/ds1338.c > @@ -88,23 +88,23 @@ static void capture_current_time(DS1338State *s) > * which will be actually read by the data transfer operation. > */ > struct tm now; > +bool mode12 = ARRAY_FIELD_EX32(s->nvram, HOUR, SET12); > qemu_get_timedate(, s->offset); > + > s->nvram[R_SEC] = to_bcd(now.tm_sec); > s->nvram[R_MIN] = to_bcd(now.tm_min); > -if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) { > -int tmp = now.tm_hour; > -if (tmp % 12 == 0) { > -tmp += 12; > -} > -s->nvram[R_HOUR] = 0; > +s->nvram[R_HOUR] = 0; > +if (mode12) { > +/* map 0-23 to 1-12 am/pm */ > ARRAY_FIELD_DP32(s->nvram, HOUR, SET12, 1); > -if (tmp <= 12) { > -ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, 0); > -ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(tmp)); > -} else { > -ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, 1); > -ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(tmp - 12)); Oh, you're rewriting all this code from the earlier patch anyway. In that case you should definitely just have the earlier patch match the logic that the existing code did, so that it's easier to review as being a no-change patch. > +ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, now.tm_hour >= 12u); > +now.tm_hour %= 12u; /* wrap 0-23 to 0-11 */ > +if (now.tm_hour == 0u) { > +/* midnight/noon stored as 12 */ > +now.tm_hour = 12u; > } > +ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(now.tm_hour)); > + > } else { > ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR24, to_bcd(now.tm_hour)); > } > @@ -182,14 +182,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data) > break; > case R_HOUR: > if (FIELD_EX32(data, HOUR, SET12)) { > -int tmp = from_bcd(FIELD_EX32(data, HOUR, HOUR12)); > +/* 12 hour (1-12) */ > +/* read and wrap 1-12 -> 0-11 */ > +now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u; > if (FIELD_EX32(data, HOUR, AMPM)) { > -tmp += 12; > +now.tm_hour += 12; > } > -if (tmp % 12 == 0) { > -tmp -= 12; > -} > -now.tm_hour = tmp; > + > } else { > now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24)); > } > -- > 2.11.0 > thanks -- PMM
[Qemu-devel] [PATCH 04/14] timer: ds1338 clarify HOUR handling
Simplify and comment the translation between registers and struct tm. Signed-off-by: Michael Davidsaver--- hw/timer/ds1338.c | 35 +-- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c index 72a4692d60..9bcda26e51 100644 --- a/hw/timer/ds1338.c +++ b/hw/timer/ds1338.c @@ -88,23 +88,23 @@ static void capture_current_time(DS1338State *s) * which will be actually read by the data transfer operation. */ struct tm now; +bool mode12 = ARRAY_FIELD_EX32(s->nvram, HOUR, SET12); qemu_get_timedate(, s->offset); + s->nvram[R_SEC] = to_bcd(now.tm_sec); s->nvram[R_MIN] = to_bcd(now.tm_min); -if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) { -int tmp = now.tm_hour; -if (tmp % 12 == 0) { -tmp += 12; -} -s->nvram[R_HOUR] = 0; +s->nvram[R_HOUR] = 0; +if (mode12) { +/* map 0-23 to 1-12 am/pm */ ARRAY_FIELD_DP32(s->nvram, HOUR, SET12, 1); -if (tmp <= 12) { -ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, 0); -ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(tmp)); -} else { -ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, 1); -ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(tmp - 12)); +ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, now.tm_hour >= 12u); +now.tm_hour %= 12u; /* wrap 0-23 to 0-11 */ +if (now.tm_hour == 0u) { +/* midnight/noon stored as 12 */ +now.tm_hour = 12u; } +ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(now.tm_hour)); + } else { ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR24, to_bcd(now.tm_hour)); } @@ -182,14 +182,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data) break; case R_HOUR: if (FIELD_EX32(data, HOUR, SET12)) { -int tmp = from_bcd(FIELD_EX32(data, HOUR, HOUR12)); +/* 12 hour (1-12) */ +/* read and wrap 1-12 -> 0-11 */ +now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u; if (FIELD_EX32(data, HOUR, AMPM)) { -tmp += 12; +now.tm_hour += 12; } -if (tmp % 12 == 0) { -tmp -= 12; -} -now.tm_hour = tmp; + } else { now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24)); } -- 2.11.0