Re: [Qemu-devel] [PATCH 11/14] timer: generalize ds1338

[Peter Maydell]

On 24 March 2018 at 19:24, Michael Davidsaver <address@hidden> wrote:
> Add class level handling for address space size
> and control register.
>
> Signed-off-by: Michael Davidsaver <address@hidden>
> ---

> @@ -206,22 +216,15 @@ static void dsrtc_update(DSRTCState *s)
>  static int dsrtc_send(I2CSlave *i2c, uint8_t data)
>  {
>      DSRTCState *s = DSRTC(i2c);
> +    DSRTCClass *k = DSRTC_GET_CLASS(s);
>
>      if (s->addr_byte) {
> -        s->ptr = data & (NVRAM_SIZE - 1);
> +        s->ptr = data % k->addr_size;
>          s->addr_byte = false;
>          return 0;
>      }
> -    if (s->ptr == R_CTRL) {
> -        /* Control register. */
> -
> -        /* Ensure bits 2, 3 and 6 will read back as zero. */
> -        data &= 0xB3;
> -
> -        /* Attempting to write the OSF flag to logic 1 leaves the
> -           value unchanged. */
> -        data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
> -
> +    if (s->ptr == k->ctrl_offset) {
> +        (k->ctrl_write)(s, data);
>      }
>      s->nvram[s->ptr] = data;
>      if (s->ptr <= R_YEAR) {
> @@ -256,15 +259,41 @@ static void dsrtc_class_init(ObjectClass *klass, void 
> *data)
>  }
>
>  static const TypeInfo dsrtc_info = {
> +    .abstract      = true,
>      .name          = TYPE_DSRTC,
>      .parent        = TYPE_I2C_SLAVE,
>      .instance_size = sizeof(DSRTCState),
>      .class_init    = dsrtc_class_init,
>  };
>
> +static void ds1338_control_write(DSRTCState *s, uint8_t data)
> +{
> +    /* Control register. */
> +
> +    /* allow guest to set no-op controls for clock out pin */
> +    s->nvram[R_DS1338_CTRL] = data & 0x93;

This is mixing a behavioural change with an otherwise
pure refactoring -- can you split them out, please?

Otherwise the patch looks good.

thanks
-- PMM