On 24 March 2018 at 19:24, Michael Davidsaver <mdavidsa...@gmail.com> wrote: > Use names for registers and bits except > for R_CTRL which will be dealt with later, > and isn't modeled anyway. > > Signed-off-by: Michael Davidsaver <mdavidsa...@gmail.com>
> --- > hw/timer/ds1338.c | 84 > ++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 58 insertions(+), 26 deletions(-) > @@ -61,25 +89,29 @@ static void capture_current_time(DS1338State *s) > */ > struct tm now; > qemu_get_timedate(&now, s->offset); > - s->nvram[0] = to_bcd(now.tm_sec); > - s->nvram[1] = to_bcd(now.tm_min); > - if (s->nvram[2] & HOURS_12) { > + 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; > + ARRAY_FIELD_DP32(s->nvram, HOUR, SET12, 1); > if (tmp <= 12) { > - s->nvram[2] = HOURS_12 | to_bcd(tmp); > + ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, 0); This is zeroing a bit that's already zero. > + ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(tmp)); > } else { > - s->nvram[2] = HOURS_12 | HOURS_PM | to_bcd(tmp - 12); > + ARRAY_FIELD_DP32(s->nvram, HOUR, AMPM, 1); > + ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR12, to_bcd(tmp - 12)); > } > } else { > - s->nvram[2] = to_bcd(now.tm_hour); > + ARRAY_FIELD_DP32(s->nvram, HOUR, HOUR24, to_bcd(now.tm_hour)); This else clause used to definitely set all the bits in nvram[2], and now it doesn't (the s->nvram[R_HOUR] = 0 is only in the if {}). For cases like this where we're setting the whole thing, I think s->nvram[R_HOUR] = R_HOUR_SET12_MASK | to_bcd(tmp); s->nvram[R_HOUR] = R_HOUR_SET12_MASK | R_HOUR_AMPM_MASK | to_bcd(tmp - 12); s->nvram[R_HOUR] = R_HOUR_HOUR24_MASK | to_bcm(now.tm_hour); is clearer than individually setting each field, but you can do the "clear and then set individual fields" approach if you like. Otherwise Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> thanks -- PMM