Re: [Qemu-devel] [PATCH 2/4] hw/ds1338.c: Minor fixes

[Peter Maydell]

On 2 December 2012 17:17, Antoine Mathys <address@hidden> wrote:
> Minor fixes in the handling of the RTC registers.
>
> Signed-off-by: Antoine Mathys <address@hidden>
> ---
>  hw/ds1338.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ds1338.c b/hw/ds1338.c
> index 1274b22..1fb152e 100644
> --- a/hw/ds1338.c
> +++ b/hw/ds1338.c
> @@ -62,9 +62,9 @@ static void capture_current_time(DS1338State *s)
>      } else {
>          s->nvram[2] = to_bcd(now.tm_hour);
>      }
> -    s->nvram[3] = to_bcd(now.tm_wday) + 1;
> +    s->nvram[3] = to_bcd(now.tm_wday + 1);
>      s->nvram[4] = to_bcd(now.tm_mday);
> -    s->nvram[5] = to_bcd(now.tm_mon) + 1;
> +    s->nvram[5] = to_bcd(now.tm_mon + 1);
>      s->nvram[6] = to_bcd(now.tm_year - 100);
>  }

Yep, doing arithmetic after to_bcd() is pretty much always
a bug.

>
> @@ -119,7 +119,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
>          s->addr_byte = false;
>          return 0;
>      }
> -    if (s->ptr < 8) {
> +    if (s->ptr < 7) {
> +        /* Time register. */
>          struct tm now;
>          qemu_get_timedate(&now, s->offset);
>          switch(s->ptr) {
> @@ -145,7 +146,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
>              }
>              break;
>          case 3:
> -            now.tm_wday = from_bcd(data & 7) - 1;
> +            now.tm_wday = from_bcd(data & 0x07) - 1;

Why bother changing this?

>              break;
>          case 4:
>              now.tm_mday = from_bcd(data & 0x3f);
> @@ -156,11 +157,11 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
>          case 6:
>              now.tm_year = from_bcd(data) + 100;
>              break;
> -        case 7:
> -            /* Control register. Currently ignored.  */
> -            break;
>          }
>          s->offset = qemu_timedate_diff(&now);
> +    } else if (s->ptr == 7) {
> +        /* Control register. Currently ignored.  */
> +        s->nvram[s->ptr] = data;

The comment says ignored but the code isn't doing that: this
change will make it read-as-written. I think you should put
this movement of the control register out of the switch()
in the next patch, not this one. That would leave this patch
with just the to_bcd() fixes, and you could give it a more
precise commit message then.

>      } else {
>          s->nvram[s->ptr] = data;
>      }
> --
> 1.7.10.4
>

-- PMM