Re: [Qemu-devel] [PATCH 02/14] timer: ds1338 use registerfields.h

[Peter Maydell]

On 24 March 2018 at 19:24, Michael Davidsaver <address@hidden> 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 <address@hidden>

> ---
>  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 <address@hidden>

thanks
-- PMM