On 24 March 2018 at 19:24, Michael Davidsaver <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
thanks
-- PMM
